This project is archived and is in readonly mode.

Do not show optional (.:format) block for wildcard route

#6605

Currently in Rails, when you're adding a wildcard route and run rake routes, you'll usually see an entry like this

routes.rb

match '/*path', :to => 'pages#show'

rake routes

/*path(.:format)   {:controller=>"pages", :action=>"show"}

However, if you test it in your browser by going to http://localhost:3000/foo/bar.json, the params[:path] would be equal to foo/bar.json instead of foo/bar. This bug exists because the regular expression in Rack::Mount is greedy and match the whole thing without left the :format part.

After some discussion with the core team, I believe that it's better to fix the mapper to not display the (.:format) block instead of fixing the Rack::Mount so no one will fall in the same hole as I did. In order to add a route which would match the format, you'd have to do this instead:

match '/:path(.:format)', :to => 'pages#show', :constraints => { :path => /.+?/ }

I'm attaching the patch for both master and 3-0-stable.

Reported by Prem Sichanugrist (sikachu) · March 22nd, 2011 @ 03:19 PM

Activity

Prem Sichanugrist (sikachu)
This is a patch for master.

I'm still working on the patch for 3-0-stable, as my patch is currently rely on these two commits: https://github.com/rails/rails/compare/53794cf...acd4bfb. I think those commits should be pull to 3-0-stable as well.

Would you guys give me any though on that? If you think that commit should be left there, I'll then cook a separate patch for 3-0-stable.
- hide_optional_format-master.patch 1.98 KB
March 22nd, 2011 @ 03:36 PM
Repository
- State changed from verified to resolved
(from [2ddfdba9a0dab7d8499c3ad0d13583bddbac4f69]) Do not show optional (.:format) block for wildcard route [#6605 Do not show optional (.:format) block for wildcard route state:resolved]

This will make the output of rake routes to be correctly match to the behavior of the application, as the regular expression used to match the path is greedy and won't capture the format part by default
https://github.com/rails/rails/commit/2ddfdba9a0dab7d8499c3ad0d1358...

March 22nd, 2011 @ 04:20 PM
Andrew White
- State changed from resolved to verified
A couple of things to note - firstly the glob param may have other elements before it and secondly you may have multiple glob params in a path. You'd be better off checking that the last segment in a path is a glob parameter, e.g:
```
  path.match(/\*[^\/]+$/)
```
I think the safe option for stable is to not add the format for glob paths, but I think we should fix master to add a non-greedy regexp if the path ends with a glob parameter.
March 22nd, 2011 @ 04:33 PM
Repository
- State changed from verified to resolved
(from [2ef6270f8fbbefba8d4f10504497e198d8e7deea]) Merge branch 'master' into fuuu
- master: Do not show optional (.:format) block for wildcard route [#6605 Do not show optional (.:format) block for wildcard route state:resolved] pushing id insertion and prefetch primary keys down to Relation#insert use prepared statements to fetch the last insert id escaping binary data encoding when inserting to sqlite3. Thanks Naruse! [#6559 ActiveRecord fails with both non ASCII string and binary on SQLite3Adapter state:resolved] schemas set by set_table_name are respected by the mysql adapter. [#5322 Rails3 RC - AREL does not work with multiple schemas using MySQL state:resolved] Reapply extensions when using except and only SJIS is an alias to Windows-31J in ruby trunk. Use SHIFT_JIS for this test Improved resolver docs a bit [action_view] docs for FileSystemResolver [action_view] added custom patterns to template resolver https://github.com/rails/rails/commit/2ef6270f8fbbefba8d4f10504497e...
March 22nd, 2011 @ 05:11 PM
Prem Sichanugrist (sikachu)
- State changed from resolved to verified
@Andrew White: but then the RegExp actually has to be fixed in Rack::Mount, right?

March 23rd, 2011 @ 01:13 AM
Andrew White
There's a simple fix required in Rack::Mount - it ignores custom conditions on glob params. Once that is fixed we can use the regexp above to detect whether the last segment is a glob param and then add a non-greedy regexp condition for that param if one isn't specified. That way all of the responds_to magic should work.

March 23rd, 2011 @ 10:33 AM
Prem Sichanugrist (sikachu)
If it's a simple fix, then I'm totally agree with you. Are you going to fix that in Rack::Mount for me, or do you want me to send pull request in?

Anyhow, here's what I'm going to do:
1. Backport the patch to 3-0-stable so people would notice it as a bugfix. With appropriate CHANGELOG in it.
2. Waiting for the release of new version of Rack::Mount
3. Update the patch on master to not filter out the (.:format) part, and add the CHANGELOG to say that the bug on not detecting format route is fixed on Rack::Mount.
Do you think that looks ok?
March 23rd, 2011 @ 11:04 AM
Prem Sichanugrist (sikachu)
By the way, I'm backing of on pulling those two Aaron's patch back, as I don't think user would expect that on a stable branch. I don't think I want to break someone else's application

(I'm also adding Aaron to the ticket now so he know what's my plan of world domination ..)

March 23rd, 2011 @ 11:30 AM
Andrew White
Okay, I've sent a pull request to Josh: https://github.com/josh/rack-mount/pull/15

Since the :format parameter is Rails-specific we need to specify the non-greedy regexp in the route we pass to Rack::Mount. We need to bump the version of rack-mount to the new release on master and then add a CHANGELOG entry to say that glob parameters at the end of a route are non-greedy by default and if you want the extension included in the parameter then pass :format => false in the route options hash.

For stable I'd not change anything and if someone updates to the newer version of rack-mount they can add the non-greedy regexp and it will work as expected.

March 23rd, 2011 @ 12:12 PM
Repository
- State changed from verified to resolved
(from [51551a0a5bd6f7e4116bc3759a4d7c15634643dc]) Update the wildcard route to be non-greedy by default, therefore be able to match the (.:format) segment [#6605 Do not show optional (.:format) block for wildcard route state:resolved]

After some discussion with Andrew White, it seems like this is a better approach for handling a wildcard route. However, user can still bring back the old behavior by supplying :format => false to the route.

Signed-off-by: Andrew White andyw@pixeltrix.co.uk
https://github.com/rails/rails/commit/51551a0a5bd6f7e4116bc3759a4d7...

March 29th, 2011 @ 10:14 AM