fix: improved unmarshalling and validation errors of eithervalues (#55)

Author: TristanSpeakEasy

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
 }

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",

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)
 }

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.

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

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{

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
 		}

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) {