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

URLMap: why any-domain-routes have more priority than domain-specific-routes? #504

Closed
runa opened this issue Jan 29, 2013 · 2 comments
Closed

Comments

@runa
Copy link

runa commented Jan 29, 2013

Heyas, we spent our afternoon trying to figure out if this is a bug or a feature.

We have 2 routes:

{
  "/" => app1,
  "http://example.com/x" => app2,
}

When requesting curl http://example.com/x the route for app1 matches (instead of the expected app2).

I think specific-domain-routes should be matched BEFORE any-domain routes. This could be easily fixed by setting INFINITY weight to any-domain routes instead of NEGATIVE_INFINITY (see https://github.com/rack/rack/blob/master/lib/rack/urlmap.rb#L15 ).

The problem, is that when we did that, (this test fails)[https://github.com/rack/rack/blob/master/test/spec_urlmap.rb#L113]. I couldn't understand why that test expects a response from default.org when requesting a URL http://foo.org/

So, the big question is: is this a bug or a feature?

There's a gist here displaying the behavior https://gist.github.com/4668565

Thanks a lot!

@raggi
Copy link
Member

raggi commented Feb 7, 2013

Interesting, I see your issue, but it's not related to the priorities. Currently, we don't handle Hosts that aren't coming in from SERVER_NAME or HTTP_HOST headers. As a consequence of not having supported this previously (this may be related to the CGI spec, I need to do some research), Rack::MockRequest drops the host information during env creation.

@raggi
Copy link
Member

raggi commented Feb 7, 2013

Fail. There are a few odd paths, but essentially, you're absolutely correct. Patch applied. Thank you!

@raggi raggi closed this as completed in 60c66b5 Feb 7, 2013
raggi added a commit that referenced this issue Feb 8, 2013
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

No branches or pull requests

2 participants