Rails Ruby on Rails → 2.3.6

Comments and changes to this ticket

• 

  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?

• You flagged this item as spam.
  

  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 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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 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 November 6th, 2009 @ 03:28 PM

  • Assigned user changed from “Michael Koziarski” to “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 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 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...