ActiveRecord::Error#full_message broken when used with I18n and autosave associations

Oops, didn't realise lighthouse wouldn't like the patch text. The same text is in the attached file

Anders, do you mind writing a test to ensure it won't happen again?

Had a quick look at a test yesterday but didn't have time to write anything then, wasn't really sure about how to go about it and where to place it (first time really messing around inside the activerecord code). I think I could have another look at it tomorrow.

Should I target the test patch against master or 2-3-stable? There seems to be some changes among the tests (new and restructured files and such) in master, not sure if it even affects the autosave_association-tests that much though.

Anders, the functionality is definitely available in ActiveRecord::Error, but is not being used properly by autosave association. So I would say that the test should be added in autosave_association, which is likely to be the same in both 2-3-stable and master.

Thanks for working on this!

Do you think the test should be "low level" and just check that #base is changed on the copied Error or should I write something "high level" that checks if the correct I18n-key is being used?

I feel the low level variant really test the stuff that AutosaveAssociation is responsible to do and leaves the ActiveRecord::Error responsible for what it does with the information it has. Still, the high level test has more of a "test what the user/developer cares about" feel to it, but it covers a lot of different modules and their responsibilities in one go. I'm not sure what kind of tests you're generally aiming for in that regard?

Thanks for your tips, I might have a lot of question for just a one line patch but as I said, I'm still trying to find my way around the rails codebase and I'm very grateful for any advice.

I think checking for error.base would be too tied to the implementation. If we change the API again, someone might just remove the API test and we would be "unprotected" again. So I suggest a "full stack" test. :)

No problem! Glad to help! :)

Just when I found some time to write the test I found another, related, issue in that the changing of @base, and indeed the changing of @attribute that is already done, breaks Error#value's ability to get the value for the erroneous attribute. This breaks the ability for the attributes values to be interpolated in translations strings (#value is called from both #generate_full_message and #generate_message). The easiest way to solve it would probably be to preload the attribute value in Error#initialize and make Error#value an attr_accessor.

def value
  @base.respond_to?(attribute) ? @base.send(attribute) : nil
end

So, currently, by fixing the first issue (previous patch, upcoming test) we would make Error#value even more unable to get the attribute value, and fixing something by making something else even more broke doesn't quite feel right.

For now will I go ahead and fix this issue by changing error.base and create a separate ticket for #value, but maybe it'd be a good idea to look at a better solution to contain AutosaveValidation specific error logic that would keep a reference to the real base and attribute while allowing I18n lookups on the parent class?

So I guess the solution would really be to apply the ticket #2904 Avoid copying errors from child to parent on autosave. It would solve both cases. I was planning to do it only on master, but I guess a backport will be needed. What do you think?

It seems to me like #2904 Avoid copying errors from child to parent on autosave is the way to go, but as far as I could tell it would still break I18n lookup as we knew it in 2.3.3, but since it's already changed in 2.3.4 maybe that isn't that big of a problem?

2.3.3 uses activerecord.attributes.parentmodel.childmodel_attribute
2.3.4 uses activerecord.attributes.childmodel.childmodel_attribute
#2904 Avoid copying errors from child to parent on autosave seems to use activerecord.attributes.childmodel.attribute

The way 2.3.4 currently does it is just weird. In my opinion childmodel.attribute is the most logical way to do this but one loses some of the flexibility that parentmodel.childmodel_attribute provides for those few (I can't think of any?) cases where you'd like to give a column on the associated model a completely different name if it's presented as an association then if it's presented alone.

Other than that it seems to me like the patch is backwards compatibile with everything written for 2.3.* as it concats association errors that's not specified in options[:associations] to the parents full_messages in Errors#include_association_errors, right?

To backport 0002_add_nested_errors_support.diff to 2-3-stable I think you'll need to add support for nested Errors objects in Errors#each (which will fail calling Errors#message) and maybe add some documentation on the fact that Errors#each_error might yield an Errors object and Errors#each_full might yield an hash (with full messages for an association). Hopefully that was of some help.

Exactly. Everything you said is right.

Indeed we would lose the flexibility that "parentmodel.childmodel_attribute" but so far it proved more to be a pain than a feature. If we need to manipulate a specific message, I guess we could do that with AR callbacks.

Aye, sounds good.

Then I'll drop my changes and wait for your patch to be backported (don't think I've enough time the next few days to look at it myself, sorry).

Shall we close this ticket as a duplicate of #2904 Avoid copying errors from child to parent on autosave or keep it along until the issue is fixed?

Great, the patch looks good to me. Hugely appreciated.

In my opinion it also puts us in a slightly better position to start looking at something in the spirit of #2904 Avoid copying errors from child to parent on autosave in the future.

I applied the patch and tested it. It looks to me. So +1

One thing though: currently the relationship of the Error object with the specific association is removed. I think this is a loss. E.g., if a pirate has_many :books and has_many :special_books, and the error is generated on the :special_books collection, then the Error object should know that for i18n purposes.

My suggestion is that autosave_association sets an +association+ attribute on the dup'ed Error object before adding it. In +generate_message+ you'd then also have this key:

  :"models.#{klass.name.underscore}.association.#{association}.attributes.#{attribute}.#{message}"

Lawrence, I didn't add that because I never had to use it. :) If someone needs such feature, another patch can be done.

This commit (f5f7c40f3aedf88b2aef4e83602a4f41ffa5d0ab) introduces a new limitation for ActiveRecord::Base instances that has (to my knowledge) never been discussed.

Briefly: an error on an attribute cannot be set as the result of an exception computing the value of the attribute.

For example:

  def v
    compute_it
  rescue
    errors.add(:v, "Dadgummit")
  end

After the aforementioned commit, when compute_it raise an exception, ActiveRecord::Error#initialize references v, which in turn raises another exception, which adds another error, which in turn proves that my stack is not infinitely deep.

Is it reasonable to assume that ActiveRecord::Error instances can reference its erroneous attribute's value? If so, the above pattern won't work and instead a sentinel value will need to be set (U-G-L-Y).

Hey @Chris, I cannot understand why exactly this error happens. Could you please provide a failing test so I can provide a fix?

Feel free to open a new ticket and assign it to me. :)

@Chris, I debugged a little bit. This error happens since ActiveRecord::Error was added and not after this commit. I created a new ticket with this issue at #3477 ActiveRecord::Error does not allow values to add errors.