Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix collection= on hm:t join models when unsaved #7661

Merged
merged 1 commit into from

6 participants

@ernie

If assigning to a has_many :through collection against an unsaved
object using the collection=[] syntax, the join models
were not properly created, previously.

This bug is present in the current 3.2.8 release, as well.

@rafaelfranca

Thanks @ernie. Could you add a CHANGELOG entry?

@steveklabnik
Collaborator

I think this is a duplicate of #7658, but is better written.

@rafaelfranca

I was thinking the same thing. @ernie could you see if this fixes #5057?

@rafaelfranca

We have these issues that can be related with this too #7486 #5178 #6161

@ernie

@rafaelfranca Sure thing. Taking a look at the other issues now. Didn't figure a CHANGELOG entry was necessary since it was a bugfix and not an API update but would be happy to make one. Also would love to get this fix backported to 3-2-stable, as I had to work around it in an app I'm working on now.

@ernie

@steveklabnik Since your PR doesn't include tests, am I right to assume that a proper test case is to have conditions on the association used in a :through and expect to see those conditions set on the join model?

@steveklabnik
Collaborator
@ernie

If I am reading the existing tests correctly, the other issues being discussed aren't issues, but misuses of the conditions on the hm:t association. See my explanation in above-referenced PR.

@ernie ernie Fix collection= on hm:t join models when unsaved
If assigning to a has_many :through collection against an unsaved
object using the collection=[<array_of_items>] syntax, the join models
were not properly created, previously.
610b632
@ernie

@rafaelfranca Updated the CHANGELOG, squashed commits and pushed.

@ernie

Wanted to restate the request that we get this fix cherry-picked to supported versions, as well. Thanks!

@rafaelfranca

@ernie your explanation make perfect sense to me. How about #6161?

@rafaelfranca

We changed the CHANGELOG policy and now we are putting entries to bug fixes too.

@ernie

@rafaelfranca I think this comment nails #6161. Inverse is required for this to work properly.

It's documented in the Association Join Models" section of the relevant API docs but maybe bears a mention in the guides?

@rafaelfranca

@ernie a mention in the guides would be great. I'm merging this one. Thanks so much.

@rafaelfranca rafaelfranca merged commit 82d507b into rails:master
@ernie

@rafaelfranca should I submit a separate PR for the backports or can someone cherry-pick this? (minus the changelog showing the change for 4.0.0, that is)

@rafaelfranca

I cherry-picked at ee43989

@gregolsen gregolsen referenced this pull request
Closed

has many through bug #8238

@gnufied

hey guys,

I spent better part of morning investigating this change and its effects and I am convinced that, this change should be rolled back. I am trying to breakdown what is going on.

A through associations is traditionally automatically saved as part of autosaving of associations. What that means is:
assuming you have code like:

class User < ActiveRecord::Base
  has_many :user_memberships
  has_many :teams, through: :user_memberships
end

u = User.new()
u.teams << Team.find(2)

Then when user object is saved, code in autosave_assocations.rb gets triggered and following bits of code ensures that, the join modal is created and saved:

if autosave
  saved = association.insert_record(record, false)
else
   association.insert_record(record) unless reflection.nested?
end

If you see code of insert_record method, it automatically builds through record.

Other than bugs (#6161 and possibly more) this pull request has introduced, let me show you another side effect of this PR.

class Article < ActiveRecord::Base
  attr_accessible :content, :title
  has_many :article_tags
  has_many :tags, :through => :article_tags

  before_validation :add_to_default_tag, :on => :create

  def add_to_default_tag
    if self.tags.empty?
      self.tags << Tag.find_or_create_by_title("default")
    end
  end
end

class ArticleTag < ActiveRecord::Base
  attr_accessible :article_id, :name, :tag_id

  belongs_to :article
  belongs_to :tag

  validate :check_stuff

  private
  def check_stuff
    puts "whoa"
  end

end

Now lets create a article:

article = Article.new(:title => "hemant", :content => "kumar")
article.save
=> "Whoa"
=> "Whoa"
=> "Whoa"

In other words the join record is getting created 3 times. One from newly introduced concat_records method and then again from autosave_assocation.rb.

If join records must be created while building the object itself, rather than when saving the record. I would want to know why. Using inverse_of argument is not convincing and I suspect specifying it in a big project is not always possible (or should be introduced in a major release. like 3.3.0, not in 3.2.9) .

\cc @ernie @tenderlove @dhh

@ernie

@gnufied Your demonstration says that the validation is running thrice, but that doesn't (necessarily) mean the creation occurs thrice. See the following code from has_many_through_association.rb:

        # We temporarily cache through record that has been build, because if we build a
        # through record in build_record and then subsequently call insert_record, then we
        # want to use the exact same object.
        #
        # However, after insert_record has been called, we clear the cache entry because
        # we want it to be possible to have multiple instances of the same record in an
        # association
        def build_through_record(record)
          @through_records[record.object_id] ||= begin
            ensure_mutable

            through_record = through_association.build
            through_record.send("#{source_reflection.name}=", record)
            through_record
          end
        end

Have you verified you end up with three through records?.

Additionally, the most in-depth discussion (and request for feedback from @tenderlove and @jonleighton) regarding this change was at #8269.

@gnufied

@ernie thanks for your comments. I did not mean 3 db records being created btw. I suspected same join record is being initialized 3 times, but apparently that is not the case. It is just validations are being triggered 3 times.

I still do not understand why it is not okay to have join record being created via autosave_assocations and why this must be done explicitly.

As for #8269 yes, I have gone through. I as well hope, either @tenderlove or @jonleighton comment on this.

@adamcooke

I have noticed this behaviour when attempting to use an association on the join model in a validation as the record is saved before the association is created and therefore the ID is nil. For example:

class UserTeam < ActiveRecord::Base

  belongs_to :user
  belongs_to :team

  validate do
    if user.account_id != team.account_id
      errors.add :base, "User and team must belong to the same account"
    end
  end
end

team = Team.new(:name => "Admins")
team.users << Users.find_by_name("Adam")
team.save

This save call causes a undefined method 'account_id' for nil:nilClass because team is nil.

If this behaviour is causing people issues it can be removed through monkey patching the class. I haven't extensively tested this however it does resolve the issues in my use-case.

class ActiveRecord::Associations::HasManyThroughAssociation
  remove_method :concat_records
end

As this change has affected the way the API operates I would encourage someone to remove this from 3-2-stable in a 3.2.12 release to avoid any further confusion with users upgrading from older versions of Rails. If necessary, it can be re-added into 4.0 with documentation to note the change of behaviour.

I have also noticed that this causes the validations to be run three times on the association object. This is documented in issue #8854.

@ernie ernie referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@ernie ernie referenced this pull request from a commit
@ernie ernie Revert "Merge pull request #7661 from ernie/build-join-records-on-uns…
…aved-hmt"

This reverts commit ee43989.

It would appear that #7661 had unintended consequences to the API. Until
we can sort those out, this should not be in 3.2.x, and wait for 4.0.0.
18b9187
@bradrobertson

Just wanted to see if this is the commit that may be affected my app upgrade. I've written out an example that illustrates our issue.

    class Role < ActiveRecord::Base; end

    class Permission < ActiveRecord::Base
      has_many :permission_roles
      has_many :roles, through: :permission_roles

      attr_accessible :roles
    end

    class PermissionRole < ActiveRecord::Base
      belongs_to :permission
      belongs_to :role

      validates :permission, presence: true
      validates :role, presence: true
    end

    # Fails in Rails 3.2.9
    Loading development environment (Rails 3.2.9)
    1.9.3-p194 :001 > p = Permission.create roles: Role.all
      Role Load (0.1ms)  SELECT "roles".* FROM "roles" 
       (0.1ms)  begin transaction
       (0.1ms)  rollback transaction
     => #<Permission id: nil, name: nil, created_at: nil, updated_at: nil> 

    # Works in Rails 3.2.8
     Loading development environment (Rails 3.2.8)
     1.9.3-p194 :001 > p = Permission.create roles: Role.all
       Role Load (0.1ms)  SELECT "roles".* FROM "roles" 
        (0.1ms)  begin transaction
       SQL (5.2ms)  INSERT INTO "permissions" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Fri, 11 Jan 2013 17:44:16 UTC +00:00], ["name", nil], ["updated_at", Fri, 11 Jan 2013 17:44:16 UTC +00:00]]
       Permission Load (0.2ms)  SELECT "permissions".* FROM "permissions" WHERE "permissions"."id" = 2 LIMIT 1
       SQL (0.5ms)  INSERT INTO "permission_roles" ("created_at", "permission_id", "role_id", "updated_at") VALUES (?, ?, ?, ?)  [["created_at", Fri, 11 Jan 2013 17:44:16 UTC +00:00], ["permission_id", 2], ["role_id", 1], ["updated_at", Fri, 11 Jan 2013 17:44:16 UTC +00:00]]
        (2.6ms)  commit transaction
      => #<Permission id: 1, name: nil, created_at: "2013-01-11 17:44:16", updated_at: "2013-01-11 17:44:16"> 

I've noticed your comment above about the revert. Am I correct in assuming that this is one of those cases you're referring to where the API has changed?

I've noticed also that using inverse_of seems to mitigate this, but I don't think a minor bug release should force a change to the API.

Is there any way I can upgrade without this affecting me?

@rafaelfranca

@bradrobertson yes. Use the 3-2-stable branch

@gnufied

@rafaelfranca @ernie do you think it is worth adding a test or two, to cover existing behaviour? I think one of culprit here was ActiveRecord tests. I can add some, I think.

@bradrobertson

I think tests are a good idea, this seems like a pretty common use case (ie: has_many :through with validations on the join model)

@ernie

I think tests to cover the existing behavior in 3-2-stable would be OK -- though active development is mostly on the master branch now, and I don't think we should be having this behaviour there.

@nashby nashby referenced this pull request in plataformatec/simple_form
Closed

f.association breaking from rails 3.2.8 to 3.2.9 #735

@snowblink snowblink referenced this pull request from a commit in notonthehighstreet/rails
@ernie ernie Revert "Merge pull request #7661 from ernie/build-join-records-on-uns…
…aved-hmt"

This reverts commit ee43989.

It would appear that #7661 had unintended consequences to the API. Until
we can sort those out, this should not be in 3.2.x, and wait for 4.0.0.

Conflicts:
	activerecord/CHANGELOG.md
56c7872
@pivotal-casebook pivotal-casebook referenced this pull request from a commit in Casecommons/rails
pivotalcb Revert "Revert "Merge pull request #7661 from ernie/build-join-record…
…s-on-unsaved-hmt""

This reverts commit 18b9187.
85801df
@smasry smasry referenced this pull request from a commit in smasry/rails
@smasry smasry Reverting changes from #7661 on master c16ff32
@pivotal-casebook pivotal-casebook referenced this pull request from a commit in Casecommons/rails
pivotalcb Revert "Revert "Merge pull request #7661 from ernie/build-join-record…
…s-on-unsaved-hmt""

This reverts commit 18b9187.
ce03a7c
@pivotal-casebook pivotal-casebook referenced this pull request from a commit in Casecommons/rails
pivotalcb Revert "Revert "Merge pull request #7661 from ernie/build-join-record…
…s-on-unsaved-hmt""

This reverts commit 18b9187.
44d4832
@pivotal-casebook pivotal-casebook referenced this pull request from a commit in Casecommons/rails
pivotalcb Revert "Revert "Merge pull request #7661 from ernie/build-join-record…
…s-on-unsaved-hmt""

This reverts commit 18b9187.
746255e
@pivotal-casebook pivotal-casebook referenced this pull request from a commit in Casecommons/rails
pivotalcb Revert "Revert "Merge pull request #7661 from ernie/build-join-record…
…s-on-unsaved-hmt""

This reverts commit 18b9187.
78c87a0
@pivotal-casebook pivotal-casebook referenced this pull request from a commit in Casecommons/rails
pivotalcb Revert "Revert "Merge pull request #7661 from ernie/build-join-record…
…s-on-unsaved-hmt""

This reverts commit 18b9187.
4662c07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 17, 2012
  1. @ernie

    Fix collection= on hm:t join models when unsaved

    ernie authored
    If assigning to a has_many :through collection against an unsaved
    object using the collection=[<array_of_items>] syntax, the join models
    were not properly created, previously.
This page is out of date. Refresh to see the latest.
View
5 activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##
+* Fix creation of through association models when using collection=[]
+ on a hm:t association from an unsaved model.
+
+ *Ernie Miller*
+
* Explain only normal CRUD sql (select / update / insert / delete).
Fix problem that explains unexplainable sql. Closes #7544 #6458.
View
14 activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -37,6 +37,20 @@ def concat(*records)
super
end
+ def concat_records(records)
+ ensure_not_nested
+
+ records = super
+
+ if owner.new_record? && records
+ records.flatten.each do |record|
+ build_through_record(record)
+ end
+ end
+
+ records
+ end
+
def insert_record(record, validate = true, raise = false)
ensure_not_nested
View
5 activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -838,6 +838,11 @@ def test_save_should_not_raise_exception_when_join_record_has_errors
end
end
+ def test_assign_array_to_new_record_builds_join_records
+ c = Category.new(:name => 'Fishing', :authors => [Author.first])
+ assert_equal 1, c.categorizations.size
+ end
+
def test_create_bang_should_raise_exception_when_join_record_has_errors
repair_validations(Categorization) do
Categorization.validate { |r| r.errors[:base] << 'Invalid Categorization' }
Something went wrong with that request. Please try again.