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

Do not create a hash key when calling ActiveModel::Errors#include? #24299

Merged
merged 1 commit into from Mar 25, 2016

Conversation

Projects
None yet
9 participants
@kitop
Contributor

kitop commented Mar 24, 2016

Summary

From: #24279

When doing record.errors.include? :foo, it adds a new key to the
@messages hash that defaults to an empty array.

This happens because of a combination of these 2 commits:
b97035d
(Added in Rails 4.1)
and
6ec8ba1#diff-fdcf8b65b5fb954372c6fe1ddf284c78R76
(Rails 5.0)

By adding the default proc that returns an array for non-existing keys,
ruby adds that key to the hash.

This commit changes #include? to check with has_key? and then check if that value is
present?.

@rails-bot

This comment has been minimized.

rails-bot commented Mar 24, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Mar 24, 2016

@kitop Can you also add a test case?

@kitop

This comment has been minimized.

Contributor

kitop commented Mar 24, 2016

@prathamesh-sonpatki will do, thanks!

@kitop

This comment has been minimized.

Contributor

kitop commented Mar 24, 2016

@prathamesh-sonpatki test case added. More than happy to hear feedback :)

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Mar 24, 2016

@kitop Please squash the commits.

@kitop kitop force-pushed the kitop:activemodel-errors-include-fix branch Mar 24, 2016

@kitop

This comment has been minimized.

Contributor

kitop commented Mar 24, 2016

@vipulnsward Done!

@prathamesh-sonpatki

View changes

activemodel/test/cases/errors_test.rb Outdated
person = Person.new
person.errors.include?(:foo)
assert_equal [], person.errors.messages.keys

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 24, 2016

Member

Can you use has_key? and assert for only :foo.

This comment has been minimized.

@kitop

kitop Mar 24, 2016

Contributor

Done. Let me know if that looks OK now.

@kitop kitop force-pushed the kitop:activemodel-errors-include-fix branch Mar 24, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Mar 24, 2016

Looks good. cc @rafaelfranca

@vipulnsward

View changes

activemodel/test/cases/errors_test.rb Outdated
person = Person.new
person.errors.include?(:foo)
assert !person.errors.messages.has_key?(:foo)

This comment has been minimized.

@vipulnsward

vipulnsward Mar 24, 2016

Member

assert_not person.errors.messages.has_key?(:foo)

This comment has been minimized.

@kitop

kitop Mar 24, 2016

Contributor

Updated. Thanks for the feedback!

@kitop kitop force-pushed the kitop:activemodel-errors-include-fix branch 3 times, most recently Mar 24, 2016

@Sen-Zhang

View changes

activemodel/lib/active_model/errors.rb Outdated
@@ -110,7 +110,7 @@ def clear
# person.errors.include?(:name) # => true
# person.errors.include?(:age) # => false
def include?(attribute)
messages[attribute].present?
messages.has_key?(attribute) && messages[attribute].present?

This comment has been minimized.

@Sen-Zhang

Sen-Zhang Mar 24, 2016

Contributor

Is has_key? deprecated? How about just using key??

This comment has been minimized.

@kitop

kitop Mar 24, 2016

Contributor

You're absolutely right! Updating now.

Do not create a hash key when calling ActiveModel::Errors#include?
From: #24279

Problem:
By doing `record.errors.include? :foo`, it adds a new key to the
@messages hash that defaults to an empty array.

This happens because of a combination of these 2 commits:
b97035d
(Added in Rails 4.1)
and
6ec8ba1#diff-fdcf8b65b5fb954372c6fe1ddf284c78R76
(Rails 5.0)

By adding the default proc that returns an array for non-existing keys,
ruby adds that key to the hash.

Solution:
Change `#include?` to check with `has_key?` and then check if that value is
`present?`.

Add test case for ActiveModels::Errors#include?

@kitop kitop force-pushed the kitop:activemodel-errors-include-fix branch to 9848c46 Mar 24, 2016

@kaspth kaspth merged commit 044baef into rails:master Mar 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tute

This comment has been minimized.

Contributor

tute commented Mar 25, 2016

👏 thank you all!

@meinac

This comment has been minimized.

Contributor

meinac commented Mar 25, 2016

I'm really happy to see this is fixed(partially) now! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment