Skip to content

fix(issues): Assert user.id is not None before ORM filter#107744

Merged
JoshFerge merged 1 commit intomasterfrom
jferg/fix-search-views-none-guard
Feb 6, 2026
Merged

fix(issues): Assert user.id is not None before ORM filter#107744
JoshFerge merged 1 commit intomasterfrom
jferg/fix-search-views-none-guard

Conversation

@JoshFerge
Copy link
Member

@JoshFerge JoshFerge commented Feb 5, 2026

Summary

Found during mypy / django-stubs upgrade in #107710.

Adds assert request.user.id is not None before GroupSearchViewStarred.objects.filter(user_id=request.user.id) in the starred views endpoint. user.id is typed as int | None.

Test plan

  • Pre-commit hooks pass

Found during mypy / django-stubs upgrade in #107710.
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 5, 2026
@JoshFerge JoshFerge marked this pull request as ready for review February 6, 2026 14:45
@JoshFerge JoshFerge requested review from a team as code owners February 6, 2026 14:45
@JoshFerge JoshFerge requested a review from a team February 6, 2026 14:45
@JoshFerge JoshFerge enabled auto-merge (squash) February 6, 2026 14:45
Retrieve a list of starred views for the current organization member.
"""

assert request.user.id is not None
Copy link
Member

Choose a reason for hiding this comment

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

We should probably return a 4XX error here, instead of an assert which will produce a 500

Copy link
Member Author

@JoshFerge JoshFerge Feb 6, 2026

Choose a reason for hiding this comment

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

FWIW it is actually impossible for our code to reach here with user id being none as this is an authenticated endpoint. just an annoyance that our type checker can't handle. can see we have a lot of these peppered throughout our endpoints.

Copy link
Member

Choose a reason for hiding this comment

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

Get your point - I think it's fine, but in general I don't love it. We should find a better way of making sure our typing works in these cases

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah it is not great. I think that it has to do with django/drf typing stubs, where request.user can be User or AnonymousUser. I don't think it's an easy fix.

Retrieve a list of starred views for the current organization member.
"""

assert request.user.id is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: The assertion assert request.user.id is not None will fail for valid API token requests that don't have an authenticated user, causing an unhandled AssertionError.
Severity: MEDIUM

Suggested Fix

Replace the assertion assert request.user.id is not None with an explicit check like if not request.user.is_authenticated: return Response(status=400). This ensures proper error handling for unauthenticated API requests, consistent with other similar endpoints in the codebase.

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/issues/endpoints/organization_group_search_views_starred.py#L33

Potential issue: The endpoint allows access via API tokens with the `member:read` scope,
which does not require an authenticated user. In such cases, `request.user` is an
`AnonymousUser` and `request.user.id` is `None`. The `assert request.user.id is not
None` statement will then raise an `AssertionError`, leading to an unhandled server
exception instead of a proper HTTP error response. This is inconsistent with similar
endpoints that explicitly check for an authenticated user and return a 400 Bad Request
status.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm pretty sure api tokens do get user ids.

@JoshFerge JoshFerge merged commit 4a34cfc into master Feb 6, 2026
74 checks passed
@JoshFerge JoshFerge deleted the jferg/fix-search-views-none-guard branch February 6, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants