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

HttpRouter::Route#significant_variable_names fails with Regexp routes #1205

Closed
Freaky opened this Issue Mar 29, 2013 · 6 comments

Comments

Projects
None yet
4 participants
@Freaky

Freaky commented Mar 29, 2013

Any Regexp based routes blow up with this:

ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:91:in `significant_variable_names': undefined method `scan' for #<Regexp:0x00000805a45af8> (NoMethodError)
        from ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:643:in `block in route'
        from ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:642:in `delete_if'
        from ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:642:in `route'
        from ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:535:in `get'
        from /home/freaky/projects/web/freshbsd-ng/app/controllers/search.rb:71:in `block in <top (required)>'
        from ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:311:in `instance_eval'
        from ruby-2.0.0-p0/gems/padrino-core-0.11.0/lib/padrino-core/application/routing.rb:311:in `controller'

The fix looks simple enough -- Freaky@fff442a

This also allows further validation of named captures, and removes the ugly use of the ternary operator. Will try to sort out test case over the weekend.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 29, 2013

Contributor

Thanks for reporting this @Freaky! :) @DAddYE?

Contributor

dariocravero commented Mar 29, 2013

Thanks for reporting this @Freaky! :) @DAddYE?

@shipstar

This comment has been minimized.

Show comment
Hide comment
@shipstar

shipstar Apr 3, 2013

Any way I can help out here? I'd love to help shepherd this into master and stop running off of a fork.

shipstar commented Apr 3, 2013

Any way I can help out here? I'd love to help shepherd this into master and stop running off of a fork.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 3, 2013

Member

Can you submit a pull request (with tests) to help us fix this issue for the next release?

Member

nesquena commented Apr 3, 2013

Can you submit a pull request (with tests) to help us fix this issue for the next release?

@shipstar

This comment has been minimized.

Show comment
Hide comment
@shipstar

shipstar Apr 5, 2013

I'm having a bit of trouble recreating the behavior in a test. My issue (hopefully the same as freaky's -- the section of stack trace he copied is the same) isn't specifically with routing, but rather starting a server with an app mounted via regex.

I'm mounting an app with

Padrino.mount("proxy").host(/.*\.lvh\.me/)

If I put a raise after that line, I see it, so it's not crashing on the mount. However, when it actually goes to setup the server, that's where I'm seeing the above error. This error is not occurring in 0.10.6.

Do any of the tests actually call Server#run!? Is there a convenient way for me to recreate the subset of the behavior I need?

I'll keep poking around!

UPDATE: Oops, it is the route. Back to poking!

UPDATE 2: Actually, looks like it's the combination of the two for me. Regexp mount + Regexp route = explosion.

shipstar commented Apr 5, 2013

I'm having a bit of trouble recreating the behavior in a test. My issue (hopefully the same as freaky's -- the section of stack trace he copied is the same) isn't specifically with routing, but rather starting a server with an app mounted via regex.

I'm mounting an app with

Padrino.mount("proxy").host(/.*\.lvh\.me/)

If I put a raise after that line, I see it, so it's not crashing on the mount. However, when it actually goes to setup the server, that's where I'm seeing the above error. This error is not occurring in 0.10.6.

Do any of the tests actually call Server#run!? Is there a convenient way for me to recreate the subset of the behavior I need?

I'll keep poking around!

UPDATE: Oops, it is the route. Back to poking!

UPDATE 2: Actually, looks like it's the combination of the two for me. Regexp mount + Regexp route = explosion.

@shipstar

This comment has been minimized.

Show comment
Hide comment
@shipstar

shipstar Apr 5, 2013

I found a way to recreate the condition, although it's a little indirect. It triggered for me specifically because I was using a Regexp + the :provides option.

My commit is simply @Freaky's fix with @dariocravero's suggestions.

Let me know if I can help with anything else.

shipstar commented Apr 5, 2013

I found a way to recreate the condition, although it's a little indirect. It triggered for me specifically because I was using a Regexp + the :provides option.

My commit is simply @Freaky's fix with @dariocravero's suggestions.

Let me know if I can help with anything else.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 6, 2013

Member

Can you send this as a pull request making it easier to merge, thanks!

Member

nesquena commented Apr 6, 2013

Can you send this as a pull request making it easier to merge, thanks!

@skade skade closed this in 9b048b1 Apr 6, 2013

skade added a commit that referenced this issue Apr 6, 2013

Merge pull request #1230 from shipstar/master
Fix #1205 -- parsing issue when using :provides with a Regexp route.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment