rails / journey

A router for rails
221 stars 57 forks source link

There should be an option to preserve trailing slashes #17

Closed y8 closed 7 years ago

y8 commented 12 years ago

In few corner cases, application may depend on trailing slashes. For example, if we want to implement "folder"-like HTTP interface, to display folder/file info when request is "GET /folder" and folder contents with "GET /folder/".

Right now trailing slashes removed by Utils.normalize_path, but it's not conform RFC 3986, where trailing slash actually matters.

tenderlove commented 12 years ago

I think you're right, but I think adding this feature will not be trivial. I'd love to get rid of the call to normalize_path all together, if possible.

One question though, when you're writing out your routes.rb file, and you have something like resources :folder (just as an example), would it match both the GET /folder and GET /folder/? Or would you have multiple declarations for each, or?

y8 commented 12 years ago

Right now I'm using ugly workaround with constrains (req.env["REQUEST_PATH"].match(/[A-z_\-0-9]*\/$/)), and this is a pain in the ass, since REQUEST_PATH not mocked in tests, and it's requires patching testing extensions (sessions and friends). So, even if you ask to sign Justin Bibers song a-capella, it will be better, than current solution. :D

Seriously, I see this like that:

# Just works. 
match "/folders/:id/" => "folders#index"
match "/folders/:id" => "folders#show"

# To preserve backwards compatibility: 
constraints(:trailing_slash => true) do
    # Now we taking slash in account and 
    # we even can capture trailing slash in :id
    resources :folder
end

# Nothing changed, just like it is now
resources :file
pixeltrix commented 12 years ago

I like the basic premise of @y8's idea but we need to be careful of the fact that your average user entering urls aren't going to be know the difference between a url with a trailing slash and one without, especially since most app/frameworks don't differentiate between the two (e.g. Rails will serve the same content at the two urls, as will WordPress but the latter redirects to the canonical url). Given this, it's a change that should only be made in Rails 4.0 and should be explicitly opt-in.

So for basic routes like match "/folders/:id" I'd see it matching either with or without a trailing slash. You could then override this either/or match using a higher precedence match "/folders/:id/". Also as part of this I think it would be useful to have a :redirect_canonical option that defaults to false that'd do a 301 redirect to the url without the slash as at the moment depending on how the link is written search engines will see it as two different pages.

match "/folders/:id" => "folders#show"
# matches /folders/1 or /folders/1/

match "/folders/:id" => "folders#show", :redirect_canonical => true
# matches /folders/1 or /folders/1/ and redirects the latter to /folders/1

match "/folders" => "folders#index"
# matches /folders or /folders/

match "/folders" => "folders#index", :redirect_canonical => true
# matches /folders or /folders/ and redirects the former to /folders/
# as the action name is index - other action names redirect to
# urls without the slash.

match "/folders" => "folders#action", :redirect_canonical => true, :format => false
# matches /folders or /folders/ and redirects the former 
# to /folders/ as the route has no format.

match "/folders/:id/" => "folders#index"
# matches /folders/1/ to folders#index with :id => 1
# a :redirect_canonical => true is ignored for paths that end in a slash
# also sets :format => false automatically

match "/folders/:id" => "folders#show"
# /folders/1/ is matched by the previous route so only matches /folders/1
# a :redirect_canonical => true would effectively disable this route but 
# could be useful if there should be no show action, however this would be 
# better achieved using a match "/folders/:id" => redirect("/folders/%{id}/")

match "/folders/*path" => "folders#show"
# matches /folders/path/to/folder or /folders/path/to/folder/

match "/folders/*path" => "folders#show", :redirect_canonical => true
# matches /folders/path/to/folder or /folders/path/to/folder/ and redirects the latter
# to /folders/path/to/folder as it has an optional format

match "/folders/*path" => "folders#index", :redirect_canonical => true
# matches /folders/path/to/folder or /folders/path/to/folder/ and redirects the former
# to /folders/path/to/folder/ as the action name is index - also sets :format => false 

match "/folders/*path" => "folders#action", :redirect_canonical => true, :format => false
# matches /folders/path/to/folder or /folders/path/to/folder/ and redirects the format
# to /folders/path/to/folder/ as it has no format format

match "/folders/*path/" => "folders#index"
# matches /folders/1/ to folders#index with :id => 1
# a :redirect_canonical => true is ignored for paths that end in a slash
# also sets :format => false automatically

For resources rather than :trailing_slash I think a better option would be to add a concept of hierarchical resources, which is what essentially what we have here - this would be particularly use for category structures. So given the following resource:

resources :categories, :hierarchical => true, :redirect_canonical => true

it would generate the following routes:

get '/categories/' => 'categories#index', :as => :categories
get '/categories' => redirect("/categories/")
get '/categories/*id/edit' => 'categories#edit', :as => :edit_category
get '/categories/*id/' => 'categories#index', :as => :category_index
get '/categories/*id' => 'categories#show', :as => :category
post '/categories/' => 'categories#create'
post '/categories' => 'categories#create'
post '/categories/*id/' => 'categories#create'
patch '/categories/*id' => 'categories#update'
put '/categories/*id', => 'categories#update'
delete '/categories/*id' => 'categories#destroy'
delete '/categories/*id/' => 'categories#destroy'

If this seems all too much magic then we could just have a :strict option that matches on the exact path specified.

tenderlove commented 12 years ago

I have a few questions! :-)

@pixeltrix In your example, you're differentiating trailing slash redirects based on the destination action. Why is this? It seems like adding special cases like that will add more complexity. Is this a feature that is really necessary? I'd rather all paths are treated the same way.

I like the idea of sending 301 redirects when people access with a slash, but does this need to be done on a per route basis? It seems like something we should turn on and off per app.

To follow the wordpress metaphor, Wordpress (the app) defines slash handling semantics, PHP does not. Maybe this should be an app level switch that defaults to on?

pixeltrix commented 12 years ago

In your example, you're differentiating trailing slash redirects based on the destination action. Why is this? It seems like adding special cases like that will add more complexity. Is this a feature that is really necessary? I'd rather all paths are treated the same way.

If you read RFC 2518 (WebDAV) section 5.2 on collection resources - basically the convention is that collections should end in a slash and if requested without a slash then redirect to the url with a slash:

There is a standing convention that when a collection is referred to by its name 
without a trailing slash, the trailing slash is automatically appended. Due to this, 
a resource may accept a URI without a trailing "/" to point to a collection. In this 
case it should return a content-location header in the response pointing to the 
URI ending with the "/". For example, if a client invokes a method on 
http://foo.bar/blah (no trailing slash), the resource http://foo.bar/blah/ (trailing slash) 
may respond as if the operation were invoked on it, and should return a 
content-location header with http://foo.bar/blah/ in it. In general clients should 
use the "/" form of collection names.

We could just do if for resource collection routes and not normal routes which map to index.

I like the idea of sending 301 redirects when people access with a slash, but does this need to be done on a per route basis? It seems like something we should turn on and off per app.

The issue with doing it on an app level is that you wouldn't be able to cover @y8's use case. However I think we can do it on an per route basis without actually creating a whole bunch of redirect routes so performance impact should be negligible.

To follow the wordpress metaphor, Wordpress (the app) defines slash handling semantics, PHP does not. Maybe this should be an app level switch that defaults to on?

I think WordPress sees itself as more of a framework now with individual themes being the apps. However there should be a app level config which gets added to the default url options.

tenderlove commented 12 years ago

@pixeltrix Gotcha. The webdav spec makes sense. As long as we push the special cases up to Rails, I think this is fine.

mapper.rb. :'(

schneems commented 12 years ago

@y8, @tenderlove, @pixeltrix. This issue has been inactive for some time. I don't see anything in the changelog that would have fixed this behavior. Seems like a user of Rails should have the ability to turn on this trailing slash functionality. Is this issue still under active consideration, or is the implementation discussion been taken offline?

pixeltrix commented 12 years ago

@schneems I have half a implementation in a local branch so I'm still working on it - hopefully should have a couple of weeks free in August to work on it.

Nowaker commented 8 years ago

@pixeltrix Did anything around trailing slashes land in Rails?

zachlatta commented 8 years ago

Yeah, any updates on this? I'm one of the corner cases with an application that breaks if I can't tell the difference between the two.

pixeltrix commented 7 years ago

Closing because the work I'd done is completely out of date