Skip to content

Commit fbf05f9

Browse files
committed
fix(api): warn on payment-option normalization coercion
validatePurchaseRecommendation silently overwrote rec.Payment with the NormalizePaymentOption result, contradicting the doc comment that callers are expected to WARN when a raw payment option is coerced to its canonical form (e.g. azure partial-upfront -> monthly). Log the transition via pkg/logging, matching the pattern already used by scheduler.convertRecommendations, so the coercion is auditable on this money-affecting field. The coercion itself is unchanged.
1 parent d2cd112 commit fbf05f9

2 files changed

Lines changed: 65 additions & 1 deletion

File tree

‎internal/api/handler_purchases_guards_test.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,60 @@ func TestValidatePurchaseRecommendation_ErrorMessage(t *testing.T) {
147147
assert.Contains(t, err.Error(), "monthly")
148148
}
149149

150+
// TestValidatePurchaseRecommendation_NormalizationWarning is a regression test
151+
// for #1504: the doc comment on config.NormalizePaymentOption
152+
// (internal/config/validation.go) states that the caller is expected to WARN
153+
// when a raw payment-option token differs from its normalized canonical form,
154+
// so an operator can audit a money-affecting coercion. validatePurchaseRecommendation
155+
// is that caller; it must emit a WARN log when normalization actually changes
156+
// the value (raw != canonical). This test captures the default logger's
157+
// output via captureDefaultLog (defined in handler_accounts_test.go, package
158+
// api) and asserts the log line is emitted with the provider, service, raw
159+
// and canonical values, and rec index.
160+
//
161+
// Not run with t.Parallel(): captureDefaultLog mutates the shared default
162+
// logger's output, which would race against other SetOutput-using tests if
163+
// parallelized (see TestValidatePlanAccountProviders_GetAccountDBError_NoPIILeak).
164+
func TestValidatePurchaseRecommendation_NormalizationWarning(t *testing.T) {
165+
logBuf := captureDefaultLog(t)
166+
167+
// Azure partial-upfront has no direct Azure equivalent; NormalizePaymentOption
168+
// coerces it to "monthly" (#1503). This is a real raw != canonical
169+
// transition and must be logged.
170+
rec := validRec()
171+
rec.Provider = "azure"
172+
rec.Payment = "partial-upfront"
173+
err := validatePurchaseRecommendation(&rec, 7)
174+
require.NoError(t, err)
175+
// The money-affecting coercion itself must be unchanged by adding the log.
176+
assert.Equal(t, "monthly", rec.Payment, "coercion behavior must stay partial-upfront -> monthly, not upfront")
177+
178+
logged := logBuf.String()
179+
assert.Contains(t, logged, "[WARN]")
180+
assert.Contains(t, logged, "rec 7 (azure/ec2)", "warning must identify the rec index, provider and service for audit")
181+
assert.Contains(t, logged, `raw="partial-upfront"`)
182+
assert.Contains(t, logged, `canonical="monthly"`)
183+
}
184+
185+
// TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical guards that
186+
// an already-canonical payment option (no real normalization) does not emit a
187+
// WARN log: only an actual raw->canonical transition should be observable, so
188+
// operators aren't flooded with noise on the common case.
189+
//
190+
// Not run with t.Parallel(); see TestValidatePurchaseRecommendation_NormalizationWarning.
191+
func TestValidatePurchaseRecommendation_NoWarningWhenAlreadyCanonical(t *testing.T) {
192+
logBuf := captureDefaultLog(t)
193+
194+
rec := validRec()
195+
rec.Provider = "azure"
196+
rec.Payment = "monthly" // already canonical for azure; no normalization occurs
197+
err := validatePurchaseRecommendation(&rec, 0)
198+
require.NoError(t, err)
199+
assert.Equal(t, "monthly", rec.Payment)
200+
201+
assert.Empty(t, logBuf.String(), "no normalization occurred, so nothing should be logged")
202+
}
203+
150204
// The per-rec #643 validation is wired into the web execute boundary
151205
// (validateExecutePurchaseRequest), NOT the shared validateAndTotalRecommendations
152206
// which the retry path also calls with replayed recs. This test pins that

‎internal/api/validation.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/aws/aws-lambda-go/events"
1515

1616
"github.com/LeanerCloud/CUDly/internal/config"
17+
"github.com/LeanerCloud/CUDly/pkg/logging"
1718
)
1819

1920
// Security constants.
@@ -564,8 +565,17 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e
564565
payment := strings.ToLower(strings.TrimSpace(rec.Payment))
565566
// Coerce any legacy/cross-provider alias before the whitelist check so
566567
// that callers using old AWS-style tokens are transparently redirected to
567-
// the canonical token for the target provider.
568+
// the canonical token for the target provider. WARN when a real
569+
// normalization occurs (raw != canonical) so an operator can audit the
570+
// coercion of this money-affecting field, matching the WARN contract
571+
// documented on config.NormalizePaymentOption and mirroring the same
572+
// coerced/uncoerced logging convertRecommendations does at the
573+
// scheduler's emission boundary (internal/scheduler/scheduler.go).
568574
if normalized, ok := config.NormalizePaymentOption(provider, payment); ok {
575+
if normalized != payment {
576+
logging.Warnf("validatePurchaseRecommendation: rec %d (%s/%s) payment option normalized: raw=%q canonical=%q",
577+
idx, provider, rec.Service, payment, normalized)
578+
}
569579
payment = normalized
570580
}
571581
if !payments[payment] {

0 commit comments

Comments
 (0)