From 2361fc541e237086596464439e24243a3c081c38 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 4 Sep 2026 11:22:08 +0200 Subject: [PATCH 1/2] ref(hybridcloud): Read webhook bucket keys through one helper --- .../integrations/middleware/hybrid_cloud/parser.py | 14 ++++++++++++++ .../middleware/integrations/parsers/github.py | 14 +++----------- .../middleware/integrations/parsers/gitlab.py | 11 ++--------- src/sentry/middleware/integrations/parsers/jira.py | 6 +----- .../middleware/integrations/parsers/jira_server.py | 14 ++------------ src/sentry/middleware/integrations/parsers/vsts.py | 6 +----- .../middleware/hybrid_cloud/test_base.py | 14 ++++++++++++++ .../middleware/integrations/parsers/test_github.py | 4 ++-- 8 files changed, 39 insertions(+), 44 deletions(-) diff --git a/src/sentry/integrations/middleware/hybrid_cloud/parser.py b/src/sentry/integrations/middleware/hybrid_cloud/parser.py index 86265a0383d7..730d9bd6cb30 100644 --- a/src/sentry/integrations/middleware/hybrid_cloud/parser.py +++ b/src/sentry/integrations/middleware/hybrid_cloud/parser.py @@ -2,6 +2,7 @@ import logging from abc import ABC +from collections.abc import Mapping from concurrent.futures import as_completed from typing import TYPE_CHECKING, Any, ClassVar @@ -37,6 +38,7 @@ from sentry.types.cell import Cell, find_cells_for_org_mappings, get_cell_by_name from sentry.utils import metrics from sentry.utils.concurrent import ContextPropagatingThreadPoolExecutor +from sentry.utils.safe import get_path logger = logging.getLogger(__name__) if TYPE_CHECKING: @@ -390,6 +392,18 @@ def mailbox_bucket_id(self, data: dict[str, Any]) -> int | None: "You must implement mailbox_bucket_id to use bucketed identifiers" ) + @staticmethod + def bucket_key_at(data: Mapping[str, Any], *path: str) -> int | None: + """Read a bucket key out of `data`, or None if it is missing or not numeric. + + Shared so every provider degrades the same way rather than raising out of the + parser on a body it did not expect. + """ + try: + return int(get_path(data, *path)) + except (TypeError, ValueError): + return None + def _mailbox_event_type(self, data: dict[str, Any]) -> str | None: """Validation lives here, not in the subclass: the discriminator comes out of a body control has not verified — gitlab and bitbucket resolve their handlers diff --git a/src/sentry/middleware/integrations/parsers/github.py b/src/sentry/middleware/integrations/parsers/github.py index 2c3ca9f053f7..85ff04aaba84 100644 --- a/src/sentry/middleware/integrations/parsers/github.py +++ b/src/sentry/middleware/integrations/parsers/github.py @@ -73,17 +73,9 @@ def _get_external_id(self, event: Mapping[str, Any]) -> str | None: return get_github_external_id(event) def mailbox_bucket_id(self, data: Mapping[str, Any]) -> int | None: - """Hash on repository ID to distribute webhooks across sub-mailboxes. - - GitHub webhook payloads include repository.id for most event types. - Installation events are routed to control silo and don't reach this path. - """ - repository = data.get("repository") - if isinstance(repository, dict): - repo_id = repository.get("id") - if isinstance(repo_id, int): - return repo_id - return None + """Every event type that reaches a cell carries `repository.id`; installation + events are handled on control and never get here.""" + return self.bucket_key_at(data, "repository", "id") def mailbox_event_type(self, data: Mapping[str, Any]) -> str | None: return self.request.META.get(GITHUB_WEBHOOK_TYPE_HEADER) diff --git a/src/sentry/middleware/integrations/parsers/gitlab.py b/src/sentry/middleware/integrations/parsers/gitlab.py index 1a01e5fa2276..e90ad1aedae1 100644 --- a/src/sentry/middleware/integrations/parsers/gitlab.py +++ b/src/sentry/middleware/integrations/parsers/gitlab.py @@ -84,15 +84,8 @@ def get_response_from_gitlab_webhook(self) -> HttpResponseBase: ) def mailbox_bucket_id(self, data: Mapping[str, Any]) -> int | None: - """ - Used by get_mailbox to find the project.id a payload is for. - In high volume gitlab instances we shard messages by project for greater - delivery throughput. - """ - project_id = data.get("project", {}).get("id", None) - if not project_id: - return None - return project_id + """Every event kind a cell processes names the project it belongs to.""" + return self.bucket_key_at(data, "project", "id") def mailbox_event_type(self, data: Mapping[str, Any]) -> str | None: """Reads the body's `object_kind`, not the `X-Gitlab-Event` header the diff --git a/src/sentry/middleware/integrations/parsers/jira.py b/src/sentry/middleware/integrations/parsers/jira.py index d1d09dd0c08c..8a7517573470 100644 --- a/src/sentry/middleware/integrations/parsers/jira.py +++ b/src/sentry/middleware/integrations/parsers/jira.py @@ -28,7 +28,6 @@ parse_integration_from_request, ) from sentry.shared_integrations.exceptions import ApiError -from sentry.utils.safe import get_path logger = logging.getLogger(__name__) @@ -97,7 +96,4 @@ def mailbox_bucket_id(self, data: Mapping[str, Any]) -> int | None: """The Connect descriptor registers only `jira:issue_updated`, so the issue is the only axis a Jira mailbox can be split on. """ - try: - return int(get_path(data, "issue", "id")) - except (TypeError, ValueError): - return None + return self.bucket_key_at(data, "issue", "id") diff --git a/src/sentry/middleware/integrations/parsers/jira_server.py b/src/sentry/middleware/integrations/parsers/jira_server.py index 30de8a3bc2b5..d17097dfc63c 100644 --- a/src/sentry/middleware/integrations/parsers/jira_server.py +++ b/src/sentry/middleware/integrations/parsers/jira_server.py @@ -57,18 +57,8 @@ def get_response_from_issue_update_webhook(self) -> HttpResponseBase: ) def mailbox_bucket_id(self, data: Mapping[str, Any]) -> int | None: - """ - Used by get_mailbox to find the issue.id a payload is for. - In high volume jira_server instances we shard messages by issue for greater - delivery throughput. - """ - issue_id = data.get("issue", {}).get("id", None) - if not issue_id: - return None - try: - return int(issue_id) - except ValueError: - return None + """Only changelog webhooks reach a cell, and each names its issue.""" + return self.bucket_key_at(data, "issue", "id") def get_response(self) -> HttpResponseBase: if self.view_class == JiraServerIssueUpdatedWebhook: diff --git a/src/sentry/middleware/integrations/parsers/vsts.py b/src/sentry/middleware/integrations/parsers/vsts.py index c1d5c5057805..78253440c61e 100644 --- a/src/sentry/middleware/integrations/parsers/vsts.py +++ b/src/sentry/middleware/integrations/parsers/vsts.py @@ -14,7 +14,6 @@ from sentry.integrations.types import IntegrationProviderSlug from sentry.integrations.vsts.webhooks import WorkItemWebhook, get_vsts_external_id from sentry.silo.base import control_silo_function -from sentry.utils.safe import get_path logger = logging.getLogger(__name__) @@ -65,7 +64,4 @@ def mailbox_bucket_id(self, data: Mapping[str, Any]) -> int | None: """The subscription is created for `workitem.updated` only, so the work item is the only axis a VSTS mailbox can be split on. """ - try: - return int(get_path(data, "resource", "workItemId")) - except (TypeError, ValueError): - return None + return self.bucket_key_at(data, "resource", "workItemId") diff --git a/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py b/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py index f55681c1bde9..57f74f00a673 100644 --- a/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py +++ b/tests/sentry/integrations/middleware/hybrid_cloud/test_base.py @@ -184,6 +184,20 @@ def mailbox_bucket_id(self, data: dict[str, Any]) -> int | None: ): assert str(parser.get_mailbox(integration, {})) == f"test_provider:{integration.id}:77" + def test_bucket_key_at_coerces_or_falls_back(self) -> None: + at = BaseRequestParser.bucket_key_at + + assert at({"issue": {"id": 10237}}, "issue", "id") == 10237 + assert at({"issue": {"id": "10237"}}, "issue", "id") == 10237 + + # Anything unusable falls back rather than raising at the modulo. + assert at({}, "issue", "id") is None + assert at({"issue": {}}, "issue", "id") is None + assert at({"issue": "PROJ-1"}, "issue", "id") is None + assert at({"issue": {"id": None}}, "issue", "id") is None + assert at({"issue": {"id": "not-a-number"}}, "issue", "id") is None + assert at({"issue": {"id": ["10237"]}}, "issue", "id") is None + def test_get_mailbox_always_bucket_skips_volume_check(self) -> None: class AlwaysBucketedParser(ExampleRequestParser): always_bucket = True diff --git a/tests/sentry/middleware/integrations/parsers/test_github.py b/tests/sentry/middleware/integrations/parsers/test_github.py index 3f2adb869669..0ac98bcca4a6 100644 --- a/tests/sentry/middleware/integrations/parsers/test_github.py +++ b/tests/sentry/middleware/integrations/parsers/test_github.py @@ -284,7 +284,7 @@ def test_issue_deleted_routing(self) -> None: "installation": {"id": "1"}, "issue": {"id": "1"}, "action": "deleted", - "repository": {"id": "1"}, + "repository": {"id": 1}, }, content_type="application/json", headers={"X-GITHUB-EVENT": GithubWebhookType.ISSUE.value}, @@ -298,7 +298,7 @@ def test_issue_deleted_routing(self) -> None: assert len(responses.calls) == 0 assert_webhook_payloads_for_mailbox( request=request, - mailbox_name=f"github:{integration.id}:issues", + mailbox_name=f"github:{integration.id}:1:issues", cell_names=[cell.name], destination_types={DestinationType.SENTRY_CELL: 1}, ) From 64a2239f3375f6540e09bcbcaf5d413517693a70 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Fri, 4 Sep 2026 12:01:49 +0200 Subject: [PATCH 2/2] Cover the gitlab reader's fallbacks The unit test came off the gate-removal PR, where its coercion and non-dict cases were asserting this helper's behaviour rather than that one's. --- .../integrations/parsers/test_gitlab.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/sentry/middleware/integrations/parsers/test_gitlab.py b/tests/sentry/middleware/integrations/parsers/test_gitlab.py index 1f81909aa234..c1a822015b69 100644 --- a/tests/sentry/middleware/integrations/parsers/test_gitlab.py +++ b/tests/sentry/middleware/integrations/parsers/test_gitlab.py @@ -58,6 +58,23 @@ def run_parser(self, request): parser = GitlabRequestParser(request=request, response_handler=self.get_response) return parser.get_response() + def test_mailbox_bucket_id(self) -> None: + request = self.factory.post( + self.path, + data=PUSH_EVENT, + content_type="application/json", + HTTP_X_GITLAB_TOKEN=WEBHOOK_TOKEN, + HTTP_X_GITLAB_EVENT="Push Hook", + ) + parser = GitlabRequestParser(request=request, response_handler=self.get_response) + + assert parser.mailbox_bucket_id({"project": {"id": 15}}) == 15 + assert parser.mailbox_bucket_id({"project": {"id": "15"}}) == 15 + assert parser.mailbox_bucket_id({}) is None + assert parser.mailbox_bucket_id({"project": {}}) is None + assert parser.mailbox_bucket_id({"project": "sentry"}) is None + assert parser.mailbox_bucket_id({"project": {"id": "sentry"}}) is None + @override_settings(SILO_MODE=SiloMode.CONTROL) @override_cells(cell_config) def test_missing_x_gitlab_token(self) -> None: