From bb2bf5d85e7274ff4bed598b751cd13289b5d9c4 Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Tue, 7 Apr 2026 13:48:23 -0400 Subject: [PATCH 1/5] fix(integrations): Handle null items in organization integrations serialization The OrganizationIntegrationSerializer could produce null entries in API responses when per-item serialization failed. The _serialize catch-all in base.py swallowed exceptions and returned None, which was included directly in the response list, crashing the frontend. - Use .get() in get_attrs to handle missing Integration records gracefully instead of KeyError-ing the entire request - Add null guards for missing integration and failed inner serialization - Catch broad exceptions in get_installation and installation methods with specific log messages instead of relying on the generic _serialize catch-all - Filter null items from both organization-integrations endpoints Co-Authored-By: Claude Opus 4.6 --- .../user_organizationintegrations.py | 2 +- .../organization_integrations_index.py | 6 +++- .../api/serializers/models/integration.py | 21 +++++++++++-- .../test_user_organizationintegration.py | 30 +++++++++++++++++++ 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/sentry/api/endpoints/user_organizationintegrations.py b/src/sentry/api/endpoints/user_organizationintegrations.py index ced6ea411bd85d..5fc86db6471bd6 100644 --- a/src/sentry/api/endpoints/user_organizationintegrations.py +++ b/src/sentry/api/endpoints/user_organizationintegrations.py @@ -53,6 +53,6 @@ def get(self, request: Request, user) -> Response: return self.paginate( request=request, queryset=queryset, - on_results=lambda x: serialize(x, request.user), + on_results=lambda x: [item for item in serialize(x, request.user) if item is not None], paginator_cls=OffsetPaginator, ) diff --git a/src/sentry/integrations/api/endpoints/organization_integrations_index.py b/src/sentry/integrations/api/endpoints/organization_integrations_index.py index cd17e705d0bf51..5760278cd901f2 100644 --- a/src/sentry/integrations/api/endpoints/organization_integrations_index.py +++ b/src/sentry/integrations/api/endpoints/organization_integrations_index.py @@ -125,7 +125,11 @@ def get( def on_results(results: Sequence[OrganizationIntegration]) -> Sequence[Mapping[str, Any]]: if feature_filters: results = filter_by_features(results, feature_filters) - return serialize(results, request.user, include_config=include_config) + return [ + item + for item in serialize(results, request.user, include_config=include_config) + if item is not None + ] return self.paginate( queryset=queryset, diff --git a/src/sentry/integrations/api/serializers/models/integration.py b/src/sentry/integrations/api/serializers/models/integration.py index 1335e646d012b1..8f1ec7349c21b5 100644 --- a/src/sentry/integrations/api/serializers/models/integration.py +++ b/src/sentry/integrations/api/serializers/models/integration.py @@ -135,7 +135,7 @@ def get_attrs( ) integrations_by_id: dict[int, RpcIntegration] = {i.id: i for i in integrations} return { - item: {"integration": integrations_by_id[item.integration_id]} for item in item_list + item: {"integration": integrations_by_id.get(item.integration_id)} for item in item_list } def serialize( @@ -145,27 +145,40 @@ def serialize( user: User | RpcUser | AnonymousUser, include_config: bool = True, **kwargs: Any, - ) -> MutableMapping[str, Any]: + ) -> MutableMapping[str, Any] | None: + logging_name = "sentry.serializers.model.organizationintegration" + logging_ctx = {"organization_integration_id": obj.id, "integration_id": obj.integration_id} # XXX(epurkhiser): This is O(n) for integrations, especially since # we're using the IntegrationConfigSerializer which pulls in the # integration installation config object which very well may be making # API request for config options. - integration: RpcIntegration = attrs.get("integration") # type: ignore[assignment] + integration: RpcIntegration | None = attrs.get("integration") + if integration is None: + logger.warning(f"{logging_name}.missing_integration", extra=logging_ctx) + return None + serialized_integration: MutableMapping[str, Any] = serialize( objects=integration, user=user, serializer=IntegrationConfigSerializer(obj.organization_id, params=self.params), include_config=include_config, ) + if serialized_integration is None: + logger.warning(f"{logging_name}.serialize_integration_failed", extra=logging_ctx) + return None dynamic_display_information = None config_data = None + logging_ctx["provider"] = integration.provider try: installation = integration.get_installation(organization_id=obj.organization_id) except NotImplementedError: # slack doesn't have an installation implementation config_data = obj.config if include_config else None + except Exception: + logger.exception(f"{logging_name}.get_installation_failed", extra=logging_ctx) + config_data = obj.config if include_config else None else: try: installation.org_integration = obj @@ -183,6 +196,8 @@ def serialize( "integration_provider": integration.provider, } logger.info(name, extra=log_info) + except Exception: + logger.exception(f"{logging_name}.serialize_installation_failed", extra=logging_ctx) serialized_integration.update( { diff --git a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py index ff3463aa297604..b69c7c509e96c9 100644 --- a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py +++ b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py @@ -2,6 +2,7 @@ import orjson +from sentry.integrations.models.integration import Integration from sentry.testutils.cases import APITestCase from sentry.testutils.silo import control_silo_test @@ -72,3 +73,32 @@ def test_billing_users_dont_see_integrations(self) -> None: assert response.status_code == 200 content = orjson.loads(response.content) assert content == [] + + def test_missing_integration_filtered_from_response(self) -> None: + """OrganizationIntegrations whose Integration was deleted don't produce null entries.""" + integration = self.create_provider_integration(provider="github") + self.create_organization_integration( + organization_id=self.organization.id, integration_id=integration.id + ) + + # Delete the Integration row directly to simulate orphaned state + Integration.objects.filter(id=integration.id).delete() + + response = self.get_success_response(self.user.id) + assert response.data == [] + + def test_installation_error_still_returns_item(self) -> None: + """An exception in get_dynamic_display_information shouldn't null out the item.""" + integration = self.create_provider_integration(provider="github") + self.create_organization_integration( + organization_id=self.organization.id, integration_id=integration.id + ) + + with patch( + "sentry.integrations.base.IntegrationInstallation.get_dynamic_display_information", + side_effect=RuntimeError("boom"), + ): + response = self.get_success_response(self.user.id) + + assert len(response.data) == 1 + assert response.data[0]["organizationId"] == self.organization.id From 0cfca8103b0d3bb16dced7260d8edc674277af4d Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Tue, 7 Apr 2026 14:13:41 -0400 Subject: [PATCH 2/5] ref(integrations): Simplify null handling in org integration serialization Revert custom exception handling in OrganizationIntegrationSerializer and instead rely on the existing _serialize catch-all in base.py to log failures. Add exception detail to the base serializer log for better debugging. Update tests to match simplified approach. Co-Authored-By: Claude Opus 4.6 --- src/sentry/api/serializers/base.py | 4 ++-- .../api/serializers/models/integration.py | 21 +++---------------- .../test_user_organizationintegration.py | 7 +++---- 3 files changed, 8 insertions(+), 24 deletions(-) diff --git a/src/sentry/api/serializers/base.py b/src/sentry/api/serializers/base.py index 6242e26c073868..8a2e81e323c0b5 100644 --- a/src/sentry/api/serializers/base.py +++ b/src/sentry/api/serializers/base.py @@ -117,8 +117,8 @@ def _serialize( ) -> Mapping[str, Any] | None: try: return self.serialize(obj, attrs, user, **kwargs) - except Exception: - logger.exception("Failed to serialize", extra={"instance": obj}) + except Exception as e: + logger.exception("Failed to serialize", extra={"instance": obj, "exception": e}) return None def serialize( diff --git a/src/sentry/integrations/api/serializers/models/integration.py b/src/sentry/integrations/api/serializers/models/integration.py index 8f1ec7349c21b5..1335e646d012b1 100644 --- a/src/sentry/integrations/api/serializers/models/integration.py +++ b/src/sentry/integrations/api/serializers/models/integration.py @@ -135,7 +135,7 @@ def get_attrs( ) integrations_by_id: dict[int, RpcIntegration] = {i.id: i for i in integrations} return { - item: {"integration": integrations_by_id.get(item.integration_id)} for item in item_list + item: {"integration": integrations_by_id[item.integration_id]} for item in item_list } def serialize( @@ -145,40 +145,27 @@ def serialize( user: User | RpcUser | AnonymousUser, include_config: bool = True, **kwargs: Any, - ) -> MutableMapping[str, Any] | None: - logging_name = "sentry.serializers.model.organizationintegration" - logging_ctx = {"organization_integration_id": obj.id, "integration_id": obj.integration_id} + ) -> MutableMapping[str, Any]: # XXX(epurkhiser): This is O(n) for integrations, especially since # we're using the IntegrationConfigSerializer which pulls in the # integration installation config object which very well may be making # API request for config options. - integration: RpcIntegration | None = attrs.get("integration") - if integration is None: - logger.warning(f"{logging_name}.missing_integration", extra=logging_ctx) - return None - + integration: RpcIntegration = attrs.get("integration") # type: ignore[assignment] serialized_integration: MutableMapping[str, Any] = serialize( objects=integration, user=user, serializer=IntegrationConfigSerializer(obj.organization_id, params=self.params), include_config=include_config, ) - if serialized_integration is None: - logger.warning(f"{logging_name}.serialize_integration_failed", extra=logging_ctx) - return None dynamic_display_information = None config_data = None - logging_ctx["provider"] = integration.provider try: installation = integration.get_installation(organization_id=obj.organization_id) except NotImplementedError: # slack doesn't have an installation implementation config_data = obj.config if include_config else None - except Exception: - logger.exception(f"{logging_name}.get_installation_failed", extra=logging_ctx) - config_data = obj.config if include_config else None else: try: installation.org_integration = obj @@ -196,8 +183,6 @@ def serialize( "integration_provider": integration.provider, } logger.info(name, extra=log_info) - except Exception: - logger.exception(f"{logging_name}.serialize_installation_failed", extra=logging_ctx) serialized_integration.update( { diff --git a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py index b69c7c509e96c9..506b29796fcf82 100644 --- a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py +++ b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py @@ -87,8 +87,8 @@ def test_missing_integration_filtered_from_response(self) -> None: response = self.get_success_response(self.user.id) assert response.data == [] - def test_installation_error_still_returns_item(self) -> None: - """An exception in get_dynamic_display_information shouldn't null out the item.""" + def test_serialization_error_filtered_from_response(self) -> None: + """An exception during serialization should produce an empty list, not null entries.""" integration = self.create_provider_integration(provider="github") self.create_organization_integration( organization_id=self.organization.id, integration_id=integration.id @@ -100,5 +100,4 @@ def test_installation_error_still_returns_item(self) -> None: ): response = self.get_success_response(self.user.id) - assert len(response.data) == 1 - assert response.data[0]["organizationId"] == self.organization.id + assert response.data == [] From dc7fb9bb2703df6109792517711d4f217094f60f Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Tue, 7 Apr 2026 15:33:35 -0400 Subject: [PATCH 3/5] fix(serializers): Remove sensitive data from serializer exception logging Remove the exception object from logger extras to avoid logging sensitive data (secrets, passwords) in cleartext, as flagged by CodeQL. The exception traceback is still captured by logger.exception and sentry_sdk.capture_exception. Co-Authored-By: Claude Opus 4.6 --- src/sentry/api/serializers/base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/serializers/base.py b/src/sentry/api/serializers/base.py index 8a2e81e323c0b5..e8ee4ec17999e6 100644 --- a/src/sentry/api/serializers/base.py +++ b/src/sentry/api/serializers/base.py @@ -118,7 +118,8 @@ def _serialize( try: return self.serialize(obj, attrs, user, **kwargs) except Exception as e: - logger.exception("Failed to serialize", extra={"instance": obj, "exception": e}) + logger.exception("Failed to serialize", extra={"instance": obj}) + sentry_sdk.capture_exception(e) return None def serialize( From 2a2852a655f88529800adb51c47410fe2ea9d930 Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Wed, 8 Apr 2026 15:04:18 -0400 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=94=A5=20revert=20serializer=20captur?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/sentry/api/serializers/base.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sentry/api/serializers/base.py b/src/sentry/api/serializers/base.py index e8ee4ec17999e6..6242e26c073868 100644 --- a/src/sentry/api/serializers/base.py +++ b/src/sentry/api/serializers/base.py @@ -117,9 +117,8 @@ def _serialize( ) -> Mapping[str, Any] | None: try: return self.serialize(obj, attrs, user, **kwargs) - except Exception as e: + except Exception: logger.exception("Failed to serialize", extra={"instance": obj}) - sentry_sdk.capture_exception(e) return None def serialize( From a18569e9ae8f3370d3020ff96ff9867c44c1cce5 Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Wed, 8 Apr 2026 15:09:31 -0400 Subject: [PATCH 5/5] test(integrations): Remove vacuous missing-integration test The test_missing_integration_filtered_from_response test never exercised the None-filtering logic it claimed to test. The Integration foreign key uses CASCADE on_delete, so deleting the Integration also cascade-deleted the OrganizationIntegration row, leaving zero results for the queryset. Even bypassing CASCADE, the queryset's integration__status JOIN would exclude orphaned rows. The test passed vacuously regardless of whether the fix existed. The remaining test_serialization_error_filtered_from_response properly exercises the None-filtering by triggering a real exception in _serialize. Co-Authored-By: Claude Opus 4.6 --- .../endpoints/test_user_organizationintegration.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py index 506b29796fcf82..b05a7964eb11e5 100644 --- a/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py +++ b/tests/sentry/integrations/api/endpoints/test_user_organizationintegration.py @@ -2,7 +2,6 @@ import orjson -from sentry.integrations.models.integration import Integration from sentry.testutils.cases import APITestCase from sentry.testutils.silo import control_silo_test @@ -74,19 +73,6 @@ def test_billing_users_dont_see_integrations(self) -> None: content = orjson.loads(response.content) assert content == [] - def test_missing_integration_filtered_from_response(self) -> None: - """OrganizationIntegrations whose Integration was deleted don't produce null entries.""" - integration = self.create_provider_integration(provider="github") - self.create_organization_integration( - organization_id=self.organization.id, integration_id=integration.id - ) - - # Delete the Integration row directly to simulate orphaned state - Integration.objects.filter(id=integration.id).delete() - - response = self.get_success_response(self.user.id) - assert response.data == [] - def test_serialization_error_filtered_from_response(self) -> None: """An exception during serialization should produce an empty list, not null entries.""" integration = self.create_provider_integration(provider="github")