Skip to content

Commit

Permalink
ref: avoid clobbering APIView.handle_exception (#81673)
Browse files Browse the repository at this point in the history
<!-- Describe your PR here. -->
  • Loading branch information
asottile-sentry authored Dec 4, 2024
1 parent d773f4d commit 9233647
Show file tree
Hide file tree
Showing 10 changed files with 38 additions and 33 deletions.
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ module = [
"sentry.incidents.endpoints.organization_incident_index",
"sentry.incidents.subscription_processor",
"sentry.incidents.tasks",
"sentry.integrations.api.bases.integration",
"sentry.integrations.aws_lambda.integration",
"sentry.integrations.bitbucket.client",
"sentry.integrations.bitbucket.installed",
Expand Down
6 changes: 3 additions & 3 deletions src/sentry/api/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ def permission_denied(self, request, message=None, code=None):

super().permission_denied(request, message, code)

def handle_exception( # type: ignore[override]
def handle_exception_with_details(
self,
request: Request,
exc: Exception,
Expand All @@ -321,7 +321,7 @@ def handle_exception( # type: ignore[override]
# Django REST Framework's built-in exception handler. If `settings.EXCEPTION_HANDLER`
# exists and returns a response, that's used. Otherwise, `exc` is just re-raised
# and caught below.
response = super().handle_exception(exc)
response = self.handle_exception(exc)
except Exception as err:
import sys
import traceback
Expand Down Expand Up @@ -456,7 +456,7 @@ def dispatch(self, request: Request, *args, **kwargs) -> Response:
response = handler(request, *args, **kwargs)

except Exception as exc:
response = self.handle_exception(request, exc)
response = self.handle_exception_with_details(request, exc)

if origin:
self.add_cors_headers(request, response)
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/api/bases/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ def get_filter_params(self, request: Request, project, date_filter_optional=Fals

return params

def handle_exception( # type: ignore[override]
def handle_exception_with_details(
self,
request: Request,
exc: Exception,
Expand All @@ -221,4 +221,4 @@ def handle_exception( # type: ignore[override]
)
response["Location"] = exc.new_url
return response
return super().handle_exception(request, exc, handler_context, scope)
return super().handle_exception_with_details(request, exc, handler_context, scope)
12 changes: 8 additions & 4 deletions src/sentry/integrations/api/bases/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,26 +42,30 @@ class IntegrationEndpoint(ControlSiloOrganizationEndpoint):
Baseclass for integration endpoints in control silo that need integration exception handling
"""

def handle_exception(
def handle_exception_with_details(
self,
request: Request,
exc: Exception,
*args: Any,
**kwds: Any,
) -> Response:
return _handle_exception(exc) or super().handle_exception(request, exc, *args, **kwds)
return _handle_exception(exc) or super().handle_exception_with_details(
request, exc, *args, **kwds
)


class RegionIntegrationEndpoint(OrganizationEndpoint):
"""
Baseclass for integration endpoints in region silo that need integration exception handling
"""

def handle_exception(
def handle_exception_with_details(
self,
request: Request,
exc: Exception,
*args: Any,
**kwds: Any,
) -> Response:
return _handle_exception(exc) or super().handle_exception(request, exc, *args, **kwds)
return _handle_exception(exc) or super().handle_exception_with_details(
request, exc, *args, **kwds
)
4 changes: 2 additions & 2 deletions src/sentry/integrations/api/endpoints/integration_proxy.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ def http_method_not_allowed(self, request):
)
return response

def handle_exception( # type: ignore[override]
def handle_exception_with_details(
self,
request: DRFRequest,
exc: Exception,
Expand All @@ -240,4 +240,4 @@ def handle_exception( # type: ignore[override]
logger.info("hybrid_cloud.integration_proxy.host_timeout_error", extra=self.log_extra)
return self.respond(status=exc.code)

return super().handle_exception(request, exc, handler_context, scope)
return super().handle_exception_with_details(request, exc, handler_context, scope)
4 changes: 2 additions & 2 deletions src/sentry/integrations/jira/webhooks/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class JiraWebhookBase(Endpoint, abc.ABC):
def dispatch(self, request: Request, *args, **kwargs) -> Response:
return super().dispatch(request, *args, **kwargs)

def handle_exception(
def handle_exception_with_details(
self,
request: Request,
exc: Exception,
Expand Down Expand Up @@ -108,7 +108,7 @@ def handle_exception(

# This will log the error locally, capture the exception and send it to Sentry, and create a
# generic 500/Internal Error response
return super().handle_exception(request, exc, handler_context, scope)
return super().handle_exception_with_details(request, exc, handler_context, scope)

def get_token(self, request: Request) -> str:
try:
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/integrations/jira/webhooks/issue_updated.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class JiraIssueUpdatedWebhook(JiraWebhookBase):
Webhook hit by Jira whenever an issue is updated in Jira's database.
"""

def handle_exception(
def handle_exception_with_details(
self,
request: Request,
exc: Exception,
Expand All @@ -45,7 +45,7 @@ def handle_exception(
if response_option:
return self.respond(response_option)

return super().handle_exception(request, exc, handler_context, scope)
return super().handle_exception_with_details(request, exc, handler_context, scope)

def post(self, request: Request, *args, **kwargs) -> Response:
token = self.get_token(request)
Expand Down
10 changes: 6 additions & 4 deletions tests/sentry/api/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def __init__(
):
# The error which will be thrown when a GET request is made
self.error = error
# The argumets which will be passed on to `Endpoint.handle_exception` via `super`
# The argumets which will be passed on to `Endpoint.handle_exception_with_details` via `super`
self.handler_context_arg = handler_context_arg
self.scope_arg = scope_arg

Expand All @@ -77,8 +77,10 @@ def __init__(
def get(self, request):
raise self.error

def handle_exception(self, request, exc, handler_context=None, scope=None):
return super().handle_exception(request, exc, self.handler_context_arg, self.scope_arg)
def handle_exception_with_details(self, request, exc, handler_context=None, scope=None):
return super().handle_exception_with_details(
request, exc, self.handler_context_arg, self.scope_arg
)


class DummyPaginationEndpoint(Endpoint):
Expand Down Expand Up @@ -339,7 +341,7 @@ def test_handle_exception_when_super_returns_response(
mock_endpoint = DummyErroringEndpoint.as_view(error=Exception("nope"))
response = mock_endpoint(self.make_request(method="GET"))

# The endpoint should pass along the response generated by `APIView.handle_exception`
# The endpoint should pass along the response generated by `APIView.handle_exception_with_details`
assert response == mock_super_handle_exception.return_value

@mock.patch("rest_framework.views.APIView.handle_exception", new=reraise)
Expand Down
8 changes: 4 additions & 4 deletions tests/sentry/integrations/api/bases/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
@patch("sys.stderr.write")
class IntegrationEndpointTest(TestCase):

# Since both `IntegrationEndpoint.handle_exception` and `Endpoint.handle_exception` potentially
# Since both `IntegrationEndpoint.handle_exception_with_details` and `Endpoint.handle_exception_with_details` potentially
# run, and they both call their own module's copy of `capture_exception`, in order to prove that
# neither one is not called, we assert on the underlying method from the SDK
@patch("sentry_sdk.Scope.capture_exception")
Expand All @@ -23,7 +23,7 @@ def test_handle_rest_framework_exception(
exc.status_code = 400 # not possible to set in init
request = Request(HttpRequest())

resp = IntegrationEndpoint().handle_exception(request, exc)
resp = IntegrationEndpoint().handle_exception_with_details(request, exc)

# `APIException`s are handled by Django REST Framework's built-in exception handler, which
# doesn't log errors or report them to Sentry
Expand All @@ -42,7 +42,7 @@ def test_handle_exception_503(
except ApiError as exc:
request = Request(HttpRequest())

resp = IntegrationEndpoint().handle_exception(request, exc)
resp = IntegrationEndpoint().handle_exception_with_details(request, exc)

mock_capture_exception.assert_called_with(exc)
(((s,), _),) = mock_stderror_write.call_args_list
Expand All @@ -63,7 +63,7 @@ def test_handle_exception_stdlib(
except ValueError as exc:
request = Request(HttpRequest())

resp = IntegrationEndpoint().handle_exception(request, exc)
resp = IntegrationEndpoint().handle_exception_with_details(request, exc)

assert mock_capture_exception.call_args.args[0] == exc
(((s,), _),) = mock_stderror_write.call_args_list
Expand Down
18 changes: 9 additions & 9 deletions tests/sentry/integrations/jira/test_webhooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -236,15 +236,15 @@ def test_atlassian_pen_testing_bot(
# This kind of error shouldn't be sent to Sentry
assert mock_capture_exception.call_count == 0

@patch("sentry.api.base.Endpoint.handle_exception", return_value=Response())
@patch("sentry.api.base.Endpoint.handle_exception_with_details", return_value=Response())
def test_APIError_host_and_path_added_as_tags(self, mock_super_handle_exception: MagicMock):
handler_error = ApiError("", url="http://maiseycharlie.jira.com/rest/api/3/dogs/tricks")
mock_endpoint = MockErroringJiraEndpoint.as_view(error=handler_error)

request = self.make_request(method="GET")
mock_endpoint(request)

# signature is super().handle_exception(request, error, handler_context, scope)
# signature is super().handle_exception_with_details(request, error, handler_context, scope)
assert (
mock_super_handle_exception.call_args.args[3]._tags["jira.host"]
== "maiseycharlie.jira.com"
Expand All @@ -254,7 +254,7 @@ def test_APIError_host_and_path_added_as_tags(self, mock_super_handle_exception:
== "/rest/api/3/dogs/tricks"
)

@patch("sentry.api.base.Endpoint.handle_exception", return_value=Response())
@patch("sentry.api.base.Endpoint.handle_exception_with_details", return_value=Response())
def test_handles_xml_as_error_message(self, mock_super_handle_exception: MagicMock):
"""Moves the XML to `handler_context` and replaces it with a human-friendly message"""
xml_string = '<?xml version="1.0"?><status><code>500</code><message>PSQLException: too many connections</message></status>'
Expand All @@ -267,12 +267,12 @@ def test_handles_xml_as_error_message(self, mock_super_handle_exception: MagicMo
request = self.make_request(method="GET")
mock_endpoint(request)

# signature is super().handle_exception(request, error, handler_context, scope)
# signature is super().handle_exception_with_details(request, error, handler_context, scope)
assert mock_super_handle_exception.call_args.args[1] == handler_error
assert str(handler_error) == "Unknown error when requesting /rest/api/3/dogs/tricks"
assert mock_super_handle_exception.call_args.args[2]["xml_response"] == xml_string

@patch("sentry.api.base.Endpoint.handle_exception", return_value=Response())
@patch("sentry.api.base.Endpoint.handle_exception_with_details", return_value=Response())
def test_handles_html_as_error_message(self, mock_super_handle_exception: MagicMock):
"""Moves the HTML to `handler_context` and replaces it with a human-friendly message"""
html_strings = [
Expand All @@ -291,12 +291,12 @@ def test_handles_html_as_error_message(self, mock_super_handle_exception: MagicM
request = self.make_request(method="GET")
mock_endpoint(request)

# signature is super().handle_exception(request, error, handler_context, scope)
# signature is super().handle_exception_with_details(request, error, handler_context, scope)
assert mock_super_handle_exception.call_args.args[1] == handler_error
assert str(handler_error) == "Unknown error when requesting /rest/api/3/dogs/tricks"
assert mock_super_handle_exception.call_args.args[2]["html_response"] == html_string

@patch("sentry.api.base.Endpoint.handle_exception", return_value=Response())
@patch("sentry.api.base.Endpoint.handle_exception_with_details", return_value=Response())
def test_replacement_error_messages(self, mock_super_handle_exception: MagicMock):
replacement_messages_by_code = {
429: "Rate limit hit when requesting /rest/api/3/dogs/tricks",
Expand All @@ -316,12 +316,12 @@ def test_replacement_error_messages(self, mock_super_handle_exception: MagicMock
request = self.make_request(method="GET")
mock_endpoint(request)

# signature is super().handle_exception(request, error, handler_context, scope)
# signature is super().handle_exception_with_details(request, error, handler_context, scope)
assert mock_super_handle_exception.call_args.args[1] == handler_error
assert str(handler_error) == new_message

@patch("sentry.integrations.jira.webhooks.base.logger")
@patch("sentry.api.base.Endpoint.handle_exception", return_value=Response())
@patch("sentry.api.base.Endpoint.handle_exception_with_details", return_value=Response())
def test_unexpected_jira_errors(
self, mock_super_handle_exception: MagicMock, mock_logger: MagicMock
):
Expand Down

0 comments on commit 9233647

Please sign in to comment.