diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 553747d23..39eb4dbd0 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "time" + "unicode/utf8" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" @@ -47,9 +48,14 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) if err != nil { return nil, fmt.Errorf("failed to read CSV header: %w", err) } + if err = validateCSVUTF8(header); err != nil { + return nil, fmt.Errorf("CSV header: %w", err) + } - // Build column index map - colIdx := buildColumnIndexMap(header) + colIdx, err := buildColumnIndexMap(header) + if err != nil { + return nil, err + } // Parse all records parsed, err := parseCSVRecords(reader, colIdx) @@ -60,13 +66,36 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) return parsed, nil } -// buildColumnIndexMap creates a map from column names to indices. -func buildColumnIndexMap(header []string) map[string]int { +// Minimal CSVs require these columns; service-specific columns stay optional. +var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count"} + +func validateCSVUTF8(fields []string) error { + for i, field := range fields { + if !utf8.ValidString(field) { + return fmt.Errorf("column %d: invalid UTF-8 encoding", i+1) + } + } + return nil +} + +func buildColumnIndexMap(header []string) (map[string]int, error) { + if len(header) > 0 { + header[0] = strings.TrimPrefix(header[0], "\ufeff") + } colIdx := make(map[string]int) for i, col := range header { colIdx[col] = i } - return colIdx + var missing []string + for _, req := range requiredCSVColumns { + if _, ok := colIdx[req]; !ok { + missing = append(missing, req) + } + } + if len(missing) > 0 { + return nil, fmt.Errorf("CSV header missing required columns: %s", strings.Join(missing, ", ")) + } + return colIdx, nil } // parseCSVRecords reads and parses all CSV records. @@ -81,6 +110,10 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm if err != nil { return nil, fmt.Errorf("failed to read CSV record: %w", err) } + if err = validateCSVUTF8(record); err != nil { + line, _ := reader.FieldPos(0) + return nil, fmt.Errorf("CSV line %d: %w", line, err) + } // Skip the trailing TOTAL summary row that writeMultiServiceCSVReport // emits (label in the Service column). Without this, feeding the tool @@ -165,9 +198,7 @@ func getCSVField(record []string, colIdx map[string]int, fieldName string) strin return "" } -// 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. +// Savings Plans exports use Count=1, preserving positive-count round trips. func parseCSVCount(record []string, colIdx map[string]int, target *int) error { const fieldName = "Count" idx, ok := colIdx[fieldName] @@ -180,17 +211,16 @@ func parseCSVCount(record []string, colIdx map[string]int, target *int) error { } n, err := strconv.Atoi(value) if err != nil { - return fmt.Errorf("column %d %q: invalid integer %q: %w", idx+1, fieldName, value, err) + return fmt.Errorf("column %d %q: invalid integer: %w", idx+1, fieldName, err) } - if n < 0 { - return fmt.Errorf("column %d %q: must not be negative, got %d", idx+1, fieldName, n) + if n < 1 { + return fmt.Errorf("column %d %q: must be at least 1, got %d", idx+1, fieldName, n) } *target = n return nil } -// parseCSVFloat parses a finite float field from a CSV record. A blank or -// absent cell leaves target untouched (see requireRankingSignal). +// Blank savings stay absent-as-zero for requireRankingSignal. func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, target *float64) error { value := strings.TrimSpace(getCSVField(record, colIdx, fieldName)) if value == "" { @@ -199,11 +229,14 @@ func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, tar 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) + return fmt.Errorf("column %d %q: invalid number: %w", col, fieldName, err) } if math.IsNaN(f) || math.IsInf(f, 0) { return fmt.Errorf("column %d %q: invalid number %q: must be finite", col, fieldName, value) } + if f < 0 { + return fmt.Errorf("column %d %q: must not be negative, got %g", col, fieldName, f) + } *target = f return nil } diff --git a/cmd/multi_service_csv_header_test.go b/cmd/multi_service_csv_header_test.go new file mode 100644 index 000000000..225b36a68 --- /dev/null +++ b/cmd/multi_service_csv_header_test.go @@ -0,0 +1,148 @@ +package main + +import ( + "encoding/binary" + "testing" + "unicode/utf16" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadRecommendationsFromCSV_HeaderValidation_1327(t *testing.T) { + tests := []struct { + name string + header string + row string + errContains string + }{ + {"missing Service", "Region,ResourceType,Count\n", "us-east-1,db.t3.micro,2\n", "Service"}, + {"missing Region", "Service,ResourceType,Count\n", "rds,db.t3.micro,2\n", "Region"}, + {"missing ResourceType", "Service,Region,Count\n", "rds,us-east-1,2\n", "ResourceType"}, + {"missing Count", "Service,Region,ResourceType\n", "rds,us-east-1,db.t3.micro\n", "Count"}, + {"multiple missing", "Service,Count\n", "rds,2\n", "Region, ResourceType"}, + { + "legacy TEST-02 headers rejected", + "Service,Region,Instance Type,Instance Count\n", + "rds,us-east-1,db.t3.micro,2\n", + "ResourceType, Count", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := loadCSVContent(t, tt.header+tt.row) + require.Error(t, err) + assert.Contains(t, err.Error(), "CSV header missing required columns") + assert.Contains(t, err.Error(), tt.errContains) + }) + } +} + +func TestLoadRecommendationsFromCSV_BOMPrefixed_1327(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "\ufeffService,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, common.ServiceRDS, recs[0].Service) + assert.Equal(t, "us-east-1", recs[0].Region) + assert.Equal(t, 2, recs[0].Count) +} + +func TestLoadRecommendationsFromCSV_Encoding_1327(t *testing.T) { + t.Run("UTF-16", func(t *testing.T) { + units := utf16.Encode([]rune("Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\n")) + for _, tt := range []struct { + name string + bom []byte + order binary.ByteOrder + }{ + {"little endian", []byte{0xff, 0xfe}, binary.LittleEndian}, + {"big endian", []byte{0xfe, 0xff}, binary.BigEndian}, + } { + t.Run(tt.name, func(t *testing.T) { + encoded := make([]byte, 2+2*len(units)) + copy(encoded, tt.bom) + for i, unit := range units { + tt.order.PutUint16(encoded[2+2*i:], unit) + } + err := loadCSVContent(t, string(encoded)) + require.Error(t, err) + assert.EqualError(t, err, "CSV header: column 1: invalid UTF-8 encoding") + }) + } + }) + + t.Run("Latin-1 optional header", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,Caf\xe9\n"+ + "rds,us-east-1,db.t3.micro,2,prod\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV header: column 5: invalid UTF-8 encoding") + }) + + t.Run("Latin-1 AccountName", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,AccountName\n"+ + "rds,us-east-1,db.t3.micro,2,Caf\xe9\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV line 2: column 5: invalid UTF-8 encoding") + }) + + t.Run("invalid ignored field in TOTAL after multiline record", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,Ignored\n"+ + "rds,us-east-1,db.t3.micro,2,\"prod\naccount\"\n"+ + "TOTAL,,,2,\xff\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV line 4: column 5: invalid UTF-8 encoding") + }) + + t.Run("valid non-ASCII UTF-8", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,AccountName\n"+ + "rds,us-east-1,db.t3.micro,2,Café 東京\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "Café 東京", recs[0].AccountName) + }) +} + +// Lock in encoding/csv behaviors the purchase path relies on. +func TestLoadRecommendationsFromCSV_EdgeCases_1327(t *testing.T) { + t.Run("CRLF line endings", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count\r\nrds,us-east-1,db.t3.micro,2\r\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, 2, recs[0].Count) + }) + + t.Run("quoted commas and newlines inside fields", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,AccountName\n"+ + "rds,us-east-1,\"db.t3.micro, burstable\",2,\"prod\naccount\"\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "db.t3.micro, burstable", recs[0].ResourceType) + assert.Equal(t, "prod\naccount", recs[0].AccountName) + }) + + t.Run("headers only loads zero recs", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count\n")) + require.NoError(t, err) + assert.Empty(t, recs) + }) + + t.Run("wrong field count on a data row", func(t *testing.T) { + err := loadCSVContent(t, + "Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro\n") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to read CSV record") + }) + + t.Run("TOTAL row still skipped with full header", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,2\nTOTAL,,,2\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + }) +} diff --git a/cmd/multi_service_csv_strict_test.go b/cmd/multi_service_csv_strict_test.go index ac0ed3fba..b965ee728 100644 --- a/cmd/multi_service_csv_strict_test.go +++ b/cmd/multi_service_csv_strict_test.go @@ -26,6 +26,7 @@ func TestLoadRecommendationsFromCSV_StrictCount(t *testing.T) { {"trailing garbage", "3abc"}, {"trailing unit", "12 units"}, {"negative", "-1"}, + {"zero", "0"}, {"blank", ""}, {"whitespace only", " "}, {"overflows int64", "99999999999999999999"}, @@ -62,6 +63,7 @@ func TestLoadRecommendationsFromCSV_StrictEstimatedSavings(t *testing.T) { name string cell string }{ + {"negative", "-100"}, {"trailing currency", "1000 USD"}, {"trailing garbage", "12.5abc"}, {"NaN", "NaN"}, diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index fdbfcc5da..32222b0d1 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -767,13 +767,13 @@ rds,us-east-1,db.t3.micro,5,1234.5678`, }, }, { - name: "CSV with zero values", + name: "CSV with zero EstimatedSavings stays valid", csvContent: `Service,Region,ResourceType,Count,EstimatedSavings -rds,us-east-1,db.t3.micro,0,0`, +rds,us-east-1,db.t3.micro,5,0`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) - assert.Equal(t, 0, recs[0].Count) + assert.Equal(t, 5, recs[0].Count) assert.Equal(t, float64(0), recs[0].EstimatedSavings) }, },