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, }