#3147 ActiveRecord::Error#full_message broken when used with I18n and autosave associations - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#3147 ✓resolved

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

Reported by Anders Carling | September 4th, 2009 @ 04:33 PM

ActiveRecord::Schema.define do
  create_table(:owners) { |o| }
  create_table(:items)  { |i| i.string :property }
end
class Owner < ActiveRecord::Base
  has_one :item, :autosave => true
  def self.human_attribute_name(attr)
    puts "Owner received: #{attr}"
    super
  end
end

class Item < ActiveRecord::Base
  validates_presence_of :property
  def self.human_attribute_name(attr)
    puts "Item received: #{attr}"
    super
  end
end
owner = Owner.new
owner.build_item
owner.valid?
owner.errors.full_messages

The above code will using Rails 2.3.4 output:
Item received: item_property

On 2.3.3 it would however output:
Owner received: item_property

I.e. the attribute name for the error message caused by validates_presence_of :property and copied to owner.errors by ActiveRecord::AutosaveAssociation now get it's human_attribute_name from the child object (Item) instead of the parent (Owner).

This changes the I18N key used to find the attribute name from 'activerecord.attributes.owner.item_property' to 'activerecord.attributes.item.item_property', which breaks any locales depending on the previous implementation.

I tracked it down to the changes caused by the new ActiveRecord::Error class introduced in 13fb26b714dec0874303f51cc125ff62f65a2729. As the new Error#generate_full_message calls human_name and human_attribute_name on its @base.class and thus will get them from the child class (Item). To avoid that we could set @base to the parent instance when we're copying the error to the parent's Errors instance.

That copying is done in ActiveRecord::AutosaveAssociation#association_valid? Something like this should restore the previous behaviour:
(Lighthouse doesn't like me pasting in patches, you'll find the code in an attached file below.)

I'm not sure about this (and the rewriting of error.attribute) as a long term solution, as I feel the Error should probably keep its connection to the object that actually caused it, but for now this should restore compatibility with the way it worked in 2.3.3.

Comments and changes to this ticket

You flagged this item as spam.
Anders Carling September 4th, 2009 @ 04:37 PM
Oops, didn't realise lighthouse wouldn't like the patch text. The same text is in the attached file
- autosave_association_assocation_valid.patch 498 Bytes
José Valim September 5th, 2009 @ 07:08 PM
Anders, do you mind writing a test to ensure it won't happen again?
You flagged this item as spam.
Anders Carling September 6th, 2009 @ 01:11 AM
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.
José Valim September 6th, 2009 @ 01:17 AM
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!
You flagged this item as spam.
Anders Carling September 6th, 2009 @ 01:46 AM
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.
José Valim September 6th, 2009 @ 01:55 AM
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! :)
You flagged this item as spam.
Anders Carling September 8th, 2009 @ 12:12 AM
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?
José Valim September 8th, 2009 @ 12:35 AM
So I guess the solution would really be to apply the ticket #2904. 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?
You flagged this item as spam.
Anders Carling September 8th, 2009 @ 11:08 AM
It seems to me like #2904 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 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.
José Valim September 8th, 2009 @ 11:11 AM
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.
You flagged this item as spam.
Anders Carling September 8th, 2009 @ 11:33 AM
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 or keep it along until the issue is fixed?
José Valim September 8th, 2009 @ 01:13 PM
- Milestone set to “2.3.4”
- Assigned user set to “José Valim”
I will let this one opened until the other one get applied.
Jeremy Kemper September 11th, 2009 @ 11:04 PM
- Milestone changed from “2.3.4” to “2.3.6”
[milestone:id#50064 bulk edit command]
José Valim September 22nd, 2009 @ 01:48 PM
- Assigned user cleared.
Anders, I'm attaching a patch for this bug, since we could not have an agreement to solve #2904.
- 0001-error-messages-nested-attributes.diff 7.5 KB
You flagged this item as spam.
Anders Carling September 25th, 2009 @ 10:37 AM
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 in the future.
José Valim September 25th, 2009 @ 11:42 PM
- Tag changed from “2-3-stable, active_record, autosave_association, bug, i18n” to “2-3-stable, active_record, autosave_association, bug, bugmash, i18n”
You flagged this item as spam.
Lawrence Pit September 27th, 2009 @ 08:34 AM
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}"
```
CancelProfileIsBroken September 27th, 2009 @ 11:59 AM
- Tag changed from “2-3-stable, active_record, autosave_association, bug, bugmash, i18n” to “2-3-stable, active_record, autosave_association, bug, bugmash-review, i18n”
- Milestone cleared.
José Valim October 18th, 2009 @ 03:55 AM
Lawrence, I didn't add that because I never had to use it. :) If someone needs such feature, another patch can be done.
Repository October 28th, 2009 @ 06:46 PM
- State changed from “new” to “resolved”
(from [f5f7c40f3aedf88b2aef4e83602a4f41ffa5d0ab]) Fix nested attributes error messages which is broken in 2.3.4. It still copies the message from child to parent, but does the lookup in the child, not in the parent, avoiding error messages duplication (as happened in 2.3.3). [#3147 state:resolved]

Signed-off-by: Joshua Peek josh@joshpeek.com
http://github.com/rails/rails/commit/f5f7c40f3aedf88b2aef4e83602a4f...
You flagged this item as spam.
Chris Hapgood November 7th, 2009 @ 08:49 PM
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).
José Valim November 8th, 2009 @ 01:58 AM
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. :)
José Valim November 10th, 2009 @ 01:20 PM
@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.
Rizwan Reza May 15th, 2010 @ 06:40 PM
- Tag changed from “2-3-stable, active_record, autosave_association, bug, bugmash-review, i18n” to “2-3-stable, active_record, autosave_association, bug, i18n”