Просмотр исходного кода

ref(commit-context): fix iterating over None. add type-checking for commit_context mixin (#44538)

Fix a bug where we're iterating over None. Also added some typing which
threw the error.

Fixes SENTRY-YEQ
Gilbert Szeto 2 лет назад
Родитель
Сommit
30493fe4de

+ 1 - 0
mypy.ini

@@ -79,6 +79,7 @@ files = fixtures/mypy-stubs,
         src/sentry/ingest/transaction_clusterer/,
         src/sentry/integrations/base.py,
         src/sentry/integrations/github/,
+        src/sentry/integrations/mixins/commit_context.py,
         src/sentry/integrations/slack/,
         src/sentry/integrations/message_builder.py,
         src/sentry/integrations/utils/atlassian_connect.py,

+ 10 - 2
src/sentry/integrations/github/integration.py

@@ -215,12 +215,17 @@ class GitHubIntegration(IntegrationInstallation, GitHubIssueBasic, RepositoryMix
         if not lineno:
             return None
         try:
-            blame_range = self.get_blame_for_file(repo, filepath, ref, lineno)
+            blame_range: Sequence[Mapping[str, Any]] | None = self.get_blame_for_file(
+                repo, filepath, ref, lineno
+            )
+
+            if blame_range is None:
+                return None
         except ApiError as e:
             raise e
 
         try:
-            commit = max(
+            commit: Mapping[str, Any] = max(
                 (
                     blame
                     for blame in blame_range
@@ -229,7 +234,10 @@ class GitHubIntegration(IntegrationInstallation, GitHubIssueBasic, RepositoryMix
                 key=lambda blame: datetime.strptime(
                     blame.get("commit", {}).get("committedDate"), "%Y-%m-%dT%H:%M:%SZ"
                 ),
+                default={},
             )
+            if not commit:
+                return None
         except (ValueError, IndexError):
             return None
 

+ 1 - 1
src/sentry/integrations/gitlab/client.py

@@ -329,4 +329,4 @@ class GitLabApiClient(ApiClient):
             request_path, params={"ref": ref, "range[start]": lineno, "range[end]": lineno}
         )
 
-        return contents
+        return contents or []

+ 16 - 6
src/sentry/integrations/mixins/commit_context.py

@@ -1,20 +1,32 @@
 from __future__ import annotations
 
-from typing import Mapping
+from typing import Any, Mapping, Protocol, Sequence
 
 from sentry.auth.exceptions import IdentityNotValid
 from sentry.models import Identity, Repository
 from sentry.shared_integrations.exceptions import ApiError
 
 
-class CommitContextMixin:
+class GetBlameForFile(Protocol):
+    def get_blame_for_file(
+        self, repo: Repository, filepath: str, ref: str, lineno: int
+    ) -> Sequence[Mapping[str, Any]]:
+        ...
+
+
+class GetClient(Protocol):
+    def get_client(self) -> GetBlameForFile:
+        ...
+
+
+class CommitContextMixin(GetClient):
     # whether or not integration has the ability to search through Repositories
     # dynamically given a search query
     repo_search = False
 
     def get_blame_for_file(
         self, repo: Repository, filepath: str, ref: str, lineno: int
-    ) -> str | None:
+    ) -> Sequence[Mapping[str, Any]] | None:
         """
         Calls the client's `get_blame_for_file` method to see if the file has a blame list.
 
@@ -29,8 +41,6 @@ class CommitContextMixin:
             return None
         try:
             response = client.get_blame_for_file(repo, filepath, ref, lineno)
-            if response is None:
-                return None
         except IdentityNotValid:
             return None
         except ApiError as e:
@@ -39,7 +49,7 @@ class CommitContextMixin:
         return response
 
     def get_commit_context(
-        self, repo: Repository, filepath: str, branch: str, event_frame
+        self, repo: Repository, filepath: str, branch: str, event_frame: Mapping[str, Any]
     ) -> Mapping[str, str] | None:
         """Formats the source code url used for stack trace linking."""
         raise NotImplementedError