New structure for Jira and Bugzilla clients + services (#795)

In this commit, we introduce a slightly new structure for the code we use to communicate with Jira and Bugzilla.

Before, we had a `services` package with a `jira` and `bugzilla` module. 
```
jbi
├── services
    ├── __init__.py
    ├── bugzilla.py
    ├── common.py
    └── jira.py
```
Each of those modules had a `*Client` and `*Service` class.

Now, we do essentially the same thing, except we have:

```
jbi
├── bugzilla
│   ├── __init__.py
│   ├── client.py
│   └── service.py
├── common
│   ├── __init__.py
│   └── instrument.py
└── jira
    ├── __init__.py
    ├── client.py
    └── service.py
```

this is to:
  - reinforce the separate concerns of a `client` and `service`, especially in
  tests
    - clients should only make HTTP requests and receive responses 
    - services should use clients to do higher-level project operations
  - allow for the continued encapsulation of different domains.  For example,
  instead of one `models` module, it might make sense to move certain models
  under specific namespaces

Author: grahamalama

jbi/bugzilla/__init__.py

@@ -0,0 +1,2 @@
+from .service import BugzillaService as BugzillaService
+from .service import get_service as get_service

jbi/services/bugzilla.py renamed to jbi/bugzilla/client.py

@@ -1,25 +1,17 @@
-"""Contains a Bugzilla REST client and functions comprised of common operations
-with that REST client
-"""
-
 import logging
-from functools import lru_cache
 
 import requests
-from statsd.defaults.env import statsd
 
-from jbi import Operation, environment
+from jbi import environment
+from jbi.common.instrument import instrument
 from jbi.models import (
-    ActionContext,
     BugzillaApiResponse,
     BugzillaBug,
     BugzillaComment,
     BugzillaComments,
     BugzillaWebhooksResponse,
 )
 
-from .common import ServiceHealth, instrument
-
 settings = environment.get_settings()
 
 logger = logging.getLogger(__name__)
@@ -132,100 +124,3 @@ def list_webhooks(self):
                 f"Unexpected response content from 'GET {url}' (no 'webhooks' field)"
             )
         return [wh for wh in parsed.webhooks if "/bugzilla_webhook" in wh.url]
-
-
-class BugzillaService:
-    """Used by action workflows to perform action-specific Bugzilla tasks"""
-
-    def __init__(self, client: BugzillaClient) -> None:
-        self.client = client
-
-    def check_health(self) -> ServiceHealth:
-        """Check health for Bugzilla Service"""
-        logged_in = self.client.logged_in()
-        all_webhooks_enabled = False
-        if logged_in:
-            all_webhooks_enabled = self._all_webhooks_enabled()
-
-        health: ServiceHealth = {
-            "up": logged_in,
-            "all_webhooks_enabled": all_webhooks_enabled,
-        }
-        return health
-
-    def _all_webhooks_enabled(self):
-        # Check that all JBI webhooks are enabled in Bugzilla,
-        # and report disabled ones.
-
-        try:
-            jbi_webhooks = self.client.list_webhooks()
-        except (BugzillaClientError, requests.HTTPError):
-            return False
-
-        if len(jbi_webhooks) == 0:
-            logger.info("No webhooks enabled")
-            return True
-
-        for webhook in jbi_webhooks:
-            # Report errors in each webhook
-            statsd.gauge(f"jbi.bugzilla.webhooks.{webhook.slug}.errors", webhook.errors)
-            # Warn developers when there are errors
-            if webhook.errors > 0:
-                logger.warning(
-                    "Webhook %s has %s error(s)", webhook.name, webhook.errors
-                )
-            if not webhook.enabled:
-                logger.error(
-                    "Webhook %s is disabled (%s errors)",
-                    webhook.name,
-                    webhook.errors,
-                )
-                return False
-        return True
-
-    def add_link_to_jira(self, context: ActionContext):
-        """Add link to Jira in Bugzilla ticket"""
-        bug = context.bug
-        issue_key = context.jira.issue
-        jira_url = f"{settings.jira_base_url}browse/{issue_key}"
-        logger.debug(
-            "Link %r on Bug %s",
-            jira_url,
-            bug.id,
-            extra=context.update(operation=Operation.LINK).model_dump(),
-        )
-        return self.client.update_bug(bug.id, see_also={"add": [jira_url]})
-
-    def get_description(self, bug_id: int):
-        """Fetch a bug's description
-
-        A Bug's description does not appear in the payload of a bug. Instead, it is "comment 0"
-        """
-
-        comment_list = self.client.get_comments(bug_id)
-        comment_body = comment_list[0].text if comment_list else ""
-        return str(comment_body)
-
-    def refresh_bug_data(self, bug: BugzillaBug):
-        """Re-fetch a bug to ensure we have the most up-to-date data"""
-
-        updated_bug = self.client.get_bug(bug.id)
-        # When bugs come in as webhook payloads, they have a "comment"
-        # attribute, but this field isn't available when we get a bug by ID.
-        # So, we make sure to add the comment back if it was present on the bug.
-        updated_bug.comment = bug.comment
-        return updated_bug
-
-    def list_webhooks(self):
-        """List the currently configured webhooks, including their status."""
-
-        return self.client.list_webhooks()
-
-
-@lru_cache(maxsize=1)
-def get_service():
-    """Get bugzilla service"""
-    client = BugzillaClient(
-        settings.bugzilla_base_url, api_key=str(settings.bugzilla_api_key)
-    )
-    return BugzillaService(client=client)

jbi/bugzilla/service.py

@@ -0,0 +1,115 @@
+import logging
+from functools import lru_cache
+
+import requests
+from statsd.defaults.env import statsd
+
+from jbi import Operation, environment
+from jbi.common.instrument import ServiceHealth
+from jbi.models import (
+    ActionContext,
+    BugzillaBug,
+)
+
+from .client import BugzillaClient, BugzillaClientError
+
+settings = environment.get_settings()
+
+logger = logging.getLogger(__name__)
+
+
+class BugzillaService:
+    """Used by action workflows to perform action-specific Bugzilla tasks"""
+
+    def __init__(self, client: BugzillaClient) -> None:
+        self.client = client
+
+    def check_health(self) -> ServiceHealth:
+        """Check health for Bugzilla Service"""
+        logged_in = self.client.logged_in()
+        all_webhooks_enabled = False
+        if logged_in:
+            all_webhooks_enabled = self._all_webhooks_enabled()
+
+        health: ServiceHealth = {
+            "up": logged_in,
+            "all_webhooks_enabled": all_webhooks_enabled,
+        }
+        return health
+
+    def _all_webhooks_enabled(self):
+        # Check that all JBI webhooks are enabled in Bugzilla,
+        # and report disabled ones.
+
+        try:
+            jbi_webhooks = self.client.list_webhooks()
+        except (BugzillaClientError, requests.HTTPError):
+            return False
+
+        if len(jbi_webhooks) == 0:
+            logger.info("No webhooks enabled")
+            return True
+
+        for webhook in jbi_webhooks:
+            # Report errors in each webhook
+            statsd.gauge(f"jbi.bugzilla.webhooks.{webhook.slug}.errors", webhook.errors)
+            # Warn developers when there are errors
+            if webhook.errors > 0:
+                logger.warning(
+                    "Webhook %s has %s error(s)", webhook.name, webhook.errors
+                )
+            if not webhook.enabled:
+                logger.error(
+                    "Webhook %s is disabled (%s errors)",
+                    webhook.name,
+                    webhook.errors,
+                )
+                return False
+        return True
+
+    def add_link_to_jira(self, context: ActionContext):
+        """Add link to Jira in Bugzilla ticket"""
+        bug = context.bug
+        issue_key = context.jira.issue
+        jira_url = f"{settings.jira_base_url}browse/{issue_key}"
+        logger.debug(
+            "Link %r on Bug %s",
+            jira_url,
+            bug.id,
+            extra=context.update(operation=Operation.LINK).model_dump(),
+        )
+        return self.client.update_bug(bug.id, see_also={"add": [jira_url]})
+
+    def get_description(self, bug_id: int):
+        """Fetch a bug's description
+
+        A Bug's description does not appear in the payload of a bug. Instead, it is "comment 0"
+        """
+
+        comment_list = self.client.get_comments(bug_id)
+        comment_body = comment_list[0].text if comment_list else ""
+        return str(comment_body)
+
+    def refresh_bug_data(self, bug: BugzillaBug):
+        """Re-fetch a bug to ensure we have the most up-to-date data"""
+
+        updated_bug = self.client.get_bug(bug.id)
+        # When bugs come in as webhook payloads, they have a "comment"
+        # attribute, but this field isn't available when we get a bug by ID.
+        # So, we make sure to add the comment back if it was present on the bug.
+        updated_bug.comment = bug.comment
+        return updated_bug
+
+    def list_webhooks(self):
+        """List the currently configured webhooks, including their status."""
+
+        return self.client.list_webhooks()
+
+
+@lru_cache(maxsize=1)
+def get_service():
+    """Get bugzilla service"""
+    client = BugzillaClient(
+        settings.bugzilla_base_url, api_key=str(settings.bugzilla_api_key)
+    )
+    return BugzillaService(client=client)

tests/unit/services/__init__.py renamed to jbi/common/__init__.py

@@ -27,13 +27,13 @@ def instrument(prefix: str, exceptions: Sequence[Type[Exception]], **backoff_par
     """
 
     def decorator(func):
-        @wraps(func)
         @backoff.on_exception(
             backoff.expo,
             exceptions,
             max_tries=settings.max_retries + 1,
             **backoff_params,
         )
+        @wraps(func)
         def wrapper(*args, **kwargs):
             # Increment the call counter.
             statsd.incr(f"jbi.{prefix}.methods.{func.__name__}.count")

jbi/services/common.py renamed to jbi/common/instrument.py

@@ -0,0 +1,2 @@
+from .service import JiraService as JiraService
+from .service import get_service as get_service

jbi/jira/__init__.py

@@ -0,0 +1,125 @@
+import logging
+from typing import Collection, Iterable, Optional
+
+import requests
+from atlassian import Jira
+from atlassian import errors as atlassian_errors
+from atlassian.rest_client import log as atlassian_logger
+from requests import exceptions as requests_exceptions
+
+from jbi import environment
+from jbi.common.instrument import instrument
+
+settings = environment.get_settings()
+
+logger = logging.getLogger(__name__)
+
+
+def fatal_code(exc):
+    """Do not retry 4XX errors, mark them as fatal."""
+    try:
+        return 400 <= exc.response.status_code < 500
+    except AttributeError:
+        # `ApiError` or `ConnectionError` won't have response attribute.
+        return False
+
+
+instrumented_method = instrument(
+    prefix="jira",
+    exceptions=(
+        atlassian_errors.ApiError,
+        requests_exceptions.RequestException,
+    ),
+    giveup=fatal_code,
+)
+
+
+class JiraCreateError(Exception):
+    """Error raised on Jira issue creation."""
+
+
+class JiraClient(Jira):
+    """Adapted Atlassian Jira client that logs errors and wraps methods
+    in our instrumentation decorator.
+    """
+
+    def raise_for_status(self, *args, **kwargs):
+        """Catch and log HTTP errors responses of the Jira self.client.
+
+        Without this the actual requests and responses are not exposed when an error
+        occurs, which makes troubleshooting tedious.
+        """
+        try:
+            return super().raise_for_status(*args, **kwargs)
+        except requests.HTTPError as exc:
+            request = exc.request
+            response = exc.response
+            atlassian_logger.error(
+                "HTTP: %s %s -> %s %s",
+                request.method,
+                request.path_url,
+                response.status_code,
+                response.reason,
+                extra={"body": response.text},
+            )
+            raise
+
+    get_server_info = instrumented_method(Jira.get_server_info)
+    get_project_components = instrumented_method(Jira.get_project_components)
+    update_issue = instrumented_method(Jira.update_issue)
+    update_issue_field = instrumented_method(Jira.update_issue_field)
+    set_issue_status = instrumented_method(Jira.set_issue_status)
+    issue_add_comment = instrumented_method(Jira.issue_add_comment)
+    create_issue = instrumented_method(Jira.create_issue)
+    get_project = instrumented_method(Jira.get_project)
+
+    @instrumented_method
+    def paginated_projects(
+        self,
+        included_archived=None,
+        expand=None,
+        url=None,
+        keys: Optional[Collection[str]] = None,
+    ):
+        """Returns a paginated list of projects visible to the user.
+
+        https://developer.atlassian.com/cloud/jira/platform/rest/v2/api-group-projects/#api-rest-api-2-project-search-get
+
+        We've patched this method of the Jira client to accept the `keys` param.
+        """
+
+        if not self.cloud:
+            raise ValueError(
+                "``projects_from_cloud`` method is only available for Jira Cloud platform"
+            )
+
+        params = []
+
+        if keys is not None:
+            if len(keys) > 50:
+                raise ValueError("Up to 50 project keys can be provided.")
+            params = [("keys", key) for key in keys]
+
+        if included_archived:
+            params.append(("includeArchived", included_archived))
+        if expand:
+            params.append(("expand", expand))
+        page_url = url or self.resource_url("project/search")
+        is_url_absolute = bool(page_url.lower().startswith("http"))
+        return self.get(page_url, params=params, absolute=is_url_absolute)
+
+    @instrumented_method
+    def permitted_projects(self, permissions: Optional[Iterable] = None) -> list[dict]:
+        """Fetches projects that the user has the required permissions for
+
+        https://developer.atlassian.com/cloud/jira/platform/rest/v2/api-group-permissions/#api-rest-api-2-permissions-project-post
+        """
+        if permissions is None:
+            permissions = []
+
+        response = self.post(
+            "/rest/api/2/permissions/project",
+            json={"permissions": list(permissions)},
+        )
+        projects: list[dict] = response["projects"]
+        return projects