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

Projects
None yet
3 participants
@namusyaka
Member

namusyaka commented Feb 9, 2017

Fixes #1251

@mwpastore @zzak Could you confirm this?

@namusyaka namusyaka changed the title from avoids executing filters even if prefix matched with other namespace to avoid executing filters even if prefix matched with other namespace Feb 9, 2017

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Mar 4, 2017

Member

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

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

@zzak zzak added the sinatra-contrib label Mar 4, 2017

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Mar 18, 2017

Member

Any progress on this?

Member

namusyaka commented Mar 18, 2017

Any progress on this?

@zzak zzak requested a review from mwpastore Mar 18, 2017

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Mar 24, 2017

Member

@mwpastore Could you check about this?

Member

namusyaka commented Mar 24, 2017

@mwpastore Could you check about this?

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Mar 24, 2017

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.

Member

mwpastore commented Mar 24, 2017

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

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Mar 24, 2017

Member

@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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Mar 25, 2017

Member

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

Member

namusyaka commented Mar 25, 2017

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

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Mar 27, 2017

Member

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!

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@namusyaka namusyaka deleted the namusyaka:fix-1251 branch Mar 28, 2017

@mwpastore

This comment has been minimized.

Show comment
Hide comment
@mwpastore

mwpastore Mar 28, 2017

Member

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

Member

mwpastore commented Mar 28, 2017

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