Skip to content
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

ActiveRecord enum: use validation if exists instead of raising ArgumentError #13971

Closed
tjschuck opened this issue Feb 7, 2014 · 27 comments · May be fixed by #41730
Closed

ActiveRecord enum: use validation if exists instead of raising ArgumentError #13971

tjschuck opened this issue Feb 7, 2014 · 27 comments · May be fixed by #41730

Comments

@tjschuck
Copy link
Contributor

@tjschuck tjschuck commented Feb 7, 2014

Regarding ActiveRecord::Enum, assume you had something like

class Project < ActiveRecord::Base
  enum color: [:blue, :green, :red]
  validates :color, inclusion: { in: [:blue, :green, :red] }
end

Currently, if a "bad" value (say, "orange") is passed in, an ArgumentError is raised.

It would be nice if the validation was preferred (with the ArgumentError as a fallback if there was no validation on that attribute) so that you'd be able to return a nicer error on the object later via the standard means:

object.errors.messages
# => {:color=>["is not included in the list"]} kind of thing
@rafaelfranca rafaelfranca added this to the 4.1.0 milestone Feb 8, 2014
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Feb 8, 2014

Not sure if we have a easy fix for this.

To make validations work we will have to store the invalid value somewhere as string/symbol.

Worth to check.

Loading

@rafaelfranca rafaelfranca removed this from the 4.1.0 milestone Feb 8, 2014
@chancancode
Copy link
Member

@chancancode chancancode commented Feb 8, 2014

Hi @tjschuck, thanks for installing the beta and testing things early on – and most importantly providing feedback 💙

With what is being implemented right now, I feel that enum is not really a good fit for what you are trying to do. The current implementation is really just narrowly focused on ("optimized for" if you prefer) the use case suggested in the documentation – used by your application internally (to track internal "states"). For this purpose, I think raising an ArgumentError is the right/more helpful behaviour.

I wouldn't go as far as saying that you shouldn't use enums for your use case (more generally, to expose user-facing attributes). However, if you do use it for that purpose, you'd have to fill in a lot of gaps manually at the moment – such as form helpers, translating the internal symbol/string to user facing labels, i18n, validations, etc. That might or might not change in the future.

Personally, I'm not against expanding the focus to cover the second use case as well – provided that can be implemented without bloating this too much and without having to add more code to other parts of Active Record to make things work. (I'm not sure if that's feasible – this might be something that's better left with other gems.) However, even if we were to do that, it would most likely not make it to Rails 4.1.

I propose that we should close this for now (we don't use the github issue tracker to track feature requests), and you can start a discussion on the rails-core mailing list if you want. But I'll let @dhh make the call.

cc @senny

Loading

@senny
Copy link
Member

@senny senny commented Feb 8, 2014

Agree with @chancancode. The current focus of AR enums is to map a set of states (labels) to an integer for performance reasons. Currently assigning a wrong state is considered an application level error and not a user input error. That's why you get an ArgumentError.

I'm closing this for now but please do as @chancancode proposed and start a discussion on the mailing list.

@tjschuck thank you for using the beta 💛 !

Loading

@heaven
Copy link

@heaven heaven commented Jan 21, 2016

Hi, we do implement a REST API and have something like this in our controller:

def handle_action
  yield
rescue Unauthorized
  render :nothing => true, :status => 401
rescue CanCan::AccessDenied
  render :nothing => true, :status => 403
rescue ActiveRecord::RecordNotFound
  render :nothing => true, :status => 404
# ...
rescue ActiveRecord::RecordInvalid => e
  render :json => { :error => e.message }, :status => 422
rescue => e
  notify_airbrake(e)
  render :nothing => true, :status => 500
ensure
  ...
end

Our model has an enum attribute called kind, and if we receive an invalid value the ArgumentError being raised, the user receives 500 error code. And I don't see a good way to handle this at our end. We have to manually validate submitted values in controller, create additional services for that, etc. Doesn't seem right to me.

It would be helpful if it at least raise a more specific error, like ActiveRecord::EnumValueError or something like that.

Loading

@kaspth
Copy link
Member

@kaspth kaspth commented Jan 21, 2016

@heaven:

We have to manually validate submitted values in controller

You should absolutely do that 👍

Also we don't take feature requests on here, try the Rails Core mailing list to see if you can garner support on your proposal. Thanks ❤️

Loading

hughdavenport pushed a commit to hughdavenport/powershop_devtrain_monopoly that referenced this issue Apr 28, 2016
4 yellows due to rails bug rails/rails#13971,
skip tag in code.
hughdavenport pushed a commit to hughdavenport/powershop_devtrain_monopoly that referenced this issue May 5, 2016
@manbeardave
Copy link

@manbeardave manbeardave commented Jul 26, 2016

Hey all -

We just ran into this issue too on an API-only project. Looked at validating things manually and decided that the performance hit from just using the two strings (in our case 'mi' and 'km', describing length unit for a given radius) was acceptable in order to utilize ActiveRecord validation on our model for this (and all the nice automated error messaging that comes with it!)

Loading

@drewish
Copy link

@drewish drewish commented Feb 27, 2017

Ugg... just ran into this. Sad to see that two years later there's still no built in validation for enums.

Loading

@dhh
Copy link
Member

@dhh dhh commented Feb 28, 2017

Loading

@maschiner
Copy link

@maschiner maschiner commented Mar 16, 2017

If you are looking for a quick fix you can add this to your application_record.rb. It will suppress the exception when you have a validation for the enum defined.

class ApplicationRecord < ActiveRecord::Base
  def initialize(*)
    super
  rescue ArgumentError
    raise if valid?
  end
end
class Project < ApplicationRecord
  enum color: [:blue, :green, :red]
  validates :color, inclusion: { in: colors }
end

Loading

@drewish
Copy link

@drewish drewish commented Mar 16, 2017

@maschiner That wouldn't do anything about assignment though would it?

irb(main):007:0> p = Project.new(color: :purple)
=> #<Project id: nil, color: nil, created_at: nil, updated_at: nil>
irb(main):008:0> p.color = 'greenish'
ArgumentError: 'greenish' is not a valid color

Updated: It also seems to stop assigning values once it hits an invalid enum:

irb(main):011:0> p = Project.new id: 2, created_at: Time.now
=> #<Project id: 2, color: nil, created_at: "2017-03-16 19:52:18", updated_at: nil>
irb(main):012:0> p = Project.new color:'adf', id: 2, created_at: Time.now
=> #<Project id: nil, color: nil, created_at: nil, updated_at: nil>

Loading

@maschiner
Copy link

@maschiner maschiner commented Mar 16, 2017

@drewish Yes, I guess that fix was too quick ;)
I will try to clean this up and open a PR.

Loading

@drewish
Copy link

@drewish drewish commented Mar 17, 2017

I think to @rafaelfranca's original point, since the current behavior only allows assignment of valid values, we'd need to store the invalid string/symbol for validation to work. I don't know enough about the rails internals to understand the best way forward with that. If folks have some hints I'm happy to dig at it further.

Loading

@CristiRazvi
Copy link

@CristiRazvi CristiRazvi commented Nov 13, 2017

i have made a gem for validating enums inclusion
https://github.com/CristiRazvi/enum_attributes_validation

Loading

@swaathi
Copy link
Contributor

@swaathi swaathi commented Feb 13, 2018

@chancancode Do you think this is still a non issue? Or can I work on providing support for enums through out the entire process? I can work on form builders, scaffolding and validation. Please let me know your thoughts.

Loading

@henriquemenezes
Copy link

@henriquemenezes henriquemenezes commented Apr 24, 2018

In Rails 5.2, you can override assert_valid_value in initializers/active_record_enum.rb as follow:

module ActiveRecord
  module Enum
    class EnumType < Type::Value
      def assert_valid_value(value)
        unless value.blank? || mapping.has_key?(value) || mapping.has_value?(value)
          nil
        end
      end
    end
  end
end

Instead raise ArgumentError. You will set nil to a invalid value.

So, you can validate the enum as follow:

class User < ApplicationRecord
  enum gender: [:male, :female]
  validates :gender, inclusion: { in: genders.keys, message: :invalid }
end

Or if you want to use the value before casting you can use the attribute self.gender_before_type_cast to validate raw input.

Loading

hpjaj added a commit to department-of-veterans-affairs/vets-api that referenced this issue Apr 26, 2019
…n status (#3000)

* Endpoint to create a new dismissed status notification

* Enforces that a user cannot have a duplicate notification record

For a given subject, a user can only have one Notification record.

* Repurposable validation for enum subject and status values

Rails enum values, when invalid, will throw an ArgumentError on .new/.save/.create/.update; and throw a ActiveRecord::StatementInvalid error on .find/.find_by/etc.

This happens before any callbacks or custom validations can take effect.

So our choices for validation were:

1. Use a controller concern
2. Use tiered rescues/raises in the controller
3. Monkey patch (rails/rails#13971 (comment))
4. Custom model methods with rescues/raises

After chatting with a couple of devs about these choices, we decided that the best course of action was the controller concerns.

Our rationale was:

- This can be repurposed in the upcoming notifications_controller.rb class
- It is transparent
- maintainable
- and supports all uses cases.

* Swagger docs and specs
@dikond
Copy link
Contributor

@dikond dikond commented Jul 17, 2019

Just run into this issue. To me, the behavior of AR enum is surprising. I would not expect this code to raise an ArgumentError

class Order
  enum status: { one: 1, two: 2, three: 3 }
  validates :status, inclusion: { in: Order.statuses.keys }
end

order = Order.new
order.status = 'zero'

In my experience, AR enums are used as user-facing attributes A LOT. Community have several gems that addresses enum issues (e.g. enumerize gem or the enum_attributes_validation mentioned above). But I think rails should address these issues and provide better experience with enums out of the box. Would be glad to help with that issue.

Loading

@stas
Copy link

@stas stas commented Jul 23, 2019

Hi there!
I'd like to challenge the reason this issue got closed. I believe the fact that we have 240+ (🙀) gems for the enum keyword is one of the effects of closing this conversation.

Would you be so kind re-opening this issue pretty please? 🙏
At least it will suggest we're open for contributions (and it seems like we are):

You can put a happy face on the situation by volunteering to work on it 😄. Rails is always open to new contributors!

Loading

@shlima
Copy link

@shlima shlima commented Oct 16, 2019

I've found a solution. Tested by myself in Rails 6.

# app/models/contact.rb
class Contact < ApplicationRecord
  include LiberalEnum

  enum kind: {
    phone: 'phone', skype: 'skype', whatsapp: 'whatsapp'
  }

  liberal_enum :kind

  validates :kind, presence: true, inclusion: { in: kinds.values }
end
# app/models/concerns/liberal_enum.rb
module LiberalEnum
  extend ActiveSupport::Concern

  class_methods do
    def liberal_enum(attribute)
      decorate_attribute_type(attribute, :enum) do |subtype|
        LiberalEnumType.new(attribute, public_send(attribute.to_s.pluralize), subtype)
      end
    end
  end
end
# app/types/liberal_enum.rb
class LiberalEnumType < ActiveRecord::Enum::EnumType
  # suppress <ArgumentError>
  # returns a value to be able to use +inclusion+ validation
  def assert_valid_value(value)
    value
  end
end

Usage:

contact = Contact.new(kind: 'foo')
contact.valid? #=> false
contact.errors.full_messages #=> ["Kind is not included in the list"]

Loading

@stas
Copy link

@stas stas commented Oct 16, 2019

ActiveRecord::Enum::EnumType was introduced only in Rails 5.

An alternative would be to just overwrite your attribute setter. Consider this:

#...
enum state: {
  ok: 'ok',
  nok: 'nok',
}

validates_inclusion_of :state, in: ->(inst) { inst.class.states.keys }

# Overwrite the setter to rely on validations instead of [ArgumentError]
#
# @return [Object]
def state=(value)
  self[:state] = value
end
#...

Loading

@shlima
Copy link

@shlima shlima commented Oct 16, 2019

An alternative would be to just overwrite your attribute setter. Consider this:

This will raise an ArgumentError at least in Rails 6

Loading

@stas
Copy link

@stas stas commented Oct 16, 2019

@shlima could you be more specific? We're successfully using this across rails 4 and 6 codebases!

Loading

@airblade
Copy link
Contributor

@airblade airblade commented Jan 29, 2020

@stas At that point there isn't much benefit to using an enum over a normal text field.

Loading

@stas
Copy link

@stas stas commented Jan 29, 2020

@airblade I think it makes sense from the API point of view to adopt the functionality and hope that the Rails team will keep improving it based on our feedback. I find the tiny sugar a nice addition, but obviously it's not a selling point.

Loading

@MacksMind
Copy link
Contributor

@MacksMind MacksMind commented Nov 4, 2020

Here's a variation on @stas's solution that appears to work in 6.0.3.4:

enum role: { regular: 0, manager: 1, admin: 2 }

validates :role, inclusion: { in: ->(inst) { inst.class.roles.keys } }

# Overwrite the setter to rely on validations instead of [ArgumentError]
def role=(value)
  self[:role] = value
rescue ArgumentError
  self[:role] = nil
end

Loading

@niri4
Copy link

@niri4 niri4 commented Nov 13, 2020

I think @MacksMind's solution appears to not works in the case of custom validation messages where the value passed in the message and is not an enum. something like "#{value} is an invalid role" for the listed example

enum role: { regular: 0, manager: 1, admin: 2 }

validates :role, inclusion: {
    in:roles.keys,
    message: "%{value} is an invalid role"
  }

@user = User.new(role: 'customer')

@user.valid?

@user.errors.messages

output 

{:role=>  is not a valid role }

Loading

kamipo added a commit to kamipo/rails that referenced this issue Mar 22, 2021
Last year, I heard about the inconveniences of ActiveRecord Enum and got
some feedback (one of those is rails#40456).

Some people have reported that in practice they are using enums for user
input and it is inconvenient to not be able to handle invalid input with
validation like other attributes.

Actually, a former colleague was overriding the attribute writer for
validation, and they had wished this inconvenience to be resolved in the
Rails.

So I propose a way to add validation on the enum attribute. This allows
people to no longer need to override the attribute writer.

Before:

```ruby
class Book < ActiveRecord::Base
  enum :status, [:proposed, :written]

  validates_inclusion_of :status, in: statuses.keys

  def status=(value)
    super
  rescue ArgumentError
    @attributes.write_cast_value("status", value)
  end
end
```

After:

```ruby
class Book < ActiveRecord::Base
  enum :status, [:proposed, :written], validate: true
end
```

Resolves rails#13971.
@blocknotes
Copy link

@blocknotes blocknotes commented Aug 23, 2021

I have the same problem, but I can't change the enum behaviour of the whole application.
So I prefer this workaround to fix a single field:

    validates :card_type, inclusion: { in: card_types.keys }

    def card_type=(value)
      self.class.card_types.has_key?(value) ? super : nil
    end

Loading

@stas
Copy link

@stas stas commented Sep 1, 2021

Folks, please weight on #41730 to get it merged and get this issue closed 🙏 🙌 !!!!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.