Skip to content
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.

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

fix(api): Validate per_page lower bound #83075

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jan 8, 2025

In our base endpoint class, we check the incoming per_page parameter against an upper bound, but we don't currently check it against a lower bound... until we go to actually use it, at which point we error out with a generic 500 response (rather than the more helpful and explanatory 400 generated by per_page values which are too high).

This fixes that by checking the lower bound alongside the upper bound.

Note: The changes in test_base.py look bigger than they are. Really the only new thing is the test_per_page_too_low test. The rest is just a) giving the per_page tests more specific names, and b) moving the invalid cursor test so all of the per_page tests are together.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #83075      +/-   ##
==========================================
- Coverage   87.59%   87.58%   -0.01%     
==========================================
  Files        9423     9423              
  Lines      536407   536411       +4     
  Branches    21120    21120              
==========================================
- Hits       469839   469822      -17     
- Misses      66209    66230      +21     
  Partials      359      359              

@lobsterkatie lobsterkatie force-pushed the kmclb-check-per-page-lower-bound branch from af95af5 to 72475ea Compare January 8, 2025 01:02
@lobsterkatie lobsterkatie marked this pull request as ready for review January 8, 2025 02:02
@lobsterkatie lobsterkatie requested a review from a team as a code owner January 8, 2025 02:02
@lobsterkatie lobsterkatie merged commit fa0de62 into master Jan 8, 2025
51 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-check-per-page-lower-bound branch January 8, 2025 21:21
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