vassili/seitime-frappe
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix: make read only mode thread safe (#28359)

Browse source 
* 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
This commit is contained in:
David Arnold 2024-11-18 15:00:50 +01:00 • committed by GitHub
parent 223640d8a7
commit 6f35a554a5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 50 additions and 60 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

75
frappe/model/document.py
View file

@ -107,58 +107,42 @@ 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):
# 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_method(func):
@wraps(func)
def wrapper(self, *args, **kwargs):
if self.doctype == "Error Log" and func.__name__ == "insert":
return original_methods["insert"](self, *args, **kwargs)
error_msg = f"Cannot call {func.__name__} in read-only document mode"
if context:
error_msg += f" ({context})"
raise frappe.DatabaseModificationError(error_msg)
return wrapper
# Use a thread-local variable to track nested invocations
"""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:
# 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))
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 @@ class Document(BaseDocument, DocRef):
)
raise frappe.PermissionError
@read_only_guard
def insert(
self,
ignore_permissions=None,
@ -430,10 +415,12 @@ class Document(BaseDocument, DocRef):
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 @@ 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()
@ -1188,6 +1177,7 @@ 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(
@ -1311,6 +1301,7 @@ 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.

16
frappe/model/mapper.py
View file

@ -5,6 +5,7 @@ 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
@ -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)

19
frappe/tests/test_document_ro_mode.py
View file

@ -54,14 +54,6 @@ class TestReadOnlyDocument(IntegrationTestCase):
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 @@ class TestReadOnlyDocument(IntegrationTestCase):
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():