PR#399: Allow nesting BaseHandler exception handler decorators. - freshmaker

freshmaker

#399 Allow nesting BaseHandler exception handler decorators.

Merged 4 years ago by jkaluza. Opened 4 years ago by jkaluza.

jkaluza/freshmaker nesting-decorators into master

Allow nesting BaseHandler exception handler decorators.

Jan Kaluza • 4 years ago

4499cf6

freshmaker/handlers/__init__.py

file modified

+21

		`@@ -61,6 +61,13 @@`
		`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 @@`
		`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 @@`
		`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):`

freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py

file modified

+3 -1

		`@@ -30,7 +30,8 @@`
		`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 @@`
		`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`

tests/handlers/koji/test_rebuild_images_on_parent_image_build.py

file modified

+5 -2

		`@@ -272,7 +272,7 @@`
		`@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 @@`
		`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()`

jkaluza commented 4 years ago

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.

jkaluza commented 4 years ago

@yashn, the test change here is related to your original PR. I have found out there was one issue I've fixed here.

You originally raised fake exception directly from RebuildImagesOnParentImageBuild.update_db_build_state by patching it. It seems this is not correct, because the update_db_build_state is already decorated, so the patch method just patched that method including the decorator and the decorator was therefore never called.

I've changed that test to patch start_to_build_images instead, which is the method called in the update_db_build_state. That way, the decorator stays there and handles the exception raised by the start_to_build_images.

yashn commented 4 years ago

@jkaluza makes sense. Thanks!

Just to make sure I understand correctly, the change you added basically stops a decorator from handling the exception because some other decorator has already handled it right?

LGTM! :)

jkaluza commented 4 years ago

@yashn, yes, correct.

jkaluza commented 4 years ago

Commit 8b51949 fixes this pull-request

Pull-Request has been merged by jkaluza

4 years ago

Pull-Request has been merged by jkaluza

4 years ago