Work around a GitLab GraphQL bug (#12941)

Work around a GitLab GraphQL bug that made merge requests metrics fail with a connection error on GitLab 18.9 and later.

Fixes #12696 and #12725.

Author: fniessink

components/collector/src/source_collectors/gitlab/merge_requests.py

@@ -4,28 +4,16 @@
 from aiogqlc import GraphQLClient
 
 from base_collectors import MergeRequestCollector
-from collector_utilities.exceptions import NotFoundError
+from collector_utilities.exceptions import CollectorError, NotFoundError
 from collector_utilities.type import URL, Value
 from model import Entities, Entity, SourceResponses
 
 from .base import GitLabBase
 
-# GraphQL query to find out which fields merge requests have in a GitLab instance. GitLab instances on the free plan
-# don't have the approved field, GitLab instances on the premium plan and up do.
-MERGE_REQUEST_FIELDS_QUERY = """
-{
-  __type(name: "MergeRequest") {
-    fields {
-      name
-    }
-  }
-}
-"""
-
 # GraphQL query to retrieve the merge requests for a specific project. The project id is passed as a variable. This
 # string needs to be formatted with or without the approved field name, depending on whether the GitLab instance has
-# the approved field (queried with the query above), and with the pagination arguments, depending on whether the
-# query is used for the first page or for consecutive pages.
+# the approved field, and with the pagination arguments, depending on whether the query is used for the first page or
+# for consecutive pages.
 MERGE_REQUEST_QUERY = """
 query MRs($projectId: ID!) {{
   project(fullPath: $projectId) {{
@@ -66,6 +54,10 @@ def __init__(self, project: str) -> None:
         super().__init__("Merge request info for project", project, extra=tip)
 
 
+class _ApprovedFieldUnknownError(Exception):
+    """Raised when GitLab reports the MergeRequest.approved field as unknown."""
+
+
 class GitLabMergeRequests(MergeRequestCollector, GitLabBase):
     """Collector class to measure the number of merge requests."""
 
@@ -84,8 +76,9 @@ async def _get_source_responses(self, *urls: URL) -> SourceResponses:
         """
         api_url = await self._api_url()
         timeout = aiohttp.ClientTimeout(total=self._session.timeout.total)
-        # We need to create a new session because the GraphQLClient expects the session to provide the headers:
-        async with aiohttp.ClientSession(raise_for_status=True, timeout=timeout, headers=self._headers()) as session:
+        # We need to create a new session because the GraphQLClient expects the session to provide the headers.
+        # raise_for_status is False so we can inspect GraphQL error bodies (e.g. HTTP 400 on unknown field).
+        async with aiohttp.ClientSession(raise_for_status=False, timeout=timeout, headers=self._headers()) as session:
             client = GraphQLClient(f"{api_url}/api/graphql", session=session)
             approved_field = await self._approved_field(client)
             responses, has_next_page, cursor = SourceResponses(), True, ""
@@ -94,13 +87,19 @@ async def _get_source_responses(self, *urls: URL) -> SourceResponses:
                 responses.append(response)
         return responses
 
-    @classmethod
-    async def _approved_field(cls, client: GraphQLClient) -> str:
-        """Determine whether the GitLab instance has the approved field for merge requests."""
-        response = await client.execute(MERGE_REQUEST_FIELDS_QUERY)
-        json = await response.json()
-        fields = [field["name"] for field in json["data"]["__type"]["fields"]]
-        return cls.APPROVED_FIELD if cls.APPROVED_FIELD in fields else ""
+    async def _approved_field(self, client: GraphQLClient) -> str:
+        """Determine whether the GitLab instance has the approved field for merge requests.
+
+        This is done by running a dry-run first-page merge-requests query that includes the approved field. If
+        GitLab accepts it, the field is supported; if GitLab rejects it as unknown, it isn't. The first page is
+        then fetched again by the pagination loop, for a total call count that matches the previous introspection
+        approach.
+        """
+        try:
+            await self._get_merge_request_response(client, self.APPROVED_FIELD)
+        except _ApprovedFieldUnknownError:
+            return ""
+        return self.APPROVED_FIELD
 
     async def _get_merge_request_response(
         self,
@@ -113,6 +112,13 @@ async def _get_merge_request_response(
         merge_request_query = MERGE_REQUEST_QUERY.format(pagination=pagination, approved=approved_field)
         response = await client.execute(merge_request_query, variables={"projectId": self._parameter("project")})
         json = await response.json()
+        if errors := json.get("errors"):
+            message = errors[0].get("message") or "unknown GitLab error"
+            if self.APPROVED_FIELD in message and ("doesn't exist" in message or "Field '" in message):
+                raise _ApprovedFieldUnknownError(message)
+            response.raise_for_status()  # Surface HTTP errors (e.g. 400) if any.
+            raise CollectorError(message)  # Otherwise surface the GraphQL error message.
+        response.raise_for_status()
         if project := json["data"]["project"]:
             page_info = project["mergeRequests"]["pageInfo"]
             return response, page_info["hasNextPage"], page_info.get("endCursor", "")

components/collector/tests/source_collectors/gitlab/test_merge_requests.py

@@ -1,6 +1,6 @@
 """Unit tests for the GitLab merge requests collector."""
 
-from unittest.mock import AsyncMock, patch
+from unittest.mock import AsyncMock, MagicMock, patch
 
 import aiohttp
 
@@ -59,14 +59,6 @@ def merge_requests_json(
             },
         }
 
-    @staticmethod
-    def merge_request_fields_json(
-        has_approved_field: bool = False,
-    ) -> dict[str, dict[str, dict[str, list[dict[str, str]]]]]:
-        """Return the GraphQL merge request fields response JSON."""
-        fields = [{"name": "approved"}] if has_approved_field else []
-        return {"data": {"__type": {"fields": fields}}}
-
     @staticmethod
     def create_entity(
         number: int,
@@ -97,6 +89,15 @@ async def collect_merge_requests(self, execute_mock: AsyncMock):
                 collector = MetricCollector(session, self.metric)
                 return await collector.collect()
 
+    @staticmethod
+    def mock_response(json_value) -> AsyncMock:
+        """Create a mock GraphQL response that returns the given JSON from its json() method."""
+        response = AsyncMock()
+        response.json = AsyncMock(return_value=json_value)
+        # raise_for_status is sync on aiohttp.ClientResponse; override AsyncMock to avoid unawaited coroutines.
+        response.raise_for_status = MagicMock()
+        return response
+
     async def test_merge_requests(self):
         """Test that the number of merge requests can be measured."""
         self.set_source_parameter("merge_request_state", ["opened", "closed", "merged"])
@@ -110,11 +111,9 @@ async def test_merge_requests(self):
                 self.merge_request_json(4, branch="dev"),  # Excluded because of target branch
             ],
         )
-        merge_request_fields_response = AsyncMock()
-        merge_requests_response = AsyncMock()
-        execute = AsyncMock(side_effect=[merge_request_fields_response, merge_requests_response])
-        merge_request_fields_response.json = AsyncMock(return_value=self.merge_request_fields_json())
-        merge_requests_response.json = AsyncMock(return_value=merge_requests_json)
+        merge_requests_response = self.mock_response(merge_requests_json)
+        # The first call is the dry-run that detects the approved field, the second is the real first page.
+        execute = AsyncMock(side_effect=[merge_requests_response, merge_requests_response])
         entities = [self.create_entity(1)]
         response = await self.collect_merge_requests(execute)
         self.assert_measurement(response, value="1", total="4", entities=entities, landing_url=self.LANDING_URL)
@@ -123,13 +122,10 @@ async def test_pagination(self):
         """Test that pagination works."""
         merge_requests_json1 = self.merge_requests_json([self.merge_request_json(1)], count=2, has_next_page=True)
         merge_requests_json2 = self.merge_requests_json([self.merge_request_json(2)], count=2)
-        merge_request_fields_response = AsyncMock()
-        merge_requests_page1 = AsyncMock()
-        merge_requests_page2 = AsyncMock()
-        execute = AsyncMock(side_effect=[merge_request_fields_response, merge_requests_page1, merge_requests_page2])
-        merge_request_fields_response.json = AsyncMock(return_value=self.merge_request_fields_json())
-        merge_requests_page1.json = AsyncMock(return_value=merge_requests_json1)
-        merge_requests_page2.json = AsyncMock(return_value=merge_requests_json2)
+        merge_requests_page1 = self.mock_response(merge_requests_json1)
+        merge_requests_page2 = self.mock_response(merge_requests_json2)
+        # Dry-run, real page 1, page 2.
+        execute = AsyncMock(side_effect=[merge_requests_page1, merge_requests_page1, merge_requests_page2])
         entities = [self.create_entity(1), self.create_entity(2)]
         response = await self.collect_merge_requests(execute)
         self.assert_measurement(response, value="2", total="2", entities=entities, landing_url=self.LANDING_URL)
@@ -140,23 +136,17 @@ async def test_approval(self):
         merge_requests_json = self.merge_requests_json(
             [self.merge_request_json(1, approved=True), self.merge_request_json(2)],
         )
-        merge_request_fields_response = AsyncMock()
-        merge_requests_response = AsyncMock()
-        execute = AsyncMock(side_effect=[merge_request_fields_response, merge_requests_response])
-        merge_request_fields_response.json = AsyncMock(return_value=self.merge_request_fields_json(True))
-        merge_requests_response.json = AsyncMock(return_value=merge_requests_json)
+        merge_requests_response = self.mock_response(merge_requests_json)
+        execute = AsyncMock(side_effect=[merge_requests_response, merge_requests_response])
         entities = [self.create_entity(1, approved="yes")]
         response = await self.collect_merge_requests(execute)
         self.assert_measurement(response, value="1", total="2", entities=entities, landing_url=self.LANDING_URL)
 
     async def test_insufficient_permissions(self):
         """Test that the collector returns a helpful error message if no merge request info is returned."""
         merge_requests_json = {"data": {"project": None}}
-        merge_request_fields_response = AsyncMock()
-        merge_requests_response = AsyncMock()
-        execute = AsyncMock(side_effect=[merge_request_fields_response, merge_requests_response])
-        merge_request_fields_response.json = AsyncMock(return_value=self.merge_request_fields_json(True))
-        merge_requests_response.json = AsyncMock(return_value=merge_requests_json)
+        merge_requests_response = self.mock_response(merge_requests_json)
+        execute = AsyncMock(side_effect=[merge_requests_response, merge_requests_response])
         response = await self.collect_merge_requests(execute)
         self.assert_measurement(
             response,
@@ -170,11 +160,8 @@ async def test_ignore_drafts(self):
         merge_requests_json = self.merge_requests_json(
             [self.merge_request_json(1, draft=True), self.merge_request_json(2)],
         )
-        merge_request_fields_response = AsyncMock()
-        merge_requests_response = AsyncMock()
-        execute = AsyncMock(side_effect=[merge_request_fields_response, merge_requests_response])
-        merge_request_fields_response.json = AsyncMock(return_value=self.merge_request_fields_json(True))
-        merge_requests_response.json = AsyncMock(return_value=merge_requests_json)
+        merge_requests_response = self.mock_response(merge_requests_json)
+        execute = AsyncMock(side_effect=[merge_requests_response, merge_requests_response])
         entities = [self.create_entity(2)]
         response = await self.collect_merge_requests(execute)
         self.assert_measurement(response, value="1", total="2", entities=entities, landing_url=self.LANDING_URL)
@@ -185,11 +172,34 @@ async def test_ignore_drafts_without_drafts(self):
         merge_requests_json = self.merge_requests_json(
             [self.merge_request_json(1)],
         )
-        merge_request_fields_response = AsyncMock()
-        merge_requests_response = AsyncMock()
-        execute = AsyncMock(side_effect=[merge_request_fields_response, merge_requests_response])
-        merge_request_fields_response.json = AsyncMock(return_value=self.merge_request_fields_json(True))
-        merge_requests_response.json = AsyncMock(return_value=merge_requests_json)
+        merge_requests_response = self.mock_response(merge_requests_json)
+        execute = AsyncMock(side_effect=[merge_requests_response, merge_requests_response])
         entities = [self.create_entity(1)]
         response = await self.collect_merge_requests(execute)
         self.assert_measurement(response, value="1", total="1", entities=entities, landing_url=self.LANDING_URL)
+
+    async def test_approved_field_unknown_falls_back(self):
+        """Test that the collector falls back to a query without the approved field if GitLab rejects it."""
+        unknown_field_json = {
+            "errors": [{"message": "Field 'approved' doesn't exist on type 'MergeRequest'"}],
+        }
+        merge_requests_json = self.merge_requests_json([self.merge_request_json(1)])
+        unknown_field_response = self.mock_response(unknown_field_json)
+        merge_requests_response = self.mock_response(merge_requests_json)
+        # Dry-run returns the unknown-field error, then the real first page succeeds without the field.
+        execute = AsyncMock(side_effect=[unknown_field_response, merge_requests_response])
+        entities = [self.create_entity(1)]
+        response = await self.collect_merge_requests(execute)
+        self.assert_measurement(response, value="1", total="1", entities=entities, landing_url=self.LANDING_URL)
+
+    async def test_unrelated_graphql_error_surfaces(self):
+        """Test that unrelated GraphQL errors are surfaced as a connection error."""
+        error_json = {"errors": [{"message": "Something else went wrong"}]}
+        error_response = self.mock_response(error_json)
+        execute = AsyncMock(side_effect=[error_response, error_response])
+        response = await self.collect_merge_requests(execute)
+        self.assert_measurement(
+            response,
+            landing_url=self.LANDING_URL,
+            connection_error="Something else went wrong",
+        )

docs/src/changelog.md

@@ -14,6 +14,7 @@ If your currently installed *Quality-time* version is not the penultimate versio
 
 ### Fixed
 
+- Work around a GitLab GraphQL bug that made merge requests metrics fail with a connection error on GitLab 18.9 and later. Fixes [#12696](https://github.com/ICTU/quality-time/issues/12696) and [#12725](https://github.com/ICTU/quality-time/issues/12725).
 - Don't attempt to send notifications to notification destinations without a webhook. Fixes [#12906](https://github.com/ICTU/quality-time/issues/12906).
 
 ### Added