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

Add implicit optional format param to routes #91

Closed
wants to merge 4 commits into from

Conversation

Ryman
Copy link
Member

@Ryman Ryman commented Oct 4, 2014

BREAKING CHANGE

This will break code that tries to handle routes such as: '/foo/:bar.:baz'

You can now handle this with: '/foo/:bar'

When this route matches '/foo/cats.gif' then request.param("format") will be ".gif"

Fixed #90.

cc: @pzol @cburgdorf @simonpersson

Currently we're using ([,a-zA-Z0-9%_-]*) to capture variables in a route. Journey (the Rails router) uses ([^./?]+).
I suggest using a blacklist, or we'll continue to see more bugs in this area, e.g. '/foo/:bar' won't match '/foo/你好' which seems a shame.

Ryman added 3 commits October 4, 2014 05:34
BREAKING CHANGE

This will break code that tries to handle routes such as:
'/foo/:bar.:baz'

You can now handle this with:
'/foo/:bar'

When this route matches '/foo/cats.gif' then request.param("format")
will be ".gif"

Fixed nickel-org#90.
@cburgdorf
Copy link
Member

Thanks for jumping on it. Do you know how express.js handles it? Do you know other frameworks/routers that handle it this way? This would break routes such as /format/:format/:file, no? If I let that match against /format/plain/some.txt then request.param("format") will be txt instead of plain, right?

@Ryman
Copy link
Member Author

Ryman commented Oct 8, 2014

Yes, good catch, I've pushed a commit to address that, checking if :format already exists in the route, and if not add it.

Here's how rails handles it:
https://github.com/rails/rails/blob/6d86762fd8f83896a6932614e9ac9bdc42102805/actionpack/lib/action_dispatch/routing/mapper.rb#L133

It has a fair amount of configuration options, and they handle things like explicit trailing slashes, which I think we should discuss in another issue if we want to restrict that additionally.

Express on the other hand seems to only restrict '/' unless you have a dot prefix (for some kind of escaping functionality)
https://github.com/pillarjs/path-to-regexp/blob/master/index.js#L129

So, by default, rails forbids . / ? while express limits /.

I think it's worth pointing out that rails does split out :format so it can do interesting default handling for a number of things, see respond_with

@cburgdorf
Copy link
Member

Ok, seams reasonable. Merged it.

@Ryman
Copy link
Member Author

Ryman commented Mar 5, 2015

This was never actually merged? Will need a rebase.

@cburgdorf
Copy link
Member

Oh noooees, what did I do back then :'(

@Ryman
Copy link
Member Author

Ryman commented Mar 6, 2015

Just looked into the history on this one, it did merge and then encountered a bad rebase or something? It gets nuked out during a PR from ME 😞 #94, specifically this commit Ryman@f12ab9a.

I guess git was overzealous when a file got moved and didn't warn me about a merge conflict, still kind of shocking to not have noticed (and the tests got nuked so nothing failed out). So definitely not a forgotten merge anyway @cburgdorf!

So, I'm going to re-add the changes in a new PR since rebasing this doesn't do anything as it's already in master's history.

@Ryman Ryman closed this Mar 6, 2015
Ryman added a commit to Ryman/nickel.rs that referenced this pull request Mar 6, 2015
Ryman added a commit to Ryman/nickel.rs that referenced this pull request Mar 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Router should recognize routes containing "."
2 participants