From a7c706672f4eb28bdaecb6109e042070db1da188 Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Wed, 25 May 2022 01:12:48 +0200 Subject: [PATCH 01/21] fix: allow All to select a User --- frappe/core/doctype/user/user.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frappe/core/doctype/user/user.json b/frappe/core/doctype/user/user.json index 42122ebfda..82e3fa71f3 100644 --- a/frappe/core/doctype/user/user.json +++ b/frappe/core/doctype/user/user.json @@ -722,7 +722,7 @@ "link_fieldname": "user" } ], - "modified": "2022-03-09 01:47:56.745069", + "modified": "2022-05-25 01:00:51.345319", "modified_by": "Administrator", "module": "Core", "name": "User", @@ -747,6 +747,10 @@ "read": 1, "role": "System Manager", "write": 1 + }, + { + "role": "All", + "select": 1 } ], "quick_entry": 1, From cdc850f12eb52214fb7d4d6d37b3f5fbc75ee08c Mon Sep 17 00:00:00 2001 From: barredterra <14891507+barredterra@users.noreply.github.com> Date: Fri, 10 Jun 2022 16:30:20 +0200 Subject: [PATCH 02/21] test: user permissions affecting User --- frappe/tests/test_permissions.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/frappe/tests/test_permissions.py b/frappe/tests/test_permissions.py index 4164b0be36..26d5c714ef 100644 --- a/frappe/tests/test_permissions.py +++ b/frappe/tests/test_permissions.py @@ -672,3 +672,31 @@ class TestPermissions(FrappeTestCase): doctype="Has Role", parent_doctype="Has Role", ) + + def test_select_user(self): + """If test3@example.com is restricted by a User Permission to see only + users linked to a certain doctype (in this case: Gender "Female"), he + should not be able to query other users (Gender "Male"). + """ + # ensure required genders exist + for gender in ("Male", "Female"): + if frappe.db.exists("Gender", gender): + continue + + frappe.get_doc({"doctype": "Gender", "gender": gender}).insert() + + # asssign gender to test users + frappe.db.set_value("User", "test1@example.com", "gender", "Male") + frappe.db.set_value("User", "test2@example.com", "gender", "Female") + frappe.db.set_value("User", "test3@example.com", "gender", "Female") + + # restrict test3@example.com to see only female users + add_user_permission("Gender", "Female", "test3@example.com") + + # become user test3@example.com and see what users he can query + frappe.set_user("test3@example.com") + users = frappe.get_list("User", pluck="name") + + self.assertNotIn("test1@example.com", users) + self.assertIn("test2@example.com", users) + self.assertIn("test3@example.com", users) From 792c1451e7eac6653c06d67f377a00cfd52ca686 Mon Sep 17 00:00:00 2001 From: Deepesh Garg Date: Mon, 13 Jun 2022 16:39:15 +0530 Subject: [PATCH 03/21] fix: Force system admin role only if active --- frappe/core/doctype/user/user.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/frappe/core/doctype/user/user.py b/frappe/core/doctype/user/user.py index 9c127d9eca..531fd316b1 100644 --- a/frappe/core/doctype/user/user.py +++ b/frappe/core/doctype/user/user.py @@ -163,6 +163,9 @@ class User(Document): toggle_notifications(self.name, enable=cint(self.enabled)) def add_system_manager_role(self): + if self.is_system_manager_disabled(): + return + # if adding system manager, do nothing if not cint(self.enabled) or ( "System Manager" in [user_role.role for user_role in self.get("roles")] @@ -189,6 +192,9 @@ class User(Document): ], ) + def is_system_manager_disabled(self): + return frappe.db.get_value("Role", {"name": "System Manager"}, ["disabled"]) + def email_new_password(self, new_password=None): if new_password and not self.flags.in_insert: _update_password(user=self.name, pwd=new_password, logout_all_sessions=self.logout_all_sessions) @@ -372,6 +378,9 @@ class User(Document): ) def a_system_manager_should_exist(self): + if self.is_system_manager_disabled(): + return + if not self.get_other_system_managers(): throw(_("There should remain at least one System Manager")) From 963648667d5a03cb80160577e565814a050565b4 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 15:41:55 +0530 Subject: [PATCH 04/21] refactor: filter_dynamic_link_doctypes API * Added typing, better variable naming * Remove unnecessary re-iterations * Optimize queries and membership processing --- frappe/contacts/address_and_contact.py | 40 +++++++++++++++----------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/frappe/contacts/address_and_contact.py b/frappe/contacts/address_and_contact.py index 036594926e..a0f742c55a 100644 --- a/frappe/contacts/address_and_contact.py +++ b/frappe/contacts/address_and_contact.py @@ -3,6 +3,7 @@ import functools import re +from typing import Dict, List import frappe from frappe import _ @@ -169,29 +170,34 @@ def delete_contact_and_address(doctype, docname): @frappe.whitelist() @frappe.validate_and_sanitize_search_inputs -def filter_dynamic_link_doctypes(doctype, txt, searchfield, start, page_len, filters): - if not txt: - txt = "" +def filter_dynamic_link_doctypes(txt: str, filters: Dict) -> List[List[str]]: + from frappe.permissions import get_doctypes_with_read - doctypes = frappe.db.get_all( - "DocField", filters=filters, fields=["parent"], distinct=True, as_list=True + txt = txt or "" + filters = filters or {} + TXT_PATTERN = re.compile(f"{txt}.*") + + _doctypes_from_df = frappe.get_all( + "DocField", + filters=filters, + pluck="parent", + distinct=True, + order_by=None, ) + doctypes_from_df = {d for d in _doctypes_from_df if TXT_PATTERN.search(_(d), re.IGNORECASE)} - doctypes = tuple(d for d in doctypes if re.search(txt + ".*", _(d[0]), re.IGNORECASE)) + filters.update({"dt": ("not in", doctypes_from_df)}) + _doctypes_from_cdf = frappe.get_all( + "Custom Field", filters=filters, pluck="dt", distinct=True, order_by=None + ) + doctypes_from_cdf = {d for d in _doctypes_from_cdf if TXT_PATTERN.search(_(d), re.IGNORECASE)} - filters.update({"dt": ("not in", [d[0] for d in doctypes])}) + all_doctypes = doctypes_from_df.union(doctypes_from_cdf) + allowed_doctypes = set(get_doctypes_with_read()) - _doctypes = frappe.db.get_all("Custom Field", filters=filters, fields=["dt"], as_list=True) + valid_doctypes = sorted(all_doctypes.intersection(allowed_doctypes)) - _doctypes = tuple([d for d in _doctypes if re.search(txt + ".*", _(d[0]), re.IGNORECASE)]) - - all_doctypes = [d[0] for d in doctypes + _doctypes] - allowed_doctypes = frappe.permissions.get_doctypes_with_read() - - valid_doctypes = sorted(set(all_doctypes).intersection(set(allowed_doctypes))) - valid_doctypes = [[doctype] for doctype in valid_doctypes] - - return valid_doctypes + return [[doctype] for doctype in valid_doctypes] def set_link_title(doc): From 13cf4964a6b921ed7c0ffe2d3daf163c6e3cd430 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 16:06:12 +0530 Subject: [PATCH 05/21] perf: Check query type via is_query_type --- frappe/database/database.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 1de22af037..7aafa3b7f0 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -29,6 +29,10 @@ SINGLE_WORD_PATTERN = re.compile(r'([`"]?)(tab([A-Z]\w+))\1') MULTI_WORD_PATTERN = re.compile(r'([`"])(tab([A-Z]\w+)( [A-Z]\w+)+)\1') +def is_query_type(query: str, query_type: Union[str, Tuple[str]]) -> bool: + return query.lstrip().split(maxsplit=1)[0].lower().startswith(query_type) + + class Database(object): """ Open a database connection with the given parmeters, if use_default is True, use the @@ -248,7 +252,7 @@ class Database(object): # debug if debug: - if explain and query.strip().lower().startswith("select"): + if explain and is_query_type(query, "select"): self.explain_query(query, values) frappe.errprint(self.mogrify(query, values)) @@ -305,7 +309,7 @@ class Database(object): could cause the system to hang.""" self.check_implicit_commit(query) - if query and query.strip().lower() in ("commit", "rollback"): + if query and is_query_type(query, ("commit", "rollback")): self.transaction_writes = 0 if query[:6].lower() in ("update", "insert", "delete"): @@ -322,8 +326,7 @@ class Database(object): if ( self.transaction_writes and query - and query.strip().split()[0].lower() - in ["start", "alter", "drop", "create", "begin", "truncate"] + and is_query_type(query, ("start", "alter", "drop", "create", "begin", "truncate")) ): raise Exception("This statement can cause implicit commit") @@ -346,7 +349,7 @@ class Database(object): @staticmethod def clear_db_table_cache(query): - if query and query.strip().split()[0].lower() in {"drop", "create"}: + if query and is_query_type(query, ("drop", "create")): frappe.cache().delete_key("db_tables") @staticmethod @@ -1207,7 +1210,7 @@ class Database(object): def log_touched_tables(self, query, values=None): if values: query = frappe.safe_decode(self._cursor.mogrify(query, values)) - if query.strip().lower().split()[0] in ("insert", "delete", "update", "alter", "drop", "rename"): + if is_query_type(query, ("insert", "delete", "update", "alter", "drop", "rename")): # single_word_regex is designed to match following patterns # `tabXxx`, tabXxx and "tabXxx" From a1691784a87b239f4efdc2e0ef62013cadca16e9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 21:20:39 +0530 Subject: [PATCH 06/21] chore: Drop duplicate get_frontmatter definition --- frappe/website/router.py | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/frappe/website/router.py b/frappe/website/router.py index e9f0d0f09c..8c21501a4e 100644 --- a/frappe/website/router.py +++ b/frappe/website/router.py @@ -8,7 +8,7 @@ import re from werkzeug.routing import Map, NotFound, Rule import frappe -from frappe.website.utils import extract_title +from frappe.website.utils import extract_title, get_frontmatter def get_page_info_from_web_page_with_dynamic_routes(path): @@ -161,26 +161,6 @@ def get_page_info(path, app, start, basepath=None, app_path=None, fname=None): return page_info -def get_frontmatter(string): - """ - Reference: https://github.com/jonbeebe/frontmatter - """ - import yaml - - fmatter = "" - body = "" - result = re.compile(r"^\s*(?:---|\+\+\+)(.*?)(?:---|\+\+\+)\s*(.+)$", re.S | re.M).search(string) - - if result: - fmatter = result.group(1) - body = result.group(2) - - return { - "attributes": yaml.safe_load(fmatter), - "body": body, - } - - def setup_source(page_info): """Get the HTML source of the template""" jenv = frappe.get_jenv() From 7e25cc4568073ff4696b75858d0d9d39adbb566c Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 22:06:46 +0530 Subject: [PATCH 07/21] perf: Login Page Improves performance 3x - from 0.047s to 0.017s * Use frappe.get_*_settings to query table once * Use cached LDAP Settings' document via get_ldap_client_settings * Use single get_all to query all Social Login providers and related data * Skip provider if client_secret doesn't exist --- .../doctype/ldap_settings/ldap_settings.py | 2 +- frappe/www/login.py | 55 +++++++++---------- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/frappe/integrations/doctype/ldap_settings/ldap_settings.py b/frappe/integrations/doctype/ldap_settings/ldap_settings.py index a14124234f..96007ee918 100644 --- a/frappe/integrations/doctype/ldap_settings/ldap_settings.py +++ b/frappe/integrations/doctype/ldap_settings/ldap_settings.py @@ -120,7 +120,7 @@ class LDAPSettings(Document): def get_ldap_client_settings(): # return the settings to be used on the client side. result = {"enabled": False} - ldap = frappe.get_doc("LDAP Settings") + ldap = frappe.get_cached_doc("LDAP Settings") if ldap.enabled: result["enabled"] = True result["method"] = "frappe.integrations.doctype.ldap_settings.ldap_settings.login" diff --git a/frappe/www/login.py b/frappe/www/login.py index 1b9a8c239a..119dfefcd7 100644 --- a/frappe/www/login.py +++ b/frappe/www/login.py @@ -36,22 +36,14 @@ def get_context(context): frappe.local.flags.redirect_location = redirect_to raise frappe.Redirect - # get settings from site config context.no_header = True context.for_test = "login.html" context["title"] = "Login" context["provider_logins"] = [] - context["disable_signup"] = frappe.utils.cint( - frappe.db.get_single_value("Website Settings", "disable_signup") - ) - context["logo"] = ( - frappe.db.get_single_value("Website Settings", "app_logo") - or frappe.get_hooks("app_logo_url")[-1] - ) + context["disable_signup"] = frappe.utils.cint(frappe.get_website_settings("disable_signup")) + context["logo"] = frappe.get_website_settings("app_logo") or frappe.get_hooks("app_logo_url")[-1] context["app_name"] = ( - frappe.db.get_single_value("Website Settings", "app_name") - or frappe.get_system_settings("app_name") - or _("Frappe") + frappe.get_website_settings("app_name") or frappe.get_system_settings("app_name") or _("Frappe") ) signup_form_template = frappe.get_hooks("signup_form_template") @@ -61,38 +53,41 @@ def get_context(context): path = frappe.get_attr(signup_form_template[-1])() else: path = "frappe/templates/signup.html" + if path: context["signup_form_template"] = frappe.get_template(path).render() - providers = [ - i.name - for i in frappe.get_all("Social Login Key", filters={"enable_social_login": 1}, order_by="name") - ] + providers = frappe.get_all( + "Social Login Key", + filters={"enable_social_login": 1}, + fields=["name", "client_id", "base_url", "provider_name", "icon"], + order_by="name", + ) + for provider in providers: - client_id, base_url = frappe.get_value("Social Login Key", provider, ["client_id", "base_url"]) - client_secret = get_decrypted_password("Social Login Key", provider, "client_secret") - provider_name = frappe.get_value("Social Login Key", provider, "provider_name") + client_secret = get_decrypted_password("Social Login Key", provider.name, "client_secret") + if not client_secret: + continue icon = None - icon_url = frappe.get_value("Social Login Key", provider, "icon") - if icon_url: - if provider_name != "Custom": - icon = "{1}".format(icon_url, provider_name) + if provider.icon: + if provider.provider_name == "Custom": + icon = get_icon_html(provider.icon, small=True) else: - icon = get_icon_html(icon_url, small=True) + icon = f"{provider.provider_name}" - if get_oauth_keys(provider) and client_secret and client_id and base_url: + if provider.client_id and provider.base_url and get_oauth_keys(provider.name): context.provider_logins.append( { - "name": provider, - "provider_name": provider_name, - "auth_url": get_oauth2_authorize_url(provider, redirect_to), + "name": provider.name, + "provider_name": provider.provider_name, + "auth_url": get_oauth2_authorize_url(provider.name, redirect_to), "icon": icon, } ) context["social_login"] = True - ldap_settings = LDAPSettings.get_ldap_client_settings() - context["ldap_settings"] = ldap_settings + + context["ldap_settings"] = LDAPSettings.get_ldap_client_settings() login_label = [_("Email")] @@ -102,7 +97,7 @@ def get_context(context): if frappe.utils.cint(frappe.get_system_settings("allow_login_using_user_name")): login_label.append(_("Username")) - context["login_label"] = " {0} ".format(_("or")).join(login_label) + context["login_label"] = f" {_('or')} ".join(login_label) return context From ce38587a4e14df9a89ec5c0a055060b464e1b6d9 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 22:22:21 +0530 Subject: [PATCH 08/21] perf: About Us Settings Use cached document for building /about page --- frappe/www/about.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/www/about.py b/frappe/www/about.py index fd2331d9f6..a233bfd311 100644 --- a/frappe/www/about.py +++ b/frappe/www/about.py @@ -7,6 +7,6 @@ sitemap = 1 def get_context(context): - context.doc = frappe.get_doc("About Us Settings", "About Us Settings") + context.doc = frappe.get_cached_doc("About Us Settings") return context From 3871fe6cd042587abbd3d722f97860a6c006e0a2 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 22:29:05 +0530 Subject: [PATCH 09/21] perf: App Page Reduced time taken for get_context to execute from 0.035s to 0.02s (75% reduction) --- frappe/sessions.py | 2 +- frappe/www/app.py | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/frappe/sessions.py b/frappe/sessions.py index 6e0ce73732..67b58e1d89 100644 --- a/frappe/sessions.py +++ b/frappe/sessions.py @@ -187,7 +187,7 @@ def get(): bootinfo["translated_search_doctypes"] = frappe.get_hooks("translated_search_doctypes") bootinfo["disable_async"] = frappe.conf.disable_async - bootinfo["setup_complete"] = cint(frappe.db.get_single_value("System Settings", "setup_complete")) + bootinfo["setup_complete"] = cint(frappe.get_system_settings("setup_complete")) bootinfo["desk_theme"] = frappe.db.get_value("User", frappe.session.user, "desk_theme") or "Light" diff --git a/frappe/www/app.py b/frappe/www/app.py index f1b62a0899..9a8c80d6b1 100644 --- a/frappe/www/app.py +++ b/frappe/www/app.py @@ -32,8 +32,6 @@ def get_context(context): frappe.db.commit() - desk_theme = frappe.db.get_value("User", frappe.session.user, "desk_theme") - boot_json = frappe.as_json(boot) # remove script tags from boot @@ -52,7 +50,7 @@ def get_context(context): "lang": frappe.local.lang, "sounds": hooks["sounds"], "boot": boot if context.get("for_mobile") else boot_json, - "desk_theme": desk_theme or "Light", + "desk_theme": boot.get("desk_theme") or "Light", "csrf_token": csrf_token, "google_analytics_id": frappe.conf.get("google_analytics_id"), "google_analytics_anonymize_ip": frappe.conf.get("google_analytics_anonymize_ip"), From 64e52737646d1661e0d295868fc85aa0da47b1f2 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 10 Jun 2022 17:42:47 +0530 Subject: [PATCH 10/21] perf: Patch qb only once - not on every init --- frappe/__init__.py | 7 +++++-- frappe/query_builder/utils.py | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 51c6ba3a74..37a03e412c 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -44,6 +44,7 @@ local = Local() STANDARD_USERS = ("Guest", "Administrator") _dev_server = int(sbool(os.environ.get("DEV_SERVER", False))) +_qb_patched = False if _dev_server: warnings.simplefilter("always", DeprecationWarning) @@ -236,8 +237,10 @@ def init(site, sites_path=None, new_site=False): local.qb = get_query_builder(local.conf.db_type or "mariadb") setup_module_map() - patch_query_execute() - patch_query_aggregation() + + if not _qb_patched: + patch_query_execute() + patch_query_aggregation() local.initialised = True diff --git a/frappe/query_builder/utils.py b/frappe/query_builder/utils.py index 69aee9b350..c75e380d49 100644 --- a/frappe/query_builder/utils.py +++ b/frappe/query_builder/utils.py @@ -64,7 +64,6 @@ def patch_query_execute(): This excludes the use of `frappe.db.sql` method while executing the query object """ - from frappe.utils.safe_exec import check_safe_sql_query def execute_query(query, *args, **kwargs): query, params = prepare_query(query) @@ -73,6 +72,8 @@ def patch_query_execute(): def prepare_query(query): import inspect + from frappe.utils.safe_exec import check_safe_sql_query + param_collector = NamedParameterWrapper() query = query.get_sql(param_wrapper=param_collector) if frappe.flags.in_safe_exec and not check_safe_sql_query(query, throw=False): @@ -103,6 +104,7 @@ def patch_query_execute(): builder_class.run = execute_query builder_class.walk = prepare_query + frappe._qb_patched = True def patch_query_aggregation(): @@ -113,3 +115,4 @@ def patch_query_aggregation(): frappe.qb.min = _min frappe.qb.avg = _avg frappe.qb.sum = _sum + frappe._qb_patched = True From e681233e982c7eb44f4b2df86ed49ba763066c03 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Fri, 3 Jun 2022 22:05:33 +0530 Subject: [PATCH 11/21] perf: Fetch and cache entire settings' dicts --- frappe/__init__.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 37a03e412c..7c71bc8815 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -871,6 +871,10 @@ def clear_cache(user=None, doctype=None): local.role_permissions = {} if hasattr(local, "request_cache"): local.request_cache.clear() + if hasattr(local, "system_settings"): + del local.system_settings + if hasattr(local, "website_settings"): + del local.website_settings def only_has_select_perm(doctype, user=None, ignore_permissions=False): @@ -1094,6 +1098,10 @@ def clear_document_cache(doctype, name): if key in local.document_cache: del local.document_cache[key] cache().hdel("document_cache", key) + if doctype == "System Settings" and hasattr(local, "system_settings"): + delattr(local, "system_settings") + if doctype == "Website Settings" and hasattr(local, "website_settings"): + delattr(local, "website_settings") def get_cached_value(doctype, name, fieldname="name", as_dict=False): @@ -2206,8 +2214,18 @@ def safe_eval(code, eval_globals=None, eval_locals=None): return eval(code, eval_globals, eval_locals) +def get_website_settings(key): + if not hasattr(local, "website_settings"): + local.website_settings = db.get_singles_dict("Website Settings") + + return local.website_settings[key] + + def get_system_settings(key): - return db.get_single_value("System Settings", key, cache=True) + if not hasattr(local, "system_settings"): + local.system_settings = db.get_singles_dict("System Settings") + + return local.system_settings[key] def get_active_domains(): From f74dc5023d5ab1598e80a586b656b34e18a5ec0c Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 13 Jun 2022 11:52:36 +0530 Subject: [PATCH 12/21] refactor!: frappe.db.get_singles_dict * Cast single's values as their fieldtypes before returning * Support previously dead debug parameter * Consider single with no meta as non-existent; skip query Decided to go ahead with the breaking change given the nature of the existing usages of get_singles_dict :crie: --- frappe/database/database.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 7aafa3b7f0..39ca846904 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.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 # Database Module @@ -18,6 +18,7 @@ import frappe import frappe.defaults import frappe.model.meta from frappe import _ +from frappe.exceptions import DoesNotExistError from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count from frappe.query_builder.utils import DocType @@ -628,14 +629,28 @@ class Database(object): # Get coulmn and value of the single doctype Accounts Settings account_settings = frappe.db.get_singles_dict("Accounts Settings") """ - result = self.query.get_sql( + return_value = frappe._dict() + + try: + meta = frappe.get_meta(doctype) + except DoesNotExistError: + return return_value + + queried_result = self.query.get_sql( "Singles", filters={"doctype": doctype}, fields=["field", "value"], for_update=for_update, - ).run() + ).run(debug=debug) - return frappe._dict(result) + for fieldname, value in queried_result: + if df := meta.get_field(fieldname): + casted_value = cast(df.fieldtype, value) + else: + casted_value = value + return_value[fieldname] = casted_value + + return return_value @staticmethod def get_all(*args, **kwargs): From 601217a4a233ce666eff5388159b7d72da75f231 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 13 Jun 2022 19:14:48 +0530 Subject: [PATCH 13/21] ci: Run tests bypassing roulette with labels "Run UI Tests", "Run Server Tests" --- .github/helper/roulette.py | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/.github/helper/roulette.py b/.github/helper/roulette.py index f68ef5046f..9165198012 100644 --- a/.github/helper/roulette.py +++ b/.github/helper/roulette.py @@ -5,8 +5,10 @@ import shlex import subprocess import sys import urllib.request +from functools import cache +@cache def fetch_pr_data(pr_number, repo, endpoint): api_url = f"https://api.github.com/repos/{repo}/pulls/{pr_number}" @@ -26,7 +28,16 @@ def get_output(command, shell=True): return subprocess.check_output(command, shell=shell, encoding="utf8").strip() def has_skip_ci_label(pr_number, repo="frappe/frappe"): - return any([label["name"] for label in fetch_pr_data(pr_number, repo, "")["labels"] if label["name"] == "Skip CI"]) + return has_label(pr_number, "Skip CI", repo) + +def has_run_server_tests_label(pr_number, repo="frappe/frappe"): + return has_label(pr_number, "Run Server Tests", repo) + +def has_run_ui_tests_label(pr_number, repo="frappe/frappe"): + return has_label(pr_number, "Run UI Tests", repo) + +def has_label(pr_number, label, repo="frappe/frappe"): + return any([label["name"] for label in fetch_pr_data(pr_number, repo, "")["labels"] if label["name"] == label]) def is_py(file): return file.endswith("py") @@ -77,11 +88,11 @@ if __name__ == "__main__": print("Only docs were updated, stopping build process.") sys.exit(0) - elif only_frontend_code_changed and build_type == "server": + elif only_frontend_code_changed and build_type == "server" and not has_run_server_tests_label(pr_number, repo): print("Only Frontend code was updated; Stopping Python build process.") sys.exit(0) - elif build_type == "ui" and only_py_changed: + elif build_type == "ui" and only_py_changed and not has_run_ui_tests_label(pr_number, repo): print("Only Python code was updated, stopping Cypress build process.") sys.exit(0) From 2058b0cbeab88aef8116ca44d85b52436fa1525e Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Fri, 10 Jun 2022 15:44:05 +0530 Subject: [PATCH 14/21] fix(ui): tab refresh was not implemented --- frappe/public/js/frappe/form/form.js | 7 +------ frappe/public/js/frappe/form/layout.js | 27 +++++++++++--------------- frappe/public/js/frappe/form/tab.js | 18 +++++++++++++++++ 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/frappe/public/js/frappe/form/form.js b/frappe/public/js/frappe/form/form.js index b8fa42fa94..eefc629b4d 100644 --- a/frappe/public/js/frappe/form/form.js +++ b/frappe/public/js/frappe/form/form.js @@ -575,7 +575,7 @@ frappe.ui.form.Form = class FrappeForm { this.$wrapper.trigger('render_complete'); - this.cscript.is_onload && this.set_first_tab_as_active(); + this.layout.set_first_tab_as_active(switched || this.cscript.is_onload); if(!this.hidden) { this.layout.show_empty_form_message(); @@ -592,11 +592,6 @@ frappe.ui.form.Form = class FrappeForm { this.setup_image_autocompletions_in_markdown(); } - set_first_tab_as_active() { - this.layout.tabs[0] - && this.layout.tabs[0].set_active(); - } - focus_on_first_input() { let first = this.form_wrapper.find('.form-layout :input:visible:first'); if (!in_list(["Date", "Datetime"], first.attr("data-fieldtype"))) { diff --git a/frappe/public/js/frappe/form/layout.js b/frappe/public/js/frappe/form/layout.js index c1c9967507..6d000c99f9 100644 --- a/frappe/public/js/frappe/form/layout.js +++ b/frappe/public/js/frappe/form/layout.js @@ -294,7 +294,7 @@ frappe.ui.form.Layout = class Layout { this.refresh_sections(); // refresh tabs - this.tabbed_layout && this.refresh_tabs(); + this.is_tabbed_layout() && this.refresh_tabs(); if (this.frm) { // collapse sections @@ -328,21 +328,9 @@ frappe.ui.form.Layout = class Layout { } refresh_tabs() { - this.tabs.forEach(tab => { - if (!tab.wrapper.hasClass('hide') || !tab.parent.hasClass('hide')) { - tab.parent.removeClass('show hide'); - tab.wrapper.removeClass('show hide'); - if ( - tab.wrapper.find( - ".form-section:not(.hide-control, .empty-section), .form-dashboard-section:not(.hide-control, .empty-section)" - ).length - ) { - tab.toggle(true); - } else { - tab.toggle(false); - } - } - }); + for (let tab of this.tabs) { + tab.refresh(); + } const visible_tabs = this.tabs.filter(tab => !tab.hidden); if (visible_tabs && visible_tabs.length == 1) { @@ -350,6 +338,13 @@ frappe.ui.form.Layout = class Layout { } } + set_first_tab_as_active(switched) { + if (this.tabs.length && (switched || !this.frm.active_tab)) { + // set first tab as active when opening for first time, or new doc + this.tabs[0].set_active(); + } + } + refresh_fields(fields) { let fieldnames = fields.map((field) => { if (field.fieldname) return field.fieldname; diff --git a/frappe/public/js/frappe/form/tab.js b/frappe/public/js/frappe/form/tab.js index 3fad807f06..fd23595d80 100644 --- a/frappe/public/js/frappe/form/tab.js +++ b/frappe/public/js/frappe/form/tab.js @@ -36,10 +36,27 @@ export default class Tab { // hide if explicitly hidden let hide = this.df.hidden || this.df.hidden_due_to_dependency; + + // hide if dashboard and not saved + if (!hide && this.df.show_dashboard && this.frm.is_new() && !this.fields_list.length) { + hide = true; + } + + // hide if no read permission if (!hide && this.frm && !this.frm.get_perm(this.df.permlevel || 0, "read")) { hide = true; } + if (!hide && !this.df.show_dashboard) { + // show only if there is at least one visibe section or control + hide = true; + if (this.wrapper.find( + ".form-section:not(.hide-control, .empty-section), .form-dashboard-section:not(.hide-control, .empty-section)" + ).length) { + hide = false; + } + } + this.toggle(!hide); } @@ -62,6 +79,7 @@ export default class Tab { set_active() { this.parent.find('.nav-link').tab('show'); this.wrapper.addClass('show'); + this.frm.active_tab = this; } is_active() { From 52359a0ad9af91413896074eb726b29a0e67bc29 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 12:22:12 +0530 Subject: [PATCH 15/21] test: Scheduler tests cleanup --- frappe/tests/test_scheduler.py | 43 +++++++++++---------------------- frappe/utils/background_jobs.py | 7 ++++-- 2 files changed, 19 insertions(+), 31 deletions(-) diff --git a/frappe/tests/test_scheduler.py b/frappe/tests/test_scheduler.py index 5161e1e80f..c6b381e487 100644 --- a/frappe/tests/test_scheduler.py +++ b/frappe/tests/test_scheduler.py @@ -1,18 +1,16 @@ +import os import time from unittest import TestCase +from unittest.mock import patch import frappe -from frappe.core.doctype.scheduled_job_type.scheduled_job_type import sync_jobs +from frappe.core.doctype.scheduled_job_type.scheduled_job_type import ScheduledJobType, sync_jobs from frappe.utils import add_days, get_datetime from frappe.utils.background_jobs import enqueue from frappe.utils.doctor import purge_pending_jobs from frappe.utils.scheduler import enqueue_events, is_dormant, schedule_jobs_based_on_activity -def test_timeout(): - time.sleep(100) - - def test_timeout_10(): time.sleep(10) @@ -23,6 +21,11 @@ def test_method(): class TestScheduler(TestCase): def setUp(self): + frappe.db.rollback() + + if not os.environ.get("CI"): + return + purge_pending_jobs() if not frappe.get_all("Scheduled Job Type", limit=1): sync_jobs() @@ -44,15 +47,9 @@ class TestScheduler(TestCase): def test_queue_peeking(self): job = get_test_job() - self.assertTrue(job.enqueue()) - job.db_set("last_execution", "2010-01-01 00:00:00") - frappe.db.commit() - - time.sleep(0.5) - - # 1st job is in the queue (or running), don't enqueue it again - self.assertFalse(job.enqueue()) - frappe.db.delete("Scheduled Job Log", {"scheduled_job_type": job.name}) + with patch.object(job, "is_job_in_queue", return_value=True): + # 1st job is in the queue (or running), don't enqueue it again + self.assertFalse(job.enqueue()) def test_is_dormant(self): self.assertTrue(is_dormant(check_time=get_datetime("2100-01-01 00:00:00"))) @@ -88,22 +85,10 @@ class TestScheduler(TestCase): ) ) - frappe.db.rollback() - def test_job_timeout(self): - return - job = enqueue(test_timeout, timeout=10) - count = 5 - while count > 0: - count -= 1 - time.sleep(5) - if job.get_status() == "failed": - break - - self.assertTrue(job.is_failed) - - -def get_test_job(method="frappe.tests.test_scheduler.test_timeout_10", frequency="All"): +def get_test_job( + method="frappe.tests.test_scheduler.test_timeout_10", frequency="All" +) -> ScheduledJobType: if not frappe.db.exists("Scheduled Job Type", dict(method=method)): job = frappe.get_doc( dict( diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index ce8e44665a..f49c641673 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -3,7 +3,7 @@ import socket import time from collections import defaultdict from functools import lru_cache -from typing import List +from typing import TYPE_CHECKING, List from uuid import uuid4 import redis @@ -19,6 +19,9 @@ from frappe.utils import cstr, get_bench_id from frappe.utils.commands import log from frappe.utils.redis_queue import RedisQueue +if TYPE_CHECKING: + from rq.job import Job + @lru_cache() def get_queues_timeout(): @@ -52,7 +55,7 @@ def enqueue( *, at_front=False, **kwargs, -): +) -> "Job": """ Enqueue method to be executed using a background worker From 80334698a79f0b948cd07de0c557cb9ac97ea61d Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 14 Jun 2022 12:54:10 +0530 Subject: [PATCH 16/21] fix(minor): Onboarding: add option to view list view in create action --- .../js/frappe/widgets/onboarding_widget.js | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/frappe/public/js/frappe/widgets/onboarding_widget.js b/frappe/public/js/frappe/widgets/onboarding_widget.js index 128d7e691a..3da102759f 100644 --- a/frappe/public/js/frappe/widgets/onboarding_widget.js +++ b/frappe/public/js/frappe/widgets/onboarding_widget.js @@ -99,12 +99,7 @@ export default class OnboardingWidget extends Widget { const toggle_content = () => { this.step_body.empty(); this.step_footer.empty(); - - this.step_body.html( - step.description ? - frappe.markdown(step.description) - : `

${step.title}

` - ); + set_description(); if (step.intro_video_url) { $(``) @@ -117,6 +112,22 @@ export default class OnboardingWidget extends Widget { } }; + const set_description = () => { + let content = step.description ? + frappe.markdown(step.description) + : `

${step.title}

` + + if (step.action === 'Create Entry') { + // add a secondary action to view list + content += `

+ + ${ __('Show {0} List', [step.reference_document])} +

` + } + + this.step_body.html(content); + } + const toggle_video = () => { this.step_body.empty(); this.step_footer.empty(); From 602d4376ba8e4db592d28292a39b2f87ce7a9e26 Mon Sep 17 00:00:00 2001 From: Rushabh Mehta Date: Tue, 14 Jun 2022 13:07:13 +0530 Subject: [PATCH 17/21] fix(minor): js lint --- frappe/public/js/frappe/widgets/onboarding_widget.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frappe/public/js/frappe/widgets/onboarding_widget.js b/frappe/public/js/frappe/widgets/onboarding_widget.js index 3da102759f..e560551f79 100644 --- a/frappe/public/js/frappe/widgets/onboarding_widget.js +++ b/frappe/public/js/frappe/widgets/onboarding_widget.js @@ -114,19 +114,18 @@ export default class OnboardingWidget extends Widget { const set_description = () => { let content = step.description ? - frappe.markdown(step.description) - : `

${step.title}

` + frappe.markdown(step.description) : `

${step.title}

`; if (step.action === 'Create Entry') { // add a secondary action to view list content += `

${ __('Show {0} List', [step.reference_document])} -

` +

`; } this.step_body.html(content); - } + }; const toggle_video = () => { this.step_body.empty(); From 60ec324956164eaa95709dde48226a19b2b4035b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 16:40:41 +0530 Subject: [PATCH 18/21] fix: Remove unwanted blacklist over fields A field like 'count(`tabBOM Update Log`.name) as total_count' split by whitespace loses it's meaning. Tried splitting it meaningfully but didn't get the point of this tbh and stopped. I'm not sure what the code before was trying to do and with what set of inputs. Imagine the following fields: [count(`tabBOM Update Log`.name) as total_count, `tabBOM Update Log`.name as update_name, `tabBOM Update Log`.name, `tabBOM Update as Log`.name, tabBOM.name, name], I couldn't see what the previous check was trying to protect - hence, didn't add any equivalent functionality. --- frappe/model/db_query.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index fe52818235..91c70f5b97 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -378,9 +378,6 @@ class DatabaseQuery(object): for field in self.fields: if sub_query_regex.match(field): - if any(keyword in field.lower().split() for keyword in blacklisted_keywords): - _raise_exception() - if any(f"({keyword}" in field.lower() for keyword in blacklisted_keywords): _raise_exception() From 678eebe4fb8531413b88865f3e1bea162758910e Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 17:16:34 +0530 Subject: [PATCH 19/21] perf: Limit re internal cache to avoid caching patterns twice --- frappe/__init__.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frappe/__init__.py b/frappe/__init__.py index 7c71bc8815..c4f4b2a690 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -15,6 +15,7 @@ import importlib import inspect import json import os +import re import warnings from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union @@ -45,6 +46,10 @@ STANDARD_USERS = ("Guest", "Administrator") _dev_server = int(sbool(os.environ.get("DEV_SERVER", False))) _qb_patched = False +re._MAXCACHE = ( + 50 # reduced from default 512 given we are already maintaining this on parent worker +) + if _dev_server: warnings.simplefilter("always", DeprecationWarning) From 9b4db43b84ead21a135fa0ce29e810306c01f9b5 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 14 Jun 2022 17:17:22 +0530 Subject: [PATCH 20/21] perf(db_query): Maintain compiled pattern globally --- frappe/model/db_query.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/frappe/model/db_query.py b/frappe/model/db_query.py index 91c70f5b97..82913db98d 100644 --- a/frappe/model/db_query.py +++ b/frappe/model/db_query.py @@ -40,6 +40,16 @@ CAST_VARCHAR_PATTERN = re.compile( r"([`\"]?tab[\w`\" -]+\.[`\"]?name[`\"]?)(?!\w)", flags=re.IGNORECASE ) ORDER_BY_PATTERN = re.compile(r"\ order\ by\ |\ asc|\ ASC|\ desc|\ DESC", flags=re.IGNORECASE) +SUB_QUERY_PATTERN = re.compile("^.*[,();@].*") +IS_QUERY_PATTERN = re.compile(r"^(select|delete|update|drop|create)\s") +IS_QUERY_PREDICATE_PATTERN = re.compile( + r"\s*[0-9a-zA-z]*\s*( from | group by | order by | where | join )" +) +FIELD_QUOTE_PATTERN = re.compile(r"[0-9a-zA-Z]+\s*'") +FIELD_COMMA_PATTERN = re.compile(r"[0-9a-zA-Z]+\s*,") +STRICT_FIELD_PATTERN = re.compile(r".*/\*.*") +STRICT_UNION_PATTERN = re.compile(r".*\s(union).*\s") +ORDER_GROUP_PATTERN = re.compile(r".*[^a-z0-9-_ ,`'\"\.\(\)].*") class DatabaseQuery(object): @@ -343,8 +353,6 @@ class DatabaseQuery(object): As field contains `,` and mysql function `version()`, with the help of regex the system will filter out this field. """ - - sub_query_regex = re.compile("^.*[,();@].*") blacklisted_keywords = ["select", "create", "insert", "delete", "drop", "update", "case", "show"] blacklisted_functions = [ "concat", @@ -368,16 +376,14 @@ class DatabaseQuery(object): frappe.throw(_("Use of sub-query or function is restricted"), frappe.DataError) def _is_query(field): - if re.compile(r"^(select|delete|update|drop|create)\s").match(field): + if IS_QUERY_PATTERN.match(field): _raise_exception() - elif re.compile(r"\s*[0-9a-zA-z]*\s*( from | group by | order by | where | join )").match( - field - ): + elif IS_QUERY_PREDICATE_PATTERN.match(field): _raise_exception() for field in self.fields: - if sub_query_regex.match(field): + if SUB_QUERY_PATTERN.match(field): if any(f"({keyword}" in field.lower() for keyword in blacklisted_keywords): _raise_exception() @@ -388,19 +394,19 @@ class DatabaseQuery(object): # prevent access to global variables _raise_exception() - if re.compile(r"[0-9a-zA-Z]+\s*'").match(field): + if FIELD_QUOTE_PATTERN.match(field): _raise_exception() - if re.compile(r"[0-9a-zA-Z]+\s*,").match(field): + if FIELD_COMMA_PATTERN.match(field): _raise_exception() _is_query(field) if self.strict: - if re.compile(r".*/\*.*").match(field): + if STRICT_FIELD_PATTERN.match(field): frappe.throw(_("Illegal SQL Query")) - if re.compile(r".*\s(union).*\s").match(field.lower()): + if STRICT_UNION_PATTERN.match(field.lower()): frappe.throw(_("Illegal SQL Query")) def extract_tables(self): @@ -907,7 +913,7 @@ class DatabaseQuery(object): if "select" in _lower and "from" in _lower: frappe.throw(_("Cannot use sub-query in order by")) - if re.compile(r".*[^a-z0-9-_ ,`'\"\.\(\)].*").match(_lower): + if ORDER_GROUP_PATTERN.match(_lower): frappe.throw(_("Illegal SQL Query")) for field in parameters.split(","): From ec2aadbf5a685591dd8f90bb8812d34fc4167c66 Mon Sep 17 00:00:00 2001 From: Faris Ansari Date: Tue, 14 Jun 2022 18:15:48 +0530 Subject: [PATCH 21/21] chore: typo --- frappe/public/js/frappe/ui/toolbar/about.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/ui/toolbar/about.js b/frappe/public/js/frappe/ui/toolbar/about.js index 47dc7ee851..464c7c4787 100644 --- a/frappe/public/js/frappe/ui/toolbar/about.js +++ b/frappe/public/js/frappe/ui/toolbar/about.js @@ -19,7 +19,7 @@ frappe.ui.misc.about = function() {

Installed Apps

\
Loading versions...
\
\ -

© Frappe Technologies Pvt. Ltd and contributors

\ +

© Frappe Technologies Pvt. Ltd. and contributors

\ ", frappe.app)); frappe.ui.misc.about_dialog = d;