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.
This commit is contained in:
Ankush Menat
GitHub
parent
f70c32c23e
commit
eb79ecaca9
4 changed files with 5 additions and 241 deletions
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
Loading…
Add table
Reference in a new issue