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 it so that the activemodel validations return the correct error types #162

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

alexcarruthers
Copy link
Contributor

We were using this gem and getting errors returned that looked like:
#<ActiveModel::Error attribute=weight, type=-25 kg must be >= 0 kg, options={}>
where the type was not deterministic. Looking into the gem and it looks like the message is being sent as a type. The documentation for ActiveRecord::Errors.add states that if a string is passed into type, it's used as the error message, but in this case it doesn't seem to do that. This PR updates the code to be more explicit, while also passing on the actual error type that's violated.

@kmcphillips kmcphillips requested a review from paracycle January 27, 2025 19:41
Copy link
Member

@kmcphillips kmcphillips left a comment

Choose a reason for hiding this comment

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

Thanks for the good test coverage.

Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Ummm, let's get this merged, since it is obviously better and more explicit code. But now that I am looking at the Rails code, this might be pointing to a regression that went undetected for many versions now?!

@alexcarruthers alexcarruthers merged commit edc12bf into main Jan 27, 2025
22 checks passed
@alexcarruthers alexcarruthers deleted the fix-model-validation-error-types branch January 27, 2025 20:14
@paracycle
Copy link
Member

... The documentation for ActiveRecord::Errors.add states that if a string is passed into type, it's used as the error message, but in this case it doesn't seem to do that.

Small correction: this is not broken, this is still happening, since the message attribute of an ActiveModel::Error is computed lazily to read either from the type value or the string message passed.

The thing that is unexpected is for the type field to also have the message as its value.

Example:

irb> require "active_model"
irb>
irb> class Foo
irb>   include ActiveModel::Model
irb>
irb>   attr_reader :name
irb>
irb>   validates_each :name do |r, a, v|
irb>     r.errors.add(:name, "this is not a message")
irb>   end
irb> end
#=> [ActiveModel::BlockValidator]

irb> foo = Foo.new
#=> #<Foo:0x000000012a83cce8>

irb> foo.valid?
#=> false

irb> foo.errors
#=> #<ActiveModel::Errors [#<ActiveModel::Error attribute=name, type=this is not a message, options={}>]>

# The following works as expected
irb> foo.errors.first.message
#=> "this is not a message"

# The following is surprising
irb> foo.errors.first.type
#=> "this is not a message"

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

Successfully merging this pull request may close these issues.

3 participants