fix-type-breaking-changes (#286)

* replicate issue

* fixing type change checks

* finish response checks

* fixed request

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)  

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
 }

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"))
 }

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)
+}

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)
+}

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