Model error as object

[lulalala]

Summary

Add ActiveModel::Error class for encapsulating a single error. It handles message generation and details access.
Utilize this in Errors class, changing it from a hash based interface, to an array of Error objects. Add more flexible query methods like where.

Introduction

ActiveModel#errors interface is currently not very object oriented. For some complex use cases, this design made it a bit tedious to use. I feel these issues can be remedied by encapsulating each individual error as an object.

Last year I implemented AdequateErrors gem to do this, and it solved many of my problems. I later found out that back in 2016, @eprothro suggested the same idea too on the core mailinglist, and Rafael was positive about this. So I made this PR.

I've made it to respect Rails’ deprecation policy. As this contains breaking changes, can this go straight into Rails 6.0?

I will fix other specs/add doc/performance tuning once interface is finalized.

Benefits

More flexible query interface such as where

It’s easy to query one particular object using the where clause.

model.errors.where(:name, :foo, bar: 3).first

delete, add, added?, where all share the same method signature (attribute, type, options). So we are able to delete specific errors now:

model.errors.delete(:name, :too_powerful, level: 9000)

Testing is more precise and flexible

We can now test if “foo” error exists, regardless of its options hash.

model.errors.add(:name, :too_powerful, level: 9000)

model.errors.added?(:name, :too_powerful, level: 9000) # returns true
model.errors.added?(:name, :too_powerful) # will be false in the past, but be true now.

In the past, added? works by re-render the message and compare it against current message. Therefore if level is not provided, it will return false. In the PR, added? only compare Error's attributes against what's provided, so it can be more general or more specific depending on the needs.

Get message of corresponding details of one particular error

If you saw that name attribute has two foo_error and one bar_error, e.g.:

# model.errors.details
{:name=>[{error: :foo_error, count: 1}, {error: :bar_error}, {error: :foo_error, count: 3}]}

How do you back track the message on the third particular error? With the current implementation, we have to resort to using array indexes:

model.errors.messages[:name][2]

Or we can call generate_message to recreate a message from the details, but that's also tedious.

With OO, we won't have this problem. Error is represented as an object, message and details are its attributes, so accessing those are straightforward:

e = model.errors.where(:name, :foo_error).last
e.full_message
e.options # similar to details, where meta informations such as `:count` is stored.

Lazily evaluating message for internationalization

@morgoth mentioned this issue that when you're adding error, it's translated right away.

# actual:
I18n.with_locale(:pl) { user.error.full_messages } # => outputs EN errors always

# expecting:
I18n.with_locale(:pl) { user.error.full_messages } # => outputs PL errors
I18n.with_locale(:pt) { user.error.full_messages } # => outputs PT errors

This PR addresses this by lazily evaluating messages only when message is called.

Opens possibility to advanced modding

Once errors are objects, it’s easy to add functionality on top of them. We can have custom methods to disable global attribute prefix on error’s full messages.

List of API changes

[]
unchanged, deprecated (use messages_for instead)

as_json, blank?, clear, count, empty?
unchanged

add
unchanged

added?
mostly unchanged, for the one change see "Testing is more precise and flexible"

delete
extended, so we can give more specific condition such as error type or options.

each
If we use each{|attr,msgs|} then it behaves the same as before. Deprecated
If we use each{|error|} then it loops through Error array.

full_message
deprecated as it is no longer needed.
unchanged message is generated in Error.

full_messages, full_messages_for
unchanged

messages_for
new, to replace deprecated []

where
new, query for error objects
generate_message
deprecated Part of Error as message is generated there.

has_key? key?, keys
deprecated

include?, size, to_hash, to_xml, to_a
unchanged

values
deprecated

import
new, imports one error as a nested error. Useful in Form Object and nested attribute error importing.

messages, details
unchanged

Some questions I have

I am not sure what's the policy for marshal across versions. marshal_dump and marshal_load are implemented, but do they have to support marshaling across Rails versions?

This has been done.

Can we deprecate to_xml? I bet is rarely used, and can exists as a gem.

[pixeltrix]

@lulalala is there a reason why you can't emulate the hash-based API?

[lulalala]

@pixeltrix for the hash like APIs, my thoughts are:

1. For enumerable methods such as each, I choose to forward them to the error array. Keeping it to enumerate as hash seems to defeat the purpose.
2. For [] and to_hash, they are emulated.
3. For values, I removed it. I feel it was there just so errors feels like a hash. It is not useful because having an array of partial error message without knowing each message's respective attribute is useless.

[rafaelfranca]

Everything that was removed needs to be deprecated first. We don't make breaking changes without deprecation, even in major versions.

marshal_dump and marshal_load are implemented, but do they have to support marshaling across Rails versions?

Yes, we try very hard to make possible that an old Rails version can read data from the new version and also the opposite. This make possible for applications to slowly rollout new versions to production.

[rafaelfranca]

We don't use yarn, we use rdoc, so we need to follow the Rails documentation guideline.

[lulalala]

Everything that was removed needs to be deprecated first. We don't make breaking changes without deprecation, even in major versions.

Cool. I'll add those back then. How about semantic change such as each?

Yes, we try very hard to make possible that an old Rails version can read data from the new version and also the opposite. This make possible for applications to slowly rollout new versions to production.

Got it, but I still have a question. When marshal_load was added (SHA b3dfd7d), I don't see it tried to accommodate Rails 3 or 4, or am I wrong?

[rafaelfranca]

Got it, but I still have a question. When marshal_load was added (SHA b3dfd7d), I don't see it tried to accommodate Rails 3 or 4, or am I wrong?

Usually we only think about backward compatibility between two close release. In that case 5.1 and 5.2. Right now it would be 6.0 and 5.2

[rafaelfranca]

How about semantic change such as each?

Can we introduce a new method with the new semantic and keep the old method with the previous semantic but with deprecation?

[lulalala]

Can we introduce a new method with the new semantic and keep the old method with the previous semantic but with deprecation?

I realized there is a better solution. I check arity of the block passed to each. If arity is 1, behave the new way. We can put deprecation notice when the old way is triggered. Thoughts?
https://github.com/rails/rails/pull/32313/files#diff-fdcf8b65b5fb954372c6fe1ddf284c78R196

[rafaelfranca]

That is a good way. 👍

[lulalala]

I have added back existing methods, some with deprecation warning (free to discuss about which to deprecate). The each now should be compatible with old hash block.

The YAML and marshal dumps are also made compatible with past versions. I added tests for each.

I am starting to look into other tests which broke, and I learned a lot about what interface are needed. For example I added group_by_attribute because it is suitable for many occasions.

I'll update in the future once all tests are fixed. After that I'll add doc and optimize :)

[lulalala]

I think I have fixed almost all tests (the remaining few seems to be sporadic).

I discovered there are many cases of errors[:foo] << 'bar'. We may be able to put a deprecation warning on that, but would involves having a custom array class which overrides << just for that purpose. I hope that's not needed.

Another behavior change is that details will not auto populate with empty array if it is access with a missing key.

If the deprecation proposal here is finalized, I'll start adding code so tests won't have deprecation warning all over the place.

[lulalala]

Hi @tenderlove, we have met in Ruby Kaigi, and you mentioned that for emulated message arrays, I should freeze those so attempts to append to them would fail. This is now added. Cheers! (Sorry I forgot who was sitting next to you who said I can also ping him as well)

[lulalala]

Hi @Larochelle, I see that you made a few changes lately to errors.rb. I am slowly trying to add the change you made to here, but after some digging, I am feeling that maybe we can DRY the logic a bit, as generate_message (here as Error#message and full_message) shares a lot of logic. Would you be interested in working with me on this together? Thanks!

[akostadinov]

Can we deprecate to_xml? I bet is rarely used, and can exists as a gem.

Nobody has an XML API that would return errors? Why need to use a separate gem?

[rafaelfranca]

Because we don't want to maintain something that Rails itself doesn't promote. In the past, we used to promote XML APIs. Those are long gone. We still support you to implement a XML API if you want to, but we will not maintain code to make that happen.

[akostadinov]

I don't understand what does it mean to support a use case without maintaining code to make that happen. Also not sure who that "we" is and whether any formal decisions were made to prevent somebody to re-introduce the to_xml (which is quiet simple as far as I see).

Either way, I assume new projects supporting XML API are a very rare occasion but for sure a lot of apps exist that need to workaround simple things like this when upgrading Rails. In my opinion a respectable app would not be able to drop a whole API without loosing user base. And something so fundamental that was promoted in the past being removed from the framework.. idk, looks wrong to me.

[rafaelfranca]

"We" is the Rails core team, and yes, formal decisions to prevent people to re-introduce to_xml where made.

When I say that "We still support" I mean that Rails still supports that. If you want to generate XML representation of a ActiveModel::Error, that like you said it is quite simple, you can do that in your application, or in a library. Rails will not do it. XML API support was removed in Rails 4.0.

If you want support to to_xml in a ActiveMovel::Error I think I'd be ok to add to https://github.com/rails/activemodel-serializers-xml, just not to Rails itself.