Use enum to differentiate different results of running a step (#761)

In this commit, we introduce an Enum that represents the different ways a step can resolve. This will:

- help us track how often certain steps are called when they do "meaningful work"
- helps as avoid flow-control via Exceptions

Author: grahamalama

jbi/errors.py

@@ -11,11 +11,3 @@ class IgnoreInvalidRequestError(Exception):
 
 class ActionError(Exception):
     """Error occurred during Action handling"""
-
-
-class IncompleteStepError(Exception):
-    """Raised when a step could not complete successfully."""
-
-    def __init__(self, context, *args: object) -> None:
-        super().__init__(*args)
-        self.context = context

jbi/runner.py

@@ -11,11 +11,7 @@
 from jbi import ActionResult, Operation
 from jbi import steps as steps_module
 from jbi.environment import get_settings
-from jbi.errors import (
-    ActionNotFoundError,
-    IgnoreInvalidRequestError,
-    IncompleteStepError,
-)
+from jbi.errors import ActionNotFoundError, IgnoreInvalidRequestError
 from jbi.models import (
     ActionContext,
     ActionParams,
@@ -26,6 +22,7 @@
     RunnerContext,
 )
 from jbi.services import bugzilla, jira
+from jbi.steps import StepStatus
 
 logger = logging.getLogger(__name__)
 
@@ -102,16 +99,16 @@ def __call__(self, context: ActionContext) -> ActionResult:
 
         for step in self.steps[context.operation]:
             context = context.update(current_step=step.__name__)
+            step_kwargs = self.build_step_kwargs(step)
             try:
-                step_kwargs = self.build_step_kwargs(step)
-                context = step(context=context, **step_kwargs)
-                statsd.incr(f"jbi.steps.{step.__name__}.count")
-            except IncompleteStepError as exc:
-                # Step did not execute all its operations.
-                context = exc.context
-                statsd.incr(
-                    f"jbi.action.{context.action.whiteboard_tag}.incomplete.count"
-                )
+                result, context = step(context=context, **step_kwargs)
+                if result == StepStatus.SUCCESS:
+                    statsd.incr(f"jbi.steps.{step.__name__}.count")
+                elif result == StepStatus.INCOMPLETE:
+                    # Step did not execute all its operations.
+                    statsd.incr(
+                        f"jbi.action.{context.action.whiteboard_tag}.incomplete.count"
+                    )
             except Exception:
                 if has_produced_request:
                     # Count the number of workflows that produced at least one request,

jbi/steps.py

@@ -11,23 +11,39 @@
 from __future__ import annotations
 
 import logging
+from enum import Enum, auto
 from typing import TYPE_CHECKING, Optional
 
 from requests import exceptions as requests_exceptions
 
 from jbi import Operation
-from jbi.errors import IncompleteStepError
+
+
+class StepStatus(Enum):
+    """
+    Options for the result of executing a step function:
+        SUCCESS: The step succeeded at doing meaningful work
+        INCOMPLETE: The step did not execute successfully, but it's an error we anticipated
+        NOOP: The step executed successfully, but didn't have any meaningful work to do
+    """
+
+    SUCCESS = auto()
+    INCOMPLETE = auto()
+    NOOP = auto()
+
 
 # https://docs.python.org/3.11/library/typing.html#typing.TYPE_CHECKING
 if TYPE_CHECKING:
     from jbi.models import ActionContext, ActionParams
     from jbi.services.bugzilla import BugzillaService
     from jbi.services.jira import JiraService
 
+    StepResult = tuple[StepStatus, ActionContext]
+
 logger = logging.getLogger(__name__)
 
 
-def create_comment(context: ActionContext, *, jira_service: JiraService):
+def create_comment(context: ActionContext, *, jira_service: JiraService) -> StepResult:
     """Create a Jira comment using `context.bug.comment`"""
     bug = context.bug
 
@@ -36,11 +52,11 @@ def create_comment(context: ActionContext, *, jira_service: JiraService):
             "No matching comment found in payload",
             extra=context.model_dump(),
         )
-        return context
+        return (StepStatus.NOOP, context)
 
     jira_response = jira_service.add_jira_comment(context)
     context = context.append_responses(jira_response)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
 def create_issue(
@@ -49,7 +65,7 @@ def create_issue(
     parameters: ActionParams,
     jira_service: JiraService,
     bugzilla_service: BugzillaService,
-):
+) -> StepResult:
     """Create the Jira issue with the first comment as the description."""
     bug = context.bug
     issue_type = parameters.issue_type_map.get(bug.type or "", "Task")
@@ -65,29 +81,33 @@ def create_issue(
         jira=context.jira.update(issue=issue_key),
     )
     context = context.append_responses(jira_create_response)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
-def add_link_to_jira(context: ActionContext, *, bugzilla_service: BugzillaService):
+def add_link_to_jira(
+    context: ActionContext, *, bugzilla_service: BugzillaService
+) -> StepResult:
     """Add the URL to the Jira issue in the `see_also` field on the Bugzilla ticket"""
     bugzilla_response = bugzilla_service.add_link_to_jira(context)
     context = context.append_responses(bugzilla_response)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
-def add_link_to_bugzilla(context: ActionContext, *, jira_service: JiraService):
+def add_link_to_bugzilla(
+    context: ActionContext, *, jira_service: JiraService
+) -> StepResult:
     """Add the URL of the Bugzilla ticket to the links of the Jira issue"""
     jira_response = jira_service.add_link_to_bugzilla(context)
     context = context.append_responses(jira_response)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
 def maybe_delete_duplicate(
     context: ActionContext,
     *,
     bugzilla_service: BugzillaService,
     jira_service: JiraService,
-):
+) -> StepResult:
     """
     In the time taken to create the Jira issue the bug may have been updated so
     re-retrieve it to ensure we have the latest data, and delete any duplicate
@@ -99,29 +119,36 @@ def maybe_delete_duplicate(
     )
     if jira_response_delete:
         context = context.append_responses(jira_response_delete)
-    return context
+        return (StepStatus.SUCCESS, context)
+    return (StepStatus.NOOP, context)
 
 
-def update_issue_summary(context: ActionContext, *, jira_service: JiraService):
+def update_issue_summary(
+    context: ActionContext, *, jira_service: JiraService
+) -> StepResult:
     """Update the Jira issue's summary if the linked bug is modified."""
 
     if "summary" not in context.event.changed_fields():
-        return context
+        return (StepStatus.NOOP, context)
 
     jira_response_update = jira_service.update_issue_summary(context)
     context = context.append_responses(jira_response_update)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
-def add_jira_comments_for_changes(context: ActionContext, *, jira_service: JiraService):
+def add_jira_comments_for_changes(
+    context: ActionContext, *, jira_service: JiraService
+) -> StepResult:
     """Add a Jira comment for each field (assignee, status, resolution) change on
     the Bugzilla ticket."""
     comments_responses = jira_service.add_jira_comments_for_changes(context)
     context.append_responses(comments_responses)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
-def maybe_assign_jira_user(context: ActionContext, *, jira_service: JiraService):
+def maybe_assign_jira_user(
+    context: ActionContext, *, jira_service: JiraService
+) -> StepResult:
     """Assign the user on the Jira issue, based on the Bugzilla assignee email.
 
     It will attempt to assign the Jira issue the same person as the bug is assigned to. This relies on
@@ -134,19 +161,19 @@ def maybe_assign_jira_user(context: ActionContext, *, jira_service: JiraService)
 
     if context.operation == Operation.CREATE:
         if not bug.is_assigned():
-            return context
+            return (StepStatus.NOOP, context)
 
         try:
             resp = jira_service.assign_jira_user(context, bug.assigned_to)  # type: ignore
             context.append_responses(resp)
-            return context
+            return (StepStatus.SUCCESS, context)
         except ValueError as exc:
             logger.debug(str(exc), extra=context.model_dump())
-            raise IncompleteStepError(context) from exc
+            return (StepStatus.INCOMPLETE, context)
 
     if context.operation == Operation.UPDATE:
         if "assigned_to" not in event.changed_fields():
-            return context
+            return (StepStatus.SUCCESS, context)
 
         if not bug.is_assigned():
             resp = jira_service.clear_assignee(context)
@@ -158,15 +185,14 @@ def maybe_assign_jira_user(context: ActionContext, *, jira_service: JiraService)
                 # If that failed then just fall back to clearing the assignee.
                 resp = jira_service.clear_assignee(context)
         context.append_responses(resp)
-        return context
+        return (StepStatus.SUCCESS, context)
 
-    # This happens when exceptions are raised an ignored.
-    return context
+    return (StepStatus.NOOP, context)
 
 
 def maybe_update_issue_resolution(
     context: ActionContext, *, parameters: ActionParams, jira_service: JiraService
-):
+) -> StepResult:
     """
     Update the Jira issue status
     https://support.atlassian.com/jira-cloud-administration/docs/what-are-issue-statuses-priorities-and-resolutions/
@@ -183,25 +209,25 @@ def maybe_update_issue_resolution(
                 operation=Operation.IGNORE,
             ).model_dump(),
         )
-        raise IncompleteStepError(context)
+        return (StepStatus.INCOMPLETE, context)
 
     if context.operation == Operation.CREATE:
         resp = jira_service.update_issue_resolution(context, jira_resolution)
         context.append_responses(resp)
-        return context
+        return (StepStatus.SUCCESS, context)
 
     if context.operation == Operation.UPDATE:
         if "resolution" in context.event.changed_fields():
             resp = jira_service.update_issue_resolution(context, jira_resolution)
             context.append_responses(resp)
-            return context
+            return (StepStatus.SUCCESS, context)
 
-    return context
+    return (StepStatus.NOOP, context)
 
 
 def maybe_update_issue_status(
     context: ActionContext, *, parameters: ActionParams, jira_service: JiraService
-):
+) -> StepResult:
     """
     Update the Jira issue resolution
     https://support.atlassian.com/jira-cloud-administration/docs/what-are-issue-statuses-priorities-and-resolutions/
@@ -217,27 +243,27 @@ def maybe_update_issue_status(
                 operation=Operation.IGNORE,
             ).model_dump(),
         )
-        raise IncompleteStepError(context)
+        return (StepStatus.INCOMPLETE, context)
 
     if context.operation == Operation.CREATE:
         resp = jira_service.update_issue_status(context, jira_status)
         context.append_responses(resp)
-        return context
+        return (StepStatus.SUCCESS, context)
 
     if context.operation == Operation.UPDATE:
         changed_fields = context.event.changed_fields()
 
         if "status" in changed_fields or "resolution" in changed_fields:
             resp = jira_service.update_issue_status(context, jira_status)
             context.append_responses(resp)
-            return context
+            return (StepStatus.SUCCESS, context)
 
-    return context
+    return (StepStatus.NOOP, context)
 
 
 def maybe_update_components(
     context: ActionContext, *, parameters: ActionParams, jira_service: JiraService
-):
+) -> StepResult:
     """
     Update the Jira issue components
     """
@@ -254,7 +280,7 @@ def maybe_update_components(
 
     if not candidate_components:
         # no components to update
-        return context
+        return (StepStatus.NOOP, context)
 
     # Although we previously introspected the project components, we
     # still have to catch any potential 400 error response here, because
@@ -280,18 +306,18 @@ def maybe_update_components(
             extra=context.model_dump(),
         )
         context.append_responses(exc.response)
-        raise IncompleteStepError(context) from exc
+        return (StepStatus.INCOMPLETE, context)
 
     if missing_components:
         logger.warning(
             "Could not find components '%s' in project",
             ",".join(sorted(missing_components)),
             extra=context.model_dump(),
         )
-        raise IncompleteStepError(context)
+        return (StepStatus.INCOMPLETE, context)
 
     context.append_responses(resp)
-    return context
+    return (StepStatus.SUCCESS, context)
 
 
 def _whiteboard_as_labels(labels_brackets: str, whiteboard: Optional[str]) -> list[str]:
@@ -328,7 +354,7 @@ def _build_labels_update(
 
 def sync_whiteboard_labels(
     context: ActionContext, *, parameters: ActionParams, jira_service: JiraService
-):
+) -> StepResult:
     """
     Set whiteboard tags as labels on the Jira issue.
     """
@@ -342,8 +368,7 @@ def sync_whiteboard_labels(
                 labels_brackets=parameters.labels_brackets,
             )
         else:
-            # Whiteboard field not changed, ignore.
-            return context
+            return (StepStatus.NOOP, context)
     else:
         # On creation, just add them all.
         additions, removals = _build_labels_update(
@@ -368,7 +393,7 @@ def sync_whiteboard_labels(
             extra=context.model_dump(),
         )
         context.append_responses(exc.response)
-        raise IncompleteStepError(context) from exc
+        return (StepStatus.INCOMPLETE, context)
 
     context.append_responses(resp)
-    return context
+    return (StepStatus.SUCCESS, context)