RE: Params in Before filter

[Matt Todd]

I recently was "troubled" with upgrading an older Sinatra app to a
newer version that doesn't include route params in filters. I'd known
about this "bug" but hadn't taken the time to address it until now.

I found this old conversation that addressed my question: http://bit.ly/fOFwMC
(Linking because I don't know if Google will relate these
conversations since I'm not replying directly.)

However, it doesn't answer if this SHOULD be the case or if it's just
an unintended consequence of when the routing happens in relation to
the filters.

Obviously, having `pass` essentially say "this route is incorrect"
makes this a little less trivial since it's unclear as to whether we
should rerun the filters or not if we did perform routing before the
filters. I am still not sure it makes more sense to duplicate routing
patterns and explicit param extraction in the filtering, though.

Is there an openly discussed opinion or decision somewhere that says
that this is how it should be?

Wrote a quick failing spec to make sure I wasn't crazy:
https://gist.github.com/764340

If anything perhaps we should add a spec that says "route params are
NOT available in filters"? Make it part of the spec?

Cheers,
Matt

[Konstantin Haase]

It took me some time to understand what you think the issue is.

The main issue is the concept behind Sinatra routing. You do not have one true params hash but different versions of that hash depending on the handler you're in.

This is how I see Sinatra routing: Prepare request, run a couple of handlers, generate response. Each handler has its own params hash, access to all helper methods and can set the response with methods like body or halt. There are special handlers that can also set the response with its return value (route handlers, error handlers), but otherwise handlers all have the same options available and only are different in when they are triggered. An incoming request is "falling" through this handlers, similar to a petri net, and triggered handlers have tools available to change the course the request takes (pass, modifying the request, etc). This is unique to Sinatra and no other Rack router I know of works that way, not even the Padrino router.

Therefore path patterns, like conditions, are criteria for triggering the specific handlers, passing those upfront would not only not fit this basic principal, but would also be a major performance impact. All incoming requests would have to be matched against all route handlers, the routing best case would be worse than the current worst case, since we would also have to check if the request patch changed after each handler and would have to reroute again. We would have to completely change the way we handle params, and all that without a use case. Or at least I'm not aware of one.

Konstantin

> --
> You received this message because you are subscribed to the Google Groups "sinatrarb" group.
> To post to this group, send email to sina...@googlegroups.com.
> To unsubscribe from this group, send email to sinatrarb+...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/sinatrarb?hl=en.
>

[Tanner Burson]

I'm with you, I'm not sure I completely understand the use case, but I thought I'd look into it, if for no reason than to get more familiar with the Sinatra router. 

My initial thought was that it should be possible to run the filters without requiring extra processing, with one caveat, before filters wouldn't run unless a route was matched. This would be achieved by moving the filter! :before call, until after we had matched a route, and compiled the params. But when I looked into the actual routing code and followed the execution path, it became obvious that this would be a pretty significant change to the structure of the routing code.

Currently the entire Sinatra dispatch process can be traced about like this:

* Check to ensure the requested path isn't a static resource. 

(https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L761)

* Run before filters. 

(https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L762) 

* Run the routing code. Which consists of looping through the routes array looking for a match

(https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L626)

* We have a matching route, NOW we have a list of route specific params. 

(https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L656)

* Execute the matched route. 

(https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L645)

* Run any after filters 

(https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L769)

From there, it's pretty obvious that the requested change would require a major change to the flow of Sinatra's dispatch, without a well defined use case, and with either a heavy performance penalty (As Konstantin mentioned), or other trade-offs that we don't know the impact of.  

--Tanner

--
===Tanner Burson===
tanner...@gmail.com
http://www.tannerburson.com

[Matt Todd]

Thanks for the response, this definitely provides yet more context to the difficulty of making a feature like this feasible.

My use case is pretty simple:

* given an API

* if a request comes in that fails to meet criteria (auth, params, etc)

* respond with error

Queries can be for multiple formats (JSON and XML, primarily) and I want to respect these formats, so I take the format requested and respond with the error message or the results of the API call if the filter doesn't take any action.

This had been working without issue until more recent versions of Sinatra (note: the API this applies to has been around for over a year and I've not been able to update to the latest and greatest version of Sinatra until recently, so keep that in mind when I say "recent"). Specifically, routing params were available in filters before my attempt to upgrade resulted in failing specs due to this change.

Given I think this is a valid use case, I don't think it necessarily means we should change the way requests are processed to make it work again. The complexity is a big concern and of course performance is one thing I don't want to make worse just to make this little issue only slightly easier.

I still feel like there may be a middle ground we're ignoring though (because I'm both lazy and don't want to duplicate what are already in my nice little routes and handled so simply (for me) there :)

Matt

Matt Todd
Highgroove Studios
www.highgroove.com
cell: 404-314-2612
blog: maraby.org

Scout - Web Monitoring and Reporting Software
www.scoutapp.com

[Konstantin Haase]

I don't really see atm where you need to access the params, can you give some code to illustrate that (I could imagine different places)? Does your code work with 1.0?

Konstantin

[Matt Todd]

It does work with 1.0.

Here's a quick example:

https://gist.github.com/768156

Since upgrading to the latest, I now look for the format query param or parse the path directly for the format if it's not set.

The idea is that the before filter will render an error in the correct format when it decides that the noun can't be found without a query.

This certainly could be moved within the actual endpoint itself which would solve the issue... and that may be what we should be doing anyhow.

Matt

[Konstantin Haase]

You might want to look into https://github.com/mynyml/rack-abstract-format.
This would also allow using accept headers rather than file
extensions.

> > > On Tue, Jan 4, 2011 at 10:20 AM, Tanner Burson <tanner.bur...@gmail.com>

> > > On Tue, Jan 4, 2011 at 6:26 AM, Konstantin Haase <k.ha...@finn.de>

> > sinatrarb+...@googlegroups.com<sinatrarb%2Bunsu...@googlegroups.com>
> > .

> > > > For more options, visit this group at
> >http://groups.google.com/group/sinatrarb?hl=en.
>
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > "sinatrarb" group.
> > > To post to this group, send email to sina...@googlegroups.com.
> > > To unsubscribe from this group, send email to

> > sinatrarb+...@googlegroups.com<sinatrarb%2Bunsu...@googlegroups.com>
> > .

> > > For more options, visit this group at
> >http://groups.google.com/group/sinatrarb?hl=en.
>
> > > --
> > > ===Tanner Burson===

> > > tanner.bur...@gmail.com

> > >http://www.tannerburson.com
>
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > "sinatrarb" group.
> > > To post to this group, send email to sina...@googlegroups.com.
> > > To unsubscribe from this group, send email to

> > sinatrarb+...@googlegroups.com<sinatrarb%2Bunsu...@googlegroups.com>
> > .

> > > For more options, visit this group at
> >http://groups.google.com/group/sinatrarb?hl=en.
>
> > > --
> > > Matt Todd
> > > Highgroove Studios
> > >www.highgroove.com
> > > cell: 404-314-2612
> > > blog: maraby.org
>
> > > Scout - Web Monitoring and Reporting Software
> > >www.scoutapp.com
>
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > "sinatrarb" group.
> > > To post to this group, send email to sina...@googlegroups.com.
> > > To unsubscribe from this group, send email to

> > sinatrarb+...@googlegroups.com<sinatrarb%2Bunsu...@googlegroups.com>
> > .

> > > For more options, visit this group at
> >http://groups.google.com/group/sinatrarb?hl=en.
>
> > --
> > You received this message because you are subscribed to the Google Groups
> > "sinatrarb" group.
> > To post to this group, send email to sina...@googlegroups.com.
> > To unsubscribe from this group, send email to

> > sinatrarb+...@googlegroups.com<sinatrarb%2Bunsu...@googlegroups.com>
> > .