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.

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

Jump to bottom

Fix naming a filter offset or limit breaks dashboards #40821

Merged
merged 1 commit into from Apr 3, 2024
Merged

Fix naming a filter offset or limit breaks dashboards #40821

merged 1 commit into from Apr 3, 2024

Conversation

johnswanson
Copy link
Contributor

@johnswanson johnswanson commented Mar 29, 2024 •
edited

Don't throw an exception from that middleware if it's sent a limit or offset query param that isn't an integer.

#38008

@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Mar 29, 2024
@replay-io replay.io
Copy link

replay-io bot commented Mar 29, 2024 •
edited

Status Complete ↗︎
Commit 3fb394c
Results
⚠️ 5 Flaky
✅ 2386 Passed

@johnswanson johnswanson force-pushed the fix-naming-a-filter-offset-or-limit-breaks-dashboards branch from c481f75 to d4582c1 Compare April 1, 2024 12:13
@johnswanson
Don't throw when sent non-int limit/offset
3fb394c 
This middleware shouldn't throw exceptions when `limit` or `offset`
query parametesr aren't integers.
@johnswanson johnswanson force-pushed the fix-naming-a-filter-offset-or-limit-breaks-dashboards branch from d4582c1 to 3fb394c Compare April 1, 2024 12:38
@johnswanson johnswanson added the backport Automatically create PR on current release branch on merge label Apr 1, 2024
@johnswanson johnswanson requested a review from a team April 2, 2024 19:22
noahmoss
noahmoss approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@noahmoss noahmoss left a comment

Choose a reason for hiding this comment

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

Just want to make sure I understand the solution fully here — we're just not throwing an error anymore if limit or offset query params are passed in that can't be parsed? But as long as they're numerical, we'll still parse & bind them as usual?

@johnswanson
Copy link
Contributor Author

Just want to make sure I understand the solution fully here — we're just not throwing an error anymore if limit or offset query params are passed in that can't be parsed? But as long as they're numerical, we'll still parse & bind them as usual?

Yes, sorry - my PR message was wrong because it described another approach which I also implemented but ended up removing from this PR.

@johnswanson johnswanson merged commit cce183e into master Apr 3, 2024
127 of 128 checks passed
@johnswanson johnswanson deleted the fix-naming-a-filter-offset-or-limit-breaks-dashboards branch April 3, 2024 18:30
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2024

@johnswanson Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

@johnswanson johnswanson added this to the 0.49.4 milestone Apr 3, 2024
github-actions bot pushed a commit that referenced this pull request Apr 3, 2024
@johnswanson
Don't throw when sent non-int limit/offset (#40821)
daf5b4e 
This middleware shouldn't throw exceptions when `limit` or `offset`
query parametesr aren't integers.
@metabase-bot metabase-bot bot mentioned this pull request Apr 3, 2024
Merged
metabase-bot bot added a commit that referenced this pull request Apr 11, 2024
@metabase-bot @johnswanson
Don't throw when sent non-int limit/offset (#40821) (#40979)
31c4887 
This middleware shouldn't throw exceptions when `limit` or `offset`
query parametesr aren't integers.

Co-authored-by: John Swanson <john.swanson@metabase.com>
@crisptrutski crisptrutski modified the milestones: 0.49.4, 0.49.5 Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahmoss noahmoss noahmoss approved these changes

@camsaul camsaul Awaiting requested review from camsaul camsaul is a code owner

Assignees

@johnswanson johnswanson

Labels
backport Automatically create PR on current release branch on merge .Team/AdminWebapp Admin and Webapp team
Projects
None yet
Milestone
0.49.5
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@johnswanson @noahmoss @crisptrutski