Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0.3/4-0-stable] ActiveSupport::HashWithIndifferentAccess regression with Ruby 2.1.1 #14188

Closed
khustochka opened this issue Feb 25, 2014 · 15 comments
Labels

Comments

@khustochka
Copy link
Contributor

@khustochka khustochka commented Feb 25, 2014

HashWithIndifferentAccess does not preserve class after applying reject (and maybe other Hash methods).

Please compare:

[1] pry(main)> RUBY_VERSION
=> "2.1.0"
[2] pry(main)> require 'active_support/hash_with_indifferent_access'
=> true
[3] pry(main)> h = ActiveSupport::HashWithIndifferentAccess.new({a: 1})
=> {"a"=>1}
[4] pry(main)> x = h.reject {|_, v| v.nil? }
=> {"a"=>1}
[5] pry(main)> x.class
=> ActiveSupport::HashWithIndifferentAccess


[1] pry(main)> RUBY_VERSION
=> "2.1.1"
[2] pry(main)> require 'active_support/hash_with_indifferent_access'
=> true
[3] pry(main)> h = ActiveSupport::HashWithIndifferentAccess.new({a: 1})
=> {"a"=>1}
[4] pry(main)> x = h.reject {|_, v| v.nil? }
=> {"a"=>1}
[5] pry(main)> x.class
=> Hash

Even if it is a Ruby bug, it may cause much pain when trying to switch Rails apps to 2.1.1.

@stephankaag
Copy link

@stephankaag stephankaag commented Feb 25, 2014

irb(main):001:0> RUBY_VERSION
=> "2.1.1"
irb(main):002:0> require 'active_support/hash_with_indifferent_access'
=> true
irb(main):003:0> h = ActiveSupport::HashWithIndifferentAccess.new({a: 1})
=> {"a"=>1}
irb(main):004:0> x = h.reject {|_, v| v.nil? }
=> {"a"=>1}
irb(main):005:0> x.class
=> ActiveSupport::HashWithIndifferentAccess
@khustochka
Copy link
Contributor Author

@khustochka khustochka commented Feb 25, 2014

@stephankaag thanks for testing.

But it still fails for me. Both in irb and pry, I tried ruby 2.1.1 compiled with ruby-build and installed via rvm, on two machines with Ubuntu 12.04 and 14.04 (beta).

@khustochka
Copy link
Contributor Author

@khustochka khustochka commented Feb 25, 2014

@pftg This is neither ActiveRecord nor ActionController issue.

But I did some investigation:


So, on master and on 4-1-stable

in activesupport/test/core_ext/hash_ext_test.rb there is a test test_indifferent_reject

and it passes with Ruby 2.1.1


on tag v4.0.3 and branch 4-0-stable

there is no such test case! But if I add it manually, it fails with Ruby 2.1.1 and passes with Ruby 2.1.0.

the test case is below. Context is defined in activesupport/test/core_ext/hash_ext_test.rb

  def test_indifferent_reject
    hash = ActiveSupport::HashWithIndifferentAccess.new(@strings).reject {|k,v| v != 1}

    assert_equal({ 'a' => 1 }, hash)
    assert_instance_of ActiveSupport::HashWithIndifferentAccess, hash
  end

reject test cases were added in 921ec9b

@marutosi
Copy link
Contributor

@marutosi marutosi commented Feb 27, 2014

I hit this issue on Rails 3.2.17.
Redmine tests pass on Ruby < 2.1.1, but fail on Ruby 2.1.1.
https://travis-ci.org/marutosi/redmine/builds/19696007

On Ruby 2.1.1:

$ ruby --version
ruby 2.1.1p76 (2014-02-24 revision 45161) [x86_64-linux]
$ ruby script/rails console
Loading development environment (Rails 3.2.17)
2.1.1 :001 > h = ActiveSupport::HashWithIndifferentAccess.new({:a => 1})
 => {"a"=>1} 
2.1.1 :002 > x = h.reject {|_, v| v.nil? }
 => {"a"=>1} 
2.1.1 :003 > x.class
 => Hash 
2.1.1 :004 > x[:a]
 => nil 

On Ruby 1.9.3:

$ ruby --version
ruby 1.9.3p545 (2014-02-24 revision 45159) [x86_64-linux]
$ ruby script/rails console
Loading development environment (Rails 3.2.17)
1.9.3-p545 :001 > h = ActiveSupport::HashWithIndifferentAccess.new({:a => 1})
 => {"a"=>1} 
1.9.3-p545 :002 > x = h.reject {|_, v| v.nil? }
 => {"a"=>1} 
1.9.3-p545 :003 > x.class
 => ActiveSupport::HashWithIndifferentAccess 
1.9.3-p545 :004 > x[:a]
 => 1 
@sorah
Copy link
Contributor

@sorah sorah commented Feb 28, 2014

Confirmed. This is not expected change.

I found that some commit fixing typo haven't backported to 2.1 branch; the typo makes this behavior change.
So I requested backport in bugs.r-l.o: https://bugs.ruby-lang.org/issues/9576

Sorry!

@marutosi
Copy link
Contributor

@marutosi marutosi commented Mar 2, 2014

I ran Redmine tests on ruby 2.1.2p80 and tests pass.

$ ruby --version
ruby 2.1.2p80 (2014-03-01 revision 45231) [x86_64-linux]
$ ruby script/rails console
Loading development environment (Rails 3.2.17)
irb(main):001:0> h = ActiveSupport::HashWithIndifferentAccess.new({:a => 1})
=> {"a"=>1}
irb(main):002:0> x = h.reject {|_, v| v.nil? }
=> {"a"=>1}
irb(main):003:0> x.class
=> ActiveSupport::HashWithIndifferentAccess
irb(main):004:0> x[:a]
=> 1
@sorah
Copy link
Contributor

@sorah sorah commented Mar 2, 2014

fix is backported to Ruby 2.1 branch: https://bugs.ruby-lang.org/issues/9576#change-45533

@arthurnn
Copy link
Member

@arthurnn arthurnn commented Mar 8, 2014

This has being fixed and merge to 4-0, 4-1, and master.
Thanks for reporting it.

@arthurnn arthurnn closed this Mar 8, 2014
@marutosi
Copy link
Contributor

@marutosi marutosi commented Mar 8, 2014

This has being fixed and merge to 4-0, 4-1, and master.

How about Rails 3.2.x?

@robin850
Copy link
Member

@robin850 robin850 commented Mar 8, 2014

@marutosi : We're sorry but 3-2-stable only receive security fixes.

@marutosi
Copy link
Contributor

@marutosi marutosi commented Mar 8, 2014

Due to #14198 (comment) , Ruby 2.2 will change behavior.
Rails 3-2-stable will break again.
Should Rails 3-2-stable backport this changes?

@sorah
Copy link
Contributor

@sorah sorah commented Mar 8, 2014

@marutosi as @robin850 said,this won't be backported to 3-2-stable because this is not security fix.

@marutosi
Copy link
Contributor

@marutosi marutosi commented Mar 8, 2014

Is this issue security fix?
Why were fc2641d and b57da83 backported to 4-0-stable in spite of not security fix?

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Mar 8, 2014

@marutosi please verify our policy on releases, bug fixes and security fixes.

Long story short, 3-2 does not get bug fixes anymore. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.