autosave should validate associations even if master is invalid

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?

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

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.

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.

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.

Paul, I'll try to fix this for has_one associations.

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 ?

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.

@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 :)

Tested second patch on top of latest commit for #1892 nested attributes should not have meaningful hash keys.

Works fine.

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?

Rebased patch into a single commit. Tried to give a concise message on the problems solved.

Tested on the latest commit (2009-02-14).

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 :-)

Just wanted to add that I do in fact have an idea that I'm currently investigating.

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

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?

Moving all auto-validations in AutosaveAssociation would really make sense.

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.

Mais oui. I'll probably have something to show tonight.

Mais oui. I'll probably have something to show tonight.

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

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?

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

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

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

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

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

@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 :)

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

@Gaspard: Cool beans, thanks for the update. Had missed your message earlier :)

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 Autosave associations don't create nested models without validation made some additional changes.

1+

Great work Eloy !

I applied your latest patch on top of edge (2009-02-22) with the patch for #1619 Support for :inverse option in associations (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.

Nice, thanks for the quick update Gaspard! :)

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

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

@Gaspard: Could you please file that as a new ticket?

@eloy. Tested new patch on my app. All looks well. +1

@Pete: Thanks.

@Gaspard: Ticket #2036 Linking and unlinking via accepts_nested_attributes_for goes into what you're trying to solve. Please join the discussion over there.

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!

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?

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

(from [5cda000bf0f6d85d1a1efedf9fa4d0b6eaf988a1]) Fixed that autosave should validate associations even if master is invalid [#1930 autosave should validate associations even if master is invalid status:committed] http://github.com/rails/rails/co...

Hooray ! Thanks for the great work Eloy !

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

see example of my problems in #2144 Don't autosave new_record associations