chore: increment count column in config access logs#1912
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughModified the ON CONFLICT update behavior in SaveConfigAccessLog to use a Set-based update approach. The change replaces AssignmentColumns with explicit updates to created_at, mfa, and properties from excluded values, while incrementing the count field by 1, maintaining the same WHERE gate condition. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
db/update.go (1)
1402-1413:⚠️ Potential issue | 🟠 Major
countwill not increment when conflicting record has an equal or oldercreated_atPostgreSQL's
ON CONFLICT DO UPDATE SET ... WHERE <cond>gates the entire SET block on the WHERE predicate. Whenexcluded.created_at <= config_access_logs.created_at, no assignment fires—including the newcount + 1. This causescountto silently skip incrementing for out-of-order or duplicate access events.The test suite ("Config access logs upsert") does not validate the
countfield behavior, creating ambiguity about intent:
- If
countshould track every conflict hit (total access events), the WHERE gate must be removed and the condition inlined as CASE expressions for conditional fields.- If
countshould only increment on "fresher" events (implicit deduplication), the current behavior is correct but should be explicitly documented.🛠️ Fix for unconditional count + conditional field updates
return ctx.DB().Clauses(clause.OnConflict{ Columns: []clause.Column{{Name: "config_id"}, {Name: "external_user_id"}, {Name: "scraper_id"}}, DoUpdates: clause.Set{ - {Column: clause.Column{Name: "created_at"}, Value: gorm.Expr("excluded.created_at")}, - {Column: clause.Column{Name: "mfa"}, Value: gorm.Expr("excluded.mfa")}, - {Column: clause.Column{Name: "properties"}, Value: gorm.Expr("excluded.properties")}, {Column: clause.Column{Name: "count"}, Value: gorm.Expr("config_access_logs.count + 1")}, + {Column: clause.Column{Name: "created_at"}, Value: gorm.Expr("CASE WHEN excluded.created_at > config_access_logs.created_at THEN excluded.created_at ELSE config_access_logs.created_at END")}, + {Column: clause.Column{Name: "mfa"}, Value: gorm.Expr("CASE WHEN excluded.created_at > config_access_logs.created_at THEN excluded.mfa ELSE config_access_logs.mfa END")}, + {Column: clause.Column{Name: "properties"}, Value: gorm.Expr("CASE WHEN excluded.created_at > config_access_logs.created_at THEN excluded.properties ELSE config_access_logs.properties END")}, }, - Where: clause.Where{Exprs: []clause.Expression{ - clause.Expr{SQL: "excluded.created_at > config_access_logs.created_at"}, - }}, }).Create(accessLog).ErrorClarify the intended semantics and add a test case that validates
countbehavior alongside the existing timestamp-based tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/update.go` around lines 1402 - 1413, The ON CONFLICT currently wraps the whole DoUpdates in a WHERE so count won't increment for conflicts where excluded.created_at <= config_access_logs.created_at; change the upsert so count is always incremented and only the timestamp/mfa/properties are conditionally updated based on created_at. Concretely, modify the clause.OnConflict in the upsert (the Create(accessLog) call) to remove the WHERE and replace the DoUpdates entries so count uses an unconditional increment (e.g., config_access_logs.count + 1) while created_at/mfa/properties are set with conditional expressions (CASE/GREATEST or CASE WHEN excluded.created_at > config_access_logs.created_at THEN excluded.<field> ELSE config_access_logs.<field> END). Also add a test in the "Config access logs upsert" suite that asserts count increments for conflicts with older/equal created_at to validate the intended semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@db/update.go`:
- Around line 1402-1413: The ON CONFLICT currently wraps the whole DoUpdates in
a WHERE so count won't increment for conflicts where excluded.created_at <=
config_access_logs.created_at; change the upsert so count is always incremented
and only the timestamp/mfa/properties are conditionally updated based on
created_at. Concretely, modify the clause.OnConflict in the upsert (the
Create(accessLog) call) to remove the WHERE and replace the DoUpdates entries so
count uses an unconditional increment (e.g., config_access_logs.count + 1) while
created_at/mfa/properties are set with conditional expressions (CASE/GREATEST or
CASE WHEN excluded.created_at > config_access_logs.created_at THEN
excluded.<field> ELSE config_access_logs.<field> END). Also add a test in the
"Config access logs upsert" suite that asserts count increments for conflicts
with older/equal created_at to validate the intended semantics.
yashmehrotra
force-pushed
the
cnt-ac-log
branch
from
9e11c3a to
2dbe628
Compare
2 participants
Summary by CodeRabbit