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.

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

1 change: 1 addition & 0 deletions BREAKING-CHANGES-EXAMPLES.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ These examples are automatically generated from unit tests.
[modifying a pattern in a schema is breaking](checker/checker_breaking_test.go?plain=1#L491)
[modifying a pattern in request parameter is breaking](checker/checker_breaking_test.go?plain=1#L507)
[modifying the default value of an optional request parameter is breaking](checker/checker_breaking_test.go?plain=1#L537)
[new header, query and cookie required request default param is breaking](checker/check-new-request-non-path-default-parameter_test.go?plain=1#L11)
[new required header param is breaking](checker/checker_breaking_test.go?plain=1#L171)
[new required path param is breaking](checker/checker_breaking_test.go?plain=1#L155)
[new required property in request header is breaking](checker/checker_breaking_property_test.go?plain=1#L17)
Expand Down
40 changes: 40 additions & 0 deletions checker/check-new-request-non-path-default-parameter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package checker

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

func NewRequestNonPathDefaultParameterCheck(diffReport *diff.Diff, _ *diff.OperationsSourcesMap, config Config) Changes {
result := make(Changes, 0)
if diffReport.PathsDiff == nil || len(diffReport.PathsDiff.Modified) == 0 {
return result
}
for path, pathItem := range diffReport.PathsDiff.Modified {
if pathItem.ParametersDiff == nil || pathItem.Revision == nil || len(pathItem.Revision.Operations()) == 0 {
continue
}
for paramLoc, paramNameList := 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
Collaborator

Choose a reason for hiding this comment

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

I suggest to report this error on each affected endpoint rather than once per path.
The reason is that ApiChange is designed to report a breaking change at the endpoint (operation level), for example, it has an operation field.
Another reason is that the breaking change may only apply to a subset of the operations, for exampple, consider moving a param from op level to path level.

The downside to this approach is that the error may be reported multiple times, one for each operation, but this is ok since we will eventually support the ability to group breaking changes (see #371).

}
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,
})
}
}
}
return result
}
61 changes: 61 additions & 0 deletions checker/check-new-request-non-path-default-parameter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package checker_test

import (
"testing"

"github.com/stretchr/testify/require"
"github.com/tufin/oasdiff/checker"
"github.com/tufin/oasdiff/diff"
)

// BC: new header, query and cookie required request default param is breaking
func TestNewRequestNonPathParameter_DetectsNewRequiredPathsAndNewOperations(t *testing.T) {
s1, err := open("../data/request_params/base.yaml")
require.NoError(t, err)

s2, err := open("../data/request_params/required-request-params.yaml")
require.NoError(t, err)

d, osm, err := diff.GetWithOperationsSourcesMap(&diff.Config{}, s1, s2)
require.NoError(t, err)

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")

}
1 change: 1 addition & 0 deletions checker/default_checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func defaultChecks() []BackwardCompatibilityCheck {
APISecurityUpdatedCheck,
APISunsetChangedCheck,
AddedRequiredRequestBodyCheck,
NewRequestNonPathDefaultParameterCheck,
NewRequestNonPathParameterCheck,
NewRequestPathParameterCheck,
NewRequiredRequestHeaderPropertyCheck,
Expand Down
6 changes: 5 additions & 1 deletion checker/localizations/localizations.go

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

2 changes: 2 additions & 0 deletions checker/localizations_src/en/messages.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,5 @@ request-optional-property-became-not-write-only: the request optional property %
request-optional-property-became-not-read-only: the request optional property %s became not read-only
request-required-property-became-write-only: the request required property %s became write-only
request-required-property-became-not-write-only: the request required property %s became not write-only
new-required-request-default-parameter-to-existing-path: added the new required %s request parameter %s to all path's operations
new-optional-request-default-parameter-to-existing-path: added the new optional %s request parameter %s to all path's operations
2 changes: 2 additions & 0 deletions checker/localizations_src/ru/messages.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,5 @@ request-optional-property-became-not-write-only: необязательное п
request-optional-property-became-not-read-only: необязательное поле запроса %s перестало быть только для чтения
request-required-property-became-write-only: обязательное поле запроса %s стало только для записи
request-required-property-became-not-write-only: обязательное поле запроса %s перестало быть только для записи
new-required-request-default-parameter-to-existing-path: добавлен новый обязательный %s параметр запроса %s для всех операций пути
new-optional-request-default-parameter-to-existing-path: добавлен новый необязательный %s параметр запроса %s ко всем операциям пути
9 changes: 9 additions & 0 deletions data/request_params/base.yaml
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

81 changes: 81 additions & 0 deletions data/request_params/required-request-params.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
info:
title: Tufin
version: 1.0.0
openapi: 3.0.3
paths:
/api/test1:
parameters:
- in: query
name: version
required: true
schema:
type: string
- in: query
name: optionalQueryParam
required: false
schema:
type: string
get:
operationId: getTest
parameters:
- in: header
name: X-NewRequestHeaderParam
required: false
schema:
type: string
responses:
200:
description: OK
post:
responses:
201:
description: OK

/api/test2:
parameters:
- in: query
name: id
required: true
schema:
type: string
- in: header
name: optionalHeaderParam
required: false
schema:
type: string
get:
operationId: getTest
parameters:
- in: query
name: newQueryParam
required: false
schema:
type: string
responses:
200:
description: OK

/api/test3:
parameters:
- in: header
name: If-None-Match
required: true
get:
operationId: getTest
parameters:
- in: cookie
name: csrf-token
required: false
schema:
type: string
responses:
200:
description: OK

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