feat(api): Make InvalidParams a ParseError#112542
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
|
|
||
| class InvalidParams(Exception): | ||
| class InvalidParams(ParseError): |
There was a problem hiding this comment.
Bug: By inheriting from ParseError, InvalidParams exceptions will be silently caught by existing handlers, preventing them from being reported to Sentry.
Severity: MEDIUM
Suggested Fix
Revert the inheritance change so that InvalidParams does not inherit from ParseError. This will ensure that InvalidParams exceptions are not caught by generic ParseError handlers and can be reported to Sentry as intended.
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/exceptions.py#L93
Potential issue: Changing the `InvalidParams` exception to inherit from `ParseError`
causes it to be silently caught in exception handlers that are designed to catch
`ParseError`. An example of this is in `organization_events.py`. As a result,
`InvalidParams` exceptions will no longer be captured and reported to Sentry, leading to
a loss of observability for this error type.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
This is true. The real question here is "given that these signal invalid parameters, why do we want to see them in Sentry?".
There was a problem hiding this comment.
^^ agree; i ran into this with a different kind of ValidationError and was surprised it didn't just work.
There was a problem hiding this comment.
It's a bit scary to change after there are already dozens of uses, but gosh we'd be better off if api-focused validation stuff produced the appropriate status code/response by default.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
InvalidParam is raised in API parsing contexts; we have 20+ cases already catching it and turning it into a 400, another 15 or so cases where we _should_ be (prior to this change). Rather than continuing to add that boilerplate, we can make InvalidParam a ParseError, so that the default behavior when parameters don't seem correct is a 400 response. The existing uses that don't convert to a 400 still work as they did previously.
3 participants
InvalidParam is raised in API parsing contexts; we have 20+ cases already catching it and turning it into a 400, another 15 or so cases where we should be (prior to this change).
Rather than continuing to add that boilerplate, we can make InvalidParam a ParseError, so that the default behavior when parameters don't seem correct is a 400 response.
The existing uses that don't convert to a 400 still work as they did previously.