Skip to content

Commit

Permalink
224 changing a request property to enum should be breaking (#230)
Browse files Browse the repository at this point in the history
* added unit test

* initial checker

* changing a request property to enum is breaking

* adding an enum value is not breaking

* changing type and adding enum simultaneously

* RequestParameterBecameEnumCheck

* RequestHeaderPropertyBecameEnumCheck

* doc

* russian messages

* seperate checks
  • Loading branch information
Reuven Harrison authored Apr 30, 2023
1 parent 53093d6 commit c3bbf82
Show file tree
Hide file tree
Showing 24 changed files with 665 additions and 40 deletions.
50 changes: 28 additions & 22 deletions BREAKING-CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,30 +1,35 @@
# Examples of Breaking and Non-Breaking Changes
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#L242)
[adding a new required property in request body is breaking](checker/checker_breaking_property_test.go?plain=1#L336)
[adding a pattern to a schema is breaking for recursive properties](checker/checker_breaking_test.go?plain=1#L469)
[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L453)
[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#L170)
[changing a response body to nullable is breaking](checker/checker_breaking_property_test.go?plain=1#L122)
[changing a response property to nullable is breaking](checker/checker_breaking_property_test.go?plain=1#L138)
[changing an embedded reponse property to nullable is breaking](checker/checker_breaking_property_test.go?plain=1#L154)
[changing a request body to enum is breaking](checker/checker_breaking_property_test.go?plain=1#L122)
[changing a request body type and changing it to enum simultaneously is breaking](checker/checker_breaking_property_test.go?plain=1#L152)
[changing a required property in response body to optional and also deleting it is breaking](checker/checker_breaking_property_test.go?plain=1#L264)
[changing a response body to nullable is breaking](checker/checker_breaking_property_test.go?plain=1#L216)
[changing a response property to nullable is breaking](checker/checker_breaking_property_test.go?plain=1#L232)
[changing an embedded reponse property to nullable is breaking](checker/checker_breaking_property_test.go?plain=1#L248)
[changing an existing header param from optional to required is breaking](checker/checker_breaking_test.go?plain=1#L183)
[changing an existing property in request body to required is breaking](checker/checker_breaking_property_test.go?plain=1#L226)
[changing an existing header param to enum is breaking](checker/checker_breaking_property_test.go?plain=1#L184)
[changing an existing property in request body to enum is breaking](checker/checker_breaking_property_test.go?plain=1#L168)
[changing an existing property in request body to required is breaking](checker/checker_breaking_property_test.go?plain=1#L320)
[changing an existing property in request header to enum is breaking](checker/checker_breaking_property_test.go?plain=1#L200)
[changing an existing property in request header to required is breaking](checker/checker_breaking_property_test.go?plain=1#L56)
[changing an existing property in response body to optional is breaking](checker/checker_breaking_property_test.go?plain=1#L106)
[changing an existing request body from optional to required is breaking](checker/checker_breaking_test.go?plain=1#L78)
[changing an existing required property in response body to not-write-only is breaking](checker/checker_breaking_property_test.go?plain=1#L466)
[changing an existing required property in response body to not-write-only is breaking](checker/checker_breaking_property_test.go?plain=1#L560)
[changing an existing response header from required to optional is breaking](checker/checker_breaking_test.go?plain=1#L207)
[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#L423)
[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#L258)
[deleting a required property in response body is breaking](checker/checker_breaking_property_test.go?plain=1#L306)
[deleting a required property under AllOf in response body is breaking](checker/checker_breaking_property_test.go?plain=1#L336)
[deleting a required property within another property in request is breaking with warn](checker/checker_breaking_property_test.go?plain=1#L275)
[deleting a required property in request is breaking with warn](checker/checker_breaking_property_test.go?plain=1#L352)
[deleting a required property in response body is breaking](checker/checker_breaking_property_test.go?plain=1#L400)
[deleting a required property under AllOf in response body is breaking](checker/checker_breaking_property_test.go?plain=1#L430)
[deleting a required property within another property in request is breaking with warn](checker/checker_breaking_property_test.go?plain=1#L369)
[deleting an enum value is breaking](checker/checker_breaking_test.go?plain=1#L99)
[deleting an operation before sunset date is breaking](checker/checker_deprecation_test.go?plain=1#L19)
[deleting an operation is breaking](checker/checker_breaking_test.go?plain=1#L46)
Expand Down Expand Up @@ -56,35 +61,36 @@ These examples are automatically generated from unit tests.

## Examples of non-breaking changes
[adding a media-type to response is not breaking](checker/checker_not_breaking_test.go?plain=1#L166)
[adding a new required property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L292)
[adding a new required property under AllOf in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L322)
[adding a new required read-only property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L376)
[adding a non-existent required property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L184)
[adding a new required property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L386)
[adding a new required property under AllOf in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L416)
[adding a new required read-only property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L470)
[adding a non-existent required property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L278)
[adding a tag is not breaking with "api-tag-removed" check](checker/checker_not_breaking_test.go?plain=1#L262)
[adding a tag is not breaking](checker/checker_not_breaking_test.go?plain=1#L247)
[adding an enum value is not breaking](checker/checker_not_breaking_test.go?plain=1#L66)
[adding an enum value to request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L138)
[adding an optional request body is not breaking](checker/checker_not_breaking_test.go?plain=1#L20)
[both max lengths in request are nil is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L178)
[both max lengths in response are nil is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L192)
[changing a link to operation ID is not breaking](checker/checker_not_breaking_test.go?plain=1#L157)
[changing an existing header param to optional is not breaking](checker/checker_not_breaking_test.go?plain=1#L116)
[changing an existing property in request body to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L212)
[changing an existing property in request body to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L306)
[changing an existing property in request header to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L82)
[changing an existing property in response body to required is not breaking](checker/checker_breaking_property_test.go?plain=1#L198)
[changing an existing read-only property in request body to required is not breaking](checker/checker_breaking_property_test.go?plain=1#L390)
[changing an existing property in response body to required is not breaking](checker/checker_breaking_property_test.go?plain=1#L292)
[changing an existing read-only property in request body to required is not breaking](checker/checker_breaking_property_test.go?plain=1#L484)
[changing an existing request body from required to optional is not breaking](checker/checker_not_breaking_test.go?plain=1#L35)
[changing an existing required property in response body to write-only is not breaking](checker/checker_breaking_property_test.go?plain=1#L452)
[changing an existing write-only property in response body to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L438)
[changing an existing required property in response body to write-only is not breaking](checker/checker_breaking_property_test.go?plain=1#L546)
[changing an existing write-only property in response body to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L532)
[changing comments is not breaking](checker/checker_not_breaking_test.go?plain=1#L90)
[changing extensions is not breaking](checker/checker_not_breaking_test.go?plain=1#L78)
[changing max length in request from any value to nil is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L144)
[changing max length in response from nil to any value is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L128)
[changing operation ID is not breaking](checker/checker_not_breaking_test.go?plain=1#L148)
[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#L421)
[deleting a non-required non-write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L515)
[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#L439)
[deleting a required write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L404)
[deleting a required write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L498)
[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)
[deprecating a header is not breaking](checker/checker_not_breaking_test.go?plain=1#L207)
Expand Down
2 changes: 1 addition & 1 deletion CONTRIB.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
1. Use oasdiff
- [run from the command-line](README.md#usage-examples)
- embed it in your CI pipeline to check how your OpenAPI spec is evolving and whether any breaking-changes were introduced
- [use the interactive oasdiff calculator](https://www.oasdiff.com/diff-calculator)
- [use the interactive oasdiff calculator](https://www.oasdiff.com/diff-calculator/)
- [use the oasdiff web service](README.md#openapi-diff-and-breaking-changes-as-a-service)
2. Give oasdiff another star
3. Improve the [documentation](README.md)
Expand Down
31 changes: 17 additions & 14 deletions CUSTOMIZING-CHECKS.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,27 @@
# How to Add Custom Breaking-Changes Checks

## Unit Tests and Documentation
1. write a unit test for your scenario and add a comment "BC: \<use-case\> is breaking"
2. optionally, add additional unit tests and comment them with "is breaking" or "is not breaking"
3. Update the breaking-changes examples doc:
```
./scripts/test.sh
```
4. make sure that [BREAKING-CHANGES.md](BREAKING-CHANGES.md) contains your comments
## Unit Test
1. Add a unit test for your scenario in one of the test files under [checker](checker) with a comment "BC: \<use-case\> is breaking"
2. Add any acompanying OpenAPI specs under [data](data)

## Localized Backwards Compatibility Messages
1. add localized texts under [checker/localizations_src](checker/localizations_src) (you can use Google Translate for Russian)
1. Add localized texts under [checker/localizations_src](checker/localizations_src) (you can use Google Translate for Russian)
2. Update [localization source file](checker/localizations/localizations.go):
```
go-localize -input checker/localizations_src -output checker/localizations
```
3. make sure that [checker/localizations/localizations.go](checker/localizations/localizations.go) contains the new messages
3. Make sure that [checker/localizations/localizations.go](checker/localizations/localizations.go) contains the new messages
## Write the Checker Function
1. create new go file under [checker](checker) and name it by the breaking-change use case
2. create a check func inside the file and name it accordingly
3. add the checker func to defaultChecks
4. verify that all unit tests pass
1. Create new go file under [checker](checker) and name it by the breaking-change use case
2. Create a check func inside the file and name it accordingly
3. Add the checker func to the defaultChecks or optionalChecks list
## Documentation
1. Optionally, add additional unit tests and comment them with "BC: \<use-case\> is breaking" or "BC: \<use-case\> is not breaking"
2. Update [BREAKING-CHANGES.md](BREAKING-CHANGES.md):
```
./scripts/test.sh
```
3. Make sure that [BREAKING-CHANGES.md](BREAKING-CHANGES.md) was updated with your use-cases
49 changes: 49 additions & 0 deletions checker/check-request-body-became-enum.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package checker

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

const requestBodyBecameEnumId = "request-body-became-enum"

func RequestBodyBecameEnumCheck(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 {

source := (*operationsSources)[operationItem.Revision]

if operationItem.RequestBodyDiff == nil ||
operationItem.RequestBodyDiff.ContentDiff == nil ||
operationItem.RequestBodyDiff.ContentDiff.MediaTypeModified == nil {
continue
}

modifiedMediaTypes := operationItem.RequestBodyDiff.ContentDiff.MediaTypeModified

for _, mediaTypeDiff := range modifiedMediaTypes {
if mediaTypeDiff.SchemaDiff == nil {
continue
}
if schemaDiff := mediaTypeDiff.SchemaDiff; schemaDiff.EnumDiff == nil || !schemaDiff.EnumDiff.EnumAdded {
continue
}
result = append(result, BackwardCompatibilityError{
Id: requestBodyBecameEnumId,
Level: ERR,
Text: config.i18n(requestBodyBecameEnumId),
Operation: operation,
Path: path,
Source: source,
})
}
}
}
return result
}
71 changes: 71 additions & 0 deletions checker/check-request-header-property-became-enum.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package checker

import (
"fmt"

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

const requestHeaderPropertyBecameEnumId = "request-header-property-became-enum"

func RequestHeaderPropertyBecameEnumCheck(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 {
source := (*operationsSources)[operationItem.Revision]

if operationItem.ParametersDiff == nil {
continue
}

for paramLocation, paramDiffs := range operationItem.ParametersDiff.Modified {

if paramLocation != "header" {
continue
}

for paramName, paramDiff := range paramDiffs {
if paramDiff.SchemaDiff == nil {
continue
}

if paramDiff.SchemaDiff.EnumDiff != nil && paramDiff.SchemaDiff.EnumDiff.EnumAdded {
result = append(result, BackwardCompatibilityError{
Id: requestHeaderPropertyBecameEnumId,
Level: ERR,
Text: fmt.Sprintf(config.i18n(requestHeaderPropertyBecameEnumId), ColorizedValue(paramName)),
Operation: operation,
Path: path,
Source: source,
})
}

CheckModifiedPropertiesDiff(
paramDiff.SchemaDiff,
func(propertyPath string, propertyName string, propertyDiff *diff.SchemaDiff, parent *diff.SchemaDiff) {

if enumDiff := propertyDiff.EnumDiff; enumDiff == nil || !enumDiff.EnumAdded {
return
}

result = append(result, BackwardCompatibilityError{
Id: requestHeaderPropertyBecameEnumId,
Level: ERR,
Text: fmt.Sprintf(config.i18n(requestHeaderPropertyBecameEnumId), ColorizedValue(paramName), ColorizedValue(propertyFullName(propertyPath, propertyName))),
Operation: operation,
Path: path,
Source: source,
})
})
}
}
}
}
return result
}
51 changes: 51 additions & 0 deletions checker/check-request-parameter-became-enum.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
package checker

import (
"fmt"

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

const requestParameterBecameEnumId = "request-parameter-became-enum"

func RequestParameterBecameEnumCheck(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.ParametersDiff == nil {
continue
}
if operationItem.ParametersDiff.Modified == nil {
continue
}
source := (*operationsSources)[operationItem.Revision]
for paramLocation, paramItems := range operationItem.ParametersDiff.Modified {
for paramName, paramItem := range paramItems {
if paramItem.SchemaDiff == nil {
continue
}

if enumDiff := paramItem.SchemaDiff.EnumDiff; enumDiff == nil || !enumDiff.EnumAdded {
continue
}

result = append(result, BackwardCompatibilityError{
Id: requestParameterBecameEnumId,
Level: ERR,
Text: fmt.Sprintf(config.i18n(requestParameterBecameEnumId), ColorizedValue(paramLocation), ColorizedValue(paramName)),
Operation: operation,
Path: path,
Source: source,
})
}
}
}
}
return result
}
Loading

0 comments on commit c3bbf82

Please sign in to comment.