Move initialized action callable to private field (#145)

* Move initialized callable to private field

When we included the initialized callable in a public model field, we ran
into a bug where `jsonable_encoder` couldn't serialzie an action with an
initalized `requests.Session` object.

In this commit, we move the initialized caller to a private field so that it's
excluded during serialization.

* Remove now-irrelevant Config from Action model

* Improve fake Bugzilla action object
- change name
- caller takes and returns a payload

* Make action caller a property

Pydantic does not include properties in the serialized object, which is
exactly what we need in this case.

This commit also changes the callable name to `caller` so we avoid
mirroring the built-in `callable` function

Author: grahamalama

src/app/api.py

@@ -10,6 +10,7 @@
 import sentry_sdk
 import uvicorn  # type: ignore
 from fastapi import Body, Depends, FastAPI, Request
+from fastapi.encoders import jsonable_encoder
 from fastapi.responses import HTMLResponse
 from fastapi.staticfiles import StaticFiles
 from fastapi.templating import Jinja2Templates
@@ -109,10 +110,8 @@ def get_whiteboard_tags(
 ):
     """API for viewing whiteboard_tags and associated data"""
     if existing := actions.get(whiteboard_tag):
-        filtered = {whiteboard_tag: existing}
-    else:
-        filtered = actions.by_tag  # type: ignore
-    return {k: v.dict(exclude={"callable"}) for k, v in filtered.items()}
+        return {whiteboard_tag: existing}
+    return actions.by_tag
 
 
 @app.get("/jira_projects/")
@@ -132,7 +131,7 @@ def powered_by_jbi(
     context = {
         "request": request,
         "title": "Powered by JBI",
-        "actions": [action.dict(exclude={"callable"}) for action in actions],
+        "actions": jsonable_encoder(actions),
         "enable_query": enabled,
     }
     return templates.TemplateResponse("powered_by_template.html", context)

src/jbi/models.py

@@ -8,7 +8,7 @@
 from types import ModuleType
 from typing import Any, Callable, Dict, List, Literal, Mapping, Optional, Set, Union
 
-from pydantic import EmailStr, Extra, Field, root_validator, validator
+from pydantic import EmailStr, Field, PrivateAttr, root_validator, validator
 from pydantic_yaml import YamlModel
 
 
@@ -24,13 +24,16 @@ class Action(YamlModel):
     enabled: bool = False
     allow_private: bool = False
     parameters: dict = {}
+    _caller: Callable = PrivateAttr(default=None)
 
-    @functools.cached_property
-    def callable(self) -> Callable:
+    @property
+    def caller(self) -> Callable:
         """Return the initialized callable for this action."""
-        action_module: ModuleType = importlib.import_module(self.module)
-        initialized: Callable = action_module.init(**self.parameters)  # type: ignore
-        return initialized
+        if not self._caller:
+            action_module: ModuleType = importlib.import_module(self.module)
+            initialized: Callable = action_module.init(**self.parameters)  # type: ignore
+            self._caller = initialized
+        return self._caller
 
     @root_validator
     def validate_action_config(cls, values):  # pylint: disable=no-self-argument
@@ -51,13 +54,6 @@ def validate_action_config(cls, values):  # pylint: disable=no-self-argument
             raise ValueError(f"action is not properly setup.{exception}") from exception
         return values
 
-    class Config:
-        """Pydantic configuration"""
-
-        extra = Extra.allow
-        keep_untouched = (functools.cached_property,)
-        fields = {"callable": {"exclude": True}}
-
 
 class Actions(YamlModel):
     """

src/jbi/runner.py

@@ -77,7 +77,7 @@ def execute_action(
             extra={"operation": Operations.EXECUTE, **log_context},
         )
 
-        content = action.callable(payload=request)
+        content = action.caller(payload=request)
 
         logger.info(
             "Action %r executed successfully for Bug %s",

tests/unit/conftest.py

@@ -8,9 +8,8 @@
 
 from src.app.api import app
 from src.app.environment import Settings
-from src.jbi.bugzilla import BugzillaBug, BugzillaWebhookComment, BugzillaWebhookRequest
-from src.jbi.models import Actions
-from src.jbi.services import get_bugzilla
+from src.jbi.bugzilla import BugzillaWebhookComment, BugzillaWebhookRequest
+from src.jbi.models import Action, Actions
 
 
 @pytest.fixture
@@ -25,13 +24,13 @@ def settings():
     return Settings()
 
 
-@pytest.fixture
+@pytest.fixture(autouse=True)
 def mocked_bugzilla():
     with mock.patch("src.jbi.services.rh_bugzilla.Bugzilla") as mocked_bz:
         yield mocked_bz
 
 
-@pytest.fixture
+@pytest.fixture(autouse=True)
 def mocked_jira():
     with mock.patch("src.jbi.services.Jira") as mocked_jira:
         yield mocked_jira
@@ -167,14 +166,17 @@ def webhook_modify_private_example(
 
 
 @pytest.fixture
-def actions_example() -> Actions:
-    return Actions.parse_obj(
-        [
-            {
-                "whiteboard_tag": "devtest",
-                "contact": "[email protected]",
-                "description": "test config",
-                "module": "tests.unit.jbi.noop_action",
-            }
-        ]
+def action_example() -> Action:
+    return Action.parse_obj(
+        {
+            "whiteboard_tag": "devtest",
+            "contact": "[email protected]",
+            "description": "test config",
+            "module": "tests.unit.jbi.noop_action",
+        }
     )
+
+
+@pytest.fixture
+def actions_example(action_example) -> Actions:
+    return Actions.parse_obj([action_example])

tests/unit/jbi/bugzilla_action.py

@@ -0,0 +1,16 @@
+from bugzilla import Bugzilla
+from requests import Session
+
+session = Session()
+
+
+class FakeBugzillaAction:
+    def __init__(self, **params):
+        self.bz = Bugzilla(url=None, requests_session=session)
+
+    def __call__(self, payload):
+        return {"payload": payload}
+
+
+def init():
+    return FakeBugzillaAction()

tests/unit/jbi/test_models.py

@@ -0,0 +1,21 @@
+from unittest.mock import patch
+
+from fastapi.encoders import jsonable_encoder
+
+from src.jbi.models import Action
+
+
+def test_model_serializes():
+    """Regression test to assert that action with initialzed Bugzilla client serializes"""
+    action = Action.parse_obj(
+        {
+            "whiteboard_tag": "devtest",
+            "contact": "[email protected]",
+            "description": "test config",
+            "module": "tests.unit.jbi.bugzilla_action",
+        }
+    )
+    action.caller(payload=action)
+    serialized_action = jsonable_encoder(action)
+    assert not serialized_action.get("_caller")
+    assert not serialized_action.get("caller")