This project is archived and is in readonly mode.

#2933 ✓resolved
Hery

2.3.2 update_attributes save many-to-many associations

Reported by Hery | July 21st, 2009 @ 05:12 PM | in 2.3.9

Hi,

I use ruby 1.8.7 and Rails 2.3.2

update_attributes

Updates all the attributes from the passed-in Hash and saves the record. If the object is invalid, the saving will >> fail and false will be returned.

Hmm, as I understand this purpose :


class User

  has_and_belongs_to_many :sites
  validates_presence_of :login

end

class Site
  has_and_belongs_to_many :users
end

class UsersController < ApplicationController

  def update
    @user = User.find(params[:id])
    if @user.update_attributes(params[:user]])
      redirect_to :action => "list"
    else
      render(:action => "edit")
    end  
  end

end

Before the request @user.site_ids == [12,13,14,15]

params[:user][:site_ids] == [12,13]
params[:user][:login] == ""

When @user is not valid, I expect that the table sites_users is not updated but as I can see in the log files rails checks the validity of User AFTER updating sites_users

SQL (0.0ms) BEGIN SQL (0.2ms) DELETE FROM sites_users WHERE user_id = 431 AND site_id IN (15,14) SQL (0.0ms) COMMIT

User Exists (0.7ms) SELECT users.id FROM users WHERE (users.login = BINARY '' AND users.id <> 431) >LIMIT 1 User Exists (0.4ms) SELECT users.id FROM users WHERE (users.name = BINARY '*******' AND users.id <> > 431) LIMIT 1

Is this the normal behaviour ? I think it should be "check for the validity of a user, then update the associations".

Comments and changes to this ticket

  • Michael Koziarski

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

    • Assigned user set to “Eloy Duran”
  • Jeremy Kemper

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

    • Milestone changed from 2.x to 3.x
  • Eloy Duran

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

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

    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

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

    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

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

    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

    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

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

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

  • José Valim

    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

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

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

  • José Valim

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

    • State changed from “open” to “resolved”

    Ok. #922 applied.

  • lcars

    lcars September 21st, 2010 @ 10:59 AM

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

  • José Valim

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

    lcarls, a patch is welcome!

  • lcars

    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.

  • lcars

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

  • lcars

    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

    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.

  • lcars

    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

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

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

  • Eloy Duran

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

  • lcars

    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

    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.

  • lcars

    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

    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.

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>

Referenced by

Pages