Rails Ruby on Rails → 3.0.2

Comments and changes to this ticket

• You flagged this item as spam.
  

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

  +1

  I'm currently using symbolize_keys to skip this problem.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  Lenary April 16th, 2010 @ 11:35 PM

  • Tag set to “hashwithindifferentaccess, url_for”

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 April 21st, 2010 @ 01:40 AM

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

• 

  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 April 21st, 2010 @ 04:28 AM

  • Tag changed from “hashwithindifferentaccess, url_for” to “hashwithindifferentaccess, rails3, url_for”
• 

  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

  • url_for.patch 2.4 KB

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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&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...?

• You flagged this item as spam.
  

  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...

• You flagged this item as spam.
  

  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 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

  • url_for.patch 2.3 KB

• You flagged this item as spam.
  

  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 April 21st, 2010 @ 05:13 PM

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

  • url_for.patch 3.4 KB
• 

  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 April 21st, 2010 @ 05:26 PM

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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 April 22nd, 2010 @ 01:20 AM

  • State changed from “verified” to “committed”

  http://github.com/rails/rails/commit/5c9c30ac65d25bd5a83d773739c2cd...

• 

  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...

• You flagged this item as spam.
  

  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.

  • 0001-ensure-that-url_for-uses-symbolized-keys-in-the-cont.patch 2 KB
• 

  Repository April 22nd, 2010 @ 03:48 PM

  (from [275e839b8dfa3cf2bdedf1b31302dec20ac96a46]) Ensure that url_for uses symbolized keys in the controller. [#4391]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/275e839b8dfa3cf2bdedf1b31302de...

• 

  Santiago Pastorino April 22nd, 2010 @ 05:47 PM

  We have fixed this issue in another way now commits here http://github.com/rails/rails/compare/275e839...ebf9820

• 

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

  • Tag cleared.

  Automatic cleanup of spam.

• 

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

  • Milestone set to “3.0.2”

  [bulk edit]