Skip to content

Commit

Permalink
Use the database type to deserialize enum
Browse files Browse the repository at this point in the history
This fixes incorrect assumptions made by e991c7b that we can assume the
DB is already casting the value for us. The enum type needs additional
information to perform casting, and needs a subtype.

I've opted not to call `super` in `cast`, as we have a known set of
types which we accept there, and the subtype likely doesn't accept them
(symbol -> integer doesn't make sense)

Close #23190
  • Loading branch information
sgrif committed Jan 23, 2016
1 parent 8de32bb commit 67c1719
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 4 deletions.
11 changes: 7 additions & 4 deletions activerecord/lib/active_record/enum.rb
Expand Up @@ -105,9 +105,10 @@ def inherited(base) # :nodoc:
end

class EnumType < Type::Value # :nodoc:
def initialize(name, mapping)
def initialize(name, mapping, subtype)
@name = name
@mapping = mapping
@subtype = subtype
end

def cast(value)
Expand All @@ -124,7 +125,7 @@ def cast(value)

def deserialize(value)
return if value.nil?
mapping.key(value)
mapping.key(subtype.deserialize(value))
end

def serialize(value)
Expand All @@ -139,7 +140,7 @@ def assert_valid_value(value)

protected

attr_reader :name, :mapping
attr_reader :name, :mapping, :subtype
end

def enum(definitions)
Expand All @@ -158,7 +159,9 @@ def enum(definitions)
detect_enum_conflict!(name, name)
detect_enum_conflict!(name, "#{name}=")

attribute name, EnumType.new(name, enum_values)
decorate_attribute_type(name, :enum) do |subtype|
EnumType.new(name, enum_values, subtype)
end

_enum_methods_module.module_eval do
pairs = values.respond_to?(:each_pair) ? values.each_pair : values.each_with_index
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/enum_test.rb
Expand Up @@ -411,4 +411,14 @@ def self.name; 'Book'; end
assert book.proposed?, "expected fixture to default to proposed status"
assert book.in_english?, "expected fixture to default to english language"
end

test "uses default value from database on initialization" do
book = Book.new
assert book.proposed?
end

test "uses default value from database on initialization when using custom mapping" do
book = Book.new
assert book.hard?
end
end
1 change: 1 addition & 0 deletions activerecord/test/models/book.rb
Expand Up @@ -14,6 +14,7 @@ class Book < ActiveRecord::Base
enum author_visibility: [:visible, :invisible], _prefix: true
enum illustrator_visibility: [:visible, :invisible], _prefix: true
enum font_size: [:small, :medium, :large], _prefix: :with, _suffix: true
enum cover: { hard: 'hard', soft: 'soft' }

This comment has been minimized.

Copy link
@jrochkind

jrochkind Sep 6, 2016

Contributor

@sgrif This seems to be the first time there was a test case around Enum with db values that were Strings rather than integers.

But it's associated with a few PR's and Issues that are about default values in Enums, and don't seem to mention db types or if/when/that db strings are supported as well as Integers.

Something that's still not documented anywhere, such as the Enum class header inline docs!

Curious if you know anything about the origin/history of Enum supporting String db values? I was wishing it did, and went to look at the source -- to find that it already did. At least in Rails5. Now I'm very curious.

This comment has been minimized.

Copy link
@sgrif

sgrif via email Sep 7, 2016

Author Contributor

This comment has been minimized.

Copy link
@jrochkind

jrochkind Sep 7, 2016

Contributor

@sgrif Thanks! interesting. But you added tests for strings? So they obviously work fine.

What would it take to make strings officially supported? A PR request can't really be used to kick it off, cause the functionality is already there. Hm, a PR request for docs?


def published!
super
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/schema/schema.rb
Expand Up @@ -104,6 +104,7 @@ def except(adapter_names_to_exclude)
t.column :author_visibility, :integer, default: 0
t.column :illustrator_visibility, :integer, default: 0
t.column :font_size, :integer, default: 0
t.column :cover, :string, default: 'hard'
end

create_table :booleans, force: true do |t|
Expand Down

0 comments on commit 67c1719

Please sign in to comment.