Change methods for fetching visible projects and issue types

In v6.3.6, we bumped atlassian-python-api from 3.41.4. That bump
included a [PR](atlassian-api/atlassian-python-api#1270)
which changed the way library fetched projects. This somehow affected
our project when we fetched visible projects and issue types in the
heartbeat. We never fully determined the root cause, but the while loop
in the linked PR was suspect.

In this commit, we make further improvements to the code that's involved
in the heartbeat to make it more efficent.

Note that one of the tradeoffs we made for now was hardcoding the
maximum number of projects that may be configured at 50. This is due to
the way we fetch issue types by project. This is something that we can
fix in the future, but is not a problem that we need to solve for now
since we only have 26 project configured.

Author: grahamalama

jbi/models.py

@@ -138,13 +138,21 @@ def validate_actions(cls, actions: list[Action]):
         """
         Inspect the list of actions:
          - Validate that lookup tags are uniques
+         - Ensure we haven't exceeded our maximum configured project limit (see error below)
          - If the action's bugzilla_user_id is "tbd", emit a warning.
         """
         tags = [action.whiteboard_tag.lower() for action in actions]
         duplicated_tags = [t for i, t in enumerate(tags) if t in tags[:i]]
         if duplicated_tags:
             raise ValueError(f"actions have duplicated lookup tags: {duplicated_tags}")
 
+        if len(tags) > 50:
+            raise ValueError(
+                "The Jira client's `paginated_projects` method assumes we have "
+                "up to 50 projects configured. Adjust that implementation before "
+                "removing this validation check."
+            )
+
         for action in actions:
             if action.bugzilla_user_id == "tbd":
                 warnings.warn(

jbi/router.py

@@ -106,8 +106,7 @@ def get_bugzilla_webhooks(bugzilla_service: BugzillaServiceDep):
 @router.get("/jira_projects/")
 def get_jira_projects(jira_service: JiraServiceDep):
     """API for viewing projects that are currently accessible by API"""
-    visible_projects: list[dict] = jira_service.fetch_visible_projects()
-    return [project["key"] for project in visible_projects]
+    return jira_service.fetch_visible_projects()
 
 
 SRC_DIR = Path(__file__).parent

jbi/services/jira.py

@@ -11,7 +11,7 @@
 import json
 import logging
 from functools import lru_cache
-from typing import TYPE_CHECKING, Any, Iterable, Optional
+from typing import TYPE_CHECKING, Any, Collection, Iterable, Optional
 
 import requests
 from atlassian import Jira
@@ -94,7 +94,6 @@ def raise_for_status(self, *args, **kwargs):
 
     get_server_info = instrumented_method(Jira.get_server_info)
     get_project_components = instrumented_method(Jira.get_project_components)
-    projects = instrumented_method(Jira.projects)
     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)
@@ -103,17 +102,55 @@ def raise_for_status(self, *args, **kwargs):
     get_project = instrumented_method(Jira.get_project)
 
     @instrumented_method
-    def permitted_projects(self, permissions: Iterable):
+    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"] = list(keys)
+
+        if included_archived:
+            params["includeArchived"] = included_archived
+        if expand:
+            params["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(JIRA_REQUIRED_PERMISSIONS)},
         )
-        return response["projects"]
+        projects: list[dict] = response["projects"]
+        return projects
 
 
 class JiraService:
@@ -122,11 +159,11 @@ class JiraService:
     def __init__(self, client) -> None:
         self.client = client
 
-    def fetch_visible_projects(self) -> list[dict]:
+    def fetch_visible_projects(self) -> list[str]:
         """Return list of projects that are visible with the configured Jira credentials"""
 
-        projects: list[dict] = self.client.projects(included_archived=None)
-        return projects
+        projects = self.client.permitted_projects()
+        return [project["key"] for project in projects]
 
     def check_health(self, actions: Actions) -> ServiceHealth:
         """Check health for Jira Service"""
@@ -149,14 +186,12 @@ def check_health(self, actions: Actions) -> ServiceHealth:
 
     def _all_projects_visible(self, actions: Actions) -> bool:
         try:
-            visible_projects = {
-                project["key"] for project in self.fetch_visible_projects()
-            }
+            visible_projects = self.fetch_visible_projects()
         except requests.HTTPError:
             logger.exception("Error fetching visible Jira projects")
             return False
 
-        missing_projects = actions.configured_jira_projects_keys - visible_projects
+        missing_projects = actions.configured_jira_projects_keys - set(visible_projects)
         if missing_projects:
             logger.error(
                 "Jira projects %s are not visible with configured credentials",
@@ -232,7 +267,10 @@ def _check_project_components(self, action):
         return True
 
     def _all_project_issue_types_exist(self, actions: Actions):
-        projects = self.client.projects(included_archived=None, expand="issueTypes")
+        paginated_project_response = self.client.paginated_projects(
+            expand="issueTypes", keys=actions.configured_jira_projects_keys
+        )
+        projects = paginated_project_response["values"]
         issue_types_by_project = {
             project["key"]: {issue_type["name"] for issue_type in project["issueTypes"]}
             for project in projects

tests/unit/test_models.py

@@ -172,3 +172,9 @@ def test_payload_changes_list(webhook_event_change_factory, webhook_event_factor
         "status",
         "assignee",
     ]
+
+
+def test_max_configured_projects_raises_error(action_factory):
+    actions = [action_factory(whiteboard_tag=str(i)) for i in range(51)]
+    with pytest.raises(pydantic.ValidationError):
+        Actions(root=actions)

tests/unit/test_router.py

@@ -3,6 +3,7 @@
 from datetime import datetime
 from unittest import mock
 
+import pytest
 from fastapi.testclient import TestClient
 
 from jbi.app import app
@@ -26,7 +27,7 @@ def test_whiteboard_tags(anon_client):
 
 
 def test_jira_projects(anon_client, mocked_jira):
-    mocked_jira.projects.return_value = [{"key": "Firefox"}, {"key": "Fenix"}]
+    mocked_jira.permitted_projects.return_value = [{"key": "Firefox"}, {"key": "Fenix"}]
 
     resp = anon_client.get("/jira_projects/")
     infos = resp.json()
@@ -247,52 +248,6 @@ def test_read_heartbeat_bugzilla_services_fails(anon_client, mocked_bugzilla):
     }
 
 
-def test_read_heartbeat_success(
-    anon_client, mocked_jira, mocked_bugzilla, bugzilla_webhook_factory
-):
-    """/__heartbeat__ returns 200 when checks succeed."""
-    mocked_bugzilla.logged_in.return_value = True
-    mocked_bugzilla.list_webhooks.return_value = [bugzilla_webhook_factory()]
-    mocked_jira.get_server_info.return_value = {}
-    mocked_jira.projects.return_value = [
-        {
-            "key": "DevTest",
-            "issueTypes": [
-                {"name": "Task"},
-                {"name": "Bug"},
-            ],
-        }
-    ]
-    mocked_jira.get_project_components.return_value = [{"name": "Main"}]
-    mocked_jira.permitted_projects.return_value = [{"id": "12345", "key": "DevTest"}]
-
-    mocked_jira.get_project_components.return_value = [{"name": "Main"}]
-    mocked_jira.permitted_projects.return_value = [{"id": "12345", "key": "DevTest"}]
-    mocked_jira.get_project.return_value = {
-        "issueTypes": [
-            {"name": "Task"},
-            {"name": "Bug"},
-        ]
-    }
-
-    resp = anon_client.get("/__heartbeat__")
-
-    assert resp.status_code == 200
-    assert resp.json() == {
-        "jira": {
-            "up": True,
-            "all_projects_are_visible": True,
-            "all_projects_have_permissions": True,
-            "all_project_custom_components_exist": True,
-            "all_projects_issue_types_exist": True,
-        },
-        "bugzilla": {
-            "up": True,
-            "all_webhooks_enabled": True,
-        },
-    }
-
-
 def test_jira_heartbeat_visible_projects(anon_client, mocked_jira):
     """/__heartbeat__ fails if configured projects don't match."""
     mocked_jira.get_server_info.return_value = {}
@@ -349,35 +304,51 @@ def test_jira_heartbeat_unknown_issue_types(anon_client, mocked_jira):
     assert not resp.json()["jira"]["all_projects_issue_types_exist"]
 
 
-def test_head_heartbeat_success(
-    anon_client, mocked_jira, mocked_bugzilla, bugzilla_webhook_factory
+@pytest.mark.parametrize("method", ["HEAD", "GET"])
+def test_read_heartbeat_success(
+    anon_client, method, mocked_jira, mocked_bugzilla, bugzilla_webhook_factory
 ):
-    """/__heartbeat__ support head requests"""
+    """/__heartbeat__ returns 200 when checks succeed."""
     mocked_bugzilla.logged_in.return_value = True
     mocked_bugzilla.list_webhooks.return_value = [bugzilla_webhook_factory()]
     mocked_jira.get_server_info.return_value = {}
-    mocked_jira.projects.return_value = [
-        {
-            "key": "DevTest",
-            "issueTypes": [
-                {"name": "Task"},
-                {"name": "Bug"},
-            ],
-        }
-    ]
+    mocked_jira.paginated_projects.return_value = {
+        "values": [
+            {
+                "key": "DevTest",
+                "issueTypes": [
+                    {"name": "Task"},
+                    {"name": "Bug"},
+                ],
+            }
+        ]
+    }
     mocked_jira.get_project_components.return_value = [{"name": "Main"}]
-    mocked_jira.permitted_projects.return_value = [{"id": "12345", "key": "DevTest"}]
+    mocked_jira.permitted_projects.return_value = [{"key": "DevTest"}]
 
-    resp = anon_client.head("/__heartbeat__")
+    resp = anon_client.request(method, "__heartbeat__")
 
     assert resp.status_code == 200
+    if method == "GET":
+        assert resp.json() == {
+            "jira": {
+                "up": True,
+                "all_projects_are_visible": True,
+                "all_projects_have_permissions": True,
+                "all_project_custom_components_exist": True,
+                "all_projects_issue_types_exist": True,
+            },
+            "bugzilla": {
+                "up": True,
+                "all_webhooks_enabled": True,
+            },
+        }
 
 
-def test_lbheartbeat(anon_client):
+@pytest.mark.parametrize("method", ["HEAD", "GET"])
+def test_lbheartbeat(anon_client, method):
     """/__lbheartbeat__ always returns 200"""
 
-    resp = anon_client.get("/__lbheartbeat__")
+    resp = anon_client.request(method, "__lbheartbeat__")
     assert resp.status_code == 200
 
-    resp = anon_client.head("/__lbheartbeat__")
-    assert resp.status_code == 200