Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking change check: New request required default param on existing path #376

Merged
merged 7 commits into from
Sep 16, 2023
Merged

Breaking change check: New request required default param on existing path #376

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d2a2497
add a check for new required default parameter for a request on an ex…
Jonathan-Rosenberg Sep 11, 2023
e3b94fc
remove unnecessary printing loop
Jonathan-Rosenberg Sep 11, 2023
2cba695
rename NewNonPathRequestDefaultParameterCheck to NewRequestNonPathDef…
Jonathan-Rosenberg Sep 11, 2023
6b311d7
add Revision nil check
Jonathan-Rosenberg Sep 14, 2023
4571825
add a check for empty path operations
Jonathan-Rosenberg Sep 14, 2023
949bb2e
report BC error per endpoint
Jonathan-Rosenberg Sep 14, 2023
6d0cd01
Merge branch 'feature/breaking-change/new-required-request-default-pa…
Sep 15, 2023
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
31 changes: 20 additions & 11 deletions checker/check-new-request-non-path-default-parameter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"github.com/tufin/oasdiff/diff"
)

func NewRequestNonPathDefaultParameterCheck(diffReport *diff.Diff, _ *diff.OperationsSourcesMap, config Config) Changes {
func NewRequestNonPathDefaultParameterCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config Config) Changes {
result := make(Changes, 0)
if diffReport.PathsDiff == nil || len(diffReport.PathsDiff.Modified) == 0 {
return result
Expand All @@ -13,26 +13,35 @@ func NewRequestNonPathDefaultParameterCheck(diffReport *diff.Diff, _ *diff.Opera
if pathItem.ParametersDiff == nil || pathItem.Revision == nil {
continue
}
for paramLoc, paramNameList := range pathItem.ParametersDiff.Added {

for paramLoc := range pathItem.ParametersDiff.Added {
if paramLoc == "path" {
continue
}

for _, param := range pathItem.Revision.Parameters {
reuvenharrison marked this conversation as resolved.
Show resolved Hide resolved
if !paramNameList.Contains(param.Value.Name) {
continue
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The removal of this code makes the check incorrect.
The level will be ERR even if the parameter wasn't added.
I'm creating a fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was under the impression that it is redundant because we are iterating on pathItem.ParametersDiff.Added
Can you replicate the issue with a unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do that, but since the pathItem.Revision.Parameters field is a slice and not a map, we traverse through it to find the ParameterRef object that matches the name we found in the Added map.
PR.

id := "new-required-request-default-parameter-to-existing-path"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still need to be implemented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll add a new issue to handle this

level := ERR
if !param.Value.Required {
id = "new-optional-request-default-parameter-to-existing-path"
level = INFO
}
result = append(result, ApiChange{
Id: id,
Level: level,
Text: config.Localize(id, ColorizedValue(paramLoc), ColorizedValue(param.Value.Name)),
Path: path,
})

for operation, operationItem := range pathItem.Revision.Operations() {

// TODO: if base operation had this param individually (not through the path) - continue

result = append(result, ApiChange{
Id: id,
Level: level,
Text: config.Localize(id, ColorizedValue(paramLoc), ColorizedValue(param.Value.Name)),
Operation: operation,
OperationId: operationItem.OperationID,
Path: path,
Source: (*operationsSources)[operationItem],
})
}
}
}
}
Expand Down
128 changes: 92 additions & 36 deletions checker/check-new-request-non-path-default-parameter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,41 +21,97 @@ func TestNewRequestNonPathParameter_DetectsNewRequiredPathsAndNewOperations(t *t

errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.NewRequestNonPathDefaultParameterCheck), d, osm, checker.INFO)
require.NotEmpty(t, errs)
require.Len(t, errs, 5)

require.IsType(t, checker.ApiChange{}, errs[0])
e0 := errs[0].(checker.ApiChange)
require.Equal(t, "new-required-request-default-parameter-to-existing-path", e0.Id)
require.Equal(t, "/api/test1", e0.Path)
require.Equal(t, checker.ERR, e0.Level)
require.Contains(t, e0.Text, "version")

require.IsType(t, checker.ApiChange{}, errs[1])
e1 := errs[1].(checker.ApiChange)
require.Equal(t, "new-required-request-default-parameter-to-existing-path", e1.Id)
require.Equal(t, "/api/test2", e1.Path)
require.Equal(t, checker.ERR, e1.Level)
require.Contains(t, e1.Text, "id")

require.IsType(t, checker.ApiChange{}, errs[2])
e2 := errs[2].(checker.ApiChange)
require.Equal(t, "new-required-request-default-parameter-to-existing-path", e2.Id)
require.Equal(t, "/api/test3", e2.Path)
require.Equal(t, checker.ERR, e2.Level)
require.Contains(t, e2.Text, "If-None-Match")

require.IsType(t, checker.ApiChange{}, errs[3])
e3 := errs[3].(checker.ApiChange)
require.Equal(t, "new-optional-request-default-parameter-to-existing-path", e3.Id)
require.Equal(t, "/api/test1", e3.Path)
require.Equal(t, checker.INFO, e3.Level)
require.Contains(t, e3.Text, "optionalQueryParam")

require.IsType(t, checker.ApiChange{}, errs[4])
e4 := errs[4].(checker.ApiChange)
require.Equal(t, "new-optional-request-default-parameter-to-existing-path", e4.Id)
require.Equal(t, "/api/test2", e4.Path)
require.Equal(t, checker.INFO, e4.Level)
require.Contains(t, e4.Text, "optionalHeaderParam")
require.Len(t, errs, 9)

require.ElementsMatch(t, []checker.ApiChange{
{
Id: "new-required-request-default-parameter-to-existing-path",
Text: "added the new required 'query' request parameter 'version' to all path's operations",
Comment: "",
Level: 3,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test1",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-required-request-default-parameter-to-existing-path",
Text: "added the new required 'query' request parameter 'version' to all path's operations",
Comment: "",
Level: 3,
Operation: "POST",
OperationId: "",
Path: "/api/test1",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-required-request-default-parameter-to-existing-path",
Text: "added the new required 'header' request parameter 'id' to all path's operations",
Comment: "",
Level: 3,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test2",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-required-request-default-parameter-to-existing-path",
Text: "added the new required 'query' request parameter 'id' to all path's operations",
Comment: "",
Level: 3,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test2",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-required-request-default-parameter-to-existing-path",
Text: "added the new required 'header' request parameter 'If-None-Match' to all path's operations",
Comment: "",
Level: 3,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test3",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-optional-request-default-parameter-to-existing-path",
Text: "added the new optional 'query' request parameter 'optionalQueryParam' to all path's operations",
Comment: "",
Level: 1,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test1",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-optional-request-default-parameter-to-existing-path",
Text: "added the new optional 'query' request parameter 'optionalQueryParam' to all path's operations",
Comment: "",
Level: 1,
Operation: "POST",
OperationId: "",
Path: "/api/test1",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-optional-request-default-parameter-to-existing-path",
Text: "added the new optional 'header' request parameter 'optionalHeaderParam' to all path's operations",
Comment: "",
Level: 1,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test2",
Source: "../data/request_params/required-request-params.yaml",
},
{
Id: "new-optional-request-default-parameter-to-existing-path",
Text: "added the new optional 'query' request parameter 'optionalHeaderParam' to all path's operations",
Comment: "",
Level: 1,
Operation: "GET",
OperationId: "getTest",
Path: "/api/test2",
Source: "../data/request_params/required-request-params.yaml",
}}, errs)
}
9 changes: 9 additions & 0 deletions data/request_params/base.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,12 @@ paths:
responses:
200:
description: OK

/api/test4:
parameters:
- in: query
name: emptyPath
required: false
schema:
type: string

8 changes: 8 additions & 0 deletions data/request_params/required-request-params.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,11 @@ paths:
responses:
200:
description: OK

/api/test4:
parameters:
- in: query
name: emptyPath2
required: true
schema:
type: string