From c04ba4b6ebe72769ecddee870ac5be8f38f6dc84 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 00:16:15 +0100 Subject: [PATCH] test(analytics): verify account-filter semantics on real Postgres The analytics integration tests were remapped mechanically from the removed single-string account argument, and every row they saved belonged to one account, so a predicate that matched the wrong column, dropped a column, lost the provider scope or ignored the filter still passed them. Add a dual-column account-filter suite that seeds UUID-only, external-only, both-column, same-number-other-provider and other-tenant rows and asserts the exact matched set through QuerySavings, QueryByProvider, QueryByService and QueryMonthlyTotals for each resolver call shape. Narrow the remapped provider keys to the providers each existing test actually saved, and replace the vacuous NotNil/NotEmpty assertions on the monthly and by-provider tests with the exact rows. Closes #32 Co-Authored-By: Claude Opus 5.5 --- ...stgres_analytics_account_filter_db_test.go | 240 ++++++++++++++++++ .../analytics/postgres_analytics_db_test.go | 32 ++- .../postgres_analytics_integration_test.go | 32 ++- 3 files changed, 292 insertions(+), 12 deletions(-) create mode 100644 internal/analytics/postgres_analytics_account_filter_db_test.go diff --git a/internal/analytics/postgres_analytics_account_filter_db_test.go b/internal/analytics/postgres_analytics_account_filter_db_test.go new file mode 100644 index 000000000..f088609fd --- /dev/null +++ b/internal/analytics/postgres_analytics_account_filter_db_test.go @@ -0,0 +1,240 @@ +//go:build integration +// +build integration + +package analytics_test + +import ( + "context" + "sort" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/analytics" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #32: the account-filter integration tests were remapped mechanically +// from the removed single-string account argument, and every row they saved +// belonged to one account, so a filter that matched the wrong column, dropped a +// column, or matched everything still passed them. This suite seeds rows that +// each hit exactly one half of the dual-column predicate (cloud_account_id UUID +// vs per-provider account_id) plus decoys that only a broken predicate would +// return, and asserts the exact matched set through every Query* method. +// +// Each row gets its own service so it is its own bucket in every query's +// output (QuerySavings rows, QueryByProvider's (provider, service), +// QueryByService's (service, region), and monthly_savings_summary's +// (month, account, cloud account, provider, service)), which lets one service +// set identify exactly which rows each method matched. + +// External account numbers. Named acct* rather than account* so these +// placeholder literals don't look like AWS account-ID assignments to the +// git-secrets scanner. +const ( + acctUnderTest = "111111111111" // the account being filtered for + acctOther = "222222222222" // another tenant's AWS account + acctStale = "333333333333" // what a UUID-only row carries in account_id; no filter names it +) + +// Fixture services, one per seeded row. +const ( + svcBoth = "both-columns" // aws, acctUnderTest, cloud_account_id = its UUID + svcExtOnly = "external-id-only" // aws, acctUnderTest, cloud_account_id NULL (ambient/legacy row) + svcUUIDOnly = "uuid-only" // aws, acctStale, cloud_account_id = acctUnderTest's UUID + svcOtherProv = "gcp-same-number" // gcp, acctUnderTest, NULL: same external number, other provider + svcOtherExt = "other-external-id" // aws, acctOther, NULL + svcOtherUUID = "other-account-uuid" // aws, acctOther, cloud_account_id = acctOther's UUID +) + +// accountFilterWindow is the half-width of the query window around the seeded +// timestamp. +const accountFilterWindow = time.Hour + +// accountFilterFixture holds the seeded state the cases query against. +type accountFilterFixture struct { + store *analytics.PostgresAnalyticsStore + uuidUnder string // cloud_accounts.id of aws/acctUnderTest + ts time.Time +} + +func TestPostgresAnalyticsStore_AccountFilterSemantics_DB(t *testing.T) { + skipIfDBTestsOptedOut(t) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + container := testhelpers.RequirePostgresContainer(ctx, t) + defer container.Cleanup(ctx) + + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + + fx := seedAccountFilterFixture(ctx, t, container) + + cases := []struct { + name string + uuids []string + ext map[string][]string + want []string + }{ + { + // What resolveSingleAccountFilterIDs returns for a known account + // UUID that has an external id: both columns, the external half + // scoped to the account's provider. Fails if either half is + // dropped, if the UUIDs are bound against account_id (the + // documented wrong-column trap), or if the provider scope is lost + // (the GCP row with the same number would leak). + name: "known account UUID plus its provider-scoped external id", + uuids: []string{fx.uuidUnder}, + ext: map[string][]string{"aws": {acctUnderTest}}, + want: []string{svcBoth, svcExtOnly, svcUUIDOnly}, + }, + { + // A known account with no external id resolves to UUIDs only. Only + // cloud_account_id can match; binding the UUIDs against account_id + // would return nothing. + name: "account UUID only matches cloud_account_id", + uuids: []string{fx.uuidUnder}, + want: []string{svcBoth, svcUUIDOnly}, + }, + { + name: "provider-scoped external id matches account_id for that provider only", + ext: map[string][]string{"aws": {acctUnderTest}}, + want: []string{svcBoth, svcExtOnly}, + }, + { + // The blanket {"aws","gcp"} shape the compile-fix remap used: it + // matches each provider's rows for the number, and nothing else. + name: "external id keyed under two providers matches both, nothing else", + ext: map[string][]string{"aws": {acctUnderTest}, "gcp": {acctUnderTest}}, + want: []string{svcBoth, svcExtOnly, svcOtherProv}, + }, + { + // An unknown raw account number resolves to the "" key, which + // matches account_id on any provider. Skipping the "" key would + // leave no predicate and fall through to TRUE (every row); + // treating it as provider = '' would match nothing. + name: "legacy external id under the empty provider key matches any provider", + ext: map[string][]string{"": {acctUnderTest}}, + want: []string{svcBoth, svcExtOnly, svcOtherProv}, + }, + { + name: "external id under a provider that has no such rows matches nothing", + ext: map[string][]string{"azure": {acctUnderTest}}, + want: []string{}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fx.assertAllQueriesMatch(ctx, t, tc.uuids, tc.ext, tc.want) + }) + } +} + +// seedAccountFilterFixture inserts two cloud accounts and one snapshot per +// fixture service, then refreshes the materialized views so +// QueryMonthlyTotals sees them. +func seedAccountFilterFixture(ctx context.Context, t *testing.T, c *testhelpers.PostgresContainer) *accountFilterFixture { + t.Helper() + + insertAccount := func(name, external string) string { + var id string + require.NoError(t, c.DB.QueryRow(ctx, + `INSERT INTO cloud_accounts (name, provider, external_id) VALUES ($1, 'aws', $2) RETURNING id::text`, + name, external).Scan(&id)) + return id + } + uuidUnder := insertAccount("account under test", acctUnderTest) + uuidOther := insertAccount("other tenant", acctOther) + + fx := &accountFilterFixture{ + store: analytics.NewPostgresAnalyticsStore(c.DB), + uuidUnder: uuidUnder, + ts: time.Now().UTC().Truncate(time.Microsecond), + } + + rows := []struct { + service, provider, external string + cloud *string + savings float64 + }{ + {svcBoth, "aws", acctUnderTest, &uuidUnder, 1}, + {svcExtOnly, "aws", acctUnderTest, nil, 2}, + {svcUUIDOnly, "aws", acctStale, &uuidUnder, 4}, + {svcOtherProv, "gcp", acctUnderTest, nil, 8}, + {svcOtherExt, "aws", acctOther, nil, 16}, + {svcOtherUUID, "aws", acctOther, &uuidOther, 32}, + } + for _, r := range rows { + require.NoError(t, fx.store.SaveSnapshot(ctx, &analytics.SavingsSnapshot{ + AccountID: r.external, + CloudAccountID: r.cloud, + Timestamp: fx.ts, + Provider: r.provider, + Service: r.service, + Region: "us-east-1", + CommitmentType: "RI", + TotalSavings: r.savings, + })) + } + + require.NoError(t, fx.store.RefreshMaterializedViews(ctx)) + return fx +} + +// assertAllQueriesMatch runs one account filter through every Query* method +// and asserts each returns exactly the want services. +func (fx *accountFilterFixture) assertAllQueriesMatch(ctx context.Context, t *testing.T, uuids []string, ext map[string][]string, want []string) { + t.Helper() + + start, end := fx.ts.Add(-accountFilterWindow), fx.ts.Add(accountFilterWindow) + want = sortedCopy(want) + + snaps, err := fx.store.QuerySavings(ctx, analytics.QueryRequest{ + AccountUUIDs: uuids, + AccountExternalIDsByProvider: ext, + StartDate: start, + EndDate: end, + }) + require.NoError(t, err) + got := make([]string, 0, len(snaps)) + for _, s := range snaps { + got = append(got, s.Service) + } + assert.Equal(t, want, sortedCopy(got), "QuerySavings") + + byProvider, err := fx.store.QueryByProvider(ctx, uuids, ext, start, end) + require.NoError(t, err) + got = got[:0] + for _, b := range byProvider { + got = append(got, b.Service) + } + assert.Equal(t, want, sortedCopy(got), "QueryByProvider") + + byService, err := fx.store.QueryByService(ctx, uuids, ext, "", start, end) + require.NoError(t, err) + got = got[:0] + for _, b := range byService { + got = append(got, b.Service) + } + assert.Equal(t, want, sortedCopy(got), "QueryByService") + + // Two months, not one, so a run in the first hour of a month still sees + // the seeded row if NOW() has crossed into the next month since seeding. + monthly, err := fx.store.QueryMonthlyTotals(ctx, uuids, ext, 2) + require.NoError(t, err) + got = got[:0] + for _, m := range monthly { + got = append(got, m.Service) + } + assert.Equal(t, want, sortedCopy(got), "QueryMonthlyTotals") +} + +func sortedCopy(in []string) []string { + out := append([]string{}, in...) + sort.Strings(out) + return out +} diff --git a/internal/analytics/postgres_analytics_db_test.go b/internal/analytics/postgres_analytics_db_test.go index 5e364ec6d..549f0ff8f 100644 --- a/internal/analytics/postgres_analytics_db_test.go +++ b/internal/analytics/postgres_analytics_db_test.go @@ -323,7 +323,9 @@ func TestPostgresAnalyticsStore_QueryByProvider_DB(t *testing.T) { t.Run("query by provider aggregates correctly", func(t *testing.T) { breakdowns, err := store.QueryByProvider(ctx, nil, map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, now.Add(-24*time.Hour), now) require.NoError(t, err) - assert.NotEmpty(t, breakdowns) + // One bucket per provider: the gcp row shares the account number and + // is matched through the "gcp" key, the aws rows through "aws". + require.Len(t, breakdowns, 2) // Find aws/rds breakdown var found bool @@ -338,6 +340,15 @@ func TestPostgresAnalyticsStore_QueryByProvider_DB(t *testing.T) { } } assert.True(t, found, "aws/rds breakdown not found") + + var gcpFound bool + for _, b := range breakdowns { + if b.Provider == "gcp" && b.Service == "cloudsql" { + gcpFound = true + assert.InDelta(t, 150.00, b.TotalSavings, 0.01) + } + } + assert.True(t, gcpFound, "gcp/cloudsql breakdown not found") }) } @@ -389,7 +400,7 @@ func TestPostgresAnalyticsStore_QueryByService_DB(t *testing.T) { } t.Run("query by service groups by region", func(t *testing.T) { - breakdowns, err := store.QueryByService(ctx, nil, map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, "aws", now.Add(-24*time.Hour), now) + breakdowns, err := store.QueryByService(ctx, nil, map[string][]string{"aws": {"123456789012"}}, "aws", now.Add(-24*time.Hour), now) require.NoError(t, err) assert.Len(t, breakdowns, 2) // Two regions @@ -453,7 +464,7 @@ func TestPostgresAnalyticsStore_BulkInsertSnapshots_DB(t *testing.T) { // Verify data was inserted req := analytics.QueryRequest{ - AccountExternalIDsByProvider: map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, + AccountExternalIDsByProvider: map[string][]string{"aws": {"123456789012"}}, StartDate: now.Add(-24 * time.Hour), EndDate: now, } @@ -550,10 +561,19 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals_DB(t *testing.T) { require.NoError(t, err) t.Run("query monthly totals from materialized view", func(t *testing.T) { - summaries, err := store.QueryMonthlyTotals(ctx, nil, map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, 6) + summaries, err := store.QueryMonthlyTotals(ctx, nil, map[string][]string{"aws": {"123456789012"}}, 6) require.NoError(t, err) - // May be empty if materialized view refresh happens before data is visible - assert.NotNil(t, summaries) + // RefreshMaterializedViews above is synchronous, so the seeded aws/rds + // row is in the view. It carries no cloud_account_id, so it can only be + // matched through the provider-scoped external account_id (#32). + require.Len(t, summaries, 1) + s := summaries[0] + assert.Equal(t, "123456789012", s.AccountID) + assert.Nil(t, s.CloudAccountID) + assert.Equal(t, "aws", s.Provider) + assert.Equal(t, "rds", s.Service) + assert.InDelta(t, 200.00, s.TotalSavings, 0.01) + assert.Equal(t, 1, s.SnapshotCount) }) t.Run("query monthly totals for non-existent account", func(t *testing.T) { diff --git a/internal/analytics/postgres_analytics_integration_test.go b/internal/analytics/postgres_analytics_integration_test.go index 885cbc88b..bd207e041 100644 --- a/internal/analytics/postgres_analytics_integration_test.go +++ b/internal/analytics/postgres_analytics_integration_test.go @@ -265,10 +265,19 @@ func TestPostgresAnalyticsStore_QueryMonthlyTotals(t *testing.T) { require.NoError(t, err) t.Run("query monthly totals from materialized view", func(t *testing.T) { - summaries, err := store.QueryMonthlyTotals(ctx, nil, map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, 6) + summaries, err := store.QueryMonthlyTotals(ctx, nil, map[string][]string{"aws": {"123456789012"}}, 6) require.NoError(t, err) - // May be empty if materialized view refresh happens before data is visible - assert.NotNil(t, summaries) + // RefreshMaterializedViews above is synchronous, so the seeded aws/rds + // row is in the view. It carries no cloud_account_id, so it can only be + // matched through the provider-scoped external account_id (#32). + require.Len(t, summaries, 1) + s := summaries[0] + assert.Equal(t, "123456789012", s.AccountID) + assert.Nil(t, s.CloudAccountID) + assert.Equal(t, "aws", s.Provider) + assert.Equal(t, "rds", s.Service) + assert.InDelta(t, 200.00, s.TotalSavings, 0.01) + assert.Equal(t, 1, s.SnapshotCount) }) t.Run("query monthly totals for non-existent account", func(t *testing.T) { @@ -336,7 +345,9 @@ func TestPostgresAnalyticsStore_QueryByProvider(t *testing.T) { t.Run("query by provider aggregates correctly", func(t *testing.T) { breakdowns, err := store.QueryByProvider(ctx, nil, map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, now.Add(-24*time.Hour), now) require.NoError(t, err) - assert.NotEmpty(t, breakdowns) + // One bucket per provider: the gcp row shares the account number and + // is matched through the "gcp" key, the aws rows through "aws". + require.Len(t, breakdowns, 2) // Find aws/rds breakdown var found bool @@ -351,6 +362,15 @@ func TestPostgresAnalyticsStore_QueryByProvider(t *testing.T) { } } assert.True(t, found, "aws/rds breakdown not found") + + var gcpFound bool + for _, b := range breakdowns { + if b.Provider == "gcp" && b.Service == "cloudsql" { + gcpFound = true + assert.InDelta(t, 150.00, b.TotalSavings, 0.01) + } + } + assert.True(t, gcpFound, "gcp/cloudsql breakdown not found") }) } @@ -400,7 +420,7 @@ func TestPostgresAnalyticsStore_QueryByService(t *testing.T) { } t.Run("query by service groups by region", func(t *testing.T) { - breakdowns, err := store.QueryByService(ctx, nil, map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, "aws", now.Add(-24*time.Hour), now) + breakdowns, err := store.QueryByService(ctx, nil, map[string][]string{"aws": {"123456789012"}}, "aws", now.Add(-24*time.Hour), now) require.NoError(t, err) assert.Len(t, breakdowns, 2) // Two regions @@ -462,7 +482,7 @@ func TestPostgresAnalyticsStore_BulkInsertSnapshots(t *testing.T) { // Verify data was inserted req := analytics.QueryRequest{ - AccountExternalIDsByProvider: map[string][]string{"aws": {"123456789012"}, "gcp": {"123456789012"}}, + AccountExternalIDsByProvider: map[string][]string{"aws": {"123456789012"}}, StartDate: now.Add(-24 * time.Hour), EndDate: now, }