From 4499cf68389d609971eab20b50ae691468f780bd Mon Sep 17 00:00:00 2001
From: Jan Kaluza <jkaluza@redhat.com>
Date: Jul 08 2019 08:34:43 +0000
Subject: Allow nesting BaseHandler exception handler decorators.


Previously, when multiple decorated methods were nested and
one of them raised an exception, the exception has been handled
by all the decorators.

This is not right way, because exception which should be handled
just by the `fail_artifact_build_on_handler_exception` decorator
was later re-raised and handled also by the `fail_event_on_handler_exception`
making the `fail_artifact_build_on_handler_exception` useless in this
combination of decorators.

This commit fixes this by changing the decorators to handle the
exception only once. That means only the first decorator will
handle the exception.

---

diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py
index 19437d5..79e1af8 100644
--- a/freshmaker/handlers/__init__.py
+++ b/freshmaker/handlers/__init__.py
@@ -61,6 +61,13 @@ def fail_event_on_handler_exception(func):
         try:
             return func(handler, *args, **kwargs)
         except Exception as e:
+            # Skip the exception in case it has been already handled by
+            # some decorator. This can happen in case multiple decorators
+            # are nested.
+            if handler._last_handled_exception == e:
+                raise
+            handler._last_handled_exception = e
+
             err = 'Could not process message handler. See the traceback.'
             log.exception(err)
 
@@ -99,6 +106,13 @@ def fail_artifact_build_on_handler_exception(whitelist=None):
             try:
                 return func(handler, *args, **kwargs)
             except Exception as e:
+                # Skip the exception in case it has been already handled by
+                # some decorator. This can happen in case multiple decorators
+                # are nested.
+                if handler._last_handled_exception == e:
+                    raise
+                handler._last_handled_exception = e
+
                 if whitelist and type(e) in whitelist:
                     raise
 
@@ -139,6 +153,13 @@ class BaseHandler(object):
         self._db_artifact_build_id = None
         self._log_prefix = ""
         self._force_dry_run = False
+        # Stores the last exception handled by exception handler decorators.
+        # Used in the exception handler decorators to support their nesting.
+        # For example, there can be `fail_artifact_build_on_handler_exception`
+        # used for method already decorated by `fail_event_on_handler_exception`.
+        # In this case, we want the exception to be handled only by the first
+        # decorator but not the others.
+        self._last_handled_exception = None
         self.odcs = FreshmakerODCSClient(self)
 
     def _log(self, log_fnc, msg, *args, **kwargs):
diff --git a/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py b/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py
index e46f9d7..d3f067f 100644
--- a/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py
+++ b/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py
@@ -30,7 +30,8 @@ from freshmaker.events import (
     BrewContainerTaskStateChangeEvent, ErrataAdvisoryRPMsSignedEvent)
 from freshmaker.models import ArtifactBuild, EVENT_TYPES
 from freshmaker.handlers import (ContainerBuildHandler,
-                                 fail_artifact_build_on_handler_exception)
+                                 fail_artifact_build_on_handler_exception,
+                                 fail_event_on_handler_exception)
 from freshmaker.kojiservice import koji_service
 from freshmaker.types import ArtifactType, ArtifactBuildState, EventState
 from freshmaker.utils import get_rebuilt_nvr
@@ -44,6 +45,7 @@ class RebuildImagesOnParentImageBuild(ContainerBuildHandler):
     def can_handle(self, event):
         return isinstance(event, BrewContainerTaskStateChangeEvent)
 
+    @fail_event_on_handler_exception
     def handle(self, event):
         """
         When build container task state changed in brew, update build state in
diff --git a/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py b/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py
index a0ed381..d1d5b53 100644
--- a/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py
+++ b/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py
@@ -272,7 +272,7 @@ class TestRebuildImagesOnParentImageBuild(helpers.ModelsTestCase):
     @mock.patch('freshmaker.handlers.ContainerBuildHandler.build_image_artifact_build')
     @mock.patch('freshmaker.handlers.ContainerBuildHandler.get_repo_urls')
     @mock.patch('freshmaker.handlers.koji.rebuild_images_on_parent_image_build.'
-                'RebuildImagesOnParentImageBuild.update_db_build_state',
+                'RebuildImagesOnParentImageBuild.start_to_build_images',
                 side_effect=RuntimeError('something went wrong!'))
     def test_no_event_state_change_if_service_fails(
             self, update_db, get_repo_urls, build_image_artifact_build):
@@ -287,7 +287,10 @@ class TestRebuildImagesOnParentImageBuild(helpers.ModelsTestCase):
             db.session, self.db_advisory_rpm_signed_event,
             'image-a-0.1-1', ArtifactType.IMAGE,
             build_id=12345,
-            state=ArtifactBuildState.PLANNED.value)
+            state=ArtifactBuildState.PLANNED.value,
+            original_nvr='image-a-0.1-1', rebuilt_nvr='image-a-0.1-2')
+        # Empty json.
+        self.image_a_build.build_args = "{}"
 
         db.session.commit()