From 98501bb78c4f139145a7bbe28edc529515e53824 Mon Sep 17 00:00:00 2001 From: Tristan Cartledge Date: Tue, 9 Sep 2025 12:42:14 +1000 Subject: [PATCH 1/2] fix: improve openapi join conflict resolution and validation --- jsonschema/oas3/resolution.go | 14 ++ marshaller/unmarshaller.go | 2 +- openapi/join.go | 197 +++++++++++++++++- openapi/join_test.go | 14 +- openapi/responses.go | 2 +- openapi/responses_validate_test.go | 14 ++ .../join/joined_conflicts_expected.yaml | 1 + .../join/joined_counter_expected.yaml | 38 +++- .../join/joined_filepath_expected.yaml | 38 +++- openapi/testdata/join/main.yaml | 1 + openapi/testdata/join/subdir/second.yaml | 20 +- openapi/testdata/join/third.yaml | 10 +- references/resolution.go | 1 + sequencedmap/map.go | 19 +- 14 files changed, 326 insertions(+), 45 deletions(-) diff --git a/jsonschema/oas3/resolution.go b/jsonschema/oas3/resolution.go index 1273f5b..2232bd2 100644 --- a/jsonschema/oas3/resolution.go +++ b/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 "" diff --git a/marshaller/unmarshaller.go b/marshaller/unmarshaller.go index ea69193..4f78d5a 100644 --- a/marshaller/unmarshaller.go +++ b/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 } diff --git a/openapi/join.go b/openapi/join.go index e6a6abb..425aced 100644 --- a/openapi/join.go +++ b/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) diff --git a/openapi/join_test.go b/openapi/join_test.go index a6a8f1e..d7018af 100644 --- a/openapi/join_test.go +++ b/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) { diff --git a/openapi/responses.go b/openapi/responses.go index 68f4494..1409317 100644 --- a/openapi/responses.go +++ b/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)) } diff --git a/openapi/responses_validate_test.go b/openapi/responses_validate_test.go index aad9646..b666c10 100644 --- a/openapi/responses_validate_test.go +++ b/openapi/responses_validate_test.go @@ -253,6 +253,20 @@ default: "200": description: Success x-test: some-value +`, + }, + { + name: "valid responses with only default", + yml: ` +default: + description: Default response for all status codes + content: + application/json: + schema: + type: object + properties: + message: + type: string `, }, } diff --git a/openapi/testdata/join/joined_conflicts_expected.yaml b/openapi/testdata/join/joined_conflicts_expected.yaml index 6460deb..39af5af 100644 --- a/openapi/testdata/join/joined_conflicts_expected.yaml +++ b/openapi/testdata/join/joined_conflicts_expected.yaml @@ -9,6 +9,7 @@ tags: paths: /users: get: + operationId: getUsers summary: Get users responses: '200': diff --git a/openapi/testdata/join/joined_counter_expected.yaml b/openapi/testdata/join/joined_counter_expected.yaml index bb7bb65..cf4b456 100644 --- a/openapi/testdata/join/joined_counter_expected.yaml +++ b/openapi/testdata/join/joined_counter_expected.yaml @@ -16,6 +16,7 @@ tags: paths: /users: get: + operationId: getUsers summary: Get users responses: '200': @@ -24,6 +25,17 @@ paths: application/json: schema: $ref: '#/components/schemas/User' + post: + operationId: createUser + summary: Create user + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/User_1' + responses: + "201": + description: Created /health: get: summary: Health check @@ -40,19 +52,25 @@ paths: application/json: schema: $ref: '#/components/schemas/Product' - /users#second: - post: - summary: Create user - requestBody: - content: - application/json: - schema: - $ref: '#/components/schemas/User_1' + /health#second: + get: + summary: Different health check + description: A different health check implementation responses: - "201": - description: Created + "200": + description: Different OK response + content: + application/json: + schema: + type: object + properties: + status: + type: string + timestamp: + type: string /orders: get: + operationId: getUsers#third summary: Get orders responses: "200": diff --git a/openapi/testdata/join/joined_filepath_expected.yaml b/openapi/testdata/join/joined_filepath_expected.yaml index 09e0cf6..18b0c74 100644 --- a/openapi/testdata/join/joined_filepath_expected.yaml +++ b/openapi/testdata/join/joined_filepath_expected.yaml @@ -16,6 +16,7 @@ tags: paths: /users: get: + operationId: getUsers summary: Get users responses: '200': @@ -24,6 +25,17 @@ paths: application/json: schema: $ref: '#/components/schemas/User' + post: + operationId: createUser + summary: Create user + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/subdir_second_yaml~User' + responses: + "201": + description: Created /health: get: summary: Health check @@ -40,19 +52,25 @@ paths: application/json: schema: $ref: '#/components/schemas/Product' - /users#second: - post: - summary: Create user - requestBody: - content: - application/json: - schema: - $ref: '#/components/schemas/subdir_second_yaml~User' + /health#second: + get: + summary: Different health check + description: A different health check implementation responses: - "201": - description: Created + "200": + description: Different OK response + content: + application/json: + schema: + type: object + properties: + status: + type: string + timestamp: + type: string /orders: get: + operationId: getUsers#third summary: Get orders responses: "200": diff --git a/openapi/testdata/join/main.yaml b/openapi/testdata/join/main.yaml index 32a6ed8..f8c65f1 100644 --- a/openapi/testdata/join/main.yaml +++ b/openapi/testdata/join/main.yaml @@ -14,6 +14,7 @@ tags: paths: /users: get: + operationId: getUsers summary: Get users responses: '200': diff --git a/openapi/testdata/join/subdir/second.yaml b/openapi/testdata/join/subdir/second.yaml index 807183d..e283291 100644 --- a/openapi/testdata/join/subdir/second.yaml +++ b/openapi/testdata/join/subdir/second.yaml @@ -23,9 +23,10 @@ paths: schema: $ref: '#/components/schemas/Product' /users: - # This conflicts with main.yaml /users path + # This should merge with main.yaml /users path (different method) post: summary: Create user + operationId: createUser requestBody: content: application/json: @@ -34,6 +35,23 @@ paths: responses: '201': description: Created + /health: + # This should conflict with main.yaml /health path (same method, different operation) + get: + summary: Different health check + description: A different health check implementation + responses: + '200': + description: Different OK response + content: + application/json: + schema: + type: object + properties: + status: + type: string + timestamp: + type: string webhooks: productUpdated: post: diff --git a/openapi/testdata/join/third.yaml b/openapi/testdata/join/third.yaml index 5cd9943..67a8ebc 100644 --- a/openapi/testdata/join/third.yaml +++ b/openapi/testdata/join/third.yaml @@ -11,6 +11,7 @@ security: paths: /orders: get: + operationId: getUsers # This conflicts with main.yaml getUsers operationId summary: Get orders responses: '200': @@ -18,7 +19,14 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/Order' + $ref: "#/components/schemas/Order" + /health: + # This should be deduplicated with main.yaml /health path (identical operation) + get: + summary: Health check + responses: + "200": + description: OK components: schemas: Order: diff --git a/references/resolution.go b/references/resolution.go index b74be9a..adccb22 100644 --- a/references/resolution.go +++ b/references/resolution.go @@ -25,6 +25,7 @@ type ResolutionTarget interface { } type Resolvable[T any] interface { + GetReference() Reference Resolve(ctx context.Context, opts ResolveOptions) ([]error, error) GetResolvedObject() *T } diff --git a/sequencedmap/map.go b/sequencedmap/map.go index 37f83bf..c5ea7ad 100644 --- a/sequencedmap/map.go +++ b/sequencedmap/map.go @@ -10,7 +10,7 @@ import ( "iter" "reflect" "slices" - "sort" + "strings" "github.com/speakeasy-api/openapi/internal/interfaces" "github.com/speakeasy-api/openapi/yml" @@ -442,8 +442,8 @@ func (m *Map[K, V]) AllOrdered(order OrderType) iter.Seq2[K, V] { // Sort by key in ascending order sortedElements := make([]*Element[K, V], len(snapshot)) copy(sortedElements, snapshot) - sort.Slice(sortedElements, func(i, j int) bool { - return compareKeys(sortedElements[i].Key, sortedElements[j].Key) < 0 + slices.SortFunc(sortedElements, func(a, b *Element[K, V]) int { + return compareKeys(a.Key, b.Key) }) for _, element := range sortedElements { // Check if element still exists in the map (it might have been deleted during iteration) @@ -458,8 +458,8 @@ func (m *Map[K, V]) AllOrdered(order OrderType) iter.Seq2[K, V] { // Sort by key in descending order sortedElements := make([]*Element[K, V], len(snapshot)) copy(sortedElements, snapshot) - sort.Slice(sortedElements, func(i, j int) bool { - return compareKeys(sortedElements[i].Key, sortedElements[j].Key) > 0 + slices.SortFunc(sortedElements, func(a, b *Element[K, V]) int { + return -compareKeys(a.Key, b.Key) }) for _, element := range sortedElements { // Check if element still exists in the map (it might have been deleted during iteration) @@ -692,17 +692,12 @@ func (m *Map[K, V]) MarshalYAML() (interface{}, error) { } // compareKeys provides a generic comparison function for keys -func compareKeys[K comparable](a, b K) int { +func compareKeys[K any](a, b K) int { // Convert to strings for comparison aStr := fmt.Sprintf("%v", a) bStr := fmt.Sprintf("%v", b) - if aStr < bStr { - return -1 - } else if aStr > bStr { - return 1 - } - return 0 + return strings.Compare(aStr, bStr) } // IsEqual compares two Map instances for equality. From 0e5abfd0c4eca1fe2ddeedaf777b1aaa3c33e78a Mon Sep 17 00:00:00 2001 From: Tristan Cartledge Date: Tue, 9 Sep 2025 13:55:32 +1000 Subject: [PATCH 2/2] test: fix error message expectations after validation improvements --- jsonschema/oas3/schema_validate_test.go | 4 ++-- marshaller/unmarshalling_test.go | 14 +++++++------- openapi/openapi_examples_test.go | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/jsonschema/oas3/schema_validate_test.go b/jsonschema/oas3/schema_validate_test.go index 6d62cab..20bec09 100644 --- a/jsonschema/oas3/schema_validate_test.go +++ b/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", }, }, { diff --git a/marshaller/unmarshalling_test.go b/marshaller/unmarshalling_test.go index cc20a64..a9c3654 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 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", diff --git a/openapi/openapi_examples_test.go b/openapi/openapi_examples_test.go index a151146..02fc5d1 100644 --- a/openapi/openapi_examples_test.go +++ b/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'