From 2f091dd703685def0bfedbe02eb4041e28529e5f Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 12:14:36 -0300 Subject: [PATCH 01/21] CLOUDP-271223: Skip conflict when x-xgen-soa-migration is set --- tools/cli/internal/openapi/oasdiff.go | 82 +++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 5 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 01c844221c..9a263da255 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -22,6 +22,7 @@ import ( "github.com/getkin/kin-openapi/openapi3" "github.com/mongodb/openapi/tools/cli/internal/openapi/errors" "github.com/tufin/oasdiff/diff" + "github.com/tufin/oasdiff/flatten/allof" "github.com/tufin/oasdiff/load" ) @@ -111,20 +112,91 @@ func (o OasDiff) mergePaths() error { return nil } - for k, v := range pathsToMerge.Map() { - if ok := basePaths.Value(k); ok == nil { - basePaths.Set(k, removeExternalRefs(v)) + for externalPath, externalPathData := range pathsToMerge.Map() { + // Tries to find if the path already exists or not + if originalPath := basePaths.Value(externalPath); originalPath == nil { + basePaths.Set(externalPath, removeExternalRefs(externalPathData)) } else { + if shouldSkipConflict(originalPath, externalPathData, externalPath) { + log.Println("Skipping conflict for path: ", externalPath) + continue + } return errors.PathConflictError{ - Entry: k, + Entry: externalPath, } } } - o.base.Spec.Paths = basePaths return nil } +// shouldSkipConflict checks if the conflict should be skipped. +// The method goes through each path operation, and validates if it exists for +// both paths, if it does not, the path conflict should not be ignored. +// If it does, then we check if there is an x-xgen-soa-migration annotation +// If it does, then we allow the conflict to be skipped. +func shouldSkipConflict(basePath, externalPath *openapi3.PathItem, basePathName string) bool { + if basePath.Get != nil && externalPath.Get == nil { + return false + } + + if basePath.Put != nil && externalPath.Put == nil { + return false + } + + if basePath.Post != nil && externalPath.Post == nil { + return false + } + + if basePath.Patch != nil && externalPath.Patch == nil { + return false + } + + if basePath.Delete != nil && externalPath.Delete == nil { + return false + } + + // now check if there is an x-xgen-soa-migration annotation in any of the operations, but if any of the operations + // doesn't have, then we should not skip the conflict + return allMethodsHaveExtension(basePath, basePathName) +} + +// allMethodsHaveExtension checks if all the methods in a path have the x-xgen-soa-migration extension. +func allMethodsHaveExtension(basePath *openapi3.PathItem, basePathName string) bool { + if basePath.Get != nil { + if basePath.Get.Extensions == nil || basePath.Get.Extensions["x-xgen-soa-migration"] == nil { + return false + } + } + + if basePath.Put != nil { + if basePath.Put.Extensions == nil || basePath.Put.Extensions["x-xgen-soa-migration"] == nil { + return false + } + } + + if basePath.Post != nil { + if basePath.Post.Extensions == nil || basePath.Post.Extensions["x-xgen-soa-migration"] == nil { + return false + } + } + + if basePath.Patch != nil { + if basePath.Patch.Extensions == nil || basePath.Patch.Extensions["x-xgen-soa-migration"] == nil { + return false + } + } + + if basePath.Delete != nil { + if basePath.Delete.Extensions == nil || basePath.Delete.Extensions["x-xgen-soa-migration"] == nil { + return false + } + } + + log.Println("Detected x-xgen-soa-migration annotation in all operations for path: ", basePathName) + return true +} + // removeExternalRefs updates the external references of OASes to remove the reference to openapi-mms.json. // Example of an external ref is "$ref": "openapi-mms.json#/components/responses/internalServerError" // Example of an external ref after removeExternalRefs: "$ref": "#/components/responses/internalServerError" From 04c2eb84cf7f97ab8af029c6d48211d8cd33f20a Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 12:16:41 -0300 Subject: [PATCH 02/21] Use const --- tools/cli/internal/openapi/oasdiff.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 9a263da255..e5590bcd6e 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -41,6 +41,10 @@ type OasDiffResult struct { SpecInfoPair *load.SpecInfoPair } +const ( + xgenSoaMigration = "x-xgen-soa-migration" +) + func (o OasDiff) NewDiffResult() (*OasDiffResult, error) { flattenBaseSpec, err := allof.MergeSpec(o.base.Spec) if err != nil { @@ -117,7 +121,7 @@ func (o OasDiff) mergePaths() error { if originalPath := basePaths.Value(externalPath); originalPath == nil { basePaths.Set(externalPath, removeExternalRefs(externalPathData)) } else { - if shouldSkipConflict(originalPath, externalPathData, externalPath) { + if shouldSkipPathConflict(originalPath, externalPathData, externalPath) { log.Println("Skipping conflict for path: ", externalPath) continue } @@ -135,7 +139,7 @@ func (o OasDiff) mergePaths() error { // both paths, if it does not, the path conflict should not be ignored. // If it does, then we check if there is an x-xgen-soa-migration annotation // If it does, then we allow the conflict to be skipped. -func shouldSkipConflict(basePath, externalPath *openapi3.PathItem, basePathName string) bool { +func shouldSkipPathConflict(basePath, externalPath *openapi3.PathItem, basePathName string) bool { if basePath.Get != nil && externalPath.Get == nil { return false } @@ -164,31 +168,31 @@ func shouldSkipConflict(basePath, externalPath *openapi3.PathItem, basePathName // allMethodsHaveExtension checks if all the methods in a path have the x-xgen-soa-migration extension. func allMethodsHaveExtension(basePath *openapi3.PathItem, basePathName string) bool { if basePath.Get != nil { - if basePath.Get.Extensions == nil || basePath.Get.Extensions["x-xgen-soa-migration"] == nil { + if basePath.Get.Extensions == nil || basePath.Get.Extensions[xgenSoaMigration] == nil { return false } } if basePath.Put != nil { - if basePath.Put.Extensions == nil || basePath.Put.Extensions["x-xgen-soa-migration"] == nil { + if basePath.Put.Extensions == nil || basePath.Put.Extensions[xgenSoaMigration] == nil { return false } } if basePath.Post != nil { - if basePath.Post.Extensions == nil || basePath.Post.Extensions["x-xgen-soa-migration"] == nil { + if basePath.Post.Extensions == nil || basePath.Post.Extensions[xgenSoaMigration] == nil { return false } } if basePath.Patch != nil { - if basePath.Patch.Extensions == nil || basePath.Patch.Extensions["x-xgen-soa-migration"] == nil { + if basePath.Patch.Extensions == nil || basePath.Patch.Extensions[xgenSoaMigration] == nil { return false } } if basePath.Delete != nil { - if basePath.Delete.Extensions == nil || basePath.Delete.Extensions["x-xgen-soa-migration"] == nil { + if basePath.Delete.Extensions == nil || basePath.Delete.Extensions[xgenSoaMigration] == nil { return false } } From 5c1b255b165d6185649f8f6a077c1de7327e0fb4 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 14:10:23 -0300 Subject: [PATCH 03/21] Update --- tools/cli/internal/openapi/oasdiff.go | 100 +++++++------------------- tools/cli/internal/openapi/paths.go | 92 ++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 73 deletions(-) create mode 100644 tools/cli/internal/openapi/paths.go diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index e5590bcd6e..8cc9ea82a4 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -15,6 +15,7 @@ package openapi import ( + "encoding/json" "log" "slices" "strings" @@ -41,10 +42,6 @@ type OasDiffResult struct { SpecInfoPair *load.SpecInfoPair } -const ( - xgenSoaMigration = "x-xgen-soa-migration" -) - func (o OasDiff) NewDiffResult() (*OasDiffResult, error) { flattenBaseSpec, err := allof.MergeSpec(o.base.Spec) if err != nil { @@ -122,9 +119,15 @@ func (o OasDiff) mergePaths() error { basePaths.Set(externalPath, removeExternalRefs(externalPathData)) } else { if shouldSkipPathConflict(originalPath, externalPathData, externalPath) { - log.Println("Skipping conflict for path: ", externalPath) - continue + log.Printf("Skipping conflict for path: %s", externalPath) + if o.arePathsIdenticalWithExcludeExtensions(externalPath) { + log.Printf("No doc diff detected for path %s, merging the paths", externalPath) + basePaths.Set(externalPath, removeExternalRefs(externalPathData)) + } else { + log.Printf("Doc diff detected failing as allowDocsDiff=true is not supported.") + } } + return errors.PathConflictError{ Entry: externalPath, } @@ -134,73 +137,6 @@ func (o OasDiff) mergePaths() error { return nil } -// shouldSkipConflict checks if the conflict should be skipped. -// The method goes through each path operation, and validates if it exists for -// both paths, if it does not, the path conflict should not be ignored. -// If it does, then we check if there is an x-xgen-soa-migration annotation -// If it does, then we allow the conflict to be skipped. -func shouldSkipPathConflict(basePath, externalPath *openapi3.PathItem, basePathName string) bool { - if basePath.Get != nil && externalPath.Get == nil { - return false - } - - if basePath.Put != nil && externalPath.Put == nil { - return false - } - - if basePath.Post != nil && externalPath.Post == nil { - return false - } - - if basePath.Patch != nil && externalPath.Patch == nil { - return false - } - - if basePath.Delete != nil && externalPath.Delete == nil { - return false - } - - // now check if there is an x-xgen-soa-migration annotation in any of the operations, but if any of the operations - // doesn't have, then we should not skip the conflict - return allMethodsHaveExtension(basePath, basePathName) -} - -// allMethodsHaveExtension checks if all the methods in a path have the x-xgen-soa-migration extension. -func allMethodsHaveExtension(basePath *openapi3.PathItem, basePathName string) bool { - if basePath.Get != nil { - if basePath.Get.Extensions == nil || basePath.Get.Extensions[xgenSoaMigration] == nil { - return false - } - } - - if basePath.Put != nil { - if basePath.Put.Extensions == nil || basePath.Put.Extensions[xgenSoaMigration] == nil { - return false - } - } - - if basePath.Post != nil { - if basePath.Post.Extensions == nil || basePath.Post.Extensions[xgenSoaMigration] == nil { - return false - } - } - - if basePath.Patch != nil { - if basePath.Patch.Extensions == nil || basePath.Patch.Extensions[xgenSoaMigration] == nil { - return false - } - } - - if basePath.Delete != nil { - if basePath.Delete.Extensions == nil || basePath.Delete.Extensions[xgenSoaMigration] == nil { - return false - } - } - - log.Println("Detected x-xgen-soa-migration annotation in all operations for path: ", basePathName) - return true -} - // removeExternalRefs updates the external references of OASes to remove the reference to openapi-mms.json. // Example of an external ref is "$ref": "openapi-mms.json#/components/responses/internalServerError" // Example of an external ref after removeExternalRefs: "$ref": "#/components/responses/internalServerError" @@ -493,6 +429,24 @@ func (o OasDiff) areSchemaIdentical(name string) bool { return !ok } +// arePathsIdenticalWithExcludeExtensions checks if the paths are identical with the extensions excluded +func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) bool { + // If the diff only has extensions diff, then we consider the paths to be identical + exclude := []string{"extensions"} + customConfig := diff.NewConfig().WithExcludeElements(exclude) + d, err := diff.Get(customConfig, o.base.Spec, o.external.Spec) + if err != nil { + log.Fatalf("error in calculating the diff of the specs: %s", err) + } + + _, ok := d.PathsDiff.Modified[name] + if ok { + j, _ := json.MarshalIndent(d.PathsDiff.Modified[name], "", " ") + log.Printf("arePathsIdenticalWithExcludeExtensions diff: %s", j) + } + return !ok +} + type ByName []*openapi3.Tag func (a ByName) Len() int { return len(a) } diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go new file mode 100644 index 0000000000..58f95aebae --- /dev/null +++ b/tools/cli/internal/openapi/paths.go @@ -0,0 +1,92 @@ +// Copyright 2024 MongoDB Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package openapi + +import ( + "log" + + "github.com/getkin/kin-openapi/openapi3" +) + +const ( + xgenSoaMigration = "x-xgen-soa-migration" +) + +// shouldSkipConflict checks if the conflict should be skipped. +// The method goes through each path operation and performs the following checks: +// 1. Validates if both paths have same operations, if not, then it returns false. +// 2. If both paths have the same operations, then it checks if there is an x-xgen-soa-migration annotation. +// If there is no annotation, then it returns false. +func shouldSkipPathConflict(basePath, externalPath *openapi3.PathItem, basePathName string) bool { + if basePath.Get != nil && externalPath.Get == nil || basePath.Get == nil && externalPath.Get != nil { + return false + } + + if basePath.Put != nil && externalPath.Put == nil || basePath.Put == nil && externalPath.Put != nil { + return false + } + + if basePath.Post != nil && externalPath.Post == nil || basePath.Post == nil && externalPath.Post != nil { + return false + } + + if basePath.Patch != nil && externalPath.Patch == nil || basePath.Patch == nil && externalPath.Patch != nil { + return false + } + + if basePath.Delete != nil && externalPath.Delete == nil || basePath.Delete == nil && externalPath.Delete != nil { + return false + } + + // now check if there is an x-xgen-soa-migration annotation in any of the operations, but if any of the operations + // doesn't have, then we should not skip the conflict + return allOperationsHaveExtension(basePath, basePathName, xgenSoaMigration) +} + +// allMethodsHaveExtension checks if all the operations in the base pat have the given extension name. +func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, extensionName string) bool { + if basePath.Get != nil { + if basePath.Get.Extensions == nil || basePath.Get.Extensions[extensionName] == nil { + return false + } + } + + if basePath.Put != nil { + if basePath.Put.Extensions == nil || basePath.Put.Extensions[extensionName] == nil { + return false + } + } + + if basePath.Post != nil { + if basePath.Post.Extensions == nil || basePath.Post.Extensions[extensionName] == nil { + return false + } + } + + if basePath.Patch != nil { + if basePath.Patch.Extensions == nil || basePath.Patch.Extensions[extensionName] == nil { + return false + } + } + + if basePath.Delete != nil { + if basePath.Delete.Extensions == nil || basePath.Delete.Extensions[extensionName] == nil { + return false + } + } + + log.Println("Detected x-xgen-soa-migration annotation in all operations for path: ", basePathName) + return true +} From b69e8a152ae4e25008f701f0291f626404630327 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 14:19:12 -0300 Subject: [PATCH 04/21] Simplify --- tools/cli/internal/openapi/oasdiff.go | 21 +++++++++++++++++- tools/cli/internal/openapi/paths.go | 31 --------------------------- 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 8cc9ea82a4..aec5d2bab3 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -118,7 +118,7 @@ func (o OasDiff) mergePaths() error { if originalPath := basePaths.Value(externalPath); originalPath == nil { basePaths.Set(externalPath, removeExternalRefs(externalPathData)) } else { - if shouldSkipPathConflict(originalPath, externalPathData, externalPath) { + if o.shouldSkipPathConflict(originalPath, externalPath) { log.Printf("Skipping conflict for path: %s", externalPath) if o.arePathsIdenticalWithExcludeExtensions(externalPath) { log.Printf("No doc diff detected for path %s, merging the paths", externalPath) @@ -172,6 +172,25 @@ func removeExternalRefs(path *openapi3.PathItem) *openapi3.PathItem { return path } +// shouldSkipConflict checks if the conflict should be skipped. +// The method goes through each path operation and performs the following checks: +// 1. Validates if both paths have same operations, if not, then it returns false. +// 2. If both paths have the same operations, then it checks if there is an x-xgen-soa-migration annotation. +// If there is no annotation, then it returns false. +func (o OasDiff) shouldSkipPathConflict(basePath *openapi3.PathItem, basePathName string) bool { + if ok := o.specDiff.PathsDiff.Modified[basePathName].OperationsDiff.Added; !ok.Empty() { + return false + } + + if ok := o.specDiff.PathsDiff.Modified[basePathName].OperationsDiff.Deleted; !ok.Empty() { + return false + } + + // now check if there is an x-xgen-soa-migration annotation in any of the operations, but if any of the operations + // doesn't have, then we should not skip the conflict + return allOperationsHaveExtension(basePath, basePathName, xgenSoaMigration) +} + // updateExternalRefResponses updates the external references of OASes to remove the reference to openapi-mms.json // in the Responses. // A Response can have an external ref in Response.Ref or in its content (Response.Content.Schema.Ref) diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index 58f95aebae..1222c0b366 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -24,37 +24,6 @@ const ( xgenSoaMigration = "x-xgen-soa-migration" ) -// shouldSkipConflict checks if the conflict should be skipped. -// The method goes through each path operation and performs the following checks: -// 1. Validates if both paths have same operations, if not, then it returns false. -// 2. If both paths have the same operations, then it checks if there is an x-xgen-soa-migration annotation. -// If there is no annotation, then it returns false. -func shouldSkipPathConflict(basePath, externalPath *openapi3.PathItem, basePathName string) bool { - if basePath.Get != nil && externalPath.Get == nil || basePath.Get == nil && externalPath.Get != nil { - return false - } - - if basePath.Put != nil && externalPath.Put == nil || basePath.Put == nil && externalPath.Put != nil { - return false - } - - if basePath.Post != nil && externalPath.Post == nil || basePath.Post == nil && externalPath.Post != nil { - return false - } - - if basePath.Patch != nil && externalPath.Patch == nil || basePath.Patch == nil && externalPath.Patch != nil { - return false - } - - if basePath.Delete != nil && externalPath.Delete == nil || basePath.Delete == nil && externalPath.Delete != nil { - return false - } - - // now check if there is an x-xgen-soa-migration annotation in any of the operations, but if any of the operations - // doesn't have, then we should not skip the conflict - return allOperationsHaveExtension(basePath, basePathName, xgenSoaMigration) -} - // allMethodsHaveExtension checks if all the operations in the base pat have the given extension name. func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, extensionName string) bool { if basePath.Get != nil { From 9df5a395bbb60842e2a5480e4c2af590844fbc49 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 15:56:57 -0300 Subject: [PATCH 05/21] Fix tests --- tools/cli/internal/openapi/oasdiff.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index aec5d2bab3..d4cbb4745c 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -178,12 +178,19 @@ func removeExternalRefs(path *openapi3.PathItem) *openapi3.PathItem { // 2. If both paths have the same operations, then it checks if there is an x-xgen-soa-migration annotation. // If there is no annotation, then it returns false. func (o OasDiff) shouldSkipPathConflict(basePath *openapi3.PathItem, basePathName string) bool { - if ok := o.specDiff.PathsDiff.Modified[basePathName].OperationsDiff.Added; !ok.Empty() { - return false + var pathsDiff *diff.PathsDiff + if o.specDiff != nil { + pathsDiff = o.specDiff.PathsDiff } - if ok := o.specDiff.PathsDiff.Modified[basePathName].OperationsDiff.Deleted; !ok.Empty() { - return false + if pathsDiff != nil && pathsDiff.Modified != nil && pathsDiff.Modified[basePathName] != nil { + if ok := o.specDiff.PathsDiff.Modified[basePathName].OperationsDiff.Added; !ok.Empty() { + return false + } + + if ok := o.specDiff.PathsDiff.Modified[basePathName].OperationsDiff.Deleted; !ok.Empty() { + return false + } } // now check if there is an x-xgen-soa-migration annotation in any of the operations, but if any of the operations @@ -458,6 +465,9 @@ func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) bool { log.Fatalf("error in calculating the diff of the specs: %s", err) } + if d.Empty() || d.PathsDiff.Empty() { + return true + } _, ok := d.PathsDiff.Modified[name] if ok { j, _ := json.MarshalIndent(d.PathsDiff.Modified[name], "", " ") From ef3b6e64e93523f5ef674d17747afed094e4bfa4 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 16:08:29 -0300 Subject: [PATCH 06/21] Simplify --- tools/cli/internal/openapi/oasdiff.go | 35 ++++++++++++++++++--------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index d4cbb4745c..905b3480b5 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -118,19 +118,12 @@ func (o OasDiff) mergePaths() error { if originalPath := basePaths.Value(externalPath); originalPath == nil { basePaths.Set(externalPath, removeExternalRefs(externalPathData)) } else { - if o.shouldSkipPathConflict(originalPath, externalPath) { - log.Printf("Skipping conflict for path: %s", externalPath) - if o.arePathsIdenticalWithExcludeExtensions(externalPath) { - log.Printf("No doc diff detected for path %s, merging the paths", externalPath) - basePaths.Set(externalPath, removeExternalRefs(externalPathData)) - } else { - log.Printf("Doc diff detected failing as allowDocsDiff=true is not supported.") - } + err := o.handlePathConflict(originalPath, externalPath) + if err != nil { + return err } - return errors.PathConflictError{ - Entry: externalPath, - } + basePaths.Set(externalPath, removeExternalRefs(externalPathData)) } } o.base.Spec.Paths = basePaths @@ -172,6 +165,26 @@ func removeExternalRefs(path *openapi3.PathItem) *openapi3.PathItem { return path } +// handlePathConflict handles the path conflict by checking if the conflict should be skipped or not. +func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName string) error { + if !o.shouldSkipPathConflict(basePath, basePathName) { + return errors.PathConflictError{ + Entry: basePathName, + } + } + + log.Printf("Skipping conflict for path: %s", basePathName) + if o.arePathsIdenticalWithExcludeExtensions(basePathName) { + log.Printf("No doc diff detected for path %s, merging the paths", basePathName) + return nil + } + + log.Printf("Doc diff detected failing as allowDocsDiff=true is not supported.") + return errors.PathConflictError{ + Entry: basePathName, + } +} + // shouldSkipConflict checks if the conflict should be skipped. // The method goes through each path operation and performs the following checks: // 1. Validates if both paths have same operations, if not, then it returns false. From dd500c83975537233cd07b3fa758f793e3726195 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 16:25:44 -0300 Subject: [PATCH 07/21] update --- tools/cli/internal/openapi/oasdiff.go | 22 +++++--- tools/cli/internal/openapi/paths.go | 78 ++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 905b3480b5..d6403fb8c9 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -113,17 +113,20 @@ func (o OasDiff) mergePaths() error { return nil } - for externalPath, externalPathData := range pathsToMerge.Map() { + for path, externalPathData := range pathsToMerge.Map() { // Tries to find if the path already exists or not - if originalPath := basePaths.Value(externalPath); originalPath == nil { - basePaths.Set(externalPath, removeExternalRefs(externalPathData)) + if originalPathData := basePaths.Value(path); originalPathData == nil { + basePaths.Set(path, removeExternalRefs(externalPathData)) } else { - err := o.handlePathConflict(originalPath, externalPath) + err := o.handlePathConflict(originalPathData, path) if err != nil { return err } - basePaths.Set(externalPath, removeExternalRefs(externalPathData)) + // if diff is not allowed and there is a diff, then fail + if !allOperationsAllowDocsDiff(originalPathData, path) { + basePaths.Set(path, removeExternalRefs(externalPathData)) + } } } o.base.Spec.Paths = basePaths @@ -179,10 +182,13 @@ func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName st return nil } - log.Printf("Doc diff detected failing as allowDocsDiff=true is not supported.") - return errors.PathConflictError{ - Entry: basePathName, + if !allOperationsAllowDocsDiff(basePath, basePathName) { + log.Printf("Doc diff detected failing as allowDocsDiff=true is not supported.") + return errors.PathConflictError{ + Entry: basePathName, + } } + return nil } // shouldSkipConflict checks if the conflict should be skipped. diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index 1222c0b366..dee532329d 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -22,36 +22,37 @@ import ( const ( xgenSoaMigration = "x-xgen-soa-migration" + allowDocsDiff = "allowDocsDiff" ) -// allMethodsHaveExtension checks if all the operations in the base pat have the given extension name. +// allOperationsHaveExtension checks if all the operations in the base pat have the given extension name. func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, extensionName string) bool { if basePath.Get != nil { - if basePath.Get.Extensions == nil || basePath.Get.Extensions[extensionName] == nil { + if result := getOperationExtensionWithName(basePath.Get, extensionName); result == nil { return false } } if basePath.Put != nil { - if basePath.Put.Extensions == nil || basePath.Put.Extensions[extensionName] == nil { + if result := getOperationExtensionWithName(basePath.Put, extensionName); result == nil { return false } } if basePath.Post != nil { - if basePath.Post.Extensions == nil || basePath.Post.Extensions[extensionName] == nil { + if result := getOperationExtensionWithName(basePath.Post, extensionName); result == nil { return false } } if basePath.Patch != nil { - if basePath.Patch.Extensions == nil || basePath.Patch.Extensions[extensionName] == nil { + if result := getOperationExtensionWithName(basePath.Patch, extensionName); result == nil { return false } } if basePath.Delete != nil { - if basePath.Delete.Extensions == nil || basePath.Delete.Extensions[extensionName] == nil { + if result := getOperationExtensionWithName(basePath.Delete, extensionName); result == nil { return false } } @@ -59,3 +60,68 @@ func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, exten log.Println("Detected x-xgen-soa-migration annotation in all operations for path: ", basePathName) return true } + +func getOperationExtensionWithName(operation *openapi3.Operation, extensionName string) interface{} { + if operation.Extensions == nil || operation.Extensions[extensionName] == nil { + log.Printf("Operation %s does not have extension %q", operation.OperationID, extensionName) + return nil + } + + return operation.Extensions[extensionName] +} + +func allOperationsAllowDocsDiff(basePath *openapi3.PathItem, basePathName string) bool { + if basePath.Get != nil { + prop := getOperationExtensionProperty(basePath.Get, xgenSoaMigration, allowDocsDiff) + if prop != "true" { + return false + } + } + + if basePath.Put != nil { + prop := getOperationExtensionProperty(basePath.Put, xgenSoaMigration, allowDocsDiff) + if prop != "true" { + return false + } + } + + if basePath.Post != nil { + prop := getOperationExtensionProperty(basePath.Post, xgenSoaMigration, allowDocsDiff) + if prop != "true" { + return false + } + } + + if basePath.Patch != nil { + prop := getOperationExtensionProperty(basePath.Patch, xgenSoaMigration, allowDocsDiff) + if prop != "true" { + return false + } + } + + if basePath.Delete != nil { + prop := getOperationExtensionProperty(basePath.Delete, xgenSoaMigration, allowDocsDiff) + if prop != "true" { + return false + } + } + + return true +} + +func getOperationExtensionProperty(operation *openapi3.Operation, extensionName, extensionProperty string) string { + if operation.Extensions == nil || operation.Extensions[extensionName] == nil { + return "" + } + + extension := operation.Extensions[extensionName] + if extension == nil { + return "" + } + + value, ok := extension.(map[string]interface{})[extensionProperty].(string) + if ok { + return value + } + return "" +} From d35e6014f355c05efdf2281be489c835a87b1dcd Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 16:47:20 -0300 Subject: [PATCH 08/21] Add unit tests --- tools/cli/internal/openapi/paths.go | 8 + tools/cli/internal/openapi/paths_test.go | 186 +++++++++++++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 tools/cli/internal/openapi/paths_test.go diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index dee532329d..d29c5ab19e 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -27,6 +27,10 @@ const ( // allOperationsHaveExtension checks if all the operations in the base pat have the given extension name. func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, extensionName string) bool { + if basePath.Operations() == nil || len(basePath.Operations()) == 0 { + return false + } + if basePath.Get != nil { if result := getOperationExtensionWithName(basePath.Get, extensionName); result == nil { return false @@ -71,6 +75,10 @@ func getOperationExtensionWithName(operation *openapi3.Operation, extensionName } func allOperationsAllowDocsDiff(basePath *openapi3.PathItem, basePathName string) bool { + if basePath.Operations() == nil || len(basePath.Operations()) == 0 { + return false + } + if basePath.Get != nil { prop := getOperationExtensionProperty(basePath.Get, xgenSoaMigration, allowDocsDiff) if prop != "true" { diff --git a/tools/cli/internal/openapi/paths_test.go b/tools/cli/internal/openapi/paths_test.go new file mode 100644 index 0000000000..3846f9a808 --- /dev/null +++ b/tools/cli/internal/openapi/paths_test.go @@ -0,0 +1,186 @@ +// Copyright 2024 MongoDB Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package openapi + +import ( + "testing" + + "github.com/getkin/kin-openapi/openapi3" +) + +func TestAllOperationsHaveExtension(t *testing.T) { + tests := []struct { + name string + input *openapi3.PathItem + expected bool + }{ + { + name: "All operations have extension", + input: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "test": "true", + }, + }, + }, + Put: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "test": "true", + }, + }, + }, + Post: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "test": "true", + }, + }, + }, + Patch: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "test": "true", + }, + }, + }, + Delete: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "test": "true", + }, + }, + }, + }, + expected: true, + }, + { + name: "Not all operations have extension", + input: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "test": "true", + }, + }, + }, + Put: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-sunset": "true", + }, + }, + }, + expected: false, + }, + { + name: "No operations", + input: &openapi3.PathItem{}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := allOperationsHaveExtension(test.input, "/test", "x-xgen-soa-migration") + if actual != test.expected { + t.Errorf("Expected %t, got %t", test.expected, actual) + } + }) + } +} + +func TestAllOperationsAllowDocsDiff(t *testing.T) { + tests := []struct { + name string + input *openapi3.PathItem + expected bool + }{ + { + name: "All operations allow docs diff", + input: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + Put: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + Post: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + Patch: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + Delete: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + }, + expected: true, + }, + { + name: "Not all operations allow docs diff", + input: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + Put: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "false", + }, + }, + }, + }, + expected: false, + }, + { + name: "No operations", + input: &openapi3.PathItem{}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := allOperationsAllowDocsDiff(test.input, "/test") + if actual != test.expected { + t.Errorf("Expected %t, got %t", test.expected, actual) + } + }) + } +} From 5ded458441304bf23353b78c9a4824884b3dfc5c Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 17:13:53 -0300 Subject: [PATCH 09/21] Lint and update --- .../openapi/errors/merge_conflict_error.go | 30 +++++++++- tools/cli/internal/openapi/oasdiff.go | 55 +++++++++++-------- tools/cli/internal/openapi/paths.go | 2 +- tools/cli/internal/openapi/paths_test.go | 2 +- 4 files changed, 63 insertions(+), 26 deletions(-) diff --git a/tools/cli/internal/openapi/errors/merge_conflict_error.go b/tools/cli/internal/openapi/errors/merge_conflict_error.go index fa1cc9ec2f..3953142d80 100644 --- a/tools/cli/internal/openapi/errors/merge_conflict_error.go +++ b/tools/cli/internal/openapi/errors/merge_conflict_error.go @@ -14,7 +14,12 @@ package errors -import "fmt" +import ( + "encoding/json" + "fmt" + + "github.com/tufin/oasdiff/diff" +) type ParamConflictError struct { Entry string @@ -56,3 +61,26 @@ type TagConflictError struct { func (e TagConflictError) Error() string { return fmt.Sprintf("there was a conflict with the Tag %q with the description: %q", e.Entry, e.Description) } + +type PathDocsDiffConflictError struct { + Entry string + Diff *diff.Diff +} + +func (e PathDocsDiffConflictError) Error() string { + var pathDiff []byte + _, ok := e.Diff.PathsDiff.Modified[e.Entry] + if ok { + pathDiff, _ = json.MarshalIndent(e.Diff.PathsDiff.Modified[e.Entry], "", " ") + } + + return fmt.Sprintf("the path: %q is enabled for merge but it has a diff between the base and external spec. See the diff:\n%s", e.Entry, pathDiff) +} + +type AllowDocsDiffNotSupportedError struct { + Entry string +} + +func (e AllowDocsDiffNotSupportedError) Error() string { + return fmt.Sprintf("the path: %q is enabled for merge but the flag to allow docs diff is not supported", e.Entry) +} diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index d6403fb8c9..a1511783fd 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -15,7 +15,6 @@ package openapi import ( - "encoding/json" "log" "slices" "strings" @@ -122,11 +121,7 @@ func (o OasDiff) mergePaths() error { if err != nil { return err } - - // if diff is not allowed and there is a diff, then fail - if !allOperationsAllowDocsDiff(originalPathData, path) { - basePaths.Set(path, removeExternalRefs(externalPathData)) - } + basePaths.Set(path, removeExternalRefs(externalPathData)) } } o.base.Spec.Paths = basePaths @@ -176,19 +171,33 @@ func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName st } } + var pathsAreIdentical bool + var err error + if pathsAreIdentical, err = o.arePathsIdenticalWithExcludeExtensions(basePathName); err != nil { + return err + } + log.Printf("Skipping conflict for path: %s", basePathName) - if o.arePathsIdenticalWithExcludeExtensions(basePathName) { - log.Printf("No doc diff detected for path %s, merging the paths", basePathName) + if pathsAreIdentical { return nil } - if !allOperationsAllowDocsDiff(basePath, basePathName) { - log.Printf("Doc diff detected failing as allowDocsDiff=true is not supported.") - return errors.PathConflictError{ + // allowDocsDiff = true not supported + if allOperationsAllowDocsDiff(basePath) { + return errors.AllowDocsDiffNotSupportedError{ Entry: basePathName, } } - return nil + + d, err := o.getDiffWithoutExtensions() + if err != nil { + return err + } + + return errors.PathDocsDiffConflictError{ + Entry: basePathName, + Diff: d, + } } // shouldSkipConflict checks if the conflict should be skipped. @@ -475,24 +484,24 @@ func (o OasDiff) areSchemaIdentical(name string) bool { } // arePathsIdenticalWithExcludeExtensions checks if the paths are identical with the extensions excluded -func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) bool { +func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, error) { // If the diff only has extensions diff, then we consider the paths to be identical - exclude := []string{"extensions"} - customConfig := diff.NewConfig().WithExcludeElements(exclude) - d, err := diff.Get(customConfig, o.base.Spec, o.external.Spec) + d, err := o.getDiffWithoutExtensions() if err != nil { - log.Fatalf("error in calculating the diff of the specs: %s", err) + return false, err } if d.Empty() || d.PathsDiff.Empty() { - return true + return true, nil } _, ok := d.PathsDiff.Modified[name] - if ok { - j, _ := json.MarshalIndent(d.PathsDiff.Modified[name], "", " ") - log.Printf("arePathsIdenticalWithExcludeExtensions diff: %s", j) - } - return !ok + return !ok, nil +} + +func (o OasDiff) getDiffWithoutExtensions() (*diff.Diff, error) { + exclude := []string{"extensions"} + customConfig := diff.NewConfig().WithExcludeElements(exclude) + return diff.Get(customConfig, o.base.Spec, o.external.Spec) } type ByName []*openapi3.Tag diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index d29c5ab19e..cc34f1bcda 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -74,7 +74,7 @@ func getOperationExtensionWithName(operation *openapi3.Operation, extensionName return operation.Extensions[extensionName] } -func allOperationsAllowDocsDiff(basePath *openapi3.PathItem, basePathName string) bool { +func allOperationsAllowDocsDiff(basePath *openapi3.PathItem) bool { if basePath.Operations() == nil || len(basePath.Operations()) == 0 { return false } diff --git a/tools/cli/internal/openapi/paths_test.go b/tools/cli/internal/openapi/paths_test.go index 3846f9a808..0c6de879a5 100644 --- a/tools/cli/internal/openapi/paths_test.go +++ b/tools/cli/internal/openapi/paths_test.go @@ -177,7 +177,7 @@ func TestAllOperationsAllowDocsDiff(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - actual := allOperationsAllowDocsDiff(test.input, "/test") + actual := allOperationsAllowDocsDiff(test.input) if actual != test.expected { t.Errorf("Expected %t, got %t", test.expected, actual) } From 0a5d7df128fa1d5124d9b9e5c0867912b11076ce Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 18:28:24 -0300 Subject: [PATCH 10/21] Add tests --- .../internal/openapi/filter/mock_filter.go | 9 +- tools/cli/internal/openapi/mock_paths.go | 56 +++++ tools/cli/internal/openapi/oasdiff.go | 31 +-- tools/cli/internal/openapi/oasdiff_test.go | 198 ++++++++++++++++++ tools/cli/internal/openapi/paths.go | 18 ++ 5 files changed, 292 insertions(+), 20 deletions(-) create mode 100644 tools/cli/internal/openapi/mock_paths.go diff --git a/tools/cli/internal/openapi/filter/mock_filter.go b/tools/cli/internal/openapi/filter/mock_filter.go index f16d593e45..30d17193b6 100644 --- a/tools/cli/internal/openapi/filter/mock_filter.go +++ b/tools/cli/internal/openapi/filter/mock_filter.go @@ -12,7 +12,6 @@ package filter import ( reflect "reflect" - openapi3 "github.com/getkin/kin-openapi/openapi3" gomock "go.uber.org/mock/gomock" ) @@ -40,15 +39,15 @@ func (m *MockFilter) EXPECT() *MockFilterMockRecorder { } // Apply mocks base method. -func (m *MockFilter) Apply(arg0 *openapi3.T, arg1 *Metadata) error { +func (m *MockFilter) Apply() error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Apply", arg0, arg1) + ret := m.ctrl.Call(m, "Apply") ret0, _ := ret[0].(error) return ret0 } // Apply indicates an expected call of Apply. -func (mr *MockFilterMockRecorder) Apply(arg0, arg1 any) *gomock.Call { +func (mr *MockFilterMockRecorder) Apply() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Apply", reflect.TypeOf((*MockFilter)(nil).Apply), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Apply", reflect.TypeOf((*MockFilter)(nil).Apply)) } diff --git a/tools/cli/internal/openapi/mock_paths.go b/tools/cli/internal/openapi/mock_paths.go new file mode 100644 index 0000000000..d7e06a7f0e --- /dev/null +++ b/tools/cli/internal/openapi/mock_paths.go @@ -0,0 +1,56 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/mongodb/openapi/tools/cli/internal/openapi (interfaces: NoExtensionDiff) +// +// Generated by this command: +// +// mockgen -destination=../openapi/mock_paths.go -package=openapi github.com/mongodb/openapi/tools/cli/internal/openapi NoExtensionDiff +// + +// Package openapi is a generated GoMock package. +package openapi + +import ( + reflect "reflect" + + openapi3 "github.com/getkin/kin-openapi/openapi3" + diff "github.com/tufin/oasdiff/diff" + gomock "go.uber.org/mock/gomock" +) + +// MockNoExtensionDiff is a mock of NoExtensionDiff interface. +type MockNoExtensionDiff struct { + ctrl *gomock.Controller + recorder *MockNoExtensionDiffMockRecorder +} + +// MockNoExtensionDiffMockRecorder is the mock recorder for MockNoExtensionDiff. +type MockNoExtensionDiffMockRecorder struct { + mock *MockNoExtensionDiff +} + +// NewMockNoExtensionDiff creates a new mock instance. +func NewMockNoExtensionDiff(ctrl *gomock.Controller) *MockNoExtensionDiff { + mock := &MockNoExtensionDiff{ctrl: ctrl} + mock.recorder = &MockNoExtensionDiffMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockNoExtensionDiff) EXPECT() *MockNoExtensionDiffMockRecorder { + return m.recorder +} + +// GetPathDiffWithoutExtensions mocks base method. +func (m *MockNoExtensionDiff) GetPathDiffWithoutExtensions(arg0, arg1 *openapi3.T) (*diff.Diff, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPathDiffWithoutExtensions", arg0, arg1) + ret0, _ := ret[0].(*diff.Diff) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetPathDiffWithoutExtensions indicates an expected call of GetPathDiffWithoutExtensions. +func (mr *MockNoExtensionDiffMockRecorder) GetPathDiffWithoutExtensions(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPathDiffWithoutExtensions", reflect.TypeOf((*MockNoExtensionDiff)(nil).GetPathDiffWithoutExtensions), arg0, arg1) +} diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index a1511783fd..6cf12f8397 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -28,11 +28,12 @@ import ( ) type OasDiff struct { - base *load.SpecInfo - external *load.SpecInfo - config *diff.Config - specDiff *diff.Diff - parser Parser + base *load.SpecInfo + external *load.SpecInfo + config *diff.Config + specDiff *diff.Diff + noExtDiff NoExtensionDiff + parser Parser } type OasDiffResult struct { @@ -177,7 +178,7 @@ func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName st return err } - log.Printf("Skipping conflict for path: %s", basePathName) + log.Printf("Skipping conflict for path: %s, pathsAreIdentical: %v", basePathName, pathsAreIdentical) if pathsAreIdentical { return nil } @@ -189,7 +190,7 @@ func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName st } } - d, err := o.getDiffWithoutExtensions() + d, err := o.noExtDiff.GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec) if err != nil { return err } @@ -486,7 +487,7 @@ func (o OasDiff) areSchemaIdentical(name string) bool { // arePathsIdenticalWithExcludeExtensions checks if the paths are identical with the extensions excluded func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, error) { // If the diff only has extensions diff, then we consider the paths to be identical - d, err := o.getDiffWithoutExtensions() + d, err := o.noExtDiff.GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec) if err != nil { return false, err } @@ -494,14 +495,14 @@ func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, erro if d.Empty() || d.PathsDiff.Empty() { return true, nil } - _, ok := d.PathsDiff.Modified[name] - return !ok, nil -} + v, ok := d.PathsDiff.Modified[name] + if ok { + if v.Empty() { + return true, nil + } + } -func (o OasDiff) getDiffWithoutExtensions() (*diff.Diff, error) { - exclude := []string{"extensions"} - customConfig := diff.NewConfig().WithExcludeElements(exclude) - return diff.Get(customConfig, o.base.Spec, o.external.Spec) + return !ok, nil } type ByName []*openapi3.Tag diff --git a/tools/cli/internal/openapi/oasdiff_test.go b/tools/cli/internal/openapi/oasdiff_test.go index 18f7a3a960..2d0066774a 100644 --- a/tools/cli/internal/openapi/oasdiff_test.go +++ b/tools/cli/internal/openapi/oasdiff_test.go @@ -19,11 +19,14 @@ import ( "testing" "github.com/getkin/kin-openapi/openapi3" + "github.com/mongodb/openapi/tools/cli/internal/openapi/errors" "github.com/mongodb/openapi/tools/cli/internal/pointer" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tufin/oasdiff/diff" "github.com/tufin/oasdiff/load" + "github.com/tufin/oasdiff/utils" + gomock "go.uber.org/mock/gomock" ) func TestOasDiff_mergePaths(t *testing.T) { @@ -1142,3 +1145,198 @@ func TestUpdateExternalRefReqBody(t *testing.T) { }) } } + +func TestHandlePathConflict(t *testing.T) { + testCases := []struct { + name string + basePath *openapi3.PathItem + basePathName string + specDiff *diff.Diff + expectedError error + }{ + { + name: "No Conflict - Identical Paths", + basePath: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "false", + }, + }, + }, + }, + basePathName: "/test", + specDiff: &diff.Diff{ + PathsDiff: &diff.PathsDiff{ + Modified: map[string]*diff.PathDiff{}, + }, + }, + expectedError: nil, + }, + { + name: "Conflict with AllowDocsDiff", + basePath: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "true", + }, + }, + }, + }, + basePathName: "/test", + specDiff: &diff.Diff{ + PathsDiff: &diff.PathsDiff{ + Modified: map[string]*diff.PathDiff{ + "/test": { + OperationsDiff: &diff.OperationsDiff{ + Modified: diff.ModifiedOperations{ + "get": { + TagsDiff: &diff.StringsDiff{ + Added: utils.StringList{"tag1"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedError: errors.AllowDocsDiffNotSupportedError{ + Entry: "/test", + }, + }, + { + name: "Conflict with Different Operations", + basePath: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "false", + }, + }, + }, + }, + basePathName: "/test", + specDiff: &diff.Diff{ + PathsDiff: &diff.PathsDiff{ + Modified: map[string]*diff.PathDiff{ + "/test": { + OperationsDiff: &diff.OperationsDiff{ + Added: utils.StringList{"get"}, + Deleted: utils.StringList{}, + }, + }, + }, + }, + }, + expectedError: errors.PathConflictError{ + Entry: "/test", + }, + }, + { + name: "Conflict with Different Path Operation", + basePath: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "false", + }, + }, + }, + }, + basePathName: "/test", + specDiff: &diff.Diff{ + PathsDiff: &diff.PathsDiff{ + Modified: map[string]*diff.PathDiff{ + "/test": { + OperationsDiff: &diff.OperationsDiff{ + Modified: diff.ModifiedOperations{ + "get": { + TagsDiff: &diff.StringsDiff{ + Added: utils.StringList{"tag1"}, + }, + }, + }, + }, + }, + }, + }, + }, + expectedError: errors.PathDocsDiffConflictError{ + Entry: "/test", + Diff: &diff.Diff{ + PathsDiff: &diff.PathsDiff{ + Modified: map[string]*diff.PathDiff{ + "/test": { + OperationsDiff: &diff.OperationsDiff{ + Modified: diff.ModifiedOperations{ + "get": { + TagsDiff: &diff.StringsDiff{ + Added: utils.StringList{"tag1"}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "Identical Paths with Extensions", + basePath: &openapi3.PathItem{ + Get: &openapi3.Operation{ + Extensions: map[string]interface{}{ + "x-xgen-soa-migration": map[string]interface{}{ + "allowDocsDiff": "false", + }, + }, + }, + }, + basePathName: "/test", + specDiff: &diff.Diff{ + PathsDiff: &diff.PathsDiff{ + Modified: map[string]*diff.PathDiff{}, + }, + }, + expectedError: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + mockNoExtensionDiff := NewMockNoExtensionDiff(ctrl) + o := OasDiff{ + base: &load.SpecInfo{ + Spec: &openapi3.T{ + Paths: &openapi3.Paths{}, + }, + }, + external: &load.SpecInfo{ + Spec: &openapi3.T{ + Paths: &openapi3.Paths{}, + }, + }, + specDiff: tc.specDiff, + noExtDiff: mockNoExtensionDiff, + } + + mockNoExtensionDiff. + EXPECT(). + GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec). + Return(tc.specDiff, nil). + AnyTimes() + + err := o.handlePathConflict(tc.basePath, tc.basePathName) + if tc.expectedError != nil { + assert.Error(t, err) + assert.IsType(t, tc.expectedError, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index cc34f1bcda..2ad4f6b65a 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -15,11 +15,15 @@ package openapi import ( + "fmt" "log" "github.com/getkin/kin-openapi/openapi3" + "github.com/tufin/oasdiff/diff" ) +//go:generate mockgen -destination=../openapi/mock_paths.go -package=openapi github.com/mongodb/openapi/tools/cli/internal/openapi NoExtensionDiff + const ( xgenSoaMigration = "x-xgen-soa-migration" allowDocsDiff = "allowDocsDiff" @@ -133,3 +137,17 @@ func getOperationExtensionProperty(operation *openapi3.Operation, extensionName, } return "" } + +type NoExtensionDiff interface { + GetPathDiffWithoutExtensions(base, external *openapi3.T) (*diff.Diff, error) +} + +func GetPathDiffWithoutExtensions(base, external *openapi3.T) (*diff.Diff, error) { + exclude := []string{"extensions"} + customConfig := diff.NewConfig().WithExcludeElements(exclude) + if base == nil || external == nil { + return nil, fmt.Errorf("base or external spec is nil") + } + + return diff.Get(customConfig, base, external) +} From 8eb3c9e05982dde767e58168446b49f7636a9244 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Tue, 8 Oct 2024 21:13:27 -0300 Subject: [PATCH 11/21] Lint --- tools/cli/internal/openapi/oasdiff_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/cli/internal/openapi/oasdiff_test.go b/tools/cli/internal/openapi/oasdiff_test.go index 2d0066774a..766c2f4021 100644 --- a/tools/cli/internal/openapi/oasdiff_test.go +++ b/tools/cli/internal/openapi/oasdiff_test.go @@ -1332,7 +1332,7 @@ func TestHandlePathConflict(t *testing.T) { err := o.handlePathConflict(tc.basePath, tc.basePathName) if tc.expectedError != nil { - assert.Error(t, err) + require.Error(t, err) assert.IsType(t, tc.expectedError, err) } else { assert.NoError(t, err) From e45a90be32d8c6edb6204e17c5e64f549576037c Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Wed, 9 Oct 2024 07:35:24 -0300 Subject: [PATCH 12/21] Address comment --- tools/cli/internal/openapi/oasdiff.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 6cf12f8397..aede2766cc 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -118,8 +118,7 @@ func (o OasDiff) mergePaths() error { if originalPathData := basePaths.Value(path); originalPathData == nil { basePaths.Set(path, removeExternalRefs(externalPathData)) } else { - err := o.handlePathConflict(originalPathData, path) - if err != nil { + if err := o.handlePathConflict(originalPathData, path); err != nil { return err } basePaths.Set(path, removeExternalRefs(externalPathData)) From bf5ed4bb12f6ffaa3b6ced127dc825398bb29504 Mon Sep 17 00:00:00 2001 From: Bianca Lisle <40155621+blva@users.noreply.github.com> Date: Wed, 9 Oct 2024 20:43:34 +0100 Subject: [PATCH 13/21] Update tools/cli/internal/openapi/paths.go Co-authored-by: Andrea Angiolillo --- tools/cli/internal/openapi/paths.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index 2ad4f6b65a..450a52e669 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -29,7 +29,7 @@ const ( allowDocsDiff = "allowDocsDiff" ) -// allOperationsHaveExtension checks if all the operations in the base pat have the given extension name. +// allOperationsHaveExtension checks if all the operations in the base path have the given extension name. func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, extensionName string) bool { if basePath.Operations() == nil || len(basePath.Operations()) == 0 { return false From 580dcd59fb90b405af4ec835c385ec447bd7fb3a Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Wed, 9 Oct 2024 16:44:40 -0300 Subject: [PATCH 14/21] address comment --- tools/cli/internal/openapi/paths.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index 450a52e669..6ccd0a6d16 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -65,7 +65,7 @@ func allOperationsHaveExtension(basePath *openapi3.PathItem, basePathName, exten } } - log.Println("Detected x-xgen-soa-migration annotation in all operations for path: ", basePathName) + log.Printf("Detected %s annotation in all operations for path: %s\n", extensionName, basePathName) return true } From 61613ab9236a3e3823d4701f6d2c77ee349f7854 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 09:24:22 -0300 Subject: [PATCH 15/21] Address comment --- tools/cli/internal/openapi/oasdiff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index aede2766cc..e60e1e1382 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -483,7 +483,7 @@ func (o OasDiff) areSchemaIdentical(name string) bool { return !ok } -// arePathsIdenticalWithExcludeExtensions checks if the paths are identical with the extensions excluded +// arePathsIdenticalWithExcludeExtensions checks if the paths are identical excluding extension diffs across operations (e.g. x-xgen-soa-migration). func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, error) { // If the diff only has extensions diff, then we consider the paths to be identical d, err := o.noExtDiff.GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec) From 413a71667cdb6d2e75b9244e5c11b0a3a792f6ac Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 11:33:02 -0300 Subject: [PATCH 16/21] Address comments --- tools/cli/internal/openapi/oasdiff.go | 21 +++++++++++--------- tools/cli/internal/openapi/oasdiff_result.go | 15 ++++++++++++++ tools/cli/internal/openapi/oasdiff_test.go | 5 +++-- tools/cli/internal/openapi/paths.go | 18 ----------------- 4 files changed, 30 insertions(+), 29 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 9e834567af..abd2d2f5a5 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -27,12 +27,11 @@ import ( ) type OasDiff struct { - base *load.SpecInfo - external *load.SpecInfo - config *diff.Config - result *OasDiffResult - noExtDiff NoExtensionDiff - parser Parser + base *load.SpecInfo + external *load.SpecInfo + config *diff.Config + result *OasDiffResult + parser Parser } func (o OasDiff) mergeSpecIntoBase() (*load.SpecInfo, error) { @@ -147,14 +146,16 @@ func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName st } } - d, err := o.noExtDiff.GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec) + exclude := []string{"extensions"} + customConfig := diff.NewConfig().WithExcludeElements(exclude) + d, err := o.GetDiffWithConfig(o.base, o.external, customConfig) if err != nil { return err } return errors.PathDocsDiffConflictError{ Entry: basePathName, - Diff: d, + Diff: d.Report, } } @@ -444,11 +445,13 @@ func (o OasDiff) areSchemaIdentical(name string) bool { // arePathsIdenticalWithExcludeExtensions checks if the paths are identical excluding extension diffs across operations (e.g. x-xgen-soa-migration). func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, error) { // If the diff only has extensions diff, then we consider the paths to be identical - d, err := o.noExtDiff.GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec) + customConfig := o.config.WithExcludeElements([]string{"extensions"}) + result, err := o.GetDiffWithConfig(o.base, o.external, customConfig) if err != nil { return false, err } + d := result.Report if d.Empty() || d.PathsDiff.Empty() { return true, nil } diff --git a/tools/cli/internal/openapi/oasdiff_result.go b/tools/cli/internal/openapi/oasdiff_result.go index 9b7d370c5e..ba1bc01b00 100644 --- a/tools/cli/internal/openapi/oasdiff_result.go +++ b/tools/cli/internal/openapi/oasdiff_result.go @@ -78,3 +78,18 @@ func (o OasDiff) GetFlattenedDiff(base, revision *load.SpecInfo) (*OasDiffResult Config: o.config, }, nil } + +// GetDiffWithConfig returns the diff between two OpenAPI specs with a custom config. +func (o OasDiff) GetDiffWithConfig(base, revision *load.SpecInfo, config *diff.Config) (*OasDiffResult, error) { + diffReport, err := diff.Get(config, base.Spec, revision.Spec) + if err != nil { + return nil, err + } + + return &OasDiffResult{ + Report: diffReport, + SourceMap: nil, + SpecInfoPair: load.NewSpecInfoPair(base, revision), + Config: config, + }, nil +} diff --git a/tools/cli/internal/openapi/oasdiff_test.go b/tools/cli/internal/openapi/oasdiff_test.go index 137de0a795..4422a06094 100644 --- a/tools/cli/internal/openapi/oasdiff_test.go +++ b/tools/cli/internal/openapi/oasdiff_test.go @@ -1324,8 +1324,9 @@ func TestHandlePathConflict(t *testing.T) { Paths: &openapi3.Paths{}, }, }, - specDiff: tc.specDiff, - noExtDiff: mockNoExtensionDiff, + result: &OasDiffResult{ + Report: tc.specDiff, + }, } mockNoExtensionDiff. diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go index 6ccd0a6d16..486da12188 100644 --- a/tools/cli/internal/openapi/paths.go +++ b/tools/cli/internal/openapi/paths.go @@ -15,15 +15,11 @@ package openapi import ( - "fmt" "log" "github.com/getkin/kin-openapi/openapi3" - "github.com/tufin/oasdiff/diff" ) -//go:generate mockgen -destination=../openapi/mock_paths.go -package=openapi github.com/mongodb/openapi/tools/cli/internal/openapi NoExtensionDiff - const ( xgenSoaMigration = "x-xgen-soa-migration" allowDocsDiff = "allowDocsDiff" @@ -137,17 +133,3 @@ func getOperationExtensionProperty(operation *openapi3.Operation, extensionName, } return "" } - -type NoExtensionDiff interface { - GetPathDiffWithoutExtensions(base, external *openapi3.T) (*diff.Diff, error) -} - -func GetPathDiffWithoutExtensions(base, external *openapi3.T) (*diff.Diff, error) { - exclude := []string{"extensions"} - customConfig := diff.NewConfig().WithExcludeElements(exclude) - if base == nil || external == nil { - return nil, fmt.Errorf("base or external spec is nil") - } - - return diff.Get(customConfig, base, external) -} From eaed0f0092527bc7f031879e36720ec07261eb6c Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 12:21:57 -0300 Subject: [PATCH 17/21] Update --- .../internal/openapi/mock_oasdiff_result.go | 73 ++++++++ tools/cli/internal/openapi/mock_paths.go | 56 ------ tools/cli/internal/openapi/oasdiff.go | 11 +- tools/cli/internal/openapi/oasdiff_result.go | 14 +- .../internal/openapi/oasdiff_result_test.go | 174 ++++++++++++++++++ tools/cli/internal/openapi/oasdiff_test.go | 6 +- 6 files changed, 267 insertions(+), 67 deletions(-) create mode 100644 tools/cli/internal/openapi/mock_oasdiff_result.go delete mode 100644 tools/cli/internal/openapi/mock_paths.go create mode 100644 tools/cli/internal/openapi/oasdiff_result_test.go diff --git a/tools/cli/internal/openapi/mock_oasdiff_result.go b/tools/cli/internal/openapi/mock_oasdiff_result.go new file mode 100644 index 0000000000..99c9074ab8 --- /dev/null +++ b/tools/cli/internal/openapi/mock_oasdiff_result.go @@ -0,0 +1,73 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/mongodb/openapi/tools/cli/internal/openapi (interfaces: DiffGetter) +// +// Generated by this command: +// +// mockgen -destination=../openapi/mock_oasdiff_result.go -package=openapi github.com/mongodb/openapi/tools/cli/internal/openapi DiffGetter +// + +// Package openapi is a generated GoMock package. +package openapi + +import ( + reflect "reflect" + + openapi3 "github.com/getkin/kin-openapi/openapi3" + diff "github.com/tufin/oasdiff/diff" + load "github.com/tufin/oasdiff/load" + gomock "go.uber.org/mock/gomock" +) + +// MockDiffGetter is a mock of DiffGetter interface. +type MockDiffGetter struct { + ctrl *gomock.Controller + recorder *MockDiffGetterMockRecorder +} + +// MockDiffGetterMockRecorder is the mock recorder for MockDiffGetter. +type MockDiffGetterMockRecorder struct { + mock *MockDiffGetter +} + +// NewMockDiffGetter creates a new mock instance. +func NewMockDiffGetter(ctrl *gomock.Controller) *MockDiffGetter { + mock := &MockDiffGetter{ctrl: ctrl} + mock.recorder = &MockDiffGetterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockDiffGetter) EXPECT() *MockDiffGetterMockRecorder { + return m.recorder +} + +// Get mocks base method. +func (m *MockDiffGetter) Get(arg0 *diff.Config, arg1, arg2 *openapi3.T) (*diff.Diff, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Get", arg0, arg1, arg2) + ret0, _ := ret[0].(*diff.Diff) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Get indicates an expected call of Get. +func (mr *MockDiffGetterMockRecorder) Get(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockDiffGetter)(nil).Get), arg0, arg1, arg2) +} + +// GetWithOperationsSourcesMap mocks base method. +func (m *MockDiffGetter) GetWithOperationsSourcesMap(arg0 *diff.Config, arg1, arg2 *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetWithOperationsSourcesMap", arg0, arg1, arg2) + ret0, _ := ret[0].(*diff.Diff) + ret1, _ := ret[1].(*diff.OperationsSourcesMap) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// GetWithOperationsSourcesMap indicates an expected call of GetWithOperationsSourcesMap. +func (mr *MockDiffGetterMockRecorder) GetWithOperationsSourcesMap(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWithOperationsSourcesMap", reflect.TypeOf((*MockDiffGetter)(nil).GetWithOperationsSourcesMap), arg0, arg1, arg2) +} diff --git a/tools/cli/internal/openapi/mock_paths.go b/tools/cli/internal/openapi/mock_paths.go deleted file mode 100644 index d7e06a7f0e..0000000000 --- a/tools/cli/internal/openapi/mock_paths.go +++ /dev/null @@ -1,56 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/mongodb/openapi/tools/cli/internal/openapi (interfaces: NoExtensionDiff) -// -// Generated by this command: -// -// mockgen -destination=../openapi/mock_paths.go -package=openapi github.com/mongodb/openapi/tools/cli/internal/openapi NoExtensionDiff -// - -// Package openapi is a generated GoMock package. -package openapi - -import ( - reflect "reflect" - - openapi3 "github.com/getkin/kin-openapi/openapi3" - diff "github.com/tufin/oasdiff/diff" - gomock "go.uber.org/mock/gomock" -) - -// MockNoExtensionDiff is a mock of NoExtensionDiff interface. -type MockNoExtensionDiff struct { - ctrl *gomock.Controller - recorder *MockNoExtensionDiffMockRecorder -} - -// MockNoExtensionDiffMockRecorder is the mock recorder for MockNoExtensionDiff. -type MockNoExtensionDiffMockRecorder struct { - mock *MockNoExtensionDiff -} - -// NewMockNoExtensionDiff creates a new mock instance. -func NewMockNoExtensionDiff(ctrl *gomock.Controller) *MockNoExtensionDiff { - mock := &MockNoExtensionDiff{ctrl: ctrl} - mock.recorder = &MockNoExtensionDiffMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockNoExtensionDiff) EXPECT() *MockNoExtensionDiffMockRecorder { - return m.recorder -} - -// GetPathDiffWithoutExtensions mocks base method. -func (m *MockNoExtensionDiff) GetPathDiffWithoutExtensions(arg0, arg1 *openapi3.T) (*diff.Diff, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetPathDiffWithoutExtensions", arg0, arg1) - ret0, _ := ret[0].(*diff.Diff) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetPathDiffWithoutExtensions indicates an expected call of GetPathDiffWithoutExtensions. -func (mr *MockNoExtensionDiffMockRecorder) GetPathDiffWithoutExtensions(arg0, arg1 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPathDiffWithoutExtensions", reflect.TypeOf((*MockNoExtensionDiff)(nil).GetPathDiffWithoutExtensions), arg0, arg1) -} diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index abd2d2f5a5..82e4637f83 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -27,11 +27,12 @@ import ( ) type OasDiff struct { - base *load.SpecInfo - external *load.SpecInfo - config *diff.Config - result *OasDiffResult - parser Parser + base *load.SpecInfo + external *load.SpecInfo + config *diff.Config + diffGetter DiffGetter + result *OasDiffResult + parser Parser } func (o OasDiff) mergeSpecIntoBase() (*load.SpecInfo, error) { diff --git a/tools/cli/internal/openapi/oasdiff_result.go b/tools/cli/internal/openapi/oasdiff_result.go index ba1bc01b00..ba0266bdb5 100644 --- a/tools/cli/internal/openapi/oasdiff_result.go +++ b/tools/cli/internal/openapi/oasdiff_result.go @@ -14,7 +14,9 @@ package openapi +//go:generate mockgen -destination=../openapi/mock_oasdiff_result.go -package=openapi github.com/mongodb/openapi/tools/cli/internal/openapi DiffGetter import ( + "github.com/getkin/kin-openapi/openapi3" "github.com/tufin/oasdiff/diff" "github.com/tufin/oasdiff/flatten/allof" "github.com/tufin/oasdiff/load" @@ -27,9 +29,15 @@ type OasDiffResult struct { Config *diff.Config } +// DiffGetter defines an interface for getting diffs. +type DiffGetter interface { + Get(config *diff.Config, base, revision *openapi3.T) (*diff.Diff, error) + GetWithOperationsSourcesMap(config *diff.Config, base, revision *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) +} + // GetSimpleDiff returns the diff between two OpenAPI specs. func (o OasDiff) GetSimpleDiff(base, revision *load.SpecInfo) (*OasDiffResult, error) { - diffReport, err := diff.Get(o.config, base.Spec, revision.Spec) + diffReport, err := o.diffGetter.Get(o.config, base.Spec, revision.Spec) if err != nil { return nil, err } @@ -66,7 +74,7 @@ func (o OasDiff) GetFlattenedDiff(base, revision *load.SpecInfo) (*OasDiffResult Version: revision.GetVersion(), } - diffReport, operationsSources, err := diff.GetWithOperationsSourcesMap(o.config, baseSpecInfo, revisionSpecInfo) + diffReport, operationsSources, err := o.diffGetter.GetWithOperationsSourcesMap(o.config, baseSpecInfo, revisionSpecInfo) if err != nil { return nil, err } @@ -81,7 +89,7 @@ func (o OasDiff) GetFlattenedDiff(base, revision *load.SpecInfo) (*OasDiffResult // GetDiffWithConfig returns the diff between two OpenAPI specs with a custom config. func (o OasDiff) GetDiffWithConfig(base, revision *load.SpecInfo, config *diff.Config) (*OasDiffResult, error) { - diffReport, err := diff.Get(config, base.Spec, revision.Spec) + diffReport, err := o.diffGetter.Get(config, base.Spec, revision.Spec) if err != nil { return nil, err } diff --git a/tools/cli/internal/openapi/oasdiff_result_test.go b/tools/cli/internal/openapi/oasdiff_result_test.go new file mode 100644 index 0000000000..c684536155 --- /dev/null +++ b/tools/cli/internal/openapi/oasdiff_result_test.go @@ -0,0 +1,174 @@ +// Copyright 2024 MongoDB Inc +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package openapi + +import ( + "errors" + "testing" + + "github.com/getkin/kin-openapi/openapi3" + "github.com/stretchr/testify/require" + "github.com/tufin/oasdiff/diff" + "github.com/tufin/oasdiff/load" + gomock "go.uber.org/mock/gomock" +) + +func TestGetSimpleDiff(t *testing.T) { + // Mock diff.Get function + ctrl := gomock.NewController(t) + mockDiffGet := NewMockDiffGetter(ctrl) + + testCases := []struct { + name string + base *load.SpecInfo + revision *load.SpecInfo + expectedError error + expectedResult *OasDiffResult + }{ + { + name: "Simple Diff Success", + base: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + revision: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + expectedError: nil, + expectedResult: &OasDiffResult{ + Report: &diff.Diff{}, + SourceMap: nil, + SpecInfoPair: load.NewSpecInfoPair(&load.SpecInfo{ + Spec: &openapi3.T{}, + }, &load.SpecInfo{ + Spec: &openapi3.T{}, + }), + Config: &diff.Config{}, + }, + }, + { + name: "Simple Diff Error", + base: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + revision: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + expectedError: errors.New("diff error"), + expectedResult: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + o := OasDiff{ + config: &diff.Config{}, + diffGetter: mockDiffGet, + } + + var returnDiff *diff.Diff + if tc.expectedResult != nil { + returnDiff = tc.expectedResult.Report + } + + mockDiffGet. + EXPECT(). + Get(o.config, tc.base.Spec, tc.revision.Spec). + Return(returnDiff, tc.expectedError) + + result, err := o.GetSimpleDiff(tc.base, tc.revision) + + // assert + require.ErrorIs(t, err, tc.expectedError) + require.Equal(t, tc.expectedResult, result) + }) + } +} + +func TestGetDiffWithConfig(t *testing.T) { + // Mock diff.Get function + ctrl := gomock.NewController(t) + mockDiffGet := NewMockDiffGetter(ctrl) + + exclude := []string{"extensions"} + customConfig := diff.NewConfig().WithExcludeElements(exclude) + + testCases := []struct { + name string + base *load.SpecInfo + revision *load.SpecInfo + config *diff.Config + expectedError error + expectedResult *OasDiffResult + }{ + { + name: "Diff With Custom Config Success", + base: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + revision: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + config: customConfig, + expectedError: nil, + expectedResult: &OasDiffResult{ + Report: &diff.Diff{}, + SourceMap: nil, + SpecInfoPair: load.NewSpecInfoPair(&load.SpecInfo{ + Spec: &openapi3.T{}, + }, &load.SpecInfo{ + Spec: &openapi3.T{}, + }), + Config: customConfig, + }, + }, + { + name: "Diff With Config Error", + base: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + revision: &load.SpecInfo{ + Spec: &openapi3.T{}, + }, + config: &diff.Config{}, + expectedError: errors.New("diff error"), + expectedResult: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + o := OasDiff{ + config: &diff.Config{}, + diffGetter: mockDiffGet, + } + + var returnDiff *diff.Diff + if tc.expectedResult != nil { + returnDiff = tc.expectedResult.Report + } + + mockDiffGet. + EXPECT(). + Get(tc.config, tc.base.Spec, tc.revision.Spec). + Return(returnDiff, tc.expectedError) + + result, err := o.GetDiffWithConfig(tc.base, tc.revision, tc.config) + + // assert + require.ErrorIs(t, err, tc.expectedError) + require.Equal(t, tc.expectedResult, result) + }) + } +} diff --git a/tools/cli/internal/openapi/oasdiff_test.go b/tools/cli/internal/openapi/oasdiff_test.go index 4422a06094..3e5ef3f998 100644 --- a/tools/cli/internal/openapi/oasdiff_test.go +++ b/tools/cli/internal/openapi/oasdiff_test.go @@ -1312,7 +1312,7 @@ func TestHandlePathConflict(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { ctrl := gomock.NewController(t) - mockNoExtensionDiff := NewMockNoExtensionDiff(ctrl) + mockDiffGetter := NewMockDiffGetter(ctrl) o := OasDiff{ base: &load.SpecInfo{ Spec: &openapi3.T{ @@ -1329,9 +1329,9 @@ func TestHandlePathConflict(t *testing.T) { }, } - mockNoExtensionDiff. + mockDiffGetter. EXPECT(). - GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec). + Get(o.config, o.base.Spec, o.external.Spec). Return(tc.specDiff, nil). AnyTimes() From 73e8aaed77d2ffc78a79ebc0e4593547a16e3d49 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 12:36:02 -0300 Subject: [PATCH 18/21] Update tests --- tools/cli/internal/openapi/oasdiff.go | 4 ++-- tools/cli/internal/openapi/oasdiff_test.go | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 82e4637f83..41fd224796 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -167,7 +167,7 @@ func (o OasDiff) handlePathConflict(basePath *openapi3.PathItem, basePathName st // If there is no annotation, then it returns false. func (o OasDiff) shouldSkipPathConflict(basePath *openapi3.PathItem, basePathName string) bool { var pathsDiff *diff.PathsDiff - if o.result.Report != nil && o.result.Report.PathsDiff != nil { + if o.result != nil && o.result.Report != nil && o.result.Report.PathsDiff != nil { pathsDiff = o.result.Report.PathsDiff } @@ -446,7 +446,7 @@ func (o OasDiff) areSchemaIdentical(name string) bool { // arePathsIdenticalWithExcludeExtensions checks if the paths are identical excluding extension diffs across operations (e.g. x-xgen-soa-migration). func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, error) { // If the diff only has extensions diff, then we consider the paths to be identical - customConfig := o.config.WithExcludeElements([]string{"extensions"}) + customConfig := diff.NewConfig().WithExcludeElements([]string{"extensions"}) result, err := o.GetDiffWithConfig(o.base, o.external, customConfig) if err != nil { return false, err diff --git a/tools/cli/internal/openapi/oasdiff_test.go b/tools/cli/internal/openapi/oasdiff_test.go index 3e5ef3f998..e3e1691e51 100644 --- a/tools/cli/internal/openapi/oasdiff_test.go +++ b/tools/cli/internal/openapi/oasdiff_test.go @@ -1335,6 +1335,12 @@ func TestHandlePathConflict(t *testing.T) { Return(tc.specDiff, nil). AnyTimes() + mockDiffGetter. + EXPECT(). + GetWithOperationsSourcesMap(o.config, o.base.Spec, o.external.Spec). + Return(tc.specDiff, nil). + AnyTimes() + err := o.handlePathConflict(tc.basePath, tc.basePathName) if tc.expectedError != nil { require.Error(t, err) From 354f4d9db5b510c62ba8ef3771a56e70dcc55415 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 12:57:00 -0300 Subject: [PATCH 19/21] Update --- tools/cli/internal/openapi/oasdiff.go | 2 +- tools/cli/internal/openapi/oasdiff_result.go | 25 +++++++++++++++----- tools/cli/internal/openapi/openapi.go | 8 ++++--- 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 41fd224796..a3ee835cc5 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.go @@ -30,7 +30,7 @@ type OasDiff struct { base *load.SpecInfo external *load.SpecInfo config *diff.Config - diffGetter DiffGetter + diffGetter Differ result *OasDiffResult parser Parser } diff --git a/tools/cli/internal/openapi/oasdiff_result.go b/tools/cli/internal/openapi/oasdiff_result.go index ba0266bdb5..aca44f1c05 100644 --- a/tools/cli/internal/openapi/oasdiff_result.go +++ b/tools/cli/internal/openapi/oasdiff_result.go @@ -22,6 +22,25 @@ import ( "github.com/tufin/oasdiff/load" ) +// DiffGetter defines an interface for getting diffs. +type Differ interface { + Get(config *diff.Config, base, revision *openapi3.T) (*diff.Diff, error) + GetWithOperationsSourcesMap(config *diff.Config, base, revision *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) +} + +type ResultGetter struct{} + +func NewResultGetter() Differ { + return &ResultGetter{} +} + +func (_ ResultGetter) Get(config *diff.Config, base, revision *openapi3.T) (*diff.Diff, error) { + return diff.Get(config, base, revision) +} +func (_ ResultGetter) GetWithOperationsSourcesMap(config *diff.Config, base, revision *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) { + return diff.GetWithOperationsSourcesMap(config, base, revision) +} + type OasDiffResult struct { Report *diff.Diff SourceMap *diff.OperationsSourcesMap @@ -29,12 +48,6 @@ type OasDiffResult struct { Config *diff.Config } -// DiffGetter defines an interface for getting diffs. -type DiffGetter interface { - Get(config *diff.Config, base, revision *openapi3.T) (*diff.Diff, error) - GetWithOperationsSourcesMap(config *diff.Config, base, revision *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) -} - // GetSimpleDiff returns the diff between two OpenAPI specs. func (o OasDiff) GetSimpleDiff(base, revision *load.SpecInfo) (*OasDiffResult, error) { diffReport, err := o.diffGetter.Get(o.config, base.Spec, revision.Spec) diff --git a/tools/cli/internal/openapi/openapi.go b/tools/cli/internal/openapi/openapi.go index 30c193172d..4002c46e5e 100644 --- a/tools/cli/internal/openapi/openapi.go +++ b/tools/cli/internal/openapi/openapi.go @@ -83,14 +83,16 @@ func NewOasDiff(base string, excludePrivatePaths bool) (*OasDiff, error) { config: &diff.Config{ IncludePathParams: true, }, + diffGetter: NewResultGetter(), }, nil } func NewOasDiffWithSpecInfo(base, external *load.SpecInfo, config *diff.Config) *OasDiff { return &OasDiff{ - base: base, - external: external, - config: config, + base: base, + external: external, + config: config, + diffGetter: NewResultGetter(), } } From d78e2c231be9c7e623f9ec435de0232bf9cd5ed9 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 13:02:39 -0300 Subject: [PATCH 20/21] Update --- tools/cli/internal/openapi/oasdiff_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff_test.go b/tools/cli/internal/openapi/oasdiff_test.go index e3e1691e51..bf44ffa6bc 100644 --- a/tools/cli/internal/openapi/oasdiff_test.go +++ b/tools/cli/internal/openapi/oasdiff_test.go @@ -1327,17 +1327,18 @@ func TestHandlePathConflict(t *testing.T) { result: &OasDiffResult{ Report: tc.specDiff, }, + diffGetter: mockDiffGetter, } mockDiffGetter. EXPECT(). - Get(o.config, o.base.Spec, o.external.Spec). - Return(tc.specDiff, nil). + GetWithOperationsSourcesMap(o.config, o.base.Spec, o.external.Spec). + Return(tc.specDiff, nil, nil). AnyTimes() mockDiffGetter. EXPECT(). - GetWithOperationsSourcesMap(o.config, o.base.Spec, o.external.Spec). + Get(gomock.Any(), o.base.Spec, o.external.Spec). Return(tc.specDiff, nil). AnyTimes() From a97ba4e9419dbdd24cdc772c0e3ccd15f2ffbf05 Mon Sep 17 00:00:00 2001 From: Bianca Lisle Date: Fri, 11 Oct 2024 13:05:14 -0300 Subject: [PATCH 21/21] Lint --- tools/cli/internal/openapi/oasdiff_result.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/cli/internal/openapi/oasdiff_result.go b/tools/cli/internal/openapi/oasdiff_result.go index aca44f1c05..ef63c24fa2 100644 --- a/tools/cli/internal/openapi/oasdiff_result.go +++ b/tools/cli/internal/openapi/oasdiff_result.go @@ -34,10 +34,11 @@ func NewResultGetter() Differ { return &ResultGetter{} } -func (_ ResultGetter) Get(config *diff.Config, base, revision *openapi3.T) (*diff.Diff, error) { +func (ResultGetter) Get(config *diff.Config, base, revision *openapi3.T) (*diff.Diff, error) { return diff.Get(config, base, revision) } -func (_ ResultGetter) GetWithOperationsSourcesMap(config *diff.Config, base, revision *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) { +func (ResultGetter) GetWithOperationsSourcesMap( + config *diff.Config, base, revision *load.SpecInfo) (*diff.Diff, *diff.OperationsSourcesMap, error) { return diff.GetWithOperationsSourcesMap(config, base, revision) }