This project is archived and is in readonly mode.

#3355 ✓resolved
Tom Stuart

Validation of autosaved associations is broken

Reported by Tom Stuart | October 8th, 2009 @ 04:27 PM | in 2.3.6

This commit from #3161 broke validation of autosaved associations in 2-3-stable (and I assume, but haven't verified, that this broke it in master too).

It's not okay to make this decision in add_autosave_association_callbacks because it's too early: the contents of reflection.options may be different at validation time.

For example, the association may not be declared as autosaved, but may have :autosave => true set by a subsequent call to accepts_nested_attributes_for. In fact the tests explicitly cover this by saying has_one :ship and belongs_to :pirate (in Pirate and Ship respectively) without explicitly setting :autosave, so that accepts_nested_attributes_for can set it later (there's even a comment explaining this); the commit in question, having broken this behaviour, simply adds :validate => true to the declaration of these associations for no reason other than to make the tests pass again.

Comments and changes to this ticket

  • Eloy Duran

    Eloy Duran October 8th, 2009 @ 07:14 PM

    At first I had indeed implemented it so that one could change the option on the reflection and it would automatically be enabled. But the speed improvement of not doing that was big enough for me to decide against it.

    We could add methods that one can call to enable validation, but it would need some thought. So, what's your use case?

  • Tom Stuart

    Tom Stuart October 8th, 2009 @ 07:46 PM

    I'm sorry, I don't quite understand: I'm reporting a regression, not requesting a feature. Autosaved associations should be validated, and accepts_nested_attributes_for automatically enables autosave for its associations. It's what the tests were doing before this change: declaring an association, calling accepts_nested_attributes_for on that association (as in Pirate and Ship), and then expecting that association to behave like any other autosaved association (as in test_should_automatically_validate_the_associated_model, test_should_merge_errors_on_the_associated_model_onto_the_parent_even_if_it_is_not_valid etc).

    The change in #3161 leaves accepts_nested_attributes_for associations, or any other associations which have been set to autosave after their initial declaration, in a weird halfway state whereby they do still get autosaved (because autosaving is triggered by the :autosave option at save time) but don't get validated (because validation is triggered by the :autosave option at declaration time). And as the ActiveRecord::AutosaveAssociation docs say:

    Validation is performed on the parent as usual, but also on all autosave enabled associations. If any of the associations fail validation, its error messages will be applied on the parents errors object and validation of the parent will fail.

    For example, any applications which were relying on the correct behaviour of accepts_nested_attributes_for, i.e. enabling both autosaving and validation, will now be broken. So I guess that's a use case.

  • Eloy Duran

    Eloy Duran October 8th, 2009 @ 10:39 PM

    • Milestone set to 2.3.6

    Ah sorry, I read too fast.

    It's not okay to make this decision in add_autosave_association_callbacks because it's too early: the contents of reflection.options may be different at validation time.

    I thought you meant you meant you were changing the reflection as part of some meta-programming.

    I now see what you mean: the association is defined with or without validation before accepts_nested_attributes_for might be called later on, for which you do want validation.

    This is a problem indeed, I'll have a look at a proper fix next week.

  • Tom Stuart

    Tom Stuart October 19th, 2009 @ 12:48 PM

    Ping.

    For now it'd be better just to revert this commit than to leave autosaved associations broken in 2-3-stable. Any regression-free performance improvements can be applied later.

  • Chris Hapgood

    Chris Hapgood November 3rd, 2009 @ 10:17 PM

    I agree with Tom that the commit in question should be reverted. And I'll go one step further: I've got several models with before_validation callbacks that are not longer being invoked. Optimizing the validations into nothingness is one thing, but skipping the validation callbacks is a whole new layer of surprise.

  • Tom Stuart

    Tom Stuart November 6th, 2009 @ 03:04 PM

    • Assigned user changed from “Eloy Duran” to “Michael Koziarski”

    Koz, can you help? This seems to have gone cold.

  • Manfred Stienstra

    Manfred Stienstra November 6th, 2009 @ 03:27 PM

    I don't think the best course of action is reverting the commit but instead fixing the shortcomings of the speedup.

    Tom and Chris, it would be great if you could supply a patch for that.

  • Eloy Duran

    Eloy Duran November 6th, 2009 @ 03:28 PM

    • Assigned user changed from “Michael Koziarski” to “Eloy Duran”
  • Eloy Duran

    Eloy Duran November 6th, 2009 @ 11:02 PM

    • State changed from “new” to “verified”

    Fixed in our repo, which will be merged in a short while.

    http://github.com/Fingertips/rails/commit/6b2291f33063b6742cba84f5f...

  • Repository

    Repository November 8th, 2009 @ 10:17 PM

    • State changed from “verified” to “resolved”

    (from [6b2291f33063b6742cba84f5f64e03de9907c1f8]) Define autosave association callbacks when using accepts_nested_attributes_for.

    This way we don't define all the validation methods for all associations by
    default, but only when needed.

    [#3355 state:resolved] http://github.com/rails/rails/commit/6b2291f33063b6742cba84f5f64e03...

  • Repository

    Repository November 8th, 2009 @ 10:17 PM

    (from [f125a34501e21b1e0da2b80d149df7a739482804]) Define autosave association callbacks when using accepts_nested_attributes_for.

    This way we don't define all the validation methods for all associations by
    default, but only when needed.

    [#3355 state:resolved] http://github.com/rails/rails/commit/f125a34501e21b1e0da2b80d149df7...

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Referenced by

Pages