From 032df946bead39b603e1eda6acd2049475bde757 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 5 Nov 2022 14:08:35 +0530 Subject: [PATCH 01/43] test: bg jobs test cleanup (#18767) * test: fix flaky RQ job tests Sometimes stop_job doesn't succeed and causes tests to timeout. Reduced sleep time to avoid this in tests. We are still testing all the important features - monitoring. * build(deps): Bump RQ to latest version Minor bugfixes that affect us ref: https://github.com/rq/rq/releases * test: sanity tests for scheduled job types * test(test_runner): dont set bench_id globally * refactor: stop_job shouldn't throw error The intention of use here is to stop stuck jobs or long running jobs, if for some reason they were stopped by the time command gets executed, there's no need to throw error. --- frappe/core/doctype/rq_job/rq_job.py | 7 +++++- frappe/core/doctype/rq_job/test_rq_job.py | 15 ++++++------- frappe/test_runner.py | 1 - frappe/tests/test_redis.py | 4 ++++ frappe/tests/test_zbg_job_sanity_test.py | 27 +++++++++++++++++++++++ pyproject.toml | 2 +- 6 files changed, 45 insertions(+), 11 deletions(-) create mode 100644 frappe/tests/test_zbg_job_sanity_test.py diff --git a/frappe/core/doctype/rq_job/rq_job.py b/frappe/core/doctype/rq_job/rq_job.py index 7e1c35a0e6..f05611fe7d 100644 --- a/frappe/core/doctype/rq_job/rq_job.py +++ b/frappe/core/doctype/rq_job/rq_job.py @@ -5,10 +5,12 @@ import functools import re from rq.command import send_stop_job_command +from rq.exceptions import InvalidJobOperation from rq.job import Job from rq.queue import Queue import frappe +from frappe import _ from frappe.model.document import Document from frappe.utils import ( cint, @@ -93,7 +95,10 @@ class RQJob(Document): @check_permissions def stop_job(self): - send_stop_job_command(connection=get_redis_conn(), job_id=self.job_id) + try: + send_stop_job_command(connection=get_redis_conn(), job_id=self.job_id) + except InvalidJobOperation: + frappe.msgprint(_("Job is not running."), title=_("Invalid Operation")) @staticmethod def get_count(args) -> int: diff --git a/frappe/core/doctype/rq_job/test_rq_job.py b/frappe/core/doctype/rq_job/test_rq_job.py index ae0691fa61..460aa08941 100644 --- a/frappe/core/doctype/rq_job/test_rq_job.py +++ b/frappe/core/doctype/rq_job/test_rq_job.py @@ -19,12 +19,11 @@ class TestRQJob(FrappeTestCase): @timeout(seconds=20) def check_status(self, job: Job, status, wait=True): - if wait: - while True: - if job.is_queued or job.is_started: - time.sleep(0.2) - else: - break + while wait: + if not (job.is_queued or job.is_started): + break + time.sleep(0.2) + self.assertEqual(frappe.get_doc("RQ Job", job.id).status, status) def test_serialization(self): @@ -69,7 +68,7 @@ class TestRQJob(FrappeTestCase): self.assertGreaterEqual(len(non_failed_jobs), 1) # Create a slow job and check if it's stuck in "Started" - job = frappe.enqueue(method=self.BG_JOB, queue="short", sleep=1000) + job = frappe.enqueue(method=self.BG_JOB, queue="short", sleep=10) time.sleep(3) self.check_status(job, "started", wait=False) stop_job(job_id=job.id) @@ -84,8 +83,8 @@ class TestRQJob(FrappeTestCase): def test_is_enqueued(self): + dummy_job = frappe.enqueue(self.BG_JOB, sleep=10, queue="short") job_name = "uniq_test_job" - dummy_job = frappe.enqueue(self.BG_JOB, sleep=100, queue="short") actual_job = frappe.enqueue(self.BG_JOB, job_name=job_name, queue="short") self.assertTrue(is_job_queued(job_name)) diff --git a/frappe/test_runner.py b/frappe/test_runner.py index 1e3573336a..7e2c7e724f 100644 --- a/frappe/test_runner.py +++ b/frappe/test_runner.py @@ -86,7 +86,6 @@ def main( frappe.utils.scheduler.disable_scheduler() set_test_email_config() - frappe.conf.update({"bench_id": "test_bench", "use_rq_auth": False}) if not frappe.flags.skip_before_tests: if verbose: diff --git a/frappe/tests/test_redis.py b/frappe/tests/test_redis.py index ca59af98b0..19001028f6 100644 --- a/frappe/tests/test_redis.py +++ b/frappe/tests/test_redis.py @@ -1,4 +1,5 @@ import functools +from unittest.mock import patch import redis @@ -30,12 +31,14 @@ def skip_if_redis_version_lt(version): class TestRedisAuth(FrappeTestCase): @skip_if_redis_version_lt("6.0") + @patch.dict(frappe.conf, {"bench_id": "test_bench", "use_rq_auth": False}) def test_rq_gen_acllist(self): """Make sure that ACL list is genrated""" acl_list = RedisQueue.gen_acl_list() self.assertEqual(acl_list[1]["bench"][0], get_bench_id()) @skip_if_redis_version_lt("6.0") + @patch.dict(frappe.conf, {"bench_id": "test_bench", "use_rq_auth": False}) def test_adding_redis_user(self): acl_list = RedisQueue.gen_acl_list() username, password = acl_list[1]["bench"] @@ -47,6 +50,7 @@ class TestRedisAuth(FrappeTestCase): conn.acl_deluser(username) @skip_if_redis_version_lt("6.0") + @patch.dict(frappe.conf, {"bench_id": "test_bench", "use_rq_auth": False}) def test_rq_namespace(self): """Make sure that user can access only their respective namespace.""" # Current bench ID diff --git a/frappe/tests/test_zbg_job_sanity_test.py b/frappe/tests/test_zbg_job_sanity_test.py new file mode 100644 index 0000000000..19dc168c04 --- /dev/null +++ b/frappe/tests/test_zbg_job_sanity_test.py @@ -0,0 +1,27 @@ +""" smoak tests to check that all registered background jobs execute without error. + +Note: Filename is intentional to run this test roughly at end. Don't change.""" + +import time + +import frappe +from frappe.core.doctype.rq_job.rq_job import RQJob, remove_failed_jobs +from frappe.tests.utils import FrappeTestCase, timeout + + +class TestScheduledJobSanity(FrappeTestCase): + def setUp(self): + remove_failed_jobs() + + @timeout(90) + def test_bg_jobs_run(self): + """Enqueue all scheduled jobs, wait for finish and verify that none failed.""" + for scheduled_job_type in frappe.get_all("Scheduled Job Type", pluck="name"): + frappe.get_doc("Scheduled Job Type", scheduled_job_type).enqueue(force=True) + + while RQJob.get_list({"filters": [["RQ Job", "status", "in", ("queued", "started")]]}): + time.sleep(0.5) + + # Check no failed, if failed print full details + failed_jobs = RQJob.get_list({"filters": [["RQ Job", "status", "=", "failed"]]}) + self.assertEqual(len(failed_jobs), 0, "Jobs failed: " + str(failed_jobs)) diff --git a/pyproject.toml b/pyproject.toml index 6f744ca186..348353003c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,7 +57,7 @@ dependencies = [ "hiredis~=2.0.0", "requests-oauthlib~=1.3.0", "requests~=2.27.1", - "rq~=1.10.1", + "rq~=1.11.1", "rsa>=4.1", "semantic-version~=2.10.0", "sqlparse~=0.4.1", From 43ccb40d0e4598bf71fe8af2f1fec6dda1e8adc8 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 5 Nov 2022 18:33:50 +0530 Subject: [PATCH 02/43] chore: rollback to 3.10 for vuln checks cython dependency fails [skip ci] --- .github/workflows/linters.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/linters.yml b/.github/workflows/linters.yml index 1d8e736538..01b5407489 100644 --- a/.github/workflows/linters.yml +++ b/.github/workflows/linters.yml @@ -79,7 +79,7 @@ jobs: steps: - uses: actions/setup-python@v4 with: - python-version: '3.11' + python-version: '3.10' - uses: actions/checkout@v3 - run: | pip install pip-audit From 313605d328d192078b41fd196b41af21754373cf Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 5 Nov 2022 20:37:43 +0530 Subject: [PATCH 03/43] fix: standard dashboards not loading --- 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 5236c21ac2..53a3300a94 100644 --- a/frappe/public/js/frappe/views/breadcrumbs.js +++ b/frappe/public/js/frappe/views/breadcrumbs.js @@ -137,13 +137,13 @@ frappe.breadcrumbs = { const doctype_meta = frappe.get_doc("DocType", doctype); if ( (doctype === "User" && !frappe.user.has_role("System Manager")) || - (doctype_meta && doctype_meta.issingle) + doctype_meta?.issingle ) { // no user listview for non-system managers and single doctypes } else { let route; const doctype_route = frappe.router.slug(frappe.router.doctype_layout || doctype); - if (doctype_meta.is_tree) { + if (doctype_meta?.is_tree) { let view = frappe.model.user_settings[doctype].last_view || "Tree"; route = `${doctype_route}/view/${view}`; } else { From cd1bbccdc3a412478dc9ec1a4cddca937f0109db Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sat, 5 Nov 2022 16:05:37 +0000 Subject: [PATCH 04/43] fix: dont release `frappe.local` twice (#18772) --- frappe/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/app.py b/frappe/app.py index 8cb32ff4bf..889c2e7cbe 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -86,7 +86,7 @@ def application(request: Request): log_request(request, response) process_response(response) - frappe.destroy() + frappe.db.close() return response From ff0c820c9c7c1b9c3173b0c2ce8d1be8fe576b76 Mon Sep 17 00:00:00 2001 From: Sagar Vora Date: Sat, 5 Nov 2022 16:06:07 +0000 Subject: [PATCH 05/43] chore(socketio): remove duplicate event handler (#18770) --- socketio.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/socketio.js b/socketio.js index 711e0028cc..30e931d6bf 100644 --- a/socketio.js +++ b/socketio.js @@ -31,11 +31,6 @@ io.on("connection", function (socket) { socket.user = cookie.parse(socket.request.headers.cookie).user_id; - socket.on("task_subscribe", function (task_id) { - var room = get_task_room(socket, task_id); - socket.join(room); - }); - let retries = 0; let join_user_room = () => { request @@ -61,13 +56,13 @@ io.on("connection", function (socket) { join_user_room(); - socket.on("task_unsubscribe", function (task_id) { + socket.on("task_subscribe", function (task_id) { var room = get_task_room(socket, task_id); - socket.leave(room); + socket.join(room); }); socket.on("task_unsubscribe", function (task_id) { - var room = "task:" + task_id; + var room = get_task_room(socket, task_id); socket.leave(room); }); From 5d1d3564ef41ceec652c7f097ae9ef4150420ffa Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 5 Nov 2022 20:45:02 +0530 Subject: [PATCH 06/43] test: Dashboard basic ui test + whitelisting --- cypress/integration/dashboard.js | 50 ++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 cypress/integration/dashboard.js diff --git a/cypress/integration/dashboard.js b/cypress/integration/dashboard.js new file mode 100644 index 0000000000..6eb28567bc --- /dev/null +++ b/cypress/integration/dashboard.js @@ -0,0 +1,50 @@ +describe("Dashboard view", { scrollBehavior: false }, () => { + before(() => { + cy.login(); + cy.visit("/app"); + }); + + it("should load", () => { + const chart = "TODO-YEARLY-TRENDS"; + const dashboard = "TODO-TEST-DASHBOARD"; // check slash in name intentionally. + + cy.insert_doc( + "Dashboard Chart", + { + is_standard: 0, + chart_name: chart, + chart_type: "Count", + document_type: "ToDo", + parent_document_type: "", + based_on: "creation", + group_by_type: "Count", + timespan: "Last Year", + time_interval: "Yearly", + timeseries: 1, + type: "Line", + filters_json: "[]", + }, + true + ); + + cy.insert_doc( + "Dashboard", + { + name: dashboard, + dashboard_name: dashboard, + is_standard: 0, + charts: [ + { + chart: chart, + }, + ], + }, + true + ); + + cy.visit(`/app/dashboard-view/${dashboard}`); + + // expect chart to be loaded + cy.findByText(chart).should("be.visible"); + }); +}); 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 07/43] 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 09d35c74eb77f76cdca073bda3833160f1d83c79 Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Sun, 6 Nov 2022 12:57:02 +0100 Subject: [PATCH 08/43] fix: remove redundant translation (#18775) --- frappe/client.py | 5 ----- frappe/core/page/permission_manager/permission_manager.py | 2 -- frappe/desk/desk_page.py | 5 ----- frappe/desk/query_report.py | 5 ----- 4 files changed, 17 deletions(-) diff --git a/frappe/client.py b/frappe/client.py index f42f73a529..404617b68c 100644 --- a/frappe/client.py +++ b/frappe/client.py @@ -333,11 +333,6 @@ def get_js(items): with open(contentpath) as srcfile: code = frappe.utils.cstr(srcfile.read()) - if frappe.local.lang != "en": - messages = frappe.get_lang_dict("jsfile", contentpath) - messages = json.dumps(messages) - code += f"\n\n$.extend(frappe._messages, {messages})" - out.append(code) return out diff --git a/frappe/core/page/permission_manager/permission_manager.py b/frappe/core/page/permission_manager/permission_manager.py index 46c9e0aca2..45c1e44fa1 100644 --- a/frappe/core/page/permission_manager/permission_manager.py +++ b/frappe/core/page/permission_manager/permission_manager.py @@ -19,7 +19,6 @@ from frappe.permissions import ( setup_custom_perms, update_permission_property, ) -from frappe.translate import send_translations from frappe.utils.user import get_users_with_role as _get_user_with_role not_allowed_in_permission_manager = ["DocType", "Patch Log", "Module Def", "Transaction Log"] @@ -28,7 +27,6 @@ not_allowed_in_permission_manager = ["DocType", "Patch Log", "Module Def", "Tran @frappe.whitelist() def get_roles_and_doctypes(): frappe.only_for("System Manager") - send_translations(frappe.get_lang_dict("doctype", "DocPerm")) active_domains = frappe.get_active_domains() diff --git a/frappe/desk/desk_page.py b/frappe/desk/desk_page.py index ad0bd549d8..bde27125f6 100644 --- a/frappe/desk/desk_page.py +++ b/frappe/desk/desk_page.py @@ -2,7 +2,6 @@ # License: MIT. See LICENSE import frappe -from frappe.translate import send_translations @frappe.whitelist() @@ -31,10 +30,6 @@ def getpage(): page = frappe.form_dict.get("name") doc = get(page) - # load translations - if frappe.lang != "en": - send_translations(frappe.get_lang_dict("page", page)) - frappe.response.docs.append(doc) diff --git a/frappe/desk/query_report.py b/frappe/desk/query_report.py index 877fdbe5bc..d0bc63f858 100644 --- a/frappe/desk/query_report.py +++ b/frappe/desk/query_report.py @@ -14,7 +14,6 @@ from frappe.model.utils import render_include from frappe.modules import get_module_path, scrub from frappe.monitor import add_data_to_monitor from frappe.permissions import get_role_permissions -from frappe.translate import send_translations from frappe.utils import ( cint, cstr, @@ -202,10 +201,6 @@ def get_script(report_name): if not script: script = "frappe.query_reports['%s']={}" % report_name - # load translations - if frappe.lang != "en": - send_translations(frappe.get_lang_dict("report", report_name)) - return { "script": render_include(script), "html_format": html_format, From e02b90cd5b0be4d6dd40742fdeff2ac01e70b638 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 6 Nov 2022 17:33:02 +0530 Subject: [PATCH 09/43] fix: dont allow reading attributes of unsafe objects (#18706) --- frappe/tests/test_safe_exec.py | 16 ++++++++++++++ frappe/utils/safe_exec.py | 39 ++++++++++++++++++++++++++++++---- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/frappe/tests/test_safe_exec.py b/frappe/tests/test_safe_exec.py index caa98737a2..fcd5832680 100644 --- a/frappe/tests/test_safe_exec.py +++ b/frappe/tests/test_safe_exec.py @@ -1,3 +1,5 @@ +import types + import frappe from frappe.tests.utils import FrappeTestCase from frappe.utils.safe_exec import get_safe_globals, safe_exec @@ -59,3 +61,17 @@ class TestSafeExec(FrappeTestCase): # enqueue whitelisted method safe_exec("""frappe.enqueue("ping", now=True)""") + + def test_ensure_getattrable_globals(self): + def check_safe(objects): + for obj in objects: + if isinstance(obj, (types.ModuleType, types.CodeType, types.TracebackType, types.FrameType)): + self.fail(f"{obj} wont work in safe exec.") + elif isinstance(obj, dict): + check_safe(obj.values()) + + check_safe(get_safe_globals().values()) + + def test_unsafe_objects(self): + unsafe_global = {"frappe": frappe} + self.assertRaises(SyntaxError, safe_exec, """frappe.msgprint("Hello")""", unsafe_global) diff --git a/frappe/utils/safe_exec.py b/frappe/utils/safe_exec.py index afdd4694a8..c75a5fd12b 100644 --- a/frappe/utils/safe_exec.py +++ b/frappe/utils/safe_exec.py @@ -2,6 +2,9 @@ import copy import inspect import json import mimetypes +import types +from contextlib import contextmanager +from functools import lru_cache import RestrictedPython.Guards from RestrictedPython import compile_restricted, safe_globals @@ -64,14 +67,20 @@ def safe_exec(script, _globals=None, _locals=None, restrict_commit_rollback=Fals exec_globals.frappe.db.pop("rollback", None) exec_globals.frappe.db.pop("add_index", None) - # execute script compiled by RestrictedPython - frappe.flags.in_safe_exec = True - exec(compile_restricted(script), exec_globals, _locals) # pylint: disable=exec-used - frappe.flags.in_safe_exec = False + with safe_exec_flags(), patched_qb(): + # execute script compiled by RestrictedPython + exec(compile_restricted(script), exec_globals, _locals) # pylint: disable=exec-used return exec_globals, _locals +@contextmanager +def safe_exec_flags(): + frappe.flags.in_safe_exec = True + yield + frappe.flags.in_safe_exec = False + + def get_safe_globals(): datautils = frappe._dict() @@ -258,6 +267,24 @@ def call_with_form_dict(function, kwargs): frappe.local.form_dict = form_dict +@contextmanager +def patched_qb(): + try: + _terms = frappe.qb.terms + frappe.qb.terms = _flatten(frappe.qb.terms) + yield + finally: + frappe.qb.terms = _terms + + +@lru_cache +def _flatten(module): + new_mod = NamespaceDict() + for name, obj in inspect.getmembers(module, lambda x: not inspect.ismodule(x)): + new_mod[name] = obj + return new_mod + + def get_python_builtins(): return { "abs": abs, @@ -350,6 +377,10 @@ def _getattr(object, name, default=None): if isinstance(name, str) and (name in UNSAFE_ATTRIBUTES): raise SyntaxError(f"{name} is an unsafe attribute") + + if isinstance(object, (types.ModuleType, types.CodeType, types.TracebackType, types.FrameType)): + raise SyntaxError(f"Reading {object} attributes is not allowed") + return RestrictedPython.Guards.safer_getattr(object, name, default=default) From e3a0b007c21858d19e86d2aa26e55737eefe6d44 Mon Sep 17 00:00:00 2001 From: Raffael Meyer <14891507+barredterra@users.noreply.github.com> Date: Sun, 6 Nov 2022 13:06:51 +0100 Subject: [PATCH 10/43] fix: translations (#18765) * fix: add missing translations * fix: capitalize label * fix: translate label on summary item --- frappe/public/js/frappe/utils/utils.js | 2 +- frappe/public/js/frappe/views/reports/query_report.js | 2 +- frappe/translations/de.csv | 7 ++++++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/frappe/public/js/frappe/utils/utils.js b/frappe/public/js/frappe/utils/utils.js index 3d17e8a8f6..cf5b619c37 100644 --- a/frappe/public/js/frappe/utils/utils.js +++ b/frappe/public/js/frappe/utils/utils.js @@ -1383,7 +1383,7 @@ Object.assign(frappe.utils, { : ""; return $(`
- ${summary.label} + ${__(summary.label)}
${value}
`); }, diff --git a/frappe/public/js/frappe/views/reports/query_report.js b/frappe/public/js/frappe/views/reports/query_report.js index 1febf11b6c..dc5e285181 100644 --- a/frappe/public/js/frappe/views/reports/query_report.js +++ b/frappe/public/js/frappe/views/reports/query_report.js @@ -1067,7 +1067,7 @@ frappe.views.QueryReport = class QueryReport extends frappe.views.BaseList { { fieldname: "sb_1", fieldtype: "Section Break", - label: "Y axis", + label: "Y Axis", }, { fieldname: "y_axis_fields", diff --git a/frappe/translations/de.csv b/frappe/translations/de.csv index 78fdcb7925..b1000f3bee 100644 --- a/frappe/translations/de.csv +++ b/frappe/translations/de.csv @@ -4080,7 +4080,7 @@ Scheduler Event,Scheduler-Ereignis, Select Event Type,Wählen Sie den Ereignistyp, Schedule Script,Zeitplan-Skript, Duration,Dauer, -Donut,Krapfen, +Donut,Donut, Custom Options,Benutzerdefinierte Optionen, "Ex: ""colors"": [""#d1d8dd"", ""#ff5858""]","Beispiel: "Farben": ["# d1d8dd", "# ff5858"]", Confirmation Email Template,Bestätigungs-E-Mail-Vorlage, @@ -4818,3 +4818,8 @@ K,Tsd,Number system M,Mio,Number system B,Mrd,Number system T,Bio,Number system +Type of Chart,Diagrammtyp, +Preview Chart,Vorschau erzeugen, +Please select X and Y fields,Bitte Felder für die X- und Y-Achse wählen, +Notification sent to,Benachrichtigung gesendet an, +Add to this activity by mailing to {0},"Senden Sie eine E-Mail an {0}, damit sie hier erscheint", From db1ed206f310603e05e30f0c0c33f5a83e831dba Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 6 Nov 2022 18:53:09 +0530 Subject: [PATCH 11/43] fix: only release db if it exists ref https://github.com/frappe/frappe/pull/18772 [skip ci] --- frappe/app.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/app.py b/frappe/app.py index 889c2e7cbe..0d7fdc1fe1 100644 --- a/frappe/app.py +++ b/frappe/app.py @@ -86,7 +86,8 @@ def application(request: Request): log_request(request, response) process_response(response) - frappe.db.close() + if frappe.db: + frappe.db.close() return response From 44a5bdc3f1d6b184797c41946aad29f9286ca289 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 6 Nov 2022 19:18:30 +0530 Subject: [PATCH 12/43] fix: ignore internal methods (#18784) --- frappe/utils/safe_exec.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frappe/utils/safe_exec.py b/frappe/utils/safe_exec.py index c75a5fd12b..04aa134d39 100644 --- a/frappe/utils/safe_exec.py +++ b/frappe/utils/safe_exec.py @@ -281,7 +281,8 @@ def patched_qb(): def _flatten(module): new_mod = NamespaceDict() for name, obj in inspect.getmembers(module, lambda x: not inspect.ismodule(x)): - new_mod[name] = obj + if not name.startswith("_"): + new_mod[name] = obj return new_mod From 62c4a3c020dcd58550c6e36e1ab32730b4ff4b99 Mon Sep 17 00:00:00 2001 From: Daizy Modi Date: Sun, 6 Nov 2022 19:59:59 +0530 Subject: [PATCH 13/43] fix: check permission before print or download document (#18757) --- frappe/utils/weasyprint.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frappe/utils/weasyprint.py b/frappe/utils/weasyprint.py index ceb064a955..d670cf96f5 100644 --- a/frappe/utils/weasyprint.py +++ b/frappe/utils/weasyprint.py @@ -9,6 +9,7 @@ import frappe @frappe.whitelist() def download_pdf(doctype, name, print_format, letterhead=None): doc = frappe.get_doc(doctype, name) + doc.check_permission("print") generator = PrintFormatGenerator(print_format, doc, letterhead) pdf = generator.render_pdf() @@ -21,6 +22,7 @@ def download_pdf(doctype, name, print_format, letterhead=None): def get_html(doctype, name, print_format, letterhead=None): doc = frappe.get_doc(doctype, name) + doc.check_permission("print") generator = PrintFormatGenerator(print_format, doc, letterhead) return generator.get_html_preview() From f04287ae83ed1531212c50defe52b75264d44d80 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sun, 6 Nov 2022 20:21:04 +0530 Subject: [PATCH 14/43] fix: dont close static modal with keyboard closes https://github.com/frappe/frappe/issues/11926 [skip ci] --- frappe/public/js/frappe/ui/keyboard.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/public/js/frappe/ui/keyboard.js b/frappe/public/js/frappe/ui/keyboard.js index 14418528a6..2e3bd95616 100644 --- a/frappe/public/js/frappe/ui/keyboard.js +++ b/frappe/public/js/frappe/ui/keyboard.js @@ -331,7 +331,7 @@ function close_grid_and_dialog() { } // close open dialog - if (cur_dialog && !cur_dialog.no_cancel_flag) { + if (cur_dialog && !cur_dialog.no_cancel_flag && !cur_dialog.static) { cur_dialog.cancel(); return false; } From b2860e6f9ed45214631f655880899227d075f1e9 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Mon, 7 Nov 2022 19:01:50 +0530 Subject: [PATCH 15/43] 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 16/43] 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 17/43] 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 18/43] 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 19/43] 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 20/43] 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 21/43] 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 22/43] 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 23/43] 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 24/43] 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 25/43] 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 26/43] 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 27/43] 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 28/43] 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 29/43] 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) -%}