Merge pull request #22110 from resilient-tech/perf-as_dict-2

perf: undo regression in `as_dict` performance

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})

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:
@@ -110,6 +120,7 @@ class BaseDocument:
 			"_doc_before_save",
 			"_table_fieldnames",
 			"_reserved_keywords",
+			"_permitted_fieldnames",
 			"dont_update_if_missing",
 		)
 	)
@@ -127,11 +138,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 +172,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 +336,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 +351,51 @@ 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)):
-						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)
 
-						d[fieldname] = 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
 
-				if isinstance(d[fieldname], list) and df.fieldtype not in table_fields:
+							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)))
 
 				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
 

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)

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")
 

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 "."