Remove support for updates from after_create for Rails < 5.1 and exec…

…ute_after_commit
magnusvk · Jun 12, 2018 · 7639b08 · 7639b08
1 parent ff191f3
commit 7639b08
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 63 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,15 @@
-## 1.13.0 (June 12, 2018)
+## 2.0.0 (June 12, 2018)
+
+Breaking changes:
+  - execute_after_commit was removed
+  - Removed workaround for incorrect counts when triggering updates from an `after_create` hook. Your options if this applies to you:
+    * continue using counter_culture 1.12.0
+    * upgrade to Rails 5.1.5 which fixes the underlying issue in Rails
+    * avoid triggering further updates on the same model in `after_create`; simply set the attribute in `before_create` instead
 
 Bugfixes:
   - Multiple updates in one transaction will now be processed correctly (#222)
 
-Deprecations:
-  - execute_after_commit is now deprecated and will be removed in gem version 2.0
-
 ## 1.12.0 (June 8, 2018)
 
 Improvements:

diff --git a/README.md b/README.md
@@ -216,16 +216,6 @@ You may also specify a custom timestamp column that gets updated only when a par
 
 With this option, any time the `category_counter_cache` changes both the `category_count_changed` and `updated_at` columns will get updated.
 
-### Executing counter cache updates after commit
-
-By default, counter_culture will run counter cache updates inside of the same ActiveRecord transaction that triggered it. (Note that this bevavior [changed from version 0.2.3 to 1.0.0](CHANGELOG.md#100-november-15-2016).) If you would like to run counter cache updates outside of that transaction, for example because you are experiencing [deadlocks with older versions of PostgreSQL](http://mina.naguib.ca/blog/2010/11/22/postgresql-foreign-key-deadlocks.html), you can enable that behavior:
-```ruby
-  counter_culture :category, execute_after_commit: true
-```
-
-Please note that using `execute_after_commit` in conjunction with transactional
-fixtures will lead to your tests no longer seeing updated counter values.
-
 ### Manually populating counter cache values
 
 You will sometimes want to populate counter-cache values from primary data. This is required when adding counter-caches to existing data. It is also recommended to run this regularly (at BestVendor, we run it once a week) to catch any incorrect values in the counter caches.

diff --git a/lib/counter_culture/counter.rb b/lib/counter_culture/counter.rb
@@ -1,6 +1,6 @@
 module CounterCulture
   class Counter
-    CONFIG_OPTIONS = [ :column_names, :counter_cache_name, :delta_column, :foreign_key_values, :touch, :delta_magnitude, :execute_after_commit ]
+    CONFIG_OPTIONS = [ :column_names, :counter_cache_name, :delta_column, :foreign_key_values, :touch, :delta_magnitude]
     ACTIVE_RECORD_VERSION = Gem.loaded_specs["activerecord"].version
 
     attr_reader :model, :relation, *CONFIG_OPTIONS
@@ -9,13 +9,16 @@ def initialize(model, relation, options)
       @model = model
       @relation = relation.is_a?(Enumerable) ? relation : [relation]
 
+      if options.fetch(:execute_after_commit, false)
+        fail("execute_after_commit was removed; updates now run within the transaction")
+      end
+
       @counter_cache_name = options.fetch(:column_name, "#{model.name.tableize}_count")
       @column_names = options[:column_names]
       @delta_column = options[:delta_column]
       @foreign_key_values = options[:foreign_key_values]
       @touch = options.fetch(:touch, false)
       @delta_magnitude = options[:delta_magnitude] || 1
-      @execute_after_commit = options.fetch(:execute_after_commit, false)
       @with_papertrail = options.fetch(:with_papertrail, false)
     end
 
@@ -29,7 +32,6 @@ def initialize(model, relation, options)
     #   :delta_column => override the default count delta (1) with the value of this column in the counted record
     #   :was => whether to get the current value or the old value of the
     #      first part of the relation
-    #   :execute_after_commit => execute the column update outside of the transaction to avoid deadlocks
     #   :with_papertrail => update the column via Papertrail touch_with_version method
     def change_counter_cache(obj, options)
       change_counter_column = options.fetch(:counter_column) { counter_cache_name_for(obj) }
@@ -45,43 +47,41 @@ def change_counter_cache(obj, options)
                           else
                             counter_delta_magnitude_for(obj)
                           end
-        execute_change_counter_cache(obj, options) do
-          # increment or decrement?
-          operator = options[:increment] ? '+' : '-'
-
-          # we don't use Rails' update_counters because we support changing the timestamp
-          quoted_column = model.connection.quote_column_name(change_counter_column)
-
-          updates = []
-          # this updates the actual counter
-          updates << "#{quoted_column} = COALESCE(#{quoted_column}, 0) #{operator} #{delta_magnitude}"
-          # and here we update the timestamp, if so desired
-          if touch
-            current_time = obj.send(:current_time_from_proper_timezone)
-            timestamp_columns = obj.send(:timestamp_attributes_for_update_in_model)
-            timestamp_columns << touch if touch != true
-            timestamp_columns.each do |timestamp_column|
-              updates << "#{timestamp_column} = '#{current_time.to_formatted_s(:db)}'"
-            end
+        # increment or decrement?
+        operator = options[:increment] ? '+' : '-'
+
+        # we don't use Rails' update_counters because we support changing the timestamp
+        quoted_column = model.connection.quote_column_name(change_counter_column)
+
+        updates = []
+        # this updates the actual counter
+        updates << "#{quoted_column} = COALESCE(#{quoted_column}, 0) #{operator} #{delta_magnitude}"
+        # and here we update the timestamp, if so desired
+        if touch
+          current_time = obj.send(:current_time_from_proper_timezone)
+          timestamp_columns = obj.send(:timestamp_attributes_for_update_in_model)
+          timestamp_columns << touch if touch != true
+          timestamp_columns.each do |timestamp_column|
+            updates << "#{timestamp_column} = '#{current_time.to_formatted_s(:db)}'"
           end
+        end
 
-          klass = relation_klass(relation, source: obj, was: options[:was])
-          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
-                instance.paper_trail.save_with_version(validate: false)
-              else
-                instance.paper_trail.touch_with_version
-              end
+        klass = relation_klass(relation, source: obj, was: options[:was])
+        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
+              instance.paper_trail.save_with_version(validate: false)
+            else
+              instance.paper_trail.touch_with_version
             end
           end
-
-          klass.where(primary_key => id_to_change).update_all updates.join(', ')
         end
+
+        klass.where(primary_key => id_to_change).update_all updates.join(', ')
       end
     end
 
@@ -278,14 +278,6 @@ def previous_model(obj)
     end
 
     private
-    def execute_change_counter_cache(obj, options)
-      if execute_after_commit
-        obj.execute_after_commit { yield }
-      else
-        yield
-      end
-    end
-
     def attribute_was(obj, attr)
       changes_method =
         if ACTIVE_RECORD_VERSION >= Gem::Version.new("5.1.0")

diff --git a/lib/counter_culture/extensions.rb b/lib/counter_culture/extensions.rb
@@ -51,12 +51,6 @@ def counter_culture(relation, options = {})
           raise ":column_names must be a Hash of conditions and column names"
         end
 
-        if options[:execute_after_commit]
-           ActiveSupport::Deprecation.warn(
-             'execute_after_commit is deprecated and will be removed from '\
-             'counter_culture 2.0')
-        end
-
         # add the counter to our collection
         @after_commit_counter_cache << Counter.new(self, relation, options)
       end

diff --git a/spec/models/review.rb b/spec/models/review.rb
@@ -20,6 +20,7 @@ class Review < ActiveRecord::Base
   after_create :update_some_text
 
   def update_some_text
+    return unless Gem::Version.new(Rails.version) >= Gem::Version.new('5.1.5')
     update_attribute(:some_text, rand(36**12).to_s(36))
   end