[FIX] rest_log: Don't prevent the retrying mechanism

In case of a retryable error, the initial error must bubble up to the retrying mechanism. If this kind of error is wrapped into another one, the retrying mechanism no more works

Author: lmignon

rest_log/components/service.py

@@ -7,11 +7,13 @@
 import logging
 import traceback
 
+from psycopg2.errors import OperationalError
 from werkzeug.urls import url_encode, url_join
 
 from odoo import exceptions
 from odoo.http import Response, request
 from odoo.modules.registry import Registry
+from odoo.service.model import PG_CONCURRENCY_ERRORS_TO_RETRY
 
 from odoo.addons.base_rest.http import JSONEncoder
 from odoo.addons.component.core import AbstractComponent
@@ -112,6 +114,15 @@ def _dispatch_exception(
                 log_entry_url = self._get_log_entry_url(log_entry)
         except Exception as e:
             _logger.exception("Rest Log Error Creation: %s", e)
+        # let the OperationalError bubble up to the retrying mechanism
+        # We can't wrap the OperationalError because we want to let it
+        # bubble up to the retrying mechanism, it will be handled by
+        # the default handler at the end of the chain.
+        if (
+            isinstance(orig_exception, OperationalError)
+            and orig_exception.pgcode in PG_CONCURRENCY_ERRORS_TO_RETRY
+        ):
+            raise orig_exception
         raise exception_klass(exc_msg, log_entry_url) from orig_exception
 
     def _get_exception_message(self, exception):

rest_log/tests/common.py

@@ -4,6 +4,9 @@
 
 import contextlib
 
+from psycopg2 import errorcodes
+from psycopg2.errors import OperationalError
+
 from odoo import exceptions
 
 from odoo.addons.base_rest import restapi
@@ -42,6 +45,7 @@ def fail(self, how):
                     "value": ValueError,
                     "validation": exceptions.ValidationError,
                     "user": exceptions.UserError,
+                    "retryable": FakeConcurrentUpdateError,
                 }
                 raise exc[how]("Failed as you wanted!")
 
@@ -61,3 +65,9 @@ def _get_mocked_request(self, env=None, httprequest=None, extra_headers=None):
             headers.update(extra_headers or {})
             mocked_request.httprequest.headers = headers
             yield mocked_request
+
+
+class FakeConcurrentUpdateError(OperationalError):
+    @property
+    def pgcode(self):
+        return errorcodes.SERIALIZATION_FAILURE

rest_log/tests/test_db_logging.py

@@ -13,7 +13,7 @@
 from odoo.addons.component.tests.common import new_rollbacked_env
 from odoo.addons.rest_log import exceptions as log_exceptions  # pylint: disable=W7950
 
-from .common import TestDBLoggingMixin
+from .common import FakeConcurrentUpdateError, TestDBLoggingMixin
 
 
 class TestDBLogging(TransactionRestServiceRegistryCase, TestDBLoggingMixin):
@@ -374,3 +374,66 @@ def test_log_exception_value(self):
         self._test_exception(
             "value", log_exceptions.RESTServiceDispatchException, "ValueError", "severe"
         )
+
+
+class TestDBLoggingRetryableError(
+    TransactionRestServiceRegistryCase, TestDBLoggingMixin
+):
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls._setup_registry(cls)
+
+    @classmethod
+    def tearDownClass(cls):
+        # pylint: disable=W8110
+        cls._teardown_registry(cls)
+        super().tearDownClass()
+
+    def _test_exception(self, test_type, wrapping_exc, exc_name, severity):
+        log_model = self.env["rest.log"].sudo()
+        initial_entries = log_model.search([])
+        # Context: we are running in a transaction case which uses savepoints.
+        # The log machinery is going to rollback the transation when catching errors.
+        # Hence we need a completely separated env for the service.
+        with new_rollbacked_env() as new_env:
+            # Init fake collection w/ new env
+            collection = _PseudoCollection(self._collection_name, new_env)
+            service = self._get_service(self, collection=collection)
+            with self._get_mocked_request(env=new_env):
+                try:
+                    service.dispatch("fail", test_type)
+                except Exception as err:
+                    # Not using `assertRaises` to inspect the exception directly
+                    self.assertTrue(isinstance(err, wrapping_exc))
+                    self.assertEqual(
+                        service._get_exception_message(err), "Failed as you wanted!"
+                    )
+
+        with new_rollbacked_env() as new_env:
+            log_model = new_env["rest.log"].sudo()
+            entry = log_model.search([]) - initial_entries
+            expected = {
+                "collection": service._collection,
+                "state": "failed",
+                "result": "null",
+                "exception_name": exc_name,
+                "exception_message": "Failed as you wanted!",
+                "severity": severity,
+            }
+            self.assertRecordValues(entry, [expected])
+
+    @staticmethod
+    def _get_test_controller(class_or_instance, root_path=None):
+        return super()._get_test_controller(
+            class_or_instance, root_path="/test_log_exception_retryable/"
+        )
+
+    def test_log_exception_retryable(self):
+        # retryable error must bubble up to the retrying mechanism
+        self._test_exception(
+            "retryable",
+            FakeConcurrentUpdateError,
+            "odoo.addons.rest_log.tests.common.FakeConcurrentUpdateError",
+            "warning",
+        )