#1930 autosave should validate associations even if master is invalid - 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.

#1930 ✓resolved

autosave should validate associations even if master is invalid

Reported by Gaspard Bucher | February 10th, 2009 @ 05:25 PM

If both the master record and the autosaved association contain errors, only the errors for the master record will be shown.

Autosave validations should work like normal association validations which are always called whatever state the master is in.

autosave_should_check_associations.diff 3.3 KB

Comments and changes to this ticket

You flagged this item as spam.
Paul Horsfall February 10th, 2009 @ 10:45 PM
Came across the same thing today. The patch works as expected, the errors are now copied across if the parent is invalid. The tests pass for me.

+1

On a related note, since we've copied errors for the attributes from the nested object, do we still need the general 'NestedModel is invalid' error?
You flagged this item as spam.
Gaspard Bucher February 11th, 2009 @ 12:07 PM
To properly fix all this, we need to replace normal validations with our own for each 'autosave' or 'accepted_nested_attribute'. If we keep both validations running side by side, we get many strange things.

Replacing the "validate_associated_records_for_#{association_name}" method seems to be the cleanest way to go.

I tested this and it works fine. It also solves the bad highlighting of multiple errors in nested forms (only first was highlighted, now they are all shown).
- autosave_better_validations.diff 15.5 KB
You flagged this item as spam.
Gaspard Bucher February 11th, 2009 @ 04:31 PM
- Tag changed from “associations, patch” to “associations, autosave, patch”
The patch also solves #1941.
Eloy Duran February 12th, 2009 @ 02:45 PM
I have checked and verified your first patch. (I also added a few more tests for collection associations). +1

I haven't yet had the time to do the same with the second patch.
- merge_validation_errors_onto_parent_even_if_parent_is_invalid.diff 8.2 KB
You flagged this item as spam.
Paul Horsfall February 12th, 2009 @ 05:59 PM
I've tried the second patch and everything looks good when creating nested models through a has many association. When creating a model through a has one association however, I'm now seeing duplicate errors from the nested model. I've not had a chance to demonstrate this in a test but I'll try to do so if I get the time.
You flagged this item as spam.
Gaspard Bucher February 12th, 2009 @ 05:59 PM
In fact the second patch contains tests for collections and is a much better fix: it also solves problems related to newly created records or collections of invalid items.
You flagged this item as spam.
Gaspard Bucher February 12th, 2009 @ 06:00 PM
Paul, I'll try to fix this for has_one associations.
You flagged this item as spam.
Gaspard Bucher February 12th, 2009 @ 09:56 PM
Paul, I cannot reproduce your double validation bug. This test passes:

Pirate has_one ship
```
    @pirate.ship.name = ''
    assert !@pirate.valid?
    assert_equal "can't be blank", @pirate.errors.on(:ship_name)
```
Could you give more details ?
You flagged this item as spam.
Paul Horsfall February 12th, 2009 @ 10:46 PM
My apologies Gaspard. I just got round to looking at this and it turned out I had a validation for an attribute of the same name set on the nested model and the parent and I shouldn't have. This wasn't obvious before your patch because the errors weren't copied over to the parent. After your patch they are copied over and at first glance looked like duplicates.

In short, my bad. Sorry.
Eloy Duran February 13th, 2009 @ 08:10 AM
@Gaspard: I understand what the second patch does, however I need some time to think it over and come up with the right test coverage.

Because actually at some point in the past I used the same kind of code as you are using: http://github.com/alloy/complex-...:L93 But I think this had to do with an issue in the after save callbacks that was recently fixed.

But no sweat, we'll get this fixed :)
You flagged this item as spam.
Gaspard Bucher February 13th, 2009 @ 10:23 AM
Tested second patch on top of latest commit for #1892.

Works fine.
Michael Koziarski February 14th, 2009 @ 02:03 AM
Can you rebase this down into a single patch with a detailed commit message please. Makes it much easier for people tracking down the history of the code :)

Eloy, any other specific comments about this change?
You flagged this item as spam.
Gaspard Bucher February 14th, 2009 @ 06:46 AM
Rebased patch into a single commit. Tried to give a concise message on the problems solved.

Tested on the latest commit (2009-02-14).
- autosave_should_check_validity_on_all_rebased.diff 11.6 KB
Eloy Duran February 15th, 2009 @ 05:58 PM
The main problem atm is that the patch duplicates the add_single_associated_validation_callbacks and add_multiple_associated_validation_callbacks methods. Which is a bit too much code to duplicate imho.

I'm thinking about an alternative, but feel free to rework it in the meantime :-)
Eloy Duran February 15th, 2009 @ 06:04 PM
Just wanted to add that I do in fact have an idea that I'm currently investigating.

Gaspard Bucher February 15th, 2009 @ 07:34 PM

@Eloy: I know that this patch duplicates add_single... and add_multiple.. methods but the only way around would be to ping pong a lot with method chaining which I found even worse...

I still think that defining the validation method is the right path to go (less callbacks during validations, no chance for duplicates).

If you can come up with a better idea that keeps the single validation but avoids the 10 lines duplication that would be really great ! I just doubt it's possible because the patch does not just duplicate these methods.

It's very easy to come up with something like:


def add_multiple_associated_validation_callbacks_with_autosave(reflection)
  if reflection.options[:autosave]
    reflection)
    association_name = reflection.name
    method_name = "validate_associated_records_for_#{association_name}".to_sym
    define_method(method_name) do
      association = association_instance_get(association_name)
      if association
        if new_record? || association.loaded?
          association
        else
          association.target || []
        end.each do |record|
          returning(record.valid?) do |valid|
            record.errors.each do |attribute, message|
              error_name = "#{reflection.name}_#{attribute}"
              errors.add error_name, message unless errors.on(error_name)
            end unless valid
          end
        end
      end
    end
    
    validate method_name
  else
    add_multiple_associated_validation_callbacks_without_autosave(
  end
end
def self.included(base)
  base.class_eval do
    alias_method_chain :add_multiple_associated_validation_callbacks :autosave
  end
end

3 more lines, same logic duplication, one more method chaining. No good.

I was aware of this problem and the patch is the best solution I could come up with.

Eloy Duran February 15th, 2009 @ 08:57 PM
Instead of alias-ing super could probably be used and I agree that going the validate way is a good one.

But I wonder if we should maybe move the automatically validating and saving of new associations completely from Associations into AutosaveAssociation. Micheal what do you think?
You flagged this item as spam.
Gaspard Bucher February 15th, 2009 @ 09:46 PM
Moving all auto-validations in AutosaveAssociation would really make sense.
Michael Koziarski February 15th, 2009 @ 10:16 PM
Sounds good to me, give it a try and see what the patch ends up looking like. Logically it sounds 'tidier', but only the patch will really show how much tidier.
Eloy Duran February 16th, 2009 @ 08:31 AM
Mais oui. I'll probably have something to show tonight.
Eloy Duran February 16th, 2009 @ 08:32 AM
Mais oui. I'll probably have something to show tonight.
Eloy Duran February 17th, 2009 @ 08:32 AM
- Assigned user changed from “Michael Koziarski” to “Eloy Duran”
Eloy Duran February 17th, 2009 @ 09:53 AM
Just an update; Not finished yet, but coming along nicely. The validation part has been merged, I'm working on merging the save part now. Need to merge the inline autosave code from Associations has_one and belongs_to.

http://github.com/alloy/rails/co...
Pratik February 17th, 2009 @ 10:23 AM
- Title changed from “[patch] autosave should validate associations even if master is invalid” to “autosave should validate associations even if master is invalid”
Eloy Duran February 20th, 2009 @ 09:34 AM
Ok, it's almost done, at least in a state which is testable. Because lots has changed I really need feedback from running the patch on real applications.

I'll fix a few other tickets as well this weekend and cleanup and update docs. So this is not the final patch.

@Micheal: There are 2 TODO's in autosave_association.rb, could you comment on those?
- move_default_autosaving_to_autosave_association.diff 49.1 KB
You flagged this item as spam.
Gaspard Bucher February 20th, 2009 @ 02:11 PM
+1

I applied the patch on top of latest edge + support for :inverse in associations.

Works fine. I will keep this patch running as I continue working on zena and I'll let you know if I find something unexpected.
You flagged this item as spam.
pete_lacey February 20th, 2009 @ 05:47 PM
+1

I have the poster child application for testing nested attributes. It uses 27 deeply nested calls to accepts_nested_attributes_for and exercises them all simultaneously in one monster POST. Prior to this patch, there were two strange edge cases that I couldn't narrow down which I worked around manually by assigning :autosave => true to the affected associations. After applying this patch to edge, the issues went away. Have not run it long enough to see if any others have popped up.
Eloy Duran February 20th, 2009 @ 07:51 PM
@Pete: Wow, is that a test application you're saying? If so, is it available online somewhere?

Also, if you would be able to create a test or example of the problem you had, that would make sure we don't introduce a regression at some point.
You flagged this item as spam.
pete_lacey February 20th, 2009 @ 08:46 PM
@eloy: Nope not a test, it's the real thing. Until now, we've been using Ryan's famous approach to multi-model form support, which worked but had a lot of moving parts. I've just ripped all of that out and replaced it with the 2.3 RC1 nested_attributes support. The product's open source (although big and industry specific) and the branch where I'm doing this work can be found here:

https://trisano.csinitiative.net...

(Please excuse some of the amateurish nature of the code, but when we started we knew next to nothing about Rails.)

I will try to create a test example of the two areas that gave me problems, but the funny thing is they were little/no different than the 25 areas that didn't. If it helps you can look in the product's event.rb model. The two associations with nested attributes that did not work were:

has_one :jurisdiction and has_many :place_child_events ...

place_child_events is interesting in a number of ways:
- It's a self-referential association
- The class it references is an STI-enabled parent of the class it's declared on
- The class that exercises nested attributes is another STI parent of this class
- In short: A MorbidityEvent can have many PlaceChildEvents which are both descendants of Event.
- It has a proxy method declared
- The PlaceEvent it refers to has two nested attributes of its own
And the problems was that the participations_place association of the PlaceEvent model was not being exercised by accepts_nested_attributes_for when the participations_place was being updated (it worked fine for new). Also of interest is there's another virtually identical association, contact_child_events, which did not suffer from this problem.

I worked around these issues simply by manually tacking an :autosave => true on both of the troublesome associations. And your recent patch seems to resolve them. I did try to sleuth around the code for a while to isolate the issue, but I don't really know Rails internals and all that meta stuff was making me a bit dizzy.
Michael Koziarski February 22nd, 2009 @ 02:27 AM
Sounds like pete's application is a great little test case for pushing the envelope with this stuff, nice to have you aboard pete. (Also, nice blogging on REST and stuff)

Eloy, I'm not sure that I follow the issue that the TODOs are about. Is it that declaring a callback twice will result in it getting called twice? If so that's at least consistent, but perhaps we could take a look.

Yehuda's got a largely reworked implementation of callbacks, so perhaps we can do it there for 3.0
Michael Koziarski February 22nd, 2009 @ 02:27 AM
- Milestone cleared.
Eloy Duran February 22nd, 2009 @ 04:05 PM
@Pete: Yes your application is definitely a good test case, thanks! About the problem you had, it seems indeed that for some reason :autosave was not set to true by accepts_nested_attributes_for… Don't spend too much time on investigating the cause, it would just have been a nice addition as a regression test :)
Eloy Duran February 22nd, 2009 @ 04:10 PM
@Michael: I'll look a bit further into the callbacks being added to the callback chain multiple times myself. The more important issue for me to cleanup this patch is one that I wrote an email about to the list as well.

http://groups.google.com/group/r...

The line in my patch is the one commented with: "# TODO: Removing this code doesn't seem to matter…" I'm unsure on whether or not I should take it out because it doesn't seem to do anything (so the tests suggest as nothing fails). Or that I should add a test for it.
Eloy Duran February 22nd, 2009 @ 04:10 PM
@Gaspard: Cool beans, thanks for the update. Had missed your message earlier :)
Eloy Duran February 22nd, 2009 @ 06:04 PM
Finished and cleaned up the patch. I'll look into the multiple callback definitions on another occasion.

@Pete & Gaspard: could you please test again, as a fix for #2013 made some additional changes.
- move_default_autosaving_to_autosave_association.diff 53.2 KB
You flagged this item as spam.
Gaspard Bucher February 22nd, 2009 @ 07:45 PM
1+

Great work Eloy !

I applied your latest patch on top of edge (2009-02-22) with the patch for #1619 (support for :inverse in associations). Works like a charm.

I tested it against a simple test app that I created to easily track the problem and against the version of zena that is being migrated to rails 2.x.
Eloy Duran February 22nd, 2009 @ 09:33 PM
Nice, thanks for the quick update Gaspard! :)
You flagged this item as spam.
Gaspard Bucher February 23rd, 2009 @ 01:55 PM
I understand that we do not want to put all found records in @target when running something like:
```
firm.clients.find(...)
```
But I think we should still have a way to update a partial set of records in a single transaction. This means we cannot use "<<". A simple solution would be to explicitly inform about the clients that should be saved later.
```
super_client = firm.clients.find(:first)
firm.clients.will_save(super_client)
# ...
client.name = 'new name'
# ...
firm.save
firm.clients.find(:first).name == 'new name'
# ===> true
```
- will_save.diff 2.3 KB
You flagged this item as spam.
Gaspard Bucher February 23rd, 2009 @ 02:07 PM
PS: the patch above is not meant to be used, it's just an illustration of what would be needed.

It could also be something like
```
firm.clents.push(super_client, false)  # false = do not save now
```
Eloy Duran February 23rd, 2009 @ 02:08 PM
@Gaspard: Could you please file that as a new ticket?
Eloy Duran February 23rd, 2009 @ 02:08 PM
- Assigned user changed from “Eloy Duran” to “Michael Koziarski”
Ready to be applied.
You flagged this item as spam.
pete_lacey February 23rd, 2009 @ 04:51 PM
@eloy. Tested new patch on my app. All looks well. +1
Eloy Duran February 24th, 2009 @ 08:49 AM
@Pete: Thanks.
Eloy Duran February 24th, 2009 @ 08:51 AM
@Gaspard: Ticket #2036 goes into what you're trying to solve. Please join the discussion over there.
You flagged this item as spam.
Paul Horsfall February 24th, 2009 @ 10:51 PM
Tested Eloy's latest patch on top of edge (24/2/2009). Validation of associations now works as I'd expect for the case I originally had problems with - good job!
You flagged this item as spam.
denise February 25th, 2009 @ 11:36 PM
- Assigned user changed from “Michael Koziarski” to “Eloy Duran”
I was having a problem when there were more than two of any kind of new children. The index numbers were being duplicated, so the child records with the duplicate index numbers weren't saved. It seems to be working now. I love this feature.
Eloy Duran February 26th, 2009 @ 12:15 AM
- Assigned user changed from “Eloy Duran” to “Michael Koziarski”

Dmitry Polushkin February 26th, 2009 @ 11:58 PM

Is it a normal behaviour?


@post.author = nil

form_for(@post) do |f|
  f.fields_for(:author) do |af| # raises an exception: The error occurred while evaluating nil.new_record?
  end
end

Can be done through:


  f.fields_for(:author, Author.new unless @post.author)

But may be it's better to auto initialize a new model object in fields_for?

Eloy Duran February 27th, 2009 @ 07:45 AM
@Dmitry Yes, it's supposed to work like that. I'm a bit on the fence about instantiating a new record in fields_for, what if you don't want that? etc. Let's see how it goes and discuss after 2.3 again.
Repository February 27th, 2009 @ 12:50 PM
(from [5cda000bf0f6d85d1a1efedf9fa4d0b6eaf988a1]) Fixed that autosave should validate associations even if master is invalid [#1930 status:committed] http://github.com/rails/rails/co...
Eloy Duran February 27th, 2009 @ 12:55 PM
- State changed from “new” to “resolved”
You flagged this item as spam.
Gaspard Bucher February 27th, 2009 @ 01:13 PM
Hooray ! Thanks for the great work Eloy !
You flagged this item as spam.
Jacob Burkhart March 5th, 2009 @ 10:13 PM
I'm finding that with Rails 2.3 RC2 Active Record is auto-saving way more associations than it used to. And yet ignoring validations on the things it auto-saves. Anybody else seeing this? Test cases and new ticket on the way...
You flagged this item as spam.
Jacob Burkhart March 5th, 2009 @ 10:42 PM
see example of my problems in #2144