2.3.2 update_attributes save many-to-many associations

Here is more info about my test case.

class Country < ActiveRecord::Base
  has_and_belongs_to_many :treaties

  validates_presence_of :name

  def self.setup
    c = Country.create(:name => 'india')
    c.treaties.create(:name => 'peace1')
    c.treaties.create(:name => 'peace2')
    c.treaties.create(:name => 'peace3')
  end

  def self.lab
    c = Country.first
    c.name = nil
    c.treaty_ids = [1,2]
  end

end

ree-1.8.7-2010.01 > Country.lab
 Country Load (0.3ms)  SELECT "countries".* FROM "countries" LIMIT 1
  Treaty Load (0.4ms)  SELECT "treaties".* FROM "treaties" WHERE ("treaties"."id" IN (1, 2))
  Treaty Load (0.4ms)  SELECT * FROM "treaties" INNER JOIN "countries_treaties" ON "treaties".id = "countries_treaties".treaty_id WHERE ("countries_treaties".country_id = 1 )
  SQL (0.6ms)  DELETE FROM "countries_treaties" WHERE ("countries_treaties"."country_id" = 1 AND "countries_treaties"."treaty_id" IN (3))
 => [1, 2] 
ree-1.8.7-2010.01 > Treaty.all
  Treaty Load (0.5ms)  SELECT "treaties".* FROM "treaties"
 => [#<Treaty id: 1, name: "peace1">, #<Treaty id: 2, name: "peace2">, #<Treaty id: 3, name: "peace3">] 
ree-1.8.7-2010.01 >

Hi Neeraj,

I would like to point that you did not use update_attributes method

Here is a modified test case and as you can see, it is the same behaviour as in 2.x

class Country < ActiveRecord::Base
  has_and_belongs_to_many :treaties
  validates_presence_of :name

  def self.setup
    Country.destroy_all
    c = Country.create(:name => 'india')
    c.treaties.create(:name => 'peace1')
    c.treaties.create(:name => 'peace2')
    c.treaties.create(:name => 'peace3')
  end

  def self.lab
    c = Country.first
    c.update_attributes({:name => nil, :treaty_ids => [1,2]})
  end
end

  Country Load (0.3ms)  SELECT `countries`.* FROM `countries` LIMIT 1
  Treaty Load (0.3ms)  SELECT `treaties`.* FROM `treaties` WHERE (`treaties`.`id` IN (1, 2))
  Treaty Load (3.5ms)  SELECT * FROM `treaties` INNER JOIN `countries_treaties` ON `treaties`.id = `countries_treaties`.treaty_id WHERE (`countries_treaties`.country_id = 4 )
  SQL (0.1ms)  BEGIN
  SQL (0.2ms)  DELETE FROM `countries_treaties` WHERE (`countries_treaties`.`country_id` = 4 AND `countries_treaties`.`treaty_id` IN (8, 9, 10))
  SQL (0.2ms)  INSERT INTO `countries_treaties` (`country_id`, `treaty_id`) VALUES (4, 1)
  SQL (0.2ms)  INSERT INTO `countries_treaties` (`country_id`, `treaty_id`) VALUES (4, 2)
  SQL (49.5ms)  COMMIT
  SQL (0.1ms)  BEGIN
  SQL (0.1ms)  ROLLBACK

I am still not able to recreate the problem in edge.

Here is my test code.

class Country < ActiveRecord::Base
  has_and_belongs_to_many :treaties

  validates_presence_of :name

  def self.setup
    c = Country.create(:name => 'india')
    c.treaties.create(:name => 'peace1')
    c.treaties.create(:name => 'peace2')
    c.treaties.create(:name => 'peace3')
  end

  def self.lab
    Country.delete_all
    Treaty.delete_all
    self.setup
    c = Country.first
    c.update_attributes({:name => nil, :treaty_ids => [Treaty.first.id, Treaty.last.id]})
    puts Treaty.all.inspect
  end

end

Here is what I got on my console

  SQL (2.0ms)  DELETE FROM "countries" WHERE 1=1
  SQL (1.7ms)  DELETE FROM "treaties" WHERE 1=1
  SQL (0.4ms)  INSERT INTO "countries" ("name") VALUES ('india')
  SQL (0.5ms)  INSERT INTO "treaties" ("name") VALUES ('peace1')
  SQL (2.2ms)  INSERT INTO "countries_treaties" ("country_id", "treaty_id") VALUES (3, 7)
  SQL (0.4ms)  INSERT INTO "treaties" ("name") VALUES ('peace2')
  SQL (1.6ms)  INSERT INTO "countries_treaties" ("country_id", "treaty_id") VALUES (3, 8)
  SQL (0.4ms)  INSERT INTO "treaties" ("name") VALUES ('peace3')
  SQL (1.4ms)  INSERT INTO "countries_treaties" ("country_id", "treaty_id") VALUES (3, 9)
  Country Load (0.3ms)  SELECT "countries".* FROM "countries" LIMIT 1
  Treaty Load (0.2ms)  SELECT "treaties".* FROM "treaties" LIMIT 1
  Treaty Load (0.3ms)  SELECT "treaties".* FROM "treaties" ORDER BY treaties.id DESC LIMIT 1
  Treaty Load (0.3ms)  SELECT "treaties".* FROM "treaties" WHERE ("treaties"."id" IN (7, 9))
  Treaty Load (0.6ms)  SELECT * FROM "treaties" INNER JOIN "countries_treaties" ON "treaties".id = "countries_treaties".treaty_id WHERE ("countries_treaties".country_id = 3 )
  SQL (0.4ms)  DELETE FROM "countries_treaties" WHERE ("countries_treaties"."country_id" = 3 AND "countries_treaties"."treaty_id" IN (8))
  Treaty Load (0.3ms)  SELECT "treaties".* FROM "treaties"
[#<Treaty id: 7, name: "peace1">, #<Treaty id: 8, name: "peace2">, #<Treaty id: 9, name: "peace3">]
 => nil

Yes you had !

  SQL (0.4ms)  DELETE FROM "countries_treaties" WHERE ("countries_treaties"."country_id" = 3 AND "countries_treaties"."treaty_id" IN (8))

It deletes countries_treaties rows even if @country is invalid !!

If the fix for #922 has_many through transaction rollback is applied then that would resolve this ticket too.

#922 has_many through transaction rollback fix for 2-3-stable has been attached in ticket #922 has_many through transaction rollback .

This bug still affects attributes= method. It should be fixed there as well.

lcarls, a patch is welcome!

Working on a patch right now, I should also note that I would treat this issue as a security bug as it allows validation to fails but still to update associations. It should be pushed to stable as soon as possible. I'm not familiar with Rails/ActiveRecord release process though, so I defer to maintainers...just voicing my opinion.

Other than validation concerns, is .attributes= expected to update associations before .save ? It is not clear from documentation that .attributes implies .save, but that's how it works against associations. Feels there are two bugs here, 1) validation is not honoured, 2) associations are instantly updated (but not attributes).

Not sure how to fix the .attributes = case. I assume that it's implied in the documentation that foo.bar_ids = immediately replaces the collection. Being this the case .attributes =, by calling that method triggers association changes (but not attribute changes which are saved only when .save is invoked).

In order to fix the validation issue we should change .attributes so that it performs a .save as well, this would allow to use transaction as a fix like it was done for update_attributes.

I'm not sure how this could be fixed otherwise.

Any input is appreciated.

#attributes= should never save any records by itself, that’s what #update_attributes is for. IMO it might be better if you’d start on a failing test case for the ActiveRecord test suite, then we can discuss the problem/solution from there.

added this test to autosave_association_test.rb

def test_assign_ids_without_saving

firm = Firm.new("name" => "Apple")
firm.save
firm.reload
firm.client_ids = [companies(:first_client).id, companies(:second_client).id]
assert_not_equal 2, firm.clients.length
assert !firm.clients.include?(companies(:second_client))

end

The existing test_assign_ids succeeds while test_assign_ids_without_saving fails.

Yes, this is expected and I believe it is well documented.

Hmm, I don’t really get what the test is trying to explain here… Afaik assigning to #assoc_ids= will always perform an implicit save directly. However, it seems José understands, so I’ll digress :)

Sorry, my test wasn't focused on the specific issue. Is it then also expected for #attributes= ti save records?

def test_assign_ids_via_attributes_without_saving

firm = Firm.new("name" => "Apple")
firm.save
firm.reload
firm.attributes = { :client_ids => [companies(:first_client).id, companies(:second_client).id] }
assert_not_equal 2, firm.clients.length
assert !firm.clients.include?(companies(:second_client))

end

Eloy, the test was showing exactly what you said: assigning to #assoc_ids= will always perform an implicit save directly. In his opinion, it seems that should not happen. That could even be a valid point, but not way we can change it as it is completely backwards incompatible.

I agree changing it's backwards incompatible and really problematic. I just wonder if #attributes= is expected to behave the same (I'm not saying it shouldn't), and if it does I wonder how we could fix it in regards to this ticket issue.

It feels "wrong" that using #attributes= { :local_column => 'foobar', :stuff_ids => [1,2,3] } would require a .save just for :local_column (which otherwise doesn't get updated) but would automatically save 'stuff' association.

This behaviour doesn't allow relying on .save return code for evaluating if the object has been saved along with associations or not, and I don't see an easy way for rolling back the effect of the association changes.

@José Gotcha :)

@lcars Ah, now I see the problem. #attributes= does basically the following for each attribute/value pair in the hash: send("#{attribute}=", value). I.e. you are calling #stuff_ids= with the array of IDs, which, as explained, does an implicit save. Maybe the solution would be to allow #assoc_ids= to defer its action until #save is called on the parent? Maybe with an association option, or even automatically when :autosave is set to true? I have no idea right now how backwards compatible that would be, though.