From afd586615660a5b42e23ce1263269667705cbde5 Mon Sep 17 00:00:00 2001 From: Jabar Asadi Date: Thu, 19 Mar 2026 19:06:36 +0100 Subject: [PATCH] refactor: surface errors for malformed conf struct tag directives Previously, malformed tag directives were silently ignored: - `conf:"min:abc"` on an int field silently skipped the min constraint - `conf:"required:yes"` silently defaulted to true - `conf:"envv:VAR"` (typo) was silently dropped This change introduces ErrCodeInvalidTag and surfaces these as FieldError values during validation, so developers get clear feedback about misconfigured struct tags instead of silent constraint bypass. Malformed min/max constraints are checked upfront (before early returns) so they are reported even when value validation is skipped. --- binding.go | 39 ++++++++++------- binding_test.go | 46 +++++++++++++------- errors.go | 1 + validate.go | 77 ++++++++++++++++++++++++++++++++++ validate_test.go | 106 +++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 239 insertions(+), 30 deletions(-) diff --git a/binding.go b/binding.go index 1a1c9c7..4ff8d79 100644 --- a/binding.go +++ b/binding.go @@ -14,27 +14,33 @@ import ( // tagConfig holds parsed directives from a struct field's `conf` tag. type tagConfig struct { - env string // Environment variable name (env:VAR_NAME) - name string // Custom key path (name:custom.path) - prefix string // Prefix for nested structs (prefix:foo) - defValue string // Default value (default:value) - min string // Minimum constraint (min:N) - max string // Maximum constraint (max:M) - oneof []string // Allowed values (oneof:a,b,c) - required bool // Field is required (required or required:true) - secret bool // Field is secret (secret or secret:true) - hasDefault bool // Whether a default directive was present + env string // Environment variable name (env:VAR_NAME) + name string // Custom key path (name:custom.path) + prefix string // Prefix for nested structs (prefix:foo) + defValue string // Default value (default:value) + min string // Minimum constraint (min:N) + max string // Maximum constraint (max:M) + oneof []string // Allowed values (oneof:a,b,c) + required bool // Field is required (required or required:true) + secret bool // Field is secret (secret or secret:true) + hasDefault bool // Whether a default directive was present + parseErrors []string // Errors from malformed tag directives } var tagConfigCache sync.Map var optionalTypePkgPath = reflect.TypeOf(Optional[int]{}).PkgPath() func cloneTagConfig(cfg tagConfig) tagConfig { - if len(cfg.oneof) == 0 { + if len(cfg.oneof) == 0 && len(cfg.parseErrors) == 0 { return cfg } - cfg.oneof = append([]string(nil), cfg.oneof...) + if len(cfg.oneof) > 0 { + cfg.oneof = append([]string(nil), cfg.oneof...) + } + if len(cfg.parseErrors) > 0 { + cfg.parseErrors = append([]string(nil), cfg.parseErrors...) + } return cfg } @@ -111,8 +117,9 @@ func parseTag(tag string) tagConfig { } else if value == "false" { cfg.required = false } else { - // Invalid value, default to true for safety cfg.required = true + cfg.parseErrors = append(cfg.parseErrors, + fmt.Sprintf("invalid required value %q: use true or false", value)) } case "secret": // No value or explicit "true" means true @@ -121,9 +128,13 @@ func parseTag(tag string) tagConfig { } else if value == "false" { cfg.secret = false } else { - // Invalid value, default to true for safety cfg.secret = true + cfg.parseErrors = append(cfg.parseErrors, + fmt.Sprintf("invalid secret value %q: use true or false", value)) } + default: + cfg.parseErrors = append(cfg.parseErrors, + fmt.Sprintf("unknown directive %q", name)) } } diff --git a/binding_test.go b/binding_test.go index dc9606d..0936a09 100644 --- a/binding_test.go +++ b/binding_test.go @@ -116,8 +116,9 @@ func TestBinding_ParseTag(t *testing.T) { name: "default with comma terminates directive", tag: "default:a,b,c", expected: tagConfig{ - defValue: "a", - hasDefault: true, + defValue: "a", + hasDefault: true, + parseErrors: []string{`unknown directive "b"`, `unknown directive "c"`}, }, }, { @@ -364,14 +365,16 @@ func TestBinding_ParseTag(t *testing.T) { name: "required with invalid value defaults to true", tag: "required:invalid", expected: tagConfig{ - required: true, + required: true, + parseErrors: []string{`invalid required value "invalid": use true or false`}, }, }, { name: "required with numeric value defaults to true", tag: "required:1", expected: tagConfig{ - required: true, + required: true, + parseErrors: []string{`invalid required value "1": use true or false`}, }, }, { @@ -399,14 +402,16 @@ func TestBinding_ParseTag(t *testing.T) { name: "secret with invalid value defaults to true", tag: "secret:invalid", expected: tagConfig{ - secret: true, + secret: true, + parseErrors: []string{`invalid secret value "invalid": use true or false`}, }, }, { name: "secret with yes defaults to true", tag: "secret:yes", expected: tagConfig{ - secret: true, + secret: true, + parseErrors: []string{`invalid secret value "yes": use true or false`}, }, }, { @@ -574,29 +579,35 @@ func TestBinding_ParseTag(t *testing.T) { // Unknown directives { - name: "unknown directive ignored", + name: "unknown directive reported", tag: "unknown:value,env:VAR", expected: tagConfig{ - env: "VAR", + env: "VAR", + parseErrors: []string{`unknown directive "unknown"`}, }, }, { name: "multiple unknown directives", tag: "foo:bar,env:VAR,baz:qux,required", expected: tagConfig{ - env: "VAR", - required: true, + env: "VAR", + required: true, + parseErrors: []string{`unknown directive "foo"`, `unknown directive "baz"`}, }, }, { - name: "only unknown directives", - tag: "unknown:value,another:thing", - expected: tagConfig{}, + name: "only unknown directives", + tag: "unknown:value,another:thing", + expected: tagConfig{ + parseErrors: []string{`unknown directive "unknown"`, `unknown directive "another"`}, + }, }, { - name: "typo in directive name", - tag: "envv:VAR,requiired:true", // intentional typos to test silent ignore - expected: tagConfig{}, + name: "typo in directive name", + tag: "envv:VAR,requiired:true", + expected: tagConfig{ + parseErrors: []string{`unknown directive "envv"`, `unknown directive "requiired"`}, + }, }, // Edge cases @@ -655,6 +666,9 @@ func TestBinding_ParseTag(t *testing.T) { if result.secret != tt.expected.secret { t.Errorf("secret: got %v, want %v", result.secret, tt.expected.secret) } + if !reflect.DeepEqual(result.parseErrors, tt.expected.parseErrors) { + t.Errorf("parseErrors: got %v, want %v", result.parseErrors, tt.expected.parseErrors) + } }) } } diff --git a/errors.go b/errors.go index f5c4af9..d7114c7 100644 --- a/errors.go +++ b/errors.go @@ -13,6 +13,7 @@ const ( ErrCodeOneOf = "oneof" // Value is not in the allowed set ErrCodeInvalidType = "invalid_type" // Type conversion failed ErrCodeUnknownKey = "unknown_key" // Configuration key doesn't map to any field (strict mode) + ErrCodeInvalidTag = "invalid_tag" // Struct tag directive is malformed or unrecognized ) // ValidationError aggregates field-level validation failures. diff --git a/validate.go b/validate.go index 3a57839..468890d 100644 --- a/validate.go +++ b/validate.go @@ -20,6 +20,19 @@ func validateFieldWithPresence(fieldValue reflect.Value, fieldPath string, tags var errors []FieldError fieldPresent := presentFields != nil && presentFields[fieldPath] + // Surface tag parse errors (malformed directives, unknown directives, etc.) + for _, msg := range tags.parseErrors { + errors = append(errors, FieldError{ + FieldPath: fieldPath, + Code: ErrCodeInvalidTag, + Message: msg, + }) + } + + // Validate min/max tag parseability upfront so malformed constraints + // are reported even when value validation is skipped (e.g., zero/missing values). + errors = append(errors, validateMinMaxParseable(fieldValue, fieldPath, tags)...) + // Check required constraint if tags.required { if presentFields != nil { @@ -187,6 +200,70 @@ func isZeroValue(v reflect.Value) bool { } } +// validateMinMaxParseable checks that min/max tag values can be parsed for the given field kind. +// Called before early returns so malformed constraints are always reported. +func validateMinMaxParseable(fieldValue reflect.Value, fieldPath string, tags tagConfig) []FieldError { + var errors []FieldError + + kind := fieldValue.Kind() + switch kind { + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + if tags.min != "" { + if _, err := strconv.ParseInt(tags.min, 10, 64); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid min constraint %q: must be a valid integer", tags.min)}) + } + } + if tags.max != "" { + if _, err := strconv.ParseInt(tags.max, 10, 64); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid max constraint %q: must be a valid integer", tags.max)}) + } + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + if tags.min != "" { + if _, err := strconv.ParseUint(tags.min, 10, 64); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid min constraint %q: must be a valid unsigned integer", tags.min)}) + } + } + if tags.max != "" { + if _, err := strconv.ParseUint(tags.max, 10, 64); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid max constraint %q: must be a valid unsigned integer", tags.max)}) + } + } + case reflect.Float32, reflect.Float64: + if tags.min != "" { + if _, err := strconv.ParseFloat(tags.min, 64); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid min constraint %q: must be a valid number", tags.min)}) + } + } + if tags.max != "" { + if _, err := strconv.ParseFloat(tags.max, 64); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid max constraint %q: must be a valid number", tags.max)}) + } + } + case reflect.String: + if tags.min != "" { + if _, err := strconv.Atoi(tags.min); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid min constraint %q: must be a valid integer", tags.min)}) + } + } + if tags.max != "" { + if _, err := strconv.Atoi(tags.max); err != nil { + errors = append(errors, FieldError{FieldPath: fieldPath, Code: ErrCodeInvalidTag, + Message: fmt.Sprintf("invalid max constraint %q: must be a valid integer", tags.max)}) + } + } + } + + return errors +} + // validateIntMinMax validates min/max constraints for signed integer types. func validateIntMinMax(fieldValue reflect.Value, fieldPath string, tags tagConfig) []FieldError { var errors []FieldError diff --git a/validate_test.go b/validate_test.go index 258e291..b096285 100644 --- a/validate_test.go +++ b/validate_test.go @@ -491,3 +491,109 @@ func TestIsZeroValue(t *testing.T) { }) } } + +func TestValidateField_InvalidMinMax(t *testing.T) { + tests := []struct { + name string + value any + tags tagConfig + }{ + { + name: "int with non-numeric min", + value: 5, + tags: tagConfig{min: "abc"}, + }, + { + name: "int with non-numeric max", + value: 5, + tags: tagConfig{max: "xyz"}, + }, + { + name: "uint with non-numeric min", + value: uint(5), + tags: tagConfig{min: "abc"}, + }, + { + name: "uint with negative min", + value: uint(5), + tags: tagConfig{min: "-1"}, + }, + { + name: "float with non-numeric min", + value: 5.0, + tags: tagConfig{min: "abc"}, + }, + { + name: "float with non-numeric max", + value: 5.0, + tags: tagConfig{max: "abc"}, + }, + { + name: "string with non-numeric min length", + value: "hello", + tags: tagConfig{min: "abc"}, + }, + { + name: "string with non-numeric max length", + value: "hello", + tags: tagConfig{max: "abc"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fieldValue := reflect.ValueOf(tt.value) + errors := validateField(fieldValue, "TestField", tt.tags) + + if len(errors) == 0 { + t.Fatal("expected invalid_tag error, got none") + } + if errors[0].Code != ErrCodeInvalidTag { + t.Errorf("expected error code %q, got %q", ErrCodeInvalidTag, errors[0].Code) + } + }) + } +} + +func TestValidateField_ParseErrors(t *testing.T) { + tags := tagConfig{ + parseErrors: []string{`unknown directive "foo"`}, + } + fieldValue := reflect.ValueOf("hello") + errors := validateField(fieldValue, "TestField", tags) + + if len(errors) != 1 { + t.Fatalf("expected 1 error, got %d: %v", len(errors), errors) + } + if errors[0].Code != ErrCodeInvalidTag { + t.Errorf("expected error code %q, got %q", ErrCodeInvalidTag, errors[0].Code) + } + if errors[0].Message != `unknown directive "foo"` { + t.Errorf("unexpected message: %s", errors[0].Message) + } +} + +func TestValidateField_InvalidMinMax_EarlyReturnPath(t *testing.T) { + // Malformed min/max must be reported even when value validation is skipped + // (e.g., required field with zero value triggers early return). + tags := tagConfig{required: true, min: "abc"} + fieldValue := reflect.ValueOf(0) + errors := validateFieldWithPresence(fieldValue, "TestField", tags, nil) + + hasRequired := false + hasInvalidTag := false + for _, e := range errors { + if e.Code == ErrCodeRequired { + hasRequired = true + } + if e.Code == ErrCodeInvalidTag { + hasInvalidTag = true + } + } + if !hasRequired { + t.Error("expected required error") + } + if !hasInvalidTag { + t.Error("expected invalid_tag error for malformed min, even on early return") + } +}