Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 9 additions & 5 deletions src/sentry/integrations/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,11 +240,15 @@ class is just a descriptor for how that object functions, and what behavior
the installer's identity to the organization integration
"""

is_cell_restricted: bool = False
"""
Returns True if each integration installation can only be connected on one cell of Sentry at a
time. It will raise an error if any organization from another cell attempts to install it.
"""
# TODO(cells): Remove once jira integration is updated and works for multi-cell.
# No integrations should be cell restricted.
@property
def is_cell_restricted(self) -> bool:
"""
Returns True if each integration installation can only be connected on one cell of Sentry at a
time. It will raise an error if any organization from another cell attempts to install it.
"""
return False
Comment on lines +245 to +251
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: Changing is_cell_restricted to a read-only @property will cause an AttributeError in tests that attempt to directly assign a value to it, leading to test failures.
Severity: HIGH

Suggested Fix

Update the tests that assign to self.provider.is_cell_restricted. Instead of direct assignment, use a mocking technique to set the property's return value for the test's duration. For example, use mock.patch.object with new_callable=mock.PropertyMock to temporarily override the property's behavior without triggering an AttributeError.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/integrations/base.py#L245-L251

Potential issue: The `is_cell_restricted` attribute in the `IntegrationProvider` class
was converted into a read-only `@property`. However, multiple tests in
`tests/sentry/integrations/test_pipeline.py` still attempt to directly assign a value to
this property, such as `self.provider.is_cell_restricted = True`. Since a read-only
property does not have a setter, these assignment attempts will raise an
`AttributeError` at runtime. This will cause any test that calls the
`_setup_cell_restriction()` helper method or directly sets the property to fail,
blocking the test suite from passing.

Did we get this right? 👍 / 👎 to inform future reviews.


features: frozenset[IntegrationFeatures] = frozenset()
"""can be any number of IntegrationFeatures"""
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/integrations/jira/endpoints/descriptor.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def get(self, request: Request) -> Response:
"content": {"type": "label", "label": {"value": "Linked Issues"}},
"target": {
"type": "web_panel",
"url": "/extensions/jira/issue/{issue.key}/",
"url": "/extensions/jira/issue-details/{issue.key}/",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know if Jira snapshots this into the Jira instances when the extension is added. We might need to keep the old URL around as well.

},
"name": {"value": "Sentry "},
"key": "sentry-issues-glance",
Expand Down
10 changes: 6 additions & 4 deletions src/sentry/integrations/jira/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -1200,10 +1200,12 @@ class JiraIntegrationProvider(IntegrationProvider):
metadata = metadata
integration_cls = JiraIntegration

# Jira is cell-restricted because the JiraSentryIssueDetailsView view does not currently
# contain organization-identifying information aside from the ExternalIssue. Multiple cells
# may contain a matching ExternalIssue and we could leak data across the organizations.
is_cell_restricted = True
@property
def is_cell_restricted(self) -> bool:
# TODO(cells): Remove this option and property once multi-cell rollout is complete.
from sentry import options

return not options.get("integrations.jira.multi-cell-enabled")

features = frozenset(
[
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/integrations/jira/views/sentry_issue_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ class JiraSentryIssueDetailsView(JiraSentryUIBaseView):
"""
Handles requests (from the Sentry integration in Jira) for HTML to display when you
click on "Sentry -> Linked Issues" in the RH sidebar of an issue in the Jira UI.

TODO(cells): Remove once all installs have migrated to JiraSentryIssueDetailsControlView.
"""

html_file = "sentry/integrations/jira-issue.html"
Expand Down
17 changes: 8 additions & 9 deletions src/sentry/middleware/integrations/parsers/jira.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,16 +67,15 @@ def get_response(self) -> HttpResponseBase:
if len(cells) == 0:
return self.get_default_missing_integration_response()

if len(cells) > 1:
# This shouldn't happen because we block multi cell install at the install time.
raise ValueError("Jira integration is installed in multiple cells")

if self.view_class in self.immediate_response_cell_classes:
Comment thread
cursor[bot] marked this conversation as resolved.
try:
return self.get_response_from_cell_silo(cell=cells[0])
except ApiError as err:
sentry_sdk.capture_exception(err)
return self.get_response_from_control_silo()
if len(cells) == 1:
try:
return self.get_response_from_cell_silo(cell=cells[0])
except ApiError as err:
sentry_sdk.capture_exception(err)
return self.get_response_from_control_silo()
# TODO(cells): Remove once all installs have migrated to JiraSentryIssueDetailsControlView.
return JiraSentryIssueDetailsControlView.as_view()(self.request, **self.match.kwargs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks reasonable to me. This endpoint was added in #98324 with the intention of replacing the region scoped endpoint, but the transition work wasn't completed.


if self.view_class in self.outbox_response_cell_classes:
return self.get_response_from_webhookpayload(
Expand Down
7 changes: 7 additions & 0 deletions src/sentry/options/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -4191,3 +4191,10 @@
type=Bool,
flags=FLAG_MODIFIABLE_BOOL | FLAG_AUTOMATOR_MODIFIABLE,
)

register(
"integrations.jira.multi-cell-enabled",
default=False,
type=Bool,
flags=FLAG_MODIFIABLE_BOOL | FLAG_AUTOMATOR_MODIFIABLE,
)
Original file line number Diff line number Diff line change
Expand Up @@ -258,26 +258,22 @@ def test_expired_invalid_installation_error(
assert response.status_code == 200
assert UNABLE_TO_VERIFY_INSTALLATION.encode() in response.content

@patch.object(ExternalIssue.objects, "get")
@patch("sentry.integrations.jira.views.sentry_issue_details.get_integration_from_request")
@responses.activate
def test_simple_get(
self,
mock_get_integration_from_request: MagicMock,
mock_get_external_issue: MagicMock,
) -> None:
responses.add(
responses.PUT, self.properties_url % (self.issue_key, self.properties_key), json={}
)

mock_get_external_issue.side_effect = [self.de_external_issue, self.us_external_issue]

mock_get_integration_from_request.return_value = self.integration
response = self.client.get(self.path)
assert response.status_code == 200
resp_content = response.content.decode()

for group in [self.us_group, self.de_group]:
assert group.title in resp_content
assert group.get_absolute_url() in resp_content

@patch("sentry.integrations.jira.views.sentry_issue_details.get_integration_from_request")
Expand Down
29 changes: 3 additions & 26 deletions tests/sentry/integrations/test_pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def _modify_provider(self):
yield

def _setup_cell_restriction(self):
self.provider.is_cell_restricted = True
setattr(self.provider, "is_cell_restricted", True)
na_orgs = [
self.create_organization(name="na_org"),
self.create_organization(name="na_org_2"),
Expand Down Expand Up @@ -141,27 +141,6 @@ def test_with_customer_domain(self, *args) -> None:
organization_id=self.organization.id, integration_id=integration.id
).exists()

@patch("sentry.signals.integration_added.send_robust")
def test_provider_should_check_cell_violation(self, *args) -> None:
"""Ensures we validate cells if `provider.is_cell_restricted` is set to True"""
self.provider.is_cell_restricted = True
self.pipeline.state.data = {"external_id": self.external_id}
with patch(
"sentry.integrations.pipeline.is_violating_cell_restriction"
) as mock_check_violation:
self.pipeline.finish_pipeline()
assert mock_check_violation.called

@patch("sentry.signals.integration_added.send_robust")
def test_provider_should_not_check_cell_violation(self, *args) -> None:
"""Ensures we don't reject cells if `provider.is_cell_restricted` is set to False"""
self.pipeline.state.data = {"external_id": self.external_id}
with patch(
"sentry.integrations.pipeline.is_violating_cell_restriction"
) as mock_check_violation:
self.pipeline.finish_pipeline()
assert not mock_check_violation.called

@patch("sentry.signals.integration_added.send_robust")
def test_is_violating_cell_restriction_success(self, *args) -> None:
"""Ensures pipeline can complete if all integration organizations reside in one cell."""
Expand Down Expand Up @@ -198,11 +177,9 @@ def test_is_violating_cell_restriction_failure(self, *args) -> None:
response = self.pipeline.finish_pipeline()
assert isinstance(response, HttpResponse)
error_message = "This integration has already been installed on another Sentry organization which resides in a different cell. Installation could not be completed."
assert error_message in response.content.decode()

if SiloMode.get_current_mode() == SiloMode.MONOLITH:
assert error_message not in response.content.decode()
if SiloMode.get_current_mode() == SiloMode.CONTROL:
else:
assert error_message in response.content.decode()

def test_aliased_integration_key(self, *args) -> None:
Expand Down Expand Up @@ -727,7 +704,7 @@ def _modify_provider(self):
yield

def _setup_cell_restriction(self):
self.provider.is_cell_restricted = True
setattr(self.provider, "is_cell_restricted", True)
na_orgs = [
self.create_organization(name="na_org"),
self.create_organization(name="na_org_2"),
Expand Down
23 changes: 8 additions & 15 deletions tests/sentry/middleware/integrations/parsers/test_jira.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

from unittest.mock import patch

import pytest
import responses
from django.http import HttpRequest, HttpResponse
from django.test import RequestFactory, override_settings
Expand Down Expand Up @@ -186,26 +185,20 @@ def test_get_response_invalid_path(self) -> None:

@override_cells(region_config)
@override_settings(SILO_MODE=SiloMode.CONTROL)
@responses.activate
def test_get_response_multiple_regions(self) -> None:
responses.add(
responses.POST,
eu_locality.to_url("/extensions/jira/issue/LR-123/"),
body="region response",
status=200,
)
request = self.factory.post(path=f"{self.path_base}/issue/LR-123/")
# Use GET — the view only handles GET, and Jira sends GET for issue hooks.
request = self.factory.get(path=f"{self.path_base}/issue/LR-123/")
parser = JiraRequestParser(request, self.get_response)

# Add a second organization. Jira only supports single regions.
other_org = self.create_organization(owner=self.user, region="eu")
integration = self.get_integration()
integration.add_organization(other_org.id)

with patch.object(parser, "get_integration_from_request") as method:
method.return_value = integration
# assert ValueError is raised if the integration is not valid
with pytest.raises(ValueError):
parser.get_response()
with patch.object(parser, "get_integration_from_request") as mock_integration:
mock_integration.return_value = integration
response = parser.get_response()

# Must not 404 — multi-cell falls back to JiraSentryIssueDetailsControlView, not
# get_response_from_control_silo() which would 404 via the @cell_silo_view restriction.
assert response.status_code == 200
assert_no_webhook_payloads()
Loading