Skip to content

feat(api): Make InvalidParams a ParseError #112542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kcons merged 2 commits into from
Apr 9, 2026
Merged

feat(api): Make InvalidParams a ParseError #112542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b67b868
feat(api): Make InvalidParams a ParseError
kcons Apr 8, 2026
7691809
fix mypy, remove truly redundant exn handing
kcons Apr 8, 2026
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
3 changes: 0 additions & 3 deletions src/sentry/api/endpoints/organization_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from sentry.apidocs.parameters import GlobalParams, OrganizationParams, VisibilityParams
from sentry.apidocs.utils import inline_sentry_response_serializer
from sentry.discover.models import DiscoverSavedQuery, DiscoverSavedQueryTypes
from sentry.exceptions import InvalidParams
from sentry.models.dashboard_widget import DashboardWidget, DashboardWidgetTypes
from sentry.models.organization import Organization
from sentry.ratelimits.config import RateLimitConfig
Expand Down Expand Up @@ -194,8 +193,6 @@ def get(self, request: Request, organization: Organization) -> Response:
},
}
)
except InvalidParams as err:
raise ParseError(detail=str(err))

batch_features = self.get_features(organization, request)

Expand Down
6 changes: 5 additions & 1 deletion src/sentry/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from django.core.exceptions import SuspiciousOperation
from rest_framework.exceptions import ParseError


class InvalidData(Exception):
Expand Down Expand Up @@ -89,5 +90,8 @@ def __init__(
self.tombstone_id = tombstone_id


class InvalidParams(Exception):
class InvalidParams(ParseError):
Copy link
Copy Markdown
Contributor

@sentry sentry bot Apr 8, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@kcons kcons Apr 8, 2026

Choose a reason for hiding this comment

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

This is true. The real question here is "given that these signal invalid parameters, why do we want to see them in Sentry?".

Copy link
Copy Markdown
Contributor

@saponifi3d saponifi3d Apr 8, 2026

Choose a reason for hiding this comment

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

^^ agree; i ran into this with a different kind of ValidationError and was surprised it didn't just work.

Copy link
Copy Markdown
Member Author

@kcons kcons Apr 8, 2026

Choose a reason for hiding this comment

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

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.

"""Inherits from ParseError so DRF automatically returns a 400 response
when this exception is unhandled in a view."""

pass
7 changes: 1 addition & 6 deletions src/sentry/issues/endpoints/group_attachments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from datetime import datetime, timedelta

from django.utils import timezone
from rest_framework.exceptions import ParseError
from rest_framework.request import Request
from rest_framework.response import Response

Expand All @@ -16,7 +15,6 @@
from sentry.api.serializers import EventAttachmentSerializer, serialize
from sentry.api.utils import get_date_range_from_params, handle_query_errors
from sentry.constants import CELL_API_DEPRECATION_DATE
from sentry.exceptions import InvalidParams
from sentry.issues.endpoints.bases.group import GroupEndpoint
from sentry.models.eventattachment import EventAttachment, event_attachment_screenshot_filter
from sentry.models.group import Group
Expand Down Expand Up @@ -103,10 +101,7 @@ def get(self, request: Request, group) -> Response:
event_ids = request.GET.getlist("event_id") or None
screenshot = "screenshot" in request.GET

try:
start, end = get_date_range_from_params(request.GET, optional=True)
except InvalidParams as e:
raise ParseError(detail=str(e))
start, end = get_date_range_from_params(request.GET, optional=True)

if start:
attachments = attachments.filter(date_added__gte=start)
Expand Down
7 changes: 2 additions & 5 deletions src/sentry/issues/endpoints/group_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from sentry.apidocs.parameters import CursorQueryParam, EventParams, GlobalParams, IssueParams
from sentry.apidocs.utils import inline_sentry_response_serializer
from sentry.constants import CELL_API_DEPRECATION_DATE
from sentry.exceptions import InvalidParams, InvalidSearchQuery
from sentry.exceptions import InvalidSearchQuery
from sentry.issues.endpoints.bases.group import GroupEndpoint
from sentry.search.events.types import SnubaParams
from sentry.search.utils import InvalidQuery, parse_query
Expand Down Expand Up @@ -99,10 +99,7 @@ def get(self, request: Request, group: Group) -> Response:
except (NoResults, ResourceDoesNotExist):
return Response([])

try:
start, end = get_date_range_from_params(request.GET, optional=True)
except InvalidParams as e:
raise ParseError(detail=str(e))
start, end = get_date_range_from_params(request.GET, optional=True)

try:
with handle_query_errors():
Expand Down
9 changes: 3 additions & 6 deletions src/sentry/issues/endpoints/organization_group_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import sentry_sdk
from django.utils import timezone
from drf_spectacular.utils import extend_schema
from rest_framework.exceptions import ParseError, PermissionDenied
from rest_framework.exceptions import PermissionDenied
from rest_framework.request import Request
from rest_framework.response import Response
from sentry_sdk import start_span
Expand Down Expand Up @@ -54,7 +54,7 @@
)
from sentry.apidocs.utils import inline_sentry_response_serializer
from sentry.constants import ALLOWED_FUTURE_DELTA
from sentry.exceptions import InvalidParams, InvalidSearchQuery
from sentry.exceptions import InvalidSearchQuery
from sentry.models.environment import Environment
from sentry.models.group import QUERY_STATUS_LOOKUP, Group, GroupStatus
from sentry.models.groupenvironment import GroupEnvironment
Expand Down Expand Up @@ -242,10 +242,7 @@ def _search(
@track_slo_response("workflow")
def get(self, request: Request, organization: Organization) -> Response:
stats_period = request.GET.get("groupStatsPeriod")
try:
start, end = get_date_range_from_stats_period(request.GET)
except InvalidParams as e:
raise ParseError(detail=str(e))
start, end = get_date_range_from_stats_period(request.GET)

expand = request.GET.getlist("expand", [])
collapse = request.GET.getlist("collapse", [])
Expand Down
6 changes: 1 addition & 5 deletions src/sentry/issues/endpoints/organization_group_index_stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
from sentry.api.serializers import serialize
from sentry.api.serializers.models.group_stream import StreamGroupSerializerSnuba
from sentry.api.utils import get_date_range_from_stats_period
from sentry.exceptions import InvalidParams
from sentry.issues.endpoints.organization_group_index import ERR_INVALID_STATS_PERIOD
from sentry.models.group import Group
from sentry.models.organization import Organization
Expand Down Expand Up @@ -68,10 +67,7 @@ def get(self, request: Request, organization: Organization) -> Response:
"""

stats_period = request.GET.get("groupStatsPeriod")
try:
start, end = get_date_range_from_stats_period(request.GET)
except InvalidParams as e:
raise ParseError(detail=str(e))
start, end = get_date_range_from_stats_period(request.GET)

expand = request.GET.getlist("expand", [])
collapse = request.GET.getlist("collapse", ["base"])
Expand Down
7 changes: 1 addition & 6 deletions src/sentry/issues/endpoints/organization_issues_count.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from rest_framework.exceptions import ParseError
from rest_framework.request import Request
from rest_framework.response import Response
from sentry_sdk import start_span
Expand All @@ -11,7 +10,6 @@
from sentry.api.helpers.group_index import validate_search_filter_permissions
from sentry.api.helpers.group_index.validators import ValidationError
from sentry.api.utils import get_date_range_from_params
from sentry.exceptions import InvalidParams
from sentry.issues.issue_search import convert_query_values, parse_search_query
from sentry.models.organization import Organization
from sentry.organizations.services.organization.model import RpcOrganization
Expand Down Expand Up @@ -76,10 +74,7 @@ def _count(

def get(self, request: Request, organization: Organization | RpcOrganization) -> Response:
stats_period = request.GET.get("groupStatsPeriod")
try:
start, end = get_date_range_from_params(request.GET)
except InvalidParams as e:
raise ParseError(detail=str(e))
start, end = get_date_range_from_params(request.GET)

if stats_period not in (None, "", "24h", "14d", "auto"):
return Response({"detail": ERR_INVALID_STATS_PERIOD}, status=400)
Expand Down
23 changes: 10 additions & 13 deletions src/sentry/releases/endpoints/organization_release_health_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,19 +71,16 @@ def _validate_fields(self, request: Request):
"""
fields = request.GET.getlist("field", [])
for field in fields:
try:
metric_field = parse_field(field, allow_mri=True)
if (
metric_field.op is None
and not metric_field.metric_mri.startswith("e:")
and metric_field.metric_mri
!= "d:sessions/duration.exited@second" # this is special case, derived metric without 'e' as entity
):
raise ParseError(
detail="You can not use generic metric public field without operation"
)
except InvalidParams as exc:
raise ParseError(detail=str(exc))
metric_field = parse_field(field, allow_mri=True)
if (
metric_field.op is None
and not metric_field.metric_mri.startswith("e:")
and metric_field.metric_mri
!= "d:sessions/duration.exited@second" # this is special case, derived metric without 'e' as entity
):
raise ParseError(
detail="You can not use generic metric public field without operation"
)

def get(self, request: Request, organization: Organization) -> Response:
projects = self.get_projects(request, organization)
Expand Down
7 changes: 1 addition & 6 deletions src/sentry/rules/history/endpoints/project_rule_stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from typing import Any, TypedDict

from drf_spectacular.utils import extend_schema
from rest_framework.exceptions import ParseError
from rest_framework.request import Request
from rest_framework.response import Response

Expand All @@ -16,7 +15,6 @@
from sentry.api.utils import get_date_range_from_params
from sentry.apidocs.constants import RESPONSE_FORBIDDEN, RESPONSE_NOT_FOUND, RESPONSE_UNAUTHORIZED
from sentry.apidocs.parameters import GlobalParams, IssueAlertParams
from sentry.exceptions import InvalidParams
from sentry.models.project import Project
from sentry.models.rule import Rule
from sentry.rules.history import fetch_rule_hourly_stats
Expand Down Expand Up @@ -67,9 +65,6 @@ def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Resp
"""
Note that results are returned in hourly buckets.
"""
try:
start, end = get_date_range_from_params(request.GET)
except InvalidParams as e:
raise ParseError(detail=str(e))
start, end = get_date_range_from_params(request.GET)
results = fetch_rule_hourly_stats(rule, start, end)
return Response(serialize(results, request.user, TimeSeriesValueSerializer()))
2 changes: 1 addition & 1 deletion src/sentry/snuba/metrics/fields/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@
try:
return self.snql_func(**kwargs)
except TypeError as e:
raise InvalidParams(e)
raise InvalidParams(str(e))

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
Loading
flows to this location and may be exposed to an external user.
Stack trace information
Loading
flows to this location and may be exposed to an external user.
Stack trace information
Loading
flows to this location and may be exposed to an external user.
Stack trace information
Loading
flows to this location and may be exposed to an external user.
Comment thread
kcons marked this conversation as resolved.
Dismissed

def get_default_null_values(self) -> int | list[tuple[float]] | None:
return self.default_null_value
Expand Down
Loading