(feature) Add optional checker for tag removal (#208)

* Add optional checker for tag removal * Add tests for tag addition * Split errors per tag * remove redundant check --------- Co-authored-by: Reuven <[email protected]>
Tufin · Mar 26, 2023 · 7a7d639 · 7a7d639
1 parent 6dc9e3a
commit 7a7d639
Show file tree

Hide file tree

Showing 9 changed files with 138 additions and 35 deletions.
diff --git a/breaking-changes.md b/breaking-changes.md
@@ -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#L391)  
-[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L357)  
-[adding a pattern to a schema is breaking](checker/checker_breaking_test.go?plain=1#L373)  
+[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 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)  
@@ -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#L327)  
+[deleting a media-type from response is breaking](checker/checker_breaking_test.go?plain=1#L345)  
 [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)  
@@ -30,75 +30,78 @@ 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#L407)  
-[modifying a pattern in request parameter is breaking](checker/checker_breaking_test.go?plain=1#L423)  
-[modifying the default value of an optional request parameter is breaking](checker/checker_breaking_test.go?plain=1#L453)  
+[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)  
 [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#L308)  
+[removing an existing optional response header is breaking as warn](checker/checker_breaking_test.go?plain=1#L326)  
 [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 an operation id is breaking (optional)](checker/checker_breaking_test.go?plain=1#L258)  
 
 ## Examples of non-breaking changes
-[adding a media-type to response is not breaking](checker/checker_not_breaking_test.go?plain=1#L165)  
+[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#L227)  
 [adding a new required property under AllOf in response body is not breaking](checker/checker_breaking_property_test.go?plain=1#L257)  
 [adding a new required read-only property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L311)  
 [adding a non-existent required property in request body is not breaking](checker/checker_breaking_property_test.go?plain=1#L136)  
-[adding an enum value is not breaking](checker/checker_not_breaking_test.go?plain=1#L65)  
-[adding an optional request body is not breaking](checker/checker_not_breaking_test.go?plain=1#L19)  
+[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 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#L156)  
-[changing an existing header param to optional is not breaking](checker/checker_not_breaking_test.go?plain=1#L115)  
+[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#L164)  
 [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#L150)  
 [changing an existing read-only property in request body to required is not breaking](checker/checker_breaking_property_test.go?plain=1#L325)  
-[changing an existing request body from required to optional is not breaking](checker/checker_not_breaking_test.go?plain=1#L34)  
+[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#L387)  
 [changing an existing write-only property in response body to optional is not breaking](checker/checker_breaking_property_test.go?plain=1#L373)  
-[changing comments is not breaking](checker/checker_not_breaking_test.go?plain=1#L89)  
-[changing extensions is not breaking](checker/checker_not_breaking_test.go?plain=1#L77)  
+[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#L147)  
-[changing servers is not breaking](checker/checker_not_breaking_test.go?plain=1#L232)  
+[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#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#L343)  
+[deleting a pattern from a schema is not breaking](checker/checker_breaking_test.go?plain=1#L361)  
 [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#L53)  
+[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#L206)  
-[deprecating a parameter is not breaking](checker/checker_not_breaking_test.go?plain=1#L193)  
-[deprecating a schema is not breaking](checker/checker_not_breaking_test.go?plain=1#L219)  
-[deprecating an operation is not breaking](checker/checker_not_breaking_test.go?plain=1#L179)  
+[deprecating a header is not breaking](checker/checker_not_breaking_test.go?plain=1#L207)  
+[deprecating a parameter is not breaking](checker/checker_not_breaking_test.go?plain=1#L194)  
+[deprecating a schema is not breaking](checker/checker_not_breaking_test.go?plain=1#L220)  
+[deprecating an operation is not breaking](checker/checker_not_breaking_test.go?plain=1#L180)  
 [deprecating an operation with a deprecation policy and sunset date after required deprecation period is not breaking](checker/checker_deprecation_test.go?plain=1#L217)  
 [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#L137)  
 [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#L439)  
-[modifying the default value of a required request parameter is not breaking](checker/checker_breaking_test.go?plain=1#L469)  
-[new optional header param is not breaking](checker/checker_not_breaking_test.go?plain=1#L101)  
+[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)  
+[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#L133)  
-[no change is not breaking](checker/checker_not_breaking_test.go?plain=1#L14)  
+[new required response header param is not breaking](checker/checker_not_breaking_test.go?plain=1#L134)  
+[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)  
 [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#L292)  
-[removing an existing response with unparseable status is not breaking](checker/checker_breaking_test.go?plain=1#L276)  
+[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 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)  
diff --git a/checker/check-api-operation-id-removed.go b/checker/check-api-operation-id-removed.go
@@ -7,7 +7,7 @@ import (
 )
 
 const (
-	id = "api-operation-id-removed"
+	apiOperationRemovedCheckId = "api-operation-id-removed"
 )
 
 func APIOperationIdRemovedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config BackwardCompatibilityCheckConfig) []BackwardCompatibilityError {
@@ -30,9 +30,9 @@ func APIOperationIdRemovedCheck(diffReport *diff.Diff, operationsSources *diff.O
 			}
 
 			result = append(result, BackwardCompatibilityError{
-				Id:        id,
+				Id:        apiOperationRemovedCheckId,
 				Level:     ERR,
-				Text:      fmt.Sprintf(config.i18n(id), ColorizedValue(operationItem.Base.OperationID), ColorizedValue(operationItem.Revision.OperationID)),
+				Text:      fmt.Sprintf(config.i18n(apiOperationRemovedCheckId), ColorizedValue(operationItem.Base.OperationID), ColorizedValue(operationItem.Revision.OperationID)),
 				Operation: operation,
 				Path:      path,
 				Source:    source,

diff --git a/checker/check-api-tag-removed.go b/checker/check-api-tag-removed.go
@@ -0,0 +1,46 @@
+package checker
+
+import (
+	"fmt"
+
+	"github.com/tufin/oasdiff/diff"
+)
+
+const (
+	apiTagRemovedCheckId = "api-tag-removed"
+)
+
+func APITagRemovedCheck(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 {
+			op := pathItem.Base.Operations()[operation]
+			source := (*operationsSources)[op]
+
+			if operationItem.TagsDiff == nil {
+				continue
+			}
+
+			for _, tag := range operationItem.TagsDiff.Deleted {
+				result = append(result, BackwardCompatibilityError{
+					Id:        apiTagRemovedCheckId,
+					Level:     ERR,
+					Text:      fmt.Sprintf(config.i18n(apiTagRemovedCheckId), ColorizedValue(tag)),
+					Operation: operation,
+					Path:      path,
+					Source:    source,
+				})
+
+			}
+		}
+	}
+	return result
+}
diff --git a/checker/checker_breaking_test.go b/checker/checker_breaking_test.go
@@ -273,6 +273,24 @@ func TestBreaking_OperationIdRemoved(t *testing.T) {
 	require.Equal(t, "api-operation-id-removed", errs[0].Id)
 }
 
+// BC: removing/updating a tag is breaking (optional)
+func TestBreaking_TagRemoved(t *testing.T) {
+	s1 := l(t, 1)
+	s2 := l(t, 1)
+
+	s2.Spec.Paths[securityScorePath].Get.Tags[0] = "newTag"
+
+	d, osm, err := diff.GetWithOperationsSourcesMap(&diff.Config{}, &s1, &s2)
+	require.NoError(t, err)
+	errs := checker.CheckBackwardCompatibility(checker.GetChecks(utils.StringList{"api-tag-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, "api-tag-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)

diff --git a/checker/checker_not_breaking_test.go b/checker/checker_not_breaking_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/tufin/oasdiff/checker"
 	"github.com/tufin/oasdiff/diff"
+	"github.com/tufin/oasdiff/utils"
 )
 
 // BC: no change is not breaking
@@ -242,3 +243,33 @@ func TestBreaking_Servers(t *testing.T) {
 	errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm)
 	require.Empty(t, errs)
 }
+
+// BC: adding a tag is not breaking
+func TestBreaking_TagAdded(t *testing.T) {
+	s1 := l(t, 1)
+	s2 := l(t, 1)
+
+	s2.Spec.Paths[securityScorePath].Get.Tags = append(s2.Spec.Paths[securityScorePath].Get.Tags, "newTag")
+	d, osm, err := diff.GetWithOperationsSourcesMap(&diff.Config{}, &s1, &s2)
+	require.NoError(t, err)
+	errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm)
+	for _, err := range errs {
+		require.Equal(t, checker.ERR, err.Level)
+	}
+	require.Empty(t, errs)
+}
+
+// BC: adding a tag is not breaking with "api-tag-removed" check
+func TestBreaking_TagAddedWithCustomCheck(t *testing.T) {
+	s1 := l(t, 1)
+	s2 := l(t, 1)
+
+	s2.Spec.Paths[securityScorePath].Get.Tags = append(s2.Spec.Paths[securityScorePath].Get.Tags, "newTag")
+	d, osm, err := diff.GetWithOperationsSourcesMap(&diff.Config{}, &s1, &s2)
+	require.NoError(t, err)
+	errs := checker.CheckBackwardCompatibility(checker.GetChecks(utils.StringList{"api-tag-removed"}), d, osm)
+	for _, err := range errs {
+		require.Equal(t, checker.ERR, err.Level)
+	}
+	require.Empty(t, errs)
+}
diff --git a/checker/default_checks.go b/checker/default_checks.go
@@ -22,6 +22,7 @@ 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,
 }
 
 func ValidateIncludeChecks(includeChecks utils.StringList) utils.StringList {