Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion tools/cli/internal/openapi/errors/merge_conflict_error.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@

package errors

import "fmt"
import (
"encoding/json"
"fmt"

"github.com/tufin/oasdiff/diff"
)

type ParamConflictError struct {
Entry string
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you also provide the two files involved in this mismatch? We have a ticket to improve the error message of a conflict to list also the files that are involved to help investigate the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me do this in another task, so that we can update all the error outputs or else this will grow. maybe we can defer to the ticket you mentioned

}

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)
}
9 changes: 4 additions & 5 deletions tools/cli/internal/openapi/filter/mock_filter.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 56 additions & 0 deletions tools/cli/internal/openapi/mock_paths.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

110 changes: 99 additions & 11 deletions tools/cli/internal/openapi/oasdiff.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,18 @@ 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"
)

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name NoExtensionDiff gives fom granted a lot of context on how we use extensions in the spec. Main question from a person without context is what is the difference between specDiff *diff.Diff and noExtDiff NoExtensionDiff?

Question: can we have an interface that includes both specDiff *diff.Diff and noExtDiff NoExtensionDiff as they are both checking differences between specifications

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

starting with decoupling specDiff since we already have an oasdiff result struct in #250

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!

parser Parser
}

type OasDiffResult struct {
Expand Down Expand Up @@ -111,16 +113,18 @@ 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,
err := o.handlePathConflict(originalPathData, path)
if err != nil {
return err
}
basePaths.Set(path, removeExternalRefs(externalPathData))
}
}

o.base.Spec.Paths = basePaths
return nil
}
Expand Down Expand Up @@ -160,6 +164,69 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this comment to add more context: what is allowDocsDiff?, why is not supported? what is the implication in this piece of code?

if allOperationsAllowDocsDiff(basePath) {
return errors.AllowDocsDiffNotSupportedError{
Entry: basePathName,
}
}

d, err := o.noExtDiff.GetPathDiffWithoutExtensions(o.base.Spec, o.external.Spec)
if err != nil {
return err
}

return errors.PathDocsDiffConflictError{
Entry: basePathName,
Diff: d,
}
}

// 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.specDiff != nil {
pathsDiff = o.specDiff.PathsDiff
}

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
// 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)
Expand Down Expand Up @@ -417,6 +484,27 @@ func (o OasDiff) areSchemaIdentical(name string) bool {
return !ok
}

// arePathsIdenticalWithExcludeExtensions checks if the paths are identical with the extensions excluded
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you provide an example of the extension in the comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this runs oasdiff with an "ignore extensions" config, I've updated the comment

func (o OasDiff) arePathsIdenticalWithExcludeExtensions(name string) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code seems to be checking if the path are identical with and without extensions 🤔 is this correct? if yes , we should rename the function to reflect this logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's excluding extensions from the diff since there will always be Extensions, will update!

// 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)
if err != nil {
return false, err
}

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) }
Expand Down
Loading
Loading