From 9a7e59e811366f5ea0aefc7d4a9d1e4e387920b0 Mon Sep 17 00:00:00 2001 From: Ponnusamy <95607086+Ponnusamy1-V@users.noreply.github.com> Date: Sun, 6 Nov 2022 15:00:53 +0530 Subject: [PATCH 01/25] fix: dashboard view from workspace --- frappe/public/js/frappe/views/breadcrumbs.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frappe/public/js/frappe/views/breadcrumbs.js b/frappe/public/js/frappe/views/breadcrumbs.js index 53a3300a94..2eaa70fe0f 100644 --- a/frappe/public/js/frappe/views/breadcrumbs.js +++ b/frappe/public/js/frappe/views/breadcrumbs.js @@ -29,7 +29,7 @@ frappe.breadcrumbs = { return localStorage["preferred_breadcrumbs:" + doctype]; }, - add(module, doctype, type) { + async add(module, doctype, type) { let obj; if (typeof module === "object") { obj = module; @@ -40,7 +40,7 @@ frappe.breadcrumbs = { type: type, }; } - + await frappe.model.with_doctype(doctype); this.all[frappe.breadcrumbs.current_page()] = obj; this.update(); }, From b2860e6f9ed45214631f655880899227d075f1e9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 7 Nov 2022 19:01:50 +0530 Subject: [PATCH 02/25] fix(UX): allow clicking on row to open in new tab (#18789) - ctrl+click on row is toggling checkbox instead of opening it in new row - This only happens on non-mac devices Root cause: incorrect grouping of predicate towards fixing https://github.com/frappe/frappe/issues/18788 [skip ci] --- frappe/public/js/frappe/list/list_view.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/list/list_view.js b/frappe/public/js/frappe/list/list_view.js index 66ff5107a1..52d0026e37 100644 --- a/frappe/public/js/frappe/list/list_view.js +++ b/frappe/public/js/frappe/list/list_view.js @@ -1179,7 +1179,7 @@ frappe.views.ListView = class ListView extends frappe.views.BaseList { this.$result.on("click", ".list-row, .image-view-header, .file-header", (e) => { const $target = $(e.target); // tick checkbox if Ctrl/Meta key is pressed - if (e.ctrlKey || (e.metaKey && !$target.is("a"))) { + if ((e.ctrlKey || e.metaKey) && !$target.is("a")) { const $list_row = $(e.currentTarget); const $check = $list_row.find(".list-row-checkbox"); $check.prop("checked", !$check.prop("checked")); From ce360b6fcea2b10b0b2fa297746fc23595bb631f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 5 Nov 2022 16:58:01 +0530 Subject: [PATCH 03/25] feat: Set default SQL statement timeouts --- frappe/database/database.py | 36 ++++++++++++++++++++++++++-- frappe/database/mariadb/database.py | 7 ++++++ frappe/database/postgres/database.py | 8 +++++++ frappe/tests/test_db.py | 21 ++++++++++++++++ frappe/utils/background_jobs.py | 1 - 5 files changed, 70 insertions(+), 3 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 3cb47e853a..64ef994a50 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -7,7 +7,7 @@ import random import re import string import traceback -from contextlib import contextmanager +from contextlib import contextmanager, suppress from time import time from pypika.dialects import MySQLQueryBuilder, PostgreSQLQueryBuilder @@ -29,7 +29,7 @@ from frappe.exceptions import DoesNotExistError, ImplicitCommitError from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count from frappe.utils import cast as cast_fieldtype -from frappe.utils import get_datetime, get_table_name, getdate, now, sbool +from frappe.utils import cint, get_datetime, get_table_name, getdate, now, sbool IFNULL_PATTERN = re.compile(r"ifnull\(", flags=re.IGNORECASE) INDEX_PATTERN = re.compile(r"\s*\([^)]+\)\s*") @@ -114,6 +114,17 @@ class Database: self._cursor = self._conn.cursor() frappe.local.rollback_observers = [] + try: + if execution_timeout := get_ideal_query_execution_timeout(): + self.set_execution_timeout(execution_timeout) + except Exception as e: + frappe.logger("database").warning(f"Couldn't set execution timeout {e}") + + def set_execution_timeout(self, seconds: int): + """Set session speicifc timeout on exeuction of statements. + If any statement takes more time it will be killed along with entire transaction.""" + raise NotImplementedError + def use(self, db_name): """`USE` db_name.""" self._conn.select_db(db_name) @@ -1340,3 +1351,24 @@ def savepoint(catch: type | tuple[type, ...] = Exception): frappe.db.rollback(save_point=savepoint) else: frappe.db.release_savepoint(savepoint) + + +def get_ideal_query_execution_timeout() -> int: + """Get execution timeout based on current timeout in contexts. + + HTTP requests: HTTP timeout or a default (300) + Background jobs: Job timeout + + Note: Timeout adds 1.5x as "safety factor" + """ + from rq import get_current_job + + # Zero means no timeout, which is the default value in db. + timeout = 0 + with suppress(Exception): + if hasattr(frappe.local, "request"): + timeout = frappe.conf.http_timeout or 300 + elif job := get_current_job(): + timeout = job.timeout + + return int(cint(timeout) * 1.5) diff --git a/frappe/database/mariadb/database.py b/frappe/database/mariadb/database.py index 1df9877eb1..322c355357 100644 --- a/frappe/database/mariadb/database.py +++ b/frappe/database/mariadb/database.py @@ -68,6 +68,10 @@ class MariaDBExceptionUtil: def is_syntax_error(e: pymysql.Error) -> bool: return e.args[0] == ER.PARSE_ERROR + @staticmethod + def is_statement_timeout(e: pymysql.Error) -> bool: + return e.args[0] == 1969 + @staticmethod def is_data_too_long(e: pymysql.Error) -> bool: return e.args[0] == ER.DATA_TOO_LONG @@ -102,6 +106,9 @@ class MariaDBConnectionUtil: def create_connection(self): return pymysql.connect(**self.get_connection_settings()) + def set_execution_timeout(self, seconds: int): + self.sql("set session max_statement_time = %s", int(seconds)) + def get_connection_settings(self) -> dict: conn_settings = { "host": self.host, diff --git a/frappe/database/postgres/database.py b/frappe/database/postgres/database.py index 3b3612c0e4..d082afceaf 100644 --- a/frappe/database/postgres/database.py +++ b/frappe/database/postgres/database.py @@ -99,6 +99,10 @@ class PostgresExceptionUtil: def is_duplicate_fieldname(e): return getattr(e, "pgcode", None) == DUPLICATE_COLUMN + @staticmethod + def is_statement_timeout(e): + return PostgresDatabase.is_timedout(e) or isinstance(e, frappe.QueryTimeoutError) + @staticmethod def is_data_too_long(e): return getattr(e, "pgcode", None) == STRING_DATA_RIGHT_TRUNCATION @@ -161,6 +165,10 @@ class PostgresDatabase(PostgresExceptionUtil, Database): return conn + def set_execution_timeout(self, seconds: int): + # Postgres expects milliseconds as input + self.sql("set local statement_timeout = %s", int(seconds) * 1000) + def escape(self, s, percent=True): """Escape quotes and percent in given string.""" if isinstance(s, bytes): diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index 08fef66bd0..9a7d086252 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -36,6 +36,27 @@ class TestDB(FrappeTestCase): def test_get_database_size(self): self.assertIsInstance(frappe.db.get_database_size(), (float, int)) + def test_db_statement_execution_timeout(self): + frappe.db.set_execution_timeout(2) + # Setting 0 means no timeout. + self.addCleanup(frappe.db.set_execution_timeout, 0) + + try: + savepoint = "statement_timeout" + frappe.db.savepoint(savepoint) + frappe.db.multisql( + { + "mariadb": "select sleep(10)", + "postgres": "select pg_sleep(10)", + } + ) + except Exception as e: + self.assertTrue(frappe.db.is_statement_timeout(e), f"exepcted {e} to be timeout error") + frappe.db.rollback(save_point=savepoint) + else: + frappe.db.rollback(save_point=savepoint) + self.fail("Long running queries not timing out") + def test_get_value(self): self.assertEqual(frappe.db.get_value("User", {"name": ["=", "Administrator"]}), "Administrator") self.assertEqual(frappe.db.get_value("User", {"name": ["like", "Admin%"]}), "Administrator") diff --git a/frappe/utils/background_jobs.py b/frappe/utils/background_jobs.py index d416857588..12c2105df8 100755 --- a/frappe/utils/background_jobs.py +++ b/frappe/utils/background_jobs.py @@ -9,7 +9,6 @@ from uuid import uuid4 import redis from redis.exceptions import BusyLoadingError, ConnectionError from rq import Connection, Queue, Worker -from rq.command import send_stop_job_command from rq.logutils import setup_loghandlers from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed From f45656217e13f0e062707d98c3c01ef2479c50a0 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 8 Nov 2022 14:58:25 +0530 Subject: [PATCH 04/25] refactor: control statement timeout via flag --- frappe/database/database.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 64ef994a50..1934bc8c30 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -115,7 +115,7 @@ class Database: frappe.local.rollback_observers = [] try: - if execution_timeout := get_ideal_query_execution_timeout(): + if execution_timeout := get_query_execution_timeout(): self.set_execution_timeout(execution_timeout) except Exception as e: frappe.logger("database").warning(f"Couldn't set execution timeout {e}") @@ -1353,20 +1353,24 @@ def savepoint(catch: type | tuple[type, ...] = Exception): frappe.db.release_savepoint(savepoint) -def get_ideal_query_execution_timeout() -> int: - """Get execution timeout based on current timeout in contexts. +def get_query_execution_timeout() -> int: + """Get execution timeout based on current timeout in different contexts. - HTTP requests: HTTP timeout or a default (300) - Background jobs: Job timeout + HTTP requests: HTTP timeout or a default (300) + Background jobs: Job timeout + Console/Commands: No timeout = 0. - Note: Timeout adds 1.5x as "safety factor" + Note: Timeout adds 1.5x as "safety factor" """ from rq import get_current_job + if not frappe.conf.get("enable_db_statement_timeout"): + return 0 + # Zero means no timeout, which is the default value in db. timeout = 0 with suppress(Exception): - if hasattr(frappe.local, "request"): + if getattr(frappe.local, "request", None): timeout = frappe.conf.http_timeout or 300 elif job := get_current_job(): timeout = job.timeout From ad7c0816f8e5f455aeb869132dda46980fe78966 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 8 Nov 2022 21:27:41 +0530 Subject: [PATCH 05/25] fix: prevent deleting standard doctypes in prod (#18803) --- frappe/core/doctype/doctype/test_doctype.py | 3 +++ frappe/installer.py | 4 ++-- frappe/model/delete_doc.py | 2 ++ frappe/tests/test_modules.py | 2 +- frappe/tests/test_virtual_doctype.py | 2 +- frappe/tests/ui_test_helpers.py | 4 ++-- 6 files changed, 11 insertions(+), 6 deletions(-) diff --git a/frappe/core/doctype/doctype/test_doctype.py b/frappe/core/doctype/doctype/test_doctype.py index 3722e5d1fa..2e74fd3a6a 100644 --- a/frappe/core/doctype/doctype/test_doctype.py +++ b/frappe/core/doctype/doctype/test_doctype.py @@ -670,6 +670,9 @@ class TestDocType(FrappeTestCase): self.assertEqual(test_json.test_json_field["hello"], "world") + def test_no_delete_doc(self): + self.assertRaises(frappe.ValidationError, frappe.delete_doc, "DocType", "Address") + @patch.dict(frappe.conf, {"developer_mode": 1}) def test_custom_field_deletion(self): """Custom child tables whose doctype doesn't exist should be auto deleted.""" diff --git a/frappe/installer.py b/frappe/installer.py index 4f1755c2a0..2a6c29a17f 100644 --- a/frappe/installer.py +++ b/frappe/installer.py @@ -402,7 +402,7 @@ def _delete_modules(modules: list[str], dry_run: bool) -> list[str]: if not dry_run: if doctype.issingle: - frappe.delete_doc("DocType", doctype.name, ignore_on_trash=True) + frappe.delete_doc("DocType", doctype.name, ignore_on_trash=True, force=True) else: drop_doctypes.append(doctype.name) @@ -460,7 +460,7 @@ def _delete_doctypes(doctypes: list[str], dry_run: bool) -> None: for doctype in set(doctypes): print(f"* dropping Table for '{doctype}'...") if not dry_run: - frappe.delete_doc("DocType", doctype, ignore_on_trash=True) + frappe.delete_doc("DocType", doctype, ignore_on_trash=True, force=True) frappe.db.sql_ddl(f"DROP TABLE IF EXISTS `tab{doctype}`") diff --git a/frappe/model/delete_doc.py b/frappe/model/delete_doc.py index d1120cc22d..5e8a12c345 100644 --- a/frappe/model/delete_doc.py +++ b/frappe/model/delete_doc.py @@ -92,6 +92,8 @@ def delete_doc( else: doc = frappe.get_doc(doctype, name) + if not (doc.custom or frappe.conf.developer_mode or frappe.flags.in_patch or force): + frappe.throw(_("Standard DocType can not be deleted.")) update_flags(doc, flags, ignore_permissions) check_permission_and_not_submitted(doc) diff --git a/frappe/tests/test_modules.py b/frappe/tests/test_modules.py index d6145afcf4..43c90456b8 100644 --- a/frappe/tests/test_modules.py +++ b/frappe/tests/test_modules.py @@ -69,7 +69,7 @@ class TestUtils(FrappeTestCase): delattr(self, "note") if self._testMethodName == "test_make_boilerplate": - self.doctype.delete() + self.doctype.delete(force=True) scrubbed = frappe.scrub(self.doctype.name) self.addCleanup( delete_path, diff --git a/frappe/tests/test_virtual_doctype.py b/frappe/tests/test_virtual_doctype.py index 22910cb64f..898c5e118c 100644 --- a/frappe/tests/test_virtual_doctype.py +++ b/frappe/tests/test_virtual_doctype.py @@ -87,7 +87,7 @@ class TestVirtualDoctypes(FrappeTestCase): cls.addClassCleanup(frappe.flags.pop, "allow_doctype_export", None) vdt = new_doctype(name=TEST_DOCTYPE_NAME, is_virtual=1, custom=0).insert() - cls.addClassCleanup(vdt.delete) + cls.addClassCleanup(vdt.delete, force=True) patch_virtual_doc = patch( "frappe.controllers", new={frappe.local.site: {TEST_DOCTYPE_NAME: VirtualDoctypeTest}} diff --git a/frappe/tests/ui_test_helpers.py b/frappe/tests/ui_test_helpers.py index 297d9f5b12..ecb6b4da97 100644 --- a/frappe/tests/ui_test_helpers.py +++ b/frappe/tests/ui_test_helpers.py @@ -451,7 +451,7 @@ def create_test_user(): @frappe.whitelist() def setup_tree_doctype(): - frappe.delete_doc_if_exists("DocType", "Custom Tree") + frappe.delete_doc_if_exists("DocType", "Custom Tree", force=True) frappe.get_doc( { @@ -475,7 +475,7 @@ def setup_tree_doctype(): @frappe.whitelist() def setup_image_doctype(): - frappe.delete_doc_if_exists("DocType", "Custom Image") + frappe.delete_doc_if_exists("DocType", "Custom Image", force=True) frappe.get_doc( { From 15cae83c50b202e53c78c8b7de8ac56698c1e117 Mon Sep 17 00:00:00 2001 From: Ponnusamy <95607086+Ponnusamy1-V@users.noreply.github.com> Date: Wed, 9 Nov 2022 13:13:39 +0530 Subject: [PATCH 06/25] Update breadcrumbs.js --- frappe/public/js/frappe/views/breadcrumbs.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/views/breadcrumbs.js b/frappe/public/js/frappe/views/breadcrumbs.js index 2eaa70fe0f..8cf9a6ffd7 100644 --- a/frappe/public/js/frappe/views/breadcrumbs.js +++ b/frappe/public/js/frappe/views/breadcrumbs.js @@ -29,7 +29,7 @@ frappe.breadcrumbs = { return localStorage["preferred_breadcrumbs:" + doctype]; }, - async add(module, doctype, type) { + add(module, doctype, type) { let obj; if (typeof module === "object") { obj = module; @@ -40,7 +40,6 @@ frappe.breadcrumbs = { type: type, }; } - await frappe.model.with_doctype(doctype); this.all[frappe.breadcrumbs.current_page()] = obj; this.update(); }, @@ -132,8 +131,9 @@ frappe.breadcrumbs = { } }, - set_list_breadcrumb(breadcrumbs) { + async set_list_breadcrumb(breadcrumbs) { const doctype = breadcrumbs.doctype; + await frappe.model.with_doctype(doctype); const doctype_meta = frappe.get_doc("DocType", doctype); if ( (doctype === "User" && !frappe.user.has_role("System Manager")) || From b71d93ef9f700a6a70a21df59314ed91c9647659 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 9 Nov 2022 14:26:06 +0530 Subject: [PATCH 07/25] test: db timeout computation --- frappe/tests/test_db.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index 9a7d086252..d8259975da 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -11,13 +11,13 @@ import frappe from frappe.core.utils import find from frappe.custom.doctype.custom_field.custom_field import create_custom_field from frappe.database import savepoint -from frappe.database.database import Database +from frappe.database.database import Database, get_query_execution_timeout from frappe.database.utils import FallBackDateTimeStr from frappe.query_builder import Field from frappe.query_builder.functions import Concat_ws from frappe.tests.test_query_builder import db_type_is, run_only_if from frappe.tests.utils import FrappeTestCase -from frappe.utils import add_days, cint, now, random_string +from frappe.utils import add_days, cint, now, random_string, set_request from frappe.utils.testutils import clear_custom_fields @@ -57,6 +57,13 @@ class TestDB(FrappeTestCase): frappe.db.rollback(save_point=savepoint) self.fail("Long running queries not timing out") + @patch.dict(frappe.conf, {"http_timeout": 20, "enable_db_statement_timeout": 1}) + def test_db_timeout_computation(self): + set_request(method="GET", path="/") + self.assertEqual(get_query_execution_timeout(), 30) + frappe.local.request = None + self.assertEqual(get_query_execution_timeout(), 0) + def test_get_value(self): self.assertEqual(frappe.db.get_value("User", {"name": ["=", "Administrator"]}), "Administrator") self.assertEqual(frappe.db.get_value("User", {"name": ["like", "Admin%"]}), "Administrator") From b0cb1adc013b58fe025faf90cb2e07297201211e Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 9 Nov 2022 15:02:03 +0530 Subject: [PATCH 08/25] ci: config cleanup and bump coverage --- .flake8 | 55 +++++++++++++++++++++++----- .github/helper/flake8.conf | 75 -------------------------------------- .pre-commit-config.yaml | 1 - .stylelintrc | 9 ----- bandit.yml | 1 - pyproject.toml | 2 +- 6 files changed, 47 insertions(+), 96 deletions(-) delete mode 100644 .github/helper/flake8.conf delete mode 100644 .stylelintrc delete mode 100644 bandit.yml diff --git a/.flake8 b/.flake8 index 4b852abd7c..2de7a154c9 100644 --- a/.flake8 +++ b/.flake8 @@ -1,37 +1,74 @@ [flake8] ignore = + B001, + B007, + B009, + B010, + B950, + E101, + E111, + E114, + E116, + E117, E121, + E122, + E123, + E124, + E125, E126, E127, E128, + E131, + E201, + E202, E203, + E211, + E221, + E222, + E223, + E224, E225, E226, + E228, E231, E241, + E242, E251, E261, + E262, E265, + E266, + E271, + E272, + E273, + E274, + E301, E302, E303, E305, + E306, E402, E501, + E502, + E701, + E702, + E703, E741, + F401, + F403, + F405, + W191, W291, W292, W293, W391, W503, W504, - F403, - B007, - B950, - W191, - E124, # closing bracket, irritating while writing QB code - E131, # continuation line unaligned for hanging indent - E123, # closing bracket does not match indentation of opening bracket's line - E101, # ensured by use of black + E711, + E129, + F841, + E713, + E712, max-line-length = 200 -exclude=.github/helper/semgrep_rules +exclude=,test_*.py diff --git a/.github/helper/flake8.conf b/.github/helper/flake8.conf deleted file mode 100644 index 20d4b912ca..0000000000 --- a/.github/helper/flake8.conf +++ /dev/null @@ -1,75 +0,0 @@ -[flake8] -ignore = - B001, - B007, - B009, - B010, - B950, - E101, - E111, - E114, - E116, - E117, - E121, - E122, - E123, - E124, - E125, - E126, - E127, - E128, - E131, - E201, - E202, - E203, - E211, - E221, - E222, - E223, - E224, - E225, - E226, - E228, - E231, - E241, - E242, - E251, - E261, - E262, - E265, - E266, - E271, - E272, - E273, - E274, - E301, - E302, - E303, - E305, - E306, - E402, - E501, - E502, - E701, - E702, - E703, - E741, - F401, - F403, - F405, - W191, - W291, - W292, - W293, - W391, - W503, - W504, - E711, - E129, - F841, - E713, - E712, - - -max-line-length = 200 -exclude=.github/helper/semgrep_rules,test_*.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 27fae671c9..0783e94457 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -59,7 +59,6 @@ repos: hooks: - id: flake8 additional_dependencies: ['flake8-bugbear',] - args: ['--config', '.github/helper/flake8.conf'] ci: autoupdate_schedule: weekly diff --git a/.stylelintrc b/.stylelintrc deleted file mode 100644 index 1e05d1fb41..0000000000 --- a/.stylelintrc +++ /dev/null @@ -1,9 +0,0 @@ -{ - "extends": ["stylelint-config-recommended"], - "plugins": ["stylelint-scss"], - "rules": { - "at-rule-no-unknown": null, - "scss/at-rule-no-unknown": true, - "no-descending-specificity": null - } -} \ No newline at end of file diff --git a/bandit.yml b/bandit.yml deleted file mode 100644 index b8560e97c8..0000000000 --- a/bandit.yml +++ /dev/null @@ -1 +0,0 @@ -skips: ['E0203', 'B605', 'B404', 'B603', 'B607'] \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index 348353003c..6506bfa13c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -93,7 +93,7 @@ ensure_newline_before_comments = true indent = "\t" [tool.bench.dev-dependencies] -coverage = "~=6.4.1" +coverage = "~=6.5.0" Faker = "~=13.12.1" pyngrok = "~=5.0.5" unittest-xml-reporting = "~=3.0.4" From ffdd368fe298f7245070d5d6a362197dc9ef1f05 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 9 Nov 2022 14:08:28 +0530 Subject: [PATCH 09/25] perf: faster as_list and as_dict --- frappe/database/database.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 1934bc8c30..8f8aa2bba2 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -7,6 +7,7 @@ import random import re import string import traceback +import warnings from contextlib import contextmanager, suppress from time import time @@ -273,6 +274,20 @@ class Database: if pluck: return [r[0] for r in self.last_result] + if as_utf8: + warnings.warn( + "as_utf8 parameter is deprecated and will be removed in version 15.", + DeprecationWarning, + stacklevel=2, + ) + + if formatted: + warnings.warn( + "formatted parameter is deprecated and will be removed in version 15.", + DeprecationWarning, + stacklevel=2, + ) + # scrub output if required if as_dict: ret = self.fetch_as_dict(formatted, as_utf8) @@ -391,10 +406,13 @@ class Database: def fetch_as_dict(self, formatted=0, as_utf8=0) -> list[frappe._dict]: """Internal. Converts results to dict.""" result = self.last_result - ret = [] if result: keys = [column[0] for column in self._cursor.description] + if not as_utf8: + return [frappe._dict(zip(keys, row)) for row in result] + + ret = [] for r in result: values = [] for value in r: @@ -429,6 +447,9 @@ class Database: @staticmethod def convert_to_lists(res, formatted=0, as_utf8=0): """Convert tuple output to lists (internal).""" + if not as_utf8: + return [[value for value in row] for row in res] + nres = [] for r in res: nr = [] From 66f5f4dd467087a7a4eeb19a9a6566201550ee89 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 9 Nov 2022 14:28:52 +0530 Subject: [PATCH 10/25] refactor(db): deprecated unused functions --- frappe/database/database.py | 22 ++++++++++------------ frappe/utils/deprecations.py | 27 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 frappe/utils/deprecations.py diff --git a/frappe/database/database.py b/frappe/database/database.py index 8f8aa2bba2..91f73ba006 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -7,7 +7,6 @@ import random import re import string import traceback -import warnings from contextlib import contextmanager, suppress from time import time @@ -31,6 +30,7 @@ from frappe.model.utils.link_count import flush_local_link_count from frappe.query_builder.functions import Count from frappe.utils import cast as cast_fieldtype from frappe.utils import cint, get_datetime, get_table_name, getdate, now, sbool +from frappe.utils.deprecations import deprecated, deprecation_warning IFNULL_PATTERN = re.compile(r"ifnull\(", flags=re.IGNORECASE) INDEX_PATTERN = re.compile(r"\s*\([^)]+\)\s*") @@ -275,18 +275,9 @@ class Database: return [r[0] for r in self.last_result] if as_utf8: - warnings.warn( - "as_utf8 parameter is deprecated and will be removed in version 15.", - DeprecationWarning, - stacklevel=2, - ) - + deprecation_warning("as_utf8 parameter is deprecated and will be removed in version 15.") if formatted: - warnings.warn( - "formatted parameter is deprecated and will be removed in version 15.", - DeprecationWarning, - stacklevel=2, - ) + deprecation_warning("formatted parameter is deprecated and will be removed in version 15.") # scrub output if required if as_dict: @@ -858,6 +849,7 @@ class Database: ).run(debug=debug, run=run, as_dict=as_dict) return {} + @deprecated def update(self, *args, **kwargs): """Update multiple values. Alias for `set_value`.""" return self.set_value(*args, **kwargs) @@ -897,6 +889,9 @@ class Database: modified_by = modified_by or frappe.session.user to_update.update({"modified": modified, "modified_by": modified_by}) + if for_update: + deprecation_warning("for_update parameter is deprecated and will be removed in v15.") + if is_single_doctype: frappe.db.delete( "Singles", filters={"field": ("in", tuple(to_update)), "doctype": dt}, debug=debug @@ -927,11 +922,13 @@ class Database: if dt in self.value_cache: del self.value_cache[dt] + @deprecated @staticmethod def set(doc, field, val): """Set value in document. **Avoid**""" doc.db_set(field, val) + @deprecated def touch(self, doctype, docname): """Update the modified timestamp of this document.""" modified = now() @@ -1254,6 +1251,7 @@ class Database: """ return self.sql_ddl(f"truncate `{get_table_name(doctype)}`") + @deprecated def clear_table(self, doctype): return self.truncate(doctype) diff --git a/frappe/utils/deprecations.py b/frappe/utils/deprecations.py new file mode 100644 index 0000000000..e98362198b --- /dev/null +++ b/frappe/utils/deprecations.py @@ -0,0 +1,27 @@ +""" Utils for deprecating functionality in Framework. + +WARNING: This file is internal, instead of depending just copy the code or use deprecation +libraries. +""" +import functools +import warnings + + +def deprecated(func): + """Decorator to wrap a function/method as deprecated.""" + + @functools.wraps(func) + def wrapper(*args, **kwargs): + deprecation_warning( + f"{func.__name__} is deprecated and will be removed in next major version.", + stacklevel=1, + ) + return func(*args, **kwargs) + + return wrapper + + +def deprecation_warning(message, category=DeprecationWarning, stacklevel=1): + """like warnings.warn but with auto incremented sane stacklevel.""" + + warnings.warn(message=message, category=category, stacklevel=stacklevel + 2) From a5240824d5cf25102d37ef3cc78d3045a11c8b3a Mon Sep 17 00:00:00 2001 From: Himanshu Shivhare Date: Wed, 9 Nov 2022 18:25:00 +0530 Subject: [PATCH 11/25] docs: youtube channel in "about" section (#18810) * YouTube channel addded in about section. I have added ERPNext YouTube channel reference in the about section. #Create an official Handle for YouTube Channel (Like @erpnext or erpnextofficial) * chore: handle [skip ci] Co-authored-by: Ankush Menat --- frappe/public/js/frappe/ui/toolbar/about.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/public/js/frappe/ui/toolbar/about.js b/frappe/public/js/frappe/ui/toolbar/about.js index e23706eff1..69cbbfaba0 100644 --- a/frappe/public/js/frappe/ui/toolbar/about.js +++ b/frappe/public/js/frappe/ui/toolbar/about.js @@ -19,6 +19,8 @@ frappe.ui.misc.about = function () { Facebook: https://facebook.com/erpnext

Twitter: https://twitter.com/erpnext

+

+ YouTube: https://www.youtube.com/@erpnextofficial

${__("Installed Apps")}

${__("Loading versions...")}
From 1dd719b123d08e8cc1dbc856ca234abe274772f9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 9 Nov 2022 19:37:35 +0530 Subject: [PATCH 12/25] fix: decorator ordering statcimethod doesn't return normal function so can't be "chained" the other way around [skip ci] --- frappe/database/database.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 91f73ba006..47ca451289 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -922,8 +922,8 @@ class Database: if dt in self.value_cache: del self.value_cache[dt] - @deprecated @staticmethod + @deprecated def set(doc, field, val): """Set value in document. **Avoid**""" doc.db_set(field, val) From a42ca7d8c1d66fd9fd90fbcbeffac5281ea58b0b Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Wed, 9 Nov 2022 14:15:41 +0000 Subject: [PATCH 13/25] fix: raise exception if doc before save is not found (#18796) * fix: raise exception if doc before save is not found * test: ensure error is raised when trying to save new doc using `doc.save()` * chore: add comment explaining condition * test: clearer name and docstring --- frappe/model/document.py | 12 ++++++++---- frappe/tests/test_document.py | 13 +++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index 8dcc57e827..942c7005a2 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -744,12 +744,13 @@ class Document(BaseDocument): Will also validate document transitions (Save > Submit > Cancel) calling `self.check_docstatus_transition`.""" - self.load_doc_before_save() + self.load_doc_before_save(raise_exception=True) self._action = "save" - previous = self.get_doc_before_save() + previous = self._doc_before_save - if not previous or self.meta.get("is_virtual"): + # previous is None for new document insert + if not previous: self.check_docstatus_transition(0) return @@ -1048,7 +1049,7 @@ class Document(BaseDocument): self.set_title_field() - def load_doc_before_save(self): + def load_doc_before_save(self, *, raise_exception: bool = False): """load existing document from db before saving""" self._doc_before_save = None @@ -1059,6 +1060,9 @@ class Document(BaseDocument): try: self._doc_before_save = frappe.get_doc(self.doctype, self.name, for_update=True) except frappe.DoesNotExistError: + if raise_exception: + raise + frappe.clear_last_message() def run_post_save_methods(self): diff --git a/frappe/tests/test_document.py b/frappe/tests/test_document.py index cec7ee3bb0..3833e911a7 100644 --- a/frappe/tests/test_document.py +++ b/frappe/tests/test_document.py @@ -411,6 +411,19 @@ class TestDocument(FrappeTestCase): todo.save() self.assertEqual(todo.notify_update.call_count, 1) + def test_error_on_saving_new_doc_with_name(self): + """Trying to save a new doc with name should raise DoesNotExistError""" + + doc = frappe.get_doc( + { + "doctype": "ToDo", + "description": "this should raise frappe.DoesNotExistError", + "name": "lets-trick-doc-save", + } + ) + + self.assertRaises(frappe.DoesNotExistError, doc.save) + class TestDocumentWebView(FrappeTestCase): def get(self, path, user="Guest"): From 104942ee0a56809552992d37a8fa323e0ef16c0e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 10 Nov 2022 11:25:44 +0530 Subject: [PATCH 14/25] build(deps): bump socket.io-parser from 4.0.4 to 4.0.5 (#18823) Bumps [socket.io-parser](https://github.com/socketio/socket.io-parser) from 4.0.4 to 4.0.5. - [Release notes](https://github.com/socketio/socket.io-parser/releases) - [Changelog](https://github.com/socketio/socket.io-parser/blob/main/CHANGELOG.md) - [Commits](https://github.com/socketio/socket.io-parser/compare/4.0.4...4.0.5) --- updated-dependencies: - dependency-name: socket.io-parser dependency-type: indirect ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index 263c531ca4..798300ae61 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3170,9 +3170,9 @@ socket.io-client@^4.5.1: socket.io-parser "~4.2.0" socket.io-parser@~4.0.4: - version "4.0.4" - resolved "https://registry.yarnpkg.com/socket.io-parser/-/socket.io-parser-4.0.4.tgz#9ea21b0d61508d18196ef04a2c6b9ab630f4c2b0" - integrity sha512-t+b0SS+IxG7Rxzda2EVvyBZbvFPBCjJoyHuE0P//7OAsN23GItzDRdWa6ALxZI/8R5ygK7jAR6t028/z+7295g== + version "4.0.5" + resolved "https://registry.yarnpkg.com/socket.io-parser/-/socket.io-parser-4.0.5.tgz#cb404382c32324cc962f27f3a44058cf6e0552df" + integrity sha512-sNjbT9dX63nqUFIOv95tTVm6elyIU4RvB1m8dOeZt+IgWwcWklFDOdmGcfo3zSiRsnR/3pJkjY5lfoGqEe4Eig== dependencies: "@types/component-emitter" "^1.2.10" component-emitter "~1.3.0" From fcaa16bb213702355449a2bc3aeda5bd1ff0b827 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 9 Nov 2022 22:25:47 +0530 Subject: [PATCH 15/25] perf: faster generate_hash --- frappe/__init__.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index c03b87be1c..483316a15f 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1018,19 +1018,12 @@ def get_precision( return get_field_precision(get_meta(doctype).get_field(fieldname), doc, currency) -def generate_hash(txt: str | None = None, length: int | None = None) -> str: - """Generates random hash for given text + current timestamp + random string.""" - import hashlib - import time +def generate_hash(txt: str | None = None, length: int | None = 56) -> str: + """Generate random hash using best available randomness source.""" + import math + import secrets - from .utils import random_string - - digest = hashlib.sha224( - ((txt or "") + repr(time.time()) + repr(random_string(8))).encode() - ).hexdigest() - if length: - digest = digest[:length] - return digest + return secrets.token_hex(math.ceil(length / 2))[:length] def reset_metadata_version(): From f34f7030a3372eb8dc2677bedcdf0948ffb20145 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 10 Nov 2022 11:34:06 +0530 Subject: [PATCH 16/25] refactor: remove `txt` param from generate_hash use --- frappe/__init__.py | 5 ++++- frappe/core/doctype/data_export/exporter.py | 2 +- frappe/core/doctype/data_import/test_importer.py | 2 +- frappe/model/naming.py | 2 +- frappe/patches/v11_0/remove_skip_for_doctype.py | 2 +- frappe/patches/v12_0/move_email_and_phone_to_child_table.py | 6 +++--- frappe/patches/v12_0/setup_tags.py | 2 +- frappe/templates/includes/navbar/navbar_items.html | 4 ++-- frappe/tests/test_modules.py | 2 +- frappe/utils/jinja_globals.py | 4 +--- frappe/utils/oauth.py | 2 +- frappe/website/doctype/website_theme/website_theme.py | 2 +- .../section_with_collapsible_content.html | 2 +- .../web_template/section_with_tabs/section_with_tabs.html | 4 ++-- frappe/website/web_template/slideshow/slideshow.html | 2 +- 15 files changed, 22 insertions(+), 21 deletions(-) diff --git a/frappe/__init__.py b/frappe/__init__.py index 483316a15f..84a27642a9 100644 --- a/frappe/__init__.py +++ b/frappe/__init__.py @@ -1018,11 +1018,14 @@ def get_precision( return get_field_precision(get_meta(doctype).get_field(fieldname), doc, currency) -def generate_hash(txt: str | None = None, length: int | None = 56) -> str: +def generate_hash(txt: str | None = None, length: int = 56) -> str: """Generate random hash using best available randomness source.""" import math import secrets + if not length: + length = 56 + return secrets.token_hex(math.ceil(length / 2))[:length] diff --git a/frappe/core/doctype/data_export/exporter.py b/frappe/core/doctype/data_export/exporter.py index e3bf669630..611592531d 100644 --- a/frappe/core/doctype/data_export/exporter.py +++ b/frappe/core/doctype/data_export/exporter.py @@ -432,7 +432,7 @@ class DataExporter: row[_column_start_end.start + i + 1] = value def build_response_as_excel(self): - filename = frappe.generate_hash("", 10) + filename = frappe.generate_hash(length=10) with open(filename, "wb") as f: f.write(cstr(self.writer.getvalue()).encode("utf-8")) f = open(filename) diff --git a/frappe/core/doctype/data_import/test_importer.py b/frappe/core/doctype/data_import/test_importer.py index af8c711ab5..978f5792dd 100644 --- a/frappe/core/doctype/data_import/test_importer.py +++ b/frappe/core/doctype/data_import/test_importer.py @@ -97,7 +97,7 @@ class TestImporter(FrappeTestCase): def test_data_import_update(self): existing_doc = frappe.get_doc( doctype=doctype_name, - title=frappe.generate_hash(doctype_name, 8), + title=frappe.generate_hash(length=8), table_field_1=[{"child_title": "child title to update"}], ) existing_doc.save() diff --git a/frappe/model/naming.py b/frappe/model/naming.py index 6f7db5cf9d..93be2204b4 100644 --- a/frappe/model/naming.py +++ b/frappe/model/naming.py @@ -278,7 +278,7 @@ def make_autoname(key="", doctype="", doc=""): DE/09/01/00001 where 09 is the year, 01 is the month and 00001 is the series """ if key == "hash": - return frappe.generate_hash(doctype, 10) + return frappe.generate_hash(length=10) series = NamingSeries(key) return series.generate_next_name(doc) diff --git a/frappe/patches/v11_0/remove_skip_for_doctype.py b/frappe/patches/v11_0/remove_skip_for_doctype.py index e7c1d71a0a..b3471ca4e8 100644 --- a/frappe/patches/v11_0/remove_skip_for_doctype.py +++ b/frappe/patches/v11_0/remove_skip_for_doctype.py @@ -60,7 +60,7 @@ def execute(): # Maintain sequence (name, user, allow, for_value, applicable_for, apply_to_all_doctypes, creation, modified) new_user_permissions_list.append( ( - frappe.generate_hash("", 10), + frappe.generate_hash(length=10), user_permission.user, user_permission.allow, user_permission.for_value, diff --git a/frappe/patches/v12_0/move_email_and_phone_to_child_table.py b/frappe/patches/v12_0/move_email_and_phone_to_child_table.py index 1a369b4e12..7283760c23 100644 --- a/frappe/patches/v12_0/move_email_and_phone_to_child_table.py +++ b/frappe/patches/v12_0/move_email_and_phone_to_child_table.py @@ -27,7 +27,7 @@ def execute(): email_values.append( ( 1, - frappe.generate_hash(contact_detail.email_id, 10), + frappe.generate_hash(length=10), contact_detail.email_id, "email_ids", "Contact", @@ -44,7 +44,7 @@ def execute(): phone_values.append( ( phone_counter, - frappe.generate_hash(contact_detail.email_id, 10), + frappe.generate_hash(length=10), contact_detail.phone, "phone_nos", "Contact", @@ -63,7 +63,7 @@ def execute(): phone_values.append( ( phone_counter, - frappe.generate_hash(contact_detail.email_id, 10), + frappe.generate_hash(length=10), contact_detail.mobile_no, "phone_nos", "Contact", diff --git a/frappe/patches/v12_0/setup_tags.py b/frappe/patches/v12_0/setup_tags.py index 6bff8d3dac..cb0d46a45d 100644 --- a/frappe/patches/v12_0/setup_tags.py +++ b/frappe/patches/v12_0/setup_tags.py @@ -28,7 +28,7 @@ def execute(): tag_list.append((tag.strip(), time, time, "Administrator")) - tag_link_name = frappe.generate_hash(_user_tags.name + tag.strip() + doctype.name, 10) + tag_link_name = frappe.generate_hash(length=10) tag_links.append( (tag_link_name, doctype.name, _user_tags.name, tag.strip(), time, time, "Administrator") ) diff --git a/frappe/templates/includes/navbar/navbar_items.html b/frappe/templates/includes/navbar/navbar_items.html index 8a10751441..99a40a02a0 100644 --- a/frappe/templates/includes/navbar/navbar_items.html +++ b/frappe/templates/includes/navbar/navbar_items.html @@ -3,7 +3,7 @@ {% if parent %} -{%- set dropdown_id = 'id-' + frappe.utils.generate_hash('Dropdown', 12) -%} +{%- set dropdown_id = 'id-' + frappe.utils.generate_hash(length=12) -%}