diff --git a/marshaller/unmarshaller.go b/marshaller/unmarshaller.go index 4f78d5a..14dac5e 100644 --- a/marshaller/unmarshaller.go +++ b/marshaller/unmarshaller.go @@ -199,28 +199,28 @@ func unmarshal(ctx context.Context, parentName string, node *yaml.Node, out refl switch { case isStructType(out): // Target expects a struct/object - if err := validateNodeKind(resolvedNode, yaml.MappingNode, parentName); err != nil { + if err := validateNodeKind(resolvedNode, yaml.MappingNode, parentName, "object"); err != nil { return []error{err}, nil //nolint:nilerr } return unmarshalMapping(ctx, parentName, node, out) case isSliceType(out): // Target expects a slice/array - if err := validateNodeKind(resolvedNode, yaml.SequenceNode, parentName); err != nil { + if err := validateNodeKind(resolvedNode, yaml.SequenceNode, parentName, "sequence"); err != nil { return []error{err}, nil //nolint:nilerr } return unmarshalSequence(ctx, parentName, node, out) case isMapType(out): // Target expects a map - if err := validateNodeKind(resolvedNode, yaml.MappingNode, parentName); err != nil { + if err := validateNodeKind(resolvedNode, yaml.MappingNode, parentName, "object"); err != nil { return []error{err}, nil //nolint:nilerr } return unmarshalMapping(ctx, parentName, node, out) default: // Target expects a scalar value (string, int, bool, etc.) - if err := validateNodeKind(resolvedNode, yaml.ScalarNode, parentName); err != nil { + if err := validateNodeKind(resolvedNode, yaml.ScalarNode, parentName, out.Type().String()); err != nil { return []error{err}, nil //nolint:nilerr } return decodeNode(ctx, parentName, resolvedNode, out.Addr().Interface()) @@ -602,16 +602,15 @@ func isMapType(out reflect.Value) bool { } // validateNodeKind checks if the node kind matches the expected kind and returns appropriate error -func validateNodeKind(resolvedNode *yaml.Node, expectedKind yaml.Kind, parentName string) error { +func validateNodeKind(resolvedNode *yaml.Node, expectedKind yaml.Kind, parentName, expectedType string) error { if resolvedNode == nil { return validation.NewValidationError(validation.NewTypeMismatchError("%sexpected %s, got nil", getOptionalParentName(parentName), yml.NodeKindToString(expectedKind)), nil) } if resolvedNode.Kind != expectedKind { - expectedKindStr := yml.NodeKindToString(expectedKind) actualKindStr := yml.NodeKindToString(resolvedNode.Kind) - return validation.NewValidationError(validation.NewTypeMismatchError("%sexpected %s, got %s", getOptionalParentName(parentName), expectedKindStr, actualKindStr), resolvedNode) + return validation.NewValidationError(validation.NewTypeMismatchError("%sexpected %s, got %s", getOptionalParentName(parentName), expectedType, actualKindStr), resolvedNode) } return nil } diff --git a/marshaller/unmarshalling_test.go b/marshaller/unmarshalling_test.go index a9c3654..a88b157 100644 --- a/marshaller/unmarshalling_test.go +++ b/marshaller/unmarshalling_test.go @@ -165,7 +165,7 @@ boolField: true intField: 42 float64Field: 3.14 `, - wantErrs: []string{"[2:14] testPrimitiveModel.stringField expected scalar, got sequence"}, + wantErrs: []string{"[2:14] testPrimitiveModel.stringField expected string, got sequence"}, }, { name: "type mismatch - bool field gets string", diff --git a/openapi/README.md b/openapi/README.md index f23c8f8..04724fc 100644 --- a/openapi/README.md +++ b/openapi/README.md @@ -185,7 +185,9 @@ Shows both automatic validation during unmarshaling and explicit validation. ```go ctx := context.Background() -f, err := os.Open("testdata/invalid.openapi.yaml") +path := "testdata/invalid.openapi.yaml" + +f, err := os.Open(path) if err != nil { panic(err) } diff --git a/openapi/openapi_examples_test.go b/openapi/openapi_examples_test.go index 02fc5d1..062adea 100644 --- a/openapi/openapi_examples_test.go +++ b/openapi/openapi_examples_test.go @@ -187,7 +187,9 @@ func Example_marshalingJSONSchema() { func Example_validating() { ctx := context.Background() - f, err := os.Open("testdata/invalid.openapi.yaml") + path := "testdata/invalid.openapi.yaml" + + f, err := os.Open(path) if err != nil { panic(err) } @@ -214,11 +216,21 @@ func Example_validating() { fmt.Println("Document is valid!") } // Output: Validation error: [3:3] info field version is missing - // Validation error: [18:30] response.content expected object, got scalar - // Validation error: [31:25] schema field properties.name.type value must be one of 'array', 'boolean', 'integer', 'null', 'number', 'object', 'string' - // Validation error: [31:25] schema field properties.name.type got string, want array - // Additional validation error: [31:25] schema field properties.name.type value must be one of 'array', 'boolean', 'integer', 'null', 'number', 'object', 'string' - // Additional validation error: [31:25] schema field properties.name.type got string, want array + // Validation error: [22:15] schema field type value must be one of 'array', 'boolean', 'integer', 'null', 'number', 'object', 'string' + // Validation error: [22:17] schema field type.0 value must be one of 'array', 'boolean', 'integer', 'null', 'number', 'object', 'string' + // Validation error: [28:30] response.content expected object, got scalar + // Validation error: [43:19] schema.properties failed to validate either Schema or bool: [43:19] schema.properties expected object, got sequence + // [43:19] schema.properties expected bool, got sequence + // Validation error: [47:7] components.schemas failed to validate either Schema or bool: [50:15] schema.properties failed to validate either Schema or bool: [50:15] schema.properties expected object, got scalar + // [50:15] schema.properties yaml: unmarshal errors: + // line 50: cannot unmarshal !!str `string` into bool + // [51:18] schema.properties failed to validate either Schema or bool: [51:18] schema.properties expected object, got scalar + // [51:18] schema.properties yaml: unmarshal errors: + // line 51: cannot unmarshal !!str `John Doe` into bool + // [54:9] schema.examples expected sequence, got object + // [47:7] components.schemas expected bool, got object + // Additional validation error: [22:15] schema field type value must be one of 'array', 'boolean', 'integer', 'null', 'number', 'object', 'string' + // Additional validation error: [22:17] schema field type.0 value must be one of 'array', 'boolean', 'integer', 'null', 'number', 'object', 'string' } // Example_mutating demonstrates how to read and modify an OpenAPI document. diff --git a/openapi/testdata/invalid.openapi.yaml b/openapi/testdata/invalid.openapi.yaml index 4cf6afe..97ecc0c 100644 --- a/openapi/testdata/invalid.openapi.yaml +++ b/openapi/testdata/invalid.openapi.yaml @@ -11,6 +11,16 @@ paths: in: path required: true # Missing schema definition + - name: invalidSchema + in: query + schema: + $ref: "#/components/schemas/InvalidSchema" + - name: invalidNullSchema + in: query + schema: + type: + - null + - string responses: "200": description: User found @@ -29,3 +39,17 @@ paths: properties: name: type: invalid_type # Invalid schema type + required: # at wrong level + - name +components: + schemas: + InvalidSchema: + type: object + properties: + # Incorrectly defined properties + name: string + example: "John Doe" + examples: + # non-sequence based examples + Example 1: + syncId: 1ad0695c-4566-4715-918c-adbb03eac81e diff --git a/validation/errors.go b/validation/errors.go index c8846b7..68a630c 100644 --- a/validation/errors.go +++ b/validation/errors.go @@ -144,7 +144,7 @@ var _ error = (*TypeMismatchError)(nil) func NewTypeMismatchError(msg string, args ...any) *TypeMismatchError { if len(args) > 0 { - msg = fmt.Sprintf(msg, args...) + msg = fmt.Errorf(msg, args...).Error() } return &TypeMismatchError{ diff --git a/values/core/eithervalue.go b/values/core/eithervalue.go index 37511bb..685b35e 100644 --- a/values/core/eithervalue.go +++ b/values/core/eithervalue.go @@ -9,6 +9,7 @@ import ( "github.com/speakeasy-api/openapi/internal/interfaces" "github.com/speakeasy-api/openapi/marshaller" + "github.com/speakeasy-api/openapi/validation" "gopkg.in/yaml.v3" ) @@ -33,7 +34,7 @@ func (v *EitherValue[L, R]) Unmarshal(ctx context.Context, parentName string, no // Try Left type without strict mode leftValidationErrs, leftUnmarshalErr = marshaller.UnmarshalCore(ctx, parentName, node, &v.Left) if leftUnmarshalErr == nil && !hasTypeMismatchErrors(leftValidationErrs) { - // No unmarshalling error and no type mismatch validation errors - this is successful + // No unmarshaling error and no type mismatch validation errors - this is successful v.IsLeft = true v.SetRootNode(node) return leftValidationErrs, nil @@ -42,22 +43,35 @@ func (v *EitherValue[L, R]) Unmarshal(ctx context.Context, parentName string, no // Try Right type without strict mode rightValidationErrs, rightUnmarshalErr = marshaller.UnmarshalCore(ctx, parentName, node, &v.Right) if rightUnmarshalErr == nil && !hasTypeMismatchErrors(rightValidationErrs) { - // No unmarshalling error and no type mismatch validation errors - this is successful + // No unmarshaling error and no type mismatch validation errors - this is successful v.IsRight = true v.SetRootNode(node) return rightValidationErrs, nil } - // Both types failed - determine if we should return validation errors or unmarshalling errors + leftType := reflect.TypeOf((*L)(nil)).Elem().Name() + rightType := reflect.TypeOf((*R)(nil)).Elem().Name() + + // Both types failed - determine if we should return validation errors or unmarshaling errors + // Both failed with validation errors only (no real unmarshaling errors) if leftUnmarshalErr == nil && rightUnmarshalErr == nil { - // Both failed with validation errors only (no real unmarshalling errors) // Combine the validation errors and return them instead of an error allValidationErrs := leftValidationErrs allValidationErrs = append(allValidationErrs, rightValidationErrs...) - return allValidationErrs, nil + + msg := fmt.Errorf("%s failed to validate either %s or %s: %w", parentName, leftType, rightType, errors.Join(allValidationErrs...)).Error() + + var validationError error + if hasTypeMismatchErrors(allValidationErrs) { + validationError = validation.NewTypeMismatchError(msg) + } else { + validationError = validation.NewValueValidationError(msg) + } + + return []error{validation.NewValidationError(validationError, node)}, nil } - // At least one had a real unmarshalling error - return as unmarshalling failure + // At least one had a real unmarshaling error - return as unmarshaling failure errs := []error{} if leftUnmarshalErr != nil { errs = append(errs, leftUnmarshalErr) @@ -71,19 +85,27 @@ func (v *EitherValue[L, R]) Unmarshal(ctx context.Context, parentName string, no errs = append(errs, fmt.Errorf("right type validation failed: %v", rightValidationErrs)) } - return nil, fmt.Errorf("unable to marshal into either %s or %s: %w", reflect.TypeOf((*L)(nil)).Elem().Name(), reflect.TypeOf((*R)(nil)).Elem().Name(), errors.Join(errs...)) + return nil, fmt.Errorf("unable to marshal into either %s or %s: %w", leftType, rightType, errors.Join(errs...)) } // hasTypeMismatchErrors checks if the validation errors contain type mismatch errors -// indicating that the type couldn't be unmarshalled successfully +// indicating that the type couldn't be unmarshaled successfully. +// It ignores type mismatch errors from nested either values to avoid cascading failures. func hasTypeMismatchErrors(validationErrs []error) bool { if len(validationErrs) == 0 { return false } for _, err := range validationErrs { - // Check if this is a type mismatch error by looking for common patterns errStr := err.Error() + + // Skip errors from nested either values - these are child validation failures + // that shouldn't cause the parent either value to fail + if strings.Contains(errStr, "failed to validate either") { + continue + } + + // Check if this is a type mismatch error by looking for common patterns if strings.Contains(errStr, "expected") && (strings.Contains(errStr, "got") || strings.Contains(errStr, "but received")) { return true } diff --git a/values/core/eithervalue_test.go b/values/core/eithervalue_test.go index edef854..9c3bc5b 100644 --- a/values/core/eithervalue_test.go +++ b/values/core/eithervalue_test.go @@ -101,14 +101,17 @@ func TestEitherValue_BothTypesFailValidation(t *testing.T) { assert.False(t, target.IsRight, "Should not have set Right as successful") // Validation errors should contain type mismatch information for both types - foundScalarError := false + foundTypeMismatchError := false for _, validationErr := range validationErrs { - if strings.Contains(validationErr.Error(), "expected scalar") { - foundScalarError = true + errStr := validationErr.Error() + // Check for type mismatch patterns like "expected X, got Y" + if (strings.Contains(errStr, "expected string") || strings.Contains(errStr, "expected bool")) && + strings.Contains(errStr, "got sequence") { + foundTypeMismatchError = true break } } - assert.True(t, foundScalarError, "Should contain type mismatch error for scalar types") + assert.True(t, foundTypeMismatchError, "Should contain type mismatch error for string/bool types") } func TestEitherValue_GetNavigableNode_Success(t *testing.T) {