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

Less query params parsing #2553

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ericproulx
Copy link
Contributor

@ericproulx ericproulx commented Apr 5, 2025

This PRs reduces potential query params parsing. Instead of calling Rack::Utils.parse_nested_query(env[Rack::QUERY_STRING]) directly, Grape will call Rack's function GET that does the same work but keeps the parsed version in RACK_REQUEST_QUERY_HASH. This way, parsing will be done at most once.

Also, it fixes #2488 and it comes with a tiny refactor in Grape::Middleware::Formatter including comments in the code. Finally, this PR adds a new function named rack_request that's create a instance of Rack::Request from env in Grape::Middleware::Base. Grape::Middleware::Formatter had already the equivalent but named request. This helps for calling GET in middlewares and I think user might benefit from it.

Add some rescues when dealing with rack_params and GET functions
Add exceptions related to rescues
@ericproulx ericproulx changed the title Less parse nested query Less query params parsing Apr 5, 2025
@ericproulx ericproulx requested a review from dblock April 5, 2025 14:50
@ericproulx ericproulx marked this pull request as ready for review April 5, 2025 14:50
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Nicely done. Only nits from me. Consider adding the removal of CONSTANTs into UPGRADING.

@ericproulx ericproulx force-pushed the less_parse_nested_query branch from a8c6300 to a872545 Compare April 6, 2025 14:28
@ericproulx ericproulx force-pushed the less_parse_nested_query branch from a872545 to e358a4f Compare April 6, 2025 14:32
@@ -1,6 +1,11 @@
Upgrading Grape
===============

### Grape::Middleware::Base
Copy link
Member

Choose a reason for hiding this comment

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

For later, we should have an Upgrading >= X.Y.Z here whatever the next version is.

@dblock
Copy link
Member

dblock commented Apr 6, 2025

Coveralls is down for maintenance, let's wait for it to come back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing query params may lead to unhandled Rack exception.
2 participants