Fix enum writers when using integers #13542

Merged
merged 2 commits into from Jan 1, 2014

Projects

None yet

5 participants

@robin850
Ruby on Rails member

Hello,

This is just a little pull request that resolves some issues mentioned in #13530. Basically, the problem is described in this gist. Previously, a call to:

Book.where(status: 0).first_or_initialize

Would fail (an ArgumentError would be raised) because only the symbol version of the enum was checked when calling its writer method. I guess that find_or_initialize is calling it through the initialize (new) method.

I also added a mention about the scopes generated dynamically based on the allowed values of the field in the documentation.

I'm not really comfortable with Active Record and its internals so I'm certainly not very clear ; therefore, sorry. Let me know if I should update anything.

By the way, the issue mentioned above is not totally resolved since the where method doesn't generate the same queries with either a symbol or a numeric value on enum fields.

Have a nice day.

@vipulnsward vipulnsward and 2 others commented on an outdated diff Dec 30, 2013
activerecord/lib/active_record/enum.rb
@@ -62,7 +67,7 @@ def enum(definitions)
_enum_methods_module.module_eval do
# def status=(value) self[:status] = STATUS[value] end
define_method("#{name}=") { |value|
- unless enum_values.has_key?(value)
+ if !enum_values.has_key?(value) && !enum_values.has_value?(value)
@vipulnsward
vipulnsward Dec 30, 2013

we can change this to unless (enum_values.has_key?(value) && enum_values.has_value?(value))
to avoid double negation.

@robin850
robin850 Dec 31, 2013

Sorry, I should revert it back, the other tests fail if we change this.

@chancancode
chancancode Jan 3, 2014

De Morgan's law is your friend :)

This would be !(enum_values.has_key?(value) || enum_values.has_value?(value)) but I actually prefer the version you have now anyway - much easy to read and hurts my brain less.

@chancancode
chancancode Jan 3, 2014

Nvm! That's basically the same thing @vipulnsward said :P (Every time I read unless x && y || z ... my head still hurts a little)

@vipulnsward vipulnsward commented on an outdated diff Dec 30, 2013
activerecord/test/cases/enum_test.rb
@@ -63,4 +63,15 @@ class EnumTest < ActiveRecord::TestCase
assert_equal 1, Book::STATUS["written"]
assert_equal 2, Book::STATUS[:published]
end
+
+ test "first_or_initialize with enums' scopes" do
+ # This is needed because the original issue append
@vipulnsward
vipulnsward Dec 30, 2013

don't think the comments are necessary.

@robin850
Ruby on Rails member

Thanks for your feedback @vipulnsward, updated!

robin850 added some commits Dec 30, 2013
@robin850 robin850 Fix the enums writer methods
Previously, the writer methods would simply check whether the passed
argument was the symbol representing the integer value of an enum field.
Therefore, it was not possible to specify the numeric value itself but
the dynamically defined scopes generate where clauses relying on this
kind of values so a chained call to a method like `find_or_initialize_by`
would trigger an `ArgumentError`.

Reference #13530
7aebcb6
@robin850 robin850 Improve enum documentation
Add a mention about the automatic generation of scopes based on the
allowed values of the field on the enum documentation.
50060e9
@rafaelfranca rafaelfranca merged commit 50060e9 into rails:master Jan 1, 2014

1 check failed

Details default The Travis CI build failed
@chancancode chancancode added a commit that referenced this pull request Jan 3, 2014
@chancancode chancancode Building new records with enum scopes now works as expected
Previously, this would give an `ArgumentError`:

   class Issue < ActiveRecord::Base
     enum :status, [:open, :finished]
   end

   Issue.open.build # => ArgumentError: '0' is not a valid status
   Issue.open.create # => ArgumentError: '0' is not a valid status

PR #13542 muted the error, but the issue remains. This commit fixes
the issue by allowing the enum value to be written directly via the
setter:

   Issue.new.status = 0 # This now sets status to :open

Assigning a value directly via the setter like this is not part of the
documented public API, so users should not rely on this behavior.

Closes #13530.
788bb40
@robin850 robin850 deleted the robin850:issue-13530 branch Jan 9, 2014
@senny senny added the enum label Sep 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment