Improve account counters handling (#15913)

* Improve account counters handling

* Use ActiveRecord::Base::sanitize_sql to pass values instead of interpolating them

Keep using string interpolation for `key` as it is safe and using
“ActiveRecord::Base::sanitize_sql_hash_for_assignment” would require stitching
bits of SQL in a way that is not more easily checked for safety.

* Add migration hook to catch PostgreSQL versions earlier than 9.5

2 files changed, 58 insertions, 44 deletions

diff --git a/app/models/account_stat.rb b/app/models/account_stat.rb
index e70b54d79..a826a9af3 100644
--- a/app/models/account_stat.rb
+++ b/app/models/account_stat.rb
@@ -18,46 +18,4 @@ class AccountStat < ApplicationRecord
   belongs_to :account, inverse_of: :account_stat
 
   update_index('accounts#account', :account)
-
-  def increment_count!(key)
-    update(attributes_for_increment(key))
-  rescue ActiveRecord::StaleObjectError, ActiveRecord::RecordNotUnique
-    begin
-      reload_with_id
-    rescue ActiveRecord::RecordNotFound
-      return
-    end
-
-    retry
-  end
-
-  def decrement_count!(key)
-    update(attributes_for_decrement(key))
-  rescue ActiveRecord::StaleObjectError, ActiveRecord::RecordNotUnique
-    begin
-      reload_with_id
-    rescue ActiveRecord::RecordNotFound
-      return
-    end
-
-    retry
-  end
-
-  private
-
-  def attributes_for_increment(key)
-    attrs = { key => public_send(key) + 1 }
-    attrs[:last_status_at] = Time.now.utc if key == :statuses_count
-    attrs
-  end
-
-  def attributes_for_decrement(key)
-    attrs = { key => [public_send(key) - 1, 0].max }
-    attrs
-  end
-
-  def reload_with_id
-    self.id = self.class.find_by!(account: account).id if new_record?
-    reload
-  end
 end
diff --git a/app/models/concerns/account_counters.rb b/app/models/concerns/account_counters.rb
index 6e25e1905..fd3f161ad 100644
--- a/app/models/concerns/account_counters.rb
+++ b/app/models/concerns/account_counters.rb
@@ -3,6 +3,8 @@
 module AccountCounters
   extend ActiveSupport::Concern
 
+  ALLOWED_COUNTER_KEYS = %i(statuses_count following_count followers_count).freeze
+
   included do
     has_one :account_stat, inverse_of: :account
     after_save :save_account_stat
@@ -14,11 +16,65 @@ module AccountCounters
            :following_count=,
            :followers_count,
            :followers_count=,
-           :increment_count!,
-           :decrement_count!,
            :last_status_at,
            to: :account_stat
 
+  # @param [Symbol] key
+  def increment_count!(key)
+    update_count!(key, 1)
+  end
+
+  # @param [Symbol] key
+  def decrement_count!(key)
+    update_count!(key, -1)
+  end
+
+  # @param [Symbol] key
+  # @param [Integer] value
+  def update_count!(key, value)
+    raise ArgumentError, "Invalid key #{key}" unless ALLOWED_COUNTER_KEYS.include?(key)
+    raise ArgumentError, 'Do not call update_count! on dirty objects' if association(:account_stat).loaded? && account_stat&.changed? && account_stat.changed_attribute_names_to_save == %w(id)
+
+    value = value.to_i
+    default_value = value.positive? ? value : 0
+
+    # We do an upsert using manually written SQL, as Rails' upsert method does
+    # not seem to support writing expressions in the UPDATE clause, but only
+    # re-insert the provided values instead.
+    # Even ARel seem to be missing proper handling of upserts.
+    sql = if value.positive? && key == :statuses_count
+            <<-SQL.squish
+              INSERT INTO account_stats(account_id, #{key}, created_at, updated_at, last_status_at)
+                VALUES (:account_id, :default_value, now(), now(), now())
+              ON CONFLICT (account_id) DO UPDATE
+              SET #{key} = account_stats.#{key} + :value,
+                  last_status_at = now(),
+                  lock_version = account_stats.lock_version + 1,
+                  updated_at = now()
+              RETURNING id;
+            SQL
+          else
+            <<-SQL.squish
+              INSERT INTO account_stats(account_id, #{key}, created_at, updated_at)
+                VALUES (:account_id, :default_value, now(), now())
+              ON CONFLICT (account_id) DO UPDATE
+              SET #{key} = account_stats.#{key} + :value,
+                  lock_version = account_stats.lock_version + 1,
+                  updated_at = now()
+              RETURNING id;
+            SQL
+          end
+
+    sql = AccountStat.sanitize_sql([sql, account_id: id, default_value: default_value, value: value])
+    account_stat_id = AccountStat.connection.exec_query(sql)[0]['id']
+
+    # Reload account_stat if it was loaded, taking into account newly-created unsaved records
+    if association(:account_stat).loaded?
+      account_stat.id = account_stat_id if account_stat.new_record?
+      account_stat.reload
+    end
+  end
+
   def account_stat
     super || build_account_stat
   end