This project is archived and is in readonly mode.

#5053 ✓resolved
szetobo

child/grandchild associations reload from db after nested attributes assignment

Reported by szetobo | July 6th, 2010 @ 12:10 PM | in 2.3.9

When validating fails on updating record by nested attributes, the child/grandchild associations collection will be reload from database.

  @ship = Ship.create!(:name => "The good ship Dollypop")
  @part = @ship.parts.create!(:name => "Mast")
  @trinket = @part.trinkets.create!(:name => "Necklace")

  @ship.attributes = {:name => nil, :parts_attributes => [{:id => @part.id, :name => 'Deck', :trinkets_attributes => [{:id => @trinket.id, :name => "Ruby"}]}]}
  @ship.save

Then

@ship.parts.first.name == "Mast"
@ship.parts.first.trinkets.first.name == "Necklace"

Comments and changes to this ticket

  • Neeraj Singh

    Neeraj Singh July 7th, 2010 @ 04:53 AM

    • State changed from “new” to “open”
    • Tag changed from “nested attributes” to “nested attributes, rails3”
    • Importance changed from “” to “Low”

    This is what's happening based on my analysis. Here is how I setup the models.

    ActiveRecord::Schema.define(:version => 20100706145311) do
  create_table "ship_parts", :force => true do |t|
    t.string  "name"
    t.integer "ship_id"
  end
  create_table "ships", :force => true do |t|
    t.string "name"
  end
end

class Ship < ActiveRecord::Base
  has_many :parts , :class_name => 'ShipPart'
  accepts_nested_attributes_for :parts, :allow_destroy => true

  validates_presence_of :name

  def self.lab
    @ship = Ship.create!(:name => "The good ship Dollypop")
    @part = @ship.parts.create!(:name => "Mast")
    @ship.reload
    @ship.attributes = {:name => nil, :parts_attributes => [{:id => @part.id, :name => 'Deck' }]}
    @ship.save
    puts @ship.parts.first.name
    #puts @ship.parts[0].name
  end

end
class ShipPart < ActiveRecord::Base
  belongs_to :ship
  validates_presence_of :name
end

    First thing is that we need to have @ship.reload. without that @ship thinks that children are already loaded and never really looks into the updated children values.

    Second thing is that when you invoke @ship.parts.first.name rails invokes a query to the database even if @ship.parts is already loaded. In order to inquire about the loaded child you need to do
    puts @ship.parts[0].name .

    And with these two changes a) @ship.reload and b)@ship.parts[0].name you get 'Deck' as the final output.

    I do think that @ship.reload issue should be fixed. Looked at code but not sure where to fix. I am marking this ticket as open because of the reload issue. Someone else might know the fix.

  • szetobo

    szetobo July 7th, 2010 @ 07:44 AM

    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.

  • Subba

    Subba July 7th, 2010 @ 02:00 PM

    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
  • Neeraj Singh

    Neeraj Singh July 7th, 2010 @ 02:14 PM

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

  • Neeraj Singh

    Neeraj Singh July 7th, 2010 @ 02:19 PM

    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.

  • szetobo

    szetobo July 8th, 2010 @ 07:52 AM

    • Title changed from “child/grandchild associations reload from db upon validation error for nested attributes” to “child/grandchild associations reload from db after nested attributes assignment”

    I agree with Neeraj that it should not need to reload before every nested_attributes assignment. It is sometimes very difficult to issue reload statement for every associations as the relationship can be few levels deep involving 10+ associated objects.

    Actually direct update through association proxy should have the same effect as nested_attributes assignment. Should the association be loaded before nested_attributes assignment, i.e. replace the code segment with

    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

    with

    existing_records = (association.loaded? ? association.to_a : association.all)

    Not sure this change has any other side effect but the test case passed without any compliant.

    BTW, when I look carefully into the existing test case about nested_attributes, I find that the use of send(:load_target). It seems solve the problem I'm facing. I just need to make function call as:

    @ship.parts.send(:load_target).first.name
@ship.parts.send(:load_target).first.trinkets.send(:load_target).first.name

    It is a bit clumsy but it works. However :load_target is a protected method, although send() can bypass to invoke it, it seems not a good practice to call it from our application, right?!

  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 11:25 AM

    • Assigned user set to “Neeraj Singh”

    @szetobo thanks for the investigation. Which version of rails you are working with.

    Just to let you know nested_attributes in rails 2.3.8 is totally broken. Also there are unexplained differences between rails edge version and rails 2-3-stable version.

    Just looking at the code it seems some edge cases like new_record etc have been missed out.

    We are in the process of writing detailed test cases for both 2-3-stable and rails edge. And then hopefully the code fix should resolve some of the issues mentioned in this thread.

    Will update this thread when we have more details.

  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 11:34 AM

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

  • szetobo

    szetobo July 8th, 2010 @ 11:52 AM

    • Assigned user cleared.

    I'm working on edge version. Nested attributes for edge version also broken badly. Apps migrate from 2.3.5 just broken without major code changes, especially on nested_attributes part.

    I'll submit another test case shortly about duplicate entry on the association collection with same id right after nested_attributes assignment. Hope that can help a little bit to speed up the debugging process.

    Anyway, thanks for looking into the issue.

  • szetobo

    szetobo July 8th, 2010 @ 11:53 AM

    • Assigned user set to “Neeraj Singh”
  • Eloy Duran

    Eloy Duran July 8th, 2010 @ 12:50 PM

    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

  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 04:11 PM

    Attached is a test patch which really tests the intent.

  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 06:17 PM

    @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

  • Eloy Duran

    Eloy Duran July 8th, 2010 @ 06:31 PM

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

  • Eloy Duran

    Eloy Duran July 8th, 2010 @ 06:32 PM

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

  • Neeraj Singh

    Neeraj Singh July 8th, 2010 @ 07:11 PM

    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
  • Neeraj Singh

    Neeraj Singh July 9th, 2010 @ 03:18 AM

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

  • Subba

    Subba July 9th, 2010 @ 03:32 AM

    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.

  • Neeraj Singh

    Neeraj Singh July 9th, 2010 @ 03:52 AM

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

  • szetobo

    szetobo July 9th, 2010 @ 07:57 AM

    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.

  • szetobo

    szetobo July 9th, 2010 @ 07:57 AM

    Sorry, forgot to include the test case fyr.

  • James Le Cuirot

    James Le Cuirot July 9th, 2010 @ 09:15 AM

    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.

  • Neeraj Singh

    Neeraj Singh July 9th, 2010 @ 10:41 AM

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

  • James Le Cuirot

    James Le Cuirot July 9th, 2010 @ 11:01 AM

    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.

  • Neeraj Singh

    Neeraj Singh July 9th, 2010 @ 11:05 AM

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

  • Neeraj Singh

    Neeraj Singh July 9th, 2010 @ 11:10 AM

    • Milestone set to “3.x”

    attached is modified test patch.

  • szetobo

    szetobo July 10th, 2010 @ 06:13 AM

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

  • Neeraj Singh

    Neeraj Singh July 10th, 2010 @ 08:30 AM

    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
  • szetobo

    szetobo July 10th, 2010 @ 08:40 AM

    Thanks a lot, really appreciate that.

  • Subba

    Subba July 10th, 2010 @ 05:34 PM

    updating patch with optimized code and test.

  • Neeraj Singh

    Neeraj Singh July 10th, 2010 @ 05:44 PM

    • Assigned user changed from “Neeraj Singh” to “José Valim”
    • Tag changed from “nested attributes, rails3” to “nested attributes, patch”
  • szetobo

    szetobo July 11th, 2010 @ 11:28 PM

    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?

  • Neeraj Singh

    Neeraj Singh July 12th, 2010 @ 12:29 AM

    @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

  • szetobo

    szetobo July 12th, 2010 @ 03:11 AM

    Here's the patch. Thanks a lot.

  • szetobo

    szetobo July 12th, 2010 @ 09:37 AM

    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.

  • Neeraj Singh

    Neeraj Singh July 12th, 2010 @ 09:08 PM

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

    f.mark_for_destruction if t && t.marked_for_destruction?
  • James Le Cuirot

    James Le Cuirot July 12th, 2010 @ 09:57 PM

    Please don't remove that line.

  • Neeraj Singh

    Neeraj Singh July 12th, 2010 @ 09:59 PM

    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.

  • szetobo

    szetobo July 13th, 2010 @ 02:42 AM

    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.

  • James Le Cuirot

    James Le Cuirot July 13th, 2010 @ 09:05 AM

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

  • Subba

    Subba July 13th, 2010 @ 11:28 AM

    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
  • Subba

    Subba July 15th, 2010 @ 03:50 AM

    the last test case patch

  • Subba

    Subba July 15th, 2010 @ 04:03 AM

    ignore the last one here is the better patch

  • Neeraj Singh

    Neeraj Singh July 15th, 2010 @ 04:54 AM

    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.

  • szetobo

    szetobo July 15th, 2010 @ 07:38 AM

    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.

  • Neeraj Singh

    Neeraj Singh July 15th, 2010 @ 09:37 AM

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

  • Eloy Duran

    Eloy Duran July 15th, 2010 @ 11:17 AM

    Sure thing, added comments to each commit.

  • James Le Cuirot

    James Le Cuirot July 15th, 2010 @ 11:34 PM

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

  • Neeraj Singh

    Neeraj Singh July 16th, 2010 @ 01:38 AM

    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

  • Neeraj Singh

    Neeraj Singh July 19th, 2010 @ 02:38 AM

    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.

  • Eloy Duran

    Eloy Duran July 19th, 2010 @ 08:57 AM

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

  • Neeraj Singh

    Neeraj Singh July 19th, 2010 @ 08:54 PM

    • Tag changed from “nested attributes, patch” to “nested attributes, rails 3, patch”

    I have pushed a new branch (again) http://github.com/neerajdotname/rails/commits/5053_v5 . This has feedback provided by Eloy.

    Eloy: thanks for all your feedback.

    szetobo: I made two minor changes to the test file you had attached. Both were based on the feedback provided by Eloy. I have retained you as the author of the commit.

    Others please take a look at the above mentioned branch. Let's try to wrap it up soon. :-)

  • szetobo

    szetobo July 20th, 2010 @ 02:42 AM

    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.

  • Neeraj Singh

    Neeraj Singh July 20th, 2010 @ 02:48 AM

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

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

  • James Le Cuirot

    James Le Cuirot July 20th, 2010 @ 10:53 PM

    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.

  • Eloy Duran

    Eloy Duran July 21st, 2010 @ 11:24 AM

    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.

  • Repository

    Repository July 21st, 2010 @ 01:26 PM

    • State changed from “open” to “resolved”

    (from [c0bfa0bfc17f4aa615cd9d1006509e0d84b5692d]) In nested_attributes when association is not loaded and association record is saved and then in memory record attributes should be saved

    [#5053 state:resolved]

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

  • Repository

    Repository July 21st, 2010 @ 01:26 PM

    (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 Singh

    Neeraj Singh July 21st, 2010 @ 01:28 PM

    • State changed from “resolved” to “open”

    http://github.com/neerajdotname/rails/commits/5053_v5 is getting committed. I am going to create two more tickets.

    • for uniq change : In that ticket I would post all the discussions that happened on my personal branch. Do not want to lose the history. And then we will commit the uniq change.

    • second ticket would be to take care of tests refactoring Eloy mentioned.

  • Neeraj Singh

    Neeraj Singh July 21st, 2010 @ 01:29 PM

    • State changed from “open” to “resolved”
  • Eloy Duran

    Eloy Duran July 21st, 2010 @ 02:09 PM

    @Neeraj Thanks for your continued work :)

  • Neeraj Singh

    Neeraj Singh July 21st, 2010 @ 02:52 PM

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

    #5171

    #5170

  • Neeraj Singh

    Neeraj Singh July 21st, 2010 @ 03:15 PM

    • Assigned user changed from “José Valim” to “Neeraj Singh”
    • State changed from “resolved” to “open”
    • Tag changed from “nested attributes, rails 3, patch” to “nested attributes, patch”
    • Milestone changed from “3.x” to “2.3.9”

    Given that nested_attributes is totally broken in 2.3.8 and 2-3-stable has fixes for nested_attributes, I would say that some part of the fix should go to 2-3-stable too. Subba is working on backporting some part of the fix.

    Marking the milestone as 2.3.9 and assigning it to myself until the patch is ready.

  • Subba

    Subba July 23rd, 2010 @ 02:56 AM

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

  • Neeraj Singh

    Neeraj Singh July 23rd, 2010 @ 09:43 PM

    • Assigned user changed from “Neeraj Singh” to “José Valim”

    patch for 2-3-stable looks good. All tests are passing.

    +1

  • Repository

    Repository August 3rd, 2010 @ 09:57 AM

    • State changed from “open” to “resolved”

    (from [12bbc34aca509aba5032e8cc8859ef0c0c845cca]) In nested_attributes when association is not loaded and association record is saved then in memory record attributes should be saved

    [#5053 state:resolved]

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

  • Josh Goebel

    Josh Goebel September 7th, 2010 @ 07:01 AM

    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?

  • Josh Goebel

    Josh Goebel September 7th, 2010 @ 07:23 AM

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

  • Jeff Kreeftmeijer

    Jeff Kreeftmeijer November 8th, 2010 @ 08:54 AM

    • Tag cleared.

    Automatic cleanup of spam.

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>

People watching this ticket

Attachments

Referenced by

Pages