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

Model error as object #32313

Merged
merged 22 commits into from Apr 24, 2019

Conversation

@lulalala
Copy link
Contributor

commented Mar 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

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

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

@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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

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.

activemodel/lib/active_model/error.rb Outdated

module ActiveModel
# Represents one single error
# @!attribute [r] base

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Mar 21, 2018

Member

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

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

That is a good way. 👍

@lulalala lulalala force-pushed the lulalala:model_error_as_object branch Mar 26, 2018

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

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 lulalala force-pushed the lulalala:model_error_as_object branch 2 times, most recently Apr 3, 2018

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

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 lulalala force-pushed the lulalala:model_error_as_object branch to 504b22b May 3, 2018

@richardvenneman richardvenneman referenced this pull request May 8, 2018

Merged

Fix style offences #1

@lulalala lulalala force-pushed the lulalala:model_error_as_object branch May 9, 2018

@lulalala lulalala force-pushed the lulalala:model_error_as_object branch to fe13e6c Jun 28, 2018

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2018

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 lulalala force-pushed the lulalala:model_error_as_object branch 2 times, most recently from 72db2d6 to 8344612 Sep 24, 2018

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2018

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!

@lulalala lulalala force-pushed the lulalala:model_error_as_object branch from 8344612 to 5a1ec44 Sep 29, 2018

@lulalala lulalala force-pushed the lulalala:model_error_as_object branch from 5a1ec44 to 4719754 Sep 29, 2018

@lulalala lulalala force-pushed the lulalala:model_error_as_object branch from 8da62d8 to 5e24c33 Mar 31, 2019

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

@rafaelfranca (apologies for another ping) I've rewrite the code, so the rendering are all done using existing full_message and generate_message methods. This means all the edge cases you mentioned should be fixed. The code size also shrunk to be less than 1000 lines. If you have time, could you try rerun this against your tests please? Thanks.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Sure! Thank you for rewriting the patch. I'd also like to say that I do think this change is a good change and I'd love to merge it , I just not sure if I'd like to merge it right now close to release. I'll test it again today to see how many things are still failing and if I find any add tests cases to our test suite so we can work on it. If we need to schedule a meeting I'll let you know. Also note that you commented in the issue during a weekend and I only look on GitHub during work hours so I'm sorry it took so long to reply.

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@rafaelfranca thanks, and no problem about not replying during weekends. I just wanted to update my dev status.

However, I must give my honest feeling: I am quite depressed about this.

Can I at least request for one thing though. Could this be put on a much higher priority after 6.0 is released? The lack of feedback loop was really stressing me out during the past year. I have spent 160 hours on this, and 50 more hours just trying to resolving conflicts. Sorry to break this to you, but I feel really unfair seeing you merge other people's work within one day, while I have to spend more hours resolving conflicts.

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Not using this as excuse for the delay but some PRs are easier to merge than others. Yeah, sure, this PR is my priority. I'm being running it on shopify's main application and fixing all tests I see failing there to make sure this PR doesn't have any breaking change.

@rafaelfranca rafaelfranca modified the milestones: 6.0.0, 6.1.0 Apr 22, 2019

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

I just tested it again and a few tests were still failing in my app. I'll bump this to 6.1 but I promise it will be the first PR I'll merge when we start the 6.1 development.

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@rafaelfranca thanks heaps! Let me know what the failures are and I will try fixing those.

@rafaelfranca rafaelfranca merged commit d4d145a into rails:master Apr 24, 2019

3 checks passed

buildkite/rails Build #59993 passed (19 minutes, 33 seconds)
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

As promised, First PR to be merged after the development of 6.1. Thank you so much for you hard work.

@rafaelfranca
Copy link
Member

left a comment

@lulalala Now that this is merged, can you work on whiting a CHANGELOG entry and improving the documentation around this change?

I usually ask this to be done in the same PR but since you already waited so long to get this merged I think it is better to do a follow up.

true
end

def strict_match?(attribute, type, **options)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 24, 2019

Member

Do we want this to be part of the public API?

@lulalala

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@rafaelfranca thanks so much, as I didn't expect it to be literally the first one to merge. I'll do the followup this weekend.

tagliala added a commit to DavyJonesLocker/client_side_validations that referenced this pull request Apr 25, 2019

@Drenmi

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Looking forward to this improvement! ❤️ Thanks for your hard work @lulalala and @rafaelfranca. 🙇

@olimart

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Thank you @lulalala & @rafaelfranca 👏

jonathanhefner pushed a commit to jonathanhefner/rails that referenced this pull request May 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.