From 6f35a554a522e6e10bfbe8cd888c6d1cf4f645ca Mon Sep 17 00:00:00 2001 From: David Arnold Date: Mon, 18 Nov 2024 15:00:50 +0100 Subject: [PATCH] fix: make read only mode thread safe (#28359) * fix: Apply read_only_method decorator to Document methods * fix: update tests for read-only document context manager * refactor: place mappers into read-only mode Reapply "refactor: place mappers into read-only mode" This reverts commit a8208d57069c63f0982bf2881bcad28a4b349f3c. read-only mode is now thread safe --- frappe/model/document.py | 67 ++++++++++++--------------- frappe/model/mapper.py | 16 +++++-- frappe/tests/test_document_ro_mode.py | 19 +++----- 3 files changed, 46 insertions(+), 56 deletions(-) diff --git a/frappe/model/document.py b/frappe/model/document.py index fb6aec24db01..8aa75f1e2437 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -107,58 +107,42 @@ def get_doc_from_dict(data: dict[str, Any], **kwargs) -> "Document": raise ImportError(data["doctype"]) -@contextmanager -def read_only_document(context=None): - # Store original methods - original_methods = { - "save": Document.save, - "_save": Document._save, - "insert": Document.insert, - "delete": Document.delete, - "submit": Document.submit, - "cancel": Document.cancel, - "db_set": Document.db_set, - } +def read_only_guard(func): + """Decorator to prevent document methods from being called in read-only mode""" - def read_only_method(func): - @wraps(func) - def wrapper(self, *args, **kwargs): + @wraps(func) + def wrapper(self, *args, **kwargs): + if getattr(frappe.local, "read_only_depth", 0) > 0: + # Allow Error Log inserts even in read-only mode if self.doctype == "Error Log" and func.__name__ == "insert": - return original_methods["insert"](self, *args, **kwargs) + return func(self, *args, **kwargs) error_msg = f"Cannot call {func.__name__} in read-only document mode" - if context: - error_msg += f" ({context})" + if getattr(frappe.local, "read_only_context", None): + error_msg += f" ({frappe.local.read_only_context})" raise frappe.DatabaseModificationError(error_msg) + return func(self, *args, **kwargs) - return wrapper + return wrapper - # Use a thread-local variable to track nested invocations + +@contextmanager +def read_only_document(context=None): + """Context manager to prevent document modifications. + Uses thread-local state to track read-only mode.""" if not hasattr(frappe.local, "read_only_depth"): frappe.local.read_only_depth = 0 - try: - # Increment the depth counter - frappe.local.read_only_depth += 1 - - # Only apply read-only methods if this is the outermost invocation - if frappe.local.read_only_depth == 1: - # Replace methods with read-only versions - for method_name, method in original_methods.items(): - setattr(Document, method_name, read_only_method(method)) + frappe.local.read_only_depth += 1 + if context: + frappe.local.read_only_context = context + try: yield - finally: - # Decrement the depth counter frappe.local.read_only_depth -= 1 - - # Only restore original methods if this is the outermost invocation if frappe.local.read_only_depth == 0: - # Restore original methods - for method_name, method in original_methods.items(): - setattr(Document, method_name, method) - - # Clean up the thread-local variable + if hasattr(frappe.local, "read_only_context"): + del frappe.local.read_only_context del frappe.local.read_only_depth @@ -337,6 +321,7 @@ def raise_no_permission_to(self, perm_type): ) raise frappe.PermissionError + @read_only_guard def insert( self, ignore_permissions=None, @@ -430,10 +415,12 @@ def check_if_locked(self): if self.creation and self.is_locked: raise frappe.DocumentLockedError + @read_only_guard def save(self, *args, **kwargs) -> "Self": """Wrapper for _save""" return self._save(*args, **kwargs) + @read_only_guard def _save(self, ignore_permissions=None, ignore_version=None) -> "Self": """Save the current document in the database in the **DocType**'s table or `tabSingles` (for single types). @@ -1155,11 +1142,13 @@ def _rename(self, name: str, merge: bool = False, force: bool = False, validate_ self.reload() @frappe.whitelist() + @read_only_guard def submit(self): """Submit the document. Sets `docstatus` = 1, then saves.""" return self._submit() @frappe.whitelist() + @read_only_guard def cancel(self): """Cancel the document. Sets `docstatus` = 2, then saves.""" return self._cancel() @@ -1188,6 +1177,7 @@ def rename(self, name: str, merge=False, force=False, validate_rename=True): """Rename the document to `name`. This transforms the current object.""" return self._rename(name=name, merge=merge, force=force, validate_rename=validate_rename) + @read_only_guard def delete(self, ignore_permissions=False, force=False, *, delete_permanently=False): """Delete document.""" return frappe.delete_doc( @@ -1311,6 +1301,7 @@ def notify_update(self): data = {"doctype": self.doctype, "name": self.name, "user": frappe.session.user} frappe.publish_realtime("list_update", data, after_commit=True) + @read_only_guard def db_set(self, fieldname, value=None, update_modified=True, notify=False, commit=False): """Set a value in the document object, update the timestamp and update the database. diff --git a/frappe/model/mapper.py b/frappe/model/mapper.py index ea06f0065bba..8aca4c438064 100644 --- a/frappe/model/mapper.py +++ b/frappe/model/mapper.py @@ -5,6 +5,7 @@ import frappe from frappe import _ from frappe.model import child_table_fields, default_fields, table_fields +from frappe.model.document import read_only_document from frappe.utils import cstr @@ -32,7 +33,8 @@ def make_mapped_doc(method, source_name, selected_children=None, args=None): frappe.flags.selected_children = selected_children or None - return method(source_name) + with read_only_document("doc-mapper"): + return method(source_name) @frappe.whitelist() @@ -49,7 +51,8 @@ def map_docs(method, source_names, target_doc, args=None): for src in json.loads(source_names): _args = (src, target_doc, json.loads(args)) if args else (src, target_doc) - target_doc = method(*_args) + with read_only_document("doc-mapper"): + target_doc = method(*_args) return target_doc @@ -100,7 +103,8 @@ def get_mapped_doc( ret_doc.run_method("before_mapping", source_doc, table_maps) - map_doc(source_doc, target_doc, table_maps[source_doc.doctype]) + with read_only_document("doc-mapper"): + map_doc(source_doc, target_doc, table_maps[source_doc.doctype]) row_exists_for_parentfield = {} @@ -159,10 +163,12 @@ def get_mapped_doc( if table_map.get("filter") and table_map.get("filter")(source_d): continue - map_child_doc(source_d, target_doc, table_map, source_doc) + with read_only_document("doc-mapper"): + map_child_doc(source_d, target_doc, table_map, source_doc) if postprocess: - postprocess(source_doc, target_doc) + with read_only_document("doc-mapper"): + postprocess(source_doc, target_doc) ret_doc.run_method("after_mapping", source_doc) ret_doc.set_onload("load_after_mapping", True) diff --git a/frappe/tests/test_document_ro_mode.py b/frappe/tests/test_document_ro_mode.py index a621f89c2bd5..9c2789222473 100644 --- a/frappe/tests/test_document_ro_mode.py +++ b/frappe/tests/test_document_ro_mode.py @@ -54,14 +54,6 @@ def nested_save(): with self.assertRaises(frappe.DatabaseModificationError): nested_save() - def test_read_only_context_manager_restoration(self): - original_save = Document.save - - with read_only_document(): - self.assertNotEqual(Document.save, original_save) - - self.assertEqual(Document.save, original_save) - def test_nested_read_only_document(self): # Check that read_only_depth is not set initially self.assertFalse(hasattr(frappe.local, "read_only_depth")) @@ -156,17 +148,18 @@ def exception_raiser(): self.assertEqual(Document.save, self.test_doc.__class__.save) def test_read_only_nested_context_managers(self): - original_save = Document.save + """Test that read_only_depth is properly managed in nested contexts""" + self.assertFalse(hasattr(frappe.local, "read_only_depth")) with read_only_document(): - self.assertNotEqual(Document.save, original_save) + self.assertEqual(frappe.local.read_only_depth, 1) with read_only_document(): - self.assertNotEqual(Document.save, original_save) + self.assertEqual(frappe.local.read_only_depth, 2) - self.assertNotEqual(Document.save, original_save) + self.assertEqual(frappe.local.read_only_depth, 1) - self.assertEqual(Document.save, original_save) + self.assertFalse(hasattr(frappe.local, "read_only_depth")) def test_read_only_method_call_details(self): with read_only_document():