Rails Ruby on Rails → 2.3.9

Comments and changes to this ticket

• 

  Michael Koziarski August 3rd, 2009 @ 06:17 AM

  • Assigned user set to “Eloy Duran”
• 

  Jeremy Kemper May 4th, 2010 @ 06:48 PM

  • Milestone changed from “2.x” to “3.x”

  [bulk edit]

• 

  Eloy Duran July 8th, 2010 @ 08:38 AM

  • Assigned user changed from “Eloy Duran” to “José Valim”
  • Importance changed from “” to “”
• 

  Neeraj Singh July 13th, 2010 @ 10:16 AM

  • Tag set to “activerecord”

  I am not able to reproduce this error in rails edge.

• 

  Neeraj Singh July 13th, 2010 @ 10:19 AM

  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 >
  

• You flagged this item as spam.
  

  Hery July 13th, 2010 @ 01:38 PM

  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
  

• 

  Neeraj Singh July 13th, 2010 @ 04:07 PM

  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
  

• You flagged this item as spam.
  

  Hery July 13th, 2010 @ 05:00 PM

  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 !!

• 

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

  • State changed from “new” to “open”

  my bad. I totally missed that part that I was dealing with habtm and not with simpe has_many.

  I will look into it.

• 

  Neeraj Singh July 13th, 2010 @ 05:50 PM

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

• 

  José Valim July 14th, 2010 @ 07:06 AM

  • Milestone changed from “3.x” to “2.3.9”

  Neeraj, could you then backport #922 to 2-3-stable so I can appply it? Thanks!

• 

  Neeraj Singh July 14th, 2010 @ 03:51 PM

  #922 fix for 2-3-stable has been attached in ticket #922 .

• 

  José Valim July 18th, 2010 @ 10:12 AM

  • State changed from “open” to “resolved”

  Ok. #922 applied.

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 10:59 AM

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

• 

  José Valim September 21st, 2010 @ 11:03 AM

  lcarls, a patch is welcome!

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 11:25 AM

  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.

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 11:52 AM

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

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 01:06 PM

  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.

• 

  Eloy Duran September 21st, 2010 @ 03:08 PM

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

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 03:38 PM

  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.

• 

  José Valim September 21st, 2010 @ 03:39 PM

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

• 

  Eloy Duran September 21st, 2010 @ 03:43 PM

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

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 03:46 PM

  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

• 

  José Valim September 21st, 2010 @ 03:46 PM

  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.

• You flagged this item as spam.
  

  lcars September 21st, 2010 @ 03:49 PM

  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.

• 

  Eloy Duran September 21st, 2010 @ 04:40 PM

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