Permalink
Browse files

Refactor enum to be defined in terms of the attributes API

In addition to cleaning up the implementation, this allows type casting
behavior to be applied consistently everywhere. (#where for example). A
good example of this was the previous need for handling value to key
conversion in the setter, because the number had to be passed to `where`
directly. This is no longer required, since we can just pass the string
along to where. (It's left around for backwards compat)

Fixes #18387
  • Loading branch information...
sgrif committed Feb 11, 2015
1 parent 5e0b555 commit c51f9b61ce1e167f5f58f07441adcfa117694301
Showing with 59 additions and 44 deletions.
  1. +5 −0 activerecord/CHANGELOG.md
  2. +38 −43 activerecord/lib/active_record/enum.rb
  3. +16 −1 activerecord/test/cases/enum_test.rb
@@ -1,3 +1,8 @@
* Have `enum` perform type casting consistently with the rest of Active
Record, such as `where`.
*Sean Griffin*
* `scoping` no longer pollutes the current scope of sibling classes when using
STI. e.x.
@@ -79,6 +79,37 @@ def inherited(base) # :nodoc:
super
end
class EnumType < Type::Value
def initialize(name, mapping)
@name = name
@mapping = mapping
end
def type_cast_from_user(value)
return if value.blank?
if mapping.has_key?(value)
value.to_s
elsif mapping.has_value?(value)
mapping.key(value)
else
raise ArgumentError, "'#{value}' is not a valid #{name}"
end
end
def type_cast_from_database(value)
mapping.key(value)
end
def type_cast_for_database(value)
mapping.fetch(value, value)
end
protected
attr_reader :name, :mapping
end
def enum(definitions)
klass = self
definitions.each do |name, values|
@@ -90,45 +121,27 @@ def enum(definitions)
detect_enum_conflict!(name, name.to_s.pluralize, true)
klass.singleton_class.send(:define_method, name.to_s.pluralize) { enum_values }
_enum_methods_module.module_eval do
# def status=(value) self[:status] = statuses[value] end
klass.send(:detect_enum_conflict!, name, "#{name}=")
define_method("#{name}=") { |value|
if enum_values.has_key?(value) || value.blank?
self[name] = enum_values[value]
elsif enum_values.has_value?(value)
# Assigning a value directly is not a end-user feature, hence it's not documented.
# This is used internally to make building objects from the generated scopes work
# as expected, i.e. +Conversation.archived.build.archived?+ should be true.
self[name] = value
else
raise ArgumentError, "'#{value}' is not a valid #{name}"
end
}
# def status() statuses.key self[:status] end
klass.send(:detect_enum_conflict!, name, name)
define_method(name) { enum_values.key self[name] }
detect_enum_conflict!(name, name)
detect_enum_conflict!(name, "#{name}=")

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Feb 14, 2015

Member

@sgrif @rafaelfranca @senny should these be (re)moved? I tried it, and there was some failing cases (logger=, I think). Since these are just regular attribute accessors, it seems like we should just do whatever the attribute methods usually does, and either fix the detection there or the test cases in enum?

@chancancode

chancancode Feb 14, 2015

Member

@sgrif @rafaelfranca @senny should these be (re)moved? I tried it, and there was some failing cases (logger=, I think). Since these are just regular attribute accessors, it seems like we should just do whatever the attribute methods usually does, and either fix the detection there or the test cases in enum?

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Feb 15, 2015

Member

I'd be fine with this.

@sgrif

sgrif Feb 15, 2015

Member

I'd be fine with this.

# def status_before_type_cast() statuses.key self[:status] end
klass.send(:detect_enum_conflict!, name, "#{name}_before_type_cast")
define_method("#{name}_before_type_cast") { enum_values.key self[name] }
attribute name, EnumType.new(name, enum_values)
_enum_methods_module.module_eval do
pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
pairs.each do |value, i|
enum_values[value] = i
# def active?() status == 0 end
klass.send(:detect_enum_conflict!, name, "#{value}?")
define_method("#{value}?") { self[name] == i }
define_method("#{value}?") { self[name] == value.to_s }
# def active!() update! status: :active end
klass.send(:detect_enum_conflict!, name, "#{value}!")
define_method("#{value}!") { update! name => value }
# scope :active, -> { where status: 0 }
klass.send(:detect_enum_conflict!, name, value, true)
klass.scope value, -> { klass.where name => i }
klass.scope value, -> { klass.where name => value }
end
end
defined_enums[name.to_s] = enum_values
@@ -138,25 +151,7 @@ def enum(definitions)
private
def _enum_methods_module
@_enum_methods_module ||= begin
mod = Module.new do
private
def save_changed_attribute(attr_name, old)
if (mapping = self.class.defined_enums[attr_name.to_s])
value = _read_attribute(attr_name)
if attribute_changed?(attr_name)
if mapping[old] == value
clear_attribute_changes([attr_name])
end
else
if old != value
set_attribute_was(attr_name, mapping.key(old))
end
end
else
super
end
end
end
mod = Module.new
include mod
mod
end
@@ -26,6 +26,17 @@ class EnumTest < ActiveRecord::TestCase
assert_equal @book, Book.unread.first
end
test "build from scope" do
assert Book.proposed.build.proposed?
refute Book.proposed.build.written?
assert Book.where(status: Book.statuses[:proposed]).build.proposed?
end
test "find via where" do
assert_equal @book, Book.where(status: "proposed").first
refute_equal @book, Book.where(status: "written").first
end
test "update by declaration" do
@book.written!
assert @book.written?
@@ -161,7 +172,11 @@ class EnumTest < ActiveRecord::TestCase
end
test "_before_type_cast returns the enum label (required for form fields)" do
assert_equal "proposed", @book.status_before_type_cast
if @book.status_came_from_user?

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Feb 11, 2015

Contributor

wouldn't a test only use one of those code paths?

@egilburg

egilburg Feb 11, 2015

Contributor

wouldn't a test only use one of those code paths?

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Aug 16, 2017

Member

Fixed in #29004.

@kamipo

kamipo Aug 16, 2017

Member

Fixed in #29004.

assert_equal "proposed", @book.status_before_type_cast
else
assert_equal "proposed", @book.status
end
end
test "reserved enum names" do

10 comments on commit c51f9b6

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 11, 2015

Member

@sgrif I LOVE YOU! ❤️

Member

rafaelfranca replied Feb 11, 2015

@sgrif I LOVE YOU! ❤️

@lucasmazza

This comment has been minimized.

Show comment
Hide comment
@lucasmazza

lucasmazza Feb 12, 2015

Member

❤️ 💚 💙 💛 💜

Member

lucasmazza replied Feb 12, 2015

❤️ 💚 💙 💛 💜

@philipgiuliani

This comment has been minimized.

Show comment
Hide comment
@philipgiuliani

philipgiuliani replied Feb 19, 2015

💥

@907th

This comment has been minimized.

Show comment
Hide comment
@907th

907th Apr 7, 2015

Contributor

@sgrif If Book.where(status: ["proposed", "other"]) would work?

Contributor

907th replied Apr 7, 2015

@sgrif If Book.where(status: ["proposed", "other"]) would work?

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Apr 7, 2015

Member

Yes on master that will work, along with everywhere else that we perform typecasting.

Member

sgrif replied Apr 7, 2015

Yes on master that will work, along with everywhere else that we perform typecasting.

@907th

This comment has been minimized.

Show comment
Hide comment
@907th

907th Apr 7, 2015

Contributor

@sgrif Thanks!

Contributor

907th replied Apr 7, 2015

@sgrif Thanks!

@KevinSjoberg

This comment has been minimized.

Show comment
Hide comment
@KevinSjoberg

KevinSjoberg Nov 9, 2015

Contributor

Just got bitten by this issue today in Rails 4. Awesome to see a fix for this in master. Great job @sgrif!

Contributor

KevinSjoberg replied Nov 9, 2015

Just got bitten by this issue today in Rails 4. Awesome to see a fix for this in master. Great job @sgrif!

@semikolon

This comment has been minimized.

Show comment
Hide comment
@semikolon

semikolon replied Nov 26, 2015

Nice!

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Nov 26, 2015

Member

@sgrif 💛

Member

senny replied Nov 26, 2015

@sgrif 💛

@bikolya

This comment has been minimized.

Show comment
Hide comment
@bikolya

bikolya Mar 22, 2016

Awesome! @sgrif FTW

bikolya replied Mar 22, 2016

Awesome! @sgrif FTW

Please sign in to comment.