CLOUDP-271223: Skip conflict when x-xgen-soa-migration is set (#243)

Author: blva

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

tools/cli/internal/openapi/filter/mock_filter.go

@@ -22,15 +22,17 @@ 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/load"
 )
 
 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 Differ
+	result     *OasDiffResult
+	parser     Parser
 }
 
 func (o OasDiff) mergeSpecIntoBase() (*load.SpecInfo, error) {
@@ -69,16 +71,17 @@ 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 path, externalPathData := range pathsToMerge.Map() {
+		// Tries to find if the path already exists or not
+		if originalPathData := basePaths.Value(path); originalPathData == nil {
+			basePaths.Set(path, removeExternalRefs(externalPathData))
 		} else {
-			return errors.PathConflictError{
-				Entry: k,
+			if err := o.handlePathConflict(originalPathData, path); err != nil {
+				return err
 			}
+			basePaths.Set(path, removeExternalRefs(externalPathData))
 		}
 	}
-
 	o.base.Spec.Paths = basePaths
 	return nil
 }
@@ -118,6 +121,71 @@ 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,
+		}
+	}
+
+	var pathsAreIdentical bool
+	var err error
+	if pathsAreIdentical, err = o.arePathsIdenticalWithExcludeExtensions(basePathName); err != nil {
+		return err
+	}
+
+	log.Printf("Skipping conflict for path: %s, pathsAreIdentical: %v", basePathName, pathsAreIdentical)
+	if pathsAreIdentical {
+		return nil
+	}
+
+	// allowDocsDiff = true not supported
+	if allOperationsAllowDocsDiff(basePath) {
+		return errors.AllowDocsDiffNotSupportedError{
+			Entry: basePathName,
+		}
+	}
+
+	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.Report,
+	}
+}
+
+// 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 {
+	var pathsDiff *diff.PathsDiff
+	if o.result != nil && o.result.Report != nil && o.result.Report.PathsDiff != nil {
+		pathsDiff = o.result.Report.PathsDiff
+	}
+
+	if pathsDiff != nil && pathsDiff.Modified != nil && pathsDiff.Modified[basePathName] != nil {
+		if ok := pathsDiff.Modified[basePathName].OperationsDiff.Added; !ok.Empty() {
+			return false
+		}
+
+		if ok := 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)
@@ -375,6 +443,29 @@ func (o OasDiff) areSchemaIdentical(name string) bool {
 	return !ok
 }
 
+// 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 := diff.NewConfig().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
+	}
+	v, ok := d.PathsDiff.Modified[name]
+	if ok {
+		if v.Empty() {
+			return true, nil
+		}
+	}
+
+	return !ok, nil
+}
+
 type ByName []*openapi3.Tag
 
 func (a ByName) Len() int           { return len(a) }

tools/cli/internal/openapi/mock_oasdiff_result.go

@@ -14,12 +14,34 @@
 
 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"
 )
 
+// 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,7 +51,7 @@ type OasDiffResult struct {
 
 // 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 +88,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
 	}
@@ -78,3 +100,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 := o.diffGetter.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
+}