Rack::URLMap should match on paths that include a file extension #554

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@stve
stve commented May 8, 2013

Requests to paths that included a file extension were not properly routing from
a URLMap. For example, the following URLMap:

Rack::URLMap.new('/widgets' => Widgets.new)

would fail to resolve 'GET /widgets.json', instead a 404 is returned. I wasn't 100% certain this behavior was correct as implemented. It would seem to make sense that file extensions should omitted when resolving the mapping.

@rkh
Member
rkh commented May 8, 2013

Hmmm... this will make it match /widgets.jpg/foo, too...

@stve
stve commented May 8, 2013

@rkh that doesn't appear to be the case. The regex uses \w which equates to [a-zA-Z0-9_]. In your example, the trailing slash after .jpg does not match and this would pass through to the next mapping.

I'm happy to add some more test cases for scenarios such as the one you've mentioned.

@rkh
Member
rkh commented May 8, 2013

Ah, right, gotcha. Can you add more tests, including tests for request.path, which is probably screwed up by this?

@stve
stve commented May 8, 2013

@rkh you were correct, this change was improperly mutating PATH_INFO. I've fixed that and added more tests with varying permutations. Any suggestions on how to test request.path? Since the tests use MockRequest I don't see an easy way to test that.

@stve stve Rack::URLMap should match on paths that include a file extension
Requests to paths that included a file extension were not properly routing from
a URLMap. For example, the following URLMap:

Rack::URLMap.new('/widgets' => Widgets.new)

would fail to resolve 'GET /widgets.json'
a096aa8
@stve
stve commented May 10, 2013

@rkh I took another look at Rack::Request#path and it essentially combines ENV['PATH_INFO'] and ENV['SCRIPT_NAME'] which are asserted in the URLMap tests. That being the case, I'm not sure that an explicit test for Rack::Request#path is necessary.

Do you think I've captured all test conditions with the additional tests that were added? I've squashed the commits in case you think this is ready to merge.

@rkh
Member
rkh commented May 11, 2013

That's what I was getting at: It should be possible to use PATH_INFO and SCRIPT_NAME to reconstruct the actual path that was called. I see that you modified the patch to actually make that possible. I'm not sure if .jpg should actually be in PATH_INFO or SCRIPT_NAME.

@chneukirchen
Member

SCRIPT_NAME is the part until you know which Rack app handles it, and PATH_INFO is the rest.

@rkh
Member
rkh commented May 12, 2013

I know. But should the app actually get a request with '.jpg' as path info?

@chneukirchen
Member

I would expect the separation to happen at a /.

@rkh
Member
rkh commented May 12, 2013

Given this PR gets accepted, there is no / to separate by.

@stve
stve commented May 14, 2013

My thoughts are that Rack::URLMap should not modify the request that gets sent to an application but it should resolve the mapping irrespective of whether a path includes an extension or not. This does mean however, that there are cases where the PATH_INFO does not include a /:

res = Rack::MockRequest.new(map).get('/foo.json')
res.should.be.ok
res["X-ScriptName"].should.equal "/foo"
res["X-PathInfo"].should.equal ".json"

It seems to make sense that it should be the responsibility of an app to handle the path appropriately and shouldn't be a concern of the URLMap.

@raggi
Member
raggi commented Jul 12, 2014

This change is a breaking change when a user has:

map '/foo' {}
map '/foo.json' {}

As for dynamic apps and resources having JSON representations, it's more correct to use the accept headers not path extensions. If a resource name is actually ending in .json, that should be in the map. If the resource is 'foo' and it happens to have a JSON representation, that should be fetched with a request that accepts json with the highest quality. I understand if you feel this is principled, but it is HTTP. This is adding a lot of runtime behavior and conflicting semantics with a major API change, to service implementations that are questionable from an HTTP standpoint - I'd need further persuasion.

I might consider a utility that makes it easy for users to build URLMaps with extensions, given a list of acceptable mimes, but again, this really would just encourage bad behavior and add code for maintenance. Users can build this quite easily themselves:

entities = {
  "foo" => Foo.new,
  "bar" => Bar.new,
  "baz" => Baz.new
}
mimes = %w[text/html application/json]
exts = mimes.map { |m| Rack::Mime::MIME_TYPES.find_all { |k,v| v == m }.map(&:first) }.flatten + [""]
entities.map { |path,app| exts.map { |ext| { path + ext => app } } }.flatten.inject(:merge)

Thoughts?

@raggi raggi added this to the Rack 1.6 milestone Jul 12, 2014
@stve
stve commented Aug 5, 2014

@raggi Since proposing this, we abandoned extensions in the project this grew out of in favor of an Accept header. I totally agree that the URL is not the correct place for such things. Thanks for consideration and feedback.

@stve stve closed this Aug 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment