This project is archived and is in readonly mode.

#4391 ✓committed
jay

url_for doesn't like HashWithIndifferentAccess

Reported by jay | April 14th, 2010 @ 06:38 AM | in 3.0.2

url_for doesn't seem to like ActiveSupport::HashWithIndifferentAccess. When used in a view via, say, link_to, the URL produced doesn't seem to be correct, or at least what you'd expect. This is noticeable when trying to replicate the old overwrite_params functionality by using the params Hash and Hash#merge, as params is a HWIA.

For instance, given a set of params where :controller is 'hello' and :action is 'world', then using Hash#merge to overwrite these like so:

url_for(params.merge(:controller => 'foo', :action => 'bar'))

You end up with a URL that looks like

http://example.com/hello/world?action=bar&controller=foo

whereas you'd probably be expecting

http://example.com/foo/bar

The following however produces the desired results:

url_for(params.merge(:controller => 'foo', :action => 'bar').to_hash.symbolize_keys)

Stringifying the keys does not produce the desired effect, however.

The attached patch appears to work and doesn't break any existing tests in ActionPack, but being as I'm not really familiar with the Rails 3 code yet compared to 2.x I hope that someone could run with it and create a proper patch, assuming that this is indeed a bug...

Comments and changes to this ticket

  • Francesc Esplugas

    Francesc Esplugas April 16th, 2010 @ 08:21 AM

    +1

    I'm currently using symbolize_keys to skip this problem.

  • Lenary

    Lenary April 16th, 2010 @ 10:47 PM

    Protip: you're going to need some tests to show exactly what it fixes.

    But other than that, it does indeed work and fix a few other bugs that people have been experiencing - like the following: https://rails.lighthouseapp.com/projects/8994/tickets/4419-current_...

    Add a few test cases and then it'll get accepted.

  • Lenary

    Lenary April 16th, 2010 @ 11:35 PM

    • Tag set to hashwithindifferentaccess, url_for
  • Lenary

    Lenary April 17th, 2010 @ 12:12 AM

    I'll take more of a look at this tomorrow, and can guide you a little if you want it. Sorry for being condescending earlier.

  • jay

    jay April 21st, 2010 @ 12:26 AM

    No prob. If there's any tests that need writing I can give it a shot... I just figured that this patch, although working, probably wasn't the best way of doing things as it seems to duplicate code by calling doing to_hash.symbolize_keys in two different places. I was just kind of hoping that someone more familiar with the code would know of a better fix, but if this patch is acceptable then all the better. I can write a few tests if it will help, although I don't really know how extensive the coverage should be if this patch affects other tickets.

  • Ryan Bigg

    Ryan Bigg April 21st, 2010 @ 01:40 AM

    Lenary, did you take a look at this ticket yet? What did you think of it?

  • Neeraj Singh

    Neeraj Singh April 21st, 2010 @ 04:11 AM

    It was not clear to me if this problem is only in rails3 branch or in 2-3-stable branch too. It turns out that 2-3-stable branch works only. This is an issue only with rails3 branch. Adding this comment so that it to clear to others who look at this ticket.

    Someone should put right tag. There are so many of them related to rails3 that I am not sure which one I should apply.

  • Ryan Bigg

    Ryan Bigg April 21st, 2010 @ 04:28 AM

    • Tag changed from hashwithindifferentaccess, url_for to hashwithindifferentaccess, rails3, url_for
  • Santiago Pastorino

    Santiago Pastorino April 21st, 2010 @ 05:35 AM

    • Milestone cleared.
    • Tag changed from hashwithindifferentaccess, rails3, url_for to hashwithindifferentaccess, patch, url_for
    • State changed from “new” to “verified”
    • Assigned user set to “Carl Lerche”

    Patch with tests

  • Lenary

    Lenary April 21st, 2010 @ 06:41 AM

    This patch still doesn't feel right IMHO. using hashwithindifferent access should mean that we don't get these issues and don't have to call to_hash.symbolize_keys on the hash to get this to work. I'm going to have a much closer look later today

    Can we not apply this patch until then. This to me kind-of feels like were barking up the wrong tree, but I need time to look at it properly.

  • jay

    jay April 21st, 2010 @ 07:13 AM

    • Assigned user changed from “Carl Lerche” to “Cezary Baginski”

    A couple of things I've noticed:

    • Using Strings as options keys to url_for seems to be hit and miss. For instance, if I put the following in a view:
    <%= url_for('controller' => 'c', 'action' => 'a' %>
    

    Produces '/c/a' as expected. However, doing the same thing in a controller produces http://example.com/?action=a&amp;controller=c.

    • The following results in an ActionController::RoutingError exception when done in an irb console:
    irb(main):001:0> app.url_for('controller' => 'c', 'action' => 'a')
    ActionController::RoutingError: No route matches {"action"=>"a", "controller"=>"c"}
    

    This while using a very simple routes.rb set up that should produce a valid route:

    TestApp::Application.routes.draw do
      match ':controller(/:action(/:id(.:format)))' => '#index'
    end
    

    The same route works for the example in the view, however.

    • In all cases, using Symbols instead of Strings for keys produces the expected results.

    • HashWithIndifferentAccess appears to always output its keys as Strings, i.e. when using the hash as an Enumerable or calling #keys or whatever.

    So it appears the problem isn't necessarily with HWIA, but rather with using Strings as keys versus Symbols, at least in some cases...?

  • jay

    jay April 21st, 2010 @ 07:13 AM

    • Assigned user changed from “Cezary Baginski” to “Carl Lerche”

    Oh my, sorry for reassigning the person responsible, that was a slip of the mouse apparently...

  • Lenary

    Lenary April 21st, 2010 @ 07:21 AM

    Jay, thanks for all the legwork. I'll take a proper look over what's happening and propose a few solutions in this thread later today.

  • Santiago Pastorino

    Santiago Pastorino April 21st, 2010 @ 03:59 PM

    I think this is a better way to solve it.
    symbolize_keys on hash with indifferent access for me should return a hash with symbolized keys.

    Deeper on the framework things doesn't expect a HWIA so we need to convert on the helper and that's what symbolize_keys should do

  • jay

    jay April 21st, 2010 @ 04:18 PM

    Yeah, this appears to be the behaviour in 2.3. In 2.3, calling symbolize_keys or stringify_keys returns an actual Hash with the appropriate keys, whereas in 3.0 they return a HWIA:

    # in 2.3:
    >> HashWithIndifferentAccess.new({ :one => :two }).stringify_keys.class
    => Hash
    >> HashWithIndifferentAccess.new({ :one => :two }).symbolize_keys.class
    => Hash
    
    # in 3:
    irb(main):005:0> HashWithIndifferentAccess.new({ :one => :two }).stringify_keys.class
    => ActiveSupport::HashWithIndifferentAccess
    irb(main):006:0> HashWithIndifferentAccess.new({ :one => :two }).symbolize_keys.class
    => ActiveSupport::HashWithIndifferentAccess
    

    That being the case and if symbolize_keys is to return a Hash with symbolized keys, then I suppose stringify_keys should also return a Hash with stringified keys, yeah?

  • Santiago Pastorino

    Santiago Pastorino April 21st, 2010 @ 05:13 PM

    I've added a test for the symbolize_keys method and re upload the patch.
    Now looks good.

  • Santiago Pastorino

    Santiago Pastorino April 21st, 2010 @ 05:26 PM

    For me HashWithIndifferentAccess.new({ :one => :two }).stringify_keys.class should return ActiveSupport::HashWithIndifferentAccess
    but we need to do the same on both versions i think.

  • José Valim

    José Valim April 21st, 2010 @ 05:26 PM

    • Assigned user changed from “Carl Lerche” to “José Valim”
  • Lenary

    Lenary April 21st, 2010 @ 08:25 PM

    I am just about to take a good look at this now. Sorry i haven't got around to it earlier.

  • Lenary

    Lenary April 21st, 2010 @ 10:54 PM

    I am no clearer on what is going on with this. I took a look at some raw output from url_for, and got some of the way with that, but then got to where you had all got to.

    I have now tried santiago's patch, and it hasn't really worked for me (still getting controller and action params in querystring when in controller). is the latest one ( https://rails.lighthouseapp.com/projects/8994/tickets/4391/a/491493... ) the entire patch, or do i have to apply more than that?

  • Santiago Pastorino
  • Repository

    Repository April 22nd, 2010 @ 01:54 AM

    (from [5c9c30ac65d25bd5a83d773739c2cd8fdd6da4d3]) url_for now works with HashWithIndifferentAccess ht jay [#4391 state:committed]

    Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
    http://github.com/rails/rails/commit/5c9c30ac65d25bd5a83d773739c2cd...

  • jay

    jay April 22nd, 2010 @ 02:46 AM

    The applied patch in http://github.com/rails/rails/commit/5c9c30ac65d25bd5a83d773739c2cd... is partially there, but url_for still fails with stringified keys in a controller.

    I think that ActionDispatch::Routing::Generator#normalize_options! and #normalize_controller_action_id! are be the culprits. They both assume symbolized keys when they do their options merging, and end up creating options Hashes that sort of duplicate their entries because of it. In the cases where HWIAs are used this merging obviously goes a lot smoother, but in the case of regular Hashes with stringified keys things start to go awry.

    For instance, in the controller given following:

    url_for('controller' => 'c', 'action' => 'a')
    

    This will pass a Hash with stringified keys to ActionDispatch::Routing::RouteSet.url_for. When normalize_options! gets a hold of it, it won't do much of anything because it specifically looks for symbolized keys and we aren't dealing with a HWIA at all.

    In my set up, I'm calling url_for from MainController#index. normalize_controller_action_id! tries to combine the original options Hash with @recall, which is now set to {:controller => 'main', :action => 'index'}, thus using symbolized keys. The use_recall_for method uses Symbols to try and fill in any missing parameters, but it ends up duplicating the parameters since we're not using a HWIA. The options Hash now looks like the following:

    {:controller=>"main", "action"=>"a", :action=>"index", "controller"=>"c"}
    

    This later causes all sorts of problems when other methods like handle_nil_action! try to delete entries from the options Hash because while they may delete :action they'll be leaving behind 'action'. Eventually these duplicate keys seem to cause problems later on down the line in rack-mount and the like.

    Part of the reason url_for is working with both stringified keys in views is because the keys are being symbolized long before they ever make it to normalize_options! and the like, whereas in ActionDispatch they are still being untouched, and thus you could slip HWIAs or Hashes with stringified keys in there. My original assumption that only HWIAs were causing the problem then is a little off -- it's actually more of a case of stringified keys.

    Anyways, that's what I've managed to figure out. So, that being the case, another quick patch seems to be needed -- the controller version of url_for needs to make sure that it's passing along a symbolized Hash.

    I've attached a patch with a couple of tests and it seems to be working now for me. Note that in this case I don't believe we'd need to call to_hash as we're actually calling symbolize_keys on url_options after the merging, and url_options is already a Hash.

  • Repository
  • Santiago Pastorino
  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 09:54 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages