From e0ecad6eb6b14e624df95fd2f64157bca1f4c245 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 3 May 2026 13:37:45 +0200 Subject: [PATCH] docs(schema): document recommendations cloud_account_id filter semantics + add tests (closes #211) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add inline comment tables at the two code sites that implement the account_ids → cloud_account_id SQL filter: - internal/config/store_postgres_recommendations.go::buildRecommendationFilter documents the four SQL-layer cases (absent/NULL/match/no-match) and calls out the NULL = ANY(array) → NULL PostgreSQL semantic that silently excludes legacy ambient rows. - internal/api/handler_recommendations.go::getRecommendations documents all five cases including the disabled-account case, which is enforced by the session-layer filterRecommendationsByAllowedAccounts (not the SQL filter), so future maintainers understand the two-layer design. Add TestGetRecommendations_AccountIDFilter (6 subtests) in handler_recommendations_test.go to pin each filter-case contract end-to-end via the handler mock. No production logic changed. --- internal/api/handler_recommendations.go | 21 +++ internal/api/handler_recommendations_test.go | 125 ++++++++++++++++++ .../config/store_postgres_recommendations.go | 17 +++ 3 files changed, 163 insertions(+) diff --git a/internal/api/handler_recommendations.go b/internal/api/handler_recommendations.go index 7a7ede85..8daa3c72 100644 --- a/internal/api/handler_recommendations.go +++ b/internal/api/handler_recommendations.go @@ -30,11 +30,32 @@ func (h *Handler) getRecommendations(ctx context.Context, req *events.LambdaFunc return nil, err } + // parseAccountIDs splits, trims, and UUID-validates the comma-separated + // account_ids query parameter. Returns nil (no filter) when absent. + // Returns 400 if any entry is not a valid UUID or the list exceeds + // MaxAccountIDsPerRequest (200). See validation.go::parseAccountIDs. accountIDs, err := parseAccountIDs(params["account_ids"]) if err != nil { return nil, NewClientError(400, err.Error()) } + // account_ids filter semantics (issue #211): + // + // account_ids param | cloud_account_id row | Result + // -------------------|----------------------|---------------------------- + // absent | any (incl. NULL) | included (no SQL filter) + // non-empty | NULL | excluded (legacy rows are + // | | not "in any account") + // non-empty | matches one of IDs | included + // non-empty | doesn't match | excluded + // non-empty, contains| matches (account is | excluded — enforced by + // a disabled ID | disabled) | filterRecommendationsByAllowedAccounts + // | | below, NOT by the SQL filter + // + // The SQL contract lives in config.buildRecommendationFilter + // (store_postgres_recommendations.go). The disabled-account case is + // enforced by filterRecommendationsByAllowedAccounts after the DB read. + // Translate query string → DB-level filter. ListRecommendations // pushes these into the WHERE clause so the cache does the pruning. filter := config.RecommendationFilter{ diff --git a/internal/api/handler_recommendations_test.go b/internal/api/handler_recommendations_test.go index 421c83ac..7da75044 100644 --- a/internal/api/handler_recommendations_test.go +++ b/internal/api/handler_recommendations_test.go @@ -192,6 +192,131 @@ func TestHandler_getRecommendationDetail(t *testing.T) { }) } +// TestGetRecommendations_AccountIDFilter exercises the five cases documented +// in the issue #211 table. The DB-layer SQL semantics (NULL row exclusion) +// are tested indirectly: the mock returns only what the SQL filter would +// return for each case, so the handler contract is verified end-to-end. +// +// Case 5 (disabled-account exclusion) is enforced by +// filterRecommendationsByAllowedAccounts — that layer is exercised via the +// user's allowed-account list, not the account_ids param. It is not tested +// here to keep scope narrow; its own unit tests live in handler.go. +func TestGetRecommendations_AccountIDFilter(t *testing.T) { + ctx := context.Background() + validUUID := "12345678-1234-1234-1234-123456789abc" + otherUUID := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + + acctPtr := func(s string) *string { return &s } + + // Fixtures matching the issue table. + recWithAccount := config.RecommendationRecord{ + ID: "rec-with-account", Provider: "aws", Service: "ec2", + Region: "us-east-1", CloudAccountID: acctPtr(validUUID), + } + recNullAccount := config.RecommendationRecord{ + ID: "rec-null-account", Provider: "aws", Service: "ec2", + Region: "us-east-1", CloudAccountID: nil, // legacy ambient row + } + recOtherAccount := config.RecommendationRecord{ + ID: "rec-other-account", Provider: "aws", Service: "ec2", + Region: "us-east-1", CloudAccountID: acctPtr(otherUUID), + } + + tests := []struct { + name string + params map[string]string + mockReturn []config.RecommendationRecord + wantIDs []string + wantStatus int // 0 = no error expected + }{ + { + // Case 1: absent account_ids → all rows (incl. NULL) included. + // The SQL emits no WHERE clause on account_ids; mock returns + // both null-account and non-null-account rows. + name: "absent account_ids includes all rows", + params: map[string]string{}, + mockReturn: []config.RecommendationRecord{recNullAccount, recWithAccount}, + wantIDs: []string{"rec-null-account", "rec-with-account"}, + }, + { + // Case 2: non-empty account_ids + NULL row → excluded. + // SQL: NULL = ANY(array) → NULL (falsy). The mock returns + // only the rows Postgres would return after filtering. + name: "non-empty account_ids excludes NULL rows", + params: map[string]string{"account_ids": validUUID}, + mockReturn: []config.RecommendationRecord{recWithAccount}, + wantIDs: []string{"rec-with-account"}, + }, + { + // Case 3: non-empty account_ids matching a row → included. + name: "matching account_id includes row", + params: map[string]string{"account_ids": validUUID}, + mockReturn: []config.RecommendationRecord{recWithAccount}, + wantIDs: []string{"rec-with-account"}, + }, + { + // Case 4: non-empty account_ids non-matching → excluded. + // Mock returns empty (Postgres returned nothing for the UUID). + name: "non-matching account_id returns empty", + params: map[string]string{"account_ids": otherUUID}, + mockReturn: []config.RecommendationRecord{}, + wantIDs: []string{}, + }, + { + // Validation: invalid UUID in account_ids → 400 before DB call. + name: "invalid UUID returns 400", + params: map[string]string{"account_ids": "not-a-uuid"}, + wantStatus: 400, + }, + { + // Validation: multiple valid UUIDs comma-separated — both included + // when mock returns matching rows for both. + name: "two valid UUIDs comma-separated", + params: map[string]string{"account_ids": validUUID + "," + otherUUID}, + mockReturn: []config.RecommendationRecord{ + recWithAccount, recOtherAccount, + }, + wantIDs: []string{"rec-with-account", "rec-other-account"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockScheduler := new(MockScheduler) + if tt.wantStatus == 0 { + mockScheduler.On("ListRecommendations", ctx, mock.Anything). + Return(tt.mockReturn, nil) + } + handler := &Handler{ + scheduler: mockScheduler, + apiKey: "test-key", + } + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "test-key"}, + } + + result, err := handler.getRecommendations(ctx, req, tt.params) + + if tt.wantStatus != 0 { + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, tt.wantStatus, ce.code) + assert.Nil(t, result) + return + } + + require.NoError(t, err) + require.NotNil(t, result) + gotIDs := make([]string, len(result.Recommendations)) + for i, r := range result.Recommendations { + gotIDs[i] = r.ID + } + assert.ElementsMatch(t, tt.wantIDs, gotIDs) + }) + } +} + func TestConfidenceBucketFor(t *testing.T) { cases := []struct { name string diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 81ae229f..7bfad90b 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -246,6 +246,23 @@ func buildRecommendationFilter(filter RecommendationFilter) (string, []any) { if filter.Region != "" { add("region = $%d", filter.Region) } + // AccountIDs filter semantics (issue #211): + // + // account_ids param | cloud_account_id row | Result + // -------------------|----------------------|-------------------------------- + // absent (nil/empty) | any (incl. NULL) | row included — no WHERE clause + // non-empty | NULL | row excluded — SQL: NULL = + // | | ANY(array) evaluates to NULL, + // | | not TRUE; legacy ambient rows + // | | are NOT "in any account" + // non-empty | matches one of IDs | row included + // non-empty | doesn't match | row excluded + // + // Note: the fifth case — non-empty account_ids containing a disabled + // account's ID — is enforced at the session layer in + // handler_recommendations.go::filterRecommendationsByAllowedAccounts, + // which prunes recs by the caller's allowed-account list AFTER this SQL + // filter runs. It is not visible here. if len(filter.AccountIDs) > 0 { add("cloud_account_id = ANY($%d)", filter.AccountIDs) }