[IAmJSD]

Summary

Test-only change; no production code is touched.

The internal/analytics integration tests were remapped mechanically from the removed single-string account argument to a blanket AccountExternalIDsByProvider: {"aws": {...}, "gcp": {...}} and had never been checked at runtime. Every row they saved belonged to one external account and carried no cloud_account_id, so the only account-filter mutation they could catch was "match everything" (through the two non-existent-account cases). If the predicate matched the wrong column, dropped the UUID half, lost the per-provider scope, mishandled the legacy "" provider key, or a single Query* method ignored the filter, the suite still passed. The monthly-totals test only asserted NotNil (its comment said the view "may be empty", which is wrong: the refresh is synchronous), so an empty result passed too.

Changes

• New postgres_analytics_account_filter_db_test.go (TestPostgresAnalyticsStore_AccountFilterSemantics_DB). One container with two cloud_accounts rows and six snapshots. Each snapshot hits exactly one part of the dual-column predicate or is a decoy:

  • both columns set
  • external id only, with cloud_account_id NULL (the ambient/legacy shape)
  • UUID only (its account_id holds a number no filter names)
  • the same external number under gcp
  • another tenant's external id
  • another tenant's UUID

  Every row has its own service, so it is its own bucket in every query. Each case asserts the exact set of matched rows through all four methods: QuerySavings, QueryByProvider, QueryByService and QueryMonthlyTotals (after a real matview refresh). The cases are the call shapes resolveSingleAccountFilterIDs / resolveAccountFilterIDs actually produce:

  • known UUID plus its provider-scoped external id
  • UUID only
  • provider-scoped external id
  • the blanket two-provider remap
  • the legacy "" key
  • a provider with no matching rows

• Existing postgres_analytics_db_test.go / postgres_analytics_integration_test.go (the same edits in both; the files are near-duplicates):

  • Narrowed the remapped provider keys to the providers each test actually saved. QueryByService, BulkInsertSnapshots and QueryMonthlyTotals save only aws rows, so they now key only aws. The QuerySavings and QueryByProvider fixtures save both aws and gcp rows under the same number, so they keep both keys.
  • AccountUUIDs vs AccountExternalIDsByProvider: these fixtures save an external id and no cloud_account_id, so the external-id map is the correct filter for them. UUID matching is now covered by the new suite.
  • QueryMonthlyTotals: NotNil became an exact single-row check: account, nil cloud account, provider, service, savings and snapshot count.
  • QueryByProvider: NotEmpty became Len 2, plus the gcp bucket's value, so the gcp key is shown to match.

Verification

All of this is local integration evidence on real PostgreSQL 16 (testcontainers, Docker 28.2.2) on macOS. The Query* calls were invoked directly; the HTTP handler and real cloud accounts were not part of the runs.

• AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test -tags=integration ./internal/analytics/ -count=1 -v: all pass, 0 skips. Also passes with -race. Run before the change too: the existing suite passed, with 0 skips.
• Mutation probes: each mutation was applied in turn to internal/analytics/postgres_analytics.go and then reverted. The DB tests ran against the origin/main test files ("old") and against this branch ("new"). The harness also checked free disk before each run.

#	Production mutation	old suite	new suite
M1	drop the cloud_account_id = ANY(uuids) half	passes (missed)	fails
M2	drop the external-id half	fails (non-existent-account cases)	fails
M3	bind the UUIDs against account_id (the wrong-column trap)	missed	fails
M4	drop the per-provider scope on the external half	missed	fails
M5	skip the legacy "" provider key (falls through to TRUE)	missed	fails
M6	treat "" as a literal provider = ''	missed	fails
M7	QueryByProvider ignores the account filter	missed	fails
M8	QueryByService ignores the account filter	missed	fails
M9	QueryMonthlyTotals drops the UUID half	missed	fails
M10	QueryMonthlyTotals matches the external id against cloud_account_id	missed	fails (new suite plus both strengthened monthly tests)
M11	QueryByProvider honors only the first provider key	missed	fails (new suite plus both strengthened by-provider tests)
M12	QuerySavings ignores the account filter	fails (non-existent-account cases)	fails

The old suite caught 2 of 12; the new one catches all 12. M10 and M11 are localized on purpose, to show that the strengthened monthly and by-provider assertions in the existing files catch a mutation the old ones missed.

• go build ./...; go vet ./internal/analytics/ with and without -tags=integration.
• go test ./internal/analytics/ ./internal/mocks/ ./internal/database/postgres/testhelpers/ -count=1 passes. The last package is the chore(iac): drop deploy-role grants left unused by retiring the cleanup functions #1597 skip guard, which also parses the new file.
• golangci-lint v2.10.1 (--allow-parallel-runners, with and without --build-tags integration) on ./internal/analytics/...: 0 issues.
• gocyclo -over 10 -ignore "_test\.go" internal/analytics/: silent. No non-test code changed.
• git-secrets: scripts/setup-git-secrets.sh was run in a fresh git init copy of the tracked files with an isolated HOME. --scan is clean, and a probe line confirmed the AWS account-ID detector fires. The new 12-digit placeholders are named acct* so they don't trip it. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• gosec v2.28.0 (the hook's excludes, -tests -tags integration) on ./internal/analytics/: no findings.
• The AWS env overrides are only there because this machine's ~/.aws/config default profile is invalid. They don't affect these tests.

Concurrency: no schema, migration or write-path change, so no new race is plausible. The suite gives each test its own container.

Out of scope

• postgres_analytics_db_test.go and postgres_analytics_integration_test.go are near-duplicates (same cases; one uses RequirePostgresContainer plus the SKIP_DB_TESTS opt-out, the other uses SetupPostgresContainer). Merging them is left for a separate cleanup. postgres_analytics_db_test.go is also over the 500-line guideline, which is why the new suite is a separate file.
• internal/api/analytics_postgres.go has its own copy of accountFilterClause on the purchase_history path. This PR does not cover it.

Closes #32

Maintainers: please mirror the issue's triage labels onto this PR: priority/p2, severity/low, urgency/this-quarter, impact/internal, effort/m, type/chore, triaged.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 2d822f3
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: b0cec710-926c-4e66-b615-3609784dbbfc

📥 Commits

Reviewing files that changed from the base of the PR and between 2d822f3 and c04ba4b.

📒 Files selected for processing: 3

• internal/analytics/postgres_analytics_account_filter_db_test.go
• internal/analytics/postgres_analytics_db_test.go
• internal/analytics/postgres_analytics_integration_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE c04ba4b

Reviewed: git diff origin/main...c04ba4b6 (1 commit, 3 test files, +292/-12; largest commit 304 changed lines, under the 400 cap). Base 2d822f3 is stale; git merge-tree --write-tree origin/main c04ba4b6 (origin/main ca1e8a8, includes #1161) merges cleanly (tree 9882cc43). All tests below ran on that merged tree.

Findings

• nit internal/analytics/postgres_analytics_account_filter_db_test.go:103 - the "provider-scoped external id" case is now also the shape fix(api): scope a scoped user's raw analytics account id to its provider #1161 produces for a scoped user's raw id of a registered account (scopedAnalyticsAccountFilter, internal/api/handler_analytics.go: providerScopedExternalID, no UUID). The comments cite only resolveSingleAccountFilterIDs; naming the fix(api): scope a scoped user's raw analytics account id to its provider #1161 caller would document why the uuid-only row must stay out. No behaviour issue.
• nit internal/analytics/postgres_analytics_db_test.go / postgres_analytics_integration_test.go - same edits applied to both near-duplicate files (verified identical hunks); the PR already defers the merge of the two files.

Contract check after #1161: the store predicate (internal/analytics/postgres_analytics.go:53-80) is unchanged on main. #1161 changed only which filter the API builds; every shape it can emit (UUID plus provider-scoped ext, provider-scoped ext only, "" key for an id no registered account has) is one of the six cases, and the expectations ("" matches any provider, provider key pins the provider, UUID matches only cloud_account_id) match the current contract, not older behaviour.

Attacked:

• go vet -tags integration ./internal/analytics/: clean.
• go test -tags integration ./internal/analytics/ -count=1 (Docker 29.8.2, testcontainers): all pass, including the 6 new subtests and the strengthened QueryByProvider/QueryMonthlyTotals tests.
• Mutation probes in a scratch export of the merged tree, each applied alone and reverted:
  • "" key treated as provider = '': new legacy-key subtest fails.
  • provider scope dropped from the external half: 3 new subtests fail (UUID+ext, provider-scoped, azure-no-rows).
  • UUID half made a no-op: UUID+ext and UUID-only subtests fail.
  • QueryMonthlyTotals ignores the filter: all 6 new subtests fail, plus the existing non-existent-account monthly test.
• Month boundary: monthly assertions query 2 months (new suite) and 6 months (existing), so a run in the first hour of a month still sees the seeded row.
• Placeholder ids use acct* names; no 12-digit literal is bound to an account* identifier.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE c04ba4b

This is a test-only PR, and it does what #32 asks. On real Postgres the new suite fails under every filter mutation I tried, and the full package passes on head and on the head merged with origin/main.

Findings (none blocking):

• [non-blocking] internal/analytics/postgres_analytics_account_filter_db_test.go:77-128 - Every case filters on at most one UUID and one external id per provider. A predicate that bound only the first element of an ANY() array would not be caught. That is unlikely, because production builds the arrays with ANY($n).
• [non-blocking, out of scope per the body] Duplicated code:
  • postgres_analytics_db_test.go and postgres_analytics_integration_test.go are still near-duplicates, and the same edits went into both.
  • internal/api/analytics_postgres.go has its own accountFilterClause copy, which this suite does not cover.
  • Both could be follow-up issues.

Attacked:

• Issue checklist (test(analytics): verify runtime semantics of account-filter integration tests #32):
  • The suite runs under Docker with 0 skips.
  • The provider keys now match the providers each test saves:
    • QueryByService, BulkInsert and MonthlyTotals save only aws rows, so they use only the aws key.
    • QuerySavings and QueryByProvider save aws and gcp rows, so they keep both keys.
  • UUID matching is covered, because the fixture sets cloud_account_id.
  • A wrong-column guard exists (M3).
• Call shapes: the cases match what resolveSingleAccountFilterIDs and resolveAccountFilterIDs produce (internal/api/scoping.go:333-428):
  • a known UUID plus {provider: ext};
  • a UUID alone;
  • the legacy "" key.
• Fixture design:
  • Each row has its own service, so an exact-set assert per method is sound.
  • The azure "matches nothing" case catches a TRUE fallthrough.
  • sortedCopy(nil) gives []string{}, so the empty case compares correctly.
• Mutation probes on internal/analytics/postgres_analytics.go in a scratch copy, each restored and confirmed identical with cmp. Every probe failed the new suite:

#	Mutation	New-suite cases failing
M1	UUID half dropped	2
M3	UUIDs bound against account_id	2
M4	provider scope dropped	3
M5	"" key skipped	the legacy case
M6	"" treated as provider = ''	the legacy case
M7	QueryByProvider ignores the filter	all 6
M8	QueryByService ignores the filter	all 6
M9	QueryMonthlyTotals drops the UUID half	2

• Fail-before: not possible by copying, since postgres_analytics.go is identical on base and head. The mutations above stand in for it.
• Drift:
  • git merge-tree --write-tree origin/main <head> is clean.
  • Main added migrations 000131-000133, including 000132 (savings_snapshots partition UTC bounds), which touches these tables. So I also ran the full package on a merged-tree export.
• No migrations added. Test files only.

Verified locally, with no cloud credentials, under the heavy lock, on the repo's testcontainers harness:

• Head and merged tree:
  • go build ./...: OK.
  • go vet ./internal/analytics/, with and without -tags=integration: OK.
  • go test -count=1 ./internal/analytics/: ok.
• Formatting: gofmt -l on the 3 changed files: clean.
• Head: go test -race -count=1 -tags=integration -v ./internal/analytics/: PASS, 250 PASS lines, 0 SKIP.
• Merged: go test -count=1 -tags=integration -v ./internal/analytics/: PASS, 250 PASS lines, 0 SKIP.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Local verification at c04ba4b6ebe72769ecddee870ac5be8f38f6dc84 merged onto current main (a093db49; merge-tree df331861; macOS):

• go build ./..., go vet ./... and go vet -tags integration ./... pass.
• go test -race -short -count=1 on the packages this PR changes passes: ./internal/analytics.
• The full go test -race -short ./... suite passes on the tree with every PR in this merge train applied (tree 99ee50e2).
• Nothing was applied to any cloud.

[IAmJSD]

Marked ready and merged by IAmJSD's agent, at IAmJSD's request, after 2 independent reviews and local verification (see the review and verification comments above, all at c04ba4b6ebe72769ecddee870ac5be8f38f6dc84).