Skip to content

Commit c7f9e75

Browse files
cristimcristim
committed
test(api): cover dual-column account filtering across history/analytics/dashboard/inventory (refs #701, #498, #866)
Update the api-package mocks and handler tests to the dual-column signatures and add end-to-end keystone regression tests for the "Account B returns nothing" bug class: - handler_history_test.go: TestHandler_getHistory_ExternalIDOnlyAccount asserts /api/history with account_ids=<B-UUID> returns B's external-id-only rows; TestMatchesExecution_ExternalIDOnlyPending asserts a pending execution carrying only B's external id survives the in-memory filter. - handler_dashboard_test.go: external-id-only commitment rows are counted via the dual-column calculateCommitmentMetrics path. - handler_analytics_test.go: TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount asserts a chip UUID resolves to (UUID, external) and both reach QueryHistory. - analytics_postgres_test.go: dual-column QueryHistory/QueryBreakdown SQL shape and array binding. These tests fail before the fix (single-column filter drops the rows) and pass after.
1 parent a0370fb commit c7f9e75

8 files changed

Lines changed: 265 additions & 104 deletions

‎internal/api/analytics_postgres_test.go‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,15 @@ func TestQueryHistory_Success(t *testing.T) {
6464
AddRow(bucket1, "rds", "aws", 40.0, 10.0, 1).
6565
AddRow(bucket2, "ec2", "aws", 75.0, 0.0, 1)
6666

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

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

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

102104
func TestQueryHistory_BadInterval(t *testing.T) {
103105
client, _ := newMockAnalyticsClient(t)
104-
_, _, err := client.QueryHistory(context.Background(), "", time.Now(), time.Now(), "yearly")
106+
_, _, err := client.QueryHistory(context.Background(), nil, nil, time.Now(), time.Now(), "yearly")
105107
assert.Error(t, err)
106108
}
107109

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

124127
mock.ExpectQuery(`SELECT service AS bucket`).
125-
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "").
128+
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).
126129
WillReturnRows(rows)
127130

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

147150
func TestQueryBreakdown_BadDimension(t *testing.T) {
148151
client, _ := newMockAnalyticsClient(t)
149-
_, err := client.QueryBreakdown(context.Background(), "", time.Now(), time.Now(), "team")
152+
_, err := client.QueryBreakdown(context.Background(), nil, nil, time.Now(), time.Now(), "team")
150153
assert.Error(t, err)
151154
}
152155

@@ -156,36 +159,38 @@ func TestQueryBreakdown_ZeroTotalYieldsZeroPct(t *testing.T) {
156159
AddRow("ec2", 0.0, 0.0, 0)
157160

158161
mock.ExpectQuery(`SELECT service AS bucket`).
159-
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), "").
162+
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).
160163
WillReturnRows(rows)
161164

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

168-
// TestQueryHistory_CloudAccountIDFilter verifies that QueryHistory accepts a
169-
// cloud_accounts UUID as the accountID parameter (issue #701). The WHERE
170-
// clause matches on BOTH account_id (legacy VARCHAR(20) external ID) and
171-
// cloud_account_id::text (UUID FK), so rows written by either code path are
172-
// included. pgxmock validates the SQL shape and arg binding.
173-
func TestQueryHistory_CloudAccountIDFilter(t *testing.T) {
171+
// TestQueryHistory_DualColumnFilter verifies the dual-column account predicate
172+
// (issue #701/#498/#866): when a UUID and its resolved external id are both
173+
// supplied, the WHERE clause ORs cloud_account_id::text = ANY($3) with
174+
// account_id = ANY($4), so a row that carries only one of the two account
175+
// representations is still aggregated. pgxmock validates the SQL shape and the
176+
// array arg binding.
177+
func TestQueryHistory_DualColumnFilter(t *testing.T) {
174178
client, mock := newMockAnalyticsClient(t)
175179
ctx := context.Background()
176180
uuid := "aabbccdd-1234-5678-abcd-aabbccddee00"
181+
external := "123456789012"
177182

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

182-
mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`).
183-
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid).
187+
mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(cloud_account_id::text = ANY\(\$3\) OR account_id = ANY\(\$4\)\)`).
188+
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, []string{external}).
184189
WillReturnRows(rows)
185190

186191
start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC)
187192
end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC)
188-
points, summary, err := client.QueryHistory(ctx, uuid, start, end, "daily")
193+
points, summary, err := client.QueryHistory(ctx, []string{uuid}, []string{external}, start, end, "daily")
189194
require.NoError(t, err)
190195
require.Len(t, points, 1)
191196
assert.InDelta(t, 50.0, points[0].TotalSavings, 1e-9)
@@ -194,21 +199,22 @@ func TestQueryHistory_CloudAccountIDFilter(t *testing.T) {
194199
assert.NoError(t, mock.ExpectationsWereMet())
195200
}
196201

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

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

207-
mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`).
208-
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid).
213+
mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(cloud_account_id::text = ANY\(\$3\) OR account_id = ANY\(\$4\)\)`).
214+
WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), []string{uuid}, []string{external}).
209215
WillReturnRows(rows)
210216

211-
out, err := client.QueryBreakdown(ctx, uuid,
217+
out, err := client.QueryBreakdown(ctx, []string{uuid}, []string{external},
212218
time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC),
213219
time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC),
214220
"service")

‎internal/api/handler_analytics_test.go‎

Lines changed: 50 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77
"time"
88

9+
"github.com/LeanerCloud/CUDly/internal/config"
910
"github.com/aws/aws-lambda-go/events"
1011
"github.com/stretchr/testify/assert"
1112
"github.com/stretchr/testify/mock"
@@ -17,8 +18,8 @@ type MockAnalyticsClient struct {
1718
mock.Mock
1819
}
1920

20-
func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountID string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) {
21-
args := m.Called(ctx, accountID, start, end, interval)
21+
func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountUUIDs, accountExternalIDs []string, start, end time.Time, interval string) ([]HistoryDataPoint, *HistorySummary, error) {
22+
args := m.Called(ctx, accountUUIDs, accountExternalIDs, start, end, interval)
2223
if args.Get(0) == nil {
2324
return nil, nil, args.Error(2)
2425
}
@@ -29,14 +30,41 @@ func (m *MockAnalyticsClient) QueryHistory(ctx context.Context, accountID string
2930
return args.Get(0).([]HistoryDataPoint), summary, args.Error(2)
3031
}
3132

32-
func (m *MockAnalyticsClient) QueryBreakdown(ctx context.Context, accountID string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) {
33-
args := m.Called(ctx, accountID, start, end, dimension)
33+
func (m *MockAnalyticsClient) QueryBreakdown(ctx context.Context, accountUUIDs, accountExternalIDs []string, start, end time.Time, dimension string) (map[string]BreakdownValue, error) {
34+
args := m.Called(ctx, accountUUIDs, accountExternalIDs, start, end, dimension)
3435
if args.Get(0) == nil {
3536
return nil, args.Error(1)
3637
}
3738
return args.Get(0).(map[string]BreakdownValue), args.Error(1)
3839
}
3940

41+
// TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount is the analytics
42+
// keystone (issue #701/#498/#866): selecting an account by its cloud_accounts
43+
// UUID resolves to (UUID, external_id) and both reach QueryHistory so rows that
44+
// carry only the external account_id (cloud_account_id NULL) are aggregated.
45+
func TestHandler_getHistoryAnalytics_ExternalIDOnlyAccount(t *testing.T) {
46+
ctx := context.Background()
47+
accountUUID := "bbbbbbbb-1111-2222-3333-444444444444"
48+
accountExternal := "999988887777"
49+
50+
mockClient := new(MockAnalyticsClient)
51+
mockClient.On("QueryHistory", ctx, []string{accountUUID}, []string{accountExternal}, mock.Anything, mock.Anything, "hourly").
52+
Return([]HistoryDataPoint{{TotalSavings: 50.0, PurchaseCount: 1}}, &HistorySummary{TotalPurchases: 1}, nil)
53+
54+
mockStore := new(MockConfigStore)
55+
mockStore.ListCloudAccountsFn = func(_ context.Context, _ config.CloudAccountFilter) ([]config.CloudAccount, error) {
56+
return []config.CloudAccount{{ID: accountUUID, Name: "Account B", Provider: "aws", ExternalID: accountExternal}}, nil
57+
}
58+
59+
mockAuth, req := adminAnalyticsReq(ctx)
60+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: mockStore}
61+
62+
result, err := handler.getHistoryAnalytics(ctx, req, map[string]string{"account_id": accountUUID})
63+
require.NoError(t, err)
64+
require.NotNil(t, result)
65+
mockClient.AssertCalled(t, "QueryHistory", ctx, []string{accountUUID}, []string{accountExternal}, mock.Anything, mock.Anything, "hourly")
66+
}
67+
4068
// MockAnalyticsCollector is a mock implementation of AnalyticsCollectorInterface
4169
type MockAnalyticsCollector struct {
4270
mock.Mock
@@ -72,10 +100,13 @@ func TestHandler_getHistoryAnalytics_Success(t *testing.T) {
72100
}
73101
summary := &HistorySummary{TotalPurchases: 8, TotalMonthlySavings: 190.0}
74102

75-
mockClient.On("QueryHistory", ctx, "account-123", mock.Anything, mock.Anything, "hourly").Return(dataPoints, summary, nil)
103+
// "account-123" is not a known UUID, so resolveSingleAccountFilterIDs
104+
// treats it as an external account number: QueryHistory is called with no
105+
// UUIDs and account-123 as the external id.
106+
mockClient.On("QueryHistory", ctx, []string(nil), []string{"account-123"}, mock.Anything, mock.Anything, "hourly").Return(dataPoints, summary, nil)
76107

77108
mockAuth, req := adminAnalyticsReq(ctx)
78-
handler := &Handler{auth: mockAuth, analyticsClient: mockClient}
109+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
79110

80111
params := map[string]string{
81112
"account_id": "account-123",
@@ -107,17 +138,18 @@ func TestHandler_getHistoryAnalytics_DefaultInterval(t *testing.T) {
107138
mockClient := new(MockAnalyticsClient)
108139

109140
dataPoints := []HistoryDataPoint{}
110-
mockClient.On("QueryHistory", ctx, "", mock.Anything, mock.Anything, "hourly").Return(dataPoints, (*HistorySummary)(nil), nil)
141+
// Empty account_id: resolver returns no UUIDs and no external ids.
142+
mockClient.On("QueryHistory", ctx, []string(nil), []string(nil), mock.Anything, mock.Anything, "hourly").Return(dataPoints, (*HistorySummary)(nil), nil)
111143

112144
mockAuth, req := adminAnalyticsReq(ctx)
113-
handler := &Handler{auth: mockAuth, analyticsClient: mockClient}
145+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
114146

115147
params := map[string]string{} // No interval specified
116148

117149
_, err := handler.getHistoryAnalytics(ctx, req, params)
118150
require.NoError(t, err)
119151

120-
mockClient.AssertCalled(t, "QueryHistory", ctx, "", mock.Anything, mock.Anything, "hourly")
152+
mockClient.AssertCalled(t, "QueryHistory", ctx, []string(nil), []string(nil), mock.Anything, mock.Anything, "hourly")
121153
}
122154

123155
func TestHandler_getHistoryAnalytics_InvalidDateRange(t *testing.T) {
@@ -140,10 +172,10 @@ func TestHandler_getHistoryAnalytics_QueryError(t *testing.T) {
140172
ctx := context.Background()
141173
mockClient := new(MockAnalyticsClient)
142174

143-
mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, errors.New("query failed"))
175+
mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil, errors.New("query failed"))
144176

145177
mockAuth, req := adminAnalyticsReq(ctx)
146-
handler := &Handler{auth: mockAuth, analyticsClient: mockClient}
178+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
147179

148180
params := map[string]string{}
149181

@@ -185,10 +217,10 @@ func TestHandler_getHistoryBreakdown_Success(t *testing.T) {
185217
"lambda": {PurchaseCount: 5, TotalSavings: 100.0},
186218
}
187219

188-
mockClient.On("QueryBreakdown", ctx, "account-123", mock.Anything, mock.Anything, "service").Return(breakdownData, nil)
220+
mockClient.On("QueryBreakdown", ctx, []string(nil), []string{"account-123"}, mock.Anything, mock.Anything, "service").Return(breakdownData, nil)
189221

190222
mockAuth, req := adminAnalyticsReq(ctx)
191-
handler := &Handler{auth: mockAuth, analyticsClient: mockClient}
223+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
192224

193225
params := map[string]string{
194226
"account_id": "account-123",
@@ -220,17 +252,17 @@ func TestHandler_getHistoryBreakdown_DefaultDimension(t *testing.T) {
220252
mockClient := new(MockAnalyticsClient)
221253

222254
breakdownData := map[string]BreakdownValue{}
223-
mockClient.On("QueryBreakdown", ctx, "", mock.Anything, mock.Anything, "service").Return(breakdownData, nil)
255+
mockClient.On("QueryBreakdown", ctx, []string(nil), []string(nil), mock.Anything, mock.Anything, "service").Return(breakdownData, nil)
224256

225257
mockAuth, req := adminAnalyticsReq(ctx)
226-
handler := &Handler{auth: mockAuth, analyticsClient: mockClient}
258+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
227259

228260
params := map[string]string{} // No dimension specified
229261

230262
_, err := handler.getHistoryBreakdown(ctx, req, params)
231263
require.NoError(t, err)
232264

233-
mockClient.AssertCalled(t, "QueryBreakdown", ctx, "", mock.Anything, mock.Anything, "service")
265+
mockClient.AssertCalled(t, "QueryBreakdown", ctx, []string(nil), []string(nil), mock.Anything, mock.Anything, "service")
234266
}
235267

236268
func TestHandler_getHistoryBreakdown_InvalidDateRange(t *testing.T) {
@@ -253,10 +285,10 @@ func TestHandler_getHistoryBreakdown_QueryError(t *testing.T) {
253285
ctx := context.Background()
254286
mockClient := new(MockAnalyticsClient)
255287

256-
mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("breakdown failed"))
288+
mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("breakdown failed"))
257289

258290
mockAuth, req := adminAnalyticsReq(ctx)
259-
handler := &Handler{auth: mockAuth, analyticsClient: mockClient}
291+
handler := &Handler{auth: mockAuth, analyticsClient: mockClient, config: new(MockConfigStore)}
260292

261293
params := map[string]string{}
262294

‎internal/api/handler_coverage_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -599,7 +599,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) {
599599
t.Run("getHistoryAnalyticsHandler", func(t *testing.T) {
600600
mockAuth, req := adminAnalyticsReq(ctx)
601601
mockClient := new(MockAnalyticsClient)
602-
mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil)
602+
mockClient.On("QueryHistory", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return([]HistoryDataPoint{}, (*HistorySummary)(nil), nil)
603603

604604
h := &Handler{auth: mockAuth, analyticsClient: mockClient}
605605
router := NewRouter(h)
@@ -612,7 +612,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) {
612612
t.Run("getHistoryBreakdownHandler", func(t *testing.T) {
613613
mockAuth, req := adminAnalyticsReq(ctx)
614614
mockClient := new(MockAnalyticsClient)
615-
mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string]BreakdownValue{}, nil)
615+
mockClient.On("QueryBreakdown", ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string]BreakdownValue{}, nil)
616616

617617
h := &Handler{auth: mockAuth, analyticsClient: mockClient}
618618
router := NewRouter(h)

0 commit comments

Comments
 (0)