fix: improve openapi join conflict resolution and validation (#45)

Author: TristanSpeakEasy

jsonschema/oas3/resolution.go

@@ -27,6 +27,7 @@ func (s *JSONSchema[Referenceable]) IsResolved() bool {
 	return !s.IsReference() || s.resolvedSchemaCache != nil || (s.referenceResolutionCache != nil && s.referenceResolutionCache.Object != nil) || s.circularErrorFound
 }
 
+// IsReference returns true if the JSONSchema is a reference, false otherwise
 func (j *JSONSchema[Referenceable]) IsReference() bool {
 	if j == nil || j.IsRight() {
 		return false
@@ -35,6 +36,18 @@ func (j *JSONSchema[Referenceable]) IsReference() bool {
 	return j.GetLeft().IsReference()
 }
 
+// GetReference returns the reference of the JSONSchema if present, otherwise an empty string
+// This method is identical to GetRef() but was added to support the Resolvable interface
+func (j *JSONSchema[Referenceable]) GetReference() references.Reference {
+	if j == nil {
+		return ""
+	}
+
+	return j.GetRef()
+}
+
+// GetRef returns the reference of the JSONSchema if present, otherwise an empty string
+// This method is identical to GetReference() but was kept for backwards compatibility
 func (j *JSONSchema[Referenceable]) GetRef() references.Reference {
 	if j == nil || j.IsRight() {
 		return ""
@@ -43,6 +56,7 @@ func (j *JSONSchema[Referenceable]) GetRef() references.Reference {
 	return j.GetLeft().GetRef()
 }
 
+// GetAbsRef returns the absolute reference of the JSONSchema if present, otherwise an empty string
 func (j *JSONSchema[Referenceable]) GetAbsRef() references.Reference {
 	if !j.IsReference() {
 		return ""

jsonschema/oas3/schema_validate_test.go

@@ -415,7 +415,7 @@ additionalProperties: "invalid"
 `,
 			wantErrs: []string{
 				"schema field additionalProperties got string, want boolean or object",
-				"schema expected object, got scalar",
+				"schema.additionalProperties expected object, got scalar",
 			},
 		},
 		{
@@ -442,7 +442,7 @@ items: "invalid"
 `,
 			wantErrs: []string{
 				"schema field items got string, want boolean or object",
-				"schema expected object, got scalar",
+				"schema.items expected object, got scalar",
 			},
 		},
 		{

marshaller/unmarshaller.go

@@ -383,7 +383,7 @@ func unmarshalModel(ctx context.Context, node *yaml.Node, structPtr any) ([]erro
 				fieldVal := out.Field(fieldIndex)
 
 				if implementsInterface(fieldVal, nodeMutatorType) {
-					fieldValidationErrs, err := unmarshalNode(ctx, modelTag, keyNode, valueNode, cachedField.Name, fieldVal)
+					fieldValidationErrs, err := unmarshalNode(ctx, modelTag+"."+key, keyNode, valueNode, cachedField.Name, fieldVal)
 					if err != nil {
 						return err
 					}

marshaller/unmarshalling_test.go

@@ -165,7 +165,7 @@ boolField: true
 intField: 42
 float64Field: 3.14
 `,
-			wantErrs: []string{"[2:14] testPrimitiveModel expected scalar, got sequence"},
+			wantErrs: []string{"[2:14] testPrimitiveModel.stringField expected scalar, got sequence"},
 		},
 		{
 			name: "type mismatch - bool field gets string",
@@ -175,7 +175,7 @@ boolField: "not a bool"
 intField: 42
 float64Field: 3.14
 `,
-			wantErrs: []string{"[3:12] testPrimitiveModel yaml: unmarshal errors:"},
+			wantErrs: []string{"[3:12] testPrimitiveModel.boolField yaml: unmarshal errors:"},
 		},
 		{
 			name: "type mismatch - int field gets string",
@@ -185,7 +185,7 @@ boolField: true
 intField: "not an int"
 float64Field: 3.14
 `,
-			wantErrs: []string{"[4:11] testPrimitiveModel yaml: unmarshal errors:"},
+			wantErrs: []string{"[4:11] testPrimitiveModel.intField yaml: unmarshal errors:"},
 		},
 		{
 			name: "type mismatch - float field gets string",
@@ -195,7 +195,7 @@ boolField: true
 intField: 42
 float64Field: "not a float"
 `,
-			wantErrs: []string{"[5:15] testPrimitiveModel yaml: unmarshal errors:"},
+			wantErrs: []string{"[5:15] testPrimitiveModel.float64Field yaml: unmarshal errors:"},
 		},
 		{
 			name: "multiple validation errors",
@@ -206,7 +206,7 @@ intField: "not an int"
 			wantErrs: []string{
 				"[2:1] testPrimitiveModel field stringField is missing",
 				"[2:1] testPrimitiveModel field float64Field is missing",
-				"[2:12] testPrimitiveModel yaml: unmarshal errors:",
+				"[2:12] testPrimitiveModel.boolField yaml: unmarshal errors:",
 			},
 		},
 	}
@@ -379,7 +379,7 @@ nestedModelValue:
 nestedModel:
   - "this should be an object"
 `,
-			wantErrs: []string{"[8:3] testComplexModel expected object, got sequence"},
+			wantErrs: []string{"[8:3] testComplexModel.nestedModel expected object, got sequence"},
 		},
 		{
 			name: "type mismatch - array field gets object",
@@ -392,7 +392,7 @@ nestedModelValue:
 arrayField:
   key: "this should be an array"
 `,
-			wantErrs: []string{"[8:3] testComplexModel expected sequence, got object"},
+			wantErrs: []string{"[8:3] testComplexModel.arrayField expected sequence, got object"},
 		},
 		{
 			name: "deeply nested validation error",

openapi/join.go

@@ -206,6 +206,12 @@ func joinSingleDocument(ctx context.Context, result, doc *OpenAPI, docPath strin
 		return fmt.Errorf("failed to update references in document: %w", err)
 	}
 
+	// Collect existing operationIds to detect conflicts
+	usedOperationIds := collectOperationIds(result)
+
+	// Resolve operationId conflicts in the source document
+	resolveOperationIdConflicts(doc, usedOperationIds, docPath)
+
 	// Join paths with conflict resolution
 	joinPaths(result, doc, docPath, usedPathNames)
 
@@ -232,19 +238,94 @@ func joinPaths(result, src *OpenAPI, srcPath string, usedPathNames map[string]bo
 		result.Paths = NewPaths()
 	}
 
-	for path, pathItem := range src.Paths.All() {
+	for path, srcPathItem := range src.Paths.All() {
 		if !usedPathNames[path] {
 			// No conflict, add directly
-			result.Paths.Set(path, pathItem)
+			result.Paths.Set(path, srcPathItem)
 			usedPathNames[path] = true
 		} else {
-			// Conflict detected, create new path with fragment
-			newPath := generateConflictPath(path, srcPath)
-			result.Paths.Set(newPath, pathItem)
-			usedPathNames[newPath] = true
+			// Path exists, need to check for operation conflicts
+			existingPathItem, exists := result.Paths.Get(path)
+			if !exists || existingPathItem == nil || existingPathItem.Object == nil || srcPathItem == nil || srcPathItem.Object == nil {
+				// Safety check - if we can't access the operations, create conflict path
+				newPath := generateConflictPath(path, srcPath)
+				result.Paths.Set(newPath, srcPathItem)
+				usedPathNames[newPath] = true
+				continue
+			}
+
+			// Try to merge operations from source into existing path
+			conflictingOperations := mergePathItemOperations(existingPathItem.Object, srcPathItem.Object)
+
+			if len(conflictingOperations) == 0 {
+				// No conflicts, all operations merged successfully - existing path already updated
+				continue
+			} else {
+				// Some operations had conflicts, create new path for conflicting operations only
+				conflictPathItem := createPathItemWithOperations(conflictingOperations)
+
+				newPath := generateConflictPath(path, srcPath)
+				result.Paths.Set(newPath, &ReferencedPathItem{Object: conflictPathItem})
+				usedPathNames[newPath] = true
+			}
+		}
+	}
+}
+
+// ConflictingOperation represents an operation that conflicts with an existing one
+type ConflictingOperation struct {
+	Method    HTTPMethod
+	Operation *Operation
+}
+
+// mergePathItemOperations attempts to merge operations from srcPathItem into existingPathItem
+// Returns conflicting operations that couldn't be merged
+func mergePathItemOperations(existingPathItem, srcPathItem *PathItem) []ConflictingOperation {
+	conflictingOperations := []ConflictingOperation{}
+
+	// Iterate through all operations in the source PathItem
+	for method, srcOp := range srcPathItem.All() {
+		if srcOp == nil {
+			continue
+		}
+
+		// Check if existing PathItem has this method
+		existingOp := existingPathItem.GetOperation(method)
+
+		if existingOp == nil {
+			// No conflict, add the operation to existing PathItem
+			existingPathItem.Set(method, srcOp)
+		} else {
+			// Both have this method, check if operations are identical
+			srcHash := hashing.Hash(srcOp)
+			existingHash := hashing.Hash(existingOp)
+
+			if srcHash == existingHash {
+				// Identical operations, keep existing (deduplicate)
+				continue
+			} else {
+				// Different operations, this is a conflict
+				conflictingOperations = append(conflictingOperations, ConflictingOperation{
+					Method:    method,
+					Operation: srcOp,
+				})
+			}
 		}
 	}
 
+	return conflictingOperations
+}
+
+// createPathItemWithOperations creates a new PathItem containing only the specified conflicting operations
+func createPathItemWithOperations(conflictingOps []ConflictingOperation) *PathItem {
+	pathItem := NewPathItem()
+
+	// Add each conflicting operation with its original method
+	for _, conflictOp := range conflictingOps {
+		pathItem.Set(conflictOp.Method, conflictOp.Operation)
+	}
+
+	return pathItem
 }
 
 // generateConflictPath creates a new path with a fragment containing the file name
@@ -738,6 +819,110 @@ func joinTags(result, src *OpenAPI) {
 	}
 }
 
+// collectOperationIds collects all operationIds from the given OpenAPI document
+func collectOperationIds(doc *OpenAPI) map[string]bool {
+	usedOperationIds := make(map[string]bool)
+
+	if doc.Paths != nil {
+		for _, pathItem := range doc.Paths.All() {
+			if pathItem == nil || pathItem.Object == nil {
+				continue
+			}
+
+			// Check all operations in the path item
+			for _, operation := range pathItem.Object.All() {
+				if operation != nil && operation.OperationID != nil && *operation.OperationID != "" {
+					usedOperationIds[*operation.OperationID] = true
+				}
+			}
+		}
+	}
+
+	// Also check webhooks
+	if doc.Webhooks != nil {
+		for _, webhook := range doc.Webhooks.All() {
+			if webhook == nil || webhook.Object == nil {
+				continue
+			}
+
+			for _, operation := range webhook.Object.All() {
+				if operation != nil && operation.OperationID != nil && *operation.OperationID != "" {
+					usedOperationIds[*operation.OperationID] = true
+				}
+			}
+		}
+	}
+
+	return usedOperationIds
+}
+
+// resolveOperationIdConflicts resolves operationId conflicts by adding #docname suffix
+func resolveOperationIdConflicts(doc *OpenAPI, usedOperationIds map[string]bool, docPath string) {
+	// Extract document name from path for suffix
+	docName := generateDocumentName(docPath)
+
+	if doc.Paths != nil {
+		for _, pathItem := range doc.Paths.All() {
+			if pathItem == nil || pathItem.Object == nil {
+				continue
+			}
+
+			// Check all operations in the path item
+			for _, operation := range pathItem.Object.All() {
+				if operation != nil && operation.OperationID != nil && *operation.OperationID != "" {
+					originalId := *operation.OperationID
+					if usedOperationIds[originalId] {
+						// Conflict detected, add suffix
+						newId := originalId + "#" + docName
+						operation.OperationID = &newId
+					} else {
+						// No conflict, mark as used
+						usedOperationIds[originalId] = true
+					}
+				}
+			}
+		}
+	}
+
+	// Also handle webhooks
+	if doc.Webhooks != nil {
+		for _, webhook := range doc.Webhooks.All() {
+			if webhook == nil || webhook.Object == nil {
+				continue
+			}
+
+			for _, operation := range webhook.Object.All() {
+				if operation != nil && operation.OperationID != nil && *operation.OperationID != "" {
+					originalId := *operation.OperationID
+					if usedOperationIds[originalId] {
+						// Conflict detected, add suffix
+						newId := originalId + "#" + docName
+						operation.OperationID = &newId
+					} else {
+						// No conflict, mark as used
+						usedOperationIds[originalId] = true
+					}
+				}
+			}
+		}
+	}
+}
+
+// generateDocumentName extracts a clean document name from the file path
+func generateDocumentName(filePath string) string {
+	// Extract filename without extension
+	baseName := filepath.Base(filePath)
+	ext := filepath.Ext(baseName)
+	if ext != "" {
+		baseName = baseName[:len(baseName)-len(ext)]
+	}
+
+	// Clean the filename to make it safe
+	safeFileName := regexp.MustCompile(`[^a-zA-Z0-9_-]`).ReplaceAllString(baseName, "_")
+
+	return safeFileName
+}
+
 // joinServersAndSecurity handles smart conflict resolution for servers and security
 func joinServersAndSecurity(result, src *OpenAPI) {
 	// Check if servers are identical (by hash)

openapi/join_test.go

@@ -257,11 +257,19 @@ func TestJoin_NoFilePath_Success(t *testing.T) {
 	err = openapi.Join(ctx, mainDoc, documents, opts)
 	require.NoError(t, err)
 
-	// Verify original /users path exists
+	// Verify original /users path exists and contains both operations
 	assert.True(t, mainDoc.Paths.Has("/users"))
+	usersPath, exists := mainDoc.Paths.Get("/users")
+	require.True(t, exists, "Users path should exist")
+	require.NotNil(t, usersPath)
+	require.NotNil(t, usersPath.Object)
 
-	// Verify conflicting path uses fallback name
-	assert.True(t, mainDoc.Paths.Has("/users#document_0"))
+	// Should have both GET (from main) and POST (from second)
+	assert.NotNil(t, usersPath.Object.Get, "Should have GET operation from main document")
+	assert.NotNil(t, usersPath.Object.Post, "Should have POST operation from second document")
+
+	// Should NOT have a duplicated path since methods are different
+	assert.False(t, mainDoc.Paths.Has("/users#document_0"), "Should not create duplicate path for different methods")
 }
 
 func TestJoin_ServersSecurityConflicts_Success(t *testing.T) {

openapi/openapi_examples_test.go

@@ -214,7 +214,7 @@ func Example_validating() {
 		fmt.Println("Document is valid!")
 	}
 	// Output: Validation error: [3:3] info field version is missing
-	// Validation error: [18:30] response expected object, got scalar
+	// 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'

openapi/responses.go

@@ -105,7 +105,7 @@ func (r *Responses) Validate(ctx context.Context, opts ...validation.Option) []e
 		errs = append(errs, r.Default.Validate(ctx, opts...)...)
 	}
 
-	if r.Len() == 0 {
+	if r.Len() == 0 && r.Default == nil {
 		errs = append(errs, validation.NewValidationError(validation.NewValueValidationError("responses must have at least one response code"), core.RootNode))
 	}