[RESEARCH] Optimisation code for named_routes

I'm seeing 15-20x speedup. It'd be nice to allow hash params also, so long as every param is provided. Commit!

Assuming the code that gets generated is sound, and doesn't break anything, there are a few other cases I can add:

• customer_url(@customer, :some=>params) # => /customers/1?some=params
• customer_url(:id=>@customer) # => /customers/1
• customer_url(:id=>@customer, :some=>params) # => /customers/1?some=params

The hash cases will have a slightly more complicated 'guard condition', but will still be faster. We'll also have to check that we don't make the other cases too much slower, but I'm guessing that testing the guard conditions is going to be a tiny piece of overhead relative to the cost of url_for.

As stated on sunday I believe this patch broke one odd case in an integration test, all my other tests (in other applications, too) run fine. I am looking at the problem now, it hardly seems like a serious issue. And I'm all for improvements like these. +1

Here's the code and the reason why it failed:

def test_should_create_cookies_after_login_and_delete_cookies_after_logout
    assert_difference('Session.count', 1) do
      post sessions_url, :username => 'manfred', :password => 'secret'
      assert_response :redirect
    end

    ...

When sessions_url gets called, it triggers line 1209 in the patched routing.rb:

elements << '#{request.protocol}'

Apparently request hasn't been set yet at that time, leading to a typical "unexpected nil" error.

This is not a problem with this patch, I just shouldn't be using named routes in places like these. :)

The new version of this patch should address the errors in integration tests, as you could previously get foos_url without an @request, I've disabled the optimisation code for integration tests.

Works great for me. +1

nzkoz : Could you please upload your benchmark script as well ?

Thanks.

The benchmark script is simply:

    n = 50000
    Benchmark.bm do |x|
      x.report("_url") { n.times {post_url(1)}}
      x.report("_path") { n.times {post_path(1)}}
    end

Run through curl and results sucked out of the logs.

Another version of the patch

I've extended the patch to also support:

• customer_url(@customer, :some=>params) # => /customers/1?some=params

This caused a few issues with the assertions in resources test, but my application appears to be working as expected.

Assuming there are no regressions, I intend to commit this in the next few days.

(In [7421]) Optimise named route generation when using positional arguments. Closes #9450 [Koz]

This change delivers significant performance benefits for the most common usage scenarios for modern rails applications by avoiding the costly trip through url_for. Initial benchmarks indicate this is between 6 and 20 times as fast.

I'm having problems with this change in a Unit Test for ActionMailer. Tests fail with "undefined local variable or method 'request'..."

Reverting to r7420 resolves the problem. Here is a snippet from my code:

class UserNotify < ActionMailer::Base

  def signup(options)
    default_url_options[:host] = "myhost.com"
    @body["url"] = verify_account_url(:token => sender.token)
  end

The unit test itself is not shown -it's pretty simple.

The problem seems to be that named routes can't be used in ActionMailer for Unit Tests. I think the optimization needs to be disabled for Unit Tests as well.

Fix for cch1's problem

cch1, could you try the patch I just uploaded? It disables the optimisation for all tests from test_help, which is usually included somewhere at the top of test_helper.

I've applied your patch (named_route_optimisation_unit_test_fix.diff) successfully to r7440 and it does not fix the problem. Same error.

-Chris

I thought this was only a problem in the tests, but apparently more people had this problem.

Generally, if you need to use absolute URLs from models it's usually best to use the :host parameter for _url helpers. You can either set it in some constant or, as I would recommend, dispatch the mail from your controller instead and pass the host as an argument.

I thought using the default_url_options[:host] = "constant" approach was equivalent to using the :host parameter in the _url helpers -and DRYer. Am I wrong?

FWIW, I also tried something like this: verify_account_url(:host => "fred.com", ...) and still got the same error.

Even if you do use the :host option in _url methods, you get something like <"undefined local variable or method `request' for #<UserNotifier:0x279e738>">

There are a couple of cases where it is beneficial to use ActionMailer outside of the context of a controller i.e. tests and models (sticking to fat model, skinny controller philosophy). It feels dirty do have to pass in a url as one of the deliver_xyz arguments rather than using named routed directly in ActionMailer.

As mentioned in http://dev.rubyonrails.org/ticket/9533, it is only _url methods causing the 'request' error, _path methods work fine. Couldn't we make ActionMailer::Base.default_url_options[:host] and ActionMailer::Base.default_url_options[:protocol] get used in preference to request.protocol and request.host_and_port?

Sorry cch1, you beat m to it.

It's probably worth simply disabling the optimisation code for cases where you include UrlWriter (such as action mailer).

I'll look into this in the next few days.

Anyone care to contribute a failing test case?

Below is a simple test case:

class AnyClass
  include ActionController::UrlWriter
  
  default_url_options[:protocol]  = 'http'
  default_url_options[:host]      = 'example.com'
  default_url_options[:port]      = 9999
end

def test_should_use_default_url_options_of_subject_class
  with_routing do |set|
    set.draw { |map| map.resources :shiny_products }
    
    assert_equal 'http://example.com:9999/shiny_products',        shiny_products_url
    assert_equal 'http://example.com:9999/shiny_products/1',      shiny_product_url(1)
    assert_equal 'http://example.com:9999/shiny_products/2/edit', edit_shiny_product_url(2)
  end
end

Note that a @controller instance is purposefully not available.

Sorry, amended below:

class AnyClass
  include ActionController::UrlWriter
  
  default_url_options[:protocol]  = 'http'
  default_url_options[:host]      = 'example.com'
  default_url_options[:port]      = 9999
end

def test_should_use_default_url_options_of_subject_class
  with_routing do |set|
    set.draw { |map| map.resources :shiny_products }
    
    instance = AnyClass.new

    assert_equal 'http://example.com:9999/shiny_products',        instance.shiny_products_url
    assert_equal 'http://example.com:9999/shiny_products/1',      instance.shiny_product_url(1)
    assert_equal 'http://example.com:9999/shiny_products/2/edit', instance.edit_shiny_product_url(2)
  end
end

Any progress likely soon here? I'm dying with these <"undefined local variable or method `request' for #<Whatever:0x279e738>"> errors. I'm going to have to bail out of edge from this unless we can get a patch or a workaround soon.

nzkoz, you mentioned that I could disable the optimisation code. How? I tried ActionController::Routing.optimise_named_routes = false in several places and it doesn't seem to have any effect.

-Chris

cch1, did [7501] fix this for you?

Disable named route optimization code of r7421

I've added a patch to completely disable the functionality of r7421. Use it until such time as the named route optimization can be safely applied. I've found that my unit and functional work again. Particularly useful when used in conjunction with the resources_controller plugin.

Whoa! Just as I upload my patch to remove the optimization, I see r7501. I'll give it a try and report back.

Good News: 7501 seems to have fixed my problems with named route optimization. Incidental Bad News: 7500 has introduced another bug!

For this ticket, I am happy. Pending some suitable time for the others to review, let's close it.

I still get "undefined local variable or method `request'" when running tests for my ActionMailers.

Replying to notahat:

I still get "undefined local variable or method `request'" when running tests for my ActionMailers.

Could you provide an example of what's failing and what it did before?

(In [7572]) Disable the routing optimisation code when dealing with foo_url helpers. Add test to actionmailer to expose the problem they introduced. References #9450 [Koz]

[7572] fixes the breakage in actionmailer, and I'll investigate a proper fix after the preview release is out.

Yep, this fixes the ActionMailer test problems I had.

This seems to have fixed the _url helpers but broken _path helpers. They now complain that a "undefined local variable or method `request'". I don't understand why _path methods need to know anything about requests?

I think what you're seeing is [7605] coming into play

Please take a look at this issue with ActionMailer:

ActionView::TemplateError (You have a nil object when you didn't expect it! The error occurred while evaluating nil.protocol) on line #2 of app/views/notifier/signup_notification.erb: 1: Hello, <%= @account.login %>! 2: <%= formatted_activate_account_url(@account, 'html', :key => @account.activation_key.key) %>

Seems to be something affected by latest changesets.

I found that issue with 'nil.protocol' caused by http://dev.rubyonrails.org/changeset/7673 at line 47.

revert changes for aplicable?

[7676] should fix this stuff once and for all

It will only use the fast route if request is 'defined?' and set.

My ActionMailer is now throwing the same errors as seen earlier: undefined method 'request' for <MyActionMailerModel>. In my example, it is happening in a <%= link_to image_tag('logo.jpg') %>.

Are there residual problems in ActionMailer?

To expound on my previous message, I am creating an ActionMailer model within an ActiveRecord model. The AM call is dynamically generated [Model.send(dynamically_generated_create_method_name)].

This problem was not happening on 18 October.

Even more information: Changset [7970] revealed this problem, and I'm pretty sure it's in the image_tag call.

Can you put a solution up for AM that works as well as [7676] worked for other cases?

-Chris

We have tests in ActionMailer which ensure that this works.

Do you have a sample that's failing? defined?(request) && request should handle ... well anything ;)

NZKoz, The failure in my environment is a bit long, but here is the condensed version:

In my AR model (which is the only tricky part):

class Message < ActiveRecord::Base
...
  def initialize
    ...
    self.msg = UserNotify.send('create_' + template, options)
    ...
  end
...
end

And my AM model:

class UserNotify < ActionMailer::Base
  ...
  def documentation_request(options)
    ...
    default_url_options[:host] = 'www.myapp.com'
    ...
  end
  ...
end

And finally, in my AM template (documentation_request.text.html.rhtml):

  ...
            <%= link_to image_tag("logo.jpg"), "http://www.mywebsite.com" %>
  ...

The error (undefined method 'request') is throwing in asset_tag_helper:388 in compute_public_path, within the new 'path_to_image'.

Reverting to r7969 solves the problem and moving to r7970 introduces it. Hope this helps...if not, let me know if I can provide any other information. -Chris

OK, I can reproduce the error in the unit tests now and will look into it.

Replying to nzkoz: Excellent. Any idea of a workaround I can employ while waiting for a resolution?

You'll have to freeze to an earlier revision that doesn't exhibit the problems.

This is actually caused by the reference to @controller.request in compute_public_path.

#10014 can track this