[FIX] core: Ensure that rollback hooks are called when the cursor is closed

Before this change the rollback hooks were never called since rollback() was never explicitely called by the framework. At the end of a transaction, if no error occurs the famework call commit() on the odoo cursor. In all cases, the transaction ends with a call to close(). Into the implementation of _close()  rollback() is called on the underlying connection to ensure that not committed changes are rollbacked. That's the reason why despite the fact that rollback() was not called on the Odoo cursor, changes are not committed into the db in case of exception. To keep the same behaviour and avoid to have to explicitely call rollback() on the odoo cursor to trigger the execution of registered rollback hooks, these hooks are now processed in _close(). Since the list of registered hooks is emptied if commit() is called, we are sure that rollback hooks are only executed in case of rollback.

OPW #2294911

closes odoo/odoo#60334

X-original-commit: dce9a05f3a5d37fab711c8ca3c5444941f98e814
Signed-off-by: Raphael Collet (rco) <rco@openerp.com>
Co-authored-by: Raphael Collet <rco@odoo.com>

2 files changed, 76 insertions, 8 deletions

diff --git a/odoo/addons/base/tests/test_db_cursor.py b/odoo/addons/base/tests/test_db_cursor.py
index 37026cc967a..648e0f04beb 100644
--- a/odoo/addons/base/tests/test_db_cursor.py
+++ b/odoo/addons/base/tests/test_db_cursor.py
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
 
+from functools import partial
 
 import odoo
 from odoo.sql_db import TestCursor
@@ -115,3 +116,60 @@ class TestTestCursor(common.TransactionCase):
 
         self.cr.rollback()
         self.check(self.record, 'A')
+
+
+class TestCursorHooks(common.TransactionCase):
+    def setUp(self):
+        super().setUp()
+        self.log = []
+
+    def prepare_hooks(self, cr, precommit_msg, postcommit_msg, prerollback_msg, postrollback_msg):
+        cr.precommit.add(partial(self.log.append, precommit_msg))
+        cr.postcommit.add(partial(self.log.append, postcommit_msg))
+        cr.prerollback.add(partial(self.log.append, prerollback_msg))
+        cr.postrollback.add(partial(self.log.append, postrollback_msg))
+
+    def test_hooks(self):
+        cr = self.registry.cursor()
+
+        # check hook on commit()
+        self.prepare_hooks(cr, 'C1a', 'C1b', 'R1a', 'R1b')
+        self.assertEqual(self.log, [])
+        cr.commit()
+        self.assertEqual(self.log, ['C1a', 'C1b'])
+
+        # check hook on rollback()
+        self.prepare_hooks(cr, 'C2a', 'C2b', 'R2a', 'R2b')
+        self.assertEqual(self.log, ['C1a', 'C1b'])
+        cr.rollback()
+        self.assertEqual(self.log, ['C1a', 'C1b', 'R2a', 'R2b'])
+
+        # check hook on close()
+        self.prepare_hooks(cr, 'C3a', 'C3b', 'R3a', 'R3b')
+        self.assertEqual(self.log, ['C1a', 'C1b', 'R2a', 'R2b'])
+        cr.close()
+        self.assertEqual(self.log, ['C1a', 'C1b', 'R2a', 'R2b', 'R3a', 'R3b'])
+
+    def test_hooks_on_testcursor(self):
+        self.registry.enter_test_mode(self.cr)
+        self.addCleanup(self.registry.leave_test_mode)
+
+        cr = self.registry.cursor()
+
+        # check hook on commit(); post-commit hooks are ignored
+        self.prepare_hooks(cr, 'C1a', 'C1b', 'R1a', 'R1b')
+        self.assertEqual(self.log, [])
+        cr.commit()
+        self.assertEqual(self.log, ['C1a'])
+
+        # check hook on rollback()
+        self.prepare_hooks(cr, 'C2a', 'C2b', 'R2a', 'R2b')
+        self.assertEqual(self.log, ['C1a'])
+        cr.rollback()
+        self.assertEqual(self.log, ['C1a', 'R2a', 'R2b'])
+
+        # check hook on close()
+        self.prepare_hooks(cr, 'C3a', 'C3b', 'R3a', 'R3b')
+        self.assertEqual(self.log, ['C1a', 'R2a', 'R2b'])
+        cr.close()
+        self.assertEqual(self.log, ['C1a', 'R2a', 'R2b', 'R3a', 'R3b'])
diff --git a/odoo/sql_db.py b/odoo/sql_db.py
index 1c15f4e4a2d..c5af069728e 100644
--- a/odoo/sql_db.py
+++ b/odoo/sql_db.py
@@ -107,6 +107,8 @@ class BaseCursor:
     def __init__(self):
         self.precommit = tools.Callbacks()
         self.postcommit = tools.Callbacks()
+        self.prerollback = tools.Callbacks()
+        self.postrollback = tools.Callbacks()
 
     @contextmanager
     @check
@@ -378,10 +380,11 @@ class Cursor(BaseCursor):
         # collected as fast as they should). The problem is probably due in
         # part because browse records keep a reference to the cursor.
         del self._obj
-        self._closed = True
 
-        # Clean the underlying connection.
-        self._cnx.rollback()
+        # Clean the underlying connection, and run rollback hooks.
+        self.rollback()
+
+        self._closed = True
 
         if leak:
             self._cnx.leaked = True
@@ -433,7 +436,7 @@ class Cursor(BaseCursor):
         if event == 'commit':
             self.postcommit.add(func)
         elif event == 'rollback':
-            raise NotImplementedError()
+            self.postrollback.add(func)
 
     @check
     def commit(self):
@@ -441,6 +444,8 @@ class Cursor(BaseCursor):
         flush_env(self)
         self.precommit.run()
         result = self._cnx.commit()
+        self.prerollback.clear()
+        self.postrollback.clear()
         self.postcommit.run()
         return result
 
@@ -450,7 +455,9 @@ class Cursor(BaseCursor):
         clear_env(self)
         self.precommit.clear()
         self.postcommit.clear()
+        self.prerollback.run()
         result = self._cnx.rollback()
+        self.postrollback.run()
         return result
 
     @check
@@ -496,8 +503,8 @@ class TestCursor(BaseCursor):
 
     def close(self):
         if not self._closed:
+            self.rollback()
             self._closed = True
-            self._cursor.execute('ROLLBACK TO SAVEPOINT "%s"' % self._savepoint)
             self._lock.release()
 
     def autocommit(self, on):
@@ -509,16 +516,19 @@ class TestCursor(BaseCursor):
         flush_env(self)
         self.precommit.run()
         self._cursor.execute('SAVEPOINT "%s"' % self._savepoint)
-        # ignore post-commit hooks
-        self.postcommit.clear()
+        self.prerollback.clear()
+        self.postrollback.clear()
+        self.postcommit.clear()         # TestCursor ignores post-commit hooks
 
     @check
     def rollback(self):
         """ Perform an SQL `ROLLBACK` """
         clear_env(self)
         self.precommit.clear()
-        self._cursor.execute('ROLLBACK TO SAVEPOINT "%s"' % self._savepoint)
         self.postcommit.clear()
+        self.prerollback.run()
+        self._cursor.execute('ROLLBACK TO SAVEPOINT "%s"' % self._savepoint)
+        self.postrollback.run()
 
     def __getattr__(self, name):
         value = getattr(self._cursor, name)