refactor(BaseDocument)!: improved `get`, `set` and `extend` methods (#16540)

* perf!: 80% faster doc.get for fields with `None` as value

* perf: quicker init child (#3)

* refactor: avoid repitition and improve error message

* test: `doc.extend`

* fix: improve constant naming

* fix: minor improvements and tests

* refactor: improve naming

frappe/core/doctype/doctype/doctype.py

@@ -1018,12 +1018,13 @@ def validate_fields(meta):
 		validate_column_name(fieldname)
 
 	def check_invalid_fieldnames(docname, fieldname):
-		invalid_fields = ("doctype",)
-		if fieldname in invalid_fields:
+		if fieldname in Document._reserved_keywords:
 			frappe.throw(
-				_("{0}: Fieldname cannot be one of {1}").format(
-					docname, ", ".join(frappe.bold(d) for d in invalid_fields)
-				)
+				_("{0}: fieldname cannot be set to reserved keyword {1}").format(
+					frappe.bold(docname),
+					frappe.bold(fieldname),
+				),
+				title=_("Invalid Fieldname"),
 			)
 
 	def check_unique_fieldname(docname, fieldname):

frappe/desk/form/meta.py

@@ -55,12 +55,6 @@ class FormMeta(Meta):
 		super(FormMeta, self).__init__(doctype)
 		self.load_assets()
 
-	def set(self, key, value, *args, **kwargs):
-		if key in ASSET_KEYS:
-			self.__dict__[key] = value
-		else:
-			super(FormMeta, self).set(key, value, *args, **kwargs)
-
 	def load_assets(self):
 		if self.get("__assets_loaded", False):
 			return

frappe/model/base_document.py

@@ -5,7 +5,7 @@ import json
 from typing import Dict, List
 
 import frappe
-from frappe import _
+from frappe import _, _dict
 from frappe.model import (
 	child_table_fields,
 	datetime_fields,
@@ -23,7 +23,16 @@ from frappe.utils.html_utils import unescape_html
 
 max_positive_value = {"smallint": 2**15, "int": 2**31, "bigint": 2**63}
 
-DOCTYPES_FOR_DOCTYPE = ("DocType", "DocField", "DocPerm", "DocType Action", "DocType Link")
+DOCTYPE_TABLE_FIELDS = [
+	_dict(fieldname="fields", options="DocField"),
+	_dict(fieldname="permissions", options="DocPerm"),
+	_dict(fieldname="actions", options="DocType Action"),
+	_dict(fieldname="links", options="DocType Link"),
+	_dict(fieldname="states", options="DocType State"),
+]
+
+TABLE_DOCTYPES_FOR_DOCTYPE = {df["fieldname"]: df["options"] for df in DOCTYPE_TABLE_FIELDS}
+DOCTYPES_FOR_DOCTYPE = {"DocType", *TABLE_DOCTYPES_FOR_DOCTYPE.values()}
 
 
 def get_controller(doctype):
@@ -78,12 +87,28 @@ def get_controller(doctype):
 
 
 class BaseDocument(object):
-	ignore_in_setter = ("doctype", "_meta", "meta", "_table_fields", "_valid_columns")
+	_reserved_keywords = {
+		"doctype",
+		"meta",
+		"_meta",
+		"flags",
+		"_table_fields",
+		"_valid_columns",
+		"_table_fieldnames",
+		"_reserved_keywords",
+		"dont_update_if_missing",
+	}
 
 	def __init__(self, d):
 		if d.get("doctype"):
 			self.doctype = d["doctype"]
 
+		self._table_fieldnames = (
+			d["_table_fieldnames"]  # from cache
+			if "_table_fieldnames" in d
+			else {df.fieldname for df in self._get_table_fields()}
+		)
+
 		self.update(d)
 		self.dont_update_if_missing = []
 
@@ -92,10 +117,10 @@ class BaseDocument(object):
 
 	@property
 	def meta(self):
-		if not getattr(self, "_meta", None):
-			self._meta = frappe.get_meta(self.doctype)
+		if not (meta := getattr(self, "_meta", None)):
+			self._meta = meta = frappe.get_meta(self.doctype)
 
-		return self._meta
+		return meta
 
 	def __getstate__(self):
 		self._meta = None
@@ -144,17 +169,12 @@ class BaseDocument(object):
 
 		if filters:
 			if isinstance(filters, dict):
-				value = _filter(self.__dict__.get(key, []), filters, limit=limit)
-			else:
-				default = filters
-				filters = None
-				value = self.__dict__.get(key, default)
-		else:
-			value = self.__dict__.get(key, default)
+				return _filter(self.__dict__.get(key, []), filters, limit=limit)
 
-		if value is None and key in (d.fieldname for d in self.meta.get_table_fields()):
-			value = []
-			self.set(key, value)
+			# perhaps you wanted to set a default instead
+			default = filters
+
+		value = self.__dict__.get(key, default)
 
 		if limit and isinstance(value, (list, tuple)) and len(value) > limit:
 			value = value[:limit]
@@ -165,14 +185,19 @@ class BaseDocument(object):
 		return self.get(key, filters=filters, limit=1)[0]
 
 	def set(self, key, value, as_value=False):
-		if key in self.ignore_in_setter:
+		if key in self._reserved_keywords:
 			return
 
-		if isinstance(value, list) and not as_value:
+		if not as_value and key in self._table_fieldnames:
 			self.__dict__[key] = []
-			self.extend(key, value)
-		else:
-			self.__dict__[key] = value
+
+			# if value is falsy, just init to an empty list
+			if value:
+				self.extend(key, value)
+
+			return
+
+		self.__dict__[key] = value
 
 	def delete_key(self, key):
 		if key in self.__dict__:
@@ -190,41 +215,27 @@ class BaseDocument(object):
 		"""
 		if value is None:
 			value = {}
-		if isinstance(value, (dict, BaseDocument)):
-			if not self.__dict__.get(key):
-				self.__dict__[key] = []
 
-			value = self._init_child(value, key)
-			self.__dict__[key].append(value)
+		if (table := self.__dict__.get(key)) is None:
+			self.__dict__[key] = table = []
 
-			# reference parent document
-			value.parent_doc = self
+		value = self._init_child(value, key)
+		table.append(value)
 
-			return value
-		else:
+		# reference parent document
+		value.parent_doc = self
 
-			# metaclasses may have arbitrary lists
-			# which we can ignore
-			if getattr(self, "_metaclass", None) or self.__class__.__name__ in (
-				"Meta",
-				"FormMeta",
-				"DocField",
-			):
-				return value
-
-			raise ValueError(
-				'Document for field "{0}" attached to child table of "{1}" must be a dict or BaseDocument, not {2} ({3})'.format(
-					key, self.name, str(type(value))[1:-1], value
-				)
-			)
+		return value
 
 	def extend(self, key, value):
-		if isinstance(value, list):
-			for v in value:
-				self.append(key, v)
-		else:
+		try:
+			value = iter(value)
+		except TypeError:
 			raise ValueError
 
+		for v in value:
+			self.append(key, v)
+
 	def remove(self, doc):
 		# Usage: from the parent doc, pass the child table doc
 		# to remove that child doc from the child table, thus removing it from the parent doc
@@ -232,15 +243,12 @@ class BaseDocument(object):
 			self.get(doc.parentfield).remove(doc)
 
 	def _init_child(self, value, key):
-		if not self.doctype:
-			return value
-
 		if not isinstance(value, BaseDocument):
-			value["doctype"] = self.get_table_field_doctype(key)
-			if not value["doctype"]:
+			if not (doctype := self.get_table_field_doctype(key)):
 				raise AttributeError(key)
 
-			value = get_controller(value["doctype"])(value)
+			value["doctype"] = doctype
+			value = get_controller(doctype)(value)
 
 		value.parent = self.name
 		value.parenttype = self.doctype
@@ -250,17 +258,35 @@ class BaseDocument(object):
 			value.docstatus = DocStatus.draft()
 
 		if not getattr(value, "idx", None):
-			value.idx = len(self.get(key) or []) + 1
+			if table := getattr(self, key, None):
+				value.idx = len(table) + 1
+			else:
+				value.idx = 1
 
 		if not getattr(value, "name", None):
 			value.__dict__["__islocal"] = 1
 
 		return value
 
+	def _get_table_fields(self):
+		"""
+		To get table fields during Document init
+		Meta.get_table_fields goes into recursion for special doctypes
+		"""
+
+		if self.doctype == "DocType":
+			return DOCTYPE_TABLE_FIELDS
+
+		# child tables don't have child tables
+		if self.doctype in DOCTYPES_FOR_DOCTYPE or getattr(self, "parentfield", None):
+			return ()
+
+		return self.meta.get_table_fields()
+
 	def get_valid_dict(
 		self, sanitize=True, convert_dates_to_str=False, ignore_nulls=False, ignore_virtual=False
 	) -> Dict:
-		d = frappe._dict()
+		d = _dict()
 		for fieldname in self.meta.get_valid_columns():
 			# column is valid, we can use getattr
 			d[fieldname] = getattr(self, fieldname, None)
@@ -316,6 +342,16 @@ class BaseDocument(object):
 
 		return d
 
+	def init_child_tables(self):
+		"""
+		This is needed so that one can loop over child table properties
+		without worrying about whether or not they have values
+		"""
+
+		for fieldname in self._table_fieldnames:
+			if self.__dict__.get(fieldname) is None:
+				self.__dict__[fieldname] = []
+
 	def init_valid_columns(self):
 		for key in default_fields:
 			if key not in self.__dict__:
@@ -365,9 +401,9 @@ class BaseDocument(object):
 		doc = self.get_valid_dict(convert_dates_to_str=convert_dates_to_str, ignore_nulls=no_nulls)
 		doc["doctype"] = self.doctype
 
-		for df in self.meta.get_table_fields():
-			children = self.get(df.fieldname) or []
-			doc[df.fieldname] = [
+		for fieldname in self._table_fieldnames:
+			children = self.get(fieldname) or []
+			doc[fieldname] = [
 				d.as_dict(
 					convert_dates_to_str=convert_dates_to_str,
 					no_nulls=no_nulls,
@@ -407,10 +443,9 @@ class BaseDocument(object):
 		try:
 			return self.meta.get_field(fieldname).options
 		except AttributeError:
-			if self.doctype == "DocType":
-				return dict(links="DocType Link", actions="DocType Action", states="DocType State").get(
-					fieldname
-				)
+			if self.doctype == "DocType" and (table_doctype := TABLE_DOCTYPES_FOR_DOCTYPE.get(fieldname)):
+				return table_doctype
+
 			raise
 
 	def get_parentfield_of_doctype(self, doctype):
@@ -519,8 +554,8 @@ class BaseDocument(object):
 		"""Raw update parent + children
 		DOES NOT VALIDATE AND CALL TRIGGERS"""
 		self.db_update()
-		for df in self.meta.get_table_fields():
-			for doc in self.get(df.fieldname):
+		for fieldname in self._table_fieldnames:
+			for doc in self.get(fieldname):
 				doc.db_update()
 
 	def show_unique_validation_message(self, e):
@@ -632,7 +667,7 @@ class BaseDocument(object):
 		if self.meta.istable:
 			for fieldname in ("parent", "parenttype"):
 				if not self.get(fieldname):
-					missing.append((fieldname, get_msg(frappe._dict(label=fieldname))))
+					missing.append((fieldname, get_msg(_dict(label=fieldname))))
 
 		return missing
 
@@ -679,7 +714,7 @@ class BaseDocument(object):
 				if not frappe.get_meta(doctype).get("is_virtual"):
 					if not fields_to_fetch:
 						# cache a single value type
-						values = frappe._dict(name=frappe.db.get_value(doctype, docname, "name", cache=True))
+						values = _dict(name=frappe.db.get_value(doctype, docname, "name", cache=True))
 					else:
 						values_to_fetch = ["name"] + [_df.fetch_from.split(".")[-1] for _df in fields_to_fetch]
 
@@ -1009,10 +1044,10 @@ class BaseDocument(object):
 		cache_key = parentfield or "main"
 
 		if not hasattr(self, "_precision"):
-			self._precision = frappe._dict()
+			self._precision = _dict()
 
 		if cache_key not in self._precision:
-			self._precision[cache_key] = frappe._dict()
+			self._precision[cache_key] = _dict()
 
 		if fieldname not in self._precision[cache_key]:
 			self._precision[cache_key][fieldname] = None

frappe/model/document.py

@@ -113,6 +113,7 @@ class Document(BaseDocument):
 		if kwargs:
 			# init base document
 			super(Document, self).__init__(kwargs)
+			self.init_child_tables()
 			self.init_valid_columns()
 
 		else:
@@ -150,25 +151,19 @@ class Document(BaseDocument):
 
 			super(Document, self).__init__(d)
 
-		if self.name == "DocType" and self.doctype == "DocType":
-			from frappe.model.meta import DOCTYPE_TABLE_FIELDS
-
-			table_fields = DOCTYPE_TABLE_FIELDS
-		else:
-			table_fields = self.meta.get_table_fields()
-
-		for df in table_fields:
-			children = frappe.db.get_values(
-				df.options,
-				{"parent": self.name, "parenttype": self.doctype, "parentfield": df.fieldname},
-				"*",
-				as_dict=True,
-				order_by="idx asc",
+		for df in self._get_table_fields():
+			children = (
+				frappe.db.get_values(
+					df.options,
+					{"parent": self.name, "parenttype": self.doctype, "parentfield": df.fieldname},
+					"*",
+					as_dict=True,
+					order_by="idx asc",
+				)
+				or []
 			)
-			if children:
-				self.set(df.fieldname, children)
-			else:
-				self.set(df.fieldname, [])
+
+			self.set(df.fieldname, children)
 
 		# sometimes __setup__ can depend on child values, hence calling again at the end
 		if hasattr(self, "__setup__"):
@@ -897,8 +892,7 @@ class Document(BaseDocument):
 			if parenttype and df.options != parenttype:
 				continue
 
-			value = self.get(df.fieldname)
-			if isinstance(value, list):
+			if value := self.get(df.fieldname):
 				children.extend(value)
 
 		return children

frappe/model/meta.py

@@ -30,7 +30,11 @@ from frappe.model import (
 	optional_fields,
 	table_fields,
 )
-from frappe.model.base_document import BaseDocument
+from frappe.model.base_document import (
+	DOCTYPE_TABLE_FIELDS,
+	TABLE_DOCTYPES_FOR_DOCTYPE,
+	BaseDocument,
+)
 from frappe.model.document import Document
 from frappe.model.workflow import get_workflow_name
 from frappe.modules import load_doctype_module
@@ -148,9 +152,9 @@ class Meta(Document):
 					out[key] = value
 
 			# set empty lists for unset table fields
-			for table_field in DOCTYPE_TABLE_FIELDS:
-				if out.get(table_field.fieldname) is None:
-					out[table_field.fieldname] = []
+			for fieldname in TABLE_DOCTYPES_FOR_DOCTYPE.keys():
+				if out.get(fieldname) is None:
+					out[fieldname] = []
 
 			return out
 
@@ -225,13 +229,7 @@ class Meta(Document):
 		return self._valid_columns
 
 	def get_table_field_doctype(self, fieldname):
-		return {
-			"fields": "DocField",
-			"permissions": "DocPerm",
-			"actions": "DocType Action",
-			"links": "DocType Link",
-			"states": "DocType State",
-		}.get(fieldname)
+		return TABLE_DOCTYPES_FOR_DOCTYPE.get(fieldname)
 
 	def get_field(self, fieldname):
 		"""Return docfield from meta"""
@@ -642,14 +640,6 @@ class Meta(Document):
 		return self.has_field("lft") and self.has_field("rgt")
 
 
-DOCTYPE_TABLE_FIELDS = [
-	frappe._dict({"fieldname": "fields", "options": "DocField"}),
-	frappe._dict({"fieldname": "permissions", "options": "DocPerm"}),
-	frappe._dict({"fieldname": "actions", "options": "DocType Action"}),
-	frappe._dict({"fieldname": "links", "options": "DocType Link"}),
-	frappe._dict({"fieldname": "states", "options": "DocType State"}),
-]
-
 #######
 
 

frappe/tests/test_base_document.py

@@ -5,7 +5,7 @@ from frappe.model.base_document import BaseDocument
 
 class TestBaseDocument(unittest.TestCase):
 	def test_docstatus(self):
-		doc = BaseDocument({"docstatus": 0})
+		doc = BaseDocument({"docstatus": 0, "doctype": "ToDo"})
 		self.assertTrue(doc.docstatus.is_draft())
 		self.assertEqual(doc.docstatus, 0)
 

frappe/tests/test_document.py

@@ -341,3 +341,19 @@ class TestDocument(unittest.TestCase):
 
 		# run_method should get overridden
 		self.assertEqual(doc.run_method("as_dict"), "success")
+
+	def test_extend(self):
+		doc = frappe.get_last_doc("User")
+		self.assertRaises(ValueError, doc.extend, "user_emails", None)
+
+		# allow calling doc.extend with iterable objects
+		doc.extend("user_emails", ())
+		doc.extend("user_emails", [])
+		doc.extend("user_emails", (x for x in ()))
+
+	def test_set(self):
+		doc = frappe.get_last_doc("User")
+
+		# setting None should init a table field to empty list
+		doc.set("user_emails", None)
+		self.assertEqual(doc.user_emails, [])