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

Conversation

mjankowski
Copy link
Contributor

Rails upsert support has improved (see notes in original impl here about lack of full support) and there now are options to support this operation via the framework.

There are specs for the increment_count! and decrement_count! methods in this class, which are the main users of the method and I think cover the two basic paths (with and without statuses_count being present).

Here's the before/after SQL for the general case when statuses_count is not present (formatting modified for comparison, but generated syntax not edited)...

Before:

INSERT INTO account_stats(account_id, followers_count, created_at, updated_at)
  VALUES (111760098696776148, 1, now(), now())
ON CONFLICT (account_id) DO UPDATE
  SET followers_count = account_stats.followers_count + 1, updated_at = now()
RETURNING id

After:

INSERT INTO "account_stats" ("account_id","followers_count","created_at","updated_at")
  VALUES (111760113389768635, 1, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)
ON CONFLICT ("account_id") DO UPDATE
  SET followers_count = (account_stats.followers_count + 1), updated_at = CURRENT_TIMESTAMP
RETURNING "id"

And when statuses_count is present...

Before:

INSERT INTO account_stats(account_id, statuses_count, created_at, updated_at, last_status_at)
  VALUES (111760098650761743, 1, now(), now(), now())
ON CONFLICT (account_id) DO UPDATE
  SET statuses_count = account_stats.statuses_count + 1, last_status_at = now(), updated_at = now()
RETURNING id

After:

INSERT INTO "account_stats" ("account_id","statuses_count","last_status_at","created_at","updated_at")
  VALUES (111760118307840353, 1, '2024-01-15 13:07:51.355159', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)
ON CONFLICT ("account_id") DO UPDATE
  SET statuses_count = (account_stats.statuses_count + 1), updated_at = CURRENT_TIMESTAMP, last_status_at = CURRENT_TIMESTAMP
RETURNING "id"

Differences here:

  • Quoting around columns/tables
  • The framework generates CURRENT_TIMESTAMP instead of NOW(), but I believe these are equivalent. I've used the former in the code so it matches framework.
  • We previously sent last_status_at = NOW() in our sql string for the scenario where we are creating the record for the first time and updating statuses_count and need a value for the status timestamp ... but I can't figure out how to do this in the hash args style, so I'm using Time.current on the ruby side. I suspect this is fine, since the previous implementation also allowed for possible gaps between the written value for the status and the value in this field, but just wanted to note it as a difference. We could potentially improve this by finding the status first, at the cost of an additional query.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.01%. Comparing base (86500e3) to head (353f72c).
Report is 205 commits behind head on main.

❗ Current head 353f72c differs from pull request most recent head bbcf607. Consider uploading reports for the commit bbcf607 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #28738      +/-   ##
==========================================
- Coverage   85.01%   85.01%   -0.01%     
==========================================
  Files        1059     1060       +1     
  Lines       28277    28286       +9     
  Branches     4538     4536       -2     
==========================================
+ Hits        24040    24047       +7     
- Misses       3074     3075       +1     
- Partials     1163     1164       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@renchap renchap added refactoring Improving code quality ruby Pull requests that update Ruby code labels Feb 16, 2024
@mjankowski mjankowski requested a review from a team March 27, 2024 18:27
app/models/concerns/account/counters.rb Outdated Show resolved Hide resolved
app/models/concerns/account/counters.rb Outdated Show resolved Hide resolved
@ClearlyClaire ClearlyClaire added this pull request to the merge queue Apr 17, 2024
Merged via the queue into mastodon:main with commit 6fed108 Apr 17, 2024
27 checks passed
@mjankowski mjankowski deleted the user-upsert-in-update-count branch April 17, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Improving code quality ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants