This project is archived and is in readonly mode.

#5137 ✓resolved
hjdivad

[PATCH] has_one :through through belongs_to associations does not build through record when assigning the has_one record

Reported by hjdivad | July 17th, 2010 @ 04:17 AM | in 3.x

The following test should illustrate the problem:

    class Minivan < ActiveRecord::Base
      set_primary_key :minivan_id

      belongs_to :speedometer
      has_one :dashboard, :through => :speedometer
    end


    def test_creating_association_builds_through_record_for_new_through_belongs_to
        new_minivan = Minivan.new
        assert_nothing_raised do
            new_minivan.dashboard = dashboards( :cool_first )
        end
        assert new_minivan.speedometer
        assert_equal dashboards( :cool_first ), new_minivan.speedometer.dashboard
        assert_equal dashboards( :cool_first ), new_minivan.dashboard
        assert new_minivan.save
        assert_equal dashboards( :cool_first ), new_minivan.dashboard
    end

There is a patch at http://github.com/hjdivad/rails/tree/issue-5137, or see
the attached file.

Comments and changes to this ticket

  • hjdivad

    hjdivad July 17th, 2010 @ 04:20 AM

    • no changes were found...
  • hjdivad

    hjdivad July 17th, 2010 @ 04:26 AM

    The above test fails in the block because the association proxy is nil, because
    of the following code:

        # 38f1ea8fe267947f4baf832d3b15ee5b83f18f71
        # associations.rb:1364
        if retval.nil? and association_proxy_class == BelongsToAssociation
            association_instance_set(reflection.name, nil)
            return nil
        end
    

    I believe the motivation for the above code was to handle the case exemplified
    by the following test:

        # 38f1ea8fe267947f4baf832d3b15ee5b83f18f71
        # belongs_to_associations_test.rb:305
        def test_forgetting_the_load_when_foreign_key_enters_late
            c = Client.new
            assert_nil c.firm_with_basic_id
    
            c.firm_id = 1
            # sometimes tests on Oracle fail if ORDER BY is not provided therefore add always :order with :first
            assert_equal Firm.find(:first, :order => "id"), c.firm_with_basic_id
        end
    
  • hjdivad

    hjdivad July 17th, 2010 @ 04:27 AM

    In case it's not clear, the patch handles the test_forgetting_the_load_when_foreign_key_enters_late test (as well as the rest of the activerecord test suite, at least in my environment :-).

  • Repository

    Repository July 26th, 2010 @ 04:50 PM

    • State changed from “new” to “resolved”

    (from [0e969bdaf8ff2e3384350687aa0b583f94d6dfbc]) fix escaping id and parameters in path [#5137 state:resolved]

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

  • hjdivad

    hjdivad July 26th, 2010 @ 06:38 PM

    I'm fairly sure the linked commit actually resolves 5173, and not this bug.

  • José Valim

    José Valim July 26th, 2010 @ 06:42 PM

    • State changed from “resolved” to “new”
    • Importance changed from “” to “Low”

    Sorry, my bad. :)

  • hjdivad
  • Zachery Hostens

    Zachery Hostens August 31st, 2010 @ 05:02 PM

    created another ticket in regards to querying a has_one :through through a belongs_to. figured id make it separate since this ticket is about creating the records.

    #5512 i also have a test appended.

  • Neeraj Singh

    Neeraj Singh August 31st, 2010 @ 10:04 PM

    I am not able to reproduce this problem with rails master. This is what I did.

    require 'test/unit'
    require 'test/unit/assertions'
    class Object
      include Test::Unit::Assertions
    end
    
    
    ActiveRecord::Schema.define(:version => 20100511140856) do
    
      create_table "dashboards", :force => true do |t|
        t.string "name"
      end
    
      create_table "minivans", :force => true do |t|
        t.string "name"
      end
    
      create_table "speedometers", :force => true do |t|
        t.string  "name"
        t.integer "minivan_id"
        t.integer "dashboard_id"
      end
    
    
    end
    class Dashboard < ActiveRecord::Base
    end
    
    class Minivan < ActiveRecord::Base
    
      has_one :speedometer
      has_one :dashboard, :through => :speedometer
    
      def self.lab
        minivan = Minivan.new(:name => 'm1')
        dashboard = Dashboard.create(:name => 'd1')
    
        minivan.dashboard = dashboard
    
        assert minivan.speedometer
        assert_equal dashboard, minivan.dashboard
        minivan.save
        assert_equal dashboard, minivan.dashboard
      end
    
    end
    
    class Speedometer < ActiveRecord::Base
      belongs_to :minivan
      belongs_to :dashboard
    end
    
    > Minivan.lab does not fail for me.
    
  • Neeraj Singh

    Neeraj Singh August 31st, 2010 @ 10:13 PM

    Ok. The test provided in #5512 does fail for me. Looking into it.

  • hjdivad

    hjdivad September 1st, 2010 @ 12:08 AM

    Neeraj,

    Thank you for looking into this.

    Your code above does not test the problem: there, you have a has_one (dashboard)
    through a has_one(speedometer).

    If you change Minivan so it belongs_to(speedometer), instead of has_one, then
    your test will exhibit the same failure.

        [davidjh@nyx ((no branch); ruby-1.9.2-p0)] ~/devel/foss/rails/activerecord
        $ ruby --version
        ruby 1.9.2p0 (2010-08-18 revision 29036) [x86_64-linux]
        [davidjh@nyx ((no branch); ruby-1.9.2-p0)] ~/devel/foss/rails/activerecord
        $ git rev-parse HEAD
        8b2e4fddbf7e9d204579cd87ac72f14bc592d5a5
        [davidjh@nyx ((no branch); ruby-1.9.2-p0)] ~/devel/foss/rails/activerecord
    
        # cat test/cases/associations/issue_5137.rb
        require 'test/unit'
        require 'test/unit/assertions'
        require 'test/cases/helper'
        class Object
          include Test::Unit::Assertions
        end
    
    
        ActiveRecord::Schema.define(:version => 20100511140856) do
    
          create_table "dashboards", :force => true do |t|
            t.string "name"
          end
    
          create_table "minivans", :force => true do |t|
            t.string "name"
          end
    
          create_table "speedometers", :force => true do |t|
            t.string  "name"
            t.integer "minivan_id"
            t.integer "dashboard_id"
          end
    
    
        end
        class Dashboard < ActiveRecord::Base
        end
    
        class Minivan < ActiveRecord::Base
    
          set_primary_key :minivan_id
    
    
          belongs_to :speedometer
          has_one :dashboard, :through => :speedometer
    
          def self.lab
            minivan = Minivan.new(:name => 'm1')
            dashboard = Dashboard.create(:name => 'd1')
    
            minivan.dashboard = dashboard
    
            assert minivan.speedometer
            assert_equal dashboard, minivan.dashboard
            minivan.save
            assert_equal dashboard, minivan.dashboard
          end
    
        end
    
        class Speedometer < ActiveRecord::Base
          belongs_to :minivan
          belongs_to :dashboard
        end
    
    
        class MinivanTest < ActiveRecord::TestCase
          def test_minivan_lab
            Minivan.lab
          end
        end
    
        [davidjh@nyx ((no branch); ruby-1.9.2-p0)] ~/devel/foss/rails/activerecord
        $ ruby -I'test' -I'test/connections/native_sqlite3/' -I'.' test/cases/associations/issue_5137.rb
        Using native SQLite3
        -- create_table("dashboards", {:force=>true})
           -> 0.0052s
        -- create_table("minivans", {:force=>true})
           -> 0.0273s
        -- create_table("speedometers", {:force=>true})
           -> 0.0055s
        -- initialize_schema_migrations_table()
           -> 0.0014s
        -- assume_migrated_upto_version(20100511140856, "db/migrate")
           -> 0.0003s
        Loaded suite test/cases/associations/issue_5137
        Started
        E
        Finished in 0.057919 seconds.
    
          1) Error:
        test_minivan_lab(MinivanTest):
        NoMethodError: undefined method `build' for nil:NilClass
            /home/davidjh/devel/foss/rails/activerecord/lib/active_record/associations/has_one_through_association.rb:27:in `create_through_record'
            /home/davidjh/devel/foss/rails/activerecord/lib/active_record/associations/has_one_through_association.rb:10:in `replace'
            /home/davidjh/devel/foss/rails/activerecord/lib/active_record/associations.rb:1474:in `block in association_accessor_methods'
            test/cases/associations/issue_5137.rb:42:in `lab'
            test/cases/associations/issue_5137.rb:60:in `test_minivan_lab'
            /home/davidjh/.rvm/gems/ruby-1.9.2-p0/gems/mocha-0.9.8/lib/mocha/integration/mini_test/version_131_and_above.rb:26:in `run'
            /home/davidjh/devel/foss/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:35:in `block in run'
            /home/davidjh/devel/foss/rails/activesupport/lib/active_support/callbacks.rb:413:in `_run_setup_callbacks'
            /home/davidjh/devel/foss/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:34:in `run'
    
        1 tests, 0 assertions, 0 failures, 1 errors, 0 skips
    
        Test run options: --seed 37085
    

    I have rebased http://github.com/hjdivad/rails/tree/issue-5137 to the current
    master, which as of now is 8b2e4fddbf7e9d204579cd87ac72f14bc592d5a5. The diff
    remains basically the same, but I have regenerated it again for your
    convenience.

  • Neeraj Singh

    Neeraj Singh September 1st, 2010 @ 09:43 PM

    • Milestone set to 3.x
    • State changed from “new” to “open”

    I am unable to follow the logic why key_changed is needed.

      key_changed = send("#{reflection.primary_key_name}_changed?"
    
  • hjdivad

    hjdivad September 2nd, 2010 @ 07:31 PM

    Neeraj,

    The test for invoking association.reload was to handle the following case,
    exhibited in a test in test/cases/associations/belongs_to_association_test.rb.

     def test_forgetting_the_load_when_foreign_key_enters_late
       c = Client.new
       assert_nil c.firm_with_basic_id
    
       c.firm_id = 1
       # sometimes tests on Oracle fail if ORDER BY is not provided
    therefore add always :order with :first
       assert_equal Firm.find(:first, :order => "id"), c.firm_with_basic_id
     end
    

    i.e. if the key changes, reload the association. However, the original logic I
    had did not properly handle all cases. I have updated the patch branch with a
    better patch that is also hopefully more clear. Attached is the new patch.

  • Neeraj Singh

    Neeraj Singh September 3rd, 2010 @ 03:46 PM

    I finally got the minivan example failing. However the attached fix is not passing. I am leaving for the long weekend. Will take a look when I come back.

  • Jon Leighton

    Jon Leighton December 21st, 2010 @ 12:12 PM

    This bug seems to be related to (or possibly a duplicate of) #142.

  • Repository

    Repository January 4th, 2011 @ 12:28 AM

    • State changed from “open” to “resolved”

    (from [a0be389d39b790e0625339251d2674b8250b16b1]) Allow assignment on has_one :through where the owner is a new record [#5137 state:resolved]

    This required changing the code to keep the association proxy for a belongs_to around, despite its target being nil. Which in turn required various changes to the way that stale target checking is handled, in order to support various edge cases (loaded target is nil then foreign key added, foreign key is changed and then changed back, etc). A side effect is that the code is nicer and more succinct.

    Note that I am removing test_no_unexpected_aliasing since that is basically checking that the proxy for a belongs_to does change, which is the exact opposite of the intention of this commit. Also adding various tests for various edge cases and related things.

    Phew, long commit message!
    https://github.com/rails/rails/commit/a0be389d39b790e0625339251d267...

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>

Attachments