From 8ea6158690e405a46f683c240f62052ed3701fb1 Mon Sep 17 00:00:00 2001 From: Aradhya-Tripathi Date: Thu, 14 Oct 2021 16:13:49 +0530 Subject: [PATCH 01/11] refactor: removed aggregation functions at db level --- frappe/database/database.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index e98cc22f41..88c3241303 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -766,18 +766,6 @@ class Database(object): except Exception: return None - def min(self, dt, fieldname, filters=None, **kwargs): - return self.query.build_conditions(dt, filters=filters).select(Min(Column(fieldname))).run(**kwargs)[0][0] or 0 - - def max(self, dt, fieldname, filters=None, **kwargs): - return self.query.build_conditions(dt, filters=filters).select(Max(Column(fieldname))).run(**kwargs)[0][0] or 0 - - def avg(self, dt, fieldname, filters=None, **kwargs): - return self.query.build_conditions(dt, filters=filters).select(Avg(Column(fieldname))).run(**kwargs)[0][0] or 0 - - def sum(self, dt, fieldname, filters=None, **kwargs): - return self.query.build_conditions(dt, filters=filters).select(Sum(Column(fieldname))).run(**kwargs)[0][0] or 0 - def count(self, dt, filters=None, debug=False, cache=False): """Returns `COUNT(*)` for given DocType and filters.""" if cache and not filters: From 18e2ab7e084447749dd189103a42ab8337ad497e Mon Sep 17 00:00:00 2001 From: Aradhya-Tripathi Date: Thu, 14 Oct 2021 16:54:44 +0530 Subject: [PATCH 02/11] refactor: moved aggregation functions from safe_exec --- frappe/utils/safe_exec.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frappe/utils/safe_exec.py b/frappe/utils/safe_exec.py index 8f6b33a838..a439ef0a8b 100644 --- a/frappe/utils/safe_exec.py +++ b/frappe/utils/safe_exec.py @@ -163,10 +163,6 @@ def get_safe_globals(): get_default=frappe.db.get_default, exists=frappe.db.exists, count=frappe.db.count, - min=frappe.db.min, - max=frappe.db.max, - avg=frappe.db.avg, - sum=frappe.db.sum, escape=frappe.db.escape, sql=read_sql, commit=frappe.db.commit, From 22434d065cf3a4a24f6e0659fa1669892019512c Mon Sep 17 00:00:00 2001 From: Aradhya Date: Fri, 26 Nov 2021 13:09:48 +0530 Subject: [PATCH 03/11] feat: Added aggregation functions to qb functions refactor: changed args to aggregation funcs to match db level aggregation funcs --- frappe/query_builder/functions.py | 39 +++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index 39c67178c2..ddb10831c5 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -2,6 +2,8 @@ from pypika.functions import * from pypika.terms import Function from frappe.query_builder.utils import ImportMapper, db_type_is from frappe.query_builder.custom import GROUP_CONCAT, STRING_AGG, MATCH, TO_TSVECTOR +from frappe.database.query import Query +from .utils import Column class Concat_ws(Function): @@ -22,3 +24,40 @@ Match = ImportMapper( db_type_is.POSTGRES: TO_TSVECTOR } ) + + +def max(dt, fieldname, filters=None, **kwargs): + return ( + Query() + .build_conditions(dt, filters) + .select(Max(Column(fieldname))) + .run(**kwargs)[0][0] + or 0 + ) + +def min(dt, fieldname, filters=None, **kwargs): + return ( + Query() + .build_conditions(dt, filters) + .select(Min(Column(fieldname))) + .run(**kwargs)[0][0] + or 0 + ) + +def avg(dt, fieldname, filters=None, **kwargs): + return ( + Query() + .build_conditions(dt, filters) + .select(Avg(Column(fieldname))) + .run(**kwargs)[0][0] + or 0 + ) + +def sum(dt, fieldname, filters=None, **kwargs): + return ( + Query() + .build_conditions(dt, filters) + .select(Sum(Column(fieldname))) + .run(**kwargs)[0][0] + or 0 + ) \ No newline at end of file From e3bdf110061c4e36fd934e205303d559884247b3 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Fri, 26 Nov 2021 14:19:59 +0530 Subject: [PATCH 04/11] refactor: moved aggregation functions to Query Builder --- frappe/__init__.py | 7 ++++++- frappe/query_builder/__init__.py | 2 +- frappe/query_builder/utils.py | 11 +++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 895bdcaddc..a87f930be6 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -28,7 +28,11 @@ from .exceptions import * from .utils.jinja import (get_jenv, get_template, render_template, get_email_from_template, get_jloader) from .utils.lazy_loader import lazy_import -from frappe.query_builder import get_query_builder, patch_query_execute +from frappe.query_builder import ( + get_query_builder, + patch_query_execute, + patch_query_aggregation, +) __version__ = '14.0.0-dev' @@ -211,6 +215,7 @@ def init(site, sites_path=None, new_site=False): setup_module_map() patch_query_execute() + patch_query_aggregation() local.initialised = True diff --git a/frappe/query_builder/__init__.py b/frappe/query_builder/__init__.py index 4a1fe8fb84..9c7432142f 100644 --- a/frappe/query_builder/__init__.py +++ b/frappe/query_builder/__init__.py @@ -1,2 +1,2 @@ from pypika import * -from frappe.query_builder.utils import Column, DocType, get_query_builder, patch_query_execute +from frappe.query_builder.utils import Column, DocType, get_query_builder, patch_query_execute, patch_query_aggregation diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index 386ddda751..e2916a859c 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -66,3 +66,14 @@ def patch_query_execute(): raise BuilderIdentificationFailed builder_class.run = execute_query + + +def patch_query_aggregation(): + """Patch aggregation functions to frappe.qb + """ + from frappe.query_builder.functions import max, min, avg, sum + + frappe.qb.max = max + frappe.qb.min = min + frappe.qb.avg = avg + frappe.qb.sum = sum \ No newline at end of file From b5c73648dc84cd4d8238080e80757e6a15472f29 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Fri, 26 Nov 2021 15:44:14 +0530 Subject: [PATCH 05/11] refactor: made DRY-er functions --- frappe/query_builder/functions.py | 38 ++++++++++--------------------- frappe/query_builder/utils.py | 10 ++++---- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/frappe/query_builder/functions.py b/frappe/query_builder/functions.py index ddb10831c5..c98df775b7 100644 --- a/frappe/query_builder/functions.py +++ b/frappe/query_builder/functions.py @@ -26,38 +26,24 @@ Match = ImportMapper( ) -def max(dt, fieldname, filters=None, **kwargs): +def _aggregate(function, dt, fieldname, filters, **kwargs): return ( Query() .build_conditions(dt, filters) - .select(Max(Column(fieldname))) + .select(function(Column(fieldname))) .run(**kwargs)[0][0] or 0 ) -def min(dt, fieldname, filters=None, **kwargs): - return ( - Query() - .build_conditions(dt, filters) - .select(Min(Column(fieldname))) - .run(**kwargs)[0][0] - or 0 - ) -def avg(dt, fieldname, filters=None, **kwargs): - return ( - Query() - .build_conditions(dt, filters) - .select(Avg(Column(fieldname))) - .run(**kwargs)[0][0] - or 0 - ) +def _max(dt, fieldname, filters=None, **kwargs): + return _aggregate(Max, dt, fieldname, filters, **kwargs) -def sum(dt, fieldname, filters=None, **kwargs): - return ( - Query() - .build_conditions(dt, filters) - .select(Sum(Column(fieldname))) - .run(**kwargs)[0][0] - or 0 - ) \ No newline at end of file +def _min(dt, fieldname, filters=None, **kwargs): + return _aggregate(Min, dt, fieldname, filters, **kwargs) + +def _avg(dt, fieldname, filters=None, **kwargs): + return _aggregate(Avg, dt, fieldname, filters, **kwargs) + +def _sum(dt, fieldname, filters=None, **kwargs): + return _aggregate(Sum, dt, fieldname, filters, **kwargs) \ No newline at end of file diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index e2916a859c..08768aa11e 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -71,9 +71,9 @@ def patch_query_execute(): def patch_query_aggregation(): """Patch aggregation functions to frappe.qb """ - from frappe.query_builder.functions import max, min, avg, sum + from frappe.query_builder.functions import _max, _min, _avg, _sum - frappe.qb.max = max - frappe.qb.min = min - frappe.qb.avg = avg - frappe.qb.sum = sum \ No newline at end of file + frappe.qb.max = _max + frappe.qb.min = _min + frappe.qb.avg = _avg + frappe.qb.sum = _sum \ No newline at end of file From 5339008fef88cf691a28b0b1b3e23f721ff635d7 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Sat, 27 Nov 2021 14:35:58 +0530 Subject: [PATCH 06/11] feat: Added patch for replacing db level aggregation calls --- frappe/patches/v14_0/remove_db_aggregation.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 frappe/patches/v14_0/remove_db_aggregation.py diff --git a/frappe/patches/v14_0/remove_db_aggregation.py b/frappe/patches/v14_0/remove_db_aggregation.py new file mode 100644 index 0000000000..373cfaf7db --- /dev/null +++ b/frappe/patches/v14_0/remove_db_aggregation.py @@ -0,0 +1,25 @@ +import frappe +import re + + +def execute(): + _sub_aggregation("frappe.db.max", "frappe.qb.max") + _sub_aggregation("frappe.db.min", "frappe.qb.min") + _sub_aggregation("frappe.db.sum", "frappe.qb.sum") + _sub_aggregation("frappe.db.avg", "frappe.qb.avg") + + +def _sub_aggregation(function, subtitution): + scripts = frappe.get_all( + "Server Script", + filters={"script": ("like", f"%{function}%")}, + fields=["name", "script"], + ) + for script in scripts: + script.update( + {"script": re.sub(f"{function}", f"{subtitution}", script["script"])} + ) + for script in scripts: + frappe.db.update( + "Server Script", {"name": script["name"]}, "script", script["script"] + ) From 4cae147aed134c358ba1b1ab9eb854aa17d45206 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 29 Nov 2021 09:23:52 +0530 Subject: [PATCH 07/11] fix: remove duplicate parent when child item option selected (backport #15101) (#15110) Co-authored-by: Bhavesh Maheshwari --- frappe/public/js/frappe/form/multi_select_dialog.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/form/multi_select_dialog.js b/frappe/public/js/frappe/form/multi_select_dialog.js index 37b7e08a80..161e4196b0 100644 --- a/frappe/public/js/frappe/form/multi_select_dialog.js +++ b/frappe/public/js/frappe/form/multi_select_dialog.js @@ -325,7 +325,9 @@ frappe.ui.form.MultiSelectDialog = class MultiSelectDialog { let parent_names = this.child_datatable.rowmanager.checkMap.reduce((parent_names, checked, index) => { if (checked == 1) { const parent_name = this.child_results[index].parent; - parent_names.push(parent_name); + if (!parent_names.includes(parent_name)) { + parent_names.push(parent_name); + } } return parent_names; }, []); From e503d8117303eb787c0099f02d91b89eb23b6ce4 Mon Sep 17 00:00:00 2001 From: Aradhya Date: Mon, 29 Nov 2021 13:07:31 +0530 Subject: [PATCH 08/11] perf: reduced no. of db calls --- frappe/patches/v14_0/remove_db_aggregation.py | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/frappe/patches/v14_0/remove_db_aggregation.py b/frappe/patches/v14_0/remove_db_aggregation.py index 373cfaf7db..231acccc6c 100644 --- a/frappe/patches/v14_0/remove_db_aggregation.py +++ b/frappe/patches/v14_0/remove_db_aggregation.py @@ -1,24 +1,37 @@ +from frappe.query_builder import DocType import frappe import re def execute(): - _sub_aggregation("frappe.db.max", "frappe.qb.max") - _sub_aggregation("frappe.db.min", "frappe.qb.min") - _sub_aggregation("frappe.db.sum", "frappe.qb.sum") - _sub_aggregation("frappe.db.avg", "frappe.qb.avg") + sub_aggregation() -def _sub_aggregation(function, subtitution): - scripts = frappe.get_all( - "Server Script", - filters={"script": ("like", f"%{function}%")}, - fields=["name", "script"], - ) - for script in scripts: - script.update( - {"script": re.sub(f"{function}", f"{subtitution}", script["script"])} +def sub_aggregation(): + server_scripts = DocType("Server Script") + scripts = ( + frappe.qb.from_(server_scripts) + .where( + server_scripts.script.like("%frappe.db.max%") + | server_scripts.script.like("%frappe.db.min%") + | server_scripts.script.like("%frappe.db.sum%") + | server_scripts.script.like("%frappe.db.avg%") ) + .select("name", "script") + .run(as_dict=True) + ) + + def _sub_aggregation(scripts, function, substitution): + for script in scripts: + script.update( + {"script": re.sub(f"{function}", f"{substitution}", script["script"])} + ) + + _sub_aggregation(scripts, "frappe.db.max", "frappe.qb.max") + _sub_aggregation(scripts, "frappe.db.min", "frappe.qb.min") + _sub_aggregation(scripts, "frappe.db.sum", "frappe.qb.sum") + _sub_aggregation(scripts, "frappe.db.avg", "frappe.qb.avg") + for script in scripts: frappe.db.update( "Server Script", {"name": script["name"]}, "script", script["script"] From 43fc713dd7ef881f8f76632b266965d94c98de21 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 29 Nov 2021 13:46:47 +0530 Subject: [PATCH 09/11] refactor: Remove Aggregation methods from DB API * Make it DRY & make it "better" * Add to patches.txt so the patch runs :') * Style fixes - tabs > spaces for consistency, removed unnecessary blocks --- frappe/patches.txt | 1 + frappe/patches/v14_0/remove_db_aggregation.py | 54 +++++++++---------- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/frappe/patches.txt b/frappe/patches.txt index b230c336b4..3078159c3d 100644 --- a/frappe/patches.txt +++ b/frappe/patches.txt @@ -186,3 +186,4 @@ frappe.patches.v14_0.rename_cancelled_documents frappe.patches.v14_0.copy_mail_data #08.03.21 frappe.patches.v14_0.update_workspace2 # 20.09.2021 frappe.patches.v14_0.update_github_endpoints #08-11-2021 +frappe.patches.v14_0.remove_db_aggregation diff --git a/frappe/patches/v14_0/remove_db_aggregation.py b/frappe/patches/v14_0/remove_db_aggregation.py index 231acccc6c..25a170f362 100644 --- a/frappe/patches/v14_0/remove_db_aggregation.py +++ b/frappe/patches/v14_0/remove_db_aggregation.py @@ -1,38 +1,32 @@ -from frappe.query_builder import DocType -import frappe import re +import frappe +from frappe.query_builder import DocType + def execute(): - sub_aggregation() + """Replace temporarily available Database Aggregate APIs on frappe (develop) + APIs changed: + * frappe.db.max => frappe.qb.max + * frappe.db.min => frappe.qb.min + * frappe.db.sum => frappe.qb.sum + * frappe.db.avg => frappe.qb.avg + """ + ServerScript = DocType("Server Script") + server_scripts = frappe.qb.from_(ServerScript).where( + ServerScript.script.like("%frappe.db.max(%") + | ServerScript.script.like("%frappe.db.min(%") + | ServerScript.script.like("%frappe.db.sum(%") + | ServerScript.script.like("%frappe.db.avg(%") + ).select( + "name", "script" + ).run(as_dict=True) -def sub_aggregation(): - server_scripts = DocType("Server Script") - scripts = ( - frappe.qb.from_(server_scripts) - .where( - server_scripts.script.like("%frappe.db.max%") - | server_scripts.script.like("%frappe.db.min%") - | server_scripts.script.like("%frappe.db.sum%") - | server_scripts.script.like("%frappe.db.avg%") - ) - .select("name", "script") - .run(as_dict=True) - ) + for server_script in server_scripts: + name, script = server_script["name"], server_script["script"] - def _sub_aggregation(scripts, function, substitution): - for script in scripts: - script.update( - {"script": re.sub(f"{function}", f"{substitution}", script["script"])} - ) + for agg in ["avg", "max", "min", "sum"]: + script = re.sub(f"frappe.db.{agg}(", f"frappe.qb.{agg}(", script) - _sub_aggregation(scripts, "frappe.db.max", "frappe.qb.max") - _sub_aggregation(scripts, "frappe.db.min", "frappe.qb.min") - _sub_aggregation(scripts, "frappe.db.sum", "frappe.qb.sum") - _sub_aggregation(scripts, "frappe.db.avg", "frappe.qb.avg") - - for script in scripts: - frappe.db.update( - "Server Script", {"name": script["name"]}, "script", script["script"] - ) + frappe.db.update("Server Script", name, "script", script) From 0e32d52e3a0a75500ac59f8fe5c875b37138009f Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 29 Nov 2021 16:38:15 +0530 Subject: [PATCH 10/11] fix: Use separate API to insert route history --- frappe/deferred_insert.py | 1 - .../doctype/route_history/route_history.py | 20 +++++++++++++++++++ frappe/public/js/frappe/router_history.js | 12 +++++------ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/frappe/deferred_insert.py b/frappe/deferred_insert.py index 499fc5e41b..b1338a73b0 100644 --- a/frappe/deferred_insert.py +++ b/frappe/deferred_insert.py @@ -5,7 +5,6 @@ from frappe.utils import cstr queue_prefix = 'insert_queue_for_' -@frappe.whitelist() def deferred_insert(doctype, records): frappe.cache().rpush(queue_prefix + doctype, records) diff --git a/frappe/desk/doctype/route_history/route_history.py b/frappe/desk/doctype/route_history/route_history.py index 01184fcc3a..489a3bd50a 100644 --- a/frappe/desk/doctype/route_history/route_history.py +++ b/frappe/desk/doctype/route_history/route_history.py @@ -1,9 +1,13 @@ # Copyright (c) 2021, Frappe Technologies and contributors # License: MIT. See LICENSE +import json + import frappe +from frappe.deferred_insert import deferred_insert from frappe.model.document import Document + class RouteHistory(Document): pass @@ -35,3 +39,19 @@ def flush_old_route_records(): "modified": ("<=", last_record_to_keep[0].modified), "user": user }) + +@frappe.whitelist() +def deferred_insert_route_history(routes): + routes_record = [] + + if isinstance(routes, str): + routes = json.loads(routes) + + for route_doc in routes: + routes_record.append({ + "user": frappe.session.user, + "route": route_doc.get("route"), + "creation": route_doc.get("creation") + }) + + deferred_insert("Route History", json.dumps(routes_record)) diff --git a/frappe/public/js/frappe/router_history.js b/frappe/public/js/frappe/router_history.js index c64c3fc9f2..1fda6f4a1b 100644 --- a/frappe/public/js/frappe/router_history.js +++ b/frappe/public/js/frappe/router_history.js @@ -5,13 +5,14 @@ const save_routes = frappe.utils.debounce(() => { if (frappe.session.user === 'Guest') return; const routes = frappe.route_history_queue; frappe.route_history_queue = []; - - frappe.xcall('frappe.deferred_insert.deferred_insert', { - 'doctype': 'Route History', - 'records': routes + + if (!routes.length) return; + + frappe.xcall('frappe.desk.doctype.route_history.route_history.deferred_insert_route_history', { + 'routes': routes }).catch(() => { frappe.route_history_queue.concat(routes); - }); + }); }, 10000); @@ -19,7 +20,6 @@ frappe.router.on('change', () => { const route = frappe.get_route(); if (is_route_useful(route)) { frappe.route_history_queue.push({ - 'user': frappe.session.user, 'creation': frappe.datetime.now_datetime(), 'route': frappe.get_route_str() }); From aa40abadb1277636e1f79d1ff787a7d615e218d5 Mon Sep 17 00:00:00 2001 From: Suraj Shetty Date: Mon, 29 Nov 2021 17:06:29 +0530 Subject: [PATCH 11/11] refactor: Simplify method naming --- frappe/desk/doctype/route_history/route_history.py | 6 +++--- frappe/public/js/frappe/router_history.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frappe/desk/doctype/route_history/route_history.py b/frappe/desk/doctype/route_history/route_history.py index 489a3bd50a..fc87312950 100644 --- a/frappe/desk/doctype/route_history/route_history.py +++ b/frappe/desk/doctype/route_history/route_history.py @@ -4,7 +4,7 @@ import json import frappe -from frappe.deferred_insert import deferred_insert +from frappe.deferred_insert import deferred_insert as _deferred_insert from frappe.model.document import Document @@ -41,7 +41,7 @@ def flush_old_route_records(): }) @frappe.whitelist() -def deferred_insert_route_history(routes): +def deferred_insert(routes): routes_record = [] if isinstance(routes, str): @@ -54,4 +54,4 @@ def deferred_insert_route_history(routes): "creation": route_doc.get("creation") }) - deferred_insert("Route History", json.dumps(routes_record)) + _deferred_insert("Route History", json.dumps(routes_record)) diff --git a/frappe/public/js/frappe/router_history.js b/frappe/public/js/frappe/router_history.js index 1fda6f4a1b..fb2d5790da 100644 --- a/frappe/public/js/frappe/router_history.js +++ b/frappe/public/js/frappe/router_history.js @@ -8,7 +8,7 @@ const save_routes = frappe.utils.debounce(() => { if (!routes.length) return; - frappe.xcall('frappe.desk.doctype.route_history.route_history.deferred_insert_route_history', { + frappe.xcall('frappe.desk.doctype.route_history.route_history.deferred_insert', { 'routes': routes }).catch(() => { frappe.route_history_queue.concat(routes);