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 issue with passed routes and provides #1095

Closed
wants to merge 1 commit into from

Conversation

@mwpastore
Copy link
Member

@mwpastore mwpastore commented Feb 10, 2016

This patch addresses an oddball issue that was the source of much hair-pulling this evening. Because the provides condition both branches on and mutates response['Content-Type'], a passed route with a provides condition can interfere with the provides condition of a later route (assuming both the initial and later routes match the requested path).

Consider an example:

get '/:id', :provides => :json do
  pass if params[:id] =~ /\D/

  # some processing
end

get '/foo', :provides => :text do
  # some other processing
end

A GET request to /foo with an Accept header of application/json,text/plain will always return 404. Why? Because the compiled provides condition of the first route will set response['Content-Type'] to application/json (here), and subsequent compiled provides conditions will short-circuit any attempts to re-examine the preferred types (here).

Workarounds:

  • Don't use a :provides condition; instead, use Sinatra::Request#preferred_type to inspect the Accept header on the request and Sinatra::Base#content_type to set the Content-Type header on the response.
  • Structure routes to prevent pattern overlap.
  • When there is pattern overlap and matching depends on the exact format of the path, use a regular expression and params['captures'] (or a block parameter) instead of regular route parameters.
  • When there is pattern overlap, combine the logic into a single route (perhaps using helpers) instead of passing to another route.
  • Manually clear request['Content-Type'] before calling pass.

Solutions:

  1. Clear request['Content-Type'] when throwing :pass
  2. Clear request['Content-Type'] when catching :pass
  3. Clear request['Content-Type'] in-between route iterations (maybe?)

This patch implements solution 1 but I'm interested in your feedback.

Thank you in advance!

@mwpastore mwpastore force-pushed the mwpastore:pass-clears-content-type branch 2 times, most recently from 845e56a to 75ab338 Feb 12, 2016
@mwpastore mwpastore force-pushed the mwpastore:pass-clears-content-type branch from 75ab338 to 3be4d73 Feb 12, 2016
@mwpastore mwpastore closed this Feb 12, 2016
@mwpastore
Copy link
Member Author

@mwpastore mwpastore commented Feb 12, 2016

It seems as though "pinning" the content-type of the response in before filters is a documented (or at least test-driven) feature, so clearing the content-type in-between calls to process_route or even after a pass is not a solution to this problem. I think there's a compromise solution here where if the content-type is set after the before filters are invoked, use it to select routes, otherwise clear the content-type in-between calls to process_route. I've closed this PR for now as it implements as breaking change and is therefore not acceptable.

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

Successfully merging this pull request may close these issues.

None yet

1 participant
You can’t perform that action at this time.