diff --git a/README.md b/README.md index a3712af..25a81a3 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,24 @@ Now, the ```Category``` model will keep the counter cache in ```special_count``` If you would like to use this with `counter_culture_fix_counts`, make sure to also provide [the `column_names` configuration](#handling-dynamic-column-names). +### Conditional counter cache shorthand + +You may also use a conditional API identical to [ActiveRecord's Callbacks](https://guides.rubyonrails.org/active_record_callbacks.html#conditional-callbacks) to conditionally count columns: + +```ruby +class Product < ActiveRecord::Base + belongs_to :category + counter_culture :category, column_name: :special_count, if: :special? +end + +class Category < ActiveRecord::Base + has_many :products +end + +Now, the ```Category``` model will keep the counter cache in ```special_count``` up-to-date. Only products where ```special?``` returns true will affect the special_count. + +If you would like to use this with `counter_culture_fix_counts`, make sure to also provide [the `column_names` configuration](#handling-dynamic-column-names). + ### Temporarily skipping counter cache updates If you would like to temporarily pause counter_culture, for example in a backfill script, you can do so as follows: diff --git a/lib/counter_culture/counter.rb b/lib/counter_culture/counter.rb index a829391..1263be4 100644 --- a/lib/counter_culture/counter.rb +++ b/lib/counter_culture/counter.rb @@ -17,6 +17,8 @@ def initialize(model, relation, options) @delta_magnitude = options[:delta_magnitude] || 1 @with_papertrail = options.fetch(:with_papertrail, false) @execute_after_commit = options.fetch(:execute_after_commit, false) + @if_conditions = Array.wrap(options[:if]) + @unless_conditions = Array.wrap(options[:unless]) if @execute_after_commit begin @@ -49,78 +51,79 @@ def change_counter_cache(obj, options) # allow overwriting of foreign key value by the caller id_to_change = foreign_key_values.call(id_to_change) if foreign_key_values - if id_to_change && change_counter_column - delta_magnitude = if delta_column - (options[:was] ? attribute_was(obj, delta_column) : obj.public_send(delta_column)) || 0 - else - counter_delta_magnitude_for(obj) - end - # increment or decrement? - operator = options[:increment] ? '+' : '-' - - klass = relation_klass(relation, source: obj, was: options[:was]) - - # MySQL throws an ambiguous column error if any joins are present and we don't include the - # table name. We isolate this change to MySQL because sqlite has the opposite behavior and - # throws an exception if the table name is present after UPDATE. - quoted_column = if klass.connection.adapter_name == 'Mysql2' - "#{klass.quoted_table_name}.#{model.connection.quote_column_name(change_counter_column)}" + return unless conditions_allow_change?(obj) + return unless id_to_change && change_counter_column + + delta_magnitude = if delta_column + (options[:was] ? attribute_was(obj, delta_column) : obj.public_send(delta_column)) || 0 else - "#{model.connection.quote_column_name(change_counter_column)}" + counter_delta_magnitude_for(obj) end - - column_type = klass.type_for_attribute(change_counter_column).type - - # we don't use Rails' update_counters because we support changing the timestamp - updates = [] - # this updates the actual counter - if column_type == :money - updates << "#{quoted_column} = COALESCE(CAST(#{quoted_column} as NUMERIC), 0) #{operator} #{delta_magnitude}" - else - updates << "#{quoted_column} = COALESCE(#{quoted_column}, 0) #{operator} #{delta_magnitude}" + # increment or decrement? + operator = options[:increment] ? '+' : '-' + + klass = relation_klass(relation, source: obj, was: options[:was]) + + # MySQL throws an ambiguous column error if any joins are present and we don't include the + # table name. We isolate this change to MySQL because sqlite has the opposite behavior and + # throws an exception if the table name is present after UPDATE. + quoted_column = if klass.connection.adapter_name == 'Mysql2' + "#{klass.quoted_table_name}.#{model.connection.quote_column_name(change_counter_column)}" + else + "#{model.connection.quote_column_name(change_counter_column)}" + end + + column_type = klass.type_for_attribute(change_counter_column).type + + # we don't use Rails' update_counters because we support changing the timestamp + updates = [] + # this updates the actual counter + if column_type == :money + updates << "#{quoted_column} = COALESCE(CAST(#{quoted_column} as NUMERIC), 0) #{operator} #{delta_magnitude}" + else + updates << "#{quoted_column} = COALESCE(#{quoted_column}, 0) #{operator} #{delta_magnitude}" + end + # and here we update the timestamp, if so desired + if touch + current_time = klass.send(:current_time_from_proper_timezone) + timestamp_columns = klass.send(:timestamp_attributes_for_update_in_model) + if touch != true + # starting in Rails 6 this is frozen + timestamp_columns = timestamp_columns.dup + timestamp_columns << touch end - # and here we update the timestamp, if so desired - if touch - current_time = klass.send(:current_time_from_proper_timezone) - timestamp_columns = klass.send(:timestamp_attributes_for_update_in_model) - if touch != true - # starting in Rails 6 this is frozen - timestamp_columns = timestamp_columns.dup - timestamp_columns << touch - end - timestamp_columns.each do |timestamp_column| - updates << "#{timestamp_column} = '#{current_time.to_formatted_s(:db)}'" - end + timestamp_columns.each do |timestamp_column| + updates << "#{timestamp_column} = '#{current_time.to_formatted_s(:db)}'" end + end + + primary_key = relation_primary_key(relation, source: obj, was: options[:was]) + + if @with_papertrail + instance = klass.where(primary_key => id_to_change).first + if instance + if instance.paper_trail.respond_to?(:save_with_version) + # touch_with_version is deprecated starting in PaperTrail 9.0.0 - primary_key = relation_primary_key(relation, source: obj, was: options[:was]) - - if @with_papertrail - instance = klass.where(primary_key => id_to_change).first - if instance - if instance.paper_trail.respond_to?(:save_with_version) - # touch_with_version is deprecated starting in PaperTrail 9.0.0 - - current_time = obj.send(:current_time_from_proper_timezone) - timestamp_columns = obj.send(:timestamp_attributes_for_update_in_model) - timestamp_columns.each do |timestamp_column| - instance.send("#{timestamp_column}=", current_time) - end - - execute_now_or_after_commit(obj) do - instance.paper_trail.save_with_version(validate: false) - end - else - execute_now_or_after_commit(obj) do - instance.paper_trail.touch_with_version - end + current_time = obj.send(:current_time_from_proper_timezone) + timestamp_columns = obj.send(:timestamp_attributes_for_update_in_model) + timestamp_columns.each do |timestamp_column| + instance.send("#{timestamp_column}=", current_time) + end + + execute_now_or_after_commit(obj) do + instance.paper_trail.save_with_version(validate: false) + end + else + execute_now_or_after_commit(obj) do + instance.paper_trail.touch_with_version end end end + end - execute_now_or_after_commit(obj) do - klass.where(primary_key => id_to_change).update_all updates.join(', ') - end + execute_now_or_after_commit(obj) do + klass.where(primary_key => id_to_change).update_all updates.join(', ') end end @@ -342,5 +345,24 @@ def attribute_was(obj, attr) end obj.public_send("#{attr}#{changes_method}") end + + # Returns whether the given conditions allow the counter cache column to update. + # The callback only runs when all the :if conditions and none of the :unless conditions are evaluated to true. + def conditions_allow_change?(obj) + @if_conditions.all? { |condition| evaluate_condition(condition, obj) } && + @unless_conditions.none? { |condition| evaluate_condition(condition, obj) } + end + + # Evaluate the conditions in the context of the object + def evaluate_condition(condition, obj) + case condition + when Symbol + obj.send(condition) + when Proc + obj.instance_exec(&condition) + else + raise ArgumentError, "Condition must be a symbol or a proc" + end + end end end diff --git a/spec/counter_culture_spec.rb b/spec/counter_culture_spec.rb index 9b56554..24b3dab 100644 --- a/spec/counter_culture_spec.rb +++ b/spec/counter_culture_spec.rb @@ -15,6 +15,7 @@ require 'models/simple_dependent' require 'models/conditional_main' require 'models/conditional_dependent' +require 'models/conditional_dependent_shorthand' require 'models/post' require 'models/post_comment' require 'models/post_like' @@ -1711,6 +1712,24 @@ def yaml_load(yaml) ConditionalMain.find_each { |main| expect(main.conditional_dependents_count).to eq(main.id % 2 == 0 ? 3 : 0) } end + it "should correctly fix the counter caches for thousands of records when counter is conditional using :if/:unless" do + # first, clean up + ConditionalDependentShorthand.delete_all + ConditionalMain.delete_all + + MANY.times do |i| + main = ConditionalMain.create + 3.times { main.conditional_dependent_shorthands.create(:condition => main.id % 2 == 0) } + end + + ConditionalMain.find_each { |main| expect(main.conditional_dependent_shorthands_count).to eq(main.id % 2 == 0 ? 3 : 0) } + + ConditionalMain.order(db_random).limit(A_FEW).update_all :conditional_dependent_shorthands_count => 1 + ConditionalDependentShorthand.counter_culture_fix_counts :batch_size => A_BATCH + + ConditionalMain.find_each { |main| expect(main.conditional_dependent_shorthands_count).to eq(main.id % 2 == 0 ? 3 : 0) } + end + it "should correctly fix the counter caches when no dependent record exists for some of main records" do # first, clean up SimpleDependent.delete_all diff --git a/spec/models/conditional_dependent_shorthand.rb b/spec/models/conditional_dependent_shorthand.rb new file mode 100644 index 0000000..8aa1449 --- /dev/null +++ b/spec/models/conditional_dependent_shorthand.rb @@ -0,0 +1,8 @@ +class ConditionalDependentShorthand < ActiveRecord::Base + belongs_to :conditional_main + scope :condition, -> { where(condition: true) } + + counter_culture :conditional_main, if: :condition?, column_names: -> { { + ConditionalDependentShorthand.condition => :conditional_dependent_shorthands_count, + } } +end diff --git a/spec/models/conditional_main.rb b/spec/models/conditional_main.rb index 06dacdb..8c24c55 100644 --- a/spec/models/conditional_main.rb +++ b/spec/models/conditional_main.rb @@ -1,3 +1,4 @@ class ConditionalMain < ActiveRecord::Base has_many :conditional_dependents + has_many :conditional_dependent_shorthands end diff --git a/spec/schema.rb b/spec/schema.rb index b2c46c9..0f1b5ed 100644 --- a/spec/schema.rb +++ b/spec/schema.rb @@ -139,6 +139,7 @@ create_table "conditional_mains", :force => true do |t| t.integer "conditional_dependents_count", :null => false, :default => 0 + t.integer "conditional_dependent_shorthands_count", :null => false, :default => 0 t.datetime "created_at" t.datetime "updated_at" end @@ -150,6 +151,13 @@ t.datetime "updated_at" end + create_table "conditional_dependent_shorthands", :force => true do |t| + t.integer "conditional_main_id" + t.boolean "condition", default: false + t.datetime "created_at" + t.datetime "updated_at" + end + create_table "categs", :primary_key => "cat_id", :force => true do |t| t.integer "posts_count", :default => 0, :null => false t.datetime "created_at"