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 #1097

Closed
wants to merge 1 commit into from

Conversation

mwpastore
Copy link
Member

GitHub won't let me reopen #1095, so this replaces that PR.

This patch addresses an oddball issue. 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 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 or catching :pass
  2. Clear request['Content-Type'] in-between route iterations (calls to process_route)

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 while throwing or catching a pass is not a solution to this problem.

This PR implements a compromise solution where if the Content-Type is set while the before filters are being applied, use it as the Content-Type of the response (i.e. it becomes a route selector), otherwise clear the Content-Type in-between calls to process_route.

@zzak
Copy link
Member

zzak commented Apr 13, 2016

Ok, I'll need some time to look into this, thanks for your explanation!

@zzak zzak added feature and removed feature labels Apr 13, 2016
@zzak
Copy link
Member

zzak commented Jan 30, 2017

@mwpastore Could you review this after the mustermann merge, and either close it or rebase it to suit your needs? Thanks!

@mwpastore
Copy link
Member Author

Will do!

@namusyaka namusyaka added this to the v2.0.2 milestone Feb 19, 2018
@namusyaka namusyaka modified the milestones: v2.0.2, v2.0.3, v2.0.4 Jun 5, 2018
@namusyaka
Copy link
Member

@mwpastore Could you rebase this branch?

@namusyaka namusyaka modified the milestones: v2.0.4, v2.0.5 Sep 14, 2018
@namusyaka namusyaka modified the milestones: v2.0.5, v2.0.6 Dec 22, 2018
@jkowens jkowens modified the milestones: v2.0.6, v2.1.0 Mar 14, 2020
@jkowens jkowens closed this Mar 16, 2020
@jkowens jkowens reopened this Mar 16, 2020
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.

None yet

4 participants