diff --git a/pkg/workflow/map_helpers.go b/pkg/workflow/map_helpers.go index a86572d92f..a603058cfd 100644 --- a/pkg/workflow/map_helpers.go +++ b/pkg/workflow/map_helpers.go @@ -18,14 +18,9 @@ // // Type Conversion: // - parseIntValue() - Safely parse numeric types to int with truncation warnings -// - isEmptyOrNil() - Check if a value is empty, nil, or zero // // Map Operations: // - filterMapKeys() - Create new map excluding specified keys -// - getMapFieldAsString() - Safely extract a string field from a map[string]any -// - getMapFieldAsMap() - Safely extract a nested map from a map[string]any -// - getMapFieldAsBool() - Safely extract a boolean field from a map[string]any -// - getMapFieldAsInt() - Safely extract an integer field from a map[string]any // // These utilities handle common type conversion and map manipulation patterns that // occur frequently during YAML-to-struct parsing and configuration processing. @@ -33,8 +28,6 @@ package workflow import ( - "strings" - "github.com/github/gh-aw/pkg/logger" ) @@ -83,221 +76,3 @@ func filterMapKeys(original map[string]any, excludeKeys ...string) map[string]an } return result } - -// isEmptyOrNil evaluates whether a value represents an empty or absent state. -// This consolidates various emptiness checks across the codebase into a single -// reusable function. The function handles multiple value types with appropriate -// emptiness semantics for each. -// -// Returns true when encountering: -// - nil values (representing absence) -// - strings that are empty or contain only whitespace -// - numeric types equal to zero -// - boolean false -// - collections (slices, maps) with no elements -// -// Usage pattern: -// -// if isEmptyOrNil(configValue) { -// return NewValidationError("fieldName", "", "required field missing", "provide a value") -// } -func isEmptyOrNil(candidate any) bool { - // Handle nil case first - if candidate == nil { - return true - } - - // Type-specific emptiness checks using reflection-free approach - switch typedValue := candidate.(type) { - case string: - // String is empty if blank after trimming whitespace - return len(strings.TrimSpace(typedValue)) == 0 - case int: - return typedValue == 0 - case int8: - return typedValue == 0 - case int16: - return typedValue == 0 - case int32: - return typedValue == 0 - case int64: - return typedValue == 0 - case uint: - return typedValue == 0 - case uint8: - return typedValue == 0 - case uint16: - return typedValue == 0 - case uint32: - return typedValue == 0 - case uint64: - return typedValue == 0 - case float32: - return typedValue == 0.0 - case float64: - return typedValue == 0.0 - case bool: - // false represents empty boolean state - return !typedValue - case []any: - return len(typedValue) == 0 - case map[string]any: - return len(typedValue) == 0 - } - - // Non-nil values of unrecognized types are considered non-empty - return false -} - -// getMapFieldAsString retrieves a string value from a configuration map with safe type handling. -// This function wraps the common pattern of extracting string fields from map[string]any structures -// that result from YAML parsing, providing consistent error behavior and logging. -// -// The function returns the fallback value in these scenarios: -// - Source map is nil -// - Requested key doesn't exist in map -// - Value at key is not a string type -// -// Parameters: -// - source: The configuration map to query -// - fieldKey: The key to look up in the map -// - fallback: Value returned when extraction fails -// -// Example usage: -// -// titleValue := getMapFieldAsString(frontmatter, "title", "") -// if titleValue == "" { -// return NewValidationError("title", "", "title required", "provide a title") -// } -func getMapFieldAsString(source map[string]any, fieldKey string, fallback string) string { - // Early return for nil map - if source == nil { - return fallback - } - - // Attempt to retrieve value - retrievedValue, keyFound := source[fieldKey] - if !keyFound { - return fallback - } - - // Verify type before returning - stringValue, isString := retrievedValue.(string) - if !isString { - mapHelpersLog.Printf("Type mismatch for key %q: expected string, found %T", fieldKey, retrievedValue) - return fallback - } - - return stringValue -} - -// getMapFieldAsMap retrieves a nested map value from a configuration map with safe type handling. -// This consolidates the pattern of extracting nested configuration sections while handling -// type mismatches gracefully. Returns nil when the field cannot be extracted as a map. -// -// Parameters: -// - source: The parent configuration map -// - fieldKey: The key identifying the nested map -// -// Example usage: -// -// toolsSection := getMapFieldAsMap(config, "tools") -// if toolsSection != nil { -// playwrightConfig := getMapFieldAsMap(toolsSection, "playwright") -// } -func getMapFieldAsMap(source map[string]any, fieldKey string) map[string]any { - // Guard against nil source - if source == nil { - return nil - } - - // Look up the field - retrievedValue, keyFound := source[fieldKey] - if !keyFound { - return nil - } - - // Type assert to nested map - mapValue, isMap := retrievedValue.(map[string]any) - if !isMap { - mapHelpersLog.Printf("Type mismatch for key %q: expected map[string]any, found %T", fieldKey, retrievedValue) - return nil - } - - return mapValue -} - -// getMapFieldAsBool retrieves a boolean value from a configuration map with safe type handling. -// This wraps the pattern of extracting boolean configuration flags while providing consistent -// fallback behavior when the value is missing or has an unexpected type. -// -// Parameters: -// - source: The configuration map to query -// - fieldKey: The key to look up -// - fallback: Value returned when extraction fails -// -// Example usage: -// -// sandboxEnabled := getMapFieldAsBool(config, "sandbox", false) -// if sandboxEnabled { -// // Enable sandbox mode -// } -func getMapFieldAsBool(source map[string]any, fieldKey string, fallback bool) bool { - // Handle nil source - if source == nil { - return fallback - } - - // Retrieve value from map - retrievedValue, keyFound := source[fieldKey] - if !keyFound { - return fallback - } - - // Verify boolean type - booleanValue, isBoolean := retrievedValue.(bool) - if !isBoolean { - mapHelpersLog.Printf("Type mismatch for key %q: expected bool, found %T", fieldKey, retrievedValue) - return fallback - } - - return booleanValue -} - -// getMapFieldAsInt retrieves an integer value from a configuration map with automatic numeric type conversion. -// This function handles the common pattern of extracting numeric config values that may be represented -// as various numeric types in YAML (int, int64, float64, uint64). It delegates to parseIntValue for -// the actual type conversion logic. -// -// Parameters: -// - source: The configuration map to query -// - fieldKey: The key to look up -// - fallback: Value returned when extraction or conversion fails -// -// Example usage: -// -// retentionDays := getMapFieldAsInt(config, "retention-days", 30) -// if err := validateIntRange(retentionDays, 1, 90, "retention-days"); err != nil { -// return err -// } -func getMapFieldAsInt(source map[string]any, fieldKey string, fallback int) int { - // Guard against nil source - if source == nil { - return fallback - } - - // Look up the value - retrievedValue, keyFound := source[fieldKey] - if !keyFound { - return fallback - } - - // Attempt numeric conversion using existing utility - convertedInt, conversionOk := parseIntValue(retrievedValue) - if !conversionOk { - mapHelpersLog.Printf("Failed to convert key %q to int: got %T", fieldKey, retrievedValue) - return fallback - } - - return convertedInt -} diff --git a/pkg/workflow/validation_helpers.go b/pkg/workflow/validation_helpers.go index 86a84bb3e3..e2065c67a0 100644 --- a/pkg/workflow/validation_helpers.go +++ b/pkg/workflow/validation_helpers.go @@ -7,12 +7,6 @@ // # Available Helper Functions // // - validateIntRange() - Validates that an integer value is within a specified range -// - ValidateRequired() - Validates that a required field is not empty -// - ValidateMaxLength() - Validates that a field does not exceed maximum length -// - ValidateMinLength() - Validates that a field meets minimum length requirement -// - ValidateInList() - Validates that a value is in an allowed list -// - ValidatePositiveInt() - Validates that a value is a positive integer -// - ValidateNonNegativeInt() - Validates that a value is a non-negative integer // - validateMountStringFormat() - Parses and validates a "source:dest:mode" mount string // // # Design Rationale @@ -31,8 +25,6 @@ package workflow import ( "errors" "fmt" - "slices" - "strconv" "strings" "github.com/github/gh-aw/pkg/logger" @@ -68,87 +60,6 @@ func validateIntRange(value, min, max int, fieldName string) error { return nil } -// ValidateRequired validates that a required field is not empty -func ValidateRequired(field, value string) error { - if strings.TrimSpace(value) == "" { - validationHelpersLog.Printf("Required field validation failed: field=%s", field) - return NewValidationError( - field, - value, - "field is required and cannot be empty", - fmt.Sprintf("Provide a non-empty value for '%s'", field), - ) - } - return nil -} - -// ValidateMaxLength validates that a field does not exceed maximum length -func ValidateMaxLength(field, value string, maxLength int) error { - if len(value) > maxLength { - return NewValidationError( - field, - value, - fmt.Sprintf("field exceeds maximum length of %d characters (actual: %d)", maxLength, len(value)), - fmt.Sprintf("Shorten '%s' to %d characters or less", field, maxLength), - ) - } - return nil -} - -// ValidateMinLength validates that a field meets minimum length requirement -func ValidateMinLength(field, value string, minLength int) error { - if len(value) < minLength { - return NewValidationError( - field, - value, - fmt.Sprintf("field is shorter than minimum length of %d characters (actual: %d)", minLength, len(value)), - fmt.Sprintf("Ensure '%s' is at least %d characters long", field, minLength), - ) - } - return nil -} - -// ValidateInList validates that a value is in an allowed list -func ValidateInList(field, value string, allowedValues []string) error { - if slices.Contains(allowedValues, value) { - return nil - } - - validationHelpersLog.Printf("List validation failed: field=%s, value=%s not in allowed list", field, value) - return NewValidationError( - field, - value, - fmt.Sprintf("value is not in allowed list: %v", allowedValues), - fmt.Sprintf("Choose one of the allowed values for '%s': %s", field, strings.Join(allowedValues, ", ")), - ) -} - -// ValidatePositiveInt validates that a value is a positive integer -func ValidatePositiveInt(field string, value int) error { - if value <= 0 { - return NewValidationError( - field, - strconv.Itoa(value), - "value must be a positive integer", - fmt.Sprintf("Provide a positive integer value for '%s'", field), - ) - } - return nil -} - -// ValidateNonNegativeInt validates that a value is a non-negative integer -func ValidateNonNegativeInt(field string, value int) error { - if value < 0 { - return NewValidationError( - field, - strconv.Itoa(value), - "value must be a non-negative integer", - fmt.Sprintf("Provide a non-negative integer value for '%s'", field), - ) - } - return nil -} - // validateMountStringFormat parses a mount string and validates its basic format. // Expected format: "source:destination:mode" where mode is "ro" or "rw". // Returns (source, dest, mode, nil) on success, or ("", "", "", error) on failure. diff --git a/pkg/workflow/validation_helpers_test.go b/pkg/workflow/validation_helpers_test.go index b6ea6df8cb..c4885e23b2 100644 --- a/pkg/workflow/validation_helpers_test.go +++ b/pkg/workflow/validation_helpers_test.go @@ -310,412 +310,6 @@ func TestValidateIntRangeWithRealWorldValues(t *testing.T) { } } -func TestValidateRequired(t *testing.T) { - t.Run("valid non-empty value", func(t *testing.T) { - err := ValidateRequired("title", "my title") - assert.NoError(t, err) - }) - - t.Run("empty value fails", func(t *testing.T) { - err := ValidateRequired("title", "") - require.Error(t, err) - assert.Contains(t, err.Error(), "field is required") - assert.Contains(t, err.Error(), "Provide a non-empty value") - }) - - t.Run("whitespace-only value fails", func(t *testing.T) { - err := ValidateRequired("title", " ") - require.Error(t, err) - assert.Contains(t, err.Error(), "cannot be empty") - }) -} - -func TestValidateMaxLength(t *testing.T) { - t.Run("value within limit", func(t *testing.T) { - err := ValidateMaxLength("title", "short", 100) - assert.NoError(t, err) - }) - - t.Run("value at limit", func(t *testing.T) { - err := ValidateMaxLength("title", "12345", 5) - assert.NoError(t, err) - }) - - t.Run("value exceeds limit", func(t *testing.T) { - err := ValidateMaxLength("title", "too long value", 5) - require.Error(t, err) - assert.Contains(t, err.Error(), "exceeds maximum length") - assert.Contains(t, err.Error(), "Shorten") - }) -} - -func TestValidateMinLength(t *testing.T) { - t.Run("value meets minimum", func(t *testing.T) { - err := ValidateMinLength("title", "hello", 3) - assert.NoError(t, err) - }) - - t.Run("value below minimum", func(t *testing.T) { - err := ValidateMinLength("title", "hi", 5) - require.Error(t, err) - assert.Contains(t, err.Error(), "shorter than minimum length") - assert.Contains(t, err.Error(), "at least 5 characters") - }) -} - -func TestValidateInList(t *testing.T) { - allowedValues := []string{"open", "closed", "draft"} - - t.Run("value in list", func(t *testing.T) { - err := ValidateInList("status", "open", allowedValues) - assert.NoError(t, err) - }) - - t.Run("value not in list", func(t *testing.T) { - err := ValidateInList("status", "invalid", allowedValues) - require.Error(t, err) - assert.Contains(t, err.Error(), "not in allowed list") - assert.Contains(t, err.Error(), "open, closed, draft") - }) -} - -func TestValidatePositiveInt(t *testing.T) { - t.Run("positive integer", func(t *testing.T) { - err := ValidatePositiveInt("count", 5) - assert.NoError(t, err) - }) - - t.Run("zero fails", func(t *testing.T) { - err := ValidatePositiveInt("count", 0) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be a positive integer") - }) - - t.Run("negative fails", func(t *testing.T) { - err := ValidatePositiveInt("count", -1) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be a positive integer") - }) -} - -func TestValidateNonNegativeInt(t *testing.T) { - t.Run("positive integer", func(t *testing.T) { - err := ValidateNonNegativeInt("count", 5) - assert.NoError(t, err) - }) - - t.Run("zero is valid", func(t *testing.T) { - err := ValidateNonNegativeInt("count", 0) - assert.NoError(t, err) - }) - - t.Run("negative fails", func(t *testing.T) { - err := ValidateNonNegativeInt("count", -1) - require.Error(t, err) - assert.Contains(t, err.Error(), "must be a non-negative integer") - }) -} - -// TestIsEmptyOrNil tests the isEmptyOrNil helper function -func TestIsEmptyOrNil(t *testing.T) { - tests := []struct { - name string - value any - expected bool - }{ - // Nil values - {"nil value", nil, true}, - - // String values - {"empty string", "", true}, - {"whitespace string", " ", true}, - {"non-empty string", "hello", false}, - - // Integer values - {"zero int", 0, true}, - {"positive int", 5, false}, - {"negative int", -1, false}, - {"zero int64", int64(0), true}, - {"positive int64", int64(5), false}, - - // Unsigned integer values - {"zero uint", uint(0), true}, - {"positive uint", uint(5), false}, - {"zero uint64", uint64(0), true}, - {"positive uint64", uint64(5), false}, - - // Float values - {"zero float32", float32(0), true}, - {"positive float32", float32(5.5), false}, - {"zero float64", float64(0), true}, - {"positive float64", float64(5.5), false}, - - // Boolean values - {"false bool", false, true}, - {"true bool", true, false}, - - // Slice values - {"empty slice", []any{}, true}, - {"non-empty slice", []any{1, 2}, false}, - - // Map values - {"empty map", map[string]any{}, true}, - {"non-empty map", map[string]any{"key": "value"}, false}, - - // Other types - {"struct value", struct{ field string }{"value"}, false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := isEmptyOrNil(tt.value) - assert.Equal(t, tt.expected, result, "isEmptyOrNil(%v) = %v, want %v", tt.value, result, tt.expected) - }) - } -} - -// TestGetMapFieldAsString tests the getMapFieldAsString helper function -func TestGetMapFieldAsString(t *testing.T) { - tests := []struct { - name string - m map[string]any - key string - defaultVal string - expected string - }{ - { - name: "extract existing string", - m: map[string]any{"title": "Test Title"}, - key: "title", - defaultVal: "", - expected: "Test Title", - }, - { - name: "missing key returns default", - m: map[string]any{"other": "value"}, - key: "title", - defaultVal: "default", - expected: "default", - }, - { - name: "non-string value returns default", - m: map[string]any{"title": 123}, - key: "title", - defaultVal: "default", - expected: "default", - }, - { - name: "nil map returns default", - m: nil, - key: "title", - defaultVal: "default", - expected: "default", - }, - { - name: "empty string value", - m: map[string]any{"title": ""}, - key: "title", - defaultVal: "default", - expected: "", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := getMapFieldAsString(tt.m, tt.key, tt.defaultVal) - assert.Equal(t, tt.expected, result) - }) - } -} - -// TestGetMapFieldAsMap tests the getMapFieldAsMap helper function -func TestGetMapFieldAsMap(t *testing.T) { - tests := []struct { - name string - m map[string]any - key string - expected map[string]any - }{ - { - name: "extract existing nested map", - m: map[string]any{ - "network": map[string]any{ - "allowed-domains": "example.com", - }, - }, - key: "network", - expected: map[string]any{"allowed-domains": "example.com"}, - }, - { - name: "missing key returns nil", - m: map[string]any{"other": "value"}, - key: "network", - expected: nil, - }, - { - name: "non-map value returns nil", - m: map[string]any{"network": "not a map"}, - key: "network", - expected: nil, - }, - { - name: "nil map returns nil", - m: nil, - key: "network", - expected: nil, - }, - { - name: "empty nested map", - m: map[string]any{"network": map[string]any{}}, - key: "network", - expected: map[string]any{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := getMapFieldAsMap(tt.m, tt.key) - assert.Equal(t, tt.expected, result) - }) - } -} - -// TestGetMapFieldAsBool tests the getMapFieldAsBool helper function -func TestGetMapFieldAsBool(t *testing.T) { - tests := []struct { - name string - m map[string]any - key string - defaultVal bool - expected bool - }{ - { - name: "extract true value", - m: map[string]any{"enabled": true}, - key: "enabled", - defaultVal: false, - expected: true, - }, - { - name: "extract false value", - m: map[string]any{"enabled": false}, - key: "enabled", - defaultVal: true, - expected: false, - }, - { - name: "missing key returns default", - m: map[string]any{"other": true}, - key: "enabled", - defaultVal: true, - expected: true, - }, - { - name: "non-bool value returns default", - m: map[string]any{"enabled": "true"}, - key: "enabled", - defaultVal: false, - expected: false, - }, - { - name: "nil map returns default", - m: nil, - key: "enabled", - defaultVal: true, - expected: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := getMapFieldAsBool(tt.m, tt.key, tt.defaultVal) - assert.Equal(t, tt.expected, result) - }) - } -} - -// TestGetMapFieldAsInt tests the getMapFieldAsInt helper function -func TestGetMapFieldAsInt(t *testing.T) { - tests := []struct { - name string - m map[string]any - key string - defaultVal int - expected int - }{ - { - name: "extract int value", - m: map[string]any{"max-size": 100}, - key: "max-size", - defaultVal: 0, - expected: 100, - }, - { - name: "extract int64 value", - m: map[string]any{"max-size": int64(200)}, - key: "max-size", - defaultVal: 0, - expected: 200, - }, - { - name: "extract float64 value", - m: map[string]any{"max-size": float64(300)}, - key: "max-size", - defaultVal: 0, - expected: 300, - }, - { - name: "extract uint64 value", - m: map[string]any{"max-size": uint64(400)}, - key: "max-size", - defaultVal: 0, - expected: 400, - }, - { - name: "missing key returns default", - m: map[string]any{"other": 100}, - key: "max-size", - defaultVal: 50, - expected: 50, - }, - { - name: "non-numeric value returns default", - m: map[string]any{"max-size": "100"}, - key: "max-size", - defaultVal: 50, - expected: 50, - }, - { - name: "nil map returns default", - m: nil, - key: "max-size", - defaultVal: 50, - expected: 50, - }, - { - name: "zero value", - m: map[string]any{"max-size": 0}, - key: "max-size", - defaultVal: 100, - expected: 0, - }, - { - name: "negative value", - m: map[string]any{"max-size": -10}, - key: "max-size", - defaultVal: 100, - expected: -10, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := getMapFieldAsInt(tt.m, tt.key, tt.defaultVal) - assert.Equal(t, tt.expected, result) - }) - } -} - // TestDirExists tests the fileutil.DirExists helper function func TestDirExists(t *testing.T) { t.Run("empty path returns false", func(t *testing.T) {