move path params before diff

Tufin · Feb 1, 2024 · aa4a488 · aa4a488
1 parent 7b00d02
commit aa4a488
Show file tree

Hide file tree

Showing 23 changed files with 261 additions and 143 deletions.
diff --git a/checker/checker_not_breaking_test.go b/checker/checker_not_breaking_test.go
@@ -167,21 +167,19 @@ func TestBreaking_NewRequiredResponseHeader(t *testing.T) {
 // BC: changing operation ID is not breaking
 func TestBreaking_OperationID(t *testing.T) {
 	r := d(t, getConfig(), 3, 1)
-	require.Len(t, r, 4)
+	require.Len(t, r, 3)
 	require.Equal(t, checker.RequestParameterMaxLengthDecreasedId, r[0].GetId())
 	require.Equal(t, checker.RequestParameterEnumValueRemovedId, r[1].GetId())
 	require.Equal(t, checker.RequestParameterPatternAddedId, r[2].GetId())
-	require.Equal(t, checker.RequestParameterRemovedId, r[3].GetId())
 }
 
 // BC: changing a link to operation ID is not breaking
 func TestBreaking_LinkOperationID(t *testing.T) {
 	r := d(t, getConfig(), 3, 1)
-	require.Len(t, r, 4)
+	require.Len(t, r, 3)
 	require.Equal(t, checker.RequestParameterMaxLengthDecreasedId, r[0].GetId())
 	require.Equal(t, checker.RequestParameterEnumValueRemovedId, r[1].GetId())
 	require.Equal(t, checker.RequestParameterPatternAddedId, r[2].GetId())
-	require.Equal(t, checker.RequestParameterRemovedId, r[3].GetId())
 }
 
 // BC: adding a media-type to response is not breaking

diff --git a/data/param-inheritance/path_revision.yaml → data/path-params/no_params.yaml b/data/param-inheritance/path_revision.yaml → data/path-params/no_params.yaml
@@ -5,7 +5,7 @@ info:
   description: Some desc
   contact:
     name: '#onetwothree'
-    url: 'https://test.com22'
+    url: 'https://test.com'
 servers:
   - description: local
     url: 'http://localhost:8080'

diff --git a/data/path-params/params_in_op.yaml b/data/path-params/params_in_op.yaml
@@ -0,0 +1,56 @@
+openapi: 3.0.0
+info:
+  title: Some API
+  version: 1.0.0
+  description: Some desc
+  contact:
+    name: '#onetwothree'
+    url: 'https://test.com'
+servers:
+  - description: local
+    url: 'http://localhost:8080'
+tags:
+  - name: One
+  - name: Two
+paths:
+  '/admin/v0/abc/{id}':
+    get:
+      parameters:
+        - $ref: '#/components/parameters/tenant_id'
+        - $ref: '#/components/parameters/id'  
+      summary: Get abc
+      tags:
+        - Two
+      responses:
+        '200':
+          description: OK
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/abc'
+components:
+  schemas:
+    abc:
+      title: abc
+      x-stoplight:
+        id: lzwt7se3t6ab2
+      type: object
+      properties:
+        details:
+          type: string
+
+  parameters:
+    tenant_id:
+      schema:
+        type: string
+      in: header
+      required: true
+      name: tenant-id
+      description: 'Tenant IDs'
+    id:
+      name: id
+      in: path
+      required: true
+      schema:
+        type: string
+      description: 'The ID'
diff --git a/data/param-inheritance/path_base.yaml → data/path-params/params_in_path.yaml b/data/param-inheritance/path_base.yaml → data/path-params/params_in_path.yaml
diff --git a/diff/diff_test.go b/diff/diff_test.go
@@ -7,6 +7,7 @@ import (
 	"github.com/getkin/kin-openapi/openapi3"
 	"github.com/stretchr/testify/require"
 	"github.com/tufin/oasdiff/diff"
+	"github.com/tufin/oasdiff/flatten/pathparams"
 	"github.com/tufin/oasdiff/load"
 	"github.com/tufin/oasdiff/utils"
 )
@@ -919,14 +920,16 @@ func TestDiff_DifferentComponentSameHeader(t *testing.T) {
 	require.Empty(t, d)
 }
 
-func TestDiff_MovedParam(t *testing.T) {
+func TestDiff_PathParamsDeleted(t *testing.T) {
 	loader := openapi3.NewLoader()
 
-	s1, err := loader.LoadFromFile("../data/param-inheritance/path_base.yaml")
+	s1, err := loader.LoadFromFile("../data/path-params/params_in_path.yaml")
 	require.NoError(t, err)
+	pathparams.Inherit(s1)
 
-	s2, err := loader.LoadFromFile("../data/param-inheritance/path_revision.yaml")
+	s2, err := loader.LoadFromFile("../data/path-params/no_params.yaml")
 	require.NoError(t, err)
+	pathparams.Inherit(s2)
 
 	d, _, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(),
 		&load.SpecInfo{
@@ -936,5 +939,49 @@ func TestDiff_MovedParam(t *testing.T) {
 			Spec: s2,
 		})
 	require.NoError(t, err)
-	require.NotEmpty(t, d.EndpointsDiff)
+	require.NotEmpty(t, d.EndpointsDiff.Modified[diff.Endpoint{Method: "GET", Path: "/admin/v0/abc/{id}"}].ParametersDiff.Deleted)
+}
+
+func TestDiff_PathParamsMoved(t *testing.T) {
+	loader := openapi3.NewLoader()
+
+	s1, err := loader.LoadFromFile("../data/path-params/params_in_path.yaml")
+	require.NoError(t, err)
+	pathparams.Inherit(s1)
+
+	s2, err := loader.LoadFromFile("../data/path-params/params_in_op.yaml")
+	require.NoError(t, err)
+	pathparams.Inherit(s2)
+
+	d, _, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(),
+		&load.SpecInfo{
+			Spec: s1,
+		},
+		&load.SpecInfo{
+			Spec: s2,
+		})
+	require.NoError(t, err)
+	require.Empty(t, d)
+}
+
+func TestDiff_PathParamsAdded(t *testing.T) {
+	loader := openapi3.NewLoader()
+
+	s1, err := loader.LoadFromFile("../data/path-params/no_params.yaml")
+	require.NoError(t, err)
+	pathparams.Inherit(s1)
+
+	s2, err := loader.LoadFromFile("../data/path-params/params_in_path.yaml")
+	require.NoError(t, err)
+	pathparams.Inherit(s2)
+
+	d, _, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(),
+		&load.SpecInfo{
+			Spec: s1,
+		},
+		&load.SpecInfo{
+			Spec: s2,
+		})
+	require.NoError(t, err)
+	require.NotEmpty(t, d.EndpointsDiff.Modified[diff.Endpoint{Method: "GET", Path: "/admin/v0/abc/{id}"}].ParametersDiff.Added)
 }
diff --git a/diff/method_diff.go b/diff/method_diff.go
@@ -36,10 +36,9 @@ func (methodDiff *MethodDiff) Empty() bool {
 	return *methodDiff == MethodDiff{Base: methodDiff.Base, Revision: methodDiff.Revision}
 }
 
-func getMethodDiff(config *Config, state *state, operation1, operation2 *openapi3.Operation, pathParamsMap PathParamsMap,
-	pathParams1, pathParams2 openapi3.Parameters) (*MethodDiff, error) {
+func getMethodDiff(config *Config, state *state, operation1, operation2 *openapi3.Operation, pathParamsMap PathParamsMap) (*MethodDiff, error) {
 
-	diff, err := getMethodDiffInternal(config, state, operation1, operation2, pathParamsMap, pathParams1, pathParams2)
+	diff, err := getMethodDiffInternal(config, state, operation1, operation2, pathParamsMap)
 
 	if err != nil {
 		return nil, err
@@ -52,8 +51,7 @@ func getMethodDiff(config *Config, state *state, operation1, operation2 *openapi
 	return diff, nil
 }
 
-func getMethodDiffInternal(config *Config, state *state, operation1, operation2 *openapi3.Operation, pathParamsMap PathParamsMap,
-	pathParams1, pathParams2 openapi3.Parameters) (*MethodDiff, error) {
+func getMethodDiffInternal(config *Config, state *state, operation1, operation2 *openapi3.Operation, pathParamsMap PathParamsMap) (*MethodDiff, error) {
 
 	result := newMethodDiff()
 	var err error
@@ -63,7 +61,7 @@ func getMethodDiffInternal(config *Config, state *state, operation1, operation2
 	result.SummaryDiff = getValueDiffConditional(config.IsExcludeSummary(), operation1.Summary, operation2.Summary)
 	result.DescriptionDiff = getValueDiffConditional(config.IsExcludeDescription(), operation1.Description, operation2.Description)
 	result.OperationIDDiff = getValueDiff(operation1.OperationID, operation2.OperationID)
-	result.ParametersDiff, err = getParametersDiffByLocation(config, state, paramsInherit(pathParams1, operation1.Parameters), paramsInherit(pathParams2, operation2.Parameters), pathParamsMap)
+	result.ParametersDiff, err = getParametersDiffByLocation(config, state, operation1.Parameters, operation2.Parameters, pathParamsMap)
 	if err != nil {
 		return nil, err
 	}

diff --git a/diff/operations_diff.go b/diff/operations_diff.go
@@ -76,8 +76,7 @@ func getOperationsDiffInternal(config *Config, state *state, pathItemPair *pathI
 	var err error
 
 	for _, op := range operations {
-		err = result.diffOperation(config, state, pathItemPair.PathItem1.GetOperation(op), pathItemPair.PathItem2.GetOperation(op), op, pathItemPair.PathParamsMap,
-			pathItemPair.PathItem1.Parameters, pathItemPair.PathItem2.Parameters)
+		err = result.diffOperation(config, state, pathItemPair.PathItem1.GetOperation(op), pathItemPair.PathItem2.GetOperation(op), op, pathItemPair.PathParamsMap)
 		if err != nil {
 			return nil, err
 		}
@@ -86,8 +85,7 @@ func getOperationsDiffInternal(config *Config, state *state, pathItemPair *pathI
 	return result, nil
 }
 
-func (operationsDiff *OperationsDiff) diffOperation(config *Config, state *state, operation1, operation2 *openapi3.Operation, method string, pathParamsMap PathParamsMap,
-	pathParams1, pathParams2 openapi3.Parameters) error {
+func (operationsDiff *OperationsDiff) diffOperation(config *Config, state *state, operation1, operation2 *openapi3.Operation, method string, pathParamsMap PathParamsMap) error {
 	if operation1 == nil && operation2 == nil {
 		return nil
 	}
@@ -102,7 +100,7 @@ func (operationsDiff *OperationsDiff) diffOperation(config *Config, state *state
 		return nil
 	}
 
-	diff, err := getMethodDiff(config, state, operation1, operation2, pathParamsMap, pathParams1, pathParams2)
+	diff, err := getMethodDiff(config, state, operation1, operation2, pathParamsMap)
 	if err != nil {
 		return err
 	}

diff --git a/diff/params_inherit.go b/diff/params_inherit.go
diff --git a/flatten/allof/doc.go b/flatten/allof/doc.go
@@ -0,0 +1,5 @@
+/*
+Package allof replaces allOf by a merged equivalent
+This is helpful to improve breaking changes accuracy
+*/
+package allof
diff --git a/flatten/merge_allof.go → flatten/allof/merge_allof.go b/flatten/merge_allof.go → flatten/allof/merge_allof.go
@@ -1,4 +1,4 @@
-package flatten
+package allof
 
 import (
 	"errors"

diff --git a/flatten/merge_allof_spec.go → flatten/allof/merge_allof_spec.go b/flatten/merge_allof_spec.go → flatten/allof/merge_allof_spec.go
@@ -1,4 +1,4 @@
-package flatten
+package allof
 
 import (
 	"github.com/getkin/kin-openapi/openapi3"

diff --git a/flatten/merge_allof_spec_test.go → flatten/allof/merge_allof_spec_test.go b/flatten/merge_allof_spec_test.go → flatten/allof/merge_allof_spec_test.go
@@ -1,18 +1,18 @@
-package flatten_test
+package allof_test
 
 import (
 	"testing"
 
 	"github.com/getkin/kin-openapi/openapi3"
 	"github.com/stretchr/testify/require"
-	"github.com/tufin/oasdiff/flatten"
+	"github.com/tufin/oasdiff/flatten/allof"
 )
 
 func Test_MergeSpecOK(t *testing.T) {
 	loader := openapi3.NewLoader()
-	s, err := loader.LoadFromFile("../data/allof/simple.yaml")
+	s, err := loader.LoadFromFile("../../data/allof/simple.yaml")
 	require.NoError(t, err)
-	merged, err := flatten.MergeSpec(s)
+	merged, err := allof.MergeSpec(s)
 	require.NoError(t, err)
 	require.Equal(t, "string", merged.Components.Schemas["GroupView"].Value.Properties["created"].Value.Type)
 	require.Equal(t, "string", merged.Components.Parameters["groupId"].Value.Schema.Value.Properties["prop1"].Value.Type)
@@ -25,9 +25,9 @@ func Test_MergeSpecOK(t *testing.T) {
 
 func Test_MergeSpecInvalid(t *testing.T) {
 	loader := openapi3.NewLoader()
-	s, err := loader.LoadFromFile("../data/allof/invalid.yaml")
+	s, err := loader.LoadFromFile("../../data/allof/invalid.yaml")
 	require.NoError(t, err)
 
-	_, err = flatten.MergeSpec(s)
+	_, err = allof.MergeSpec(s)
 	require.EqualError(t, err, "unable to resolve Type conflict: all Type values must be identical")
 }