Calling request.params in a constraint class causes subsequent routes to be broken #2510

Closed
joevandyk opened this Issue Aug 12, 2011 · 19 comments

Comments

Projects
None yet
5 participants
@joevandyk
Contributor

joevandyk commented Aug 12, 2011

joevandyk/rails@f5a9d65 is a failing test case.

class RouteMatcher
  def self.matches? request
    request.params 
    return false
  end
end

Routes::Application.routes.draw do
  match '/:id' => 'categories#show', :constraints => RouteMatcher

  # For a URL of /about, params[:id] will == 'about'
  # You'd expect params[:path] == 'about'
  # If you remove 'request.params' up above, it behaves as expected.
  match '*path' => 'pages#find' 
end
@joevandyk

This comment has been minimized.

Show comment
Hide comment
@joevandyk

joevandyk Aug 12, 2011

Contributor

A possibly related bug that was fixed last year: https://rails.lighthouseapp.com/projects/8994/tickets/5157

Contributor

joevandyk commented Aug 12, 2011

A possibly related bug that was fixed last year: https://rails.lighthouseapp.com/projects/8994/tickets/5157

franckverrot added a commit to franckverrot/rails that referenced this issue Aug 15, 2011

Calling `request.params` in routing constraints cannot interfere with…
… subsequent routes. [Closes #2510]

As the same request is reused thru all the routing constraints,
accessing a memoized form of `params` would prevent the router from
redefining it with a new set of parameters (the current route
parameters).
@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Aug 15, 2011

Member

@joevandyk does request.path_parameters contain the right values?

Member

pixeltrix commented Aug 15, 2011

@joevandyk does request.path_parameters contain the right values?

@ghost ghost assigned pixeltrix Aug 15, 2011

@joevandyk

This comment has been minimized.

Show comment
Hide comment
@joevandyk

joevandyk Aug 15, 2011

Contributor

Yes, it does. An example:

# Routes
match '/:id' => 'categories#show', :constraints => RouteMatcher
match '/:goo', :controller => 'pages', :action => 'find'

# a request for /pages
params: {"controller"=>"categories", "action"=>"show", "id"=>"pages"}
path parameters: {:controller=>"pages", :action=>"find", :goo=>"pages"}
Contributor

joevandyk commented Aug 15, 2011

Yes, it does. An example:

# Routes
match '/:id' => 'categories#show', :constraints => RouteMatcher
match '/:goo', :controller => 'pages', :action => 'find'

# a request for /pages
params: {"controller"=>"categories", "action"=>"show", "id"=>"pages"}
path parameters: {:controller=>"pages", :action=>"find", :goo=>"pages"}
@nodakjones

This comment has been minimized.

Show comment
Hide comment
@nodakjones

nodakjones Aug 15, 2011

request.path_parameters worked for me. Is this the preferred method for accessing params within a constraint? If so, should the routing guide be updated?

request.path_parameters worked for me. Is this the preferred method for accessing params within a constraint? If so, should the routing guide be updated?

@joevandyk

This comment has been minimized.

Show comment
Hide comment
@joevandyk

joevandyk Aug 15, 2011

Contributor

88862a0 seems to fix the bug, as far as I can tell.

Contributor

joevandyk commented Aug 15, 2011

88862a0 seems to fix the bug, as far as I can tell.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Aug 15, 2011

Member

@cesario closed the pull request that contained 88862a0 - probably because he spotted that it was merging path_parameters, query_parameters and request_parameters every time request.params is accessed.

Looking at the Constraints module it seems as though the request is created each time matches? method is called. If so it should be possible to cache the hash in an instance variable rather than the environment hash, similar to the fix in the LightHouse ticket mentioned above. However it will have impact elsewhere so we need to check that we're not breaking something.

It may be easier to just delete the key from the environment hash after the constraint has been checked using an ensure block. What do you think @josevalim?

Member

pixeltrix commented Aug 15, 2011

@cesario closed the pull request that contained 88862a0 - probably because he spotted that it was merging path_parameters, query_parameters and request_parameters every time request.params is accessed.

Looking at the Constraints module it seems as though the request is created each time matches? method is called. If so it should be possible to cache the hash in an instance variable rather than the environment hash, similar to the fix in the LightHouse ticket mentioned above. However it will have impact elsewhere so we need to check that we're not breaking something.

It may be easier to just delete the key from the environment hash after the constraint has been checked using an ensure block. What do you think @josevalim?

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron Mar 26, 2012

Contributor

We are having this same problem. We fail a constraint that has a match '*anything' route. The request is not routed there because of the constraint, but in instead routed to the correct matching route. Unfortunately the params are then equal to {'anything' => '/resource/1/foo'} We do have a failing test, but can't find where we can fix this.

We tried cloning the request object, and that doesn't help.

Contributor

adkron commented Mar 26, 2012

We are having this same problem. We fail a constraint that has a match '*anything' route. The request is not routed there because of the constraint, but in instead routed to the correct matching route. Unfortunately the params are then equal to {'anything' => '/resource/1/foo'} We do have a failing test, but can't find where we can fix this.

We tried cloning the request object, and that doesn't help.

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron Mar 26, 2012

Contributor

We have a workaround that fixes the problem for us:

# We delete this key from the env hash to prevent params from being mangled in later routes
# See issue: https://github.com/rails/rails/issues/2510
request.env.delete('action_dispatch.request.parameters')

which basically prevents memoization of the parameters. We are unsure of the performance consequences of this workaround, and would prefer a real fix that we could use in our production code. Any other ideas or suggestions would be appreicated.

Contributor

adkron commented Mar 26, 2012

We have a workaround that fixes the problem for us:

# We delete this key from the env hash to prevent params from being mangled in later routes
# See issue: https://github.com/rails/rails/issues/2510
request.env.delete('action_dispatch.request.parameters')

which basically prevents memoization of the parameters. We are unsure of the performance consequences of this workaround, and would prefer a real fix that we could use in our production code. Any other ideas or suggestions would be appreicated.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Mar 26, 2012

Member

@adkron are you able to use path_parameters or symbolized_path_parameters in your constraint? Accessing the params hash causes it to be memoized in the environment hash. Preparing the params hash is not a trivial operation so recreating it for every constraint check would have a performance impact. If you use an arity of 2 with the block you can get at the path parameters easily:

class RoutingConstraint
  def self.matches?(params, request)
    #constraint check
  end
end

constraints(RoutingConstraint) do
  #routes
end
Member

pixeltrix commented Mar 26, 2012

@adkron are you able to use path_parameters or symbolized_path_parameters in your constraint? Accessing the params hash causes it to be memoized in the environment hash. Preparing the params hash is not a trivial operation so recreating it for every constraint check would have a performance impact. If you use an arity of 2 with the block you can get at the path parameters easily:

class RoutingConstraint
  def self.matches?(params, request)
    #constraint check
  end
end

constraints(RoutingConstraint) do
  #routes
end
@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron Mar 26, 2012

Contributor

No, path parameters doesn't work for us because we need the full request_parameters (e.g. an auth token). We tried using the two argument form by defining self.call(params, request) which doesn't trigger the bug with the wildcard matcher, but the parameters passed in are just the path parameters, not the full query and post parameters.

Contributor

adkron commented Mar 26, 2012

No, path parameters doesn't work for us because we need the full request_parameters (e.g. an auth token). We tried using the two argument form by defining self.call(params, request) which doesn't trigger the bug with the wildcard matcher, but the parameters passed in are just the path parameters, not the full query and post parameters.

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Mar 27, 2012

Member

@adkron is it a form or query parameter? You should be able to use request.GET and request.POST in the block. They're also aliased as request.query_parameters and request.request_parameters.

Member

pixeltrix commented Mar 27, 2012

@adkron is it a form or query parameter? You should be able to use request.GET and request.POST in the block. They're also aliased as request.query_parameters and request.request_parameters.

@pixeltrix pixeltrix closed this Apr 29, 2012

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix Apr 29, 2012

Member

@adkron as I said it's impossible to fix this without rebuilding the combined params hash on every route comparison so I'm going to close it - if you can see a way that it can be done without that then please point it out and I'll have another look at it.

Member

pixeltrix commented Apr 29, 2012

@adkron as I said it's impossible to fix this without rebuilding the combined params hash on every route comparison so I'm going to close it - if you can see a way that it can be done without that then please point it out and I'll have another look at it.

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron May 2, 2012

Contributor

Impossible is quite a big word. I love when I hear another developer say something is impossible. Hard, maybe, but impossible, no.

It doesn't matter what parameters I touch. I ended up just killing the cache every time I have to touch the params. This is a good enough work around, but it is really not ideal. It is a big piece of tribal knowledge.

Maybe there could be some way to only cache once the path is chosen?

This is still an issue.

Contributor

adkron commented May 2, 2012

Impossible is quite a big word. I love when I hear another developer say something is impossible. Hard, maybe, but impossible, no.

It doesn't matter what parameters I touch. I ended up just killing the cache every time I have to touch the params. This is a good enough work around, but it is really not ideal. It is a big piece of tribal knowledge.

Maybe there could be some way to only cache once the path is chosen?

This is still an issue.

franckverrot added a commit to franckverrot/rails that referenced this issue May 2, 2012

@franckverrot

This comment has been minimized.

Show comment
Hide comment
@franckverrot

franckverrot May 2, 2012

Contributor

@pixeltrix @adkron would something like this be an acceptable solution: https://github.com/cesario/rails/compare/2510 ?

I "simplified" the constraints check and used tap to chain the reset of the parameters.

Contributor

franckverrot commented May 2, 2012

@pixeltrix @adkron would something like this be an acceptable solution: https://github.com/cesario/rails/compare/2510 ?

I "simplified" the constraints check and used tap to chain the reset of the parameters.

andhapp pushed a commit to andhapp/rails that referenced this issue May 2, 2012

Reset the request parameters after a constraints check
A callable object passed as a constraint for a route may access the request
parameters as part of its check. This causes the combined parameters hash
to be cached in the environment hash. If the constraint fails then any subsequent
access of the request parameters will be against that stale hash.

To fix this we delete the cache after every call to `matches?`. This may have a
negative performance impact if the contraint wraps a large number of routes as the
parameters hash is built by merging GET, POST and path parameters.

Fixes #2510.
@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron May 3, 2012

Contributor

@cesario 👍 I think that is a pretty good solution. If you add some tests for that I would totally go for it. @pixeltrix, does that meet the best of both worlds. Only reset the env when a constraint doesn't match?

@cesario I actually had some routing specs trying to drive out this very issue. I can see if I can dig them up and then we can get together on it.

Contributor

adkron commented May 3, 2012

@cesario 👍 I think that is a pretty good solution. If you add some tests for that I would totally go for it. @pixeltrix, does that meet the best of both worlds. Only reset the env when a constraint doesn't match?

@cesario I actually had some routing specs trying to drive out this very issue. I can see if I can dig them up and then we can get together on it.

carlosantoniodasilva pushed a commit to carlosantoniodasilva/rails that referenced this issue May 3, 2012

Reset the request parameters after a constraints check
A callable object passed as a constraint for a route may access the request
parameters as part of its check. This causes the combined parameters hash
to be cached in the environment hash. If the constraint fails then any subsequent
access of the request parameters will be against that stale hash.

To fix this we delete the cache after every call to `matches?`. This may have a
negative performance impact if the contraint wraps a large number of routes as the
parameters hash is built by merging GET, POST and path parameters.

Fixes #2510.
(cherry picked from commit 5603050)
@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 3, 2012

Member

@adkron I the end I just added an ensure section to the matches? method which resets the parameters.

Member

pixeltrix commented May 3, 2012

@adkron I the end I just added an ensure section to the matches? method which resets the parameters.

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron May 5, 2012

Contributor

Would it be better to only reset when a constraint fails?

Contributor

adkron commented May 5, 2012

Would it be better to only reset when a constraint fails?

@pixeltrix

This comment has been minimized.

Show comment
Hide comment
@pixeltrix

pixeltrix May 7, 2012

Member

@adkron the Constraints instance wraps the Dispatcher instance (or whatever is the target of the route). It may still return a 404 with a X-Cascade: pass header. The router will continue matching so if the constraints matched and the cache was not deleted from the environment hash then they will be stale if another route is matched.

Member

pixeltrix commented May 7, 2012

@adkron the Constraints instance wraps the Dispatcher instance (or whatever is the target of the route). It may still return a 404 with a X-Cascade: pass header. The router will continue matching so if the constraints matched and the cache was not deleted from the environment hash then they will be stale if another route is matched.

@adkron

This comment has been minimized.

Show comment
Hide comment
@adkron

adkron May 9, 2012

Contributor

Thanks for the fix

Contributor

adkron commented May 9, 2012

Thanks for the fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment