From 638dbb6bcd57865eed020ea9fcc6465ac8348e31 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Wed, 6 Mar 2024 14:49:11 +0530 Subject: [PATCH 1/3] fix: disable transaction commits during doc events - Events like doc.save and doc.submit need to be atomic - Document hooks can make it not so atomic. This is extending server script behaviour where server script hooks are not allowed to commit/rollback. --- frappe/database/database.py | 16 +++++++++++++--- frappe/model/document.py | 6 +++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index fc0f5f50cf..03f4cacff4 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -95,8 +95,12 @@ class Database: self.before_rollback = CallbackManager() self.after_rollback = CallbackManager() - # self.db_type: str - # self.last_query (lazy) attribute of last sql query executed + # Setting this to true will disable full rollback and commit + # You can still use savepoint with partial rollback. + self.disable_transaction_control = False + + # self.db_type: str + # self.last_query (lazy) attribute of last sql query executed def setup_type_map(self): pass @@ -1028,6 +1032,10 @@ class Database: def commit(self): """Commit current transaction. Calls SQL `COMMIT`.""" + if self.disable_transaction_control: + self.logger.error("Commit issued during disabled transaction state ignored") + return + self.before_rollback.reset() self.after_rollback.reset() @@ -1042,7 +1050,7 @@ class Database: """`ROLLBACK` current transaction. Optionally rollback to a known save_point.""" if save_point: self.sql(f"rollback to savepoint {save_point}") - else: + elif not self.disable_transaction_control: self.before_commit.reset() self.after_commit.reset() @@ -1052,6 +1060,8 @@ class Database: self.begin() self.after_rollback.run() + else: + self.logger.error("Rollback issued during disabled transaction state ignored") def savepoint(self, save_point): """Savepoints work as a nested transaction. diff --git a/frappe/model/document.py b/frappe/model/document.py index e000923a10..c9ae34a354 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1300,7 +1300,11 @@ class Document(BaseDocument): def runner(self, method, *args, **kwargs): add_to_return_value(self, fn(self, *args, **kwargs)) for f in hooks: - add_to_return_value(self, f(self, method, *args, **kwargs)) + try: + frappe.db.disable_transaction_control = True + add_to_return_value(self, f(self, method, *args, **kwargs)) + finally: + frappe.db.disable_transaction_control = False return self.__dict__.pop("_return_value", None) From f66b23b96d4b2a52179c95771f02f3e04c6e2565 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 28 Mar 2024 11:14:05 +0530 Subject: [PATCH 2/3] fix: handle nested event calls Treat disable_transaction_control as a stack incr/decr when moving in and out of context. --- frappe/database/database.py | 14 +++++++++----- frappe/model/document.py | 4 ++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 03f4cacff4..3ce3ccfbbc 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -7,6 +7,7 @@ import random import re import string import traceback +import warnings from collections.abc import Iterable, Sequence from contextlib import contextmanager, suppress from time import time @@ -47,6 +48,9 @@ MULTI_WORD_PATTERN = re.compile(r'([`"])(tab([A-Z]\w+)( [A-Z]\w+)+)\1') SQL_ITERATOR_BATCH_SIZE = 100 +TRANSACTION_DISABLED_MSG = "Commit/rollback are disabled during certain events. This command will be ignored." + + class Database: """ Open a database connection with the given parmeters, if use_default is True, use the @@ -97,7 +101,7 @@ class Database: # Setting this to true will disable full rollback and commit # You can still use savepoint with partial rollback. - self.disable_transaction_control = False + self._disable_transaction_control = 0 # self.db_type: str # self.last_query (lazy) attribute of last sql query executed @@ -1032,8 +1036,8 @@ class Database: def commit(self): """Commit current transaction. Calls SQL `COMMIT`.""" - if self.disable_transaction_control: - self.logger.error("Commit issued during disabled transaction state ignored") + if self._disable_transaction_control: + warnings.warn(message=TRANSACTION_DISABLED_MSG, stacklevel=2) return self.before_rollback.reset() @@ -1050,7 +1054,7 @@ class Database: """`ROLLBACK` current transaction. Optionally rollback to a known save_point.""" if save_point: self.sql(f"rollback to savepoint {save_point}") - elif not self.disable_transaction_control: + elif not self._disable_transaction_control: self.before_commit.reset() self.after_commit.reset() @@ -1061,7 +1065,7 @@ class Database: self.after_rollback.run() else: - self.logger.error("Rollback issued during disabled transaction state ignored") + warnings.warn(message=TRANSACTION_DISABLED_MSG, stacklevel=2) def savepoint(self, save_point): """Savepoints work as a nested transaction. diff --git a/frappe/model/document.py b/frappe/model/document.py index c9ae34a354..602749f5f6 100644 --- a/frappe/model/document.py +++ b/frappe/model/document.py @@ -1301,10 +1301,10 @@ class Document(BaseDocument): add_to_return_value(self, fn(self, *args, **kwargs)) for f in hooks: try: - frappe.db.disable_transaction_control = True + frappe.db._disable_transaction_control += 1 add_to_return_value(self, f(self, method, *args, **kwargs)) finally: - frappe.db.disable_transaction_control = False + frappe.db._disable_transaction_control -= 1 return self.__dict__.pop("_return_value", None) From e5a64fd50c0ce235898f88d8efefd89535711c8c Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Thu, 28 Mar 2024 11:58:38 +0530 Subject: [PATCH 3/3] test: transaction control --- frappe/database/database.py | 4 +++- frappe/tests/test_db.py | 26 +++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/frappe/database/database.py b/frappe/database/database.py index 3ce3ccfbbc..1812576d63 100644 --- a/frappe/database/database.py +++ b/frappe/database/database.py @@ -48,7 +48,9 @@ MULTI_WORD_PATTERN = re.compile(r'([`"])(tab([A-Z]\w+)( [A-Z]\w+)+)\1') SQL_ITERATOR_BATCH_SIZE = 100 -TRANSACTION_DISABLED_MSG = "Commit/rollback are disabled during certain events. This command will be ignored." +TRANSACTION_DISABLED_MSG = """Commit/rollback are disabled during certain events. This command will +be ignored. Commit/Rollback from here WILL CAUSE very hard to debug problems with atomicity and +concurrent data update bugs.""" class Database: diff --git a/frappe/tests/test_db.py b/frappe/tests/test_db.py index c1abd1798e..cb87d63217 100644 --- a/frappe/tests/test_db.py +++ b/frappe/tests/test_db.py @@ -15,7 +15,7 @@ from frappe.database.utils import FallBackDateTimeStr from frappe.query_builder import Field from frappe.query_builder.functions import Concat_ws from frappe.tests.test_query_builder import db_type_is, run_only_if -from frappe.tests.utils import FrappeTestCase, timeout +from frappe.tests.utils import FrappeTestCase, patch_hooks, timeout from frappe.utils import add_days, now, random_string, set_request from frappe.utils.testutils import clear_custom_fields @@ -459,6 +459,19 @@ class TestDB(FrappeTestCase): ) self.assertEqual(1, frappe.db.transaction_writes - writes) + def test_transactions_disabled_during_writes(self): + hook_name = f"{bad_hook.__module__}.{bad_hook.__name__}" + nested_hook_name = f"{bad_nested_hook.__module__}.{bad_nested_hook.__name__}" + + with patch_hooks( + {"doc_events": {"*": {"before_validate": hook_name, "on_update": nested_hook_name}}} + ): + note = frappe.new_doc("Note", title=frappe.generate_hash()) + note.insert() + self.assertGreater(frappe.db.transaction_writes, 0) # This would've reset for commit/rollback + + self.assertFalse(frappe.db._disable_transaction_control) + def test_pk_collision_ignoring(self): # note has `name` generated from title for _ in range(3): @@ -1007,6 +1020,17 @@ class TestConcurrency(FrappeTestCase): self.assertRaises(frappe.QueryTimeoutError, frappe.delete_doc, note.doctype, note.name) +def bad_hook(*args, **kwargs): + frappe.db.commit() + frappe.db.rollback() + + +def bad_nested_hook(doc, *args, **kwargs): + doc.run_method("before_validate") + frappe.db.commit() + frappe.db.rollback() + + class TestSqlIterator(FrappeTestCase): def test_db_sql_iterator(self): test_queries = [