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

Make enums validatable without raising error #49100

Merged

Conversation

mechnicov
Copy link
Contributor

@mechnicov mechnicov commented Sep 1, 2023

Motivation / Background

There is a pretty old issue #13971
@dhh offered to make such contributions in 2017
But opened MR #41730 was not maintained since 2021
I tried to take into account the comments in that code review

Detail

This Pull Request adds :validate option for enums

If you want the enum value to be validated before saving, use the option :validate:

class Conversation < ApplicationRecord
  enum :status, %i[active archived], validate: true
end

conversation = Conversation.new

conversation.status = :unknown
conversation.valid? # => false

conversation.status = nil
conversation.valid? # => false

conversation.status = :active
conversation.valid? # => true

It is also possible to pass additional validation options:

class Conversation < ApplicationRecord
  enum :status, %i[active archived], validate: { allow_nil: true }
end

conversation = Conversation.new

conversation.status = :unknown
conversation.valid? # => false

conversation.status = nil
conversation.valid? # => true

conversation.status = :active
conversation.valid? # => true

Otherwise ArgumentError will raise (standard current behavior):

class Conversation < ApplicationRecord
  enum :status, %i[active archived]
end

conversation = Conversation.new

conversation.status = :unknown # 'unknown' is not a valid status (ArgumentError)

Additional information

This was intended as non-breaking change

Checklist

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@@ -209,7 +256,7 @@ def _enum(name, values, prefix: nil, suffix: nil, scopes: true, instance_methods

attribute(name, **options) do |subtype|
subtype = subtype.subtype if EnumType === subtype
EnumType.new(name, enum_values, subtype)
EnumType.new(name, enum_values, subtype, raise_on_invalid_values: !validate)
Copy link
Member

Choose a reason for hiding this comment

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

Is it expected for an ArgumentError to be raised when we explicitly include validate: false in the enum definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is

Copy link
Member

Choose a reason for hiding this comment

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

If that is the case, why do we need the validate option, since we are raising the error for both cases? I mean for 'true' and 'false' cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you don't need pass validate: false. Like in belongs_to you don't need to pass optional: false explicitly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR:

  • when :validate is falsey (by default) — standard "old" behaviour with raising error
  • when :validate is truthy (need explicit specify) — soft adding of validation message

Detailed:

ArgumentError raises only if the validate argument is falsy (validate: nil or validate: false) and only if you try to assign invalid value. It is current behaviour of enum before this PR. And PR suggests keep this behaviour as is. That's why these change don't break legacy code. And you don't need pass validate: false explicitly. It is redundant like with above example (belongs_to :user, optional: false — it is unlikely somebody write)

If pass truthy validate argument, enum is validated with rails inclusion validation. If you try to assign invalid value, a validation error is added to the model instance (ArgumentError doesn't raise). But ActiveRecord::RecordInvalid can raise if use bang methods save!, create! etc.)

Please see examples in the PR description and tests. I think it will be more understandable

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense @mechnicov. Thanks :)

@rafaelfranca
Copy link
Member

validate as the name of this option is strange. Raising Argument error is a form of validation. Maybe a better name would be custom_validation?

@rafaelfranca
Copy link
Member

Or invert the meaning validate: false is what disable the ArgumentError behavior, not validate: true. Or rename to strict: false matching the validates API.

:strict - If the :strict option is set to true will raise ActiveModel::StrictValidationFailed instead of adding the error. :strict option can also be set to any other exception.

@mechnicov
Copy link
Contributor Author

mechnicov commented Sep 1, 2023

@rafaelfranca yes, probably there is some ambiguity when we talk about validation in this particular case

validate was meant as "usual" rails validation. I don't think custom_validation is much better. I thought about active_record_validation option or something like this. But it seemed to me that it was too long a name. What are your thoughts on this?

I was also thinking about strict: false, but in this case it will be strange to pass hash with validation options. Or it is okay to have something like strict: { allow_nil: true }?

@rafaelfranca
Copy link
Member

hmm. Good point. I think validate: true makes sense, specially if we are allowing options like :allow_nil.

@mechnicov mechnicov force-pushed the make-enum-validatable-without-raising-error branch from d8b419b to 6ee0d5e Compare September 1, 2023 19:06
@rafaelfranca rafaelfranca merged commit 7c65a4b into rails:main Sep 1, 2023
4 checks passed
kamipo added a commit that referenced this pull request Sep 4, 2023
Since #41730 is the original work for the enum attribute validation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants