From 2031d2e5d51ed4fb024a73dcb1f6dbd47a430de0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 29 Sep 2026 00:02:38 +0200 Subject: [PATCH] fix(cli): parse CSV Count and EstimatedSavings strictly fmt.Sscanf stopped at the first unconsumable character and reported success, so a Count of "3.7" loaded as 3, "12 units" as 12, "-5" as -5, and an EstimatedSavings of "1000 USD" as 1000. These values feed savingsPerInstance, ApplyInstanceLimit and the purchase loop. Parse the trimmed cell with strconv.Atoi / strconv.ParseFloat, reject a negative, blank or missing Count and non-finite savings, and name the CSV line and column in the error. A blank EstimatedSavings still loads as zero, which requireRankingSignal depends on. Closes #1944 --- cmd/multi_service_csv.go | 51 ++++++++++----- cmd/multi_service_csv_strict_test.go | 95 ++++++++++++++++++++++++++++ cmd/multi_service_csv_test.go | 4 +- 3 files changed, 132 insertions(+), 18 deletions(-) create mode 100644 cmd/multi_service_csv_strict_test.go diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 297b87188..553747d23 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -6,8 +6,11 @@ import ( "fmt" "io" "log" + "math" "os" "sort" + "strconv" + "strings" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -89,7 +92,8 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm rec, err := parseCSVRecord(record, colIdx) if err != nil { - return nil, err + line, _ := reader.FieldPos(0) + return nil, fmt.Errorf("CSV line %d: %w", line, err) } recs = append(recs, rec) @@ -111,12 +115,10 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati rec.Term = getCSVField(record, colIdx, "Term") rec.PaymentOption = getCSVField(record, colIdx, "PaymentOption") - // Parse integer fields - if err := parseCSVInt(record, colIdx, "Count", &rec.Count); err != nil { + if err := parseCSVCount(record, colIdx, &rec.Count); err != nil { return rec, err } - // Parse float fields if err := parseCSVFloat(record, colIdx, "EstimatedSavings", &rec.EstimatedSavings); err != nil { return rec, err } @@ -163,29 +165,46 @@ func getCSVField(record []string, colIdx map[string]int, fieldName string) strin return "" } -// parseCSVInt parses an integer field from a CSV record. -func parseCSVInt(record []string, colIdx map[string]int, fieldName string, target *int) error { - value := getCSVField(record, colIdx, fieldName) +// parseCSVCount parses the required Count field as a whole non-negative +// integer. It drives purchase quantities, so a blank, missing, fractional or +// otherwise malformed cell is an error rather than a truncated or zero value. +func parseCSVCount(record []string, colIdx map[string]int, target *int) error { + const fieldName = "Count" + idx, ok := colIdx[fieldName] + if !ok { + return fmt.Errorf("missing required %s column", fieldName) + } + value := strings.TrimSpace(getCSVField(record, colIdx, fieldName)) if value == "" { - return nil + return fmt.Errorf("column %d %q: value is required", idx+1, fieldName) } - - if _, err := fmt.Sscanf(value, "%d", target); err != nil { - return fmt.Errorf("invalid %s value '%s': %w", fieldName, value, err) + n, err := strconv.Atoi(value) + if err != nil { + return fmt.Errorf("column %d %q: invalid integer %q: %w", idx+1, fieldName, value, err) } + if n < 0 { + return fmt.Errorf("column %d %q: must not be negative, got %d", idx+1, fieldName, n) + } + *target = n return nil } -// parseCSVFloat parses a float field from a CSV record. +// parseCSVFloat parses a finite float field from a CSV record. A blank or +// absent cell leaves target untouched (see requireRankingSignal). func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, target *float64) error { - value := getCSVField(record, colIdx, fieldName) + value := strings.TrimSpace(getCSVField(record, colIdx, fieldName)) if value == "" { return nil } - - if _, err := fmt.Sscanf(value, "%f", target); err != nil { - return fmt.Errorf("invalid %s value '%s': %w", fieldName, value, err) + col := colIdx[fieldName] + 1 + f, err := strconv.ParseFloat(value, 64) + if err != nil { + return fmt.Errorf("column %d %q: invalid number %q: %w", col, fieldName, value, err) + } + if math.IsNaN(f) || math.IsInf(f, 0) { + return fmt.Errorf("column %d %q: invalid number %q: must be finite", col, fieldName, value) } + *target = f return nil } diff --git a/cmd/multi_service_csv_strict_test.go b/cmd/multi_service_csv_strict_test.go new file mode 100644 index 000000000..f629cac1c --- /dev/null +++ b/cmd/multi_service_csv_strict_test.go @@ -0,0 +1,95 @@ +package main + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func loadCSVContent(t *testing.T, content string) error { + t.Helper() + csvPath := filepath.Join(t.TempDir(), "recs.csv") + require.NoError(t, os.WriteFile(csvPath, []byte(content), 0o600)) + _, err := loadRecommendationsFromCSV(csvPath) + return err +} + +// Count cells must parse whole or not at all (#1944): fmt.Sscanf truncated +// "3.7" to 3 and accepted trailing garbage, both feeding the purchase loop. +func TestLoadRecommendationsFromCSV_StrictCount(t *testing.T) { + tests := []struct { + name string + cell string + }{ + {"fractional", "3.7"}, + {"trailing garbage", "3abc"}, + {"trailing unit", "12 units"}, + {"negative", "-1"}, + {"blank", ""}, + {"whitespace only", " "}, + {"overflows int64", "99999999999999999999"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + content := "Service,Region,ResourceType,Count\n" + + "rds,us-east-1,db.t3.micro,1\n" + + "rds,us-east-1,db.t3.small,\"" + tt.cell + "\"\n" + err := loadCSVContent(t, content) + require.Error(t, err) + assert.Contains(t, err.Error(), "line 3") + assert.Contains(t, err.Error(), `column 4 "Count"`) + }) + } +} + +func TestLoadRecommendationsFromCSV_CountTrimmed(t *testing.T) { + csvPath := filepath.Join(t.TempDir(), "recs.csv") + require.NoError(t, os.WriteFile(csvPath, []byte("Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,\" 3 \"\n"), 0o600)) + recs, err := loadRecommendationsFromCSV(csvPath) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, 3, recs[0].Count) +} + +func TestLoadRecommendationsFromCSV_CountColumnRequired(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType\nrds,us-east-1,db.t3.micro\n") + require.Error(t, err) + assert.Contains(t, err.Error(), "Count") +} + +func TestLoadRecommendationsFromCSV_StrictEstimatedSavings(t *testing.T) { + tests := []struct { + name string + cell string + }{ + {"trailing currency", "1000 USD"}, + {"trailing garbage", "12.5abc"}, + {"NaN", "NaN"}, + {"infinity", "Inf"}, + {"overflows float64", "1e400"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + content := "Service,Region,ResourceType,Count,EstimatedSavings\n" + + "rds,us-east-1,db.t3.micro,2,\"" + tt.cell + "\"\n" + err := loadCSVContent(t, content) + require.Error(t, err) + assert.Contains(t, err.Error(), "line 2") + assert.Contains(t, err.Error(), `column 5 "EstimatedSavings"`) + }) + } +} + +// A blank EstimatedSavings cell stays absent-as-zero: requireRankingSignal +// relies on it to refuse a binding --max-instances cap. +func TestLoadRecommendationsFromCSV_BlankSavingsStillZero(t *testing.T) { + csvPath := filepath.Join(t.TempDir(), "recs.csv") + require.NoError(t, os.WriteFile(csvPath, []byte("Service,Region,ResourceType,Count,EstimatedSavings\nrds,us-east-1,db.t3.micro,2, \n"), 0o600)) + recs, err := loadRecommendationsFromCSV(csvPath) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Zero(t, recs[0].EstimatedSavings) +} diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index c62f20a6c..fdbfcc5da 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -721,14 +721,14 @@ TOTAL,,,,,3,,`, csvContent: `Service,Region,ResourceType,Count rds,us-east-1,db.t3.micro,abc`, wantErr: true, - errContains: "invalid Count value", + errContains: `column 4 "Count": invalid integer`, }, { name: "Invalid EstimatedSavings value - non-numeric", csvContent: `Service,Region,ResourceType,Count,EstimatedSavings rds,us-east-1,db.t3.micro,5,invalid`, wantErr: true, - errContains: "invalid EstimatedSavings value", + errContains: `column 5 "EstimatedSavings": invalid number`, }, { name: "Multiple rows with various services",