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

avoid executing filters even if prefix matched with other namespace #1253

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

namusyaka
Copy link
Member

Fixes #1251

@mwpastore @zzak Could you confirm this?

@namusyaka namusyaka force-pushed the fix-1251 branch 2 times, most recently from 31d796a to f458b71 Compare February 9, 2017 14:36
@namusyaka namusyaka changed the title avoids executing filters even if prefix matched with other namespace avoid executing filters even if prefix matched with other namespace Feb 9, 2017
@zzak
Copy link
Member

zzak commented Mar 4, 2017

Thank you @namusyaka, I'm happy to merge this but will wait for @mwpastore's seal of approval ;)

@zzak zzak added this to the 2.0.0 milestone Mar 4, 2017
@namusyaka
Copy link
Member Author

Any progress on this?

@zzak zzak requested a review from mwpastore March 18, 2017 23:55
@namusyaka
Copy link
Member Author

@mwpastore Could you check about this?

@mwpastore
Copy link
Member

Hi @namusyaka, I pinged you on the Slack about this, but I guess you didn't see it. I'm getting hung up on this because I don't really understand the solution. Can you help me understand the why and how? That being said, my review is superfluous if it solves the problem.

@namusyaka
Copy link
Member Author

namusyaka commented Mar 24, 2017

@mwpastore @zzak Sorry for the late reply.
I will explain the issue.

The sinatra namespace is prefixed to verb and filter methods (see https://github.com/sinatra/sinatra/blob/master/sinatra-contrib/lib/sinatra/namespace.rb#L232) by using original methods of indivisual namespaces.
The prefix rule is defined in this line, but the filter is a special one using the constant regexp.
Root of your issue is here.

Please take a look at these lines.
The before filter is registered with the regexp and the prefix for the namespace.
prefixed_path will make the concat pattern which is something like:

require 'mustermann'

# These vars are prefix of namespace.
foo     = Mustermann.new('/foo')
foo_bar = Mustermann.new('/foo-bar')

# current pattern
regexp = Mustermann.new(/.*/)

p (foo_bar + regexp) === '/foo-bar' #=> true
p (foo + regexp) === '/foo-bar'     #=> true (but expected to return false)

##############################################

# my patch
regexp = Mustermann.new(%r{(?:/.*)?})

p (foo_bar + regexp) === '/foo-bar' #=> true
p (foo + regexp) === '/foo-bar'     #=> false

Under the above principle, even filter which does not expect to operate originally has been executed on the current pattern.
Thus, your issue has been occured by calling twice extend method.

@namusyaka
Copy link
Member Author

So my patch makes it avoid the unexpected matching with filter. I guess this is reasonable. Thoughts?

@zzak
Copy link
Member

zzak commented Mar 27, 2017

Because this patch affects namespace, which was changed in 2.0 to support Mustermann, we are fine to merge it.

Thank you @namusyaka for your patch and explanation!

@zzak zzak merged commit 69a7b8f into sinatra:master Mar 27, 2017
@namusyaka namusyaka deleted the fix-1251 branch March 28, 2017 00:15
@mwpastore
Copy link
Member

Thank you for the patch and the explanation, @namusyaka, and thank you both for your patience! 🙇

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

Successfully merging this pull request may close these issues.

3 participants