Skip to content

Commit

Permalink
(feature) Add optional checker for enum removal (#210)
Browse files Browse the repository at this point in the history
* Add enum optional checkers

* Improve property check

* Minor fixes

* Address comments
  • Loading branch information
blva authored Mar 31, 2023
1 parent 6ce0ee2 commit 85ef2ab
Show file tree
Hide file tree
Showing 13 changed files with 372 additions and 18 deletions.
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,10 @@ If you don't use the `x-extensible-enum` in your OpenAPI specifications, nothing
You can use the `-include-checks` flag to include the following optional backwards compatibility checks:
- response-non-success-status-removed
- api-operation-id-removed
- api-tag-removed
- response-property-enum-value-removed
- response-mediatype-enum-value-removed
- request-body-enum-value-removed

### Advantages of the New Breaking Changes Method
- output is human readable
Expand Down
30 changes: 16 additions & 14 deletions breaking-changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
These examples are automatically generated from unit tests.
## Examples of breaking changes
[adding a new required property in request body is breaking](checker/checker_breaking_property_test.go?plain=1#L194)
[adding a pattern to a schema is breaking for recursive properties](checker/checker_breaking_test.go?plain=1#L409)
[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L375)
[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L391)
[adding a pattern to a schema is breaking for recursive properties](checker/checker_breaking_test.go?plain=1#L467)
[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L433)
[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L449)
[adding a required request body is breaking](checker/checker_breaking_test.go?plain=1#L61)
[changing a required property in response body to optional and also deleting it is breaking](checker/checker_breaking_property_test.go?plain=1#L122)
[changing an existing header param from optional to required is breaking](checker/checker_breaking_test.go?plain=1#L163)
Expand All @@ -16,7 +16,7 @@ These examples are automatically generated from unit tests.
[changing an existing response header from required to optional is breaking](checker/checker_breaking_test.go?plain=1#L187)
[changing max length in request from nil to any value is breaking](checker/checker_breaking_min_max_test.go?plain=1#L110)
[changing max length in response from any value to nil is breaking](checker/checker_breaking_min_max_test.go?plain=1#L160)
[deleting a media-type from response is breaking](checker/checker_breaking_test.go?plain=1#L345)
[deleting a media-type from response is breaking](checker/checker_breaking_test.go?plain=1#L403)
[deleting a path is breaking](checker/checker_breaking_test.go?plain=1#L39)
[deleting a path with some operations having sunset date in the future is breaking](checker/checker_deprecation_test.go?plain=1#L251)
[deleting a required property in request is breaking with warn](checker/checker_breaking_property_test.go?plain=1#L210)
Expand All @@ -30,23 +30,25 @@ These examples are automatically generated from unit tests.
[deprecating an operation with a deprecation policy but without specifying sunset date is breaking](checker/checker_deprecation_test.go?plain=1#L68)
[increasing max length in response is breaking](checker/checker_breaking_min_max_test.go?plain=1#L93)
[increasing min items in request is breaking](checker/checker_breaking_min_max_test.go?plain=1#L236)
[modifying a pattern in a schema is breaking](checker/checker_breaking_test.go?plain=1#L425)
[modifying a pattern in request parameter is breaking](checker/checker_breaking_test.go?plain=1#L441)
[modifying the default value of an optional request parameter is breaking](checker/checker_breaking_test.go?plain=1#L471)
[modifying a pattern in a schema is breaking](checker/checker_breaking_test.go?plain=1#L483)
[modifying a pattern in request parameter is breaking](checker/checker_breaking_test.go?plain=1#L499)
[modifying the default value of an optional request parameter is breaking](checker/checker_breaking_test.go?plain=1#L529)
[new required header param is breaking](checker/checker_breaking_test.go?plain=1#L147)
[new required path param is breaking](checker/checker_breaking_test.go?plain=1#L131)
[new required property in request header is breaking](checker/checker_breaking_property_test.go?plain=1#L17)
[reducing max in request is breaking](checker/checker_breaking_min_max_test.go?plain=1#L264)
[reducing max length in request is breaking](checker/checker_breaking_min_max_test.go?plain=1#L12)
[reducing min items in response is breaking](checker/checker_breaking_min_max_test.go?plain=1#L220)
[reducing min length in response is breaking](checker/checker_breaking_min_max_test.go?plain=1#L62)
[removing an existing optional response header is breaking as warn](checker/checker_breaking_test.go?plain=1#L326)
[removing an existing optional response header is breaking as warn](checker/checker_breaking_test.go?plain=1#L384)
[removing an existing required response header is breaking as error](checker/checker_breaking_test.go?plain=1#L203)
[removing an existing response with non-successful status is breaking (optional)](checker/checker_breaking_test.go?plain=1#L240)
[removing an existing response with successful status is breaking](checker/checker_breaking_test.go?plain=1#L222)
[removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not alpha stability level](checker/checker_deprecation_test.go?plain=1#L121)
[removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not draft stability level](checker/checker_deprecation_test.go?plain=1#L173)
[removing/updating a tag is breaking (optional)](checker/checker_breaking_test.go?plain=1#L276)
[removing/updating a property enum in response is breaking (optional)](checker/checker_breaking_test.go?plain=1#L298)
[removing/updating a tag is breaking (optional)](checker/checker_breaking_test.go?plain=1#L315)
[removing/updating an enum in request body is breaking (optional)](checker/checker_breaking_test.go?plain=1#L276)
[removing/updating an operation id is breaking (optional)](checker/checker_breaking_test.go?plain=1#L258)

## Examples of non-breaking changes
Expand Down Expand Up @@ -78,7 +80,7 @@ These examples are automatically generated from unit tests.
[changing servers is not breaking](checker/checker_not_breaking_test.go?plain=1#L233)
[deleting a non-required non-write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L356)
[deleting a path after sunset date of all contained operations is not breaking](checker/checker_deprecation_test.go?plain=1#L236)
[deleting a pattern from a schema is not breaking](checker/checker_breaking_test.go?plain=1#L361)
[deleting a pattern from a schema is not breaking](checker/checker_breaking_test.go?plain=1#L419)
[deleting a required write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L339)
[deleting a tag is not breaking](checker/checker_not_breaking_test.go?plain=1#L54)
[deleting an operation after sunset date is not breaking](checker/checker_deprecation_test.go?plain=1#L53)
Expand All @@ -91,8 +93,8 @@ These examples are automatically generated from unit tests.
[deprecating an operation without a deprecation policy and without specifying sunset date is not breaking](checker/checker_deprecation_test.go?plain=1#L87)
[increasing max length in request is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L76)
[increasing min items in response is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L250)
[modifying a pattern to ".*"" in a schema is not breaking](checker/checker_breaking_test.go?plain=1#L457)
[modifying the default value of a required request parameter is not breaking](checker/checker_breaking_test.go?plain=1#L487)
[modifying a pattern to ".*"" in a schema is not breaking](checker/checker_breaking_test.go?plain=1#L515)
[modifying the default value of a required request parameter is not breaking](checker/checker_breaking_test.go?plain=1#L545)
[new optional header param is not breaking](checker/checker_not_breaking_test.go?plain=1#L102)
[new optional property in request header is not breaking](checker/checker_breaking_property_test.go?plain=1#L38)
[new required response header param is not breaking](checker/checker_not_breaking_test.go?plain=1#L134)
Expand All @@ -101,7 +103,7 @@ These examples are automatically generated from unit tests.
[reducing max length in response is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L31)
[reducing min items in request is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L206)
[reducing min length in request is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L48)
[removing an existing response with error status is not breaking](checker/checker_breaking_test.go?plain=1#L310)
[removing an existing response with unparseable status is not breaking](checker/checker_breaking_test.go?plain=1#L294)
[removing an existing response with error status is not breaking](checker/checker_breaking_test.go?plain=1#L368)
[removing an existing response with unparseable status is not breaking](checker/checker_breaking_test.go?plain=1#L352)
[removing the path without a deprecation policy and without specifying sunset date is not breaking for alpha level](checker/checker_deprecation_test.go?plain=1#L102)
[removing the path without a deprecation policy and without specifying sunset date is not breaking for draft level](checker/checker_deprecation_test.go?plain=1#L154)
57 changes: 57 additions & 0 deletions checker/check-request-body-enum-deleted.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package checker

import (
"fmt"

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

const requestBodyEnumRemovedId = "request-body-enum-value-removed"

func RequestBodyEnumValueRemovedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
result := make([]BackwardCompatibilityError, 0)
if diffReport.PathsDiff == nil {
return result
}
for path, pathItem := range diffReport.PathsDiff.Modified {
if pathItem.OperationsDiff == nil {
continue
}
for operation, operationItem := range pathItem.OperationsDiff.Modified {
if operationItem.RequestBodyDiff == nil {
continue
}
if operationItem.RequestBodyDiff.ContentDiff == nil {
continue
}
if operationItem.RequestBodyDiff.ContentDiff.MediaTypeModified == nil {
continue
}

mediaTypeChanges := operationItem.RequestBodyDiff.ContentDiff.MediaTypeModified

source := (*operationsSources)[operationItem.Revision]
for _, mediaTypeItem := range mediaTypeChanges {
if mediaTypeItem.SchemaDiff == nil {
continue
}

enumDiff := mediaTypeItem.SchemaDiff.EnumDiff
if enumDiff == nil || enumDiff.Deleted == nil {
continue
}
for _, enumVal := range enumDiff.Deleted {
result = append(result, BackwardCompatibilityError{
Id: requestBodyEnumRemovedId,
Level: ERR,
Text: fmt.Sprintf(config.i18n(requestBodyEnumRemovedId), ColorizedValue(enumVal)),
Operation: operation,
Path: path,
Source: source,
})
}
}
}
}
return result
}
56 changes: 56 additions & 0 deletions checker/check-response-mediatype-enum-value-removed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package checker

import (
"fmt"

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

const responseMediatypeEnumValueRemovedId = "response-mediatype-enum-value-removed"

func ResponseMediaTypeEnumValueRemovedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
result := make([]BackwardCompatibilityError, 0)
if diffReport.PathsDiff == nil {
return result
}
for path, pathItem := range diffReport.PathsDiff.Modified {
if pathItem.OperationsDiff == nil {
continue
}
for operation, operationItem := range pathItem.OperationsDiff.Modified {
if operationItem.ResponsesDiff == nil {
continue
}
if operationItem.ResponsesDiff.Modified == nil {
continue
}
source := (*operationsSources)[operationItem.Revision]
for _, responseItems := range operationItem.ResponsesDiff.Modified {
for mediaType, mediaTypeItem := range responseItems.ContentDiff.MediaTypeModified {
if mediaTypeItem.SchemaDiff == nil {
continue
}

enumDiff := mediaTypeItem.SchemaDiff.EnumDiff
if enumDiff == nil {
continue
}

for _, enumVal := range enumDiff.Deleted {
result = append(result, BackwardCompatibilityError{
Id: responseMediatypeEnumValueRemovedId,
Level: ERR,
Text: fmt.Sprintf(config.i18n(responseMediatypeEnumValueRemovedId), mediaType, ColorizedValue(enumVal)),
Operation: operation,
Path: path,
Source: source,
})
}

}

}
}
}
return result
}
58 changes: 58 additions & 0 deletions checker/check-response-property-enum-value-removed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package checker

import (
"fmt"

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

const responsePropertyEnumValueRemovedId = "response-property-enum-value-removed"

func ResponseParameterEnumValueRemovedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
result := make([]BackwardCompatibilityError, 0)
if diffReport.PathsDiff == nil {
return result
}
for path, pathItem := range diffReport.PathsDiff.Modified {
if pathItem.OperationsDiff == nil {
continue
}
for operation, operationItem := range pathItem.OperationsDiff.Modified {
if operationItem.ResponsesDiff == nil || operationItem.ResponsesDiff.Modified == nil {
continue
}

source := (*operationsSources)[operationItem.Revision]
for responseStatus, responseDiff := range operationItem.ResponsesDiff.Modified {
if responseDiff == nil ||
responseDiff.ContentDiff == nil ||
responseDiff.ContentDiff.MediaTypeModified == nil {
continue
}
for _, mediaTypeDiff := range responseDiff.ContentDiff.MediaTypeModified {
CheckModifiedPropertiesDiff(
mediaTypeDiff.SchemaDiff,
func(propertyPath string, propertyName string, propertyDiff *diff.SchemaDiff, parent *diff.SchemaDiff) {
enumDiff := propertyDiff.EnumDiff
if enumDiff == nil || enumDiff.Deleted == nil {
return
}

for _, enumVal := range enumDiff.Deleted {
result = append(result, BackwardCompatibilityError{
Id: responsePropertyEnumValueRemovedId,
Level: ERR,
Text: fmt.Sprintf(config.i18n(responsePropertyEnumValueRemovedId), enumVal, ColorizedValue(propertyFullName(propertyPath, propertyName)), ColorizedValue(responseStatus)),
Operation: operation,
Path: path,
Source: source,
})
}
})
}

}
}
}
return result
}
58 changes: 58 additions & 0 deletions checker/checker_breaking_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,45 @@ func TestBreaking_OperationIdRemoved(t *testing.T) {
require.Equal(t, "api-operation-id-removed", errs[0].Id)
}

// BC: removing/updating an enum in request body is breaking (optional)
func TestBreaking_RequestBodyEnumRemoved(t *testing.T) {
s1, err := checker.LoadOpenAPISpecInfoFromFile("../data/enums/request-body-enum.yaml")
require.NoError(t, err)

s2, err := checker.LoadOpenAPISpecInfoFromFile("../data/enums/request-body-enum.yaml")
require.NoError(t, err)

s2.Spec.Paths["/api/v2/changeOfRequestFieldValueTiedToEnumTest"].Get.RequestBody.Value.Content["application/json"].Schema.Value.Enum = []interface{}{}

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

errs := checker.CheckBackwardCompatibility(checker.GetChecks(utils.StringList{"request-body-enum-value-removed"}), d, osm)
for _, err := range errs {
require.Equal(t, checker.ERR, err.Level)
}

require.Len(t, errs, 3)
require.Equal(t, "request-body-enum-value-removed", errs[0].Id)
}

// BC: removing/updating a property enum in response is breaking (optional)
func TestBreaking_ResponsePropertyEnumRemoved(t *testing.T) {
s1 := l(t, 704)
s2 := l(t, 703)

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

errs := checker.CheckBackwardCompatibility(checker.GetChecks(utils.StringList{"response-property-enum-value-removed"}), d, osm)
for _, err := range errs {
require.Equal(t, checker.ERR, err.Level)
}
require.NotEmpty(t, errs)
require.Len(t, errs, 2)
require.Equal(t, "response-property-enum-value-removed", errs[0].Id)
}

// BC: removing/updating a tag is breaking (optional)
func TestBreaking_TagRemoved(t *testing.T) {
s1 := l(t, 1)
Expand All @@ -291,6 +330,25 @@ func TestBreaking_TagRemoved(t *testing.T) {
require.Equal(t, "api-tag-removed", errs[0].Id)
}

// BC: removing/updating a media type enum in response (optional)
func TestBreaking_ResponseMediaTypeEnumRemoved(t *testing.T) {
s1, err := checker.LoadOpenAPISpecInfoFromFile("../data/enums/response-enum.yaml")
require.NoError(t, err)

s2, err := checker.LoadOpenAPISpecInfoFromFile("../data/enums/response-enum-2.yaml")
require.NoError(t, err)

d, osm, err := diff.GetWithOperationsSourcesMap(&diff.Config{}, s1, s2)
require.NoError(t, err)
errs := checker.CheckBackwardCompatibility(checker.GetChecks(utils.StringList{"response-mediatype-enum-value-removed"}), d, osm)
for _, err := range errs {
require.Equal(t, checker.ERR, err.Level)
}
require.NotEmpty(t, errs)
require.Len(t, errs, 1)
require.Equal(t, "response-mediatype-enum-value-removed", errs[0].Id)
}

// BC: removing an existing response with unparseable status is not breaking
func TestBreaking_ResponseUnparseableStatusRemoved(t *testing.T) {
s1 := l(t, 1)
Expand Down
9 changes: 6 additions & 3 deletions checker/default_checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,12 @@ func GetChecks(includeChecks utils.StringList) BackwardCompatibilityCheckConfig
}

var optionalChecks = map[string]BackwardCompatibilityCheck{
"response-non-success-status-removed": ResponseNonSuccessStatusRemoved,
"api-operation-id-removed": APIOperationIdRemovedCheck,
"api-tag-removed": APITagRemovedCheck,
"response-non-success-status-removed": ResponseNonSuccessStatusRemoved,
"api-operation-id-removed": APIOperationIdRemovedCheck,
"api-tag-removed": APITagRemovedCheck,
"response-property-enum-value-removed": ResponseParameterEnumValueRemovedCheck,
"response-mediatype-enum-value-removed": ResponseMediaTypeEnumValueRemovedCheck,
"request-body-enum-value-removed": RequestBodyEnumValueRemovedCheck,
}

func ValidateIncludeChecks(includeChecks utils.StringList) utils.StringList {
Expand Down
Loading

0 comments on commit 85ef2ab

Please sign in to comment.