From 10f0f0d2e99691559f7f16b91d204caf3aafe75f Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 7 May 2024 16:58:40 +0200 Subject: [PATCH 1/7] fix!: Dont delete existing ScheduledJobType on change Prior to this, any change in the cron would lead to previous SJT documents & corresponding SJL documents deleted which may be undesirable. --- .../doctype/server_script/server_script.py | 99 +++++++------------ 1 file changed, 35 insertions(+), 64 deletions(-) diff --git a/frappe/core/doctype/server_script/server_script.py b/frappe/core/doctype/server_script/server_script.py index d3af5d1fc6..0d406cc0cb 100644 --- a/frappe/core/doctype/server_script/server_script.py +++ b/frappe/core/doctype/server_script/server_script.py @@ -3,6 +3,7 @@ from functools import partial from types import FunctionType, MethodType, ModuleType +from typing import TYPE_CHECKING import frappe from frappe import _ @@ -16,6 +17,9 @@ from frappe.utils.safe_exec import ( safe_exec, ) +if TYPE_CHECKING: + from frappe.core.doctype.scheduled_job_type.scheduled_job_type import ScheduledJobType + class ServerScript(Document): # begin: auto-generated types @@ -77,12 +81,10 @@ class ServerScript(Document): def validate(self): frappe.only_for("Script Manager", True) - self.sync_scheduled_jobs() - self.clear_scheduled_events() self.check_if_compilable_in_restricted_context() def on_update(self): - self.sync_scheduler_events() + self.sync_scheduled_job_type() def clear_cache(self): frappe.cache.delete_value("server_script_map") @@ -92,7 +94,10 @@ class ServerScript(Document): frappe.cache.delete_value("server_script_map") if self.script_type == "Scheduler Event": for job in self.scheduled_jobs: - frappe.delete_doc("Scheduled Job Type", job.name) + scheduled_job_type: "ScheduledJobType" = frappe.get_doc("Scheduled Job Type", job.name) + scheduled_job_type.stopped = True + scheduled_job_type.server_script = None + scheduled_job_type.save() def get_code_fields(self): return {"script": "py"} @@ -105,33 +110,35 @@ class ServerScript(Document): fields=["name", "stopped"], ) - def sync_scheduled_jobs(self): - """Sync Scheduled Job Type statuses if Server Script's disabled status is changed""" - if self.script_type != "Scheduler Event" or not self.has_value_changed("disabled"): + def sync_scheduled_job_type(self): + """Create or update Scheduled Job Type documents for Scheduler Event Server Scripts""" + if self.script_type != "Scheduler Event" or ( + (previous_script_type := self.has_value_changed("script_type")) + # True will be sent if its a new record + and previous_script_type not in (True, "Scheduler Event") + ): return - for scheduled_job in self.scheduled_jobs: - if bool(scheduled_job.stopped) != bool(self.disabled): - job = frappe.get_doc("Scheduled Job Type", scheduled_job.name) - job.stopped = self.disabled - job.save() - - def sync_scheduler_events(self): - """Create or update Scheduled Job Type documents for Scheduler Event Server Scripts""" - if not self.disabled and self.event_frequency and self.script_type == "Scheduler Event": - cron_format = self.cron_format if self.event_frequency == "Cron" else None - setup_scheduler_events( - script_name=self.name, frequency=self.event_frequency, cron_format=cron_format + if scheduled_script := frappe.db.get_value("Scheduled Job Type", {"server_script": self.name}): + scheduled_job_type: "ScheduledJobType" = frappe.get_doc("Scheduled Job Type", scheduled_script) + else: + scheduled_job_type: "ScheduledJobType" = frappe.get_doc( + { + "doctype": "Scheduled Job Type", + "server_script": self.name, + } ) - def clear_scheduled_events(self): - """Deletes existing scheduled jobs by Server Script if self.event_frequency or self.cron_format has changed""" - if ( - self.script_type == "Scheduler Event" - and (self.has_value_changed("event_frequency") or self.has_value_changed("cron_format")) - ) or (self.has_value_changed("script_type") and self.script_type != "Scheduler Event"): - for scheduled_job in self.scheduled_jobs: - frappe.delete_doc("Scheduled Job Type", scheduled_job.name, delete_permanently=1) + scheduled_job_type.update( + { + "method": frappe.scrub(f"{self.name}-{self.event_frequency}"), + "frequency": self.event_frequency, + "cron_format": self.cron_format, + "stopped": self.disabled, + } + ).save() + + frappe.msgprint(_("Scheduled execution for script {0} has updated").format(self.name)) def check_if_compilable_in_restricted_context(self): """Check compilation errors and send them back as warnings.""" @@ -247,43 +254,7 @@ class ServerScript(Document): return items -def setup_scheduler_events(script_name: str, frequency: str, cron_format: str | None = None): - """Creates or Updates Scheduled Job Type documents based on the specified script name and frequency - - Args: - script_name (str): Name of the Server Script document - frequency (str): Event label compatible with the Frappe scheduler - """ - method = frappe.scrub(f"{script_name}-{frequency}") - scheduled_script = frappe.db.get_value("Scheduled Job Type", {"method": method}) - - if not scheduled_script: - frappe.get_doc( - { - "doctype": "Scheduled Job Type", - "method": method, - "frequency": frequency, - "server_script": script_name, - "cron_format": cron_format, - } - ).insert() - - frappe.msgprint(_("Enabled scheduled execution for script {0}").format(script_name)) - - else: - doc = frappe.get_doc("Scheduled Job Type", scheduled_script) - - if doc.frequency == frequency: - return - - doc.frequency = frequency - doc.cron_format = cron_format - doc.save() - - frappe.msgprint(_("Scheduled execution for script {0} has updated").format(script_name)) - - -def execute_api_server_script(script=None, *args, **kwargs): +def execute_api_server_script(script: ServerScript, *args, **kwargs): # These are only added for compatibility with rate limiter. del args del kwargs From ffbf7fb9d13956cffb8b11b159fbc1e56b7b399a Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 7 May 2024 17:19:01 +0200 Subject: [PATCH 2/7] fix!: Document.has_value_changed returns Truthy or False - Return changed value to avoid re-accessing previous object & it's attribute - Wrap returned value as Truthy to avoid breaking change in API --- .../core/doctype/server_script/server_script.py | 2 +- frappe/model/document.py | 9 ++++++--- frappe/utils/__init__.py | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/frappe/core/doctype/server_script/server_script.py b/frappe/core/doctype/server_script/server_script.py index 0d406cc0cb..54b650871f 100644 --- a/frappe/core/doctype/server_script/server_script.py +++ b/frappe/core/doctype/server_script/server_script.py @@ -115,7 +115,7 @@ class ServerScript(Document): if self.script_type != "Scheduler Event" or ( (previous_script_type := self.has_value_changed("script_type")) # True will be sent if its a new record - and previous_script_type not in (True, "Scheduler Event") + and previous_script_type.value not in (True, "Scheduler Event") ): return diff --git a/frappe/model/document.py b/frappe/model/document.py index f9c9ae7ae1..1b0c34f1fd 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -21,7 +21,7 @@ from frappe.model.naming import set_new_name, validate_name from frappe.model.utils import is_virtual_doctype from frappe.model.workflow import set_workflow_state_on_action, validate_workflow from frappe.types import DF -from frappe.utils import compare, cstr, date_diff, file_lock, flt, now +from frappe.utils import Truthy, compare, cstr, date_diff, file_lock, flt, now from frappe.utils.data import get_absolute_url, get_datetime, get_timedelta, getdate from frappe.utils.global_search import update_global_search @@ -468,7 +468,7 @@ class Document(BaseDocument): previous = self.get_doc_before_save() if not previous: - return True + return Truthy(context="New Document") previous_value = previous.get(fieldname) current_value = self.get(fieldname) @@ -480,7 +480,10 @@ class Document(BaseDocument): elif isinstance(previous_value, timedelta): current_value = get_timedelta(current_value) - return previous_value != current_value + if previous_value != current_value: + return Truthy(value=previous_value) + + return False def set_new_name(self, force=False, set_name=None, set_child_names=True): """Calls `frappe.naming.set_new_name` for parent and child docs.""" diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 70a5e2223b..5677e990fd 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -42,6 +42,8 @@ EMAIL_MATCH_PATTERN = re.compile( re.IGNORECASE, ) +UNSET = object() + def get_fullname(user=None): """get the full name (first name + last name) of the user from User""" @@ -1166,3 +1168,18 @@ class CallbackManager: def reset(self): self._functions.clear() + + +class Truthy: + def __init__(self, value=UNSET, context=UNSET): + self.value = value + self.context = context + + def __bool__(self): + return True + + def __repr__(self) -> str: + _val = "UNSET" if self.value is UNSET else self.value + _ctx = "UNSET" if self.context is UNSET else self.context + + return f"Truthy(value={_val}, context={_ctx})" From cf2a3e926e92094f77a8355f61ca7cd37290802b Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 7 May 2024 17:30:46 +0200 Subject: [PATCH 3/7] fix: ServerScript.safe_exec as a doc method --- frappe/core/doctype/server_script/server_script.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/frappe/core/doctype/server_script/server_script.py b/frappe/core/doctype/server_script/server_script.py index 54b650871f..18d9a6359c 100644 --- a/frappe/core/doctype/server_script/server_script.py +++ b/frappe/core/doctype/server_script/server_script.py @@ -177,13 +177,15 @@ class ServerScript(Document): Args: doc (Document): Executes script with for a certain document's events """ - safe_exec( - self.script, + self.safe_exec( _locals={"doc": doc}, restrict_commit_rollback=True, script_filename=self.name, ) + def safe_exec(self, **kwargs): + return safe_exec(script=self.script, **kwargs) + def execute_scheduled_method(self): """Specific to Scheduled Jobs via Server Scripts @@ -193,7 +195,7 @@ class ServerScript(Document): if self.script_type != "Scheduler Event": raise frappe.DoesNotExistError - safe_exec(self.script, script_filename=self.name) + self.safe_exec(script_filename=self.name) def get_permission_query_conditions(self, user: str) -> list[str]: """Specific to Permission Query Server Scripts. @@ -205,7 +207,7 @@ class ServerScript(Document): list: Return list of conditions defined by rules in self.script. """ locals = {"user": user, "conditions": ""} - safe_exec(self.script, None, locals, script_filename=self.name) + self.safe_exec(_locals=locals, script_filename=self.name) if locals["conditions"]: return locals["conditions"] @@ -267,7 +269,7 @@ def execute_api_server_script(script: ServerScript, *args, **kwargs): raise frappe.PermissionError # output can be stored in flags - _globals, _locals = safe_exec(script.script, script_filename=script.name) + _globals, _locals = script.safe_exec(script_filename=script.name) return _globals.frappe.flags From 179305495ab5a8a47d22426e59d220cbd3030a84 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Tue, 30 Apr 2024 17:33:41 +0200 Subject: [PATCH 4/7] perf: Use cached Server Script to execute doc method events --- frappe/core/doctype/server_script/server_script_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/core/doctype/server_script/server_script_utils.py b/frappe/core/doctype/server_script/server_script_utils.py index 418f3aea83..9237b66de5 100644 --- a/frappe/core/doctype/server_script/server_script_utils.py +++ b/frappe/core/doctype/server_script/server_script_utils.py @@ -43,7 +43,7 @@ def run_server_script_for_doc_event(doc, event): if scripts: # run all scripts for this doctype + event for script_name in scripts: - frappe.get_doc("Server Script", script_name).execute_doc(doc) + frappe.get_cached_doc("Server Script", script_name).execute_doc(doc) def get_server_script_map(): From 36c80b77a446bf1995dc29bcaff46c29f1be0d13 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Wed, 1 May 2024 17:43:19 +0200 Subject: [PATCH 5/7] fix: Set value default True for Truthy cls --- frappe/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 5677e990fd..304cedcfc0 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -1171,7 +1171,7 @@ class CallbackManager: class Truthy: - def __init__(self, value=UNSET, context=UNSET): + def __init__(self, value=True, context=UNSET): self.value = value self.context = context From 9c20649022ea9ffb7ffaa2a6e625079c12828569 Mon Sep 17 00:00:00 2001 From: Gavin D'souza Date: Mon, 13 May 2024 11:31:19 +0200 Subject: [PATCH 6/7] Revert "fix: ServerScript.safe_exec as a doc method" This reverts commit cf2a3e926e92094f77a8355f61ca7cd37290802b. --- frappe/core/doctype/server_script/server_script.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/frappe/core/doctype/server_script/server_script.py b/frappe/core/doctype/server_script/server_script.py index 18d9a6359c..54b650871f 100644 --- a/frappe/core/doctype/server_script/server_script.py +++ b/frappe/core/doctype/server_script/server_script.py @@ -177,15 +177,13 @@ class ServerScript(Document): Args: doc (Document): Executes script with for a certain document's events """ - self.safe_exec( + safe_exec( + self.script, _locals={"doc": doc}, restrict_commit_rollback=True, script_filename=self.name, ) - def safe_exec(self, **kwargs): - return safe_exec(script=self.script, **kwargs) - def execute_scheduled_method(self): """Specific to Scheduled Jobs via Server Scripts @@ -195,7 +193,7 @@ class ServerScript(Document): if self.script_type != "Scheduler Event": raise frappe.DoesNotExistError - self.safe_exec(script_filename=self.name) + safe_exec(self.script, script_filename=self.name) def get_permission_query_conditions(self, user: str) -> list[str]: """Specific to Permission Query Server Scripts. @@ -207,7 +205,7 @@ class ServerScript(Document): list: Return list of conditions defined by rules in self.script. """ locals = {"user": user, "conditions": ""} - self.safe_exec(_locals=locals, script_filename=self.name) + safe_exec(self.script, None, locals, script_filename=self.name) if locals["conditions"]: return locals["conditions"] @@ -269,7 +267,7 @@ def execute_api_server_script(script: ServerScript, *args, **kwargs): raise frappe.PermissionError # output can be stored in flags - _globals, _locals = script.safe_exec(script_filename=script.name) + _globals, _locals = safe_exec(script.script, script_filename=script.name) return _globals.frappe.flags From 665a18b063cd995b2aa936bb5a5a114f97ea03ba Mon Sep 17 00:00:00 2001 From: gavin Date: Mon, 13 May 2024 12:13:41 +0200 Subject: [PATCH 7/7] fix: Implement Truthy.__eq __ Implement equality check, compare with True rather than self for a Truthy instance Co-authored-by: Ankush Menat --- frappe/utils/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frappe/utils/__init__.py b/frappe/utils/__init__.py index 304cedcfc0..ceba8bab05 100644 --- a/frappe/utils/__init__.py +++ b/frappe/utils/__init__.py @@ -1178,6 +1178,9 @@ class Truthy: def __bool__(self): return True + def __eq__(self, other: object) -> bool: + return True == other # noqa: E712 + def __repr__(self) -> str: _val = "UNSET" if self.value is UNSET else self.value _ctx = "UNSET" if self.context is UNSET else self.context