This project is archived and is in readonly mode.

#1930 ✓resolved
Gaspard Bucher

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.

Comments and changes to this ticket

  • Paul Horsfall

    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?

  • Gaspard Bucher

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

  • Gaspard Bucher

    Gaspard Bucher February 11th, 2009 @ 04:31 PM

    • Tag changed from associations, patch to associations, autosave, patch

    The patch also solves #1941.

  • Eloy Duran

    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.

  • Paul Horsfall

    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.

  • Gaspard Bucher

    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.

  • Gaspard Bucher

    Gaspard Bucher February 12th, 2009 @ 06:00 PM

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

  • Gaspard Bucher

    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 ?

  • Paul Horsfall

    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

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

  • Gaspard Bucher

    Gaspard Bucher February 13th, 2009 @ 10:23 AM

    Tested second patch on top of latest commit for #1892.

    Works fine.

  • Michael Koziarski

    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?

  • Gaspard Bucher

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

  • Eloy Duran

    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

    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

    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

    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?

  • Gaspard Bucher

    Gaspard Bucher February 15th, 2009 @ 09:46 PM

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

  • Michael Koziarski

    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

    Eloy Duran February 16th, 2009 @ 08:31 AM

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

  • Eloy Duran

    Eloy Duran February 16th, 2009 @ 08:32 AM

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

  • Eloy Duran

    Eloy Duran February 17th, 2009 @ 08:32 AM

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

    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

    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?

  • Gaspard Bucher

    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.

  • pete_lacey

    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

    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.

  • pete_lacey

    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

    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
  • Eloy Duran

    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

    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

    Eloy Duran February 22nd, 2009 @ 04:10 PM

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

  • Eloy Duran

    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.

  • Gaspard Bucher

    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

    Eloy Duran February 22nd, 2009 @ 09:33 PM

    Nice, thanks for the quick update Gaspard! :)

  • Gaspard Bucher

    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
    
  • Gaspard Bucher

    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

    Eloy Duran February 23rd, 2009 @ 02:08 PM

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

  • Eloy Duran

    Eloy Duran February 23rd, 2009 @ 02:08 PM

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

    Ready to be applied.

  • pete_lacey

    pete_lacey February 23rd, 2009 @ 04:51 PM

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

  • Eloy Duran
  • Eloy Duran

    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.

  • Paul Horsfall

    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!

  • denise

    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

    Eloy Duran February 26th, 2009 @ 12:15 AM

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

    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

    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

    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

    Eloy Duran February 27th, 2009 @ 12:55 PM

    • State changed from “new” to “resolved”
  • Gaspard Bucher

    Gaspard Bucher February 27th, 2009 @ 01:13 PM

    Hooray ! Thanks for the great work Eloy !

  • Jacob Burkhart

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

  • Jacob Burkhart

    Jacob Burkhart March 5th, 2009 @ 10:42 PM

    see example of my problems in #2144

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>

Referenced by

Pages