refactor(storage): unify the auditlog readers and cover MongoDB stores - #587
Merged
Conversation
…er rules loadFailoverConfig did env expansion, file IO, strict JSON decoding, a three-way merge and validation in one body, with the decode-then-merge step written out once per source. Each source is now a named function that returns nil when unconfigured, so the loader reads as four steps. The hand-rolled JSON decoder is split into decodeStrictJSONObject, whose doc comment states what it rejects that a plain Unmarshal accepts and why that needs the token stream. Behaviour is unchanged, including that an unset FAILOVER_RULES_JSON is no rules while an empty rules *file* stays a parse error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The resolver built the same key list twice on every request — once for the disabled check, once for the manual-rule lookup — and each build allocates a map and two slices. A requestIdentity value now carries the source model, its canonical key and the match keys, computed once. BenchmarkResolveFailovers: 330 -> 237 ns/op, 312 -> 216 B/op, 8 -> 6 allocs/op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
reader_sqlite.go (497) and reader_postgresql.go (395) were the same reader written twice, down to a duplicated latency predicate and a scan body the SQLite half repeated inline as well as in its own helper. One SQLReader now serves both, with a readerDialect value holding the six spellings that genuinely differ — ILIKE, the id casts a pre-unification PostgreSQL database still needs, the JSON path operators, the date-range boundary and the hour-bucketing expression — each carrying why. sqlx.Timestamp is the read side of TimestampArg: PostgreSQL returns a time.Time, SQLite the RFC3339 text that was written. It leaves an unparseable value invalid rather than failing the scan, so one bad row does not fail a page of results, as before. Two PostgreSQL JSON lookups now use the `#>>` spelling the indexes in jsonPathIndexes are built on. `data->'response_body'->>'id'` is a different expression to the planner, so those indexes could not be used. Coverage: the store tests paired with the reader ran on SQLite only. They now run through sqlxtest on both engines — auditlog goes from 0 to 19 PostgreSQL subtests. reader_postgresql_test.go, 205 lines of reflection-based fake pgx rows asserting NULL handling, is deleted: the same behaviour is now asserted against a real database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The admin handler set params.CacheMode = CacheModeCached before calling GetCacheOverview, which every implementation then overrides anyway. The duplication made the guarantee look like the caller's job, and the only thing asserting it was three admin tests reading it back off a stub reader that cannot enforce anything. The handler no longer sets it, the interface documents that the method overrides rather than honours CacheMode, and a new usage test asserts the real behaviour: passing "all" or "uncached" still returns cached rows only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MongoDB stays hand-written rather than sharing an implementation with the SQL backends, which makes it the one place a behaviour can drift unnoticed — and 12 of its 16 stores had no test that touched a database. mongotest mirrors sqlxtest: it hands a suite an empty database and drops it afterwards, skipping unless MONGO_TEST_DSN names a reachable server. Names are bounded to MongoDB's 64-byte limit and stripped of the characters it rejects, which a nested subtest name would otherwise overrun. Six domains — virtualmodels, failover, pricingoverrides, mcpgateway, batch, responsestore — now run their store round-trips through a suite written against the domain's Store interface, so the same assertions cover SQLite, PostgreSQL and MongoDB. Tests that reach past the interface for a raw handle stay SQL-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 16 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR tightens failover parsing and resolution, replaces separate SQLite and PostgreSQL audit readers with a unified SQL reader, adds SQL statistics and round-trip coverage, introduces MongoDB store testing, adds timestamp scanning, and clarifies cached overview behavior. ChangesFailover configuration and resolution
Unified SQL audit reader
Cross-backend store testing
Cache overview behavior
SQL timestamp scanning
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AuditFactory
participant SQLReader
participant SQLDatabase
AuditFactory->>SQLReader: construct reader from sqlx.DB
SQLReader->>SQLDatabase: query logs or request statistics
SQLDatabase-->>SQLReader: return rows and aggregates
SQLReader->>SQLDatabase: hydrate audit log attempts
SQLDatabase-->>SQLReader: return attempt snapshots
SQLReader-->>AuditFactory: return decoded audit data
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/auditlog/stats_sql.go`:
- Around line 84-106: Update statsHour.parse to return the time.ParseInLocation
error instead of swallowing it; wrap the original err with the existing
lower-level context while preserving the caller’s row-context error and
successful parsing behavior.
In `@internal/responsestore/store_sql_test.go`:
- Around line 36-42: Add a t.Cleanup callback immediately after NewSQLStore
succeeds in the sqlxtest.Run callback, invoking store.Close() and ignoring its
returned error. Keep the cleanup scoped to the SQL branch before calling body(t,
store).
In `@internal/storage/mongotest/mongotest.go`:
- Around line 31-33: Update the database naming logic around databaseCounter and
DatabaseName to include a process-unique component such as os.Getpid() in every
generated name, while retaining the atomic counter for concurrent subtests.
Adjust DatabaseName’s allocated length or formatting capacity to reserve space
for this additional suffix.
- Around line 61-76: Update the MongoDB setup and cleanup flow to use short
timeout contexts for Ping, Drop, and Disconnect instead of context.Background().
Ensure the timeout context is applied when validating the connection and in the
t.Cleanup callback, while preserving the existing skip-on-Ping-error and cleanup
behavior in the mongotest setup.
In `@internal/usage/reader_cache_mode_test.go`:
- Around line 64-66: Strengthen the assertions in the CacheModeAll test around
the existing overview.Summary check by also validating a cached-only miss,
request, or token aggregate. Use the expected aggregate for the single cached
row so incorrectly including the uncached fixture row causes the test to fail,
while preserving the existing TotalHits assertion.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bcc63615-8db0-4b5f-8417-b6181ef91dca
📒 Files selected for processing (29)
config/failover.gointernal/admin/handler_test.gointernal/admin/handler_usage.gointernal/auditlog/reader_factory.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_postgresql_test.gointernal/auditlog/reader_sql.gointernal/auditlog/reader_sql_boundary_test.gointernal/auditlog/reader_sqlite.gointernal/auditlog/reader_sqlite_boundary_test.gointernal/auditlog/roundtrip_sql_test.gointernal/auditlog/stats_postgresql.gointernal/auditlog/stats_sql.gointernal/auditlog/stats_sqlite.gointernal/auditlog/stats_test.gointernal/auditlog/store_sqlite_test.gointernal/batch/store_sql_test.gointernal/failover/resolver.gointernal/failover/resolver_bench_test.gointernal/failover/store_sql_test.gointernal/mcpgateway/store_sql_test.gointernal/pricingoverrides/store_sql_test.gointernal/responsestore/store_sql_test.gointernal/storage/mongotest/mongotest.gointernal/storage/sqlx/timestamp.gointernal/usage/reader.gointernal/usage/reader_cache_mode_test.gointernal/virtualmodels/helpers_test.gointernal/virtualmodels/store_test.go
💤 Files with no reviewable changes (9)
- internal/auditlog/store_sqlite_test.go
- internal/admin/handler_usage.go
- internal/auditlog/stats_postgresql.go
- internal/auditlog/reader_postgresql_test.go
- internal/auditlog/reader_sqlite_boundary_test.go
- internal/auditlog/reader_sqlite.go
- internal/auditlog/reader_postgresql.go
- internal/auditlog/stats_sqlite.go
- internal/admin/handler_test.go
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defects identified. The unified audit reader preserves prior dialect-specific SQL, timestamp, filtering, and legacy-schema behavior, while usage readers enforce the cached-only contract and failover refactors retain prior semantics.
 T-Rex LogsWhat T-Rex did
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Admin[Admin audit endpoints] --> Factory[Audit reader factory]
Factory --> SQL[Unified SQLReader]
Factory --> Mongo[MongoDB reader]
SQL --> Dialect{SQL dialect}
Dialect --> SQLite[SQLite expressions and timestamp bounds]
Dialect --> PostgreSQL[PostgreSQL expressions and UUID casts]
SQLite --> AuditDB[(audit_logs and attempts)]
PostgreSQL --> AuditDB
Reviews (1): Last reviewed commit: "test(storage): run store suites against ..." | Re-trigger Greptile |
- mongotest database names now carry the pid. The counter only separates subtests inside one process, and `go test ./...` runs packages in parallel — batch and responsestore both define TestStoreDelete, so two processes could create *and drop* the same database. A new test pins the naming rules: under 64 bytes, no character MongoDB rejects, pid and counter present. - Every mongotest server call is bounded, so an unreachable DSN skips promptly instead of stalling every opted-in suite, and cleanup cannot hang the test binary. - runStoreSuite closes the store it built. responsestore's SQL store runs a retention goroutine that the SQL-only helper stopped and the shared one did not. - The cache-mode test asserts token totals, not just hit counts: only one fixture row is a cache hit either way, so a mode that wrongly widened to both rows still counted 1. Verified it now fails without the reader's override. - The request-stats hour parse error is carried to the caller instead of being dropped, so the failure names the reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sqlx.Timestamp deliberately does not normalise on read, so the guarantee has to hold on the way in. This asserts it against the column itself, not the reader's interpretation: a caller writing 14:30+02:00 must leave 12:30 with a UTC marker in the row, on SQLite text and PostgreSQL timestamptz alike, and the instant must survive the round trip for both the entry and its attempts. Without it, a write that bypassed Dialect.TimestampArg would store a local-offset string on SQLite that sorts wrongly against every other row, and the date-range filters would quietly return the wrong day. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Writes are UTC on every backend and now have a test asserting it against the stored column. Reads are not normalised, so the same entry serialises with a Z on SQLite and a server-local offset on PostgreSQL. Pre-existing, same instant either way, and normalising would change the timestamp string in every PostgreSQL deployment's admin API responses — so it is a separate decision rather than part of a refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Three of its deferred items were done in #587 and its MongoDB coverage numbers were stale, so the one section anyone reads was actively wrong. Removed what had a better home or no longer applied: a baseline snapshot of a commit two releases back, line-count tables git already holds, a paragraph on sqlxtest duplicating that package's own doc comment, the driver binding facts now stated on the type tokens, and a reconciliation against a file deleted in #586. 261 lines to 112, and renamed, since it stopped being a survey the moment the survey was acted on. Also fixes the dangling pointer to that deleted file in the 2026-07-04 architecture review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
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 `@internal/auditlog/timestamp_utc_test.go`:
- Around line 22-84: Refactor TestStoreWritesTimestampsInUTC into table-driven
cases covering UTC, Europe/Warsaw standard time, and Europe/Warsaw
daylight-saving time inputs. Run the existing write, raw stored-value,
round-trip, and hydrated-attempt assertions for each case, deriving the expected
UTC wall-clock value and preserving the current instant-equality checks.
- Around line 45-60: Update the raw timestamp assertions in the timestamp UTC
test to project PostgreSQL values in UTC, avoiding dependence on the session
time zone while preserving the existing SQLite behavior. Extend the same
wall-clock and UTC-marker checks to the raw started_at value in
audit_log_attempts, using the attempt created by the test, so non-UTC attempt
writes are detected.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3898fb78-642b-4092-a137-d3ccc893a4d3
📒 Files selected for processing (2)
docs/dev/2026-07-25_backend-refactor-survey.mdinternal/auditlog/timestamp_utc_test.go
The implementation notes told contributors to look in cmd/gomodel/main.go for the SetHooks call, twice. It moved to run/providers.go (defaultProviderFactory). A link check would not catch it: the file still exists, it just no longer contains what the doc says it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
api-examples.md cited five retired models, so a chunk of it failed on paste. tests/e2e/release-e2e-scenarios.md covers the same ground executably and is verified every release, so the hand-maintained copy could only fall further behind it. 2026-03-16_ARCHITECTURE_SNAPSHOT.md predated the MCP gateway, rate limiting, provider credentials and the storage refactor. The 2026-07-04 architecture review had already flagged both dated snapshots as looking authoritative while stale; this executes that for the one in docs/dev and records that its sibling in docs/ still has the problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…me zone The check read timestamp::text, and PostgreSQL renders TIMESTAMPTZ in the *session* time zone — so it asserted the UTC wall clock only because this container happens to default to UTC. Against a server on Europe/Warsaw it failed on perfectly correct data: stored timestamp = "2026-07-25 14:30:00.123456+02", want the 12:30 UTC wall clock Both verified: the previous assertion fails under PGTZ=Europe/Warsaw, the projection through AT TIME ZONE 'UTC' passes under both. Also covers audit_log_attempts.started_at, which was only checked through the hydrated instant, and makes the cases table-driven across UTC, a summer +02:00 offset and a winter +01:00 one, so the contract is exercised either side of a DST change rather than at a single offset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #586, working through the deferred list in
docs/dev/2026-07-25_backend-refactor-survey.md§7.What is here
config/failover.go— separated loading, decoding and validation. One function did env expansion, file IO, strict JSON decoding, a three-way merge and validation, with decode-then-merge written out once per source. Each source is now a named function returning nil when unconfigured. The hand-rolled decoder is split out, with a doc comment stating what it rejects that a plainUnmarshalaccepts, and why that needs the token stream. Behaviour is unchanged, including that an unsetFAILOVER_RULES_JSONis no rules while an empty rules file stays a parse error.failover/resolver.go— match keys computed once per request. The resolver built the same key list twice on every request (once for the disabled check, once for the manual-rule lookup), each build allocating a map and two slices.BenchmarkResolveFailovers: 330 → 237 ns/op, 312 → 216 B/op, 8 → 6 allocs/op.auditlog— one reader for both SQL backends.reader_sqlite.go(497) andreader_postgresql.go(395) were the same reader written twice, down to a duplicated latency predicate and a scan body the SQLite half also repeated inline. AreaderDialectvalue holds the six spellings that genuinely differ, each carrying why.sqlx.Timestampis the read side ofTimestampArg.Two PostgreSQL JSON lookups now use the
#>>spelling the indexes are built on —data->'response_body'->>'id'is a different expression to the planner, so those indexes could not be used.usage—GetCacheOverviewowns its cached-only scope. The admin handler setCacheModebefore calling a method that overrides it anyway; the only thing asserting the guarantee was three admin tests reading it back off a stub reader that cannot enforce anything. The interface now documents it and a usage test asserts the real behaviour.MongoDB store coverage. 12 of 16 MongoDB stores had no test that touched a database.
internal/storage/mongotestmirrorssqlxtest; six domains now run their store round-trips through a suite written against the domain'sStoreinterface.Coverage
Net −268 lines across 29 files.
Not done
The
usagestore and readers. Attempted and reverted rather than landed half-finished.RecalculatePricingis a store method whose row filter is built by the readers'sqliteUsageConditions/pgUsageConditions, and those emit different placeholder styles — so routing the unified store throughsqlxforces the reader migration too. That is ~2,100 lines of implementation plus ~1,500 lines of tests, over the dashboard's analytics surface, and it wants its own PR. The survey's conclusion that "usage should follow its reader rather than lead it" holds.Verified locally:
make test-racewith bothGOMODEL_TEST_POSTGRES_URLandMONGO_TEST_DSNset,make lint0 issues,make fix-check, perf guard.🤖 Generated with Claude Code
Summary by CodeRabbit