From 0098f7341163e4f1aab1001438eab5ec7af855c6 Mon Sep 17 00:00:00 2001 From: Derbylock Date: Tue, 8 Aug 2023 15:24:01 +0300 Subject: [PATCH] Fixed panic when body type changed from array to object (#357) --- BREAKING-CHANGES-EXAMPLES.md | 4 +- .../check-request-property-type-changed.go | 3 + ...heck-request-property-type-changed_test.go | 64 +++++++++++++++++++ checker/localizations/localizations.go | 4 +- checker/localizations_src/ru/messages.yaml | 4 +- ...rty_type_changed_base_array_to_object.yaml | 46 +++++++++++++ ...type_changed_revision_array_to_object.yaml | 61 ++++++++++++++++++ diff/schema_diff.go | 59 +++++------------ 8 files changed, 198 insertions(+), 47 deletions(-) create mode 100644 data/checker/request_property_type_changed_base_array_to_object.yaml create mode 100644 data/checker/request_property_type_changed_revision_array_to_object.yaml diff --git a/BREAKING-CHANGES-EXAMPLES.md b/BREAKING-CHANGES-EXAMPLES.md index 8326da09..c59cd3c6 100644 --- a/BREAKING-CHANGES-EXAMPLES.md +++ b/BREAKING-CHANGES-EXAMPLES.md @@ -200,6 +200,8 @@ These examples are automatically generated from unit tests. [changing optional response property to write-only](checker/check-response-optional-property-write-only-read-only_test.go?plain=1#L11) [changing optional response write-only property to required](checker/check-response-property-became-required_test.go?plain=1#L33) [changing pattern of request parameters](checker/check-request-parameter-pattern-added-or-changed_test.go?plain=1#L11) +[changing request body and property types from array to object](checker/check-request-property-type-changed_test.go?plain=1#L84) +[changing request body and property types from object to array](checker/check-request-property-type-changed_test.go?plain=1#L116) [changing request body default value](checker/check-request-property-default-value-changed_test.go?plain=1#L11) [changing request body to not nullable](checker/check-request-property-became-not-nuallable_test.go?plain=1#L84) [changing request body to nullable](checker/check-request-property-became-not-nuallable_test.go?plain=1#L58) @@ -211,7 +213,7 @@ These examples are automatically generated from unit tests. [changing request path parameter format](checker/check-request-parameters-type-changed_test.go?plain=1#L86) [changing request path parameter type](checker/check-request-parameters-type-changed_test.go?plain=1#L11) [changing request property default value](checker/check-request-property-default-value-changed_test.go?plain=1#L34) -[changing request property format](checker/check-request-property-type-changed_test.go?plain=1#L84) +[changing request property format](checker/check-request-property-type-changed_test.go?plain=1#L148) [changing request property pattern](checker/check-request-property-pattern-added-or-changed_test.go?plain=1#L11) [changing request property required value to false](checker/check-request-property-required-updated_test.go?plain=1#L34) [changing request property required value to true](checker/check-request-property-required-updated_test.go?plain=1#L11) diff --git a/checker/check-request-property-type-changed.go b/checker/check-request-property-type-changed.go index ea9c5f9b..839bc61e 100644 --- a/checker/check-request-property-type-changed.go +++ b/checker/check-request-property-type-changed.go @@ -49,6 +49,9 @@ func RequestPropertyTypeChangedCheck(diffReport *diff.Diff, operationsSources *d CheckModifiedPropertiesDiff( mediaTypeDiff.SchemaDiff, func(propertyPath string, propertyName string, propertyDiff *diff.SchemaDiff, parent *diff.SchemaDiff) { + if propertyDiff.Revision == nil || propertyDiff.Revision.Value == nil { + return + } if propertyDiff.Revision.Value.ReadOnly { return } diff --git a/checker/check-request-property-type-changed_test.go b/checker/check-request-property-type-changed_test.go index b5b2f546..6e5fcc25 100644 --- a/checker/check-request-property-type-changed_test.go +++ b/checker/check-request-property-type-changed_test.go @@ -81,6 +81,70 @@ func TestRequestPropertyTypeChangedCheck(t *testing.T) { }, errs[0]) } +// CL: changing request body and property types from array to object +func TestRequestBodyAndPropertyTypesChangedCheckArrayToObject(t *testing.T) { + s1, err := open("../data/checker/request_property_type_changed_base_array_to_object.yaml") + require.NoError(t, err) + s2, err := open("../data/checker/request_property_type_changed_revision_array_to_object.yaml") + require.NoError(t, err) + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + + errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestPropertyTypeChangedCheck), d, osm, checker.ERR) + require.Len(t, errs, 2) + require.Equal(t, checker.ApiChange{ + Id: "request-property-type-changed", + Level: checker.ERR, + Text: "the 'colors' request property type/format changed from 'array'/'none' to 'object'/'none'", + Operation: "POST", + Path: "/dogs", + Source: "../data/checker/request_property_type_changed_revision_array_to_object.yaml", + OperationId: "addDog", + }, errs[0]) + require.Equal(t, checker.ApiChange{ + Id: "request-body-type-changed", + Level: checker.ERR, + Text: "the request's body type/format changed from 'array'/'none' to 'object'/'none'", + Operation: "POST", + Path: "/pets", + Source: "../data/checker/request_property_type_changed_revision_array_to_object.yaml", + OperationId: "addPet", + }, errs[1]) +} + +// CL: changing request body and property types from object to array +func TestRequestBodyAndPropertyTypesChangedCheckObjectToArray(t *testing.T) { + s1, err := open("../data/checker/request_property_type_changed_revision_array_to_object.yaml") + require.NoError(t, err) + s2, err := open("../data/checker/request_property_type_changed_base_array_to_object.yaml") + require.NoError(t, err) + + d, osm, err := diff.GetWithOperationsSourcesMap(getConfig(), s1, s2) + require.NoError(t, err) + + errs := checker.CheckBackwardCompatibilityUntilLevel(singleCheckConfig(checker.RequestPropertyTypeChangedCheck), d, osm, checker.ERR) + require.Len(t, errs, 2) + require.Equal(t, checker.ApiChange{ + Id: "request-property-type-changed", + Level: checker.ERR, + Text: "the 'colors' request property type/format changed from 'object'/'none' to 'array'/'none'", + Operation: "POST", + Path: "/dogs", + Source: "../data/checker/request_property_type_changed_base_array_to_object.yaml", + OperationId: "addDog", + }, errs[0]) + require.Equal(t, checker.ApiChange{ + Id: "request-body-type-changed", + Level: checker.ERR, + Text: "the request's body type/format changed from 'object'/'none' to 'array'/'none'", + Operation: "POST", + Path: "/pets", + Source: "../data/checker/request_property_type_changed_base_array_to_object.yaml", + OperationId: "addPet", + }, errs[1]) +} + // CL: changing request property format func TestRequestPropertyFormatChangedCheck(t *testing.T) { s1, err := open("../data/checker/request_property_type_changed_base.yaml") diff --git a/checker/localizations/localizations.go b/checker/localizations/localizations.go index da7a18fc..d9c16b87 100644 --- a/checker/localizations/localizations.go +++ b/checker/localizations/localizations.go @@ -311,11 +311,11 @@ var localizations = map[string]string{ "ru.messages.at": "в", "ru.messages.in": "в", "ru.messages.new-optional-request-parameter": "добавлен новый необязательный %s параметр зароса %s", - "ru.messages.new-optional-request-property": "добавлено новле необязательное поле запроса %s", + "ru.messages.new-optional-request-property": "добавлено новое необязательное поле запроса %s", "ru.messages.new-request-path-parameter": "добален новый path параметр запроса %s", "ru.messages.new-required-request-header-property": "в заголовке запроса %s добавлено новое обязательное поле %s", "ru.messages.new-required-request-parameter": "добавлен новый обязательный %s параметр зароса %s", - "ru.messages.new-required-request-property": "добавлено новле обязательное поле запроса %s", + "ru.messages.new-required-request-property": "добавлено новое обязательное поле запроса %s", "ru.messages.optional-response-header-removed": "удалён ранее необязательный заголовок ответа %s для ответа со статусом %s", "ru.messages.pattern-changed-warn-comment": "Это предупреждение, потому что сложно автоматически проанализировать, является ли новый шаблон надмножеством предыдущего шаблона (например, изменен с '[0-9]+' на '[0-9]*').", "ru.messages.request-allOf-modified": "изменено allOf для поля запроса %s", diff --git a/checker/localizations_src/ru/messages.yaml b/checker/localizations_src/ru/messages.yaml index 8e8476e7..bb2cf59f 100644 --- a/checker/localizations_src/ru/messages.yaml +++ b/checker/localizations_src/ru/messages.yaml @@ -28,8 +28,8 @@ sunset-deleted: удалена дата sunset date у API, но сохранё api-sunset-date-changed-too-small: дата sunset у API изменена на более раннюю с %s на %s, новая дата sunset должна быть либо не раньше %s, либо, как минимум, %d дней от текущего дня new-required-request-parameter: добавлен новый обязательный %s параметр зароса %s new-optional-request-parameter: добавлен новый необязательный %s параметр зароса %s -new-required-request-property: добавлено новле обязательное поле запроса %s -new-optional-request-property: добавлено новле необязательное поле запроса %s +new-required-request-property: добавлено новое обязательное поле запроса %s +new-optional-request-property: добавлено новое необязательное поле запроса %s new-required-request-header-property: в заголовке запроса %s добавлено новое обязательное поле %s request-body-became-required: тело запроса стало обязательным request-body-became-optional: тело запроса стало необязательным diff --git a/data/checker/request_property_type_changed_base_array_to_object.yaml b/data/checker/request_property_type_changed_base_array_to_object.yaml new file mode 100644 index 00000000..4df9fda3 --- /dev/null +++ b/data/checker/request_property_type_changed_base_array_to_object.yaml @@ -0,0 +1,46 @@ +openapi: 3.0.1 +info: + title: Sample API + version: 1.0.0 +paths: + /pets: + post: + summary: Add a new pet + operationId: addPet + requestBody: + required: true + content: + application/json: + schema: + type: array + items: + type: object + properties: + name: + type: string + format: string + age: + type: integer + format: int32 + required: + - name + /dogs: + post: + summary: Add a new dog + operationId: addDog + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + name: + type: string + format: string + colors: + type: array + items: + type: integer + required: + - name diff --git a/data/checker/request_property_type_changed_revision_array_to_object.yaml b/data/checker/request_property_type_changed_revision_array_to_object.yaml new file mode 100644 index 00000000..a85097a5 --- /dev/null +++ b/data/checker/request_property_type_changed_revision_array_to_object.yaml @@ -0,0 +1,61 @@ +openapi: 3.0.1 +info: + title: Sample API + version: 1.0.0 +paths: + /pets: + post: + summary: Add a new pet + operationId: addPet + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - data + properties: + data: + type: array + items: + type: object + properties: + name: + type: string + format: string + age: + type: integer + format: int32 + required: + - name + /dogs: + post: + summary: Add a new dog + operationId: addDog + requestBody: + required: true + content: + application/json: + schema: + type: object + properties: + name: + type: string + format: string + colors: + type: object + properties: + red: + type: integer + green: + type: integer + blue: + type: integer + required: + - red + - blue + - green + required: + - name + diff --git a/diff/schema_diff.go b/diff/schema_diff.go index fd7d67c0..50bfb3e5 100644 --- a/diff/schema_diff.go +++ b/diff/schema_diff.go @@ -78,23 +78,29 @@ func getSchemaDiff(config *Config, state *state, schema1, schema2 *openapi3.Sche } func getSchemaDiffInternal(config *Config, state *state, schema1, schema2 *openapi3.SchemaRef) (*SchemaDiff, error) { - if status := getSchemaStatus(schema1, schema2); status != schemaStatusOK { - switch status { - case schemaStatusNoSchemas: - return nil, nil - case schemaStatusSchemaAdded: - return &SchemaDiff{SchemaAdded: true}, nil - case schemaStatusSchemaDeleted: - return &SchemaDiff{SchemaDeleted: true}, nil - } + + result := SchemaDiff{ + Base: schema1, + Revision: schema2, + } + + if schema1 == nil && schema2 == nil { + return &result, nil + } else if schema1 == nil { + result.SchemaAdded = true + return &result, nil + } else if schema2 == nil { + result.SchemaDeleted = true + return &result, nil } if status := getCircularRefsDiff(state.visitedSchemasBase, state.visitedSchemasRevision, schema1, schema2); status != circularRefStatusNone { switch status { case circularRefStatusDiff: - return &SchemaDiff{CircularRefDiff: true}, nil + result.CircularRefDiff = true + return &result, nil case circularRefStatusNoDiff: - return nil, nil + return &result, nil } } @@ -119,8 +125,6 @@ func getSchemaDiffInternal(config *Config, state *state, schema1, schema2 *opena defer state.visitedSchemasRevision.Remove(schema2.Ref) } - result := SchemaDiff{} - result.ExtensionsDiff = getExtensionsDiff(config, state, value1.Extensions, value2.Extensions) result.OneOfDiff, err = getSchemaListsDiff(config, state, value1.OneOf, value2.OneOf) if err != nil { @@ -186,38 +190,9 @@ func getSchemaDiffInternal(config *Config, state *state, schema1, schema2 *opena result.DiscriminatorDiff = getDiscriminatorDiff(config, state, value1.Discriminator, value2.Discriminator) - result.Base = schema1 - result.Revision = schema2 - return &result, nil } -type schemaStatus int - -const ( - schemaStatusOK schemaStatus = iota - schemaStatusNoSchemas - schemaStatusSchemaAdded - schemaStatusSchemaDeleted - schemaStatusCircularRefDiff -) - -func getSchemaStatus(schema1, schema2 *openapi3.SchemaRef) schemaStatus { - if schema1 == nil && schema2 == nil { - return schemaStatusNoSchemas - } - - if schema1 == nil && schema2 != nil { - return schemaStatusSchemaAdded - } - - if schema1 != nil && schema2 == nil { - return schemaStatusSchemaDeleted - } - - return schemaStatusOK -} - func derefSchema(ref *openapi3.SchemaRef) (*openapi3.Schema, error) { if ref == nil || ref.Value == nil { return nil, errors.New("schema reference is nil")