Fix errors observed after v4.4.0 release (#543)

* Revert "Ref #520: add typing for action parameters (#523)"

This reverts commit c178e30.

* Include action module name in raised exceptions

* action_factory fixture doesn't return an Action

* Remove test Bugzilla action

We don't include clients in the action object anymore, so this test
class isn't useful

* Add typing for steps

* Rework some tests `test_modules`

- Instead of a test class, assert that the default action serializes
- Rework tests for asserting valid and invalid values for bugzilla_user_id
- use registered `action_factory` instead of import where possible

* use `.get()` for param guard clauses

* Add tests for _all_projects_components_exist

* Add guards for getting params in steps

---------

Co-authored-by: Mathieu Leplatre <[email protected]>

Author: grahamalama

jbi/actions/default.py

@@ -7,7 +7,7 @@
 and when a comment is posted, it will be set to `Operation.COMMENT`.
 """
 import logging
-from typing import Optional
+from typing import Callable, Optional
 
 from jbi import ActionResult, Operation
 from jbi.actions import steps as steps_module
@@ -86,7 +86,7 @@ def init(
 class Executor:
     """Callable class that encapsulates the default action."""
 
-    def __init__(self, steps, **parameters):
+    def __init__(self, steps: dict[Operation, list[Callable]], **parameters):
         """Initialize Executor Object"""
         self.steps = steps
         self.parameters = parameters

jbi/actions/steps.py

@@ -5,7 +5,7 @@
 """
 
 import logging
-from typing import Optional
+from typing import Iterable, Optional
 
 from requests import exceptions as requests_exceptions
 
@@ -154,6 +154,8 @@ def maybe_update_issue_resolution(
     https://support.atlassian.com/jira-cloud-administration/docs/what-are-issue-statuses-priorities-and-resolutions/
     """
     resolution_map: dict[str, str] = parameters.get("resolution_map", {})
+    if not isinstance(resolution_map, dict):
+        raise TypeError("The 'resolution_map' parameter must be a dictionary.")
     jira_resolution = resolution_map.get(context.bug.resolution or "")
     if jira_resolution is None:
         logger.debug(
@@ -181,9 +183,12 @@ def maybe_update_issue_status(context: ActionContext, **parameters):
     Update the Jira issue resolution
     https://support.atlassian.com/jira-cloud-administration/docs/what-are-issue-statuses-priorities-and-resolutions/
     """
-    resolution_map: dict[str, str] = parameters.get("status_map", {})
+    status_map = parameters.get("status_map", {})
+    if not isinstance(status_map, dict):
+        raise TypeError("The 'status_map' parameter must be a dictionary.")
+
     bz_status = context.bug.resolution or context.bug.status
-    jira_status = resolution_map.get(bz_status or "")
+    jira_status = status_map.get(bz_status or "")
 
     if jira_status is None:
         logger.debug(
@@ -212,7 +217,9 @@ def maybe_update_components(context: ActionContext, **parameters):
     """
     Update the Jira issue components
     """
-    config_components: list[str] = parameters.get("jira_components", [])
+    config_components = parameters.get("jira_components", [])
+    if not isinstance(config_components, Iterable):
+        raise TypeError("The 'jira_components' parameter must be an iterable.")
     candidate_components = set(config_components)
     if context.bug.component:
         candidate_components.add(context.bug.component)

jbi/models.py

@@ -9,7 +9,7 @@
 import warnings
 from inspect import signature
 from types import ModuleType
-from typing import Callable, Literal, Mapping, Optional, TypedDict
+from typing import Any, Callable, Literal, Mapping, Optional, TypedDict
 from urllib.parse import ParseResult, urlparse
 
 from pydantic import BaseModel, Extra, Field, PrivateAttr, root_validator, validator
@@ -23,19 +23,6 @@
 JIRA_HOSTNAMES = ("jira", "atlassian")
 
 
-class ActionParameters(BaseModel):
-    """Action parameters"""
-
-    # For runner
-    jira_project_key: str
-    steps: Optional[dict[str, list[str]]] = None
-    # For steps
-    status_map: Optional[dict[str, str]] = None
-    resolution_map: Optional[dict[str, str]] = None
-    jira_components: Optional[list[str]] = None
-    sync_whiteboard_labels: bool = True
-
-
 class Action(YamlModel):
     """
     Action is the inner model for each action in the configuration file"""
@@ -46,21 +33,21 @@ class Action(YamlModel):
     description: str
     enabled: bool = True
     allow_private: bool = False
-    parameters: ActionParameters
+    parameters: dict = {}
     _caller: Optional[Callable] = PrivateAttr(default=None)
     _required_jira_permissions: set[str] = PrivateAttr(default=None)
 
     @property
     def jira_project_key(self):
         """Return the configured project key."""
-        return self.parameters.jira_project_key
+        return self.parameters["jira_project_key"]
 
     @property
     def caller(self) -> Callable:
         """Return the initialized callable for this action."""
         if self._caller is None:
             action_module: ModuleType = importlib.import_module(self.module)
-            initialized: Callable = action_module.init(**self.parameters.dict())  # type: ignore
+            initialized: Callable = action_module.init(**self.parameters)  # type: ignore
             self._caller = initialized
         return self._caller
 
@@ -78,17 +65,14 @@ def validate_action_config(cls, values):  # pylint: disable=no-self-argument
         """Validate action: exists, has init function, and has expected params"""
         try:
             module: str = values["module"]  # type: ignore
-            try:
-                action_parameters = values["parameters"].dict()
-            except KeyError:
-                action_parameters = {}
+            action_parameters: Optional[dict[str, Any]] = values["parameters"]
             action_module: ModuleType = importlib.import_module(module)
             if not action_module:
-                raise TypeError("Module is not found.")
+                raise TypeError(f"Module '{module}' is not found.")
             if not hasattr(action_module, "init"):
-                raise TypeError("Module is missing `init` method.")
+                raise TypeError(f"Module '{module}' is missing `init` method.")
 
-            signature(action_module.init).bind(**action_parameters)
+            signature(action_module.init).bind(**action_parameters)  # type: ignore
         except ImportError as exception:
             raise ValueError(f"unknown Python module `{module}`.") from exception
         except (TypeError, AttributeError) as exception:
@@ -126,7 +110,11 @@ def get(self, tag: Optional[str]) -> Optional[Action]:
     @functools.cached_property
     def configured_jira_projects_keys(self) -> set[str]:
         """Return the list of Jira project keys from all configured actions"""
-        return {action.jira_project_key for action in self.__root__}
+        return {
+            action.jira_project_key
+            for action in self.__root__
+            if action.parameters.get("jira_project_key")
+        }
 
     @validator("__root__")
     def validate_actions(  # pylint: disable=no-self-argument

jbi/runner.py

@@ -81,7 +81,7 @@ def execute_action(
             event=event,
             operation=Operation.IGNORE,
             jira=JiraContext(project=action.jira_project_key, issue=linked_issue_key),
-            extra={k: str(v) for k, v in action.parameters.dict().items()},
+            extra={k: str(v) for k, v in action.parameters.items()},
         )
 
         if action_context.jira.issue is None:

jbi/services/jira.py

@@ -129,8 +129,9 @@ def _all_projects_permissions(actions: Actions):
 def _fetch_project_permissions(actions):
     """Fetches permissions for the configured projects"""
     required_perms_by_project = {
-        action.parameters.jira_project_key: action.required_jira_permissions
+        action.parameters["jira_project_key"]: action.required_jira_permissions
         for action in actions
+        if action.parameters.get("jira_project_key")
     }
     client = get_client()
     all_projects_perms = {}
@@ -183,8 +184,9 @@ def _validate_permissions(all_projects_perms):
 
 def _all_projects_components_exist(actions: Actions):
     components_by_project = {
-        action.parameters.jira_project_key: action.parameters.jira_components
+        action.parameters["jira_project_key"]: action.parameters["jira_components"]
         for action in actions
+        if action.parameters.get("jira_components")
     }
     success = True
     for project, specified_components in components_by_project.items():

tests/conftest.py

@@ -229,7 +229,7 @@ def webhook_modify_private_example() -> BugzillaWebhookRequest:
 
 
 @pytest.fixture
-def action_factory() -> Action:
+def action_factory():
     return factories.action_factory
 
 

tests/fixtures/bugzilla_action.py

@@ -7,6 +7,7 @@
 from requests.exceptions import ConnectionError
 
 from jbi.environment import get_settings
+from jbi.models import Actions
 from jbi.services import jira
 
 
@@ -84,3 +85,38 @@ def test_jira_retries_failing_connections_in_health_check(
         jira.check_health(actions_example)
 
     assert len(mocked_responses.calls) == 4
+
+
+@pytest.mark.parametrize(
+    "jira_components, project_components, expected_result",
+    [
+        (["Foo"], [{"name": "Foo"}], True),
+        (["Foo"], [{"name": "Foo"}, {"name": "Bar"}], True),
+        (["Foo", "Bar"], [{"name": "Foo"}, {"name": "Bar"}], True),
+        (None, [], True),
+        (["Foo"], [{"name": "Bar"}], False),
+        (["Foo", "Bar"], [{"name": "Foo"}], False),
+        (["Foo"], [], False),
+    ],
+)
+def test_all_projects_components_exist(
+    mocked_jira, action_factory, jira_components, project_components, expected_result
+):
+    action = action_factory(
+        parameters={"jira_project_key": "ABC", "jira_components": jira_components}
+    )
+    mocked_jira.get_project_components.return_value = project_components
+    actions = Actions(__root__=[action])
+    result = jira._all_projects_components_exist(actions)
+    assert result is expected_result
+
+
+def test_all_projects_components_exist_no_components_param(action_factory):
+    action = action_factory(
+        parameters={
+            "jira_project_key": "ABC",
+        }
+    )
+    actions = Actions(__root__=[action])
+    result = jira._all_projects_components_exist(actions)
+    assert result is True

tests/unit/services/test_jira.py

@@ -1,58 +1,33 @@
+import pydantic
 import pytest
 from fastapi.encoders import jsonable_encoder
 
-from jbi.models import Action, ActionParameters, Actions
-from tests.fixtures.factories import action_factory
+from jbi import Operation
+from jbi.models import Actions
+from tests.fixtures.factories import action_context_factory
 
 
-def test_model_serializes():
-    """Regression test to assert that action with initialized Bugzilla client serializes"""
-    action = Action.parse_obj(
-        {
-            "whiteboard_tag": "devtest",
-            "bugzilla_user_id": 123456,
-            "description": "test config",
-            "module": "tests.fixtures.bugzilla_action",
-            "parameters": ActionParameters(jira_project_key="fooo"),
-        }
+def test_default_action_serializes(action_factory):
+    action = action_factory(
+        module="jbi.actions.default",
+        parameters={"jira_project_key": "ABC", "steps": {"new": []}},
     )
-    action.caller(bug=None, event=None)
+    action.caller(action_context_factory(operation=Operation.CREATE))
     serialized_action = jsonable_encoder(action)
     assert not serialized_action.get("_caller")
     assert not serialized_action.get("caller")
 
 
-def test_model_with_user_list_serializes():
-    """Regression test to assert that action with initialized Bugzilla client serializes (with user id list)"""
-    action = Action.parse_obj(
-        {
-            "whiteboard_tag": "devtest",
-            "bugzilla_user_id": [123456, 654321, 000000, 111111],
-            "description": "test config",
-            "module": "tests.fixtures.bugzilla_action",
-            "parameters": ActionParameters(jira_project_key="fooo"),
-        }
-    )
-    action.caller(bug=None, event=None)
-    serialized_action = jsonable_encoder(action)
-    assert not serialized_action.get("_caller")
-    assert not serialized_action.get("caller")
+@pytest.mark.parametrize("value", [123456, [123456], [12345, 67890], "tbd"])
+def test_valid_bugzilla_user_ids(action_factory, value):
+    action = action_factory(bugzilla_user_id=value)
+    assert action.bugzilla_user_id == value
 
-    def test_model_with_user_list_of_one_serializes():
-        """Regression test to assert that action with initialized Bugzilla client serializes (with user id in list)"""
-        action = Action.parse_obj(
-            {
-                "whiteboard_tag": "devtest",
-                "bugzilla_user_id": [123456],
-                "description": "test config",
-                "module": "tests.fixtures.bugzilla_action",
-                "parameters": ActionParameters(jira_project_key="fooo"),
-            }
-        )
-        action.caller(bug=None, event=None)
-        serialized_action = jsonable_encoder(action)
-        assert not serialized_action.get("_caller")
-        assert not serialized_action.get("caller")
+
+@pytest.mark.parametrize("value", [None, "[email protected]"])
+def test_invalid_bugzilla_user_ids(action_factory, value):
+    with pytest.raises(pydantic.ValidationError):
+        action = action_factory(bugzilla_user_id=value)
 
 
 def test_no_actions_fails():
@@ -69,11 +44,13 @@ def test_unknown_module_fails():
 
 def test_bad_module_fails():
     with pytest.raises(ValueError) as exc_info:
+        # use a module that exists in the source, but isn't properly set up as
+        # a valid action module
         Actions.parse_obj([{"whiteboard_tag": "x", "module": "jbi.runner"}])
     assert "action 'jbi.runner' is not properly setup" in str(exc_info.value)
 
 
-def test_duplicated_whiteboard_tag_fails():
+def test_duplicated_whiteboard_tag_fails(action_factory):
     with pytest.raises(ValueError) as exc_info:
         Actions.parse_obj(
             [