-
Notifications
You must be signed in to change notification settings - Fork 210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add conditional api #390
base: master
Are you sure you want to change the base?
Add conditional api #390
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's a different idea, I'd love to discuss whether that's a better approach: What if we took what's passed into
The advantage I see of that is that it simplifies the code paths—there's still only one code path to consider and Is that better? Or does that make the code more complicated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that was the approach I considered originally, but then we can't specify a custom column name when using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you say a bit more about that? I don't think I follow why that doesn't work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, if I’m understanding you correctly that we “compile” into a proc passed internally to column_name, then we can’t do the following: column_name: :foo_count, if: :special? Because our compiled proc for if would overwrite the :foo_count symbol There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't we use the value passed as the name in the proc we compile the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you’re right. Nice, I’ll take a look at refactoring to that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated, LMK what you think. Then I'll look at updating the tests |
||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lots of noise here but the only change is switching to a guard clause since there are two now |
||
|
||
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) | ||
tatethurston marked this conversation as resolved.
Show resolved
Hide resolved
|
||
when Proc | ||
obj.instance_exec(&condition) | ||
else | ||
raise ArgumentError, "Condition must be a symbol or a proc" | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd need a test for the normal updating on insert / update / delete as well, right? I don't even think we change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree with sidestepping the fix counts logic since that's orthogonal. I tried to think of a way to have fix_count work well out of the box with this approach, but it would require counting in ruby instead of sql. |
||
# 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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: -> { { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd need a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, and when passing an array of symbols for if / unless, and when using if and unless together. |
||
ConditionalDependentShorthand.condition => :conditional_dependent_shorthands_count, | ||
} } | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
class ConditionalMain < ActiveRecord::Base | ||
has_many :conditional_dependents | ||
has_many :conditional_dependent_shorthands | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have an example with
unless
as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Should I continue this pattern to expand out the variations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, symbol, proc, array of symbols, array of procs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think so