Skip to content

Commit

Permalink
Added ActiveRecord::Base#enum for declaring enum attributes where the…
Browse files Browse the repository at this point in the history
… values map to integers in the database, but can be queried by name
  • Loading branch information
dhh committed Nov 2, 2013
1 parent deaf285 commit db41eb8
Show file tree
Hide file tree
Showing 7 changed files with 128 additions and 2 deletions.
25 changes: 25 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,28 @@
* Added ActiveRecord::Base#enum for declaring enum attributes where the values map to integers in the database, but can be queried by name.

Example:
class Conversation < ActiveRecord::Base
enum status: %i( active archived )
end

Conversation::STATUS # => { active: 0, archived: 1 }

# conversation.update! status: 0
conversation.active!
conversation.active? # => true
conversation.status # => :active

# conversation.update! status: 1
conversation.archived!
conversation.archived? # => true
conversation.status # => :archived

# conversation.update! status: 1
conversation.status = :archived

*DHH*


* ActiveRecord::Base#attribute_for_inspect now truncates long arrays (more than 10 elements)

*Jan Bernacki*
Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record.rb
Expand Up @@ -37,6 +37,7 @@ module ActiveRecord
autoload :ConnectionHandling
autoload :CounterCache
autoload :DynamicMatchers
autoload :Enum
autoload :Explain
autoload :Inheritance
autoload :Integration
Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record/base.rb
Expand Up @@ -291,6 +291,7 @@ class Base
extend Translation
extend DynamicMatchers
extend Explain
extend Enum
extend Delegation::DelegateCache

include Persistence
Expand Down
60 changes: 60 additions & 0 deletions activerecord/lib/active_record/enum.rb
@@ -0,0 +1,60 @@
module ActiveRecord
# Declare an enum attribute where the values map to integers in the database, but can be queried by name. Example:
#
# class Conversation < ActiveRecord::Base
# enum status: %i( active archived )
# end
#
# Conversation::STATUS # => { active: 0, archived: 1 }
#
# # conversation.update! status: 0
# conversation.active!
# conversation.active? # => true
# conversation.status # => :active
#
# # conversation.update! status: 1
# conversation.archived!
# conversation.archived? # => true
# conversation.status # => :archived
#
# # conversation.update! status: 1
# conversation.status = :archived
#
# You can set the default value from the database declaration, like:
#
# create_table :conversation do
# t.column :status, :integer, default: 0
# end
#
# Good practice is to let the first declared status be the default.
module Enum
def enum(definitions)
definitions.each do |name, values|
const_name = name.to_s.upcase

# DIRECTION = { }
const_set const_name, {}

# def direction=(value) self[:direction] = DIRECTION[value] end
class_eval "def #{name}=(value) self[:#{name}] = #{const_name}[value] end"

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Nov 3, 2013

Contributor

@tenderlove didn't we decide that using define_method was better than class_eval for dynamically defining methods - I remember you doing some research into this.

This comment has been minimized.

Copy link
@tenderlove

tenderlove via email Nov 3, 2013

Member

This comment has been minimized.

Copy link
@dhh

dhh Nov 3, 2013

Author Member

I think it just appeared clearer to me in the code. Are you saying that class_eval is a performance issue? That seems surprising to me given that this is only called once per listing and at compile time.

This comment has been minimized.

Copy link
@ka8725

ka8725 Nov 4, 2013

Contributor

define_method is better because you will have properly worked respond_to?

This comment has been minimized.

Copy link
@thedarkone

thedarkone Nov 4, 2013

Contributor

define_method is better because you will have properly worked respond_to?

@ka8725 what do you mean?

This comment has been minimized.

Copy link
@ck3g

ck3g Nov 4, 2013

Contributor

@ka8725 I guess there is no difference for respond_to?.

2.0.0p247 :001 > class MyClass
2.0.0p247 :002?>   define_method :my_define_method do
2.0.0p247 :003 >       'my_define_method'
2.0.0p247 :004?>     end
2.0.0p247 :005?>   
2.0.0p247 :006 >     class_eval <<-EVAL
2.0.0p247 :007">       def my_class_eval
2.0.0p247 :008">         'my_class_eval'
2.0.0p247 :009">       end
2.0.0p247 :010">     EVAL
2.0.0p247 :011?>   end
 => nil 
2.0.0p247 :012 > MyClass.new.respond_to? :my_define_method
 => true 
2.0.0p247 :013 > MyClass.new.respond_to? :my_class_eval
 => true 

Did I understood you correctly?

This comment has been minimized.

Copy link
@ka8725

ka8725 Nov 4, 2013

Contributor

Yes, you are right. It's my fault

This comment has been minimized.

Copy link
@pixeltrix

pixeltrix Nov 4, 2013

Contributor

@dhh what @tenderlove is saying is that you should only use class_eval if you need maximum performance for a simple method. If the work done in the method is anything significant then the call performance gains are masked by the time taken to do the work. More information is in his blog post: http://tenderlovemaking.com/2013/03/03/dynamic_method_definitions.html

This comment has been minimized.

Copy link
@dhh

dhh via email Nov 4, 2013

Author Member

This comment has been minimized.

Copy link
@ck3g

ck3g Nov 4, 2013

Contributor

@dhh

Ah. I'd be ok with a patch for define_method. I prefer the look of class_eval but I'm not attached to that.

#12754

This comment has been minimized.

Copy link
@dhh

dhh Nov 4, 2013

Author Member

Gotta love open source! Thanks @ck3g!


# def direction() DIRECTION.key self[:direction] end
class_eval "def #{name}() #{const_name}.key self[:#{name}] end"

values.each_with_index do |value, i|
# DIRECTION[:incoming] = 0
const_get(const_name)[value] = i

# scope :incoming, -> { where direction: 0 }
scope value, -> { where name => i }

# def incoming?() direction == 0 end
class_eval "def #{value}?() self[:#{name}] == #{i} end"

# def incoming! update! direction: :incoming end
class_eval "def #{value}!() update! #{name}: :#{value} end"
end
end
end
end
end
36 changes: 36 additions & 0 deletions activerecord/test/cases/enum_test.rb
@@ -0,0 +1,36 @@
require 'cases/helper'
require 'models/book'

class StoreTest < ActiveRecord::TestCase
fixtures :books

setup do
@book = Book.create! name: 'REMOTE'

This comment has been minimized.

Copy link
@drbonzo

drbonzo Nov 2, 2013

nice line :)

end

test "query state by predicate" do
assert @book.proposed?
assert_not @book.written?
assert_not @book.published?
end

test "query state with symbol" do
assert_equal :proposed, @book.status
end

test "update by declaration" do
@book.written!
assert @book.written?
end

test "update by setter" do
@book.update! status: :written
assert @book.written?
end

test "constant" do
assert_equal 0, Book::STATUS[:proposed]
assert_equal 1, Book::STATUS[:written]
assert_equal 2, Book::STATUS[:published]
end
end
6 changes: 4 additions & 2 deletions activerecord/test/models/book.rb
Expand Up @@ -2,8 +2,10 @@ class Book < ActiveRecord::Base
has_many :authors

has_many :citations, :foreign_key => 'book1_id'

This comment has been minimized.

Copy link
@mcspring

mcspring Nov 3, 2013

Refactor this to new ruby hash syntax?

has_many :references, -> { distinct }, :through => :citations, :source => :reference_of
has_many :references, -> { distinct }, through: :citations, source: :reference_of

has_many :subscriptions
has_many :subscribers, :through => :subscriptions
has_many :subscribers, through: :subscriptions

enum status: %i( proposed written published )
end
1 change: 1 addition & 0 deletions activerecord/test/schema/schema.rb
Expand Up @@ -94,6 +94,7 @@ def create_table(*args, &block)
create_table :books, :force => true do |t|
t.integer :author_id
t.column :name, :string
t.column :status, :integer, default: 0
end

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

29 comments on commit db41eb8

@lbramos
Copy link

@lbramos lbramos commented on db41eb8 Nov 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome x2 (Y)

@byroot
Copy link
Member

@byroot byroot commented on db41eb8 Nov 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though it was a support for mysql and postgres enum types. :sad:

@gotar
Copy link

@gotar gotar commented on db41eb8 Nov 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, (+1)

@mlangenberg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful when adding or removing values later on. It might be better to do an explicit mapping to an integer, instead of relying on the order of elements in the status array.

Given the example enum status: %i( proposed written published ) and you eventually decide you don't need the 'written' state at all. Making this change enum status: %i( proposed published ), will be very confusing, since written books are now suddenly published. You could argue for a migration, but I would rather stick with mapping '2' to 'published'.

At least when using MySQL enum type, it can migrate data for you (albeit expensive). With the mapping in Rails, why not support a tuples with values and integers?

enum status: [[:proposed, 0], [:written, 1], [:published, 2]]

@dhh
Copy link
Member Author

@dhh dhh commented on db41eb8 Nov 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlangenberg, I'd be happy to see a patch that also supports explicit mapping, but let's go with this format:

enum status: { proposed: 0, written: 1, published: 2 }

But it would be in addition to what we have now.

@egilburg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally a more common pattern I've seen is where the enum values are already stored in db under the matching column name. For example:

In seeds.rb:

Role.create!(name: 'admin')
Role.create!(name: 'user')

And what is needed API like:

Role.admin # => [<#Role>]
Role.first.admin? # => true or false, instead of the more clunky StringInquirer way of Role.first.name.admin?
Role.first.admin! # (sets 'name' to 'admin')

It would be cool if this was supported by:

class Role < ActiveRecord::Base
  enum name: i%( admin user )
end

Not sure if this will clash with current implementation though.

@dhh
Copy link
Member Author

@dhh dhh commented on db41eb8 Nov 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty inefficient to store enums as text. You're going to repeat the same text over and over and over again. I'd consider that an anti pattern. People are better off doing a migration to ints if they want to use this.

@byroot
Copy link
Member

@byroot byroot commented on db41eb8 Nov 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But integers remove readability / discoverability.

This is why MySQL, Postgres and Oracle (at least) offer enums that expose text but store integers. Support was added for postgres arrays and hstore, why not adding support for enums ?

@dhh
Copy link
Member Author

@dhh dhh commented on db41eb8 Nov 2, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@byroot
Copy link
Member

@byroot byroot commented on db41eb8 Nov 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do.

@yury
Copy link
Contributor

@yury yury commented on db41eb8 Nov 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh here is PR for explicit mapping.

@rahult
Copy link

@rahult rahult commented on db41eb8 Nov 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome 👍

@ck3g
Copy link
Contributor

@ck3g ck3g commented on db41eb8 Nov 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@koteus
Copy link

@koteus koteus commented on db41eb8 Nov 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this had a bitmask feature out of the box. For example to assign multiple roles to a user:

enum roles: %i(writer publisher editor admin), multiple: true

And usage:

user = User.create name: "David", roles: %i(publisher editor)
user.roles
# => [:publisher, :editor]
user.roles << :writer
user.roles
# => [:publisher, :editor, :writer]

But there's a gem for this already: https://github.com/joelmoss/bitmask_attributes

@joelmoss
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be biased here, but not seeing why you wouldn't just use the bitmask_attributes gem.

@yonbergman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I have a pretty similar gem that adds enum functionality /yonbergman/enumify
and we found that having a not scope for each enum value is really helpful -

# scope :not_incoming, -> { where.not direction: 0 }
scope "not_#{value}", -> { where.not name => i }

Other than that awesome feature :)

@ka8725
Copy link
Contributor

@ka8725 ka8725 commented on db41eb8 Nov 4, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yonbergman, you forgot to mention about validations, form helpers, i18n, ability to change column type, multiplying and some more useful features.

@samqiu
Copy link

@samqiu samqiu commented on db41eb8 Nov 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh Why are you so awesome!? 👍 👍 👍

@brendanstennett
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be helpful to prepend the enum name to the generated instance methods. This would help avoid collisions of two different enum fields on the same model having an identical enum value. Example below:

class Foo < ActiveRecord::Base
  enum :state, [:open, :closed]
  enum :other_state, [:something, :closed]
end

The generated methods #closed, #closed? and #closed! for other_state would collide. Generating methods like #state_closed, #state_closed? and #state_closed! (likewise with other_state) would solve the collision issue.

@dhh
Copy link
Member Author

@dhh dhh commented on db41eb8 Dec 20, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chancancode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are adding the error, see #13389

@brendanstennett
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhh make sense, probably not a likely scenario anyways I suppose.

@chancancode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, if you ended up with a collision, you can always prefix them yourself, like enum :state, [:state_open, :state_closed], and you'll get exactly the same thing. Meanwhile, the common case gets the nice syntax. Best of both worlds(TM)

@kenn
Copy link
Contributor

@kenn kenn commented on db41eb8 Dec 20, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think raise on collision is a sane default for simple use cases, but when your requirements need more, here's another gem: https://github.com/kenn/enum_accessor it does what @HuffMoody mentioned above.

I'm looking forward to seeing validation / i18n support and hopefully I'll be able to ditch my own implementation someday. :)

@imanel
Copy link
Contributor

@imanel imanel commented on db41eb8 Dec 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to implement optional prefixing for those that need it in #13433 - I believe that it's much easier to use it like:

# enum :state, [:open, :state], nested: true
conversation.status = :open
conversation.status #=> :open
conversation.status_open?
Conversation.status_open

instead of currently proposed solution:

# enum :state, [:state_open, :state_closed]
conversation.status = :status_open
conversation.status #=> :status_open
conversation.status_open?
Conversation.status_open

Current implementation might be little to verbose (I have this slight Java-like feeling ;)

@dhh
Copy link
Member Author

@dhh dhh commented on db41eb8 Dec 21, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imanel
Copy link
Contributor

@imanel imanel commented on db41eb8 Dec 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which, I believe, is very inconvenient and counterintuitive for some users. It's much more obvious to set status to "open" than to "status_open", needles to say that it's really unnecessary duplication. Best example is exposing such value via API, where you would need to convert from "status_open" to "open" during both read and write (I can't imagine API with "status" set to "status_open", so conversion would be a must). Prefixing is also optional, so default behavior is not changed and it should not interrupt how your proposal works.

@kenn
Copy link
Contributor

@kenn kenn commented on db41eb8 Dec 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, enum :state, [ :state_value ] should not be recommended as a workaround, I know first hand how ugly it would become in the source, that's the main reason that I created enum_accessor when I found at least 7 enum gems at the time but none did prefixing.

user.gender = :gender_man
user.save

user.gender
 => :gender_man

This is just so uncool.

Let's just raise on a collision and if multiple enums are necessary, say use gems like enum_accessor.

@imanel
Copy link
Contributor

@imanel imanel commented on db41eb8 Dec 21, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should raise on a collision, but I would also be glad to see optional namespacing in Rails core so there would be no need to duplicate it using third party gems (especially that having it and not having it is 1 line of code)

Please sign in to comment.