diff --git a/checker/check_api_removed.go b/checker/check_api_removed.go index 61579dd1..3b6dadb2 100644 --- a/checker/check_api_removed.go +++ b/checker/check_api_removed.go @@ -19,25 +19,42 @@ const ( ) func APIRemovedCheck(diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, config *Config) Changes { - result := make(Changes, 0) - if diffReport.PathsDiff == nil { - return result + return append( + checkRemovedPaths(diffReport.PathsDiff, operationsSources, config), + checkRemovedOperations(diffReport.PathsDiff, operationsSources, config)..., + ) +} + +func checkRemovedPaths(pathsDiff *diff.PathsDiff, operationsSources *diff.OperationsSourcesMap, config *Config) Changes { + + if pathsDiff == nil { + return nil } - for _, path := range diffReport.PathsDiff.Deleted { - if diffReport.PathsDiff.Base.Value(path) == nil { + result := make(Changes, 0) + for _, path := range pathsDiff.Deleted { + if pathsDiff.Base.Value(path) == nil { continue } - for operation := range diffReport.PathsDiff.Base.Value(path).Operations() { - op := diffReport.PathsDiff.Base.Value(path).GetOperation(operation) + for operation := range pathsDiff.Base.Value(path).Operations() { + op := pathsDiff.Base.Value(path).GetOperation(operation) if change := checkAPIRemoval(config, true, op, operationsSources, operation, path); change != nil { result = append(result, change) } } } + return result +} + +func checkRemovedOperations(pathsDiff *diff.PathsDiff, operationsSources *diff.OperationsSourcesMap, config *Config) Changes { + if pathsDiff == nil { + return nil + } + + result := make(Changes, 0) - for path, pathItem := range diffReport.PathsDiff.Modified { + for path, pathItem := range pathsDiff.Modified { if pathItem.OperationsDiff == nil { continue } diff --git a/checker/check_api_removed_test.go b/checker/check_api_removed_test.go index bc52ce33..a019999d 100644 --- a/checker/check_api_removed_test.go +++ b/checker/check_api_removed_test.go @@ -26,7 +26,7 @@ func TestBreaking_RemoveBeforeSunset(t *testing.T) { require.Equal(t, "api removed before the sunset date '9999-08-10'", errs[0].GetUncolorizedText(checker.NewDefaultLocalizer())) } -// BC: deleting an operation without sunset date is not breaking +// BC: deleting a deprecated operation without sunset date is not breaking func TestBreaking_DeprecationNoSunset(t *testing.T) { s1, err := open(getDeprecationFile("deprecated-no-sunset.yaml")) @@ -36,9 +36,12 @@ func TestBreaking_DeprecationNoSunset(t *testing.T) { require.NoError(t, err) d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2) - errs := checker.CheckBackwardCompatibility(singleCheckConfig(checker.APIRemovedCheck), d, osm) + errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.APIRemovedCheck), d, osm, checker.INFO) require.NoError(t, err) - require.Empty(t, errs) + require.Len(t, errs, 1) + require.Equal(t, checker.APIRemovedWithDeprecationId, errs[0].GetId()) + require.Equal(t, "api removed with deprecation", errs[0].GetUncolorizedText(checker.NewDefaultLocalizer())) + require.Equal(t, checker.INFO, errs[0].GetLevel()) } // BC: removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not alpha stability level @@ -101,6 +104,29 @@ func TestBreaking_DeprecationPathMixed(t *testing.T) { require.Equal(t, "api path removed before the sunset date '9999-08-10'", errs[0].GetUncolorizedText(checker.NewDefaultLocalizer())) } +// BC: deleting a path with deprecated operations without sunset date is not breaking +func TestBreaking_PathDeprecationNoSunset(t *testing.T) { + + s1, err := open(getDeprecationFile("deprecated-path-no-sunset.yaml")) + require.NoError(t, err) + + s2, err := open(getDeprecationFile("sunset-path.yaml")) + require.NoError(t, err) + + d, osm, err := diff.GetWithOperationsSourcesMap(diff.NewConfig(), s1, s2) + errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.APIRemovedCheck), d, osm, checker.INFO) + require.NoError(t, err) + require.Len(t, errs, 2) + + require.Equal(t, checker.APIPathRemovedWithDeprecationId, errs[0].GetId()) + require.Equal(t, "api path removed with deprecation", errs[0].GetUncolorizedText(checker.NewDefaultLocalizer())) + require.Equal(t, checker.INFO, errs[0].GetLevel()) + + require.Equal(t, checker.APIPathRemovedWithDeprecationId, errs[1].GetId()) + require.Equal(t, "api path removed with deprecation", errs[1].GetUncolorizedText(checker.NewDefaultLocalizer())) + require.Equal(t, checker.INFO, errs[1].GetLevel()) +} + // BC: removing a deprecated enpoint with an invalid date is breaking func TestBreaking_RemoveEndpointWithInvalidSunset(t *testing.T) { diff --git a/checker/checker.go b/checker/checker.go index b22fc500..55c42f0b 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -14,10 +14,12 @@ const ( APIStabilityDecreasedId = "api-stability-decreased" ) +// CheckBackwardCompatibility runs the checks with level WARN and ERR func CheckBackwardCompatibility(config *Config, diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap) Changes { return CheckBackwardCompatibilityUntilLevel(config, diffReport, operationsSources, WARN) } +// CheckBackwardCompatibilityUntilLevel runs the checks with level equal or higher than the given level func CheckBackwardCompatibilityUntilLevel(config *Config, diffReport *diff.Diff, operationsSources *diff.OperationsSourcesMap, level Level) Changes { result := make(Changes, 0) diff --git a/data/deprecation/deprecated-path-no-sunset.yaml b/data/deprecation/deprecated-path-no-sunset.yaml new file mode 100644 index 00000000..35b250ca --- /dev/null +++ b/data/deprecation/deprecated-path-no-sunset.yaml @@ -0,0 +1,21 @@ +info: + title: Tufin + version: 1.0.0 +openapi: 3.0.3 +paths: + /api/placeholder: + get: + responses: + 200: + description: OK + /api/test: + get: + deprecated: true + responses: + 200: + description: OK + post: + deprecated: true + responses: + 201: + description: OK diff --git a/docs/BREAKING-CHANGES-EXAMPLES.md b/docs/BREAKING-CHANGES-EXAMPLES.md index 991c8caa..228ee97a 100644 --- a/docs/BREAKING-CHANGES-EXAMPLES.md +++ b/docs/BREAKING-CHANGES-EXAMPLES.md @@ -50,7 +50,7 @@ These examples are automatically generated from unit tests. [deleting a media-type from response is breaking](../checker/check_breaking_test.go?plain=1#L442) [deleting a non-required non-write-only property in response body is breaking with warning](../checker/check_breaking_property_test.go?plain=1#L512) [deleting a path is breaking](../checker/check_breaking_test.go?plain=1#L37) -[deleting a path with some operations having sunset date in the future is breaking](../checker/check_api_removed_test.go?plain=1#L86) +[deleting a path with some operations having sunset date in the future is breaking](../checker/check_api_removed_test.go?plain=1#L89) [deleting a required property in request is breaking with warn](../checker/check_breaking_property_test.go?plain=1#L369) [deleting a required property in response body is breaking](../checker/check_breaking_property_test.go?plain=1#L421) [deleting a required property under AllOf in response body is breaking](../checker/check_breaking_property_test.go?plain=1#L451) @@ -80,7 +80,7 @@ These examples are automatically generated from unit tests. [removing 'allOf' subschema from the request body or request body property is breaking with warn](../checker/check_breaking_test.go?plain=1#L749) [removing 'anyOf' schema from the request body or request body property is breaking](../checker/check_breaking_test.go?plain=1#L684) [removing 'oneOf' schema from the request body or request body property is breaking](../checker/check_breaking_test.go?plain=1#L706) -[removing a deprecated enpoint with an invalid date is breaking](../checker/check_api_removed_test.go?plain=1#L104) +[removing a deprecated enpoint with an invalid date is breaking](../checker/check_api_removed_test.go?plain=1#L130) [removing a media type from request body is breaking](../checker/check_breaking_test.go?plain=1#L668) [removing a success status is breaking](../checker/check_response_status_updated_test.go?plain=1#L87) [removing an existing optional response header is breaking as warn](../checker/check_breaking_test.go?plain=1#L422) @@ -89,8 +89,8 @@ These examples are automatically generated from unit tests. [removing an existing response with successful status is breaking](../checker/check_breaking_test.go?plain=1#L251) [removing an schema object from components is breaking (optional)](../checker/check_breaking_test.go?plain=1#L643) [removing the default value of an optional request parameter is breaking](../checker/check_breaking_test.go?plain=1#L606) -[removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not alpha stability level](../checker/check_api_removed_test.go?plain=1#L44) -[removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not draft stability level](../checker/check_api_removed_test.go?plain=1#L64) +[removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not alpha stability level](../checker/check_api_removed_test.go?plain=1#L47) +[removing the path without a deprecation policy and without specifying sunset date is breaking if some APIs are not draft stability level](../checker/check_api_removed_test.go?plain=1#L67) [removing/updating a property enum in response is breaking (optional)](../checker/check_breaking_test.go?plain=1#L333) [removing/updating a tag is breaking (optional)](../checker/check_breaking_test.go?plain=1#L351) [removing/updating an enum in request body is breaking (optional)](../checker/check_breaking_test.go?plain=1#L310) @@ -132,12 +132,13 @@ These examples are automatically generated from unit tests. [changing response's body schema type from number to integer is not breaking](../checker/check_breaking_response_type_changed_test.go?plain=1#L52) [changing response's body schema type from number/none to integer/int32 is not breaking](../checker/check_breaking_response_type_changed_test.go?plain=1#L90) [changing servers is not breaking](../checker/check_not_breaking_test.go?plain=1#L252) +[deleting a deprecated operation without sunset date is not breaking](../checker/check_api_removed_test.go?plain=1#L29) [deleting a path after sunset date of all contained operations is not breaking](../checker/check_api_deprecation_test.go?plain=1#L255) +[deleting a path with deprecated operations without sunset date is not breaking](../checker/check_api_removed_test.go?plain=1#L107) [deleting a pattern from a schema is not breaking](../checker/check_breaking_test.go?plain=1#L459) [deleting a required write-only property in response body is not breaking](../checker/check_breaking_property_test.go?plain=1#L495) [deleting a tag is not breaking](../checker/check_not_breaking_test.go?plain=1#L71) [deleting an operation after sunset date is not breaking](../checker/check_api_deprecation_test.go?plain=1#L33) -[deleting an operation without sunset date is not breaking](../checker/check_api_removed_test.go?plain=1#L29) [deleting other extension (not sunset) header for a deprecated endpoint is not breaking](../checker/check_api_sunset_changed_test.go?plain=1#L84) [deprecating a header is not breaking](../checker/check_not_breaking_test.go?plain=1#L226) [deprecating a parameter is not breaking](../checker/check_not_breaking_test.go?plain=1#L213) diff --git a/docs/DEPRECATION.md b/docs/DEPRECATION.md index 17d98899..f2131f2e 100644 --- a/docs/DEPRECATION.md +++ b/docs/DEPRECATION.md @@ -3,7 +3,8 @@ Sometimes APIs need to be removed, for example, when we replace an old API by a As API owners, we want a process that will allow us to phase out the old API version and transition to the new one smoothly as possible and with minimal disruptions to business. OpenAPI specification supports a ```deprecated``` flag which can be used to mark operations and other object types as deprecated. -Normally, deprecation **is not** considered a breaking change since it doesn't break the client but only serves as an indication of an intent to remove something in the future, in contrast, the eventual removal of a resource **is** considered a breaking change. +Deprecating a resource **isn't** considered a breaking change since it doesn't break the client but only serves as an indication of an intent to remove something in the future. +After deprecating a resource, it can be removed without triggering a breaking change since the client already knows it is going to be removed. ### Deprecation without a sunset date Oasdiff allows you to gracefully remove a resource without getting a breaking change error, as follows: