Skip to content
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

Use Rails upsert to generate update_count! query in Counters concern #28738

Merged
merged 4 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 24 additions & 31 deletions app/models/concerns/account/counters.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,40 +35,11 @@ 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(),
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,
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']
result = updated_account_stat(key, value.to_i)

# 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.id = result.first['id'] if account_stat.new_record?
account_stat.reload
end
end
Expand All @@ -79,6 +50,28 @@ def account_stat

private

def updated_account_stat(key, value)
AccountStat.upsert(
initial_values(key, value),
on_duplicate: Arel.sql(
duplicate_values(key, value).join(', ')
),
unique_by: :account_id
)
end

def initial_values(key, value)
{ :account_id => id, key => [value, 0].max }.tap do |values|
values.merge!(last_status_at: Time.current) if key == :statuses_count
end
end

def duplicate_values(key, value)
["#{key} = (account_stats.#{key} + #{value})", 'updated_at = CURRENT_TIMESTAMP'].tap do |values|
values << 'last_status_at = CURRENT_TIMESTAMP' if key == :statuses_count && value.positive?
end
end

def save_account_stat
return unless association(:account_stat).loaded? && account_stat&.changed?

Expand Down
13 changes: 13 additions & 0 deletions spec/models/concerns/account/counters_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,18 @@

expect(account.statuses_count).to eq 5
end

it 'preserves last_status_at when decrementing statuses_count' do
account_stat = Fabricate(
:account_stat,
account: account,
last_status_at: 3.days.ago,
statuses_count: 10
)

expect { account.decrement_count!(:statuses_count) }
.to change(account_stat.reload, :statuses_count).by(-1)
.and not_change(account_stat.reload, :last_status_at)
end
end
end