From 3358fdf9a99ae9ff57be9ecb4e310f9a1bd105ac Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 30 Mar 2022 13:30:02 +0530 Subject: [PATCH 01/25] refactor(nestedset): Using qb or db APIs inplace of raw SQLs --- frappe/utils/nestedset.py | 119 +++++++++++++++++++++----------------- 1 file changed, 65 insertions(+), 54 deletions(-) diff --git a/frappe/utils/nestedset.py b/frappe/utils/nestedset.py index 2517761c45..f1ebf8eeea 100644 --- a/frappe/utils/nestedset.py +++ b/frappe/utils/nestedset.py @@ -16,6 +16,9 @@ import frappe from frappe import _ from frappe.model.document import Document from frappe.query_builder import DocType, Order +from frappe.query_builder.functions import Coalesce, Max +from frappe.query_builder.utils import DocType + class NestedSetRecursionError(frappe.ValidationError): pass class NestedSetMultipleRootsError(frappe.ValidationError): pass @@ -51,87 +54,91 @@ def update_add_node(doc, parent, parent_field): """ insert a new node """ - doctype = doc.doctype name = doc.name + Table = DocType(doctype) # get the last sibling of the parent if parent: - left, right = frappe.db.sql("select lft, rgt from `tab{0}` where name=%s for update" - .format(doctype), parent)[0] + left, right = frappe.db.get_value(doctype, {"name": parent}, ["lft", "rgt"], for_update=True) validate_loop(doc.doctype, doc.name, left, right) else: # root - right = frappe.db.sql(""" - SELECT COALESCE(MAX(rgt), 0) + 1 FROM `tab{0}` - WHERE COALESCE(`{1}`, '') = '' - """.format(doctype, parent_field))[0][0] + + right = frappe.qb.from_(Table).select( + Coalesce(Max(Table.rgt), 0) + ).where(Coalesce(Table[parent_field], "") == "").run(pluck=True)[0] + right = right or 1 # update all on the right - frappe.db.sql("update `tab{0}` set rgt = rgt+2 where rgt >= %s" - .format(doctype), (right,)) - frappe.db.sql("update `tab{0}` set lft = lft+2 where lft >= %s" - .format(doctype), (right,)) + frappe.qb.update(Table).set(Table.rgt, Table.rgt + 2).where(Table.rgt >= right).run() + frappe.qb.update(Table).set(Table.lft, Table.lft + 2).where(Table.lft >= right).run() + + if frappe.qb.from_(Table).select("*").where((Table.lft == right) | (Table.rgt == right + 1)).run(): + frappe.throw(_("Nested set error. Please contact the Administrator.")) # update index of new node - if frappe.db.sql("select * from `tab{0}` where lft=%s or rgt=%s".format(doctype), (right, right+1)): - frappe.msgprint(_("Nested set error. Please contact the Administrator.")) - raise Exception - - frappe.db.sql("update `tab{0}` set lft=%s, rgt=%s where name=%s".format(doctype), - (right,right+1, name)) + frappe.qb.update(Table).set(Table.lft, right).set(Table.rgt, right + 1).where(Table.name == name).run() return right -def update_move_node(doc, parent_field): - parent = doc.get(parent_field) +def update_move_node(doc: Document, parent_field: str): + parent: str = doc.get(parent_field) + Table = DocType(doc.doctype) if parent: - new_parent = frappe.db.sql("""select lft, rgt from `tab{0}` - where name = %s for update""".format(doc.doctype), parent, as_dict=1)[0] + new_parent = frappe.qb.from_(Table).select( + Table.lft, Table.rgt + ).where(Table.name == parent).for_update().run(as_dict=True)[0] validate_loop(doc.doctype, doc.name, new_parent.lft, new_parent.rgt) # move to dark side - frappe.db.sql("""update `tab{0}` set lft = -lft, rgt = -rgt - where lft >= %s and rgt <= %s""".format(doc.doctype), (doc.lft, doc.rgt)) + frappe.qb.update(Table).set(Table.lft, - Table.lft).set(Table.rgt, - Table.rgt).where( + (Table.lft >= doc.lft) & (Table.rgt <= doc.rgt) + ).run() # shift left diff = doc.rgt - doc.lft + 1 - frappe.db.sql("""update `tab{0}` set lft = lft -%s, rgt = rgt - %s - where lft > %s""".format(doc.doctype), (diff, diff, doc.rgt)) + frappe.qb.update(Table).set(Table.lft, Table.lft - diff).set(Table.rgt, Table.rgt - diff).where( + Table.lft > doc.rgt + ).run() # shift left rgts of ancestors whose only rgts must shift - frappe.db.sql("""update `tab{0}` set rgt = rgt - %s - where lft < %s and rgt > %s""".format(doc.doctype), (diff, doc.lft, doc.rgt)) + frappe.qb.update(Table).set(Table.rgt, Table.rgt - diff).where( + (Table.lft < doc.lft) & (Table.rgt > doc.rgt) + ).run() if parent: - new_parent = frappe.db.sql("""select lft, rgt from `tab%s` - where name = %s for update""" % (doc.doctype, '%s'), parent, as_dict=1)[0] - # set parent lft, rgt - frappe.db.sql("""update `tab{0}` set rgt = rgt + %s - where name = %s""".format(doc.doctype), (diff, parent)) + frappe.qb.update(Table).set(Table.rgt, Table.rgt + diff).where(Table.name == parent).run() # shift right at new parent - frappe.db.sql("""update `tab{0}` set lft = lft + %s, rgt = rgt + %s - where lft > %s""".format(doc.doctype), (diff, diff, new_parent.rgt)) + frappe.qb.update(Table).set(Table.lft, Table.lft + diff).set(Table.rgt, Table.rgt + diff).where( + (Table.lft >= new_parent.lft) & (Table.lft <= new_parent.rgt) + ).run() + + frappe.qb.update(Table).set(Table.lft, Table.lft + diff).set(Table.rgt, Table.rgt + diff).where( + Table.lft > new_parent.rgt + ).run() # shift right rgts of ancestors whose only rgts must shift - frappe.db.sql("""update `tab{0}` set rgt = rgt + %s - where lft < %s and rgt > %s""".format(doc.doctype), - (diff, new_parent.lft, new_parent.rgt)) - + frappe.qb.update(Table).set(Table.rgt, Table.rgt + diff).where( + (Table.lft < new_parent.lft) & (Table.rgt > new_parent.rgt) + ).run() new_diff = new_parent.rgt - doc.lft else: # new root - max_rgt = frappe.db.sql("""select max(rgt) from `tab{0}`""".format(doc.doctype))[0][0] + max_rgt = frappe.qb.from_(Table).select(Max(Table.rgt)).run(pluck=True)[0] new_diff = max_rgt + 1 - doc.lft # bring back from dark side - frappe.db.sql("""update `tab{0}` set lft = -lft + %s, rgt = -rgt + %s - where lft < 0""".format(doc.doctype), (new_diff, new_diff)) + frappe.qb.update(Table).set( + Table.lft, -Table.lft + new_diff + ).set( + Table.rgt, -Table.rgt + new_diff + ).where(Table.lft < 0).run() @frappe.whitelist() @@ -197,10 +204,10 @@ def rebuild_node(doctype, parent, left, parent_field): def validate_loop(doctype, name, lft, rgt): """check if item not an ancestor (loop)""" - if name in frappe.db.sql_list("""select name from `tab{0}` where lft <= %s and rgt >= %s""" - .format(doctype), (lft, rgt)): + if name in frappe.get_all(doctype, filters={"lft": ["<=", lft], "rgt": [">=", rgt]}, pluck="name"): frappe.throw(_("Item cannot be added to its own descendents"), NestedSetRecursionError) + class NestedSet(Document): def __setup__(self): if self.meta.get("nsm_parent_field"): @@ -232,9 +239,7 @@ class NestedSet(Document): raise def validate_if_child_exists(self): - has_children = frappe.db.sql("""select count(name) from `tab{doctype}` - where `{nsm_parent_field}`=%s""".format(doctype=self.doctype, nsm_parent_field=self.nsm_parent_field), - (self.name,))[0][0] + has_children = frappe.db.count(self.doctype, filters={self.nsm_parent_field: self.name}) if has_children: frappe.throw(_("Cannot delete {0} as it has child nodes").format(self.name), NestedSetChildExistsError) @@ -251,8 +256,7 @@ class NestedSet(Document): parent_field = self.nsm_parent_field # set old_parent for children - frappe.db.sql("update `tab{0}` set old_parent=%s where {1}=%s" - .format(self.doctype, parent_field), (newdn, newdn)) + frappe.db.set_value(self.doctype, {"old_parent": newdn}, {parent_field: newdn}, update_modified=False, for_update=False) if merge: rebuild_tree(self.doctype, parent_field) @@ -269,8 +273,7 @@ class NestedSet(Document): def validate_ledger(self, group_identifier="is_group"): if hasattr(self, group_identifier) and not bool(self.get(group_identifier)): - if frappe.db.sql("""select name from `tab{0}` where {1}=%s and docstatus!=2""" - .format(self.doctype, self.nsm_parent_field), (self.name)): + if frappe.get_all(self.doctype, {self.nsm_parent_field: self.name, "docstatus": ("!=", 2)}): frappe.throw(_("{0} {1} cannot be a leaf node as it has children").format(_(self.doctype), self.name)) def get_ancestors(self): @@ -291,10 +294,18 @@ class NestedSet(Document): def get_root_of(doctype): """Get root element of a DocType with a tree structure""" - result = frappe.db.sql("""select t1.name from `tab{0}` t1 where - (select count(*) from `tab{1}` t2 where - t2.lft < t1.lft and t2.rgt > t1.rgt) = 0 - and t1.rgt > t1.lft""".format(doctype, doctype)) + from frappe.query_builder.functions import Count + Table = DocType(doctype) + t1 = Table.as_("t1") + t2 = Table.as_("t2") + + subq = frappe.qb.from_(t2).select(Count("*")).where( + (t2.lft < t1.lft) & (t2.rgt > t1.rgt) + ) + result = frappe.qb.from_(t1).select(t1.name).where( + (subq == 0) & (t1.rgt > t1.lft) # depends on https://github.com/frappe/frappe/pull/16107 + ).run() + return result[0][0] if result else None def get_ancestors_of(doctype, name, order_by="lft desc", limit=None): From 459c179916cc633f5b6dc948437e1deaba3f9a94 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 30 Mar 2022 13:31:01 +0530 Subject: [PATCH 02/25] refactor(utils): Use qb & db APIs inplace of raw SQls --- frappe/utils/error.py | 8 ++++++-- frappe/utils/install.py | 2 +- frappe/utils/user.py | 10 +++++----- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/frappe/utils/error.py b/frappe/utils/error.py index c38b320d98..ba0fbf1605 100644 --- a/frappe/utils/error.py +++ b/frappe/utils/error.py @@ -176,9 +176,13 @@ def collect_error_snapshots(): def clear_old_snapshots(): """Clear snapshots that are older than a month""" + from frappe.query_builder import DocType, Interval + from frappe.query_builder.functions import Now - frappe.db.sql("""delete from `tabError Snapshot` - where creation < (NOW() - INTERVAL '1' MONTH)""") + ErrorSnapshot = DocType("Error Snapshot") + frappe.db.delete(ErrorSnapshot, filters=( + ErrorSnapshot.creation < (Now() - Interval(months=1)) + )) path = get_error_snapshot_path() today = datetime.datetime.now() diff --git a/frappe/utils/install.py b/frappe/utils/install.py index ac26a98eb6..d197304c98 100644 --- a/frappe/utils/install.py +++ b/frappe/utils/install.py @@ -34,7 +34,7 @@ def after_install(): print_settings.save() # all roles to admin - frappe.get_doc("User", "Administrator").add_roles(*frappe.db.sql_list("""select name from tabRole""")) + frappe.get_doc("User", "Administrator").add_roles(*frappe.get_all("Role", pluck="name")) # update admin password update_password("Administrator", get_admin_password()) diff --git a/frappe/utils/user.py b/frappe/utils/user.py index ca7a555c72..c84b29b1c9 100644 --- a/frappe/utils/user.py +++ b/frappe/utils/user.py @@ -64,14 +64,14 @@ class UserPermissions: def build_doctype_map(self): """build map of special doctype properties""" + self.doctype_map = {} active_domains = frappe.get_active_domains() + all_doctypes = frappe.get_all("DocType", fields=["name", "in_create", "module", "istable", "issingle", "read_only", "restrict_to_domain"]) - self.doctype_map = {} - for r in frappe.db.sql("""select name, in_create, issingle, istable, - read_only, restrict_to_domain, module from tabDocType""", as_dict=1): - if (not r.restrict_to_domain) or (r.restrict_to_domain in active_domains): - self.doctype_map[r['name']] = r + for dt in all_doctypes: + if not dt.restrict_to_domain or (dt.restrict_to_domain in active_domains): + self.doctype_map[dt["name"]] = dt def build_perm_map(self): """build map of permissions at level 0""" From 10e4ed7c1e44c69697391fbd85dbd03aebee158b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 13:38:32 +0530 Subject: [PATCH 03/25] fix(query): Add human friendly operator oprions --- frappe/database/query.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index 15ab85ff56..a8a1271e70 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -115,21 +115,23 @@ def change_orderby(order: str): OPERATOR_MAP = { - "+": operator.add, - "=": operator.eq, - "-": operator.sub, - "!=": operator.ne, - "<": operator.lt, - ">": operator.gt, - "<=": operator.le, - ">=": operator.ge, - "in": func_in, - "not in": func_not_in, - "like": like, - "not like": not_like, - "regex": func_regex, - "between": func_between - } + "+": operator.add, + "=": operator.eq, + "-": operator.sub, + "!=": operator.ne, + "<": operator.lt, + ">": operator.gt, + "<=": operator.le, + "=>": operator.le, + ">=": operator.ge, + "=>": operator.ge, + "in": func_in, + "not in": func_not_in, + "like": like, + "not like": not_like, + "regex": func_regex, + "between": func_between, +} class Query: @@ -211,8 +213,7 @@ class Query: _operator = OPERATOR_MAP[f[1]] conditions = conditions.where(_operator(Field(f[0]), f[2])) - conditions = self.add_conditions(conditions, **kwargs) - return conditions + return self.add_conditions(conditions, **kwargs) def dict_query(self, table: str, filters: Dict[str, Union[str, int]] = None, **kwargs) -> frappe.qb: """Build conditions using the given dictionary filters @@ -251,8 +252,7 @@ class Query: field = getattr(_table, key) conditions = conditions.where(field.isnull()) - conditions = self.add_conditions(conditions, **kwargs) - return conditions + return self.add_conditions(conditions, **kwargs) def build_conditions( self, From 71d3f1c0d1962fe3d09cee4d40713c658e1ee55b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 13:39:06 +0530 Subject: [PATCH 04/25] feat(minor): Add DateFormat function util for qb --- frappe/query_builder/functions.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index 4cbdb96281..b0292f2728 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -43,6 +43,10 @@ CombineDatetime = ImportMapper( } ) +DateFormat = ImportMapper({ + db_type_is.MARIADB: CustomFunction("DATE_FORMAT", ["date", "format"]), + db_type_is.POSTGRES: ToChar, +}) class Cast_(Function): def __init__(self, value, as_type, alias=None): From 2d806b5d6d0239fab096be351b0accb92f254ab9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 13:40:09 +0530 Subject: [PATCH 05/25] fix: Ported subqry to branch for compatibility Via https://github.com/frappe/frappe/pull/16107 --- frappe/query_builder/terms.py | 15 +++++++++++++-- frappe/utils/nestedset.py | 4 +++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/frappe/query_builder/terms.py b/frappe/query_builder/terms.py index 205f1f9dcd..d3785e049a 100644 --- a/frappe/query_builder/terms.py +++ b/frappe/query_builder/terms.py @@ -1,10 +1,12 @@ from datetime import timedelta from typing import Any, Dict, Optional -from frappe.utils.data import format_timedelta -from pypika.terms import Function, ValueWrapper +from pypika.queries import QueryBuilder +from pypika.terms import Criterion, Function, ValueWrapper from pypika.utils import format_alias_sql +from frappe.utils.data import format_timedelta + class NamedParameterWrapper: """Utility class to hold parameter values and keys""" @@ -100,3 +102,12 @@ class ParameterizedFunction(Function): ) return function_sql + +class subqry(Criterion): + def __init__(self, subq: QueryBuilder, alias: Optional[str] = None,) -> None: + super().__init__(alias) + self.subq = subq + + def get_sql(self, **kwg: Any) -> str: + kwg["subquery"] = True + return self.subq.get_sql(**kwg) diff --git a/frappe/utils/nestedset.py b/frappe/utils/nestedset.py index f1ebf8eeea..1b6bd3e66d 100644 --- a/frappe/utils/nestedset.py +++ b/frappe/utils/nestedset.py @@ -295,6 +295,8 @@ class NestedSet(Document): def get_root_of(doctype): """Get root element of a DocType with a tree structure""" from frappe.query_builder.functions import Count + from frappe.query_builder.terms import subqry + Table = DocType(doctype) t1 = Table.as_("t1") t2 = Table.as_("t2") @@ -303,7 +305,7 @@ def get_root_of(doctype): (t2.lft < t1.lft) & (t2.rgt > t1.rgt) ) result = frappe.qb.from_(t1).select(t1.name).where( - (subq == 0) & (t1.rgt > t1.lft) # depends on https://github.com/frappe/frappe/pull/16107 + (subqry(subq) == 0) & (t1.rgt > t1.lft) ).run() return result[0][0] if result else None From cb3bcb57030b0ba1f5af70b55e3920b2cb3a5804 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 13:41:31 +0530 Subject: [PATCH 06/25] fix: Update Goal utils' tests --- frappe/tests/test_goal.py | 60 +++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 22 deletions(-) diff --git a/frappe/tests/test_goal.py b/frappe/tests/test_goal.py index 428855ade5..09bc6baedb 100644 --- a/frappe/tests/test_goal.py +++ b/frappe/tests/test_goal.py @@ -1,33 +1,49 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import unittest import frappe - -from frappe.utils.goal import get_monthly_results, get_monthly_goal_graph_data from frappe.test_runner import make_test_objects -import frappe.utils +from frappe.utils import format_date, today +from frappe.utils.goal import get_monthly_goal_graph_data, get_monthly_results +from frappe.tests.utils import FrappeTestCase -class TestGoal(unittest.TestCase): - def setUp(self): - make_test_objects('Event', reset=True) - def tearDown(self): - frappe.db.delete("Event") - # make_test_objects('Event', reset=True) - frappe.db.commit() +class TestGoal(FrappeTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + make_test_objects("Event", reset=True) def test_get_monthly_results(self): - '''Test monthly aggregation values of a field''' - result_dict = get_monthly_results('Event', 'subject', 'creation', "event_type='Private'", 'count') + """Test monthly aggregation values of a field""" + result_dict = get_monthly_results( + "Event", + "subject", + "creation", + filters={"event_type": "Private"}, + aggregation="count", + ) - from frappe.utils import today, formatdate - self.assertEqual(result_dict.get(formatdate(today(), "MM-yyyy")), 2) + self.assertEqual(result_dict.get(format_date(today(), "MM-yyyy")), 2) def test_get_monthly_goal_graph_data(self): - '''Test for accurate values in graph data (based on test_get_monthly_results)''' - docname = frappe.get_list('Event', filters = {"subject": ["=", "_Test Event 1"]})[0]["name"] - frappe.db.set_value('Event', docname, 'description', 1) - data = get_monthly_goal_graph_data('Test', 'Event', docname, 'description', 'description', 'description', - 'Event', '', 'description', 'creation', "starts_on = '2014-01-01'", 'count') - self.assertEqual(float(data['data']['datasets'][0]['values'][-1]), 1) + """Test for accurate values in graph data (based on test_get_monthly_results)""" + docname = frappe.get_list("Event", filters={"subject": ["=", "_Test Event 1"]})[ + 0 + ]["name"] + frappe.db.set_value("Event", docname, "description", 1) + data = get_monthly_goal_graph_data( + "Test", + "Event", + docname, + "description", + "description", + "description", + "Event", + "", + "description", + "creation", + filters={"starts_on": "2014-01-01"}, + aggregation="count", + ) + self.assertEqual(float(data["data"]["datasets"][0]["values"][-1]), 1) From b4485665ad1c325c4f3935a99f7bdf28cc21acb0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 13:42:13 +0530 Subject: [PATCH 07/25] refactor: frappe.utils.goal * Deprecate filter_str in whitelisted get_monthly_goal_graph_data API; use filter Dict instead * Simplify logic * Use qb notation instead of raw SQLs * Raise 403 if user does not have access to document Note: This includes a security fix --- frappe/utils/goal.py | 232 +++++++++++++++++++++---------------------- 1 file changed, 112 insertions(+), 120 deletions(-) diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index 2d7e73eb1a..97624e6272 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -1,157 +1,149 @@ -# Copyright (c) 2021, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE +from typing import Dict, Optional + import frappe from frappe import _ +from frappe.query_builder.functions import DateFormat, Function +from frappe.query_builder.utils import DocType +from frappe.utils.data import add_to_date, flt, now_datetime +from frappe.utils.formatters import format_value +from contextlib import suppress +def get_monthly_results( + goal_doctype: str, + goal_field: str, + date_col: str, + filters: Dict, + aggregation: str = "sum", +) -> Dict: + """Get monthly aggregation values for given field of doctype""" -def get_monthly_results(goal_doctype, goal_field, date_col, filter_str, aggregation = 'sum'): - '''Get monthly aggregation values for given field of doctype''' - # TODO: move to ORM? - if(frappe.db.db_type == 'postgres'): - month_year_format_query = '''to_char("{}", 'MM-YYYY')'''.format(date_col) - else: - month_year_format_query = 'date_format(`{}`, "%m-%Y")'.format(date_col) + Table = DocType(goal_doctype) + date_format = "%m-%Y" if frappe.db.db_type != "postgres" else "MM-YYYY" - conditions = ('where ' + filter_str) if filter_str else '' - results = frappe.db.sql('''SELECT {aggregation}(`{goal_field}`) AS {goal_field}, - {month_year_format_query} AS month_year - FROM `{table_name}` {conditions} - GROUP BY month_year''' - .format( - aggregation=aggregation, - goal_field=goal_field, - month_year_format_query=month_year_format_query, - table_name="tab" + goal_doctype, - conditions=conditions - ), as_dict=True) + return dict( + frappe.db.query.build_conditions(table=goal_doctype, filters=filters) + .select( + DateFormat(Table[date_col], date_format).as_("month_year"), + Function(aggregation, goal_field), + ) + .groupby("month_year") + .run() + ) - month_to_value_dict = {} - for d in results: - month_to_value_dict[d['month_year']] = d[goal_field] - - return month_to_value_dict @frappe.whitelist() -def get_monthly_goal_graph_data(title, doctype, docname, goal_value_field, goal_total_field, goal_history_field, - goal_doctype, goal_doctype_link, goal_field, date_field, filter_str, aggregation="sum"): - ''' - Get month-wise graph data for a doctype based on aggregation values of a field in the goal doctype +def get_monthly_goal_graph_data( + title: str, + doctype: str, + docname: str, + goal_value_field: str, + goal_total_field: str, + goal_history_field: str, + goal_doctype: str, + goal_doctype_link: str, + goal_field: str, + date_field: str, + filter_str: str = None, + aggregation: str = "sum", + filters: Optional[Dict] = None, +) -> Dict: + """ + Get month-wise graph data for a doctype based on aggregation values of a field in the goal doctype - :param title: Graph title - :param doctype: doctype of graph doc - :param docname: of the doc to set the graph in - :param goal_value_field: goal field of doctype - :param goal_total_field: current month value field of doctype - :param goal_history_field: cached history field - :param goal_doctype: doctype the goal is based on - :param goal_doctype_link: doctype link field in goal_doctype - :param goal_field: field from which the goal is calculated - :param filter_str: where clause condition - :param aggregation: a value like 'count', 'sum', 'avg' + :param title: Graph title + :param doctype: doctype of graph doc + :param docname: of the doc to set the graph in + :param goal_value_field: goal field of doctype + :param goal_total_field: current month value field of doctype + :param goal_history_field: cached history field + :param goal_doctype: doctype the goal is based on + :param goal_doctype_link: doctype link field in goal_doctype + :param goal_field: field from which the goal is calculated + :param filter_str: [DEPRECATED] where clause condition. Use filters. + :param aggregation: a value like 'count', 'sum', 'avg' + :param filters: optional filters - :return: dict of graph data - ''' + :return: dict of graph data + """ + if isinstance(filter_str, str): + frappe.throw("String filters have been deprecated. Pass Dict filters instead.", exc=DeprecationWarning) - from frappe.utils.formatters import format_value - import json - - # should have atleast read perm - if not frappe.has_permission(goal_doctype): - return None - - meta = frappe.get_meta(doctype) doc = frappe.get_doc(doctype, docname) + doc.check_permission() + meta = doc.meta goal = doc.get(goal_value_field) - formatted_goal = format_value(goal, meta.get_field(goal_value_field), doc) + today_date = now_datetime().date() current_month_value = doc.get(goal_total_field) - formatted_value = format_value(current_month_value, meta.get_field(goal_total_field), doc) - - from frappe.utils import today, getdate, formatdate, add_months - current_month_year = formatdate(today(), "MM-yyyy") - + current_month_year = today_date.strftime("%m-%Y") # eg: "02-2022" + formatted_value = format_value( + current_month_value, meta.get_field(goal_total_field), doc + ) history = doc.get(goal_history_field) - try: - month_to_value_dict = json.loads(history) if history and '{' in history else None - except ValueError: - month_to_value_dict = None + + month_to_value_dict = None + if history: + with suppress(ValueError): + month_to_value_dict = frappe.parse_json(history) if month_to_value_dict is None: - doc_filter = (goal_doctype_link + " = " + frappe.db.escape(docname)) if doctype != goal_doctype else '' - if filter_str: - doc_filter += ' and ' + filter_str if doc_filter else filter_str - month_to_value_dict = get_monthly_results(goal_doctype, goal_field, date_field, doc_filter, aggregation) + doc_filter = {} + with suppress(ValueError): + doc_filter = frappe.parse_json(filters or "{}") + if doctype != goal_doctype: + doc_filter[goal_doctype_link] = docname + + month_to_value_dict = get_monthly_results( + goal_doctype, goal_field, date_field, doc_filter, aggregation + ) month_to_value_dict[current_month_year] = current_month_value - months = [] - months_formatted = [] - values = [] + month_labels = [] + dataset_values = [] values_formatted = [] - for i in range(0, 12): - date_value = add_months(today(), -i) - month_value = formatdate(date_value, "MM-yyyy") - month_word = getdate(date_value).strftime('%b %y') - month_year = getdate(date_value).strftime('%B') + ', ' + getdate(date_value).strftime('%Y') - months.insert(0, month_word) - months_formatted.insert(0, month_year) - if month_value in month_to_value_dict: - val = month_to_value_dict[month_value] - else: - val = 0 - values.insert(0, val) - values_formatted.insert(0, format_value(val, meta.get_field(goal_total_field), doc)) + y_markers = {} - y_markers = [] summary_values = [ - { - 'title': _("This month"), - 'color': '#ffa00a', - 'value': formatted_value - } + {"title": _("This month"), "color": "#ffa00a", "value": formatted_value}, ] - if float(goal) > 0: - y_markers = [ - { - 'label': _("Goal"), - 'lineType': "dashed", - 'value': goal - }, - ] + if flt(goal) > 0: + formatted_goal = format_value(goal, meta.get_field(goal_value_field), doc) summary_values += [ + {"title": _("Goal"), "color": "#5e64ff", "value": formatted_goal}, { - 'title': _("Goal"), - 'color': '#5e64ff', - 'value': formatted_goal + "title": _("Completed"), + "color": "#28a745", + "value": f"{int(round(flt(current_month_value) / flt(goal) * 100))}%", }, - { - 'title': _("Completed"), - 'color': '#28a745', - 'value': str(int(round(float(current_month_value)/float(goal)*100))) + "%" - } ] + y_markers = { + "yMarkers": [{"label": _("Goal"), "lineType": "dashed", "value": flt(goal)}] + } - data = { - 'title': title, - # 'subtitle': + for i in range(12): + date_value = add_to_date(today_date, months=-i, as_datetime=True) + month_word = date_value.strftime("%b %y") # eg: "Feb 22" + month_labels.insert(0, month_word) - 'data': { - 'datasets': [ - { - 'values': values, - 'formatted': values_formatted - } - ], - 'labels': months, + month_value = date_value.strftime("%m-%Y") # eg: "02-2022" + val = month_to_value_dict.get(month_value, 0) + dataset_values.insert(0, val) + values_formatted.insert( + 0, format_value(val, meta.get_field(goal_total_field), doc) + ) + + return { + "title": title, + "data": { + "datasets": [{"values": dataset_values, "formatted": values_formatted}], + "labels": month_labels, + **y_markers, }, - - 'summary': summary_values, + "summary": summary_values, } - - if y_markers: - data["data"]["yMarkers"] = y_markers - - return data From f8f9b1946156552a28b476652d16b5db7725d0f0 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 16:02:28 +0530 Subject: [PATCH 08/25] fix(set-last-active-for-user): Skip the extra lines to just do a set_value --- frappe/commands/site.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/commands/site.py b/frappe/commands/site.py index e788c7ec4d..6bee442054 100644 --- a/frappe/commands/site.py +++ b/frappe/commands/site.py @@ -780,9 +780,8 @@ def set_user_password(site, user, password, logout_all_sessions=False): @pass_context def set_last_active_for_user(context, user=None): "Set users last active date to current datetime" - from frappe.core.doctype.user.user import get_system_users - from frappe.utils.user import set_last_active_to_now + from frappe.utils import now_datetime site = get_site(context) @@ -795,9 +794,10 @@ def set_last_active_for_user(context, user=None): else: return - set_last_active_to_now(user) + frappe.db.set_value("User", user, "last_active", now_datetime()) frappe.db.commit() + @click.command('publish-realtime') @click.argument('event') @click.option('--message') From d0bd4730da36da559f472e954bf9fba883c61e20 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 16:03:24 +0530 Subject: [PATCH 09/25] refactor(file_manager): Refactor raw SQLs to use qb & db_query APIs --- frappe/utils/file_manager.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/frappe/utils/file_manager.py b/frappe/utils/file_manager.py index 1e654d7881..a31577e20b 100644 --- a/frappe/utils/file_manager.py +++ b/frappe/utils/file_manager.py @@ -6,6 +6,7 @@ import os, base64, re, json import hashlib import mimetypes import io +from frappe.query_builder.utils import DocType from frappe.utils import get_hook_method, get_files_path, random_string, encode, cstr, call_hook_method, cint from frappe import _ from frappe import conf @@ -176,7 +177,7 @@ def save_file(fname, content, dt, dn, folder=None, decode=False, is_private=0, d def get_file_data_from_hash(content_hash, is_private=0): - for name in frappe.db.sql_list("select name from `tabFile` where content_hash=%s and is_private=%s", (content_hash, is_private)): + for name in frappe.get_all("File", {"content_hash": content_hash, "is_private": is_private}, pluck="name"): b = frappe.get_doc('File', name) return {k: b.get(k) for k in frappe.get_hooks()['write_file_keys']} return False @@ -230,8 +231,7 @@ def write_file(content, fname, is_private=0): def remove_all(dt, dn, from_delete=False, delete_permanently=False): """remove all files in a transaction""" try: - for fid in frappe.db.sql_list("""select name from `tabFile` where - attached_to_doctype=%s and attached_to_name=%s""", (dt, dn)): + for fid in frappe.get_all("File", {"attached_to_doctype": dt, "attached_to_name": dn}, pluck="name"): if from_delete: # If deleting a doc, directly delete files frappe.delete_doc("File", fid, ignore_permissions=True, delete_permanently=delete_permanently) @@ -319,8 +319,10 @@ def get_file_path(file_name): if '../' in file_name: return - f = frappe.db.sql("""select file_url from `tabFile` - where name=%s or file_name=%s""", (file_name, file_name)) + File = DocType("File") + + f = frappe.qb.from_(File).where((File.name == file_name) & (File.file_name == file_name)).select(File.file_url).run() + if f: file_name = f[0][0] @@ -351,7 +353,7 @@ def get_file_name(fname, optional_suffix): # convert to unicode fname = cstr(fname) - n_records = frappe.db.sql("select name from `tabFile` where file_name=%s", fname) + n_records = frappe.get_all("File", {"file_name": fname}, pluck="name") if len(n_records) > 0 or os.path.exists(encode(get_files_path(fname))): f = fname.rsplit('.', 1) if len(f) == 1: From b99cd7ab3b924b96e5d3a36334ac770b112e0524 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 16:15:02 +0530 Subject: [PATCH 10/25] refactor: frappe.utils.user * Refactored raw SQLs and outdated APIs to use newer DB + QB APIs * Simplified Python & Query logic * Added type hints * Styled with Black --- frappe/utils/user.py | 283 +++++++++++++++++++++++++------------------ 1 file changed, 165 insertions(+), 118 deletions(-) diff --git a/frappe/utils/user.py b/frappe/utils/user.py index c84b29b1c9..43d9d26ab8 100644 --- a/frappe/utils/user.py +++ b/frappe/utils/user.py @@ -1,15 +1,22 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE -import frappe, json -from frappe import _dict +from email.utils import formataddr +from typing import Dict, List, Optional, TYPE_CHECKING + +import frappe import frappe.share -from frappe.utils import cint +from frappe import _dict from frappe.boot import get_allowed_reports -from frappe.permissions import get_roles, get_valid_perms from frappe.core.doctype.domain_settings.domain_settings import get_active_modules +from frappe.permissions import get_roles, get_valid_perms from frappe.query_builder import DocType from frappe.query_builder.functions import Concat_ws +from frappe.query_builder import Order + +if TYPE_CHECKING: + from frappe.core.doctype.user.user import User + class UserPermissions: """ @@ -150,10 +157,8 @@ class UserPermissions: self.can_write += self.in_create self.can_read += self.can_write - self.shared = frappe.db.sql_list("""select distinct share_doctype from `tabDocShare` - where `user`=%s and `read`=1""", self.name) + self.shared = frappe.get_all("DocShare", {"user": self.name, "read": 1}, distinct=True, pluck="share_doctype") self.can_read = list(set(self.can_read + self.shared)) - self.all_read += self.can_read for dt in no_list_view_link: @@ -161,11 +166,12 @@ class UserPermissions: self.can_read.remove(dt) if "System Manager" in self.get_roles(): - docs = frappe.get_all("DocType", {'allow_import': 1}) - self.can_import += [doc.name for doc in docs] - - customizations = frappe.get_all("Property Setter", fields=['doc_type'], filters={'property': 'allow_import', 'value': "1"}) - self.can_import += [custom.doc_type for custom in customizations] + self.can_import += frappe.get_all("DocType", {'allow_import': 1}, pluck="name") + self.can_import += frappe.get_all( + "Property Setter", + pluck="doc_type", + filters={"property": "allow_import", "value": "1"}, + ) frappe.cache().hset("can_import", frappe.session.user, self.can_import) @@ -186,10 +192,24 @@ class UserPermissions: return self.can_read def load_user(self): - d = frappe.db.sql("""select email, first_name, last_name, creation, - email_signature, user_type, desk_theme, language, - mute_sounds, send_me_a_copy, document_follow_notify - from tabUser where name = %s""", (self.name,), as_dict=1)[0] + d = frappe.db.get_value( + "User", + self.name, + [ + "creation", + "desk_theme", + "document_follow_notify", + "email", + "email_signature", + "first_name", + "language", + "last_name", + "mute_sounds", + "send_me_a_copy", + "user_type", + ], + as_dict=True, + ) if not self.can_read: self.build_permissions() @@ -209,142 +229,169 @@ class UserPermissions: def get_all_reports(self): return get_allowed_reports() -def get_user_fullname(user): + +def get_user_fullname(user: str) -> str: user_doctype = DocType("User") - fullname = frappe.get_value( - user_doctype, - filters={"name": user}, - fieldname=Concat_ws(" ", user_doctype.first_name, user_doctype.last_name), + return ( + frappe.get_value( + user_doctype, + filters={"name": user}, + fieldname=Concat_ws(" ", user_doctype.first_name, user_doctype.last_name), + ) + or "" ) - return fullname or '' -def get_fullname_and_avatar(user): - first_name, last_name, avatar, name = frappe.db.get_value("User", - user, ["first_name", "last_name", "user_image", "name"]) - return _dict({ - "fullname": " ".join(list(filter(None, [first_name, last_name]))), - "avatar": avatar, - "name": name - }) -def get_system_managers(only_name=False): +def get_fullname_and_avatar(user: str) -> _dict: + first_name, last_name, avatar, name = frappe.db.get_value( + "User", user, ["first_name", "last_name", "user_image", "name"] + ) + return _dict( + { + "fullname": " ".join(list(filter(None, [first_name, last_name]))), + "avatar": avatar, + "name": name, + } + ) + + +def get_system_managers(only_name: bool = False) -> List[str]: """returns all system manager's user details""" - import email.utils - system_managers = frappe.db.sql("""SELECT DISTINCT `name`, `creation`, - CONCAT_WS(' ', - CASE WHEN `first_name`= '' THEN NULL ELSE `first_name` END, - CASE WHEN `last_name`= '' THEN NULL ELSE `last_name` END - ) AS fullname - FROM `tabUser` AS p - WHERE `docstatus` < 2 - AND `enabled` = 1 - AND `name` NOT IN ({}) - AND exists - (SELECT * - FROM `tabHas Role` AS ur - WHERE ur.parent = p.name - AND ur.role='System Manager') - ORDER BY `creation` DESC""".format(", ".join(["%s"]*len(frappe.STANDARD_USERS))), - frappe.STANDARD_USERS, as_dict=True) + HasRole = DocType("Has Role") + User = DocType("User") + + if only_name: + fields = [User.name] + else: + fields = [User.full_name, User.name] + + system_managers = ( + frappe.qb.from_(User) + .join(HasRole) + .on((HasRole.parent == User.name)) + .where( + (HasRole.parenttype == "User") + & (User.enabled == 1) + & (HasRole.role == "System Manager") + & (User.docstatus < 2) + & (User.name.notin(frappe.STANDARD_USERS)) + ) + .select(*fields) + .orderby(User.creation, order=Order.desc) + .run(as_dict=True) + ) if only_name: return [p.name for p in system_managers] else: - return [email.utils.formataddr((p.fullname, p.name)) for p in system_managers] + return [formataddr((p.full_name, p.name)) for p in system_managers] -def add_role(user, role): + +def add_role(user: str, role: str) -> None: frappe.get_doc("User", user).add_roles(role) -def add_system_manager(email, first_name=None, last_name=None, send_welcome_email=False, password=None): + +def add_system_manager( + email: str, + first_name: Optional[str] = None, + last_name: Optional[str] = None, + send_welcome_email: bool = False, + password: str = None, +) -> "User": # add user user = frappe.new_doc("User") - user.update({ - "name": email, - "email": email, - "enabled": 1, - "first_name": first_name or email, - "last_name": last_name, - "user_type": "System User", - "send_welcome_email": 1 if send_welcome_email else 0 - }) + user.update( + { + "name": email, + "email": email, + "enabled": 1, + "first_name": first_name or email, + "last_name": last_name, + "user_type": "System User", + "send_welcome_email": 1 if send_welcome_email else 0, + } + ) user.insert() # add roles - roles = frappe.get_all('Role', - fields=['name'], - filters={ - 'name': ['not in', ('Administrator', 'Guest', 'All')] - } + roles = frappe.get_all( + "Role", + fields=["name"], + filters={"name": ["not in", ("Administrator", "Guest", "All")]}, ) roles = [role.name for role in roles] user.add_roles(*roles) if password: from frappe.utils.password import update_password - update_password(user=user.name, pwd=password) -def get_enabled_system_users(): - # add more fields if required - return frappe.get_all('User', - fields=['email', 'language', 'name'], + update_password(user=user.name, pwd=password) + return user + + +def get_enabled_system_users() -> List[Dict]: + return frappe.get_all( + "User", + fields=["email", "language", "name"], filters={ - 'user_type': 'System User', - 'enabled': 1, - 'name': ['not in', ('Administrator', 'Guest')] - } + "user_type": "System User", + "enabled": 1, + "name": ["not in", ("Administrator", "Guest")], + }, ) -def is_website_user(): - return frappe.db.get_value('User', frappe.session.user, 'user_type') == "Website User" -def is_system_user(username): - return frappe.db.get_value("User", {"email": username, "enabled": 1, "user_type": "System User"}) +def is_website_user(username: Optional[str] = None) -> Optional[str]: + return ( + frappe.db.get_value("User", username or frappe.session.user, "user_type") + == "Website User" + ) -def get_users(): + +def is_system_user(username: Optional[str] = None) -> Optional[str]: + return frappe.db.get_value( + "User", + { + "email": username or frappe.session.user, + "enabled": 1, + "user_type": "System User", + }, + ) + + +def get_users() -> List[Dict]: from frappe.core.doctype.user.user import get_system_users + users = [] - system_managers = frappe.utils.user.get_system_managers(only_name=True) + system_managers = get_system_managers(only_name=True) + for user in get_system_users(): - users.append({ - "full_name": frappe.utils.user.get_user_fullname(user), - "email": user, - "is_system_manager": 1 if (user in system_managers) else 0 - }) + users.append( + { + "full_name": get_user_fullname(user), + "email": user, + "is_system_manager": user in system_managers, + } + ) return users -def set_last_active_to_now(user): - from frappe.utils import now_datetime - frappe.db.set_value("User", user, "last_active", now_datetime()) +def get_users_with_role(role: str) -> List[str]: + User = DocType("User") + HasRole = DocType("Has Role") -def reset_simultaneous_sessions(user_limit): - for user in frappe.db.sql("""select name, simultaneous_sessions from tabUser - where name not in ('Administrator', 'Guest') and user_type = 'System User' and enabled=1 - order by creation desc""", as_dict=1): - if user.simultaneous_sessions < user_limit: - user_limit = user_limit - user.simultaneous_sessions - else: - frappe.db.set_value("User", user.name, "simultaneous_sessions", 1) - user_limit = user_limit - 1 - -def get_link_to_reset_password(user): - link = '' - - if not cint(frappe.db.get_single_value('System Settings', 'setup_complete')): - user = frappe.get_doc("User", user) - link = user.reset_password(send_email=False) - frappe.db.commit() - - return { - 'link': link - } - -def get_users_with_role(role): - return [p[0] for p in frappe.db.sql("""SELECT DISTINCT `tabUser`.`name` - FROM `tabHas Role`, `tabUser` - WHERE `tabHas Role`.`role`=%s - AND `tabUser`.`name`!='Administrator' - AND `tabHas Role`.`parent`=`tabUser`.`name` - AND `tabUser`.`enabled`=1""", role)] + return ( + frappe.qb.from_(HasRole) + .from_(User) + .where( + (HasRole.role == role) + & (User.name != "Administrator") + & (User.enabled == 1) + & (HasRole.parent == User.name) + ) + .select(User.name) + .distinct() + .run(pluck=True) + ) From 69df3edba3dd0a6c51ca0e18fcad2af322c0d90b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Thu, 31 Mar 2022 16:24:03 +0530 Subject: [PATCH 11/25] fix(query): Typo in OPERATOR_MAP --- frappe/database/query.py | 2 +- frappe/utils/goal.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/database/query.py b/frappe/database/query.py index a8a1271e70..641b584932 100644 --- a/frappe/database/query.py +++ b/frappe/database/query.py @@ -122,7 +122,7 @@ OPERATOR_MAP = { "<": operator.lt, ">": operator.gt, "<=": operator.le, - "=>": operator.le, + "=<": operator.le, ">=": operator.ge, "=>": operator.ge, "in": func_in, diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index 97624e6272..fde948bcea 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -69,7 +69,7 @@ def get_monthly_goal_graph_data( :return: dict of graph data """ if isinstance(filter_str, str): - frappe.throw("String filters have been deprecated. Pass Dict filters instead.", exc=DeprecationWarning) + frappe.throw("String filters have been deprecated. Pass Dict filters instead.", exc=DeprecationWarning) # nosemgrep doc = frappe.get_doc(doctype, docname) doc.check_permission() From bd3573430757438347dda12b7f4ffd4de151f4a7 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 00:03:54 +0530 Subject: [PATCH 12/25] perf: reduce query when calling `get_doc` with filters --- frappe/model/document.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index b3c6f6ef20..c60c4c9e85 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -88,25 +88,19 @@ class Document(BaseDocument): If DocType name and document name are passed, the object will load all values (including child documents) from the database. """ - self.doctype = self.name = None - self._default_new_docs = {} + self.doctype = None + self.name = None self.flags = frappe._dict() if args and args[0] and isinstance(args[0], str): # first arugment is doctype + self.doctype = args[0] + if len(args)==1: # single - self.doctype = self.name = args[0] + self.name = self.doctype else: - self.doctype = args[0] - if isinstance(args[1], dict): - # filter - self.name = frappe.db.get_value(args[0], args[1], "name") - if self.name is None: - frappe.throw(_("{0} {1} not found").format(_(args[0]), args[1]), - frappe.DoesNotExistError) - else: - self.name = args[1] + self.name = args[1] if 'for_update' in kwargs: self.flags.for_update = kwargs.get('for_update') From e1cc626bd5cf34c0412b92bde35ab4d17e615fb8 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 00:54:31 +0530 Subject: [PATCH 13/25] style: improve code readability --- frappe/model/document.py | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index c60c4c9e85..2f2a90f210 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -92,25 +92,24 @@ class Document(BaseDocument): self.name = None self.flags = frappe._dict() - if args and args[0] and isinstance(args[0], str): - # first arugment is doctype - self.doctype = args[0] + if args and args[0]: + if isinstance(args[0], str): + # first arugment is doctype + self.doctype = args[0] - if len(args)==1: - # single - self.name = self.doctype - else: - self.name = args[1] + if len(args) == 1: + # single + self.name = self.doctype + else: + self.name = args[1] + self.flags.for_update = kwargs.get("for_update") - if 'for_update' in kwargs: - self.flags.for_update = kwargs.get('for_update') + self.load_from_db() + return - self.load_from_db() - return - - if args and args[0] and isinstance(args[0], dict): - # first argument is a dict - kwargs = args[0] + if isinstance(args[0], dict): + # first argument is a dict + kwargs = args[0] if kwargs: # init base document @@ -127,10 +126,6 @@ class Document(BaseDocument): frappe.whitelist()(fn) return fn - def reload(self): - """Reload document from database""" - self.load_from_db() - def load_from_db(self): """Load document and children from database and create properties from fields""" @@ -171,6 +166,8 @@ class Document(BaseDocument): if hasattr(self, "__setup__"): self.__setup__() + reload = load_from_db + def get_latest(self): if not getattr(self, "latest", None): self.latest = frappe.get_doc(self.doctype, self.name) From 0ec26cee70563609e2b1df2c5adc40447fcaf3a2 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 01:57:09 +0530 Subject: [PATCH 14/25] perf: use `as_dict` parameter --- frappe/model/document.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 2f2a90f210..d491246fa2 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -132,7 +132,7 @@ class Document(BaseDocument): if not getattr(self, "_metaclass", False) and self.meta.issingle: single_doc = frappe.db.get_singles_dict(self.doctype) if not single_doc: - single_doc = frappe.new_doc(self.doctype).as_dict() + single_doc = frappe.new_doc(self.doctype, as_dict=True) single_doc["name"] = self.doctype del single_doc["__islocal"] From 811c2130820560f1847673bcf345aef1dd5c4aa1 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 03:59:39 +0530 Subject: [PATCH 15/25] perf: 80% faster `doc.get` for fields with `None` as value --- frappe/model/base_document.py | 53 ++++++++++++++++++++++++++--------- frappe/model/document.py | 30 ++++++++++---------- frappe/model/meta.py | 12 ++------ 3 files changed, 57 insertions(+), 38 deletions(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index 8fd64689fc..e38880589b 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -3,7 +3,7 @@ import datetime import frappe -from frappe import _ +from frappe import _, _dict from frappe.model import child_table_fields, default_fields, display_fieldtypes, table_fields from frappe.model.naming import set_new_name from frappe.model.utils.link_count import notify_link_count @@ -20,6 +20,14 @@ max_positive_value = { 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"}), +] + def get_controller(doctype): """Returns the **class** object of the given DocType. @@ -146,12 +154,6 @@ class BaseDocument(object): else: value = self.__dict__.get(key, default) - if value is None and key in ( - d.fieldname for d in self.meta.get_table_fields() - ): - value = [] - self.set(key, value) - if limit and isinstance(value, (list, tuple)) and len(value) > limit: value = value[:limit] @@ -189,7 +191,7 @@ class BaseDocument(object): if value is None: value={} if isinstance(value, (dict, BaseDocument)): - if not self.__dict__.get(key): + if self.__dict__.get(key) is None: self.__dict__[key] = [] value = self._init_child(value, key) @@ -252,8 +254,23 @@ class BaseDocument(object): return value + def _get_table_fields(self): + """ + To get table fields during Document init + Meta.get_table_fields goes into recursion for special doctypes + """ + + # child tables don't have child tables + if getattr(self, "parentfield", None): + return () + + if self.doctype == "DocType": + return DOCTYPE_TABLE_FIELDS + + return self.meta.get_table_fields() + def get_valid_dict(self, sanitize=True, convert_dates_to_str=False, ignore_nulls=False, ignore_virtual=False): - d = frappe._dict() + d = _dict() for fieldname in self.meta.get_valid_columns(): d[fieldname] = self.get(fieldname) @@ -314,6 +331,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 df in self._get_table_fields(): + if self.__dict__.get(df.fieldname) is None: + self.__dict__[df.fieldname] = [] + def init_valid_columns(self): for key in default_fields: if key not in self.__dict__: @@ -598,7 +625,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 @@ -643,7 +670,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, + values = _dict(name=frappe.db.get_value(doctype, docname, 'name', cache=True)) else: values_to_fetch = ['name'] + [_df.fetch_from.split('.')[-1] @@ -938,10 +965,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 diff --git a/frappe/model/document.py b/frappe/model/document.py index b3c6f6ef20..718c6875cf 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -121,6 +121,7 @@ class Document(BaseDocument): if kwargs: # init base document super(Document, self).__init__(kwargs) + self.init_child_tables() self.init_valid_columns() else: @@ -158,20 +159,20 @@ 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 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 [] - 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") - 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__"): @@ -856,8 +857,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 diff --git a/frappe/model/meta.py b/frappe/model/meta.py index a3d167fb9b..6cfba382c6 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -30,7 +30,7 @@ from frappe.model import ( optional_fields, table_fields, ) -from frappe.model.base_document import BaseDocument +from frappe.model.base_document import BaseDocument, DOCTYPE_TABLE_FIELDS from frappe.model.document import Document from frappe.model.workflow import get_workflow_name from frappe.modules import load_doctype_module @@ -180,7 +180,7 @@ class Meta(Document): def get_table_fields(self): if not hasattr(self, "_table_fields"): - if self.name!="DocType": + if self.name != "DocType": self._table_fields = self.get('fields', {"fieldtype": ['in', table_fields]}) else: self._table_fields = DOCTYPE_TABLE_FIELDS @@ -609,14 +609,6 @@ class Meta(Document): def is_nested_set(self): 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"}), -] - ####### def is_single(doctype): From e73c5526328af7302080e7f23c7943bd0918d834 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 14:15:35 +0530 Subject: [PATCH 16/25] fix: implement `for_update` for Single documents --- frappe/database/database.py | 11 +++++++---- frappe/model/document.py | 15 +++++++-------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 511d993aa5..a2bf3de80b 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -549,7 +549,7 @@ class Database(object): return r and [[i[1] for i in r]] or [] - def get_singles_dict(self, doctype, debug = False): + def get_singles_dict(self, doctype, debug=False, *, for_update=False): """Get Single DocType as dict. :param doctype: DocType of the single object whose value is requested @@ -560,10 +560,13 @@ class Database(object): account_settings = frappe.db.get_singles_dict("Accounts Settings") """ result = self.query.get_sql( - "Singles", filters={"doctype": doctype}, fields=["field", "value"] + "Singles", + filters={"doctype": doctype}, + fields=["field", "value"], + for_update=for_update, ).run() - dict_ = frappe._dict(result) - return dict_ + + return frappe._dict(result) @staticmethod def get_all(*args, **kwargs): diff --git a/frappe/model/document.py b/frappe/model/document.py index d491246fa2..abb8301873 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -96,14 +96,11 @@ class Document(BaseDocument): if isinstance(args[0], str): # first arugment is doctype self.doctype = args[0] + self.name = self.doctype if len(args) == 1 else args[1] - if len(args) == 1: - # single - self.name = self.doctype - else: - self.name = args[1] - self.flags.for_update = kwargs.get("for_update") - + # for_update is set in flags to avoid changing load_from_db signature + # since it is used in virtual doctypes and inherited in child classes + self.flags.for_update = kwargs.get("for_update") self.load_from_db() return @@ -130,7 +127,9 @@ class Document(BaseDocument): """Load document and children from database and create properties from fields""" if not getattr(self, "_metaclass", False) and self.meta.issingle: - single_doc = frappe.db.get_singles_dict(self.doctype) + single_doc = frappe.db.get_singles_dict( + self.doctype, for_update=self.flags.for_update + ) if not single_doc: single_doc = frappe.new_doc(self.doctype, as_dict=True) single_doc["name"] = self.doctype From 428fc98fa7814817e84a5f138791f81884640de6 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 14:28:13 +0530 Subject: [PATCH 17/25] chore: add comment to explain line --- frappe/model/document.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/model/document.py b/frappe/model/document.py index abb8301873..76e73d69fa 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -96,6 +96,8 @@ class Document(BaseDocument): if isinstance(args[0], str): # first arugment is doctype self.doctype = args[0] + + # doctype for singles, string value or filters for other documents self.name = self.doctype if len(args) == 1 else args[1] # for_update is set in flags to avoid changing load_from_db signature From 6f99a4d8db9e891ce42550ceb18bb1b478b1b4c7 Mon Sep 17 00:00:00 2001 From: Suraj Shetty <13928957+surajshetty3416@users.noreply.github.com> Date: Fri, 1 Apr 2022 15:00:57 +0530 Subject: [PATCH 18/25] chore: Update CODEOWNERS --- CODEOWNERS | 2 -- 1 file changed, 2 deletions(-) diff --git a/CODEOWNERS b/CODEOWNERS index f7d759c123..170334a4b4 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -6,9 +6,7 @@ * @frappe/frappe-review-team templates/ @surajshetty3416 www/ @surajshetty3416 -integrations/ @leela patches/ @surajshetty3416 @gavindsouza -email/ @leela event_streaming/ @ruchamahabal data_import* @netchampfaris core/ @surajshetty3416 From 842a4b03a5dce1ea236822e4dafcea17f0bf9f2a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 1 Apr 2022 16:42:43 +0530 Subject: [PATCH 19/25] fix(test_goal): Generate fresh data for each test --- frappe/tests/test_goal.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frappe/tests/test_goal.py b/frappe/tests/test_goal.py index 09bc6baedb..35028d939f 100644 --- a/frappe/tests/test_goal.py +++ b/frappe/tests/test_goal.py @@ -9,11 +9,12 @@ from frappe.tests.utils import FrappeTestCase class TestGoal(FrappeTestCase): - @classmethod - def setUpClass(cls): - super().setUpClass() + def setUp(self): make_test_objects("Event", reset=True) + def tearDown(self): + frappe.db.delete("Event") + def test_get_monthly_results(self): """Test monthly aggregation values of a field""" result_dict = get_monthly_results( From eaa332e901369bd3265e889e55533a2eec0e4714 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 1 Apr 2022 16:43:25 +0530 Subject: [PATCH 20/25] fix(goal): Make sure history field contains a JSON dump --- frappe/utils/goal.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/utils/goal.py b/frappe/utils/goal.py index fde948bcea..0a9116f0e5 100644 --- a/frappe/utils/goal.py +++ b/frappe/utils/goal.py @@ -7,7 +7,7 @@ import frappe from frappe import _ from frappe.query_builder.functions import DateFormat, Function from frappe.query_builder.utils import DocType -from frappe.utils.data import add_to_date, flt, now_datetime +from frappe.utils.data import add_to_date, cstr, flt, now_datetime from frappe.utils.formatters import format_value from contextlib import suppress @@ -86,11 +86,11 @@ def get_monthly_goal_graph_data( history = doc.get(goal_history_field) month_to_value_dict = None - if history: + if history and "{" in cstr(history): with suppress(ValueError): month_to_value_dict = frappe.parse_json(history) - if month_to_value_dict is None: + if month_to_value_dict is None: # nosemgrep doc_filter = {} with suppress(ValueError): doc_filter = frappe.parse_json(filters or "{}") From 1b0731010e7b1f6a29c259d8a496670fd42120b8 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 1 Apr 2022 17:01:49 +0530 Subject: [PATCH 21/25] chore(test): Cleanup imports for global search --- frappe/tests/test_global_search.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/frappe/tests/test_global_search.py b/frappe/tests/test_global_search.py index 3dc8f1691a..448ad68810 100644 --- a/frappe/tests/test_global_search.py +++ b/frappe/tests/test_global_search.py @@ -2,13 +2,14 @@ # License: MIT. See LICENSE import unittest + import frappe -from frappe.utils import global_search -from frappe.test_runner import make_test_objects +from frappe.custom.doctype.property_setter.property_setter import make_property_setter from frappe.desk.page.setup_wizard.install_fixtures import update_global_search_doctypes +from frappe.utils import global_search, now_datetime +from frappe.test_runner import make_test_objects -import frappe.utils class TestGlobalSearch(unittest.TestCase): def setUp(self): @@ -17,7 +18,6 @@ class TestGlobalSearch(unittest.TestCase): self.assertTrue('__global_search' in frappe.db.get_tables()) doctype = "Event" global_search.reset() - from frappe.custom.doctype.property_setter.property_setter import make_property_setter make_property_setter(doctype, "subject", "in_global_search", 1, "Int") make_property_setter(doctype, "event_type", "in_global_search", 1, "Int") make_property_setter(doctype, "roles", "in_global_search", 1, "Int") @@ -42,12 +42,11 @@ class TestGlobalSearch(unittest.TestCase): doctype='Event', subject=text, repeat_on='Monthly', - starts_on=frappe.utils.now_datetime())).insert() + starts_on=now_datetime())).insert() global_search.sync_global_search() frappe.db.commit() - def test_search(self): self.insert_test_events() results = global_search.search('awakens') @@ -75,7 +74,6 @@ class TestGlobalSearch(unittest.TestCase): results = global_search.search('Monthly') self.assertEqual(len(results), 0) doctype = "Event" - from frappe.custom.doctype.property_setter.property_setter import make_property_setter make_property_setter(doctype, "repeat_on", "in_global_search", 1, "Int") global_search.rebuild_for_doctype(doctype) results = global_search.search('Monthly') @@ -111,7 +109,7 @@ class TestGlobalSearch(unittest.TestCase): doc = frappe.get_doc({ 'doctype':'Event', 'subject': text, - 'starts_on': frappe.utils.now_datetime() + 'starts_on': now_datetime() }) doc.insert() @@ -172,7 +170,7 @@ class TestGlobalSearch(unittest.TestCase): doc = frappe.get_doc({ 'doctype':'Event', 'subject': 'Lorem Ipsum', - 'starts_on': frappe.utils.now_datetime(), + 'starts_on': now_datetime(), 'description': case["data"] }) From 179f61b2e1dfa1e6b1454a041c444ca8a0de7d0a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 1 Apr 2022 17:02:29 +0530 Subject: [PATCH 22/25] fix(test): Commit post global_search.sync_global_search This is an attempt to tackle the flaky test_delete_doc test. Not sure if this will do anything but the other tests seem to be having a commit directly after the sync...so following that here. If this works, the goal would be to find a way to get rid of the commits altogether. Since the flaky test can't be replicated locally, let's see if this works !!!! --- frappe/tests/test_global_search.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/tests/test_global_search.py b/frappe/tests/test_global_search.py index 448ad68810..0faff55e7f 100644 --- a/frappe/tests/test_global_search.py +++ b/frappe/tests/test_global_search.py @@ -1,4 +1,4 @@ -# Copyright (c) 2015, Frappe Technologies Pvt. Ltd. and Contributors +# Copyright (c) 2022, Frappe Technologies Pvt. Ltd. and Contributors # License: MIT. See LICENSE import unittest @@ -89,6 +89,7 @@ class TestGlobalSearch(unittest.TestCase): frappe.delete_doc('Event', event_name) global_search.sync_global_search() + frappe.db.commit() results = global_search.search(test_subject) self.assertTrue(all(r["name"] != event_name for r in results), msg="Deleted documents appearing in global search.") From 1728db8428654815a4732ccebed93b1517edeec6 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 1 Apr 2022 17:50:33 +0530 Subject: [PATCH 23/25] fix(file_manager): Correct and => or usage in refactored query --- frappe/utils/file_manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/file_manager.py b/frappe/utils/file_manager.py index a31577e20b..ce5985f619 100644 --- a/frappe/utils/file_manager.py +++ b/frappe/utils/file_manager.py @@ -321,7 +321,7 @@ def get_file_path(file_name): File = DocType("File") - f = frappe.qb.from_(File).where((File.name == file_name) & (File.file_name == file_name)).select(File.file_url).run() + f = frappe.qb.from_(File).where((File.name == file_name) | (File.file_name == file_name)).select(File.file_url).run() if f: file_name = f[0][0] From 42643c3faa4cc17b39fde8104de81dc55d3b21d5 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Fri, 1 Apr 2022 18:47:05 +0530 Subject: [PATCH 24/25] fix: Follow FIFO while inserting global search record the latest enqueued value should override the value of the existing document --- frappe/utils/global_search.py | 4 +++- frappe/utils/redis_wrapper.py | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/frappe/utils/global_search.py b/frappe/utils/global_search.py index 22938671a6..f2bc9946a1 100644 --- a/frappe/utils/global_search.py +++ b/frappe/utils/global_search.py @@ -355,7 +355,9 @@ def sync_global_search(): :return: """ while frappe.cache().llen('global_search_queue') > 0: - value = json.loads(frappe.cache().lpop('global_search_queue').decode('utf-8')) + # rpop to follow FIFO + # Last one should override all previous contents of same document + value = json.loads(frappe.cache().rpop('global_search_queue').decode('utf-8')) sync_value(value) def sync_value_in_queue(value): diff --git a/frappe/utils/redis_wrapper.py b/frappe/utils/redis_wrapper.py index c40180b538..fbf272e906 100644 --- a/frappe/utils/redis_wrapper.py +++ b/frappe/utils/redis_wrapper.py @@ -138,6 +138,9 @@ class RedisWrapper(redis.Redis): def lpop(self, key): return super(RedisWrapper, self).lpop(self.make_key(key)) + def rpop(self, key): + return super(RedisWrapper, self).rpop(self.make_key(key)) + def llen(self, key): return super(RedisWrapper, self).llen(self.make_key(key)) From aedd2fb2b691a8706a72244b80b86cca3d05c0cb Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Fri, 1 Apr 2022 20:39:27 +0530 Subject: [PATCH 25/25] Revert "perf: 80% faster `doc.get` for fields with `None` as value" (#16490) --- frappe/model/base_document.py | 53 +++++++++-------------------------- frappe/model/document.py | 30 ++++++++++---------- frappe/model/meta.py | 12 ++++++-- 3 files changed, 38 insertions(+), 57 deletions(-) diff --git a/frappe/model/base_document.py b/frappe/model/base_document.py index 05aa0ef2cd..3e9d1317e8 100644 --- a/frappe/model/base_document.py +++ b/frappe/model/base_document.py @@ -3,7 +3,7 @@ import datetime import frappe -from frappe import _, _dict +from frappe import _ from frappe.model import child_table_fields, default_fields, display_fieldtypes, table_fields from frappe.model.naming import set_new_name from frappe.model.utils.link_count import notify_link_count @@ -20,14 +20,6 @@ max_positive_value = { 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"}), -] - def get_controller(doctype): """Returns the **class** object of the given DocType. @@ -154,6 +146,12 @@ class BaseDocument(object): else: value = self.__dict__.get(key, default) + if value is None and key in ( + d.fieldname for d in self.meta.get_table_fields() + ): + value = [] + self.set(key, value) + if limit and isinstance(value, (list, tuple)) and len(value) > limit: value = value[:limit] @@ -191,7 +189,7 @@ class BaseDocument(object): if value is None: value={} if isinstance(value, (dict, BaseDocument)): - if self.__dict__.get(key) is None: + if not self.__dict__.get(key): self.__dict__[key] = [] value = self._init_child(value, key) @@ -254,23 +252,8 @@ class BaseDocument(object): return value - def _get_table_fields(self): - """ - To get table fields during Document init - Meta.get_table_fields goes into recursion for special doctypes - """ - - # child tables don't have child tables - if getattr(self, "parentfield", None): - return () - - if self.doctype == "DocType": - return DOCTYPE_TABLE_FIELDS - - return self.meta.get_table_fields() - def get_valid_dict(self, sanitize=True, convert_dates_to_str=False, ignore_nulls=False, ignore_virtual=False): - d = _dict() + d = frappe._dict() for fieldname in self.meta.get_valid_columns(): d[fieldname] = self.get(fieldname) @@ -331,16 +314,6 @@ 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 df in self._get_table_fields(): - if self.__dict__.get(df.fieldname) is None: - self.__dict__[df.fieldname] = [] - def init_valid_columns(self): for key in default_fields: if key not in self.__dict__: @@ -625,7 +598,7 @@ class BaseDocument(object): if self.meta.istable: for fieldname in ("parent", "parenttype"): if not self.get(fieldname): - missing.append((fieldname, get_msg(_dict(label=fieldname)))) + missing.append((fieldname, get_msg(frappe._dict(label=fieldname)))) return missing @@ -670,7 +643,7 @@ class BaseDocument(object): if not frappe.get_meta(doctype).get('is_virtual'): if not fields_to_fetch: # cache a single value type - values = _dict(name=frappe.db.get_value(doctype, docname, + values = frappe._dict(name=frappe.db.get_value(doctype, docname, 'name', cache=True)) else: values_to_fetch = ['name'] + [_df.fetch_from.split('.')[-1] @@ -972,10 +945,10 @@ class BaseDocument(object): cache_key = parentfield or "main" if not hasattr(self, "_precision"): - self._precision = _dict() + self._precision = frappe._dict() if cache_key not in self._precision: - self._precision[cache_key] = _dict() + self._precision[cache_key] = frappe._dict() if fieldname not in self._precision[cache_key]: self._precision[cache_key][fieldname] = None diff --git a/frappe/model/document.py b/frappe/model/document.py index 0d1b0a6af3..15e9c28d83 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -113,7 +113,6 @@ class Document(BaseDocument): if kwargs: # init base document super(Document, self).__init__(kwargs) - self.init_child_tables() self.init_valid_columns() else: @@ -149,20 +148,20 @@ class Document(BaseDocument): super(Document, self).__init__(d) - 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 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() - self.set(df.fieldname, children) + 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") + if children: + self.set(df.fieldname, children) + else: + self.set(df.fieldname, []) # sometimes __setup__ can depend on child values, hence calling again at the end if hasattr(self, "__setup__"): @@ -850,7 +849,8 @@ class Document(BaseDocument): if parenttype and df.options != parenttype: continue - if value := self.get(df.fieldname): + value = self.get(df.fieldname) + if isinstance(value, list): children.extend(value) return children diff --git a/frappe/model/meta.py b/frappe/model/meta.py index 6cfba382c6..a3d167fb9b 100644 --- a/frappe/model/meta.py +++ b/frappe/model/meta.py @@ -30,7 +30,7 @@ from frappe.model import ( optional_fields, table_fields, ) -from frappe.model.base_document import BaseDocument, DOCTYPE_TABLE_FIELDS +from frappe.model.base_document import BaseDocument from frappe.model.document import Document from frappe.model.workflow import get_workflow_name from frappe.modules import load_doctype_module @@ -180,7 +180,7 @@ class Meta(Document): def get_table_fields(self): if not hasattr(self, "_table_fields"): - if self.name != "DocType": + if self.name!="DocType": self._table_fields = self.get('fields', {"fieldtype": ['in', table_fields]}) else: self._table_fields = DOCTYPE_TABLE_FIELDS @@ -609,6 +609,14 @@ class Meta(Document): def is_nested_set(self): 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"}), +] + ####### def is_single(doctype):