child/grandchild associations reload from db after nested attributes assignment

If I call reload before assigning attributes, it seems that @ship.parts[0].name and @ship.parts.first.name behave the same. You are correct that the issue is related to reloading association before nested attributes assignment.

it is nothing to do with validation failure.

it is due optimization present in nested_attributes.

if you look into nested attributes line 358 (nested_attributes.rb)

      existing_records = if association.loaded?
        association.to_a
      else
        attribute_ids = attributes_collection.map {|a| a['id'] || a[:id] }.compact
        attribute_ids.present? ? association.all(:conditions => {association.primary_key => attribute_ids}) : []
      end

from above code snippet nested attributes will only select objects if the association is not loaded
but partially selecting ids will not cause loaded flag to be set in association_collection

so any subsequent call to association after update_attributes will
load_target will trigger call database wiping out all the association changes except new records line number 391) association_collection.rb

below is 2-3-8 version of load target

      def load_target
          if !@owner.new_record? || foreign_key_present
            begin
              if !loaded?
                if @target.is_a?(Array) && @target.any?
                  @target = find_target + @target.find_all {|t| t.new_record? }
                else
                  @target = find_target
                end
              end
            rescue ActiveRecord::RecordNotFound
              reset
            end
          end

          loaded if target
          target
        end

2-3-stable version fixes this

      def load_target
          if !@owner.new_record? || foreign_key_present
            begin
              if !loaded?
                if @target.is_a?(Array) && @target.any?
                  @target = find_target.map do |f|
                    i = @target.index(f)
                    t = @target.delete_at(i) if i
                    if t && t.changed?
                      t
                    else
                      f.mark_for_destruction if t && t.marked_for_destruction?
                      f
                    end
                  end + @target
                else
                  @target = find_target
                end
              end
            rescue ActiveRecord::RecordNotFound
              reset
            end
          end

thanks for the explanation Subba. That's some complicated code.

I still think that we should not have to reload to get the right behavior.

@part = @ship.parts.create!(:name => "Mast")

From the above code one can't be sure if parts is part of nested_attributes or a simple has_many. However if it is part of nested_attributes then @ship.reset (or something equivalent ) should be called so that later operations do not interfere with already loaded children.

Some discussion regarded nested_attribute took place on #4830 . So just mentioning that ticket here in case someone wants to get full picture.

Forgive me if I missed anything, but the reason you can use methods like #first on a collection association without the full collection being loaded first, is because #first doesn't cache the object. Thus, if its values are updated/marked for destruction, then autosave won't find the object in the ‘target’ when save time comes.

This is why in the tests I explicitly load the target first. (In other tests they used #class to make sure the target was loaded, I opted for the private but more declarative load_target.)

The only way to make using #first, without first loading the complete target, work would be to finally have a full identity map, and NOT replace objects already loaded through #first.

I did make a patch for this once, but since it's not a full identity map it was rejected, which I agreed with in the end. Someone that really needs this should simply sit down and implement a proper identity map.

Finally, autosave does NOT need this behavior for nested attributes, because nested attributes operates on the loaded target, or loads it. It does NOT use #first.

HTH

Attached is a test patch which really tests the intent.

@Eloy I am looking at the tests and it is full of @ship.reload._do_something.

The reason why this ticket was opened gets resolved is user does a model.reload after populating children but before starting nested_attributes activities.

Is there a way to get the whole thing working without forcing one to do reload because while working in rails console this is not the most intuitive thing to do. I am looking into it but so far no luck.

Thanks

@Neeraj Well, I can think of two options. Either create/merge a patch like the one I mentioned earlier: #1751.

Or add something like this to your ~/.irbrc, because I'm assuming that's really the only place one would use #first in combination with autosave:

if defined?(ActiveRecord)
  class ActiveRecord::AssociationCollection
    alias_method :first_before_load_target, :first
    def first(*args)
      load_target
      first_before_load_target(*args)
    end
  end
end

Effectively always loading the target when in the console. (Don't forget to do the same for #last.)

I hope I got the class etc right, I haven't looked at AR internals for a while :)

Thanks Eloy.

I will take a look at #1751 and the other option you suggested. In the meantime wanted to add that without that reload both of the following cases do not work. Just to clear the air that it is not just about 'first'.

puts @ship.parts[0].name
puts @ship.parts.name

@szetobo I believe we have a fix for this issue.

My buddy is going to upload the fix in a moment. However since .first gets the data from the database I would suggest you to update your test. A fixed version might look something like this http://pastie.org/1036943 .

attached is code fix.

This is what is currently happening.

• a new @ship record is created.
• @ship.parts.create(:name => 'part_name') creates a new part in the database. However this @part instance is added to @target. Note that @ship.parts.loaded? is NOT set to true.
• next nested_attributes assignment happens
• rails gets existing_records by pulling data from the database for the @part.id
• next assocation.send(:add_record_to_target_with_callbacks, existing_record) unless association.loaded?
• when above code is executed then the database representation of @part gets added to @target.
• so the end result is that @target ends up with two different instances of same @part.
• everything goes downhill from there

If I explained the problem correctly then fix should be obvious. And that is not to add to @target if @target already has an instance of same @part. My fix replaces the instance held by @target with new instance.

I am attaching patch for both 2-3-stable and rails edge.

Attached is patch which strengthens the test cases further and it has minor code refactoring.

The patch work!! Thanks so much for the prompt responses. It is a show stopper in our project and now it is gone.

I've updated the test case in a way that help me understand the effect of different way to access the data. A funny finding is that even target[0] & target.first is a little bit difference internally, the association @target seems already catered and the result is consistency no matter it is access through [] or .first. In the test case, I added a lot of checking assertion to see when the association get loaded and will the load target respect partial loaded subset produced by nested attributes assignment. It just work as expected.

I agree that the identity map Eloy mention is a better solution as a whole, but since it is not yet there and still sometime before it will be added into the code base, I have to live with a workable solution at this moment.

Looking forward to get this patch commit into the edge master.

Sorry, forgot to include the test case fyr.

Neeraj, your "refactoring" of my changes in load_target doesn't have the same behaviour. If a record is marked for destruction but otherwise unchanged, it should use the newly fetched record and mark that for destruction.

@James I agree that as per my code change the old record would be marked for destruction and not the newly fetched one. However does that really matter? In the end an existing record is marked for destruction and that's what counts. However I am not averse to reverting back to the way code was.

Thanks.

If the changes aren't saved due to a validation error and the form is redisplayed in such a way that you can still see some details about the record to be deleted, I think it would be worthwhile showing the most up-to-date details about that record in case someone else has changed it in the meantime. You may then decide not to delete it after all. Both approaches would work sufficiently but given the choice, I think I prefer this one.

makes sense. I will revert my changes. Thanks for the feedback.

What should I do to make my rails app reference the latest rails source directory, including the patch, instead of the gem installed on my system. Just want to try it out before the patch has been commit into the master head.

(I know it may not be the right place to ask a question like this, but most of the post I've searched seems outdated and not working.)

In your Gemfile comment out the beta reference to rails and refer to git version.

#gem 'rails', '3.0.0.beta4'
gem 'rails', :git => 'git://github.com/rails/rails.git'

Then run following command and you will have rails edge in your vendor/bundler directory.

bundle install vendor --disable-shared-gems

Thanks a lot, really appreciate that.

updating patch with optimized code and test.

I've found another problem when applied the patch into my app. It happens when Parts doesn't change and one of its Trinket has updated, the changes on Trinket will be replaced with db image when part.trinkets reload. That's because load_target of association_collection will only retain @target record when it has been changed, otherwise it will be replaced by a freshly loaded record form db. Doing so will wipe out the next level associations collection, if any, and that's caused the problem.

I've attached a patch with additional fix which alter load_target in order to maintain the loaded record with its unchanged attributes updated by the freshly load instance return by find_target. An additional test case has been added as well to guard against this case.

test "if association is not loaded and child doesn't change and I am saving a grandchild then in memory record should be used" do
  @ship.parts_attributes=[{:id => @part.id,:trinkets_attributes =>[{:id => @trinket.id, :name => 'Ruby'}]}]
  assert_equal 1, @ship.parts.proxy_target.size
  assert_equal 'Mast', @ship.parts[0].name
  assert_equal 1, @ship.parts[0].trinkets.proxy_target.size
  assert_equal 'Ruby', @ship.parts[0].trinkets[0].name
end

During the testing by running my app on top of the rails patch installed into vendor/bundle, I found that rails framework doesn't reload automatically if source code updated. So that a server restart is needed on every code change attempt, anything to do with this situation, or is it a limitation that can't be solved currently?

@szetobo your patch looks good. However I will spend some more time tomorrow testing it.

Could you please attach your patch again in such a manner that you do not include the code provided by Subba. In this way Subba will get credit for the work he has done and you would get credit for the work your work.

Thanks

Here's the patch. Thanks a lot.

Sorry for my careless mistake. Here's another minor fix for excluding "id" when copying attributes in the load_target of association_collection.rb. Otherwise WARNING: Can't mass-assign protected attributes: id will blow up in the log.

I am not able to follow the logic of removing this line

f.mark_for_destruction if t && t.marked_for_destruction?

Please don't remove that line.

I have not run the tests. However if all tests are passing after removing that line the I would add a test for that case.

f.mark_for_destruction is need in original code segment because f will be used instead of t if t && t.changed?. As I stated above, doing so will remove all association collection of t, if any, even if t.changed? is false.

The latest patch will not replace t but will update attributes from db, so if t already mark_for_destruction, it will still marked. For f freshly loaded from db, mark_for_destruction should be cleared. Therefore I just remove this line from my patch. I've run all the test in nested attribute test case and all passed without failure. Please let me know if my understanding is correct or not.

That sounds okay except that marked_for_destruction isn't an attribute, it's an instance variable.

szetobo's changes propogating the changes to destroyed records below test case proves this.
his code is much shorter than current code.

  test "if association is loaded before saving parent destroyed in memory children should be destroyed" do
      @ship.parts_attributes=[{:id => @part.id,:_destroy =>'1'}]
      @part.update_attribute(:name, 'Deck')
      assert @ship.parts.proxy_target[0].marked_for_destruction?
      assert_equal 'Deck', @part.name

      @ship.parts.to_ary

      assert @ship.parts[0].marked_for_destruction?
      assert_equal 'Deck', @part.name

      @ship.save
      assert_nil ShipPart.find_by_id(@part.id)
    end

the last test case patch

ignore the last one here is the better patch

There has been a lot of patches attached to this ticket to the point where it is starting to get a bit confusing.

I have consolidated all the patches and have applied them on my forked branch of rails.

Please take a look at http://github.com/neerajdotname/rails/commits/ticket_5053 . Last 4 committs are related to this ticket.

Please have a look and do chime in with your thoughts.

once again in the forked copy look for branch ticket_5053 .

thanks everyone.

Thanks Neeraj for organize the patches submitted so far. May I know what is missing before the patches will be committed into edge? Thanks again for everyone.

@Eloy Duran and @James Le Cuirot : Could you +1 the work done so far at http://github.com/neerajdotname/rails/commits/ticket_5053 (last 4 comitts) so that we could be sure that patches make sense.

@szetobo once we have the sign off from them then it would be committed to edge.

Sure thing, added comments to each commit.

Sorry if I'm holding this up but I'd like a chance to review this. I'll try to do it tomorrow.

Eloy provided his comments on committs. Tonight I will work on his feedback and will provide a new codebase to look at. So hold your review until I post a new link.

thanks

Feedback provided by Eloy has been incorporated at a new branch http://github.com/neerajdotname/rails/commits/5053_v4 .

Please take a look at last 4 committs in that branch and provide your feedback.

Looks good. Just one note for which I left a comment: http://github.com/neerajdotname/rails/commit/12794678c84185f6a8751e...

Look good, I admitted that assert_no_difference has better semantic than assert_equal.

Neeraj: It doesn't matter you retained me as the author or not, I just want the issue fixed because it is a show stopper to us. I really appreciate the work done so far by rails community. My work just so minor and can't really compare to what I have got from it.

@szetobo I believe we are quite close to finish line :-)

A few more eyes , a few +1 and it will be good to go.

Made one minor comment but okay other than that. I realise now that my earlier comment about marked_for_destruction being an instance variable doesn't actually apply.

Left some feedback on http://github.com/neerajdotname/rails/commit/49cc2d682f17dcf74e11b5....

As a final note, it seems that some of these patches are written because of an issue encountered when using nested attributes. However, they are not specifically about nested attributes. For instance the one mentioned above, which is really about association collection. So I would expect at least a test added to the association collection test, and maybe a regression one to the nested attributes tests. I especially was a bit surprised to find that there are a whole bunch of these ‘lost’ tests in the nested attributes tests.

All tests from line 811 down should not be there at all: http://github.com/neerajdotname/rails/blob/49cc2d682f17dcf74e11b56c...

They should either be in the autosave association tests, or in the association collection tests, as far as I have studied them. Furthermore, they should not use the nested attributes API, as again, it's unrelated and only adds noise.

Having said that, I think the patches can go in. But I would appreciate a test refactoring next, improving the descriptions and moving them to the correct test file.

(from [0057d2df71ec7c2a788acd3b4bade263fc0fe361]) association load target shouldn't replace records from db if it is already loaded by nested attributes assignment

[#5053 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/0057d2df71ec7c2a788acd3b4bade2...

@Neeraj Thanks for your continued work :)

Two tickets have been created as follow up tasks. Please follow these tickets if you are interested.

#5171

#5170

i am attaching 2-3-stable patch. neeraj can you verify it.

Since upgrading to 2.3.9 I have this errors when I try and do some simple things with collections:

NoMethodError: undefined method `keys' for false:FalseClass
    development/ruby/1.8/gems/activerecord-2.3.9/lib/active_record/associations/association_collection.rb:357:in `load_target'
    development/ruby/1.8/gems/activerecord-2.3.9/lib/active_record/associations/association_collection.rb:356:in `tap'
    development/ruby/1.8/gems/activerecord-2.3.9/lib/active_record/associations/association_collection.rb:356:in `load_target'
    development/ruby/1.8/gems/activerecord-2.3.9/lib/active_record/associations/association_collection.rb:353:in `map'
    development/ruby/1.8/gems/activerecord-2.3.9/lib/active_record/associations/association_collection.rb:353:in `load_target'
    development/ruby/1.8/gems/activerecord-2.3.9/lib/active_record/associations/association_collection.rb:149:in `delete_all'
    test/unit/assignment_test.rb:127:in `__bind_1283838340_803300'

TEST CODE

Line # 127: up.assignments.delete_all

MODEL CODE

has_many :assignments, :dependent => :delete_all

Another example that causes the same error in the same place:

return unless self.assignments.any? { |x| x.changes? and x.user_id == current_user.id }

Same exact error... I'm posting to this ticket since the error falls inside it's changes... and this was not an issue on 2.3.8. Seems t.changes is returning true/false instead of a hash?

Ah, changes is reserved (dirty tracking, duh)... and my model has a changes column... hasn't hurt anything until now because I guess Rails itself was not dependent on the changes column and after this update it is.

My problem to go and fix. :)