From eb79ecaca9ace18d90b2e47bfcc7275de2af515f Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Sat, 12 Apr 2025 18:27:16 +0530 Subject: [PATCH] chore: Revert document read only mode (#32102) We can trust mappers to do the right thing, restricting few methods doesn't prevent anything real anyway, unnecessary overhead and breakages. --- frappe/model/document.py | 46 ------- frappe/model/mapper.py | 16 +-- frappe/tests/test_document_ro_mode.py | 184 -------------------------- tests/test_document_ro_mode.py | 0 4 files changed, 5 insertions(+), 241 deletions(-) delete mode 100644 frappe/tests/test_document_ro_mode.py delete mode 100644 tests/test_document_ro_mode.py diff --git a/frappe/model/document.py b/frappe/model/document.py index 6658f1114e..a194751e22 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -142,45 +142,6 @@ def get_doc_from_dict(data: dict[str, Any], **kwargs) -> "Document": raise ImportError(data["doctype"]) -def read_only_guard(func): - """Decorator to prevent document methods from being called in read-only mode""" - - @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 func(self, *args, **kwargs) - error_msg = f"Cannot call {func.__name__} in read-only document mode" - 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 - - -@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 - - frappe.local.read_only_depth += 1 - if context: - frappe.local.read_only_context = context - - try: - yield - finally: - frappe.local.read_only_depth -= 1 - if frappe.local.read_only_depth == 0: - if hasattr(frappe.local, "read_only_context"): - del frappe.local.read_only_context - del frappe.local.read_only_depth - - class Document(BaseDocument, DocRef): """All controllers inherit from `Document`.""" @@ -395,7 +356,6 @@ class Document(BaseDocument, DocRef): ) raise frappe.PermissionError - @read_only_guard def insert( self, ignore_permissions=None, @@ -512,12 +472,10 @@ class Document(BaseDocument, DocRef): exc=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). @@ -1245,13 +1203,11 @@ class Document(BaseDocument, DocRef): 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() @@ -1280,7 +1236,6 @@ class Document(BaseDocument, DocRef): """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( @@ -1404,7 +1359,6 @@ class Document(BaseDocument, DocRef): 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 f337fce529..a9a256df9a 100644 --- a/frappe/model/mapper.py +++ b/frappe/model/mapper.py @@ -5,7 +5,6 @@ import json 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 @@ -27,8 +26,7 @@ def make_mapped_doc(method, source_name, selected_children=None, args=None): frappe.flags.selected_children = selected_children or None - with read_only_document("doc-mapper"): - return method(source_name) + return method(source_name) @frappe.whitelist() @@ -45,8 +43,7 @@ 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) - with read_only_document("doc-mapper"): - target_doc = method(*_args) + target_doc = method(*_args) return target_doc @@ -97,8 +94,7 @@ def get_mapped_doc( ret_doc.run_method("before_mapping", source_doc, table_maps) - with read_only_document("doc-mapper"): - map_doc(source_doc, target_doc, table_maps[source_doc.doctype]) + map_doc(source_doc, target_doc, table_maps[source_doc.doctype]) row_exists_for_parentfield = {} @@ -157,12 +153,10 @@ def get_mapped_doc( if table_map.get("filter") and table_map.get("filter")(source_d): continue - with read_only_document("doc-mapper"): - map_child_doc(source_d, target_doc, table_map, source_doc) + map_child_doc(source_d, target_doc, table_map, source_doc) if postprocess: - with read_only_document("doc-mapper"): - postprocess(source_doc, target_doc) + 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 deleted file mode 100644 index 9c27892224..0000000000 --- a/frappe/tests/test_document_ro_mode.py +++ /dev/null @@ -1,184 +0,0 @@ -import unittest -from contextlib import contextmanager - -import frappe -from frappe.model.document import Document, read_only_document -from frappe.tests import IntegrationTestCase - - -class TestReadOnlyDocument(IntegrationTestCase): - def setUp(self): - # Create a test document - self.test_doc = frappe.get_doc({"doctype": "ToDo", "description": "Test ToDo"}) - self.test_doc.insert() - - def tearDown(self): - # Delete the test document - frappe.delete_doc("ToDo", self.test_doc.name) - - def test_read_only_save(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.save() - - def test_read_only_insert(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - frappe.get_doc({"doctype": "ToDo", "description": "Another Test ToDo"}).insert() - - def test_read_only_delete(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.delete() - - def test_read_only_submit(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.submit() - - def test_read_only_cancel(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.cancel() - - def test_read_only_db_set(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.db_set("status", "Closed") - - def test_read_only_nested_calls(self): - def nested_save(): - self.test_doc.save() - - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - nested_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")) - - with read_only_document(): - # Check that read_only_depth is set to 1 - self.assertEqual(frappe.local.read_only_depth, 1) - - # Attempt to modify the document - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.description = "Modified in outer context" - self.test_doc.save() - - with read_only_document(): - # Check that read_only_depth is incremented to 2 - self.assertEqual(frappe.local.read_only_depth, 2) - - # Attempt to modify the document in nested context - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.description = "Modified in inner context" - self.test_doc.save() - - # Check that read_only_depth is back to 1 after nested context - self.assertEqual(frappe.local.read_only_depth, 1) - - # Attempt to modify the document again - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.description = "Modified after inner context" - self.test_doc.save() - - # Check that read_only_depth is removed after all contexts are closed - self.assertFalse(hasattr(frappe.local, "read_only_depth")) - - # Verify that document can be modified outside read_only_document - self.test_doc.description = "Modified outside read_only_document" - self.test_doc.save() - self.assertEqual(self.test_doc.description, "Modified outside read_only_document") - - def test_error_log_exception_in_read_only(self): - with read_only_document(): - # Attempt to insert an Error Log - error_log = frappe.get_doc({"doctype": "Error Log", "error": "Test error in read-only mode"}) - - # This should not raise an exception - error_log.insert() - - # Verify that the Error Log was inserted - self.assertTrue(error_log.name) - - # Attempt to modify a different document - with self.assertRaises(frappe.DatabaseModificationError): - self.test_doc.description = "Modified in read-only mode" - self.test_doc.save() - - # Clean up the inserted Error Log - frappe.delete_doc("Error Log", error_log.name) - - def test_read_only_multiple_documents(self): - doc1 = frappe.get_doc({"doctype": "ToDo", "description": "Test ToDo 1"}) - doc2 = frappe.get_doc({"doctype": "ToDo", "description": "Test ToDo 2"}) - - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - doc1.insert() - with self.assertRaises(frappe.DatabaseModificationError): - doc2.insert() - - def test_read_only_custom_method(self): - class CustomDocument(Document): - def custom_save(self): - self.save() - - custom_doc = CustomDocument({"doctype": "ToDo", "description": "Custom Test ToDo"}) - - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError): - custom_doc.custom_save() - - def test_read_only_exception_handling(self): - @contextmanager - def exception_raiser(): - raise Exception("Test exception") - yield - - try: - with read_only_document(), exception_raiser(): - pass - except Exception: - pass - - # Ensure methods are restored even if an exception occurs - self.assertEqual(Document.save, self.test_doc.__class__.save) - - def test_read_only_nested_context_managers(self): - """Test that read_only_depth is properly managed in nested contexts""" - self.assertFalse(hasattr(frappe.local, "read_only_depth")) - - with read_only_document(): - self.assertEqual(frappe.local.read_only_depth, 1) - - with read_only_document(): - self.assertEqual(frappe.local.read_only_depth, 2) - - self.assertEqual(frappe.local.read_only_depth, 1) - - self.assertFalse(hasattr(frappe.local, "read_only_depth")) - - def test_read_only_method_call_details(self): - with read_only_document(): - with self.assertRaises(frappe.DatabaseModificationError) as cm: - self.test_doc.save() - - self.assertIn("Cannot call save in read-only document mode", str(cm.exception)) - - def test_read_only_does_not_affect_reads(self): - with read_only_document(): - # These operations should not raise exceptions - doc = frappe.get_doc("ToDo", self.test_doc.name) - self.assertEqual(doc.description, "Test ToDo") - - docs = frappe.get_all("ToDo", filters={"name": self.test_doc.name}) - self.assertEqual(len(docs), 1) - - def test_read_only_with_new_document_instance(self): - with read_only_document(): - new_doc = frappe.new_doc("ToDo") - with self.assertRaises(frappe.DatabaseModificationError): - new_doc.insert() diff --git a/tests/test_document_ro_mode.py b/tests/test_document_ro_mode.py deleted file mode 100644 index e69de29bb2..0000000000