fix(features): return 404 instead of 500 for a non-integer identity ID in the URL - #8514
IshanA2007 wants to merge 1 commit into
Conversation
…D in the URL The identity_pk path parameter is now validated as an integer before it reaches the ORM filter, raising NotFound otherwise. Closes Flagsmith#7361
|
@IshanA2007 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change validates non-integer identity IDs and returns the intended 404 response. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
A non-integer identity ID in the URL crashes the API.
GET /api/v1/environments/{api_key}/identities/{identity_pk}/featurestates/passesidentity_pkstraight into an ORM filter on an integer field, so a value such asorg_3COWhASRXfhcdxVrd0wEjpQp4dgraises an unhandledValueErrorand the caller gets a 500 (Sentry FLAGSMITH-API-5NF).Why 404 rather than 400: the other two actions behind the same URL prefix (
/featurestates/all/and/featurestates/clone-from-given-identity/) already answer 404 for this same value, because DRF'sget_object_or_404converts theValueError, andBaseFeatureStateViewSetalready raisesNotFoundfor an unknownenvironment_api_key. Returning 400 fromlistalone would make the endpoint inconsistent with itself. Happy to switch to a 400ValidationError(as inintegrations/github/views.py) if you'd prefer to keep every #6809 fix on 400.Spotted while working on this and deliberately left alone to keep the PR to one concern: a non-integer
?feature=query parameter on the same endpoint still returns a 500. That looks like another instance of #6809 if you'd like it raised separately.Closes #7361
Review effort: 1/5
AI disclosure: this change was prepared with the assistance of an AI coding agent (Claude).
How did you test this code?
Added
test_list_identity_feature_states__non_integer_identity_pk__returns_404toapi/tests/unit/features/test_unit_features_views.py, asserting a 404 and a{"detail": "Identity not found."}body for bothuserandmaster_api_keyauthentication. Reverting the fix makes it fail with theValueErrorfrom the Sentry trace.Ran
make test opts="tests/unit/features/test_unit_features_views.py -k identity -n0"(44 passed), plusruff check,ruff format --checkandmypyon both changed files.