Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #10209 (closed defect: fixed)

Opened 10 months ago

Last modified 10 months ago

[PATCH] Routing optimizations break :host key

Reported by: bscofield Assigned to: nzkoz
Priority: normal Milestone: 2.0
Component: ActionPack Version: edge
Severity: normal Keywords: routing optimization rails2
Cc:

Description

The optimizations introduced in #9450 and committed in [7421] (and friends) break named routes when you're specifying :host. For example: 

map.foo 'foo' :controller => 'foo', :action => 'bar', :host => 'www.foo.bar'

foo_url #=> 'http://localhost:3000/foo'
foo_url(:a => 1) #=> 'http://www.foo.bar/foo?a=1

I'm attaching a patch that fixes the issue, but I'm not sure how to update the routing tests to prove the benefit - as far as I can tell, the optimized routes aren't being properly tested now, due to some of the changes introduced in response to problems people say with default_url_options and the like (check the comments in #9450)

Attachments

fix_host_bug_in_routing_optimization.patch (1.7 kB) - added by bscofield on 11/19/07 20:51:01.
pickier-optimisation.diff (1.8 kB) - added by nzkoz on 11/20/07 21:17:29.
Disable the optimisation if there are any additional arguments to the mapper.

Change History

11/19/07 18:24:39 changed by technoweenie

  • owner changed from core to nzkoz.

11/19/07 20:42:18 changed by nzkoz

  • keywords changed from routing optimization to routing optimization rails2.
  • status changed from new to assigned.
  • milestone changed from 2.x to 2.0.

Can I have a failing test case please?

11/19/07 20:51:01 changed by bscofield

  • attachment fix_host_bug_in_routing_optimization.patch added.

11/19/07 20:51:53 changed by bscofield

OK, here's one - I just realized that the LegacyRouteSet tests actually were using the optimized code. Sorry!

11/19/07 21:29:23 changed by nzkoz

That test fails even if the optimisation code is disabled? When did it pass?

11/19/07 21:45:32 changed by bscofield

OK, that's odd - when I originally tracked down the problem (in the ticket description), I could fix it by commenting out line 1205 in routing.rb - it's in the eval within define_url_helper, where generate_optimisation_block is called. From there, I was able to fix the issue with this patch to routing_optimisation, so I (incorrectly, it appears) assumed that the core problem was in the optimization code itself.

I'll try to track down the actual problem and post a new patch to this ticket tonight or tomorrow. My apologies for the confusion.

11/19/07 23:17:15 changed by nzkoz

No worries. My original fix was to disable the optimisation if the requirements were anything other than :controller and :action as there are probably other things which we won't include in the generated URLs.

11/20/07 03:41:57 changed by bscofield

Wow, this is a little more complicated than I originally anticipated :)

Turns out the test fails for a different reason when run with and without optimization. *With* optimization, it fails because of a substantive issue: the optimizer code drops any specified host key. *Without* optimization, it fails because of a testing-logisitics issue: url_for is redefined in the routing tests as: 

def url_for(options)
  only_path = options.delete(:only_path)
  path = routes.generate(options)
  only_path ? path : "http://named.route.test#{path}"
end

So *any* test for host and related keys is bound to fail in this test case, through no fault of the patch itself. The most frustrating thing is that I checked in the UrlRewriter tests where the host key is already tested, and everything already works there (without this patch). I'm at a loss for how to test this properly, so I'm going to sleep on it and take another look tomorrow.

11/20/07 18:34:46 changed by bscofield

OK, I'm still stumped. The mocking going on in the routing tests has broken me. I feel confident in the patch and the supplied test 1) fixing the problem when the optimization block is run, and 2) not introducing any new request.host (or related) issues. So my question is, since the failure of this test when run without optimization is a result of the mocking, and since the UrlRewriter tests show that the :host key works properly in that situation, do we actually need this test to pass in that case?

11/20/07 21:17:29 changed by nzkoz

  • attachment pickier-optimisation.diff added.

Disable the optimisation if there are any additional arguments to the mapper.

11/20/07 21:18:09 changed by nzkoz

Can you confirm that this patch fixes your issue?

I'll take your test and make a mocha based one out of it.

11/20/07 21:25:30 changed by nzkoz

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [8169]) Ensure that the routing optimisation code isn't used when additional arguments are passed to the named route. Closes #10209 [bscofield, Koz]

11/21/07 01:20:08 changed by bscofield

OK, looks like the latest patch fixed it for my case. Thanks!