Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

OrderedHash mishandles keys

#1676

The latest implementation of OrderedHash does not behave as expected when it comes to managing keys in several situations. For example, #merge! doesn't work:

>> h = ActiveSupport::OrderedHash.new
>> h.merge! :a => 1, :b => 2
=> {:a=>1, :b=>2}
>> h.keys
=> []
>> h.inspect
=> "{:a=>1, :b=>2}"

Also, when calling non-destructive methods like #reject it mangles the keys:

>> h = ActiveSupport::OrderedHash.new
>> h[:a] = 1
>> h[:b] = 2
>> h2 = h.reject {|k,v| k == :a }
=> {:b=>2}
>> h.inspect
=> "{:a=>1, :b=>2}"
>> h.keys
=> [:b]

Reported by Brandon Keepers · December 31st, 2008 @ 03:46 PM

State: resolved
Milestone: 2.x
Assigned to: nobody
Importance: none

Activity

  1. Brandon Keepers
    Brandon Keepers

    Attached is a patch that fixes these two issues, and probably a few more.

    December 31st, 2008 @ 03:49 PM

  2. Frederick Cheung
    Frederick Cheung
    • Tag changed from activesupport, hash to activesupport, hash, patch

    Is the dup in initialize_copy not redundant given that keys already does a dup? Other than that looks sane to me.

    December 31st, 2008 @ 04:01 PM

  3. Brandon Keepers
    Brandon Keepers

    Yeah, it is redundant, but I thought it more clearly communicated the intent. I'm ok with removing it, I just didn't want people to be confused about it's just setting the instance variable to the value of keys.

    December 31st, 2008 @ 05:10 PM

  4. Frederick Cheung
    Frederick Cheung

    I'd remove it. If anything (for me at least) it adds confusion (' I thought the return value of keys was safe to mess around with but it's dup-ing it here, should I always dup it myself ? ').

    There might be other bugs like this. When I rewrote OrderedHash a few weeks ago I was mostly worried about backwards compatibility with the existing implementation (when it was an array with knobs on) so I didn't worry to much about methods that didn't exist on the old OrderedHash.

    December 31st, 2008 @ 05:22 PM

  5. Brandon Keepers
    Brandon Keepers

    Updated patch that removes the unnecessary dup

    January 1st, 2009 @ 07:13 PM

  6. Repository
    Repository
    • State changed from new to resolved

    (from [452cd74d81ecfa38d0c7d3a4dc83d2b731de9e73]) Dup keys in OrderedHash to prevent them from being modified [#1676 OrderedHash mishandles keys state:resolved]

    Signed-off-by: Frederick Cheung frederick.cheung@gmail.com http://github.com/rails/rails/co...

    January 16th, 2009 @ 05:40 PM