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/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_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/oasdiff.go b/tools/cli/internal/openapi/oasdiff.go index 212c88f097..a3ee835cc5 100644 --- a/tools/cli/internal/openapi/oasdiff.go +++ b/tools/cli/internal/openapi/oasdiff.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) } diff --git a/tools/cli/internal/openapi/oasdiff_result.go b/tools/cli/internal/openapi/oasdiff_result.go index 9b7d370c5e..ef63c24fa2 100644 --- a/tools/cli/internal/openapi/oasdiff_result.go +++ b/tools/cli/internal/openapi/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 +} 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 1b5c2caa9e..bf44ffa6bc 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) { @@ -1146,3 +1149,206 @@ 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) + mockDiffGetter := NewMockDiffGetter(ctrl) + o := OasDiff{ + base: &load.SpecInfo{ + Spec: &openapi3.T{ + Paths: &openapi3.Paths{}, + }, + }, + external: &load.SpecInfo{ + Spec: &openapi3.T{ + Paths: &openapi3.Paths{}, + }, + }, + result: &OasDiffResult{ + Report: tc.specDiff, + }, + diffGetter: mockDiffGetter, + } + + mockDiffGetter. + EXPECT(). + GetWithOperationsSourcesMap(o.config, o.base.Spec, o.external.Spec). + Return(tc.specDiff, nil, nil). + AnyTimes() + + mockDiffGetter. + EXPECT(). + Get(gomock.Any(), 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) + assert.IsType(t, tc.expectedError, err) + } else { + assert.NoError(t, err) + } + }) + } +} 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(), } } diff --git a/tools/cli/internal/openapi/paths.go b/tools/cli/internal/openapi/paths.go new file mode 100644 index 0000000000..486da12188 --- /dev/null +++ b/tools/cli/internal/openapi/paths.go @@ -0,0 +1,135 @@ +// 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" + allowDocsDiff = "allowDocsDiff" +) + +// 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 + } + + if basePath.Get != nil { + if result := getOperationExtensionWithName(basePath.Get, extensionName); result == nil { + return false + } + } + + if basePath.Put != nil { + if result := getOperationExtensionWithName(basePath.Put, extensionName); result == nil { + return false + } + } + + if basePath.Post != nil { + if result := getOperationExtensionWithName(basePath.Post, extensionName); result == nil { + return false + } + } + + if basePath.Patch != nil { + if result := getOperationExtensionWithName(basePath.Patch, extensionName); result == nil { + return false + } + } + + if basePath.Delete != nil { + if result := getOperationExtensionWithName(basePath.Delete, extensionName); result == nil { + return false + } + } + + log.Printf("Detected %s annotation in all operations for path: %s\n", extensionName, 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) bool { + if basePath.Operations() == nil || len(basePath.Operations()) == 0 { + return false + } + + 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 "" +} diff --git a/tools/cli/internal/openapi/paths_test.go b/tools/cli/internal/openapi/paths_test.go new file mode 100644 index 0000000000..0c6de879a5 --- /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) + if actual != test.expected { + t.Errorf("Expected %t, got %t", test.expected, actual) + } + }) + } +}