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

Fix exception with regexp routes and trailing slashes #2177

Merged
merged 1 commit into from Apr 27, 2018

Conversation

Projects
None yet
3 participants
@pdlug
Copy link
Contributor

pdlug commented Feb 21, 2018

Fixes #2175 where defining a route with a regex that does not include a trailing slash results in an exception when accessed with a trailing slash. I'm not convinced this is the right behavior, IMO if the regex is not defined with a trailing slash it should 404 but it is consistent with the existing behavior of the other route definition types which are agnostic to trailing slashes. Fix was simply to ignore the check if the route being matched is an instance of Mustermann::Sinatra before determining whether to strip the trailing slash in the match.

Fix exception when a route is defined as a regex that does not includ…
…e an optional trailing slash but accessed with a trailing slash. The trailing slash is now ignored consistent with the behavior of routes defined via strings or names.
@pdlug

This comment has been minimized.

Copy link
Contributor Author

pdlug commented Feb 22, 2018

Please note that the CI here is failing due to #2174 not this commit/PR.

@ujifgc

This comment has been minimized.

Copy link
Member

ujifgc commented Feb 23, 2018

I don't understand how and why the patch works. Is it some kind of regression?

@papile

This comment has been minimized.

Copy link
Contributor

papile commented Apr 27, 2018

Full disclosure I work at the same place as the author of this PR, I just hit this again when trying to quiet unnecessary exceptions. The real question here is what is the mustermann? logic doing? I can't seem to tell from the routing tests.

If you look at the logic for match when given the test case @pdlug sent, mustermann? is false so that logic added in after the ampersands is not going to fire, and then the trailing slash case is not handled.

The code that calls match in params_for has no provision for that returning nil which then calls a chained method on nil. Perhaps the mustermann? was put in there because this was only an issue when the handler was Mustermann? The handler in this exact case is of class Regexp. I'm not sure if it was once Mustermann::Sinatra or what would cause it to reach this with handler Mustermann::Sinatra I think regardless of what happens match should have an else or some other short circuit not to return nil ever.

@ujifgc ujifgc merged commit 6ed325b into padrino:master Apr 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment