Skip to content

ref(cells): Update assert_webhook_payloads_for_mailbox #111856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lynnagara merged 8 commits into from
Mar 31, 2026
Merged

ref(cells): Update assert_webhook_payloads_for_mailbox #111856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0404151
ref(cells): Update assert_webhook_payloads_for_mailbox
lynnagara Mar 30, 2026
f266c5d
update tests
lynnagara Mar 30, 2026
a2c5a41
update gh tests
lynnagara Mar 30, 2026
fc87ec4
.
lynnagara Mar 30, 2026
2cd8e32
update more integrations
lynnagara Mar 30, 2026
2e0c183
.
lynnagara Mar 30, 2026
ff5bc2f
.
lynnagara Mar 30, 2026
71d2ce8
.
lynnagara Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions src/sentry/testutils/outbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ def assert_no_webhook_payloads() -> None:
def assert_webhook_payloads_for_mailbox(
request: WSGIRequest,
mailbox_name: str,
region_names: list[str],
# TODO(cells): make required once getsentry passes cell everywhere
cell_names: list[str] | None = None,
# TODO(cells): remove once getsentry passes cell everywhere
region_names: list[str] | None = None,
destination_types: dict[DestinationType, int] | None = None,
) -> None:
"""
Expand All @@ -73,16 +76,16 @@ def assert_webhook_payloads_for_mailbox(

:param request:
:param mailbox_name: The mailbox name that messages should be found in.
:param region_names: Optional list of regions each messages should be queued for
:param cell_names: List of cells each messages should be queued for
:param destination_types: Optional Mapping of destination types to the number of messages that should be found for that destination type
"""
expected_payload = WebhookPayload.get_attributes_from_request(request=request)
region_names_set = set(region_names)
cell_names_set = set(cell_names or region_names or [])
Copy link
Copy Markdown
Contributor

@cursor cursor bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Falsy-check on cell_names ignores explicit empty list

Low Severity

The expression cell_names or region_names or [] uses Python's truthiness check, which treats an empty list [] the same as None. If a caller explicitly passes cell_names=[] (meaning "expect no cells") while getsentry simultaneously passes region_names=["us"] during the transition period, the empty cell_names would be silently ignored and region_names would be used instead. Using cell_names if cell_names is not None else (region_names if region_names is not None else []) would correctly distinguish "not provided" from "explicitly empty." Current in-repo callers aren't affected because they never pass both parameters, but this could bite getsentry callers during the synchronization window.

Fix in Cursor Fix in Web

messages = WebhookPayload.objects.filter(mailbox_name=mailbox_name)
messages_with_region_count = messages.filter(cell_name__isnull=False).count()
if messages_with_region_count != len(region_names_set):
messages_with_cell_count = messages.filter(cell_name__isnull=False).count()
if messages_with_cell_count != len(cell_names_set):
raise Exception(
f"Mismatch: Found {messages_with_region_count} WebhookPayload but {len(region_names_set)} region_names"
f"Mismatch: Found {messages_with_cell_count} WebhookPayload but {len(cell_names_set)} cell_names"
)
for message in messages:
assert message.request_method == expected_payload["request_method"]
Expand All @@ -104,13 +107,13 @@ def assert_webhook_payloads_for_mailbox(
else:
assert message.cell_name is not None
try:
region_names_set.remove(message.cell_name)
cell_names_set.remove(message.cell_name)
except KeyError:
raise Exception(
f"Found ControlOutbox for '{message.cell_name}', which was not in region_names: {str(region_names_set)}"
f"Found ControlOutbox for '{message.cell_name}', which was not in cell_names: {str(cell_names_set)}"
)
if len(region_names_set) != 0:
raise Exception(f"WebhookPayload not found for some region_names: {str(region_names_set)}")
if len(cell_names_set) != 0:
raise Exception(f"WebhookPayload not found for some cell_names: {str(cell_names_set)}")

if destination_types and len(destination_types) != 0:
exc_strs = [
Expand Down
30 changes: 15 additions & 15 deletions tests/sentry/middleware/integrations/parsers/test_bitbucket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
from sentry.testutils.silo import control_silo_test
from sentry.types.cell import Cell, RegionCategory

region = Cell("us", 1, "http://us.testserver", RegionCategory.MULTI_TENANT)
region_config = (region,)
cell = Cell("us", 1, "http://us.testserver", RegionCategory.MULTI_TENANT)
cell_config = (cell,)


@control_silo_test
Expand All @@ -22,16 +22,16 @@ def get_response(self, req: HttpRequest) -> HttpResponse:
return HttpResponse(status=200, content="passthrough")

factory = RequestFactory()
region = Cell("us", 1, "https://us.testserver", RegionCategory.MULTI_TENANT)
region_config = (region,)
cell = Cell("us", 1, "https://us.testserver", RegionCategory.MULTI_TENANT)
cell_config = (cell,)

def get_integration(self) -> Integration:
return self.create_integration(
organization=self.organization, external_id="bitbucket:1", provider="bitbucket"
)

@override_settings(SILO_MODE=SiloMode.CONTROL)
@override_cells(region_config)
@override_cells(cell_config)
def test_routing_endpoints(self) -> None:
self.get_integration()
control_routes = [
Expand All @@ -57,15 +57,15 @@ def test_routing_endpoints(self) -> None:
assert_no_webhook_payloads()

@override_settings(SILO_MODE=SiloMode.CONTROL)
@override_cells(region_config)
def test_routing_webhook_no_regions(self) -> None:
region_route = reverse(
@override_cells(cell_config)
def test_routing_webhook_no_cells(self) -> None:
cell_route = reverse(
"sentry-extensions-bitbucket-webhook", kwargs={"organization_id": self.organization.id}
)
request = self.factory.post(region_route)
request = self.factory.post(cell_route)
parser = BitbucketRequestParser(request=request, response_handler=self.get_response)

# Missing region
# Missing cell
OrganizationMapping.objects.get(organization_id=self.organization.id).update(cell_name="eu")
response = parser.get_response()

Expand All @@ -75,13 +75,13 @@ def test_routing_webhook_no_regions(self) -> None:
assert_no_webhook_payloads()

@override_settings(SILO_MODE=SiloMode.CONTROL)
@override_cells(region_config)
def test_routing_webhook_with_regions(self) -> None:
@override_cells(cell_config)
def test_routing_webhook_with_cells(self) -> None:
self.get_integration()
region_route = reverse(
cell_route = reverse(
"sentry-extensions-bitbucket-webhook", kwargs={"organization_id": self.organization.id}
)
request = self.factory.post(region_route)
request = self.factory.post(cell_route)
parser = BitbucketRequestParser(request=request, response_handler=self.get_response)

response = parser.get_response()
Expand All @@ -91,5 +91,5 @@ def test_routing_webhook_with_regions(self) -> None:
assert_webhook_payloads_for_mailbox(
request=request,
mailbox_name=f"bitbucket:{self.organization.id}",
region_names=[self.region.name],
cell_names=[self.cell.name],
)
16 changes: 8 additions & 8 deletions tests/sentry/middleware/integrations/parsers/test_bitbucket_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,29 @@
class BitbucketServerRequestParserTest(TestCase):
get_response = MagicMock(return_value=HttpResponse(content=b"no-error", status=200))
factory = RequestFactory()
region = Cell("us", 1, "https://us.testserver", RegionCategory.MULTI_TENANT)
region_config = (region,)
cell = Cell("us", 1, "https://us.testserver", RegionCategory.MULTI_TENANT)
cell_config = (cell,)

@override_cells(region_config)
@override_cells(cell_config)
@override_settings(SILO_MODE=SiloMode.CONTROL)
def test_routing_webhook(self) -> None:
region_route = reverse(
cell_route = reverse(
"sentry-extensions-bitbucketserver-webhook",
kwargs={"organization_id": self.organization.id, "integration_id": self.integration.id},
)
with outbox_runner():
request = self.factory.post(region_route)
request = self.factory.post(cell_route)
parser = BitbucketServerRequestParser(request=request, response_handler=self.get_response)

# Missing region
# Missing cell
OrganizationMapping.objects.get(organization_id=self.organization.id).update(cell_name="eu")
with mock.patch.object(
parser, "get_response_from_control_silo"
) as get_response_from_control_silo:
parser.get_response()
assert get_response_from_control_silo.called

# Valid region
# Valid cell
OrganizationMapping.objects.get(organization_id=self.organization.id).update(cell_name="us")
response = parser.get_response()
assert isinstance(response, HttpResponse)
Expand All @@ -51,5 +51,5 @@ def test_routing_webhook(self) -> None:
assert_webhook_payloads_for_mailbox(
request=request,
mailbox_name=f"bitbucket_server:{self.organization.id}",
region_names=[self.region.name],
cell_names=[self.cell.name],
)
Loading
Loading