ref: fix typing for integrations.github.client (#97501)

<!-- Describe your PR here. -->

Author: asottile-sentry

pyproject.toml

@@ -113,7 +113,6 @@ module = [
     "sentry.api.paginator",
     "sentry.db.postgres.base",
     "sentry.eventstore.models",
-    "sentry.integrations.github.client",
     "sentry.integrations.pagerduty.actions.form",
     "sentry.integrations.slack.message_builder.notifications.issues",
     "sentry.integrations.slack.webhooks.event",

src/sentry/integrations/github/blame.py

@@ -4,7 +4,7 @@
 from collections.abc import Mapping, Sequence
 from dataclasses import asdict
 from datetime import datetime, timezone
-from typing import Any, TypedDict
+from typing import Any, TypedDict, TypeIs
 
 from django.utils.datastructures import OrderedSet
 
@@ -13,6 +13,7 @@
     FileBlameInfo,
     SourceLineInfo,
 )
+from sentry.shared_integrations.response.mapping import MappingApiResponse
 
 logger = logging.getLogger("sentry.integrations.github")
 
@@ -49,6 +50,11 @@ class GitHubGraphQlResponse(TypedDict):
     errors: list[dict[str, str]]
 
 
+def is_graphql_response(response: object) -> TypeIs[GitHubGraphQlResponse]:
+    # TODO: should probably narrow this better
+    return isinstance(response, MappingApiResponse) and ("data" in response or "errors" in response)
+
+
 FilePathMapping = dict[str, dict[str, OrderedSet]]
 GitHubRepositoryResponse = dict[str, GitHubRefResponse]
 

src/sentry/integrations/github/client.py

@@ -14,6 +14,7 @@
     create_blame_query,
     extract_commits_from_blame_response,
     generate_file_path_mapping,
+    is_graphql_response,
 )
 from sentry.integrations.github.utils import get_jwt, get_next_link
 from sentry.integrations.models.integration import Integration
@@ -30,7 +31,6 @@
 from sentry.models.repository import Repository
 from sentry.shared_integrations.client.proxy import IntegrationProxyClient
 from sentry.shared_integrations.exceptions import ApiError, ApiRateLimitedError
-from sentry.shared_integrations.response.mapping import MappingApiResponse
 from sentry.silo.base import control_silo_function
 from sentry.utils import metrics
 
@@ -210,7 +210,7 @@ def get_access_token(self) -> AccessTokenData | None:
         access_token: str | None = self.integration.metadata.get("access_token")
         expires_at: str | None = self.integration.metadata.get("expires_at")
         is_expired = (
-            bool(expires_at) and datetime.fromisoformat(expires_at).replace(tzinfo=None) < now
+            expires_at is not None and datetime.fromisoformat(expires_at).replace(tzinfo=None) < now
         )
         should_refresh = not access_token or not expires_at or is_expired
 
@@ -230,7 +230,7 @@ def authorize_request(self, prepared_request: PreparedRequest) -> PreparedReques
         integration: RpcIntegration | Integration | None = None
         if hasattr(self, "integration"):
             integration = self.integration
-        elif hasattr(self, "org_integration_id"):
+        elif self.org_integration_id is not None:
             integration = Integration.objects.filter(
                 organizationintegration__id=self.org_integration_id,
                 provider=EXTERNAL_PROVIDERS[ExternalProviders.GITHUB],
@@ -430,7 +430,7 @@ def get_repos(self) -> list[dict[str, Any]]:
         It uses page_size from the base class to specify how many items per page.
         The upper bound of requests is controlled with self.page_number_limit to prevent infinite requests.
         """
-        return self.get_with_pagination("/installation/repositories", response_key="repositories")
+        return self._get_with_pagination("/installation/repositories", response_key="repositories")
 
     def search_repositories(self, query: bytes) -> Mapping[str, Sequence[Any]]:
         """
@@ -445,9 +445,9 @@ def get_assignees(self, repo: str) -> Sequence[Any]:
         """
         https://docs.github.com/en/rest/issues/assignees#list-assignees
         """
-        return self.get_with_pagination(f"/repos/{repo}/assignees")
+        return self._get_with_pagination(f"/repos/{repo}/assignees")
 
-    def get_with_pagination(
+    def _get_with_pagination(
         self, path: str, response_key: str | None = None, page_number_limit: int | None = None
     ) -> list[Any]:
         """
@@ -549,7 +549,7 @@ def get_labels(self, owner: str, repo: str) -> list[Any]:
         Fetches all labels for a repository.
         https://docs.github.com/en/rest/issues/labels#list-labels-for-a-repository
         """
-        return self.get_with_pagination(f"/repos/{owner}/{repo}/labels")
+        return self._get_with_pagination(f"/repos/{owner}/{repo}/labels")
 
     def check_file(self, repo: Repository, path: str, version: str | None) -> object | None:
         return self.head_cached(path=f"/repos/{repo.name}/contents/{path}", params={"ref": version})
@@ -580,7 +580,7 @@ def get_file(
 
     def get_blame_for_files(
         self, files: Sequence[SourceLineInfo], extra: dict[str, Any]
-    ) -> Sequence[FileBlameInfo]:
+    ) -> list[FileBlameInfo]:
         log_info = {
             **extra,
             "provider": IntegrationProviderSlug.GITHUB,
@@ -633,7 +633,7 @@ def get_blame_for_files(
             else:
                 self.set_cache(cache_key, response, 60)
 
-        if not isinstance(response, MappingApiResponse):
+        if not is_graphql_response(response):
             raise ApiError("Response is not JSON")
 
         errors = response.get("errors", [])
@@ -662,6 +662,10 @@ def get_blame_for_files(
         )
 
 
+class _IntegrationIdParams(TypedDict, total=False):
+    integration_id: int
+
+
 class GitHubApiClient(GitHubBaseClient):
     def __init__(
         self,
@@ -671,7 +675,7 @@ def __init__(
         logging_context: Mapping[str, Any] | None = None,
     ) -> None:
         self.integration = integration
-        kwargs = {}
+        kwargs: _IntegrationIdParams = {}
         if hasattr(self.integration, "id"):
             kwargs["integration_id"] = integration.id
 

tests/sentry/integrations/github/test_client.py

@@ -196,7 +196,7 @@ def test_get_with_pagination(self, get_jwt) -> None:
             # The code only cares about the `next` value which is not included here
             headers={"link": f'<{url}&page=1>; rel="first", <{url}&page=3>; rel="prev"'},
         )
-        self.github_client.get_with_pagination(f"/repos/{self.repo.name}/assignees")
+        self.github_client._get_with_pagination(f"/repos/{self.repo.name}/assignees")
         assert len(responses.calls) == 4
         assert responses.calls[0].response.status_code == 200
 
@@ -207,7 +207,7 @@ def test_get_with_pagination_only_one_page(self, get_jwt) -> None:
 
         # No link in the headers because there are no more pages
         responses.add(method=responses.GET, url=url, json={}, headers={})
-        self.github_client.get_with_pagination(f"/repos/{self.repo.name}/assignees")
+        self.github_client._get_with_pagination(f"/repos/{self.repo.name}/assignees")
         assert len(responses.calls) == 1
         assert responses.calls[0].response.status_code == 200