Skip to content

Commit 26f4372

Browse files
committed
fix(config): make SQL active-commitment expiry boundary inclusive
GetActivePurchaseHistory excluded a commitment expiring exactly at asOf (`> $1`) while the API layer's isActiveCommitment treats that instant as still active (!now.After(expiry)). Align the SQL predicate to `>= $1` so both sides share one boundary definition, and document the inclusive contract in the store and interface docstrings. The pgxmock tests pin the new predicate. Addresses CodeRabbit review on PR #1221 (store_postgres.go).
1 parent 2d8eb35 commit 26f4372

3 files changed

Lines changed: 12 additions & 6 deletions

File tree

‎internal/config/interfaces.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,8 @@ type StoreInterface interface {
122122
GetPurchaseHistory(ctx context.Context, accountID string, limit int) ([]PurchaseHistoryRecord, error)
123123
GetAllPurchaseHistory(ctx context.Context, limit int) ([]PurchaseHistoryRecord, error)
124124
// GetActivePurchaseHistory returns every purchase_history row whose commitment
125-
// is still within its term at asOf (term > 0 AND timestamp + term years > asOf),
125+
// is still within its term at asOf (term > 0 AND timestamp + term years >= asOf;
126+
// the expiry boundary is inclusive, matching the API layer's isActiveCommitment),
126127
// newest-first, optionally scoped to a set of accounts. The account scope uses
127128
// the same dual-column predicate as GetPurchaseHistoryFiltered: accountIDs
128129
// match cloud_account_id (cloud_accounts UUIDs) and externalIDsByProvider

‎internal/config/store_postgres.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1626,11 +1626,14 @@ func (s *PostgresStore) GetAllPurchaseHistory(ctx context.Context, limit int) ([
16261626
// it cannot silently truncate older-but-still-active 1y/3y commitments the way
16271627
// a newest-first LIMIT page does (issue #1140). term*8760 hours matches the
16281628
// collector's HoursPerYear and the API layer's commitmentExpiry (both 365*24)
1629-
// so the SQL and Go term windows agree.
1629+
// so the SQL and Go term windows agree. The expiry comparison is inclusive
1630+
// (expiry >= asOf): a commitment expiring exactly at asOf is still active,
1631+
// matching the API layer's isActiveCommitment (!now.After(expiry)) so the SQL
1632+
// result set and the Go-side active checks share one boundary definition.
16301633
func (s *PostgresStore) GetActivePurchaseHistory(ctx context.Context, asOf time.Time, accountIDs []string, externalIDsByProvider map[string][]string) ([]PurchaseHistoryRecord, error) {
16311634
conds := []string{
16321635
"term > 0",
1633-
"timestamp + make_interval(hours => term * 8760) > $1",
1636+
"timestamp + make_interval(hours => term * 8760) >= $1",
16341637
}
16351638
args := []any{asOf}
16361639
conds, args = appendAccountPredicate(conds, args, accountIDs, externalIDsByProvider)

‎internal/config/store_postgres_pgxmock_test.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -786,7 +786,9 @@ func TestPGXMock_GetPurchaseHistoryFiltered_NoFilters(t *testing.T) {
786786
// the active filter into the WHERE clause with NO LIMIT (the `$` anchor after
787787
// ORDER BY proves no trailing LIMIT clause), so the result is bounded by the
788788
// number of live commitments and a newest-first row cap can never silently
789-
// drop the oldest still-active 1y/3y commitments (issue #1140). It also pins
789+
// drop the oldest still-active 1y/3y commitments (issue #1140). The pinned
790+
// `>= $1` expiry comparison is inclusive so a commitment expiring exactly at
791+
// asOf stays active, matching the API layer's isActiveCommitment. It also pins
790792
// the full 21-column SELECT including the issue-#290 revocation columns:
791793
// before this fix the query selected only 17 columns while
792794
// queryPurchaseHistory scans 21 destinations, so every call failed at Scan.
@@ -798,7 +800,7 @@ func TestPGXMock_GetActivePurchaseHistory_Unscoped(t *testing.T) {
798800
now := time.Now().Truncate(time.Second)
799801
rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "acct-1")...)
800802
mock.ExpectQuery(
801-
`SELECT account_id, purchase_id, .*revocation_window_closes_at, revoked_at, revoked_via, support_case_id FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) > \$1 ORDER BY timestamp DESC$`,
803+
`SELECT account_id, purchase_id, .*revocation_window_closes_at, revoked_at, revoked_via, support_case_id FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 ORDER BY timestamp DESC$`,
802804
).WithArgs(now).WillReturnRows(rows)
803805

804806
records, err := store.GetActivePurchaseHistory(ctx, now, nil, nil)
@@ -820,7 +822,7 @@ func TestPGXMock_GetActivePurchaseHistory_AccountScoped(t *testing.T) {
820822
now := time.Now().Truncate(time.Second)
821823
rows := pgxmock.NewRows(purchaseHistoryCols).AddRow(purchaseHistoryRow(now, "aws", "111122223333")...)
822824
mock.ExpectQuery(
823-
`FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) > \$1 AND \(cloud_account_id = ANY\(\$2\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\) ORDER BY timestamp DESC$`,
825+
`FROM purchase_history WHERE term > 0 AND timestamp \+ make_interval\(hours => term \* 8760\) >= \$1 AND \(cloud_account_id = ANY\(\$2\) OR \(provider = \$3 AND account_id = ANY\(\$4\)\)\) ORDER BY timestamp DESC$`,
824826
).WithArgs(now, []string{"acct-uuid-1"}, "aws", []string{"111122223333"}).WillReturnRows(rows)
825827

826828
records, err := store.GetActivePurchaseHistory(ctx, now,

0 commit comments

Comments
 (0)