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

active_support/indifferent_access: fix not raising when default_proc does #20828

Merged
merged 3 commits into from Jul 10, 2015

Conversation

@sirupsen
Copy link
Contributor

sirupsen commented Jul 10, 2015

Ran into this with our Rails application: HashWithDifferentAccess's behaviour is not consistent with Ruby when default_proc is set and cloned and when it raises.

h = HashWithIndifferentAccess.new
h.default_proc = proc { |h, k| raise "OMG" }
h.dup
# => RuntimeError: OMG
h = {}
h.default_proc = proc { |h, k| raise "OMG” }
h.dup
# => {}

This PR fixes #dup and #to_hash to set defaults safely when #default_proc raises, by avoiding to call into it.

\cc @byroot @arthurnn @rafaelfranca

@sirupsen sirupsen force-pushed the sirupsen:hash-indifferent-dup-default-proc branch 3 times, most recently from e6c9bde to 63b7872 Jul 10, 2015
@@ -275,6 +277,14 @@ def convert_value(value, options = {})
value
end
end

def set_defaults(target, source = self)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Jul 10, 2015

Member

What is the reason for the source argument?

This comment has been minimized.

Copy link
@sirupsen

sirupsen Jul 10, 2015

Author Contributor

I originally used this for the new_from_hash_copying_default as well, but realized later I didn't need it. I'll remove it.

@sirupsen sirupsen force-pushed the sirupsen:hash-indifferent-dup-default-proc branch 2 times, most recently from 6c2ecd6 to a0ce045 Jul 10, 2015
@rafaelfranca rafaelfranca reopened this Jul 10, 2015
@sirupsen sirupsen force-pushed the sirupsen:hash-indifferent-dup-default-proc branch from a0ce045 to 9578d57 Jul 10, 2015
@rafaelfranca rafaelfranca reopened this Jul 10, 2015
rafaelfranca added a commit that referenced this pull request Jul 10, 2015
…proc

active_support/indifferent_access: fix not raising when default_proc does
@rafaelfranca rafaelfranca merged commit ebf8961 into rails:master Jul 10, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jul 10, 2015

❤️ 💚 💙 💛 💜

rafaelfranca added a commit that referenced this pull request Jul 10, 2015
…proc

active_support/indifferent_access: fix not raising when default_proc does
@sirupsen

This comment has been minimized.

Copy link
Contributor Author

sirupsen commented Jul 10, 2015

Thanks @rafaelfranca!

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Jul 10, 2015

Backported in 9ce5a79

kamipo added a commit to kamipo/rails that referenced this pull request Dec 4, 2019
I digged the history for the internal utility methods (`convert_key`,
`convert_value`, and `set_defaults`), I believe it is apparently not
intended to appear them in the public API doc.

* `convert_key`, `convert_value`: 7174211
* `set_defaults`: rails#20828

https://api.rubyonrails.org/v6.0.1/classes/ActiveSupport/HashWithIndifferentAccess.html

Since we sometimes makes breaking change for internal methods (e.g.
rails#37870), so that methods should not be leaked to the public API doc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.