From e2714c3e1ce0f11beab3d1a338e333a476d6fdc5 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 18 Sep 2023 15:50:31 +0530 Subject: [PATCH] feat: v2 error and debug log structure --- frappe/__init__.py | 12 +++++------ frappe/tests/test_api.py | 30 +++++++++++++++++++-------- frappe/utils/response.py | 45 ++++++++++++++++++++++++++++++++-------- 3 files changed, 63 insertions(+), 24 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 9addfc0785..0b9b340c8d 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -488,9 +488,12 @@ def msgprint( def _raise_exception(): if raise_exception: if inspect.isclass(raise_exception) and issubclass(raise_exception, Exception): - raise raise_exception(msg) + exc = raise_exception(msg) else: - raise ValidationError(msg) + exc = ValidationError(msg) + if out.__frappe_exc_id: + exc.__frappe_exc_id = out.__frappe_exc_id + raise exc if flags.mute_messages: _raise_exception() @@ -527,6 +530,7 @@ def msgprint( if raise_exception: out.raise_exception = 1 + out.__frappe_exc_id = generate_hash() if primary_action: out.primary_action = primary_action @@ -535,10 +539,6 @@ def msgprint( out.wide = wide message_log.append(out) - - if raise_exception and hasattr(raise_exception, "__name__"): - local.response["exc_type"] = raise_exception.__name__ - _raise_exception() diff --git a/frappe/tests/test_api.py b/frappe/tests/test_api.py index a39d6a07ba..3825121834 100644 --- a/frappe/tests/test_api.py +++ b/frappe/tests/test_api.py @@ -200,7 +200,7 @@ class TestResourceAPI(FrappeAPITestCase): self.assertIsInstance(json.data, list) self.assertIsInstance(json.data[0], list) - @parameterize("", "v1", "v2") + @parameterize("", "v1") def test_get_list_debug(self): # test 5: fetch response with debug with suppress_stdout(): @@ -253,12 +253,6 @@ class TestResourceAPI(FrappeAPITestCase): self.assertEqual(response.status_code, 404) self.GENERATED_DOCUMENTS.remove(doc_to_delete) - non_existent_doc = frappe.generate_hash(length=12) - with suppress_stdout(): - response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) - self.assertEqual(response.status_code, 404) - self.assertDictEqual(response.json, {}) - @parameterize("", "v1") def test_run_doc_method(self): # test 10: Run whitelisted method on doc via /api/resource @@ -372,6 +366,15 @@ class TestDocumentAPIV2(TestResourceAPI): response = self.get(self.resource_path(self.DOCTYPE, random_doc)) self.assertEqual(response.json["data"]["description"], generated_desc) + def test_delete_document_non_existing(self): + non_existent_doc = frappe.generate_hash(length=12) + with suppress_stdout(): + response = self.delete(self.resource_path(self.DOCTYPE, non_existent_doc)) + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json["errors"][0]["type"], "DoesNotExistError") + # 404s dont return exceptions + self.assertFalse(response.json["errors"][0].get("exception")) + class TestMethodAPIV2(FrappeAPITestCase): version = "v2" @@ -479,7 +482,7 @@ class TestMethodAPIV2(FrappeAPITestCase): response = self.get( self.method_path(method), {"sid": self.sid, "message": expected_message, "fail": True, "handled": False}, - ) + ).json self.assertIsInstance(response["errors"], list) self.assertEqual(response["errors"][0]["type"], "ZeroDivisionError") @@ -522,7 +525,7 @@ class TestReadOnlyMode(FrappeAPITestCase): self.assertIsInstance(response.json, dict) self.assertIsInstance(response.json["data"], list) - @parameterize("", "v1", "v2") + @parameterize("", "v1") def test_blocked_writes(self): with suppress_stdout(): response = self.post( @@ -531,6 +534,15 @@ class TestReadOnlyMode(FrappeAPITestCase): self.assertEqual(response.status_code, 503) self.assertEqual(response.json["exc_type"], "InReadOnlyMode") + @parameterize("v2") + def test_blocked_writes_v2(self): + with suppress_stdout(): + response = self.post( + self.resource_path("ToDo"), {"description": frappe.mock("paragraph"), "sid": self.sid} + ) + self.assertEqual(response.status_code, 503) + self.assertEqual(response.json["errors"][0]["type"], "InReadOnlyMode") + class TestWSGIApp(FrappeAPITestCase): def test_request_hooks(self): diff --git a/frappe/utils/response.py b/frappe/utils/response.py index e05be5994b..3fcf1a365e 100644 --- a/frappe/utils/response.py +++ b/frappe/utils/response.py @@ -6,6 +6,7 @@ import decimal import json import mimetypes import os +import sys from typing import TYPE_CHECKING, Literal from urllib.parse import quote @@ -29,22 +30,45 @@ if TYPE_CHECKING: def report_error(status_code): """Build error. Show traceback in developer mode""" - allow_traceback = frappe.get_system_settings("allow_error_traceback") if frappe.db else False - if ( - allow_traceback - and (status_code != 404 or frappe.conf.logging) + from frappe.api import ApiVersion, get_api_version + + allow_traceback = ( + (frappe.get_system_settings("allow_error_traceback") if frappe.db else False) and not frappe.local.flags.disable_traceback - ): - traceback = frappe.utils.get_traceback() - if traceback: - frappe.errprint(traceback) - frappe.local.response.exception = traceback.splitlines()[-1] + and (status_code != 404 or frappe.conf.logging) + ) + + traceback = frappe.utils.get_traceback() + exc_type, exc_value, _ = sys.exc_info() + + match get_api_version(): + case ApiVersion.V1: + if allow_traceback: + frappe.errprint(traceback) + frappe.response.exception = traceback.splitlines()[-1] + frappe.response["exc_type"] = exc_type.__name__ + case ApiVersion.V2: + error_log = {"type": exc_type.__name__} + if allow_traceback: + error_log["exception"] = traceback + _link_error_with_message_log(error_log, exc_value, frappe.message_log) + frappe.local.response.errors = [error_log] response = build_response("json") response.status_code = status_code return response +def _link_error_with_message_log(error_log, exception, message_logs): + for message in message_logs: + if message.get("__frappe_exc_id") == exception.__frappe_exc_id: + error_log.update(message) + message_logs.remove(message) + error_log.pop("raise_exception", None) + error_log.pop("__frappe_exc_id", None) + return + + def build_response(response_type=None): if "docs" in frappe.local.response and not frappe.local.response.docs: del frappe.local.response["docs"] @@ -164,6 +188,9 @@ def _make_logs_v2(): if frappe.local.message_log: response["messages"] = frappe.local.message_log + if frappe.debug_log and frappe.conf.get("logging"): + response["debug"] = [{"message": m} for m in frappe.local.debug_log] + def json_handler(obj): """serialize non-serializable data for json"""