use UPSERT instead of select for hashtag statistics update - #17795
Conversation
dosubot
Bot
added
the
size:L
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughハッシュタグの6つのユーザーID配列カラムに空配列の既定値を追加し、関連ユーザーの追加・削除処理を条件付きSQLによる更新へ変更しています。 Changesハッシュタグ更新
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant 呼び出し元
participant HashtagService
participant hashtag_table
呼び出し元->>HashtagService: 添付・メンションの増減を指定
HashtagService->>hashtag_table: 条件付きUPSERTまたはUPDATE
hashtag_table-->>HashtagService: 配列と件数を更新
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #17795 +/- ##
============================================
+ Coverage 15.00% 26.53% +11.52%
============================================
Files 248 1183 +935
Lines 12402 40398 +27996
Branches 4224 11192 +6968
============================================
+ Hits 1861 10718 +8857
- Misses 8239 23818 +15579
- Partials 2302 5862 +3560 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
このPRによるapi.jsonの差分 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/migration/1784899839024-HashtagTableDefaults.js`:
- Around line 10-15: Backfill existing NULL values in all six hashtag user-ID
array columns to empty arrays before applying their defaults. In the migration’s
up flow, add updates for mentionedUserIds, mentionedLocalUserIds,
mentionedRemoteUserIds, attachedUserIds, attachedLocalUserIds, and
attachedRemoteUserIds, then retain the existing ALTER COLUMN ... SET DEFAULT
'{}' statements.
In `@packages/backend/src/core/HashtagService.ts`:
- Around line 103-159: Extend the backend hashtag tests covering the
`#incrementHashTag` and `#decrementHashTag` flows to verify that re-adding the same
user does not increase user counts or duplicate IDs, and removing a user who is
not recorded leaves the hashtag aggregation unchanged. Reuse the existing
hashtag retrieval/setup helpers and assert both stored user IDs and counts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 901b57bc-938e-4ce3-94be-511a4b2681fa
📒 Files selected for processing (3)
packages/backend/migration/1784899839024-HashtagTableDefaults.jspackages/backend/src/core/HashtagService.tspackages/backend/src/models/Hashtag.ts
| async #incrementHashTag( | ||
| user: { id: MiUser['id']; host: MiUser['host']; }, | ||
| tag: string, | ||
| columns: UpdatingHashtagColumn, | ||
| ) { | ||
| const isLocal = this.userEntityService.isLocalUser(user); | ||
| const { totalUserIds, totalUsersCount } = columns; | ||
| const localOrRemoteUserIds = isLocal ? columns.localUserIds : columns.remoteUserIds; | ||
| const localOrRemoteUserCount = isLocal ? columns.localUsersCount : columns.remoteUsersCount; | ||
|
|
||
| await this.db.createQueryRunner('master') | ||
| .query( | ||
| `INSERT into "hashtag"("id", "name", "${totalUserIds}", "${totalUsersCount}", "${localOrRemoteUserIds}", | ||
| "${localOrRemoteUserCount}") | ||
| VALUES ($3, $1, ARRAY [$2], 1, ARRAY [$2], 1) | ||
| ON CONFLICT ("name") | ||
| DO UPDATE SET "${totalUserIds}" = ${appendUserIdIfNotExists(totalUserIds)}, | ||
| "${totalUsersCount}" = ${incrementCountIfNotExists(totalUserIds, totalUsersCount)}, | ||
| "${localOrRemoteUserIds}" = ${appendUserIdIfNotExists(localOrRemoteUserIds)}, | ||
| "${localOrRemoteUserCount}" = ${incrementCountIfNotExists(localOrRemoteUserIds, localOrRemoteUserCount)}`, | ||
| [tag, user.id, this.idService.gen()], | ||
| ); | ||
|
|
||
| function appendUserIdIfNotExists(userIds: keyof MiHashtag & `${string}UserIds`): string { | ||
| return `CASE WHEN NOT ("hashtag"."${userIds}" @> ARRAY[$2 ::varchar]) THEN array_append("hashtag"."${userIds}", $2) ELSE "hashtag"."${userIds}" END`; | ||
| } | ||
|
|
||
| if (Object.keys(set).length > 0) { | ||
| await transactionalHashtagRepository | ||
| .createQueryBuilder() | ||
| .update() | ||
| .where('id = :id', { id: index.id }) | ||
| .set(set) | ||
| .execute(); | ||
| } | ||
| }); | ||
| function incrementCountIfNotExists(userIds: keyof MiHashtag & `${string}UserIds`, userCount: keyof MiHashtag & `${string}UsersCount`): string { | ||
| return `CASE WHEN NOT ("hashtag"."${userIds}" @> ARRAY[$2 ::varchar]) THEN "hashtag"."${userCount}" + 1 ELSE "hashtag"."${userCount}" END`; | ||
| } | ||
| } | ||
|
|
||
| async #decrementHashTag( | ||
| user: { id: MiUser['id']; host: MiUser['host']; }, | ||
| tag: string, | ||
| columns: UpdatingHashtagColumn, | ||
| ) { | ||
| const isLocal = this.userEntityService.isLocalUser(user); | ||
| const { totalUserIds, totalUsersCount } = columns; | ||
| const localOrRemoteUserIds = isLocal ? columns.localUserIds : columns.remoteUserIds; | ||
| const localOrRemoteUserCount = isLocal ? columns.localUsersCount : columns.remoteUsersCount; | ||
|
|
||
| await this.db.createQueryRunner('master') | ||
| .query( | ||
| `UPDATE "hashtag" | ||
| SET "${totalUserIds}" = array_remove("${totalUserIds}", $2), | ||
| "${totalUsersCount}" = ${decrementIfExists(totalUserIds, totalUsersCount)}, | ||
| "${localOrRemoteUserIds}" = array_remove("${localOrRemoteUserIds}", $2), | ||
| "${localOrRemoteUserCount}" = ${decrementIfExists(localOrRemoteUserIds, localOrRemoteUserCount)} | ||
| WHERE "name" = $1`, | ||
| [tag, user.id], | ||
| ); | ||
|
|
||
| function decrementIfExists(userIds: keyof MiHashtag & `${string}UserIds`, userCount: keyof MiHashtag & `${string}UsersCount`): string { | ||
| return `CASE WHEN ("${userIds}" @> ARRAY[$2]) THEN "${userCount}" - 1 ELSE "${userCount}" END`; | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# HashtagService の新しいUPSERT/UPDATEロジックに対するテストの有無を確認
rg -n -A3 -B3 'HashtagService' packages/backend/test --type=ts -g '!**/node_modules/**' 2>/dev/null
fd -e ts hashtag packages/backend/testRepository: misskey-dev/misskey
Length of output: 1584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 変更箇所と関連テストを確認
ast-grep outline packages/backend/src/core/HashtagService.ts --view expanded || true
echo '---'
ast-grep outline packages/backend/test/unit/entities/UserEntityService.ts --view expanded || true
echo '---'
rg -n "incrementHashTag|decrementHashTag|hashtag" packages/backend/test packages/backend/src/core -g '!**/node_modules/**'Repository: misskey-dev/misskey
Length of output: 9636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '820,890p' packages/backend/test/e2e/users.ts
echo '---'
rg -n "updateHashtags\(|updateUsertags\(|hashtags/users|hashtag" packages/backend/test/unit packages/backend/test/e2e -g '!**/node_modules/**'Repository: misskey-dev/misskey
Length of output: 5451
Hashtag 集計更新の境界ケースをテストで押さえてください
packages/backend/test/e2e/users.ts にはハッシュタグ経由の取得確認はありますが、今回の #incrementHashTag / #decrementHashTag で重要な「同一ユーザーの再追加でカウントが増えない」「存在しないユーザーの削除が no-op になる」ケースまでは見えていません。集計が崩れると影響が大きいので、これらを含むバックエンドテストの追加をお願いします。
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 112-123: Avoid SQL injection
Context: this.db.createQueryRunner('master')
.query(
INSERT into "hashtag"("id", "name", "${totalUserIds}", "${totalUsersCount}", "${localOrRemoteUserIds}", "${localOrRemoteUserCount}") VALUES ($3, $1, ARRAY [$2], 1, ARRAY [$2], 1) ON CONFLICT ("name") DO UPDATE SET "${totalUserIds}" = ${appendUserIdIfNotExists(totalUserIds)}, "${totalUsersCount}" = ${incrementCountIfNotExists(totalUserIds, totalUsersCount)}, "${localOrRemoteUserIds}" = ${appendUserIdIfNotExists(localOrRemoteUserIds)}, "${localOrRemoteUserCount}" = ${incrementCountIfNotExists(localOrRemoteUserIds, localOrRemoteUserCount)},
[tag, user.id, this.idService.gen()],
)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-typescript)
[error] 144-153: Avoid SQL injection
Context: this.db.createQueryRunner('master')
.query(
UPDATE "hashtag" SET "${totalUserIds}" = array_remove("${totalUserIds}", $2), "${totalUsersCount}" = ${decrementIfExists(totalUserIds, totalUsersCount)}, "${localOrRemoteUserIds}" = array_remove("${localOrRemoteUserIds}", $2), "${localOrRemoteUserCount}" = ${decrementIfExists(localOrRemoteUserIds, localOrRemoteUserCount)} WHERE "name" = $1,
[tag, user.id],
)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-typescript)
🪛 OpenGrep (1.25.0)
[ERROR] 113-124: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
[ERROR] 145-154: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.
(coderabbit.sql-injection.raw-query-concat-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/core/HashtagService.ts` around lines 103 - 159, Extend
the backend hashtag tests covering the `#incrementHashTag` and `#decrementHashTag`
flows to verify that re-adding the same user does not increase user counts or
duplicate IDs, and removing a user who is not recorded leaves the hashtag
aggregation unchanged. Reuse the existing hashtag retrieval/setup helpers and
assert both stored user IDs and counts.
Source: Coding guidelines
|
テストが失敗しとるわね |
|
queryrunnerのクリーンアップ忘れがありました。すみません。 |
238eec9 to
562a7bb
Compare
562a7bb to
24e479b
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/src/core/HashtagService.ts (1)
145-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winQueryRunner を
await usingで解放してください。削除側で
createQueryRunner('master')したままquery()しており、接続が返却されません。更新が重なると接続プールを圧迫するため、増分側と同様にawait usingに切り替えてください。修正案
- await this.db.createQueryRunner('master') - .query( + await using runner = this.db.createQueryRunner('master'); + await runner.query( `UPDATE "hashtag" SET "${totalUserIds}" = array_remove("${totalUserIds}", $2), "${totalUsersCount}" = ${decrementIfExists(totalUserIds, totalUsersCount)}, "${localOrRemoteUserIds}" = array_remove("${localOrRemoteUserIds}", $2), "${localOrRemoteUserCount}" = ${decrementIfExists(localOrRemoteUserIds, localOrRemoteUserCount)} WHERE "name" = $1`, [tag, user.id], - ); + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/src/core/HashtagService.ts` around lines 145 - 154, Update the delete-side query execution around createQueryRunner('master') to declare the QueryRunner with await using, matching the increment-side pattern, so it is automatically released after query() completes. Keep the existing SQL and parameters unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/backend/src/core/HashtagService.ts`:
- Around line 145-154: Update the delete-side query execution around
createQueryRunner('master') to declare the QueryRunner with await using,
matching the increment-side pattern, so it is automatically released after
query() completes. Keep the existing SQL and parameters unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13eac8aa-ffdb-44b8-95c2-28f5c42e4416
📒 Files selected for processing (1)
packages/backend/src/core/HashtagService.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/core/HashtagService.ts`:
- Around line 145-154: Await the `runner.query()` call in the hashtag update
flow so the database update completes before the `await using` query runner is
released and any failure propagates to the caller, matching the corresponding
increment path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b9ed7c85-a0ce-414f-b87f-4d2e921d35ee
📒 Files selected for processing (1)
packages/backend/src/core/HashtagService.ts
8855f71 to
3987428
Compare
|
await usingって22だとダメなのか... |
|
🙏🏻 |
2 participants
What
use UPSERT instead of select for hashtag statistics update to loos lock to
FOR NO KEY UPDATE(automatically acquired by db) and shorten the duration of lock by manipulating array on the server.Why
to
FOR NO KEY UPDATE(automatically acquired by db) and shorten the duration of lock by manipulating array on the server.内部 discussion で上がってきた懸念だったので
Additional info (optional)
データベースの構造を変えない前提で書いたのでこうなりましたが
UsersCount系カラムは GENERATED COLUMN にしたりそもそもカラムを廃止してcardinality(UserIds)を EXPRESSION INDEX にして ORDER BY 対応すると色々考えることが減っていいような気もしてます。また存在しない時にappendするという処理自体面倒なので TRIGGER 処理にしちゃうとか
Checklist