changelog: request param required flag changed (#288)

* Add request param required property update checker * Update breaking changes * Update localizations * Lint * Handle info logs * update examples * Extend check backward compatibility to use levels * Update examples * rm "is not breaking" from changelog comments --------- Co-authored-by: Reuven <[email protected]>
Tufin · Jun 8, 2023 · 7ab12e3 · 7ab12e3
1 parent a574235
commit 7ab12e3
Show file tree

Hide file tree

Showing 12 changed files with 113 additions and 39 deletions.
diff --git a/BREAKING-CHANGES-EXAMPLES.md b/BREAKING-CHANGES-EXAMPLES.md
@@ -75,20 +75,19 @@ These examples are automatically generated from unit tests.
 [setting the default value of an optional request parameter is breaking](checker/checker_breaking_test.go?plain=1#L553)  
 
 ## 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 media-type to response is not breaking](checker/checker_not_breaking_test.go?plain=1#L168)  
 [adding a new required property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L402)  
 [adding a new required property under AllOf in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L432)  
 [adding a new required read-only property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L486)  
 [adding a non-existent required property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L294)  
-[adding a tag is not breaking with "api-tag-removed" check](checker/checker_not_breaking_test.go?plain=1#L263)  
-[adding a tag is not breaking](checker/checker_not_breaking_test.go?plain=1#L248)  
+[adding a tag is not breaking with "api-tag-removed" check](checker/checker_not_breaking_test.go?plain=1#L265)  
+[adding a tag is not breaking](checker/checker_not_breaking_test.go?plain=1#L250)  
 [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 a link to operation ID is not breaking](checker/checker_not_breaking_test.go?plain=1#L159)  
 [changing an existing property in request body to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L322)  
 [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#L308)  
@@ -100,20 +99,20 @@ These examples are automatically generated from unit tests.
 [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 operation ID is not breaking](checker/checker_not_breaking_test.go?plain=1#L150)  
 [changing request's body schema type from integer to number is not breaking](checker/checker_breaking_request_type_changed_test.go?plain=1#L71)  
 [changing response's body schema type from number to integer is not breaking](checker/checker_breaking_response_type_changed_test.go?plain=1#L51)  
 [changing response's body schema type from number/none to integer/int32 is not breaking](checker/checker_breaking_response_type_changed_test.go?plain=1#L89)  
-[changing servers is not breaking](checker/checker_not_breaking_test.go?plain=1#L234)  
+[changing servers is not breaking](checker/checker_not_breaking_test.go?plain=1#L236)  
 [deleting a non-required non-write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L531)  
 [deleting a path after sunset date of all contained operations is not breaking](checker/checker_deprecation_test.go?plain=1#L258)  
 [deleting a pattern from a schema is not breaking](checker/checker_breaking_test.go?plain=1#L443)  
 [deleting a required write-only property in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L514)  
 [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#L69)  
-[deprecating a header is not breaking](checker/checker_not_breaking_test.go?plain=1#L208)  
-[deprecating a parameter is not breaking](checker/checker_not_breaking_test.go?plain=1#L195)  
-[deprecating a schema is not breaking](checker/checker_not_breaking_test.go?plain=1#L221)  
+[deprecating a header is not breaking](checker/checker_not_breaking_test.go?plain=1#L210)  
+[deprecating a parameter is not breaking](checker/checker_not_breaking_test.go?plain=1#L197)  
+[deprecating a schema is not breaking](checker/checker_not_breaking_test.go?plain=1#L223)  
 [deprecating an operation with a deprecation policy and sunset date after required deprecation period is not breaking](checker/checker_deprecation_test.go?plain=1#L237)  
 [deprecating an operation without a deprecation policy and without specifying sunset date is not breaking for draft level](checker/checker_deprecation_test.go?plain=1#L155)  
 [deprecating an operation without a deprecation policy and without specifying sunset date is not breaking](checker/checker_deprecation_test.go?plain=1#L103)  
@@ -123,7 +122,7 @@ These examples are automatically generated from unit tests.
 [modifying the default value of a required request parameter is not breaking](checker/checker_breaking_test.go?plain=1#L571)  
 [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)  
+[new required response header param is not breaking](checker/checker_not_breaking_test.go?plain=1#L136)  
 [no change is not breaking](checker/checker_not_breaking_test.go?plain=1#L15)  
 [reducing max in response is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L281)  
 [reducing max length in response is not breaking](checker/checker_breaking_min_max_test.go?plain=1#L31)  
@@ -136,7 +135,9 @@ These examples are automatically generated from unit tests.
 [renaming a path parameter is not breaking](checker/checker_breaking_test.go?plain=1#L135)  
 
 ## Examples of info-level changes for changelog
-[deprecating an operation with sunset greater than min](checker/checker_not_breaking_test.go?plain=1#L180)  
+[changing an existing header param from required to optional](checker/checker_request_parameter_required_value_updated_test.go?plain=1#L36)  
+[changing an existing header param to optional](checker/checker_not_breaking_test.go?plain=1#L116)  
+[deprecating an operation with sunset greater than min](checker/checker_not_breaking_test.go?plain=1#L182)  
 [new paths or path operations](checker/check-api-added_test.go?plain=1#L11)  
 [path operations that became deprecated](checker/checker_deprecation_test.go?plain=1#L324)  
 [path operations that were re-activated](checker/checker_deprecation_test.go?plain=1#L344)  
diff --git a/checker/check-api-added_test.go b/checker/check-api-added_test.go
@@ -19,7 +19,7 @@ func TestApiAdded_DetectsNewPathsAndNewOperations(t *testing.T) {
 	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2)
 	require.NoError(t, err)
 
-	errs := checker.CheckBackwardCompatibility(checker.GetAllChecks(), d, osm)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.APIAddedCheck), d, osm, checker.INFO)
 	require.NotEmpty(t, errs)
 	require.Len(t, errs, 2)
 
@@ -43,7 +43,7 @@ func TestApiAdded_DetectsModifiedPathsWithPathParam(t *testing.T) {
 	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2)
 	require.NoError(t, err)
 
-	errs := checker.CheckBackwardCompatibility(checker.GetAllChecks(), d, osm)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.APIAddedCheck), d, osm, checker.INFO)
 	require.NotEmpty(t, errs)
 	require.Len(t, errs, 1)
 

diff --git a/...heck-request-parameter-became-required.go → ...quest-parameter-required-value-updated.go b/...heck-request-parameter-became-required.go → ...quest-parameter-required-value-updated.go
@@ -6,7 +6,7 @@ import (
 	"github.com/tufin/oasdiff/diff"
 )
 
-func RequestParameterBecameRequiredCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
+func RequestParameterRequiredValueUpdatedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
 	result := make([]BackwardCompatibilityError, 0)
 	if diffReport.PathsDiff == nil {
 		return result
@@ -28,15 +28,20 @@ func RequestParameterBecameRequiredCheck(diffReport *diff.Diff, operationsSource
 					if requiredDiff == nil {
 						continue
 					}
+
+					id := "request-parameter-became-required"
+					level := ERR
+
 					if requiredDiff.To != true {
-						continue
+						id = "request-parameter-became-optional"
+						level = INFO
 					}
 
 					source := (*operationsSources)[operationItem.Revision]
 					result = append(result, BackwardCompatibilityError{
-						Id:          "request-parameter-became-required",
-						Level:       ERR,
-						Text:        fmt.Sprintf(config.i18n("request-parameter-became-required"), ColorizedValue(paramLocation), ColorizedValue(paramName)),
+						Id:          id,
+						Level:       level,
+						Text:        fmt.Sprintf(config.i18n(id), ColorizedValue(paramLocation), ColorizedValue(paramName)),
 						Operation:   operation,
 						OperationId: operationItem.Revision.OperationID,
 						Path:        path,

diff --git a/checker/checker.go b/checker/checker.go
@@ -158,6 +158,10 @@ func (c *BackwardCompatibilityCheckConfig) i18n(messageID string) string {
 }
 
 func CheckBackwardCompatibility(config BackwardCompatibilityCheckConfig, diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap) BackwardCompatibilityErrors {
+	return CheckBackwardCompatibilityUntilLevel(config, diffReport, operationsSources, WARN)
+}
+
+func CheckBackwardCompatibilityUntilLevel(config BackwardCompatibilityCheckConfig, diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, level Level) BackwardCompatibilityErrors {
 	result := make(BackwardCompatibilityErrors, 0)
 
 	if diffReport == nil {
@@ -171,8 +175,15 @@ func CheckBackwardCompatibility(config BackwardCompatibilityCheckConfig, diffRep
 		result = append(result, errs...)
 	}
 
-	sort.Sort(result)
-	return result
+	filteredResult := make(BackwardCompatibilityErrors, 0)
+	for _, change := range result {
+		if change.Level <= level {
+			filteredResult = append(filteredResult, change)
+		}
+	}
+
+	sort.Sort(filteredResult)
+	return filteredResult
 }
 
 func removeDraftAndAlphaOperationsDiffs(diffReport *diff.Diff, result []BackwardCompatibilityError, operationsSources *diff.OperationsSourcesMap) []BackwardCompatibilityError {

diff --git a/checker/checker_deprecation_test.go b/checker/checker_deprecation_test.go
@@ -249,7 +249,7 @@ func TestBreaking_DeprecationWithProperSunset(t *testing.T) {
 	require.NoError(t, err)
 	c := singleCheckConfig(checker.APIDeprecationCheck)
 	c.MinSunsetStableDays = 10
-	errs := checker.CheckBackwardCompatibility(c, d, osm)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(c, d, osm, checker.INFO)
 	require.Len(t, errs, 1)
 	// only a non-breaking change detected
 	require.Equal(t, errs[0].Level, checker.INFO)
@@ -332,7 +332,7 @@ func TestApiDeprecated_DetectsDeprecatedOperations(t *testing.T) {
 	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2)
 	require.NoError(t, err)
 
-	errs := checker.CheckBackwardCompatibility(singleCheckConfig(checker.APIDeprecationCheck), d, osm)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.APIDeprecationCheck), d, osm, checker.INFO)
 	require.NotEmpty(t, errs)
 	require.Len(t, errs, 1)
 
@@ -352,7 +352,7 @@ func TestApiDeprecated_DetectsReactivatedOperations(t *testing.T) {
 	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2)
 	require.NoError(t, err)
 
-	errs := checker.CheckBackwardCompatibility(singleCheckConfig(checker.APIDeprecationCheck), d, osm)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.APIDeprecationCheck), d, osm, checker.INFO)
 	require.NotEmpty(t, errs)
 	require.Len(t, errs, 1)
 

diff --git a/checker/checker_not_breaking_test.go b/checker/checker_not_breaking_test.go
@@ -113,7 +113,7 @@ func TestBreaking_NewOptionalHeaderParam(t *testing.T) {
 	require.Empty(t, errs)
 }
 
-// BC: changing an existing header param to optional is not breaking
+// CL: changing an existing header param to optional
 func TestBreaking_HeaderParamRequiredDisabled(t *testing.T) {
 	s1 := l(t, 1)
 	s2 := l(t, 1)
@@ -123,8 +123,10 @@ func TestBreaking_HeaderParamRequiredDisabled(t *testing.T) {
 
 	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), &s1, &s2)
 	require.NoError(t, err)
-	errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm)
-	require.Empty(t, errs)
+	changes := checker.CheckBackwardCompatibilityUntilLevel(checker.GetDefaultChecks(), d, osm, checker.INFO)
+	require.NotEmpty(t, changes)
+	require.Equal(t, "request-parameter-became-optional", changes[0].Id)
+	require.Len(t, changes, 1)
 }
 
 func deleteResponseHeader(response *openapi3.Response, name string) {
@@ -187,7 +189,7 @@ func TestBreaking_DeprecatedOperation(t *testing.T) {
 
 	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), &s1, &s2)
 	require.NoError(t, err)
-	errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(checker.GetDefaultChecks(), d, osm, checker.INFO)
 	require.Len(t, errs, 1)
 	require.Equal(t, errs[0].Level, checker.INFO)
 }

diff --git a/checker/checker_request_parameter_required_value_updated_test.go b/checker/checker_request_parameter_required_value_updated_test.go
@@ -0,0 +1,58 @@
+package checker_test
+
+import (
+	"testing"
+
+	"github.com/getkin/kin-openapi/openapi3"
+	"github.com/stretchr/testify/require"
+	"github.com/tufin/oasdiff/checker"
+	"github.com/tufin/oasdiff/diff"
+)
+
+// BC: changing an existing header param from optional to required is breaking
+func TestBreaking_HeaderParamBecameRequired(t *testing.T) {
+	s1 := l(t, 1)
+	s2 := l(t, 1)
+
+	s1.Spec.Paths[installCommandPath].Get.Parameters.GetByInAndName(openapi3.ParameterInHeader, "network-policies").Required = false
+	s2.Spec.Paths[installCommandPath].Get.Parameters.GetByInAndName(openapi3.ParameterInHeader, "network-policies").Required = true
+
+	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), &s1, &s2)
+	require.NoError(t, err)
+	errs := checker.CheckBackwardCompatibility(singleCheckConfig(checker.RequestParameterRequiredValueUpdatedCheck), d, osm)
+	require.NotEmpty(t, errs)
+	require.Equal(t, checker.BackwardCompatibilityErrors{
+		{
+			Id:        "request-parameter-became-required",
+			Text:      "the 'header' request parameter 'network-policies' became required",
+			Comment:   "",
+			Level:     checker.ERR,
+			Operation: "GET",
+			Path:      "/api/{domain}/{project}/install-command",
+			Source:    "../data/openapi-test1.yaml",
+		}}, errs)
+}
+
+// CL: changing an existing header param from required to optional
+func TestBreaking_HeaderParamBecameOptional(t *testing.T) {
+	s1 := l(t, 1)
+	s2 := l(t, 1)
+
+	s1.Spec.Paths[installCommandPath].Get.Parameters.GetByInAndName(openapi3.ParameterInHeader, "network-policies").Required = true
+	s2.Spec.Paths[installCommandPath].Get.Parameters.GetByInAndName(openapi3.ParameterInHeader, "network-policies").Required = false
+
+	d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), &s1, &s2)
+	require.NoError(t, err)
+	errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestParameterRequiredValueUpdatedCheck), d, osm, checker.INFO)
+	require.NotEmpty(t, errs)
+	require.Equal(t, checker.BackwardCompatibilityErrors{
+		{
+			Id:        "request-parameter-became-optional",
+			Text:      "the 'header' request parameter 'network-policies' became optional",
+			Comment:   "",
+			Level:     checker.INFO,
+			Operation: "GET",
+			Path:      "/api/{domain}/{project}/install-command",
+			Source:    "../data/openapi-test1.yaml",
+		}}, errs)
+}
diff --git a/checker/default_checks.go b/checker/default_checks.go
@@ -62,7 +62,7 @@ func defaultChecks() []BackwardCompatibilityCheck {
 		RequestParameterPatternAddedOrChangedCheck,
 		RequestPropertyPatternAddedOrChangedCheck,
 		AddedRequiredRequestBodyCheck,
-		RequestParameterBecameRequiredCheck,
+		RequestParameterRequiredValueUpdatedCheck,
 		RequestParameterBecameEnumCheck,
 		RequestPropertyBecameRequiredCheck,
 		RequestPropertyBecameEnumCheck,

diff --git a/checker/localizations/localizations.go b/checker/localizations/localizations.go
diff --git a/checker/localizations_src/en/messages.yaml b/checker/localizations_src/en/messages.yaml
@@ -31,6 +31,7 @@ response-mediatype-enum-value-removed: response schema %s enum value removed %s
 request-header-property-became-required: the %s request header's property %s became required
 request-header-property-became-enum: the %s request header's property %s was restricted to a list of enum values
 request-parameter-became-required: the %s request parameter %s became required
+request-parameter-became-optional: the %s request parameter %s became optional
 request-parameter-became-enum: the %s request parameter %s was restricted to a list of enum values
 request-parameter-enum-value-removed: removed the enum value %s for the %s request parameter %s
 pattern-changed-warn-comment: "This is a warning because it is difficult to automatically analyze if the new pattern is a superset of the previous pattern(e.g. changed from '[0-9]+' to '[0-9]*')"