Skip to content

fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) #956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 5 commits into from
Jun 5, 2026
Merged

fix(api): filter purchases by account UUID AND external id (closes #701, #498, #866) #956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
970382d
fix(config/store): match purchase history on account UUID AND externa…
cristim Jun 4, 2026
bee4082
fix(api): filter purchases/analytics/inventory by account UUID AND ex…
cristim Jun 4, 2026
b1d18f3
test(api): cover dual-column account filtering across history/analyti…
cristim Jun 4, 2026
1613f95
refactor(api,config): extract helpers to satisfy gocyclo on account-f…
cristim Jun 4, 2026
5f0842f
refactor(api,config): revert ExternalIDsByProvider to flat ExternalID…
cristim Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitallowed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
111222333444
555666777888
999888777666
# Descending-step fixture added in #956 tests (store_postgres_pgxmock_test.go,
# handler_analytics_test.go, handler_history_test.go, handler_dashboard_test.go).
999988887777

# UUID-shaped account ID used in handler_accounts_test.go (synthetic, not a
# real subscription/account).
Expand Down
2 changes: 1 addition & 1 deletion internal/analytics/collector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func (m *mockConfigStore) GetAllPurchaseHistory(ctx context.Context, limit int)
return nil, nil
}

func (m *mockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, providerFilter string, accountIDs []string, start, end *time.Time, limit int) ([]config.PurchaseHistoryRecord, error) {
func (m *mockConfigStore) GetPurchaseHistoryFiltered(ctx context.Context, filter config.PurchaseHistoryFilter) ([]config.PurchaseHistoryRecord, error) {
return nil, nil
}

Expand Down
112 changes: 89 additions & 23 deletions internal/api/analytics_postgres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ package api
import (
"context"
"fmt"
"sort"
"strings"
"time"

"github.com/LeanerCloud/CUDly/internal/database"
Expand Down Expand Up @@ -74,13 +76,78 @@ func dimensionToColumn(dimension string) (string, error) {
}
}

// accountFilterClause builds the dual-column account WHERE fragment and the
// full positional arg list for the analytics aggregate queries. baseArgs holds
// the fixed leading binds ($1=start, $2=end); the account array binds (if any)
// are appended after them and the returned clause references them by the right
// positions.
//
// purchase_history carries two account identifiers, either of which may be the
// only one populated on a row: account_id (the cloud-provider external number,
// always populated) and cloud_account_id (the cloud_accounts UUID FK, NULL on
// every direct-execute/ambient/pre-000011 row). The top-bar chip emits the
// UUID, so matching only cloud_account_id silently dropped every NULL row
// (issue #701/#498) and matching only account_id dropped UUID-only rows (#866).
//
// The external-id half is grouped per provider so each external number only
// matches rows of its own provider:
//
// (cloud_account_id = ANY($u)
// OR (provider = $p AND account_id = ANY($extsForP)) OR ...)
//
// preserving the (provider, external_id) pairing so a filter for aws/123 never
// pulls azure/123 rows. The "" provider key (legacy raw external number, unknown
// provider) matches account_id with no provider gate. cloud_account_id is
// compared directly (no ::text cast) so the plain cloud_account_id UUID index
// (migration 000011) can back it. Providers are sorted for deterministic SQL.
// Both empty -> "TRUE" (no account filter).
func accountFilterClause(accountUUIDs []string, accountExternalIDsByProvider map[string][]string, baseArgs []any) (clause string, args []any) {
args = baseArgs
var ors []string
if len(accountUUIDs) > 0 {
args = append(args, accountUUIDs)
ors = append(ors, fmt.Sprintf("cloud_account_id = ANY($%d)", len(args)))
}
for _, provider := range sortedProviderKeys(accountExternalIDsByProvider) {
exts := accountExternalIDsByProvider[provider]
if len(exts) == 0 {
continue
}
if provider == "" {
args = append(args, exts)
ors = append(ors, fmt.Sprintf("account_id = ANY($%d)", len(args)))
continue
}
args = append(args, provider)
providerArg := len(args)
args = append(args, exts)
ors = append(ors, fmt.Sprintf("(provider = $%d AND account_id = ANY($%d))", providerArg, len(args)))
}
if len(ors) == 0 {
return "TRUE", args
}
return "(" + strings.Join(ors, " OR ") + ")", args
}

// sortedProviderKeys returns the map keys in ascending order so the generated
// SQL (and its bind-arg ordering) is deterministic across calls and testable.
func sortedProviderKeys(m map[string][]string) []string {
keys := make([]string, 0, len(m))
for k := range m {
keys = append(keys, k)
}
sort.Strings(keys)
return keys
}

// QueryHistory aggregates purchase_history rows bucketed by interval.
// accountID == "" means "all accounts accessible to the caller"; scoping
// is enforced upstream in the handler. Returns data points in ascending
// order and a summary covering the full window.
// Empty accountUUIDs AND accountExternalIDsByProvider means "all accounts
// accessible to the caller"; scoping is enforced upstream in the handler.
// Returns data points in ascending order and a summary covering the full window.
func (c *PostgresAnalyticsClient) QueryHistory(
ctx context.Context,
accountID string,
accountUUIDs []string,
accountExternalIDsByProvider map[string][]string,
start, end time.Time,
interval string,
) ([]HistoryDataPoint, *HistorySummary, error) {
Expand All @@ -91,16 +158,12 @@ func (c *PostgresAnalyticsClient) QueryHistory(

// Single query grouped by (bucket, service, provider) so we can assemble
// both the top-line bucket totals and the by_service / by_provider
// breakdowns without a second trip to the DB.
//
// account_id is the legacy VARCHAR(20) external-account identifier
// (e.g. a 12-digit AWS account number) written by older code paths.
// cloud_account_id is the UUID FK to cloud_accounts added in migration
// 000011 and written by all current purchase flows. The topbar Account
// chip sends the cloud_accounts.id UUID, so we match BOTH columns so
// that rows written by either path are included (issue #701).
// breakdowns without a second trip to the DB. The account predicate is the
// shared dual-column clause (see accountFilterClause).
//
// #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above.
// #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above and the
// account clause is parameter-bound (no user input interpolated).
accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{start, end})
query := fmt.Sprintf(`
SELECT date_trunc('%s', timestamp) AS bucket,
service,
Expand All @@ -111,12 +174,12 @@ func (c *PostgresAnalyticsClient) QueryHistory(
FROM purchase_history
WHERE timestamp >= $1
AND timestamp <= $2
AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3)
AND %s
GROUP BY bucket, service, provider
ORDER BY bucket ASC
`, unit)
`, unit, accountClause)

rows, err := c.db.Query(ctx, query, start, end, accountID)
rows, err := c.db.Query(ctx, query, args...)
if err != nil {
return nil, nil, fmt.Errorf("query purchase_history: %w", err)
}
Expand Down Expand Up @@ -184,7 +247,8 @@ func (c *PostgresAnalyticsClient) QueryHistory(
// bucket.
func (c *PostgresAnalyticsClient) QueryBreakdown(
ctx context.Context,
accountID string,
accountUUIDs []string,
accountExternalIDsByProvider map[string][]string,
start, end time.Time,
dimension string,
) (map[string]BreakdownValue, error) {
Expand All @@ -193,10 +257,12 @@ func (c *PostgresAnalyticsClient) QueryBreakdown(
return nil, err
}

// account_id / cloud_account_id dual-column match: see QueryHistory
// comment for rationale (issue #701).
// Dual-column account predicate: see accountFilterClause / QueryHistory for
// rationale (issue #701/#498/#866).
//
// #nosec G201 — `column` is allowlisted by dimensionToColumn above.
// #nosec G201 — `column` is allowlisted by dimensionToColumn above and the
// account clause is parameter-bound (no user input interpolated).
accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{start, end})
query := fmt.Sprintf(`
SELECT %s AS bucket,
SUM(estimated_savings)::float8 AS savings,
Expand All @@ -205,12 +271,12 @@ func (c *PostgresAnalyticsClient) QueryBreakdown(
FROM purchase_history
WHERE timestamp >= $1
AND timestamp <= $2
AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3)
AND %s
GROUP BY bucket
ORDER BY savings DESC
`, column)
`, column, accountClause)

rows, err := c.db.Query(ctx, query, start, end, accountID)
rows, err := c.db.Query(ctx, query, args...)
if err != nil {
return nil, fmt.Errorf("query breakdown: %w", err)
}
Expand Down
106 changes: 80 additions & 26 deletions internal/api/analytics_postgres_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,15 @@ func TestQueryHistory_Success(t *testing.T) {
AddRow(bucket1, "rds", "aws", 40.0, 10.0, 1).
AddRow(bucket2, "ec2", "aws", 75.0, 0.0, 1)

mock.ExpectQuery(`SELECT date_trunc\('day', timestamp\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "acct-1").
// External-id-only filter: predicate is (account_id = ANY($3)), bound to the
// single external id. No cloud_account_id half since no UUIDs supplied.
mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(account_id = ANY\(\$3\)\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{"acct-1"}).
WillReturnRows(rows)

start := time.Date(2026, 4, 20, 0, 0, 0, 0, time.UTC)
end := time.Date(2026, 4, 24, 0, 0, 0, 0, time.UTC)
points, summary, err := client.QueryHistory(ctx, "acct-1", start, end, "daily")
points, summary, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"acct-1"}}, start, end, "daily")
require.NoError(t, err)
require.Len(t, points, 2)

Expand Down Expand Up @@ -101,16 +103,17 @@ func TestQueryHistory_Success(t *testing.T) {

func TestQueryHistory_BadInterval(t *testing.T) {
client, _ := newMockAnalyticsClient(t)
_, _, err := client.QueryHistory(context.Background(), "", time.Now(), time.Now(), "yearly")
_, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "yearly")
assert.Error(t, err)
}

func TestQueryHistory_QueryError(t *testing.T) {
client, mock := newMockAnalyticsClient(t)
// No account filter: predicate degrades to TRUE, only start/end bound.
mock.ExpectQuery(`SELECT date_trunc`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "").
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).
WillReturnError(errors.New("db down"))
_, _, err := client.QueryHistory(context.Background(), "", time.Now(), time.Now(), "daily")
_, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "daily")
assert.Error(t, err)
assert.NoError(t, mock.ExpectationsWereMet())
}
Expand All @@ -122,10 +125,10 @@ func TestQueryBreakdown_Success(t *testing.T) {
AddRow("rds", 100.0, 50.0, 2)

mock.ExpectQuery(`SELECT service AS bucket`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "").
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).
WillReturnRows(rows)

out, err := client.QueryBreakdown(context.Background(), "",
out, err := client.QueryBreakdown(context.Background(), nil, nil,
time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC),
time.Date(2026, 12, 31, 0, 0, 0, 0, time.UTC),
"service")
Expand All @@ -146,7 +149,7 @@ func TestQueryBreakdown_Success(t *testing.T) {

func TestQueryBreakdown_BadDimension(t *testing.T) {
client, _ := newMockAnalyticsClient(t)
_, err := client.QueryBreakdown(context.Background(), "", time.Now(), time.Now(), "team")
_, err := client.QueryBreakdown(context.Background(), nil, nil, time.Now(), time.Now(), "team")
assert.Error(t, err)
}

Expand All @@ -156,36 +159,39 @@ func TestQueryBreakdown_ZeroTotalYieldsZeroPct(t *testing.T) {
AddRow("ec2", 0.0, 0.0, 0)

mock.ExpectQuery(`SELECT service AS bucket`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "").
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).
WillReturnRows(rows)

out, err := client.QueryBreakdown(context.Background(), "", time.Now(), time.Now(), "service")
out, err := client.QueryBreakdown(context.Background(), nil, nil, time.Now(), time.Now(), "service")
require.NoError(t, err)
assert.Equal(t, 0.0, out["ec2"].Percentage, "percentage must be 0 when total savings is 0")
assert.NoError(t, mock.ExpectationsWereMet())
}

// TestQueryHistory_CloudAccountIDFilter verifies that QueryHistory accepts a
// cloud_accounts UUID as the accountID parameter (issue #701). The WHERE
// clause matches on BOTH account_id (legacy VARCHAR(20) external ID) and
// cloud_account_id::text (UUID FK), so rows written by either code path are
// included. pgxmock validates the SQL shape and arg binding.
func TestQueryHistory_CloudAccountIDFilter(t *testing.T) {
// TestQueryHistory_DualColumnFilter verifies the dual-column account predicate
// (issue #701/#498/#866): when a UUID and its resolved external id (grouped
// under its provider) are both supplied, the WHERE clause ORs cloud_account_id =
// ANY($3) with (provider = $4 AND account_id = ANY($5)), so a row that carries
// only one of the two account representations is still aggregated while the
// external-id half stays provider-scoped. pgxmock validates the SQL shape and
// the array arg binding (cloud_account_id is compared directly, no ::text cast).
func TestQueryHistory_DualColumnFilter(t *testing.T) {
client, mock := newMockAnalyticsClient(t)
ctx := context.Background()
uuid := "aabbccdd-1234-5678-abcd-aabbccddee00"
external := "123456789012"

bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC)
rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}).
AddRow(bucket, "ec2", "aws", 50.0, 20.0, 1)

mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid).
mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(cloud_account_id = ANY\(\$3\) OR \(provider = \$4 AND account_id = ANY\(\$5\)\)\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, "aws", []string{external}).
WillReturnRows(rows)

start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC)
end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC)
points, summary, err := client.QueryHistory(ctx, uuid, start, end, "daily")
points, summary, err := client.QueryHistory(ctx, []string{uuid}, map[string][]string{"aws": {external}}, start, end, "daily")
require.NoError(t, err)
require.Len(t, points, 1)
assert.InDelta(t, 50.0, points[0].TotalSavings, 1e-9)
Expand All @@ -194,21 +200,22 @@ func TestQueryHistory_CloudAccountIDFilter(t *testing.T) {
assert.NoError(t, mock.ExpectationsWereMet())
}

// TestQueryBreakdown_CloudAccountIDFilter verifies that QueryBreakdown accepts
// a UUID as the accountID (issue #701). Mirrors TestQueryHistory_CloudAccountIDFilter.
func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) {
// TestQueryBreakdown_DualColumnFilter mirrors TestQueryHistory_DualColumnFilter
// for the breakdown aggregate (issue #701/#498/#866).
func TestQueryBreakdown_DualColumnFilter(t *testing.T) {
client, mock := newMockAnalyticsClient(t)
ctx := context.Background()
uuid := "aabbccdd-1234-5678-abcd-aabbccddee00"
external := "123456789012"

rows := mock.NewRows([]string{"bucket", "savings", "upfront", "purchases"}).
AddRow("rds", 200.0, 80.0, 3)

mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid).
mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(cloud_account_id = ANY\(\$3\) OR \(provider = \$4 AND account_id = ANY\(\$5\)\)\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, "aws", []string{external}).
WillReturnRows(rows)

out, err := client.QueryBreakdown(ctx, uuid,
out, err := client.QueryBreakdown(ctx, []string{uuid}, map[string][]string{"aws": {external}},
time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC),
time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC),
"service")
Expand All @@ -217,3 +224,50 @@ func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) {
assert.InDelta(t, 200.0, out["rds"].TotalSavings, 1e-9)
assert.NoError(t, mock.ExpectationsWereMet())
}

// TestQueryHistory_CrossProviderScoped is the analytics regression for issue
// #956 CR finding #1: when the same external id "123" exists under two providers
// the WHERE clause emits a separate provider-gated branch per provider (sorted),
// so an external id only matches rows of its own provider and aws/123 never
// pulls azure/123 rows. The args are bound in sorted-provider order.
func TestQueryHistory_CrossProviderScoped(t *testing.T) {
client, mock := newMockAnalyticsClient(t)
ctx := context.Background()

bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC)
rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}).
AddRow(bucket, "ec2", "aws", 10.0, 5.0, 1)

mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(\(provider = \$3 AND account_id = ANY\(\$4\)\) OR \(provider = \$5 AND account_id = ANY\(\$6\)\)\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "aws", []string{"123"}, "azure", []string{"123"}).
WillReturnRows(rows)

start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC)
end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC)
_, _, err := client.QueryHistory(ctx, nil, map[string][]string{"aws": {"123"}, "azure": {"123"}}, start, end, "daily")
require.NoError(t, err)
assert.NoError(t, mock.ExpectationsWereMet())
}

// TestQueryHistory_ExternalIDFilter verifies that when only external IDs are
// supplied, the WHERE clause emits account_id = ANY($3) bound to the flat slice,
// so a row that carries only account_id (cloud_account_id NULL) is still matched
// (issue #701/#498/#866).
func TestQueryHistory_ExternalIDFilter(t *testing.T) {
client, mock := newMockAnalyticsClient(t)
ctx := context.Background()

bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC)
rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}).
AddRow(bucket, "ec2", "aws", 10.0, 5.0, 1)

mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(account_id = ANY\(\$3\)\)`).
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{"123456789012"}).
WillReturnRows(rows)

start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC)
end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC)
_, _, err := client.QueryHistory(ctx, nil, map[string][]string{"": {"123456789012"}}, start, end, "daily")
require.NoError(t, err)
assert.NoError(t, mock.ExpectationsWereMet())
}
Loading
Loading