From 6629e0df26c369e0c292156f2bf1dbdc39c3e14c Mon Sep 17 00:00:00 2001 From: Reuven Harrison Date: Tue, 6 Jun 2023 13:18:16 +0300 Subject: [PATCH] fix-type-breaking-changes (#286) * replicate issue * fixing type change checks * finish response checks * fixed request --- BREAKING-CHANGES-EXAMPLES.md | 11 ++ .../check-request-property-type-changed.go | 23 ++-- .../check-response-property-type-changed.go | 50 +++++-- ...cker_breaking_request_type_changed_test.go | 108 ++++++++++++++++ ...ker_breaking_response_type_changed_test.go | 122 ++++++++++++++++++ data/type-change/base-response.yaml | 40 ++++++ data/type-change/revision-response.yaml | 41 ++++++ data/type-change/simple-request.yaml | 12 ++ data/type-change/simple-response.yaml | 13 ++ 9 files changed, 397 insertions(+), 23 deletions(-) create mode 100644 checker/checker_breaking_request_type_changed_test.go create mode 100644 checker/checker_breaking_response_type_changed_test.go create mode 100644 data/type-change/base-response.yaml create mode 100644 data/type-change/revision-response.yaml create mode 100644 data/type-change/simple-request.yaml create mode 100644 data/type-change/simple-response.yaml diff --git a/BREAKING-CHANGES-EXAMPLES.md b/BREAKING-CHANGES-EXAMPLES.md index 9fb7da37..a9b49873 100644 --- a/BREAKING-CHANGES-EXAMPLES.md +++ b/BREAKING-CHANGES-EXAMPLES.md @@ -27,6 +27,14 @@ 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#L211) [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) +[changing request's body schema type from number to integer is breaking](checker/checker_breaking_request_type_changed_test.go?plain=1#L51) +[changing request's body schema type from number to string is breaking](checker/checker_breaking_request_type_changed_test.go?plain=1#L31) +[changing request's body schema type from number/none to integer/int32 is breaking](checker/checker_breaking_request_type_changed_test.go?plain=1#L89) +[changing request's body schema type from string to number is breaking](checker/checker_breaking_request_type_changed_test.go?plain=1#L11) +[changing response's body schema type from integer to number is breaking](checker/checker_breaking_response_type_changed_test.go?plain=1#L69) +[changing response's body schema type from number to string is breaking](checker/checker_breaking_response_type_changed_test.go?plain=1#L31) +[changing response's body schema type from string to number is breaking](checker/checker_breaking_response_type_changed_test.go?plain=1#L11) +[changing response's embedded property schema type from string/none to integer/int32 is breaking](checker/checker_breaking_response_type_changed_test.go?plain=1#L108) [deleting a media-type from response is breaking](checker/checker_breaking_test.go?plain=1#L427) [deleting a path is breaking](checker/checker_breaking_test.go?plain=1#L43) [deleting a path with some operations having sunset date in the future is breaking](checker/checker_deprecation_test.go?plain=1#L273) @@ -93,6 +101,9 @@ These examples are automatically generated from unit tests. [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 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) [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) diff --git a/checker/check-request-property-type-changed.go b/checker/check-request-property-type-changed.go index bcce05ff..390590a9 100644 --- a/checker/check-request-property-type-changed.go +++ b/checker/check-request-property-type-changed.go @@ -82,17 +82,14 @@ func fillEmptyTypeAndFormatDiffs(typeDiff *diff.ValueDiff, schemaDiff *diff.Sche } func breakingTypeFormatChangedInRequestProperty(typeDiff *diff.ValueDiff, formatDiff *diff.ValueDiff, mediaType string, schemaDiff *diff.SchemaDiff) bool { - return (typeDiff != nil || formatDiff != nil) && (typeDiff == nil || typeDiff != nil && - !(typeDiff.From == "integer" && typeDiff.To == "number") && - !(typeDiff.To == "string" && mediaType != "application/json" && mediaType != "application/xml")) && - (formatDiff == nil || formatDiff != nil && formatDiff.To != nil && formatDiff.To != "" && - !(schemaDiff.Revision.Value.Type == "string" && - (formatDiff.From == "date" && formatDiff.To == "date-time" || - formatDiff.From == "time" && formatDiff.To == "date-time")) && - !(schemaDiff.Revision.Value.Type == "number" && - (formatDiff.From == "float" && formatDiff.To == "double")) && - !(schemaDiff.Revision.Value.Type == "integer" && - (formatDiff.From == "int32" && formatDiff.To == "int64" || - formatDiff.From == "int32" && formatDiff.To == "bigint" || - formatDiff.From == "int64" && formatDiff.To == "bigint"))) + + if typeDiff != nil { + return !isTypeContained(typeDiff.To, typeDiff.From, mediaType) + } + + if formatDiff != nil { + return !isFormatContained(schemaDiff.Revision.Value.Type, formatDiff.To, formatDiff.From) + } + + return false } diff --git a/checker/check-response-property-type-changed.go b/checker/check-response-property-type-changed.go index 2075d098..fb6fec69 100644 --- a/checker/check-response-property-type-changed.go +++ b/checker/check-response-property-type-changed.go @@ -2,6 +2,7 @@ package checker import ( "fmt" + "strings" "github.com/tufin/oasdiff/diff" ) @@ -82,14 +83,43 @@ func ResponsePropertyTypeChangedCheck(diffReport *diff.Diff, operationsSources * } func breakingTypeFormatChangedInResponseProperty(typeDiff *diff.ValueDiff, formatDiff *diff.ValueDiff, mediaType string, schemaDiff *diff.SchemaDiff) bool { - return (typeDiff != nil || formatDiff != nil) && (typeDiff == nil || typeDiff != nil && - !(typeDiff.To == "integer" && typeDiff.From == "number") && - !(typeDiff.From == "string" && mediaType != "application/json" && mediaType != "application/xml")) && - (formatDiff == nil || formatDiff != nil && formatDiff.From != nil && formatDiff.From != "" && - !(schemaDiff.Revision.Value.Type == "number" && - (formatDiff.To == "float" && formatDiff.From == "double")) && - !(schemaDiff.Revision.Value.Type == "integer" && - (formatDiff.To == "int32" && formatDiff.From == "int64" || - formatDiff.To == "int32" && formatDiff.From == "bigint" || - formatDiff.To == "int64" && formatDiff.From == "bigint"))) + + if typeDiff != nil { + return !isTypeContained(typeDiff.From, typeDiff.To, mediaType) + } + + if formatDiff != nil { + return !isFormatContained(schemaDiff.Revision.Value.Type, formatDiff.From, formatDiff.To) + } + + return false +} + +// isTypeContained checks if type2 is contained in type1 +func isTypeContained(type1, type2 interface{}, mediaType string) bool { + return (type1 == "number" && type2 == "integer") || + (type1 == "string" && !isJsonMediaType(mediaType) && mediaType != "application/xml") // string can change to anything, unless it's json or xml +} + +// isFormatContained checks if format2 is contained in format1 +func isFormatContained(schemaType string, format1, format2 interface{}) bool { + + switch schemaType { + case "number": + return format1 == "double" && format2 == "float" + case "integer": + return (format1 == "int64" && format2 == "int32") || + (format1 == "bigint" && format2 == "int32") || + (format1 == "bigint" && format2 == "int64") + case "string": + return (format1 == "date-time" && format2 == "date" || + format1 == "date-time" && format2 == "time") + } + + return false +} + +func isJsonMediaType(mediaType string) bool { + return mediaType == "application/json" || + (strings.HasPrefix(mediaType, "application/vnd.") && strings.HasSuffix(mediaType, "+json")) } diff --git a/checker/checker_breaking_request_type_changed_test.go b/checker/checker_breaking_request_type_changed_test.go new file mode 100644 index 00000000..18a88735 --- /dev/null +++ b/checker/checker_breaking_request_type_changed_test.go @@ -0,0 +1,108 @@ +package checker_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/tufin/oasdiff/checker" + "github.com/tufin/oasdiff/diff" +) + +// BC: changing request's body schema type from string to number is breaking +func TestBreaking_ReqTypeStringToNumber(t *testing.T) { + file := "../data/type-change/simple-request.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "string" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "number" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "request-body-type-changed", errs[0].Id) + require.Equal(t, "the request's body type/format changed from 'string'/'none' to 'number'/'none'", errs[0].Text) +} + +// BC: changing request's body schema type from number to string is breaking +func TestBreaking_ReqTypeNumberToString(t *testing.T) { + file := "../data/type-change/simple-request.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "number" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "string" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "request-body-type-changed", errs[0].Id) + require.Equal(t, "the request's body type/format changed from 'number'/'none' to 'string'/'none'", errs[0].Text) +} + +// BC: changing request's body schema type from number to integer is breaking +func TestBreaking_ReqTypeNumberToInteger(t *testing.T) { + file := "../data/type-change/simple-request.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "number" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "integer" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "request-body-type-changed", errs[0].Id) + require.Equal(t, "the request's body type/format changed from 'number'/'none' to 'integer'/'none'", errs[0].Text) +} + +// BC: changing request's body schema type from integer to number is not breaking +func TestBreaking_ReqTypeIntegerToNumber(t *testing.T) { + file := "../data/type-change/simple-request.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "integer" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "number" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Empty(t, errs) +} + +// BC: changing request's body schema type from number/none to integer/int32 is breaking +func TestBreaking_ReqTypeNumberToInt32(t *testing.T) { + file := "../data/type-change/simple-request.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "number" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Type = "integer" + s2.Spec.Paths["/test"].Post.RequestBody.Value.Content["application/json"].Schema.Value.Format = "int32" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "request-body-type-changed", errs[0].Id) + require.Equal(t, "the request's body type/format changed from 'number'/'none' to 'integer'/'int32'", errs[0].Text) +} diff --git a/checker/checker_breaking_response_type_changed_test.go b/checker/checker_breaking_response_type_changed_test.go new file mode 100644 index 00000000..1e79c55a --- /dev/null +++ b/checker/checker_breaking_response_type_changed_test.go @@ -0,0 +1,122 @@ +package checker_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + "github.com/tufin/oasdiff/checker" + "github.com/tufin/oasdiff/diff" +) + +// BC: changing response's body schema type from string to number is breaking +func TestBreaking_RespTypeStringToNumber(t *testing.T) { + file := "../data/type-change/simple-response.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "string" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "number" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "response-body-type-changed", errs[0].Id) + require.Equal(t, "the response's body type/format changed from 'string'/'none' to 'number'/'none' for status '200'", errs[0].Text) +} + +// BC: changing response's body schema type from number to string is breaking +func TestBreaking_RespTypeNumberToString(t *testing.T) { + file := "../data/type-change/simple-response.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "number" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "string" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "response-body-type-changed", errs[0].Id) + require.Equal(t, "the response's body type/format changed from 'number'/'none' to 'string'/'none' for status '200'", errs[0].Text) +} + +// BC: changing response's body schema type from number to integer is not breaking +func TestBreaking_RespTypeNumberToInteger(t *testing.T) { + file := "../data/type-change/simple-response.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "number" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "integer" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Empty(t, errs) +} + +// BC: changing response's body schema type from integer to number is breaking +func TestBreaking_RespTypeIntegerToNumber(t *testing.T) { + file := "../data/type-change/simple-response.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "integer" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "number" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "response-body-type-changed", errs[0].Id) + require.Equal(t, "the response's body type/format changed from 'integer'/'none' to 'number'/'none' for status '200'", errs[0].Text) +} + +// BC: changing response's body schema type from number/none to integer/int32 is not breaking +func TestBreaking_RespTypeNumberToInt32(t *testing.T) { + file := "../data/type-change/simple-response.yaml" + + s1, err := open(file) + require.NoError(t, err) + s1.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "number" + + s2, err := open(file) + require.NoError(t, err) + s2.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Type = "integer" + s2.Spec.Paths["/test"].Get.Responses["200"].Value.Content["application/json"].Schema.Value.Format = "int32" + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Empty(t, errs) +} + +// BC: changing response's embedded property schema type from string/none to integer/int32 is breaking +func TestBreaking_RespTypeChanged(t *testing.T) { + s1, err := open("../data/type-change/base-response.yaml") + require.NoError(t, err) + + s2, err := open("../data/type-change/revision-response.yaml") + require.NoError(t, err) + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + errs := checker.CheckBackwardCompatibility(checker.GetDefaultChecks(), d, osm) + require.Len(t, errs, 1) + require.Equal(t, "response-property-type-changed", errs[0].Id) + require.Equal(t, "the response's property type/format changed from 'string'/'none' to 'integer'/'int32' for status '200'", errs[0].Text) +} diff --git a/data/type-change/base-response.yaml b/data/type-change/base-response.yaml new file mode 100644 index 00000000..dd4d03aa --- /dev/null +++ b/data/type-change/base-response.yaml @@ -0,0 +1,40 @@ +openapi: 3.0.1 +info: + title: Test + version: "2.0" +servers: +- url: http://localhost:8080 +tags: +- name: Tests + description: Test tag. +paths: + /api/atlas/v2/changeOfResponseArrayFieldTest: + get: + tags: + - Tests + summary: This is a test + description: Test description. + operationId: getTest + parameters: + - name: new + in: query + description: Test param + schema: + type: string + responses: + "200": + description: OK + content: + application/vnd.atlas.2023-01-01+json: + schema: + type: array + items: + $ref: '#/components/schemas/ChangeOfResponseArrayFieldTestView' +components: + schemas: + ChangeOfResponseArrayFieldTestView: + type: object + properties: + testField: + type: string + description: A nested view diff --git a/data/type-change/revision-response.yaml b/data/type-change/revision-response.yaml new file mode 100644 index 00000000..6a4e6ce3 --- /dev/null +++ b/data/type-change/revision-response.yaml @@ -0,0 +1,41 @@ +openapi: 3.0.1 +info: + title: Test + version: "2.0" +servers: +- url: http://localhost:8080 +tags: +- name: Tests + description: Test tag. +paths: + /api/atlas/v2/changeOfResponseArrayFieldTest: + get: + tags: + - Tests + summary: This is a test + description: Test description. + operationId: getTest + parameters: + - name: new + in: query + description: Test param + schema: + type: string + responses: + "200": + description: OK + content: + application/vnd.atlas.2023-01-01+json: + schema: + type: array + items: + $ref: '#/components/schemas/ChangeOfResponseArrayFieldTestView' +components: + schemas: + ChangeOfResponseArrayFieldTestView: + type: object + properties: + testField: + type: integer + description: A nested view + format: int32 diff --git a/data/type-change/simple-request.yaml b/data/type-change/simple-request.yaml new file mode 100644 index 00000000..73decc26 --- /dev/null +++ b/data/type-change/simple-request.yaml @@ -0,0 +1,12 @@ +openapi: 3.0.1 +info: + title: Test + version: "2.0" +paths: + /test: + post: + requestBody: + content: + application/json: + schema: + type: "string" diff --git a/data/type-change/simple-response.yaml b/data/type-change/simple-response.yaml new file mode 100644 index 00000000..d052ed70 --- /dev/null +++ b/data/type-change/simple-response.yaml @@ -0,0 +1,13 @@ +openapi: 3.0.1 +info: + title: Test + version: "2.0" +paths: + /test: + get: + responses: + "200": + content: + application/json: + schema: + type: "string"