diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index afbc0084d..f31da946a 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -199,8 +199,8 @@ func TestApplyCountOverride(t *testing.T) { tests := []struct { name string recs []common.Recommendation - overrideCount int32 expectedCounts []int + overrideCount int32 }{ { name: "Override with positive value", @@ -252,8 +252,8 @@ func TestApplyCoverage(t *testing.T) { tests := []struct { name string recs []common.Recommendation - coverage float64 expectedCounts []int + coverage float64 expectedLen int }{ { @@ -383,8 +383,8 @@ func TestAdjustRecommendationsForExisting(t *testing.T) { name string inputRecs []common.Recommendation existingRIs []common.Commitment - expectedLen int expectedCounts []int + expectedLen int }{ { name: "No existing RIs - all recommendations kept", @@ -490,8 +490,8 @@ func TestAdjustRecommendationsForExisting(t *testing.T) { func TestGetRecommendationDescription(t *testing.T) { tests := []struct { name string - rec common.Recommendation expected string + rec common.Recommendation }{ { name: "RDS recommendation with database details", @@ -570,8 +570,8 @@ func TestNormalizeEngineName(t *testing.T) { func TestGetEngineFromRecommendation(t *testing.T) { tests := []struct { name string - rec common.Recommendation expected string + rec common.Recommendation }{ { name: "DatabaseDetails value type", @@ -627,7 +627,7 @@ func TestGetEngineFromRecommendation(t *testing.T) { // confirmPurchaseWithInput is a testable variant of ConfirmPurchase that reads // from the provided reader rather than os.Stdin, allowing stdin to be mocked in tests. -func confirmPurchaseWithInput(totalInstances int, totalCost float64, skipConfirmation bool, input string) bool { +func confirmPurchaseWithInput(skipConfirmation bool, input string) bool { if skipConfirmation { return true } @@ -693,7 +693,7 @@ func TestConfirmPurchaseInput(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := confirmPurchaseWithInput(1, 10.0, false, tt.input) + result := confirmPurchaseWithInput(false, tt.input) assert.Equal(t, tt.expected, result) }) } @@ -706,8 +706,8 @@ func TestAdjustRecommendationsForExistingRIsEdgeCases(t *testing.T) { name string inputRecs []common.Recommendation existingRIs []common.Commitment - expectedLen int expectedCounts []int + expectedLen int }{ { name: "Multiple RIs same instance type different regions", @@ -771,9 +771,9 @@ func TestApplyInstanceLimit(t *testing.T) { tests := []struct { name string recs []common.Recommendation - maxInstances int32 - expectedLen int expectedCounts []int + expectedLen int + maxInstances int32 }{ { name: "No limit - all recommendations kept", diff --git a/cmd/main_test.go b/cmd/main_test.go index 79fdbd7b5..b9f1979b1 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -98,12 +98,12 @@ func TestGetAllServices(t *testing.T) { func TestGeneratePurchaseID(t *testing.T) { tests := []struct { name string - rec common.Recommendation region string + expectedPrefix string + rec common.Recommendation index int - isDryRun bool coverage float64 - expectedPrefix string + isDryRun bool }{ { name: "RDS Recommendation - dry run", @@ -401,11 +401,11 @@ func TestGeneratePurchaseIDComprehensive(t *testing.T) { tests := []struct { name string - rec common.Recommendation region string - isDryRun bool + rec common.Recommendation expectedContains []string expectedNotContains []string + isDryRun bool }{ { name: "RDS with account name and engine", @@ -617,15 +617,15 @@ func TestGeneratePurchaseIDCoverageVariations(t *testing.T) { tests := []struct { name string - coverage float64 expectedCoverage string + coverage float64 }{ - {"Coverage 0%", 0.0, "0pct"}, - {"Coverage 50%", 50.0, "50pct"}, - {"Coverage 75.5%", 75.5, "76pct"}, // Rounds to nearest integer - {"Coverage 99%", 99.0, "99pct"}, - {"Coverage 100%", 100.0, "100pct"}, - {"Coverage 33.3%", 33.3, "33pct"}, + {"Coverage 0%", "0pct", 0.0}, + {"Coverage 50%", "50pct", 50.0}, + {"Coverage 75.5%", "76pct", 75.5}, // Rounds to nearest integer + {"Coverage 99%", "99pct", 99.0}, + {"Coverage 100%", "100pct", 100.0}, + {"Coverage 33.3%", "33pct", 33.3}, } for _, tt := range tests { @@ -685,12 +685,12 @@ func TestFilterFlagValidation(t *testing.T) { tests := []struct { name string + errorContains string includeRegions []string excludeRegions []string includeInstanceTypes []string excludeInstanceTypes []string expectError bool - errorContains string }{ { name: "No conflicts", @@ -770,9 +770,9 @@ func TestCreateServiceClientAllServices(t *testing.T) { func TestValidateFlags(t *testing.T) { tests := []struct { name string + setPayment string setCoverage float64 setTerm int - setPayment string expectError bool }{ { @@ -846,21 +846,21 @@ func TestValidateFlagsExtended(t *testing.T) { }() tests := []struct { - name string - setCoverage float64 - setTerm int - setPayment string - setMaxInstances int32 - setCSVOutput string setCSVInput string + setCSVOutput string + errorContains string + setPayment string + name string setIncludeEngines []string - setExcludeEngines []string - setIncludeAccounts []string setExcludeAccounts []string + setIncludeAccounts []string + setExcludeEngines []string setIncludeTypes []string setExcludeTypes []string + setCoverage float64 + setTerm int + setMaxInstances int32 expectError bool - errorContains string }{ // Coverage boundary tests { @@ -1104,8 +1104,8 @@ func TestSanitizeAccountName(t *testing.T) { func TestGeneratePurchaseID_EdgeCases(t *testing.T) { tests := []struct { name string - rec common.Recommendation region string + rec common.Recommendation index int isDryRun bool }{ @@ -1179,9 +1179,9 @@ func TestGeneratePurchaseID_EdgeCases(t *testing.T) { func TestValidateInstanceTypes(t *testing.T) { tests := []struct { name string + errorContains string instanceTypes []string expectError bool - errorContains string }{ { name: "Empty slice is valid", diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 7a3252c16..ae3bab8f8 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -394,7 +394,7 @@ func filterAndAdjustRecommendations(recommendations []common.Recommendation, csv // Apply filters (empty currentRegion since we're processing from CSV, not iterating regions) originalCount := len(recommendations) - recommendations = applyFilters(recommendations, cfg, instanceVersions, versionInfo, "") + recommendations = applyFilters(recommendations, &cfg, instanceVersions, versionInfo, "") if len(recommendations) < originalCount { AppLogger.Printf("🔍 After filters: %d recommendations (filtered out %d)\n", len(recommendations), originalCount-len(recommendations)) } diff --git a/cmd/multi_service_engine_versions_test.go b/cmd/multi_service_engine_versions_test.go index ff80820eb..d4f45fcfa 100644 --- a/cmd/multi_service_engine_versions_test.go +++ b/cmd/multi_service_engine_versions_test.go @@ -11,10 +11,10 @@ import ( func TestAdjustRecommendationForExcludedVersions(t *testing.T) { tests := []struct { - name string - recommendation common.Recommendation versionInfo map[string]MajorEngineVersionInfo instanceVersions map[string][]InstanceEngineVersion + name string + recommendation common.Recommendation expectedCount int expectedAdjusted bool }{ @@ -313,7 +313,7 @@ func TestExtractMajorVersion_Additional(t *testing.T) { } } -// Comprehensive tests for extractMajorVersion function +// Comprehensive tests for extractMajorVersion function. func TestExtractMajorVersion_Comprehensive(t *testing.T) { tests := []struct { name string @@ -502,10 +502,10 @@ func TestIsInExtendedSupport(t *testing.T) { futureDate := now.AddDate(3, 0, 0) tests := []struct { + versionInfo map[string]MajorEngineVersionInfo name string engine string version string - versionInfo map[string]MajorEngineVersionInfo expected bool }{ { diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index da3ffdadc..88320917c 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -7,13 +7,13 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) -// applyFilters applies region, instance type, engine, and engine version filters to recommendations -// currentRegion is the region being processed in the current loop iteration - if non-empty, only recommendations for that region are included -func applyFilters(recs []common.Recommendation, cfg Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) []common.Recommendation { +// applyFilters applies region, instance type, engine, and engine version filters to recommendations. +// currentRegion is the region being processed in the current loop iteration - if non-empty, only recommendations for that region are included. +func applyFilters(recs []common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) []common.Recommendation { var filtered []common.Recommendation - for _, rec := range recs { - adjusted, include := processRecommendation(rec, cfg, instanceVersions, versionInfo, currentRegion) + for i := range recs { + adjusted, include := processRecommendation(&recs[i], cfg, instanceVersions, versionInfo, currentRegion) if include { filtered = append(filtered, adjusted) } @@ -26,28 +26,29 @@ func applyFilters(recs []common.Recommendation, cfg Config, instanceVersions map // the adjusted recommendation and whether to include it. The flat // boolean-filter checks are delegated to passesDimensionFilters to keep // this function under gocyclo's complexity threshold. -func processRecommendation(rec common.Recommendation, cfg Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool) { +func processRecommendation(rec *common.Recommendation, cfg *Config, instanceVersions map[string][]InstanceEngineVersion, versionInfo map[string]MajorEngineVersionInfo, currentRegion string) (common.Recommendation, bool) { // Filter to only recommendations for the current region being processed // This prevents duplicating recommendations across all regions // Skip this filter for Savings Plans as they are account-level, not regional if currentRegion != "" && rec.Region != currentRegion && !common.IsSavingsPlan(rec.Service) { - return rec, false + return *rec, false } if !passesDimensionFilters(rec, cfg) { - return rec, false + return *rec, false } // Apply engine version filters - adjust instance count by subtracting extended support versions if !cfg.IncludeExtendedSupport { - rec = adjustRecommendationForExcludedVersions(rec, instanceVersions, versionInfo) + adjusted := adjustRecommendationForExcludedVersions(*rec, instanceVersions, versionInfo) // Skip if all instances were excluded (count reduced to 0) - if rec.Count <= 0 { - return rec, false + if adjusted.Count <= 0 { + return adjusted, false } + return adjusted, true } - return rec, true + return *rec, true } // passesDimensionFilters runs the stateless include/exclude checks on @@ -56,7 +57,7 @@ func processRecommendation(rec common.Recommendation, cfg Config, instanceVersio // each function's cyclomatic complexity under the gocyclo limit; the // dimension filters here are pure functions of rec + cfg with no side // effects. -func passesDimensionFilters(rec common.Recommendation, cfg Config) bool { +func passesDimensionFilters(rec *common.Recommendation, cfg *Config) bool { if !shouldIncludeRegion(rec.Region, cfg) { return false } @@ -78,15 +79,15 @@ func passesDimensionFilters(rec common.Recommendation, cfg Config) bool { // shouldIncludePoolSize filters out RI recommendations for pools whose // AverageInstancesUsedPerHour is below cfg.MinPoolSize. The purpose is to // drop tiny pools where integer-arithmetic sizing forces 100% coverage -// regardless of --target-coverage (e.g. avg=1 with target=80% → floor(0.8)=0 +// regardless of --target-coverage (e.g. avg=1 with target=80% -> floor(0.8)=0 // drops, ceil(0.8)=1 over-covers). Setting --min-pool-size=2 keeps pools // where target can be meaningfully approximated. // // Pass-through cases: filter disabled (MinPoolSize<=0), or rec has no -// per-hour signal (avg<=0 — SPs and recs CE didn't return usage for). +// per-hour signal (avg<=0 - SPs and recs CE didn't return usage for). // Those pools aren't sized via the per-hour formula so the filter doesn't // apply to them. -func shouldIncludePoolSize(rec common.Recommendation, cfg Config) bool { +func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { if cfg.MinPoolSize <= 0 { return true } @@ -96,8 +97,8 @@ func shouldIncludePoolSize(rec common.Recommendation, cfg Config) bool { return rec.AverageInstancesUsedPerHour >= cfg.MinPoolSize } -// shouldIncludeRegion checks if a region should be included based on filters -func shouldIncludeRegion(region string, cfg Config) bool { +// shouldIncludeRegion checks if a region should be included based on filters. +func shouldIncludeRegion(region string, cfg *Config) bool { // If include list is specified, region must be in it if len(cfg.IncludeRegions) > 0 && !slices.Contains(cfg.IncludeRegions, region) { return false @@ -111,8 +112,8 @@ func shouldIncludeRegion(region string, cfg Config) bool { return true } -// shouldIncludeInstanceType checks if an instance type should be included based on filters -func shouldIncludeInstanceType(instanceType string, cfg Config) bool { +// shouldIncludeInstanceType checks if an instance type should be included based on filters. +func shouldIncludeInstanceType(instanceType string, cfg *Config) bool { // If include list is specified, instance type must be in it if len(cfg.IncludeInstanceTypes) > 0 && !slices.Contains(cfg.IncludeInstanceTypes, instanceType) { return false @@ -126,10 +127,10 @@ func shouldIncludeInstanceType(instanceType string, cfg Config) bool { return true } -// shouldIncludeEngine checks if a recommendation should be included based on engine filters -func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool { +// shouldIncludeEngine checks if a recommendation should be included based on engine filters. +func shouldIncludeEngine(rec *common.Recommendation, cfg *Config) bool { // Extract engine from recommendation - engine := getEngineFromRecommendation(rec) + engine := getEngineFromRecommendation(*rec) if engine == "" { // If no engine info, include by default unless there's an include list return len(cfg.IncludeEngines) == 0 @@ -142,7 +143,7 @@ func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool { if len(cfg.IncludeEngines) > 0 { found := false for _, e := range cfg.IncludeEngines { - if strings.ToLower(e) == engine { + if strings.EqualFold(e, engine) { found = true break } @@ -155,7 +156,7 @@ func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool { // If exclude list is specified, engine must not be in it if len(cfg.ExcludeEngines) > 0 { for _, e := range cfg.ExcludeEngines { - if strings.ToLower(e) == engine { + if strings.EqualFold(e, engine) { return false } } @@ -164,8 +165,8 @@ func shouldIncludeEngine(rec common.Recommendation, cfg Config) bool { return true } -// shouldIncludeAccount checks if an account should be included based on filters -func shouldIncludeAccount(accountName string, cfg Config) bool { +// shouldIncludeAccount checks if an account should be included based on filters. +func shouldIncludeAccount(accountName string, cfg *Config) bool { // If account name is empty and there are filters, skip it (unless include list is empty) if accountName == "" { return len(cfg.IncludeAccounts) == 0 && len(cfg.ExcludeAccounts) == 0 @@ -186,7 +187,7 @@ func shouldIncludeAccount(accountName string, cfg Config) bool { return true } -// checkIncludeList checks if an account matches the include filters +// checkIncludeList checks if an account matches the include filters. func checkIncludeList(accountLower string, includeAccounts []string) bool { if len(includeAccounts) == 0 { return true @@ -201,7 +202,7 @@ func checkIncludeList(accountLower string, includeAccounts []string) bool { return false } -// checkExcludeList checks if an account matches any exclude filters +// checkExcludeList checks if an account matches any exclude filters. func checkExcludeList(accountLower string, excludeAccounts []string) bool { for _, filter := range excludeAccounts { if accountMatchesFilter(accountLower, filter) { @@ -211,7 +212,7 @@ func checkExcludeList(accountLower string, excludeAccounts []string) bool { return false } -// accountMatchesFilter checks if an account matches a filter pattern (exact or substring match) +// accountMatchesFilter checks if an account matches a filter pattern (exact or substring match). func accountMatchesFilter(accountLower, filter string) bool { filterLower := strings.ToLower(filter) return filterLower == accountLower || strings.Contains(accountLower, filterLower) diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index 215d65ae0..ec2bc5f72 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -111,7 +111,7 @@ func TestApplyFilters(t *testing.T) { toolCfg.ExcludeInstanceTypes = tt.excludeInstanceTypes // Apply filters with Config (empty currentRegion for test) - result := applyFilters(tt.recommendations, toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), "") + result := applyFilters(tt.recommendations, &toolCfg, make(map[string][]InstanceEngineVersion), make(map[string]MajorEngineVersionInfo), "") // Check count assert.Equal(t, tt.expectedCount, len(result)) @@ -169,7 +169,7 @@ func TestShouldIncludeRegion(t *testing.T) { toolCfg.IncludeRegions = tt.includeRegions toolCfg.ExcludeRegions = tt.excludeRegions - result := shouldIncludeRegion(tt.region, toolCfg) + result := shouldIncludeRegion(tt.region, &toolCfg) assert.Equal(t, tt.expected, result) }) } @@ -225,7 +225,7 @@ func TestShouldIncludeInstanceType(t *testing.T) { toolCfg.IncludeInstanceTypes = tt.includeInstanceTypes toolCfg.ExcludeInstanceTypes = tt.excludeInstanceTypes - result := shouldIncludeInstanceType(tt.instanceType, toolCfg) + result := shouldIncludeInstanceType(tt.instanceType, &toolCfg) assert.Equal(t, tt.expected, result) }) } @@ -345,7 +345,7 @@ func TestShouldIncludeEngine(t *testing.T) { toolCfg.IncludeEngines = tt.includeEngines toolCfg.ExcludeEngines = tt.excludeEngines - result := shouldIncludeEngine(tt.recommendation, toolCfg) + result := shouldIncludeEngine(&tt.recommendation, &toolCfg) assert.Equal(t, tt.expected, result) }) } @@ -408,7 +408,7 @@ func TestShouldIncludeAccount(t *testing.T) { toolCfg.IncludeAccounts = tt.includeAccounts toolCfg.ExcludeAccounts = tt.excludeAccounts - result := shouldIncludeAccount(tt.accountID, toolCfg) + result := shouldIncludeAccount(tt.accountID, &toolCfg) assert.Equal(t, tt.expected, result) }) } diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 317fff5d6..b30426820 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -458,7 +458,7 @@ func applyRegionFilters( cfg Config, ) []common.Recommendation { originalCount := len(recs) - recs = applyFilters(recs, cfg, engineData.instanceVersions, engineData.versionInfo, region) + recs = applyFilters(recs, &cfg, engineData.instanceVersions, engineData.versionInfo, region) if len(recs) < originalCount { AppLogger.Printf(" 🔍 After filters: %d recommendations (filtered out %d)\n", len(recs), originalCount-len(recs)) diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 7187e6232..fc28c92e7 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -837,7 +837,7 @@ func TestApplyFilters_RegionFiltering(t *testing.T) { ExcludeRegions: tt.excludeRegions, } - result := applyFilters(tt.recs, cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, tt.currentRegion) + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, tt.currentRegion) assert.Equal(t, tt.expectedCount, len(result), "Expected %d recommendations, got %d", tt.expectedCount, len(result)) }) } @@ -882,7 +882,7 @@ func TestApplyFilters_InstanceTypeFiltering(t *testing.T) { ExcludeInstanceTypes: tt.excludeInstanceTypes, } - result := applyFilters(tt.recs, cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") assert.Equal(t, tt.expectedCount, len(result)) }) } @@ -957,7 +957,7 @@ func TestApplyFilters_EngineFiltering(t *testing.T) { ExcludeEngines: tt.excludeEngines, } - result := applyFilters(tt.recs, cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") assert.Equal(t, tt.expectedCount, len(result)) }) } @@ -1021,7 +1021,7 @@ func TestApplyFilters_AccountFiltering(t *testing.T) { ExcludeAccounts: tt.excludeAccounts, } - result := applyFilters(tt.recs, cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(tt.recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") assert.Equal(t, tt.expectedCount, len(result)) }) } @@ -1042,7 +1042,7 @@ func TestApplyFilters_CombinedFilters(t *testing.T) { IncludeAccounts: []string{"prod", "dev"}, } - result := applyFilters(recs, cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") + result := applyFilters(recs, &cfg, map[string][]InstanceEngineVersion{}, map[string]MajorEngineVersionInfo{}, "") // Only the first two should pass all filters assert.Equal(t, 2, len(result)) diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go index 8e322b4f3..d8ec3d878 100644 --- a/internal/auth/service_password_test.go +++ b/internal/auth/service_password_test.go @@ -591,15 +591,15 @@ func TestService_ResetTokenStatus(t *testing.T) { }) } -// Test password validation rules +// Test password validation rules. func TestValidatePassword(t *testing.T) { service := &Service{} tests := []struct { name string password string - wantErr bool errMsg string + wantErr bool }{ { name: "valid strong password", @@ -708,7 +708,7 @@ func TestValidatePassword(t *testing.T) { } } -// Test checkCommonPasswords uses exact match (not substring) +// Test checkCommonPasswords uses exact match (not substring). func TestCheckCommonPasswords(t *testing.T) { service := &Service{} @@ -722,7 +722,7 @@ func TestCheckCommonPasswords(t *testing.T) { assert.NoError(t, service.checkCommonPasswords("SuperAdmin2024")) } -// Test containsRepeatedChars function +// Test containsRepeatedChars function. func TestContainsSequentialChars(t *testing.T) { tests := []struct { name string @@ -788,14 +788,14 @@ func TestContainsSequentialChars(t *testing.T) { } } -// Test addToPasswordHistory function +// Test addToPasswordHistory function. func TestAddToPasswordHistory(t *testing.T) { tests := []struct { name string currentHash string + expectedFirst string existingHistory []string expectedLen int - expectedFirst string }{ { name: "empty history", @@ -838,7 +838,7 @@ func TestAddToPasswordHistory(t *testing.T) { } } -// Test checkPasswordHistory +// Test checkPasswordHistory. func TestCheckPasswordHistory(t *testing.T) { service := newTestService() diff --git a/internal/auth/store_postgres_test.go b/internal/auth/store_postgres_test.go index cfd06d00f..567547b03 100644 --- a/internal/auth/store_postgres_test.go +++ b/internal/auth/store_postgres_test.go @@ -14,7 +14,7 @@ import ( "github.com/stretchr/testify/require" ) -// MockDBConnection mocks database.Connection +// MockDBConnection mocks database.Connection. type MockDBConnection struct { mock.Mock } @@ -42,10 +42,10 @@ func (m *MockDBConnection) Ping(ctx context.Context) error { return args.Error(0) } -// MockRow mocks pgx.Row +// MockRow mocks pgx.Row. type MockRow struct { - mock.Mock scanFunc func(dest ...interface{}) error + mock.Mock } func (m *MockRow) Scan(dest ...interface{}) error { @@ -64,7 +64,7 @@ func (m *MockRow) Scan(dest ...interface{}) error { // mfa_pending_secret_expires_at (NullTime), mfa_recovery_codes ([]string), // reset_token (NullString), reset_expiry (NullTime), // failed_login_attempts, locked_until (NullTime), password_history, -// created_at, updated_at, last_login_at (NullTime) +// created_at, updated_at, last_login_at (NullTime). func createMockRowWithUser(user *User) *MockRow { return &MockRow{ scanFunc: func(dest ...interface{}) error { @@ -131,7 +131,7 @@ func createMockRowWithUser(user *User) *MockRow { } } -// Helper function to create a mock row that returns an error +// Helper function to create a mock row that returns an error. func createMockRowWithError(err error) *MockRow { return &MockRow{ scanFunc: func(dest ...interface{}) error { @@ -691,7 +691,7 @@ func TestPostgresStore_CleanupExpiredSessions(t *testing.T) { // GROUP TESTS // ========================================== -// Helper function to create a mock row that returns a group +// Helper function to create a mock row that returns a group. func createMockRowWithGroup(group *Group) *MockRow { return &MockRow{ scanFunc: func(dest ...interface{}) error { @@ -910,7 +910,7 @@ func TestPostgresStore_DeleteGroup(t *testing.T) { // API KEY TESTS // ========================================== -// Helper function to create a mock row that returns an API key +// Helper function to create a mock row that returns an API key. func createMockRowWithAPIKey(key *UserAPIKey) *MockRow { return &MockRow{ scanFunc: func(dest ...interface{}) error { diff --git a/internal/purchase/approvals.go b/internal/purchase/approvals.go index 3c3225e77..0e85cc46a 100644 --- a/internal/purchase/approvals.go +++ b/internal/purchase/approvals.go @@ -51,10 +51,10 @@ func (m *Manager) ApproveExecution(ctx context.Context, executionID, token, acto // Preflight guard (issue #609): reject non-AWS orphan executions before // the cloud SDK is reached. See OrphanExecutionError for the full rationale. - if err := OrphanExecutionError(execution); err != nil { + if checkErr := OrphanExecutionError(execution); checkErr != nil { logging.Errorf("purchase[%s]: ApproveExecution preflight rejected after %s: %v", - executionID, time.Since(t0), err) - return err + executionID, time.Since(t0), checkErr) + return checkErr } // Token/SQS path: no authenticated session UUID is available, so the @@ -190,7 +190,7 @@ func (m *Manager) ApproveAndExecute(ctx context.Context, executionID, actor stri // status IN ('pending','notified') so a concurrent approve that wins // the race causes zero rows to be affected and the caller receives a // clean error with the current status rather than silently overwriting -// the approved row. This is the token/email-link cancel analogue of +// the approved row. This is the token/email-link cancel analog of // the atomic guard TransitionExecutionStatus provides for ApproveAndExecute. func (m *Manager) CancelExecution(ctx context.Context, executionID, token, actor string) error { logging.Infof("Cancelling execution: %s", executionID) diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 078145fc4..6032ea9ec 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -227,7 +227,7 @@ func TestHandleExecutePurchase_ApprovedStatus(t *testing.T) { Type: MessageTypeExecutePurchase, ExecutionID: "exec-approved", } - err := manager.handleExecutePurchase(ctx, msg) + err := manager.handleExecutePurchase(ctx, &msg) require.NoError(t, err) mockStore.AssertExpectations(t) @@ -251,7 +251,7 @@ func TestHandleExecutePurchase_GetError(t *testing.T) { Type: MessageTypeExecutePurchase, ExecutionID: "exec-err", } - err := manager.handleExecutePurchase(ctx, msg) + err := manager.handleExecutePurchase(ctx, &msg) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to get execution") } @@ -310,7 +310,7 @@ func TestHandleExecutePurchase_SaveError(t *testing.T) { Type: MessageTypeExecutePurchase, ExecutionID: "exec-save-err", } - err := manager.handleExecutePurchase(ctx, msg) + err := manager.handleExecutePurchase(ctx, &msg) // The terminal-save failure now surfaces from executeAndFinalize as // ErrAuditLoss (the row is stranded in "running"). The SQS handler returns // it so the message is redelivered. A single-account provider failure with @@ -497,7 +497,7 @@ func TestProcessMessage_ApproveRejectsNonMatchingActor(t *testing.T) { err := manager.ProcessMessage(ctx, `{"type":"approve","execution_id":"exec-mismatch","token":"correct-token","actor_email":"intruder@example.com"}`) require.Error(t, err) - assert.Contains(t, err.Error(), "not an authorised approver") + assert.Contains(t, err.Error(), "not an authorized approver") // SavePurchaseExecution must NOT have been called — no mutation. mockStore.AssertNotCalled(t, "SavePurchaseExecution") } diff --git a/internal/purchase/messages.go b/internal/purchase/messages.go index 661af009d..d83d2a3c1 100644 --- a/internal/purchase/messages.go +++ b/internal/purchase/messages.go @@ -12,17 +12,17 @@ import ( "github.com/LeanerCloud/CUDly/pkg/logging" ) -// MessageType defines the types of async messages that can be processed +// MessageType defines the types of async messages that can be processed. type MessageType string const ( - // MessageTypeExecutePurchase triggers execution of a scheduled purchase + // MessageTypeExecutePurchase triggers execution of a scheduled purchase. MessageTypeExecutePurchase MessageType = "execute_purchase" - // MessageTypeApprove approves a pending execution + // MessageTypeApprove approves a pending execution. MessageTypeApprove MessageType = "approve" - // MessageTypeCancel cancels a pending execution + // MessageTypeCancel cancels a pending execution. MessageTypeCancel MessageType = "cancel" - // MessageTypeSendNotification sends a notification for upcoming purchase + // MessageTypeSendNotification sends a notification for upcoming purchase. MessageTypeSendNotification MessageType = "send_notification" ) @@ -64,11 +64,11 @@ func (m *Manager) ProcessMessage(ctx context.Context, body string) error { switch msg.Type { case MessageTypeExecutePurchase: - return m.handleExecutePurchase(ctx, msg) + return m.handleExecutePurchase(ctx, &msg) case MessageTypeApprove: - return m.handleApproveMessage(ctx, msg) + return m.handleApproveMessage(ctx, &msg) case MessageTypeCancel: - return m.handleCancelMessage(ctx, msg) + return m.handleCancelMessage(ctx, &msg) case MessageTypeSendNotification: _, err := m.SendUpcomingPurchaseNotifications(ctx) return err @@ -78,8 +78,8 @@ func (m *Manager) ProcessMessage(ctx context.Context, body string) error { } } -// handleExecutePurchase processes an execute_purchase message -func (m *Manager) handleExecutePurchase(ctx context.Context, msg AsyncMessage) error { +// handleExecutePurchase processes an execute_purchase message. +func (m *Manager) handleExecutePurchase(ctx context.Context, msg *AsyncMessage) error { if msg.ExecutionID == "" { return fmt.Errorf("execution_id required for execute_purchase message") } @@ -140,11 +140,11 @@ func (m *Manager) handleExecutePurchase(ctx context.Context, msg AsyncMessage) e // for any reason is exactly the threat model this fix addresses. Any // legitimate stranded action can be re-issued via the HTTP route gated // by authorizeApprovalAction. -func (m *Manager) handleApproveMessage(ctx context.Context, msg AsyncMessage) error { +func (m *Manager) handleApproveMessage(ctx context.Context, msg *AsyncMessage) error { if msg.ExecutionID == "" || msg.Token == "" { return fmt.Errorf("execution_id and token required for approve message") } - if err := m.verifyAsyncApprovalActor(ctx, &msg); err != nil { + if err := m.verifyAsyncApprovalActor(ctx, msg); err != nil { return err } return m.ApproveExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail) @@ -153,11 +153,11 @@ func (m *Manager) handleApproveMessage(ctx context.Context, msg AsyncMessage) er // handleCancelMessage processes a cancel message. Same hardening as // handleApproveMessage: token + actor_email + approver-list match are // all required. -func (m *Manager) handleCancelMessage(ctx context.Context, msg AsyncMessage) error { +func (m *Manager) handleCancelMessage(ctx context.Context, msg *AsyncMessage) error { if msg.ExecutionID == "" || msg.Token == "" { return fmt.Errorf("execution_id and token required for cancel message") } - if err := m.verifyAsyncApprovalActor(ctx, &msg); err != nil { + if err := m.verifyAsyncApprovalActor(ctx, msg); err != nil { return err } return m.CancelExecution(ctx, msg.ExecutionID, msg.Token, msg.ActorEmail) @@ -233,7 +233,7 @@ func (m *Manager) matchActorAgainstApprovers(ctx context.Context, actor string, return nil } } - return fmt.Errorf("actor email is not an authorised approver for this purchase") + return fmt.Errorf("actor email is not an authorized approver for this purchase") } // gatherApproverContactEmails mirrors the algorithm in @@ -253,7 +253,8 @@ func (m *Manager) gatherApproverContactEmails(ctx context.Context, recs []config seenAccount := map[string]bool{} seenEmail := map[string]bool{} var out []string - for _, rec := range recs { + for i := range recs { + rec := &recs[i] if rec.CloudAccountID == nil || *rec.CloudAccountID == "" { continue } diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index d8f661e96..5f9d6cc73 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -266,8 +266,8 @@ func TestSQSRedeliveryDoesNotDoubleExecute(t *testing.T) { } msg := AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "exec-dup"} - require.NoError(t, manager.handleExecutePurchase(ctx, msg), "first delivery executes cleanly") - require.NoError(t, manager.handleExecutePurchase(ctx, msg), "redelivery is a benign no-op (acked)") + require.NoError(t, manager.handleExecutePurchase(ctx, &msg), "first delivery executes cleanly") + require.NoError(t, manager.handleExecutePurchase(ctx, &msg), "redelivery is a benign no-op (acked)") mu.Lock() defer mu.Unlock() @@ -348,7 +348,7 @@ func TestMultiAccountPartialSuccessIsAcked(t *testing.T) { } msg := AsyncMessage{Type: MessageTypeExecutePurchase, ExecutionID: "root-partial"} - err := manager.handleExecutePurchase(ctx, msg) + err := manager.handleExecutePurchase(ctx, &msg) require.NoError(t, err, "a multi-account run with >=1 committed account must be ACKed (return nil), not redelivered (issue #1014)")