From 928bc46be3653c99c0254f74a8b99245a89f2cae Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 18 Aug 2023 17:44:45 +0530 Subject: [PATCH 1/4] perf: undo regression in `as_dict` performance --- frappe/model/base_document.py | 66 ++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index 8e5ecdc62c..edf74e3dca 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -110,6 +110,7 @@ class BaseDocument: "_doc_before_save", "_table_fieldnames", "_reserved_keywords", + "_permitted_fieldnames", "dont_update_if_missing", ) ) @@ -127,11 +128,22 @@ class BaseDocument: @property def meta(self): - if not (meta := getattr(self, "_meta", None)): + meta = getattr(self, "_meta", None) + if meta is None: self._meta = meta = frappe.get_meta(self.doctype) return meta + @property + def permitted_fieldnames(self): + permitted_fieldnames = getattr(self, "_permitted_fieldnames", None) + if permitted_fieldnames is None: + self._permitted_fieldnames = permitted_fieldnames = get_permitted_fields( + doctype=self.doctype, parenttype=getattr(self, "parenttype", None) + ) + + return permitted_fieldnames + def __getstate__(self): """ Called when pickling. @@ -150,6 +162,7 @@ class BaseDocument: """Remove unpicklable values before pickling""" state.pop("_meta", None) + state.pop("_permitted_fieldnames", None) def update(self, d): """Update multiple fields of a doctype using a dictionary of key-value pairs. @@ -313,18 +326,14 @@ class BaseDocument: self, sanitize=True, convert_dates_to_str=False, ignore_nulls=False, ignore_virtual=False ) -> _dict: d = _dict() - permitted_fields = get_permitted_fields( - doctype=self.doctype, parenttype=getattr(self, "parenttype", None) - ) + field_values = self.__dict__ for fieldname in self.meta.get_valid_columns(): - field_value = getattr(self, fieldname, None) - - # column is valid, we can use getattr - d[fieldname] = field_value + value = field_values.get(fieldname) # if no need for sanitization and value is None, continue - if not sanitize and d[fieldname] is None: + if not sanitize and value is None: + d[fieldname] = None continue df = self.meta.get_field(fieldname) @@ -332,46 +341,47 @@ class BaseDocument: if df: if is_virtual_field: - if ignore_virtual or fieldname not in permitted_fields: - del d[fieldname] + if ignore_virtual or fieldname not in self.permitted_fieldnames: continue - if d[fieldname] is None and (options := getattr(df, "options", None)): + if value is None and (options := getattr(df, "options", None)): from frappe.utils.safe_exec import get_safe_globals - d[fieldname] = frappe.safe_eval( + value = frappe.safe_eval( code=options, eval_globals=get_safe_globals(), eval_locals={"doc": self}, ) - if isinstance(d[fieldname], list) and df.fieldtype not in table_fields: + if isinstance(value, list) and df.fieldtype not in table_fields: frappe.throw(_("Value for {0} cannot be a list").format(_(df.label))) if df.fieldtype == "Check": - d[fieldname] = 1 if cint(d[fieldname]) else 0 + value = 1 if cint(value) else 0 - elif df.fieldtype == "Int" and not isinstance(d[fieldname], int): - d[fieldname] = cint(d[fieldname]) + elif df.fieldtype == "Int" and not isinstance(value, int): + value = cint(value) - elif df.fieldtype == "JSON" and isinstance(d[fieldname], dict): - d[fieldname] = json.dumps(d[fieldname], sort_keys=True, indent=4, separators=(",", ": ")) + elif df.fieldtype == "JSON" and isinstance(value, dict): + value = json.dumps(value, sort_keys=True, indent=4, separators=(",", ": ")) - elif df.fieldtype in float_like_fields and not isinstance(d[fieldname], float): - d[fieldname] = flt(d[fieldname]) + elif df.fieldtype in float_like_fields and not isinstance(value, float): + value = flt(value) - elif (df.fieldtype in datetime_fields and d[fieldname] == "") or ( - getattr(df, "unique", False) and cstr(d[fieldname]).strip() == "" + elif (df.fieldtype in datetime_fields and value == "") or ( + getattr(df, "unique", False) and cstr(value).strip() == "" ): - d[fieldname] = None + value = None if convert_dates_to_str and isinstance( - d[fieldname], (datetime.datetime, datetime.date, datetime.time, datetime.timedelta) + value, (datetime.datetime, datetime.date, datetime.time, datetime.timedelta) ): - d[fieldname] = str(d[fieldname]) + value = str(value) - if ignore_nulls and not is_virtual_field and d[fieldname] is None: - del d[fieldname] + if ignore_nulls and not is_virtual_field and value is None: + continue + + d[fieldname] = value return d From 7dc67f2feb64e17b855dbaf46b38b4c8e90e399a Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 18 Aug 2023 21:51:56 +0530 Subject: [PATCH 2/4] chore: add back `getattr` for virtual docfields which get value from a property --- frappe/core/doctype/doctype/doctype.py | 9 +------- frappe/model/base_document.py | 30 +++++++++++++++++++------- frappe/utils/__init__.py | 5 +++++ 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/frappe/core/doctype/doctype/doctype.py b/frappe/core/doctype/doctype/doctype.py index b90ae39506..d428373c05 100644 --- a/frappe/core/doctype/doctype/doctype.py +++ b/frappe/core/doctype/doctype/doctype.py @@ -33,7 +33,7 @@ from frappe.model.meta import Meta from frappe.modules import get_doc_path, make_boilerplate from frappe.modules.import_file import get_file_path from frappe.query_builder.functions import Concat -from frappe.utils import cint, flt, get_table_name, random_string +from frappe.utils import cint, flt, is_a_property, random_string from frappe.website.utils import clear_cache if TYPE_CHECKING: @@ -1820,13 +1820,6 @@ def make_module_and_roles(doc, perm_fieldname="permissions"): raise -def is_a_property(x) -> bool: - """Get properties (@property, @cached_property) in a controller class""" - from functools import cached_property - - return isinstance(x, (property, cached_property)) - - def check_fieldname_conflicts(docfield): """Checks if fieldname conflicts with methods or properties""" doc = frappe.get_doc({"doctype": docfield.dt}) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index edf74e3dca..59cdea8031 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -19,7 +19,17 @@ from frappe.model.docstatus import DocStatus from frappe.model.naming import set_new_name from frappe.model.utils.link_count import notify_link_count from frappe.modules import load_doctype_module -from frappe.utils import cast_fieldtype, cint, compare, cstr, flt, now, sanitize_html, strip_html +from frappe.utils import ( + cast_fieldtype, + cint, + compare, + cstr, + flt, + is_a_property, + now, + sanitize_html, + strip_html, +) from frappe.utils.html_utils import unescape_html if TYPE_CHECKING: @@ -344,14 +354,18 @@ class BaseDocument: if ignore_virtual or fieldname not in self.permitted_fieldnames: continue - if value is None and (options := getattr(df, "options", None)): - from frappe.utils.safe_exec import get_safe_globals + if value is None: + if (prop := getattr(type(self), fieldname, None)) and is_a_property(prop): + value = getattr(self, fieldname) - value = frappe.safe_eval( - code=options, - eval_globals=get_safe_globals(), - eval_locals={"doc": self}, - ) + elif options := getattr(df, "options", None): + from frappe.utils.safe_exec import get_safe_globals + + value = frappe.safe_eval( + code=options, + eval_globals=get_safe_globals(), + eval_locals={"doc": self}, + ) if isinstance(value, list) and df.fieldtype not in table_fields: frappe.throw(_("Value for {0} cannot be a list").format(_(df.label))) diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 4ad4c0536b..00f2a8726c 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -655,6 +655,11 @@ def is_markdown(text): return not NON_MD_HTML_PATTERN.search(text) +def is_a_property(x) -> bool: + """Get properties (@property, @cached_property) in a controller class""" + return isinstance(x, (property, functools.cached_property)) + + def get_sites(sites_path=None): if not sites_path: sites_path = getattr(frappe.local, "sites_path", None) or "." From 151874c03561a688ad0268379e6826330b9f5577 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 18 Aug 2023 22:47:37 +0530 Subject: [PATCH 3/4] test: ensure `get_permitted_fieldnames` is called only once per doc --- frappe/tests/test_perf.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/frappe/tests/test_perf.py b/frappe/tests/test_perf.py index ed9470d4d2..d4fc933571 100644 --- a/frappe/tests/test_perf.py +++ b/frappe/tests/test_perf.py @@ -17,6 +17,7 @@ query. This test can be written like this. """ import time +from unittest.mock import patch from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed @@ -54,6 +55,18 @@ class TestPerformance(FrappeTestCase): with self.assertQueryCount(0): frappe.get_meta("User") + def test_permitted_fieldnames(self): + frappe.clear_cache() + + doc = frappe.new_doc("Prepared Report") + # load permitted fieldnames once + doc.permitted_fieldnames + + with patch("frappe.model.base_document.get_permitted_fields") as mocked: + doc.as_dict() + # get_permitted_fields should not be called again + mocked.assert_not_called() + def test_set_value_query_count(self): frappe.db.set_value("User", "Administrator", "interest", "Nothing") From 91ceb889ccdd921a273cd97e0778b143249ffd5b Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 18 Aug 2023 23:04:22 +0530 Subject: [PATCH 4/4] test: ensure only property can be called for virtual docfields --- frappe/tests/test_document.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index cdd1d08c62..82d796d7f8 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -21,6 +21,11 @@ class CustomTestNote(Note): return now_datetime() - self.creation +class CustomNoteWithoutProperty(Note): + def age(self): + return now_datetime() - self.creation + + class TestDocument(FrappeTestCase): def test_get_return_empty_list_for_table_field_if_none(self): d = frappe.get_doc({"doctype": "User"}) @@ -303,8 +308,8 @@ class TestDocument(FrappeTestCase): note.title = frappe.generate_hash(length=20) note.insert() - def patch_note(): - return patch("frappe.controllers", new={frappe.local.site: {"Note": CustomTestNote}}) + def patch_note(class_=None): + return patch("frappe.controllers", new={frappe.local.site: {"Note": class_ or CustomTestNote}}) @contextmanager def customize_note(with_options=False): @@ -345,6 +350,14 @@ class TestDocument(FrappeTestCase): self.assertIsInstance(doc.as_dict().get("age"), timedelta) self.assertIsInstance(doc.get_valid_dict().get("age"), timedelta) + # has virtual field, but age method is not a property + with customize_note(), patch_note(class_=CustomNoteWithoutProperty): + doc = frappe.get_last_doc("Note") + self.assertIsInstance(doc, CustomNoteWithoutProperty) + self.assertNotIsInstance(type(doc).age, property) + self.assertIsNone(doc.as_dict().get("age")) + self.assertIsNone(doc.get_valid_dict().get("age")) + with customize_note(with_options=True): doc = frappe.get_last_doc("Note") self.assertIsInstance(doc, Note)