Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #5050 (closed defect: untested)

Opened 3 years ago

Last modified 2 years ago

ActiveRecord save method wrongly overwrite counter_cache (or ANY counter)

Reported by: curio@free.fr Assigned to: David
Priority: high Milestone: 1.x
Component: ActiveRecord Version: 0.14.4.
Severity: major Keywords: counter_cache
Cc: bitsweat

Description

Saving object after adding an has_many element with counter cache reset the database value of the counter cache to the value loaded from database.
For example post.comments_count gives 1 but post.attributescomments_count? gives 0.

instanciated object is right but value in object attribues are wrong (the only method i found for now is to do post.reload and continue)

In case this code does not follow good practices, any correction is welcome :)

example in console :

>> Post.find_all
=> []
>> post = Post.create
=> #<Post:0xb75d0120 @new_record=false, @errors=#<ActiveRecord::Errors:0xb75cee60 @errors={}, @base=#<Post:0xb75d0120 ...>>, @new_record_before_save=true, @attributes={"title"=>nil, "id"=>1, "comments_count"=>0}>
>> post.comments.create()
=> #<Comment:0xb75c6ef4 @new_record=false, @commentable=#<Post:0xb75c30ec @attributes={"title"=>nil, "id"=>"1", "comments_count"=>"0"}>, @errors=#<ActiveRecord::Errors:0xb75c5d88 @errors={}, @base=#<Comment:0xb75c6ef4 ...>>, @attributes={"commentable_type"=>"Post", "updated_at"=>Thu May 11 17:05:10 CEST 2006, "id"=>3, "commentable_id"=>1, "created_at"=>Thu May 11 17:05:10 CEST 2006}>
>> post.comments_count
=> 1
>> post.title = 'test'
=> "test"
>> post.save
=> true
>> post.comments_count
=> 1
>> post.id
=> 1
>> post = Post.find(1)
=> #<Post:0xb75ad2c4 @attributes={"title"=>"test", "id"=>"1", "comments_count"=>"0"}>
>> post.comments_count
=> 0

Models :

class Comment < ActiveRecord::Base
  belongs_to :commentable, :polymorphic => true, :counter_cache => true
end

class Post < ActiveRecord::Base
  has_many :comments, :as => :commentable, :dependent => :destroy
end

class Photo < ActiveRecord::Base
    has_many :comments, :as => :commentable, :dependent => :destroy
end

Migrations :

class CreatePhotos < ActiveRecord::Migration
  def self.up
    create_table :photos do |t|
      t.column :title, :string
      t.column "comments_count", :integer, :default => 0, :null => false
    end
  end

  def self.down
    drop_table :photos
  end
end

class CreatePosts < ActiveRecord::Migration
  def self.up
    create_table :posts do |t|
      t.column :title, :string
      t.column "comments_count", :integer, :default => 0, :null => false
    end
  end

  def self.down
    drop_table :posts
  end
end

class CreateComments < ActiveRecord::Migration
  def self.up
    create_table "comments" do |t|
      t.column "commentable_type", :string
      t.column "commentable_id", :integer
      t.column "created_at", :datetime
      t.column "updated_at", :datetime
    end
  end

  def self.down
    drop_table :comments
  end
end

Attachments

models_and_migrations.tar.gz (0.6 kB) - added by curio@free.fr on 05/11/06 15:25:39.
tarball of the model and migrations files

Change History

05/11/06 15:25:39 changed by curio@free.fr

  • attachment models_and_migrations.tar.gz added.

tarball of the model and migrations files

05/11/06 21:07:27 changed by curio@free.fr

After some discussions over #rubyonrails.fr, here is a quick conclusion :

post.comments.create() is right
Post Update (0.001694) UPDATE posts SET comments_count = comments_count + 1 WHERE (id = 1)

post.save is wrong
Post Update (0.001344) UPDATE posts SET title = 'test', comments_count = 0 WHERE id = 1

comments_count should never be saved like another attribute, because only the database knows the real actual value of the counter.

The correct SQL request for Post Update would be
UPDATE posts SET title = 'test' WHERE id = 1

08/25/06 13:08:08 changed by anonymous

  • priority changed from normal to highest.
  • version changed from 1.1.1 to 1.1.6.

This is a HUGE problem and needs to be fixed...

This is basic accounting not working as intended. Ie. this is so bad programs should be made to cause an exception if they attempt to use this feature. ANY calculation made using ActiveRecord becomes suspect and can't be trusted if counter_cache is used.

08/26/06 03:32:02 changed by anonymous

  • severity changed from normal to blocker.
  • summary changed from ActiveRecord save method wrongly overwrite counter_cache to ActiveRecord save method wrongly overwrite counter_cache (or ANY counter).

(same poster as immediately above) Actually counter_cache doesn't need to be used for this to be an issue. Any counter, even one by hand using ActiveRecord.increment_counter, or raw sql, is also broken by this.

The only solution curently is to NOT USE ActiveRecord#save or to override it and use AR#update_all or similar instead and provide the sql for the update.

08/26/06 05:29:18 changed by anonymous

(same poster) Here is a workaround for this issue. Only use this if you know that your schema only has columns ending in _count that are actual counters. I also have included _sum here because I think there is a glaring missing feature in increment_counter, which is the increment by amount.

Add this to your application.rb:

module ActiveRecord
	class Base
private

		# Updates the associated record with values matching those of the instance attributes.
		# OVERLOADIG TO DEAL WITH BUG#5050 http://dev.rubyonrails.org/ticket/5050#preview
		def update
			connection.update(
				"UPDATE #{self.class.table_name} " +
				"SET #{quoted_comma_pair_list(connection, attributes_with_quotes_FIXED(false))} " +
				"WHERE #{self.class.primary_key} = #{quote(id)}",
				"#{self.class.name} Update"
			)
			
			return true
		end

		# Returns copy of the attributes hash where all the values have been safely quoted for use in
		# an SQL statement.
		def attributes_with_quotes_FIXED(include_primary_key = true)
			attributes.inject({}) do |quoted, (name, value)|
				if column = column_for_attribute(name)
					quoted[name] = quote(value, column) unless ((!include_primary_key && column.primary) or column.name =~ /(_sum|_count)$/)
				end
				quoted
			end
		end


	end
end

08/26/06 10:28:40 changed by anonymous

(same poster lol)

Ok, well the above isn't a good workaround because the new update function doesnt trigger before_update or other callbacks. WTH... the only option I see is modify the original file which is crap. Still looking into this

08/27/06 05:01:41 changed by anonymous

So I gave up and just modified the rails source...

In {RUBY_INSTALL_DIR}/lib/ruby/gems/1.8/gems/activerecord-1.14.4/lib/active_record/base.rb: change line 1947 to:

quoted[name] = quote(value, column) unless ((!include_primary_key && column.primary) or column.name =~ /(_sum|_count)$/)

The function looks like this:

# Returns copy of the attributes hash where all the values have been safely quoted for use in # an SQL statement. def attributes_with_quotes(include_primary_key = true)

attributes.inject({}) do |quoted, (name, value)|

if column = column_for_attribute(name)

== this one ==> quoted[name] = quote(value, column) unless ((!include_primary_key && column.primary) or column.name =~ /(_sum|_count)$/)

end quoted

end

end

10/04/06 21:21:10 changed by slain

  • severity changed from 1 to major.
  • cc deleted.
  • component changed from 1 to ActiveRecord.
  • summary changed from Otto to ActiveRecord save method wrongly overwrite counter_cache (or ANY counter).
  • priority changed from 1 to high.
  • version changed from 1 to 0.14.4..
  • milestone deleted.
  • keywords changed from Otto to counter_cache.
  • type changed from 1 to defect.

Fixed the vandalized ticket info.

11/09/06 09:08:48 changed by bitsweat

  • cc set to bitsweat.
  • milestone set to 1.x.

This looks like a fine fix. Please attach a diff and updated unit tests demonstrating that the problem is fixed.

11/09/06 09:09:04 changed by bitsweat

  • status changed from new to closed.
  • resolution set to untested.

01/09/07 21:57:28 changed by dcmanges

I submitted ticket #6896 to fix this problem without relying on conventions of the counter_cache column name.