From 2a1882112c96e275f95f18685cdf7e5181b2c596 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 18 Oct 2024 12:37:51 -0700 Subject: [PATCH 1/7] better logging and errors for select requester --- .../endpoints/installation_external_requests.py | 4 ++-- .../external_requests/select_requester.py | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py index 0565b1b592aec9..cc139ca4e96312 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py @@ -36,7 +36,7 @@ def get(self, request: Request, installation) -> Response: try: choices = SelectRequester(**kwargs).run() - except Exception: - return Response({"error": "Error communicating with Sentry App service"}, status=400) + except Exception as e: + return Response({"error": str(e)}, status=400) return Response(choices) diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index 14f9c6ba6b8634..1fefce1df44623 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -36,9 +36,10 @@ class SelectRequester: def run(self) -> dict[str, Any]: response: list[dict[str, str]] = [] try: + url = self._build_url() body = safe_urlread( send_and_save_sentry_app_request( - self._build_url(), + url, self.sentry_app, self.install.organization_id, "select_options.requested", @@ -56,10 +57,23 @@ def run(self) -> dict[str, Any]: "project_slug": self.project_slug, "uri": self.uri, "error_message": str(e), + "url": url, }, ) + raise if not self._validate_response(response) or not response: + logger.info( + "select-requester.invalid-response", + extra={ + "response": response, + "sentry_app_slug": self.sentry_app.slug, + "install_uuid": self.install.uuid, + "project_slug": self.project_slug, + "uri": self.uri, + "url": url, + }, + ) raise APIError( f"Invalid response format for SelectField in {self.sentry_app} from uri: {self.uri}" ) From d28a1050bc11e0e23ab6ef52e7a896d455cc7a4f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 18 Oct 2024 14:32:29 -0700 Subject: [PATCH 2/7] maybe dont expose everything to the outside world --- .../api/endpoints/installation_external_requests.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py index cc139ca4e96312..11b6be75690843 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py @@ -36,7 +36,11 @@ def get(self, request: Request, installation) -> Response: try: choices = SelectRequester(**kwargs).run() - except Exception as e: - return Response({"error": str(e)}, status=400) + except Exception: + message = f'Error retrieving select field options from {request.GET.get("uri")}' + return Response( + {"error": message}, + status=400, + ) return Response(choices) From ce9d06ac84ff8684ab34fd06894742ce5f9ade72 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 18 Oct 2024 14:41:50 -0700 Subject: [PATCH 3/7] differentiate between validation and exception --- .../api/endpoints/installation_external_requests.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py index 11b6be75690843..58a0aa922c7248 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py @@ -4,6 +4,7 @@ from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import region_silo_endpoint +from sentry.api.client import ApiError from sentry.models.project import Project from sentry.sentry_apps.api.bases.sentryapps import SentryAppInstallationBaseEndpoint from sentry.sentry_apps.external_requests.select_requester import SelectRequester @@ -36,6 +37,12 @@ def get(self, request: Request, installation) -> Response: try: choices = SelectRequester(**kwargs).run() + except ApiError: + message = f"Error validating response options from {installation.sentry_app.slug} for {request.GET.get('uri')}" + return Response( + {"error": message}, + status=400, + ) except Exception: message = f'Error retrieving select field options from {request.GET.get("uri")}' return Response( From 7d8052e63c9863a4508ecd6cd86bd8b9f2a897a6 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 18 Oct 2024 15:56:06 -0700 Subject: [PATCH 4/7] update tests to differentiate between exceptions --- .../sentry_apps/external_requests/test_select_requester.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/sentry_apps/external_requests/test_select_requester.py b/tests/sentry/sentry_apps/external_requests/test_select_requester.py index f620f0a2b16218..775eab625dc055 100644 --- a/tests/sentry/sentry_apps/external_requests/test_select_requester.py +++ b/tests/sentry/sentry_apps/external_requests/test_select_requester.py @@ -106,7 +106,7 @@ def test_500_response(self): status=500, ) - with pytest.raises(APIError): + with pytest.raises(Exception): SelectRequester( install=self.install, project_slug=self.project.slug, From 133aa4b48f79094d93c39a13b27ff69a14815d56 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 18 Oct 2024 18:01:27 -0700 Subject: [PATCH 5/7] update tests and update returned errors to be a bit more acc --- .../endpoints/installation_external_requests.py | 14 +++++++++----- .../external_requests/select_requester.py | 14 +++++++++++--- .../sentry_apps/models/sentry_app_installation.py | 3 ++- .../external_requests/test_select_requester.py | 7 ++++--- 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py index 58a0aa922c7248..d7b0d19592303a 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py @@ -1,14 +1,19 @@ +import logging + +from jsonschema import ValidationError from rest_framework.request import Request from rest_framework.response import Response from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import region_silo_endpoint -from sentry.api.client import ApiError +from sentry.coreapi import APIError from sentry.models.project import Project from sentry.sentry_apps.api.bases.sentryapps import SentryAppInstallationBaseEndpoint from sentry.sentry_apps.external_requests.select_requester import SelectRequester +logger = logging.getLogger("sentry.sentry-apps") + @region_silo_endpoint class SentryAppInstallationExternalRequestsEndpoint(SentryAppInstallationBaseEndpoint): @@ -37,13 +42,12 @@ def get(self, request: Request, installation) -> Response: try: choices = SelectRequester(**kwargs).run() - except ApiError: - message = f"Error validating response options from {installation.sentry_app.slug} for {request.GET.get('uri')}" + except ValidationError as e: return Response( - {"error": message}, + {"error": e.message}, status=400, ) - except Exception: + except APIError: message = f'Error retrieving select field options from {request.GET.get("uri")}' return Response( {"error": message}, diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index 1fefce1df44623..c4d1f54ab57c45 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -5,6 +5,7 @@ from uuid import uuid4 from django.utils.functional import cached_property +from jsonschema import ValidationError from sentry.coreapi import APIError from sentry.http import safe_urlread @@ -60,7 +61,7 @@ def run(self) -> dict[str, Any]: "url": url, }, ) - raise + raise APIError from e if not self._validate_response(response) or not response: logger.info( @@ -74,7 +75,7 @@ def run(self) -> dict[str, Any]: "url": url, }, ) - raise APIError( + raise ValidationError( f"Invalid response format for SelectField in {self.sentry_app} from uri: {self.uri}" ) return self._format_response(response) @@ -109,7 +110,14 @@ def _format_response(self, resp: list[dict[str, Any]]) -> dict[str, Any]: for option in resp: if not ("value" in option and "label" in option): - raise APIError("Missing `value` or `label` in option data for SelectField") + logger.info( + "select-requester.invalid-response", + extra={ + "resposnse": resp, + "error_msg": "Missing `value` or `label` in option data for SelectField", + }, + ) + raise ValidationError("Missing `value` or `label` in option data for SelectField") choices.append([option["value"], option["label"]]) diff --git a/src/sentry/sentry_apps/models/sentry_app_installation.py b/src/sentry/sentry_apps/models/sentry_app_installation.py index 55b126bd2e7689..c0eb232c0b6f99 100644 --- a/src/sentry/sentry_apps/models/sentry_app_installation.py +++ b/src/sentry/sentry_apps/models/sentry_app_installation.py @@ -7,6 +7,7 @@ from django.db import models from django.db.models import QuerySet from django.utils import timezone +from jsonschema import ValidationError from sentry.auth.services.auth import AuthenticatedToken from sentry.backup.scopes import RelocationScope @@ -241,6 +242,6 @@ def prepare_ui_component( component=component, install=installation, project_slug=project_slug, values=values ).run() return component - except APIError: + except (APIError, ValidationError): # TODO(nisanthan): For now, skip showing the UI Component if the API requests fail return None diff --git a/tests/sentry/sentry_apps/external_requests/test_select_requester.py b/tests/sentry/sentry_apps/external_requests/test_select_requester.py index 775eab625dc055..63a726515e5b58 100644 --- a/tests/sentry/sentry_apps/external_requests/test_select_requester.py +++ b/tests/sentry/sentry_apps/external_requests/test_select_requester.py @@ -1,5 +1,6 @@ import pytest import responses +from jsonschema import ValidationError from sentry.coreapi import APIError from sentry.sentry_apps.external_requests.select_requester import SelectRequester @@ -69,7 +70,7 @@ def test_invalid_response_missing_label(self): content_type="application/json", ) - with pytest.raises(APIError): + with pytest.raises(ValidationError): SelectRequester( install=self.install, project_slug=self.project.slug, @@ -90,7 +91,7 @@ def test_invalid_response_missing_value(self): content_type="application/json", ) - with pytest.raises(APIError): + with pytest.raises(ValidationError): SelectRequester( install=self.install, project_slug=self.project.slug, @@ -106,7 +107,7 @@ def test_500_response(self): status=500, ) - with pytest.raises(Exception): + with pytest.raises(APIError): SelectRequester( install=self.install, project_slug=self.project.slug, From 69d2872627483bbfa135b1259d283eea763b6397 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 21 Oct 2024 13:45:06 -0700 Subject: [PATCH 6/7] only log the url to debug --- src/sentry/sentry_apps/external_requests/select_requester.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index c4d1f54ab57c45..3a01d070ac75e6 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -56,7 +56,6 @@ def run(self) -> dict[str, Any]: "sentry_app_slug": self.sentry_app.slug, "install_uuid": self.install.uuid, "project_slug": self.project_slug, - "uri": self.uri, "error_message": str(e), "url": url, }, @@ -71,7 +70,6 @@ def run(self) -> dict[str, Any]: "sentry_app_slug": self.sentry_app.slug, "install_uuid": self.install.uuid, "project_slug": self.project_slug, - "uri": self.uri, "url": url, }, ) From 05943ee459f3ad8308dc9e9e05a07f3ea358638c Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 22 Oct 2024 14:31:47 -0700 Subject: [PATCH 7/7] empty arrays should be valid --- src/sentry/sentry_apps/external_requests/select_requester.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index 3a01d070ac75e6..67f11b3b16c72e 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -62,7 +62,7 @@ def run(self) -> dict[str, Any]: ) raise APIError from e - if not self._validate_response(response) or not response: + if not self._validate_response(response): logger.info( "select-requester.invalid-response", extra={