From 3582dd504c988c0176d250976285d8bb6c37d11e Mon Sep 17 00:00:00 2001 From: lchan Date: Mon, 4 May 2020 16:22:51 -0700 Subject: [PATCH 01/37] add TestAccCDN_CreateResourceWithIgnoreListOrderExtension - check that plan should result in no diffs for resource with array property where elements may not always be returned in the same order - test currently failing - pending implementation of fix --- tests/e2e/gray_box_cdns_test.go | 124 ++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/tests/e2e/gray_box_cdns_test.go b/tests/e2e/gray_box_cdns_test.go index b4e51a376..6895572e1 100644 --- a/tests/e2e/gray_box_cdns_test.go +++ b/tests/e2e/gray_box_cdns_test.go @@ -438,6 +438,130 @@ func (a *api) apiResponse(t *testing.T, responseBody string, httpResponseStatusC } } +//TODO: Check that test passes after implementing fix to ignore order of array props +func TestAccCDN_CreateResourceWithIgnoreListOrderExtension(t *testing.T) { + swagger := `swagger: "2.0" +host: %s +schemes: +- "http" + +paths: + ###################### + #### CDN Resource #### + ###################### + + /v1/cdns: + x-terraform-resource-name: "cdn" + post: + summary: "Create cdn" + operationId: "ContentDeliveryNetworkCreateV1" + parameters: + - in: "body" + name: "body" + description: "Created CDN" + required: true + schema: + $ref: "#/definitions/ContentDeliveryNetworkV1" + responses: + 201: + description: "successful operation" + schema: + $ref: "#/definitions/ContentDeliveryNetworkV1" + /v1/cdns/{cdn_id}: + get: + summary: "Get cdn by id" + description: "" + operationId: "ContentDeliveryNetworkGetV1" + parameters: + - name: "cdn_id" + in: "path" + description: "The cdn id that needs to be fetched." + required: true + type: "string" + responses: + 200: + description: "successful operation" + schema: + $ref: "#/definitions/ContentDeliveryNetworkV1" + delete: + summary: "Delete cdn" + operationId: "ContentDeliveryNetworkDeleteV1" + parameters: + - name: "id" + in: "path" + description: "The cdn that needs to be deleted" + required: true + type: "string" + responses: + 204: + description: "successful operation, no content is returned" +definitions: + ContentDeliveryNetworkV1: + type: "object" + required: + - label + properties: + id: + type: "string" + readOnly: true + label: + type: "string" + list_prop: + type: "array" + items: + type: "string"` + apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodDelete { + w.WriteHeader(http.StatusNotFound) + return + } + body := `{"id": "someid", "label":"some label", "list_prop": ["value1", "value2", "value3"]}` + w.WriteHeader(http.StatusOK) + w.Write([]byte(body)) + })) + apiHost := apiServer.URL[7:] + swaggerServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + swaggerReturned := fmt.Sprintf(swagger, apiHost) + w.Write([]byte(swaggerReturned)) + })) + + tfFileContents := `# URI /v1/cdns/ +resource "openapi_cdn_v1" "my_cdn" { + label = "some label" + list_prop = ["value3", "value1", "value2"] +}` + + p := openapi.ProviderOpenAPI{ProviderName: providerName} + provider, err := p.CreateSchemaProviderFromServiceConfiguration(&openapi.ServiceConfigStub{SwaggerURL: swaggerServer.URL}) + assert.NoError(t, err) + + var testAccProviders = map[string]terraform.ResourceProvider{providerName: provider} + resource.Test(t, resource.TestCase{ + IsUnitTest: true, + Providers: testAccProviders, + PreCheck: func() { testAccPreCheck(t, swaggerServer.URL) }, + Steps: []resource.TestStep{ + { + ExpectNonEmptyPlan: false, + Config: tfFileContents, + Check: resource.ComposeTestCheckFunc( + // check resource + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "label", "some label"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "list_prop.#", "3"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "list_prop.0", "value3"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "list_prop.1", "value1"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "list_prop.2", "value2"), + ), + }, + }, + }) +} + func TestAccCDN_Create_and_UpdateSubResource(t *testing.T) { api := initAPI(t, cdnSwaggerYAMLTemplate) tfFileContents := createTerraformFile(expectedCDNLabel, expectedCDNFirewallLabel) From 7484ad87b7f9186ffa03f1dcbc7473d539bc657f Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 5 May 2020 15:30:47 -0700 Subject: [PATCH 02/37] create method that compares local proeprty value with remote - this method will be in charge of deciding whether the state needs to be updated with the property value or not. terraform saves already the input properties and values from the tf file into the ResourceData property, so if the values from the API matches the ones introduced by the user then the update does not need to updated. For other properties through like computed the state needs to be updated accordingly. --- openapi/common.go | 12 +++++++++++ openapi/common_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/openapi/common.go b/openapi/common.go index 1d318d3c3..edd605650 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -108,6 +108,18 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str return nil } +func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) (bool, error) { + if property.Required { + switch property.Type { + case typeString, typeInt: + if inputPropertyValue == remoteValue { + return true, nil + } + } + } + return false, nil +} + func convertPayloadToLocalStateDataValue(property *specSchemaDefinitionProperty, propertyValue interface{}, useString bool) (interface{}, error) { if propertyValue == nil { return nil, nil diff --git a/openapi/common_test.go b/openapi/common_test.go index 6bd88319b..95ab3cb06 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -788,3 +788,49 @@ func TestSetStateID(t *testing.T) { }) }) } + +func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { + testCases := []struct { + name string + property specSchemaDefinitionProperty + inputPropertyValue interface{} + remoteValue interface{} + expectedSkip bool + expectedError error + }{ + { + name: "required input string matches the value returned by the API", + property: specSchemaDefinitionProperty{ + Name: "string_prop", + Type: typeString, + Required: true, + }, + inputPropertyValue: "inputValue", + remoteValue: "inputValue", + expectedSkip: true, + expectedError: nil, + }, + { + name: "required input integer matches the value returned by the API", + property: specSchemaDefinitionProperty{ + Name: "int_prop", + Type: typeInt, + Required: true, + }, + inputPropertyValue: 123, + remoteValue: 123, + expectedSkip: true, + expectedError: nil, + }, + } + + for _, tc := range testCases { + skip, err := compareInputPropertyValueWithPayloadPropertyValue(tc.property, tc.inputPropertyValue, tc.remoteValue) + if tc.expectedError == nil { + assert.Nil(t, err, tc.name) + assert.Equal(t, tc.expectedSkip, skip) + } else { + assert.Equal(t, tc.expectedError.Error(), err.Error(), tc.name) + } + } +} From 8aeb727f8481977ba9f8985c72b8d1f125c18422 Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 5 May 2020 15:43:57 -0700 Subject: [PATCH 03/37] add typeFloat and typeBool use case --- openapi/common.go | 3 ++- openapi/common_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/openapi/common.go b/openapi/common.go index edd605650..ddc0c01f2 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -95,6 +95,7 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str if property.isPropertyNamedID() { continue } + value, err := convertPayloadToLocalStateDataValue(property, propertyValue, false) if err != nil { return err @@ -111,7 +112,7 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) (bool, error) { if property.Required { switch property.Type { - case typeString, typeInt: + case typeString, typeInt, typeFloat, typeBool: if inputPropertyValue == remoteValue { return true, nil } diff --git a/openapi/common_test.go b/openapi/common_test.go index 95ab3cb06..e5e169d4e 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -822,6 +822,30 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { expectedSkip: true, expectedError: nil, }, + { + name: "required input number matches the value returned by the API", + property: specSchemaDefinitionProperty{ + Name: "number_prop", + Type: typeFloat, + Required: true, + }, + inputPropertyValue: 99.99, + remoteValue: 99.99, + expectedSkip: true, + expectedError: nil, + }, + { + name: "required input bool matches the value returned by the API", + property: specSchemaDefinitionProperty{ + Name: "bool_prop", + Type: typeBool, + Required: true, + }, + inputPropertyValue: true, + remoteValue: true, + expectedSkip: true, + expectedError: nil, + }, } for _, tc := range testCases { From 692767bc1e5bc86c47178d47e365741a5254fbfc Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 5 May 2020 16:27:22 -0700 Subject: [PATCH 04/37] Add initial checks on typeList in compareInputPropertyValueWithPayloadPropertyValue - Check that input prop items are found in the remote items - otherwise, return an error - Add test cases for list prop - input and remote with values in same order, input and remote with values in different order --- openapi/common.go | 16 ++++++++++++++++ openapi/common_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/openapi/common.go b/openapi/common.go index ddc0c01f2..a6b0c0bba 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -116,6 +116,22 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini if inputPropertyValue == remoteValue { return true, nil } + case typeList: + if property.ArrayItemsType == typeString { + for _, inputItemValue := range inputPropertyValue.([]interface{}) { + foundMatch := false + for _, remoteItemValue := range remoteValue.([]interface{}) { + if inputItemValue == remoteItemValue { + foundMatch = true + break + } + } + if !foundMatch { + return false, fmt.Errorf("API returned a payload containg an array property (%s) which does not contain the expected desired items specified by the user", property.Name) + } + } + } + return true, nil } } return false, nil diff --git a/openapi/common_test.go b/openapi/common_test.go index e5e169d4e..dfb07df8f 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -846,6 +846,30 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { expectedSkip: true, expectedError: nil, }, + { + name: "required input list matches the value returned by the API where order of input values match", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + }, + inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, + remoteValue: []string{"inputVal1", "inputVal2", "inputVal3"}, + expectedSkip: true, + expectedError: nil, + }, + { + name: "required input list matches the value returned by the API where order of input values doesn't match", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + }, + inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, + remoteValue: []string{"inputVal2", "inputVal3", "inputVal1"}, + expectedSkip: true, + expectedError: nil, + }, } for _, tc := range testCases { From 8c9e2c177437016475e3b2b5e55f69b34544f458 Mon Sep 17 00:00:00 2001 From: dikhan Date: Thu, 7 May 2020 15:11:09 -0700 Subject: [PATCH 05/37] add support for x-terraform-ignore-order extension --- ...pec_resource_schema_definition_property.go | 6 +++- openapi/openapi_v2_resource.go | 6 ++++ openapi/openapi_v2_resource_test.go | 28 +++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index d13a8529f..7c54f4acd 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -28,7 +28,11 @@ type specSchemaDefinitionProperty struct { PreferredName string Type schemaDefinitionPropertyType ArrayItemsType schemaDefinitionPropertyType - Required bool + + // IgnoreItemsOrder if set to true means that the array items order should be ignored + IgnoreItemsOrder bool + + Required bool // ReadOnly properties are included in responses but not in request ReadOnly bool // Computed properties describe properties where the value is computed by the API diff --git a/openapi/openapi_v2_resource.go b/openapi/openapi_v2_resource.go index 0153cc0df..23342f9bd 100644 --- a/openapi/openapi_v2_resource.go +++ b/openapi/openapi_v2_resource.go @@ -57,6 +57,7 @@ const extTfFieldStatus = "x-terraform-field-status" const extTfID = "x-terraform-id" const extTfComputed = "x-terraform-computed" const extTfComplexObjectType = "x-terraform-complex-object-legacy-config" +const extTfIgnoreOrder = "x-terraform-ignore-order" // Operation level extensions const extTfResourceTimeout = "x-terraform-resource-timeout" @@ -390,6 +391,11 @@ func (o *SpecV2Resource) createSchemaDefinitionProperty(propertyName string, pro } schemaDefinitionProperty.ArrayItemsType = itemsType schemaDefinitionProperty.SpecSchemaDefinition = itemsSchema // only diff than nil if type is object + + if o.isBoolExtensionEnabled(property.Extensions, extTfIgnoreOrder) { + schemaDefinitionProperty.IgnoreItemsOrder = true + } + log.Printf("[DEBUG] found array type property '%s' with items of type '%s'", propertyName, itemsType) } diff --git a/openapi/openapi_v2_resource_test.go b/openapi/openapi_v2_resource_test.go index ef8a74508..4b7947e00 100644 --- a/openapi/openapi_v2_resource_test.go +++ b/openapi/openapi_v2_resource_test.go @@ -2468,6 +2468,34 @@ func TestCreateSchemaDefinitionProperty(t *testing.T) { }) }) + Convey("When createSchemaDefinitionProperty is called with a property schema that has the 'x-terraform-ignore-order' extension", func() { + expectedIgnoreOrder := true + propertySchema := spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: spec.StringOrArray{"string"}, + }, + }, + }, + }, + VendorExtensible: spec.VendorExtensible{ + Extensions: spec.Extensions{ + extTfIgnoreOrder: expectedIgnoreOrder, + }, + }, + } + schemaDefinitionProperty, err := r.createSchemaDefinitionProperty("propertyName", propertySchema, []string{}) + Convey("Then the error returned should be nil", func() { + So(err, ShouldBeNil) + }) + Convey("And the schema definition property should have the IgnoreItemsOrder field enabled", func() { + So(schemaDefinitionProperty.IgnoreItemsOrder, ShouldEqual, expectedIgnoreOrder) + }) + }) + Convey("When createSchemaDefinitionProperty is called with a property schema that has the 'x-terraform-field-status' extension", func() { expectedIsStatusFieldValue := true propertySchema := spec.Schema{ From 830e55a50280626f578bc0052dac18cc8ea15742 Mon Sep 17 00:00:00 2001 From: lchan Date: Thu, 7 May 2020 15:14:06 -0700 Subject: [PATCH 06/37] Update compareInputPropertyValueWithPayloadPropertyValue - cast to []string - Add related test cases --- openapi/common.go | 6 +++--- openapi/common_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index a6b0c0bba..ed568a299 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -118,16 +118,16 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini } case typeList: if property.ArrayItemsType == typeString { - for _, inputItemValue := range inputPropertyValue.([]interface{}) { + for _, inputItemValue := range inputPropertyValue.([]string) { foundMatch := false - for _, remoteItemValue := range remoteValue.([]interface{}) { + for _, remoteItemValue := range remoteValue.([]string) { if inputItemValue == remoteItemValue { foundMatch = true break } } if !foundMatch { - return false, fmt.Errorf("API returned a payload containg an array property (%s) which does not contain the expected desired items specified by the user", property.Name) + return false, fmt.Errorf("API returned a payload containing an array property (%s) which does not contain the expected desired items specified by the user", property.Name) } } } diff --git a/openapi/common_test.go b/openapi/common_test.go index dfb07df8f..2c986f248 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -870,6 +870,30 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { expectedSkip: true, expectedError: nil, }, + { + name: "required input list doesn't match the value returned by the API (same list length)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeString, + }, + inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, + remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, + expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), + }, + { + name: "required input list doesn't match the value returned by the API (different list length)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeString, + }, + inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, + remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, + expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), + }, } for _, tc := range testCases { From 16107a5cbfe4c4fc3c30e5eea6bd075b1f52a278 Mon Sep 17 00:00:00 2001 From: lchan Date: Thu, 7 May 2020 15:47:10 -0700 Subject: [PATCH 07/37] Add method (shouldIgnoreArrayItemsOrder) and related test to see if property has the x-terraform-ignore-order ext set --- ...pec_resource_schema_definition_property.go | 4 ++ ...esource_schema_definition_property_test.go | 54 +++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 7c54f4acd..02a4dd759 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -130,6 +130,10 @@ func (s *specSchemaDefinitionProperty) isOptional() bool { return !s.Required } +func (s *specSchemaDefinitionProperty) shouldIgnoreArrayItemsOrder() bool { + return s.isArrayProperty() && s.IgnoreItemsOrder +} + // isComputed returns true if one of the following cases is met: //- The property is optional (marked as required=false), in which case there few use cases: // - readOnly properties (marked as readOnly=true): diff --git a/openapi/openapi_spec_resource_schema_definition_property_test.go b/openapi/openapi_spec_resource_schema_definition_property_test.go index 8b1befd2f..0eee9442d 100644 --- a/openapi/openapi_spec_resource_schema_definition_property_test.go +++ b/openapi/openapi_spec_resource_schema_definition_property_test.go @@ -354,6 +354,60 @@ func TestIsRequired(t *testing.T) { }) } +func TestShouldIgnoreArrayItemsOrder(t *testing.T) { + Convey("Given a specSchemaDefinitionProperty that is a typeList and where the 'x-terraform-ignore-order' ext is set to true", t, func() { + s := &specSchemaDefinitionProperty{ + Name: "array_prop", + Type: typeList, + IgnoreItemsOrder: true, + } + Convey("When shouldIgnoreArrayItemsOrder method is called", func() { + isRequired := s.shouldIgnoreArrayItemsOrder() + Convey("Then the resulted bool should be true", func() { + So(isRequired, ShouldBeTrue) + }) + }) + }) + Convey("Given a specSchemaDefinitionProperty that is a typeList and where the 'x-terraform-ignore-order' ext is set to false", t, func() { + s := &specSchemaDefinitionProperty{ + Name: "array_prop", + Type: typeList, + IgnoreItemsOrder: false, + } + Convey("When shouldIgnoreArrayItemsOrder method is called", func() { + isRequired := s.shouldIgnoreArrayItemsOrder() + Convey("Then the resulted bool should be false", func() { + So(isRequired, ShouldBeFalse) + }) + }) + }) + Convey("Given a specSchemaDefinitionProperty that is a typeList and where the 'x-terraform-ignore-order' ext is NOT set", t, func() { + s := &specSchemaDefinitionProperty{ + Name: "array_prop", + Type: typeList, + } + Convey("When shouldIgnoreArrayItemsOrder method is called", func() { + isRequired := s.shouldIgnoreArrayItemsOrder() + Convey("Then the resulted bool should be false", func() { + So(isRequired, ShouldBeFalse) + }) + }) + }) + Convey("Given a specSchemaDefinitionProperty that is NOT a typeList where the 'x-terraform-ignore-order' ext is set", t, func() { + s := &specSchemaDefinitionProperty{ + Name: "string_prop", + Type: typeString, + IgnoreItemsOrder: true, + } + Convey("When shouldIgnoreArrayItemsOrder method is called", func() { + isRequired := s.shouldIgnoreArrayItemsOrder() + Convey("Then the resulted bool should be false", func() { + So(isRequired, ShouldBeFalse) + }) + }) + }) +} + func TestSchemaDefinitionPropertyIsComputed(t *testing.T) { Convey("Given a specSchemaDefinitionProperty that is optional and readonly", t, func() { s := &specSchemaDefinitionProperty{ From 337ae097a1b139ebe468936f74adba0ee411c04c Mon Sep 17 00:00:00 2001 From: dikhan Date: Thu, 7 May 2020 16:33:08 -0700 Subject: [PATCH 08/37] modify logic to cover the new set of use cases - compareInputPropertyValueWithPayloadPropertyValue will not returned an updated array keeping as much as possible the order from the input; however if there are diffs with the APIs remote value those will be considered too in the resulted array --- openapi/common.go | 37 +++++++++---- openapi/common_test.go | 122 ++++++++++++----------------------------- 2 files changed, 60 insertions(+), 99 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index ed568a299..be6ae6e91 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -109,32 +109,47 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str return nil } -func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) (bool, error) { +// Use case 0: The desired state for an array property (input from user) contains items in certain order AND the remote state comes back with the same items in the same order. +// Use case 1: The desired state for an array property (input from user) contains items in certain order BUT the remote state comes back with the same items in different order. +// Use case 2: The desired state for an array property (input from user) contains items in certain order BUT the remote state comes back with the same items in different order PLUS new ones. +// Use case 3: The desired state for an array property (input from user) contains items in certain order BUT the remote state comes back with a shorter list where the remaining elems match the inputs. +// Use case 4: The desired state for an array property (input from user) contains items in certain order BUT the remote state some back with the list with the same size but some elems were updated +func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) interface{} { + newPropertyValue := []interface{}{} if property.Required { switch property.Type { - case typeString, typeInt, typeFloat, typeBool: - if inputPropertyValue == remoteValue { - return true, nil - } case typeList: if property.ArrayItemsType == typeString { for _, inputItemValue := range inputPropertyValue.([]string) { - foundMatch := false for _, remoteItemValue := range remoteValue.([]string) { if inputItemValue == remoteItemValue { - foundMatch = true + newPropertyValue = append(newPropertyValue, inputItemValue) + break + } + } + } + modifiedItems := []interface{}{} + for _, remoteItemValue := range remoteValue.([]string) { + match := false + for _, inputItemValue := range inputPropertyValue.([]string) { + if inputItemValue == remoteItemValue { + match = true break } } - if !foundMatch { - return false, fmt.Errorf("API returned a payload containing an array property (%s) which does not contain the expected desired items specified by the user", property.Name) + if !match { + modifiedItems = append(modifiedItems, remoteItemValue) } } + for _, updatedItem := range modifiedItems { + newPropertyValue = append(newPropertyValue, updatedItem) + } + } - return true, nil + return newPropertyValue } } - return false, nil + return newPropertyValue } func convertPayloadToLocalStateDataValue(property *specSchemaDefinitionProperty, propertyValue interface{}, useString bool) (interface{}, error) { diff --git a/openapi/common_test.go b/openapi/common_test.go index 2c986f248..35e0c0362 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -795,114 +795,60 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { property specSchemaDefinitionProperty inputPropertyValue interface{} remoteValue interface{} - expectedSkip bool - expectedError error + expectedOutput []interface{} }{ - { - name: "required input string matches the value returned by the API", - property: specSchemaDefinitionProperty{ - Name: "string_prop", - Type: typeString, - Required: true, - }, - inputPropertyValue: "inputValue", - remoteValue: "inputValue", - expectedSkip: true, - expectedError: nil, - }, - { - name: "required input integer matches the value returned by the API", - property: specSchemaDefinitionProperty{ - Name: "int_prop", - Type: typeInt, - Required: true, - }, - inputPropertyValue: 123, - remoteValue: 123, - expectedSkip: true, - expectedError: nil, - }, - { - name: "required input number matches the value returned by the API", - property: specSchemaDefinitionProperty{ - Name: "number_prop", - Type: typeFloat, - Required: true, - }, - inputPropertyValue: 99.99, - remoteValue: 99.99, - expectedSkip: true, - expectedError: nil, - }, - { - name: "required input bool matches the value returned by the API", - property: specSchemaDefinitionProperty{ - Name: "bool_prop", - Type: typeBool, - Required: true, - }, - inputPropertyValue: true, - remoteValue: true, - expectedSkip: true, - expectedError: nil, - }, { name: "required input list matches the value returned by the API where order of input values match", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + Required: true, }, inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, remoteValue: []string{"inputVal1", "inputVal2", "inputVal3"}, - expectedSkip: true, - expectedError: nil, + expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, }, { name: "required input list matches the value returned by the API where order of input values doesn't match", - property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - }, - inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, - remoteValue: []string{"inputVal2", "inputVal3", "inputVal1"}, - expectedSkip: true, - expectedError: nil, - }, - { - name: "required input list doesn't match the value returned by the API (same list length)", property: specSchemaDefinitionProperty{ Name: "list_prop", Type: typeList, - Required: true, ArrayItemsType: typeString, - }, - inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, - remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, - expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), - }, - { - name: "required input list doesn't match the value returned by the API (different list length)", - property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, Required: true, - ArrayItemsType: typeString, }, inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, - remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, - expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), + remoteValue: []string{"inputVal2", "inputVal3", "inputVal1"}, + expectedOutput: []interface{}{"inputVal3", "inputVal1", "inputVal2"}, }, + //{ + // name: "required input list doesn't match the value returned by the API (same list length)", + // property: specSchemaDefinitionProperty{ + // Name: "list_prop", + // Type: typeList, + // Required: true, + // ArrayItemsType: typeString, + // }, + // inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, + // remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, + // expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), + //}, + //{ + // name: "required input list doesn't match the value returned by the API (different list length)", + // property: specSchemaDefinitionProperty{ + // Name: "list_prop", + // Type: typeList, + // Required: true, + // ArrayItemsType: typeString, + // }, + // inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, + // remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, + // expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), + //}, } for _, tc := range testCases { - skip, err := compareInputPropertyValueWithPayloadPropertyValue(tc.property, tc.inputPropertyValue, tc.remoteValue) - if tc.expectedError == nil { - assert.Nil(t, err, tc.name) - assert.Equal(t, tc.expectedSkip, skip) - } else { - assert.Equal(t, tc.expectedError.Error(), err.Error(), tc.name) - } + output := compareInputPropertyValueWithPayloadPropertyValue(tc.property, tc.inputPropertyValue, tc.remoteValue) + assert.Equal(t, tc.expectedOutput, output) } } From a8150e90391d74e3e8a0c7d68690ac5ed3032f54 Mon Sep 17 00:00:00 2001 From: lchan Date: Thu, 7 May 2020 16:44:40 -0700 Subject: [PATCH 09/37] Add test cases to TestCompareInputPropertyValueWithPayloadPropertyValue - One case where the input item has an extra value and another case where the remote returns an extra item --- openapi/common_test.go | 48 +++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/openapi/common_test.go b/openapi/common_test.go index 35e0c0362..f32d78851 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -821,30 +821,30 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { remoteValue: []string{"inputVal2", "inputVal3", "inputVal1"}, expectedOutput: []interface{}{"inputVal3", "inputVal1", "inputVal2"}, }, - //{ - // name: "required input list doesn't match the value returned by the API (same list length)", - // property: specSchemaDefinitionProperty{ - // Name: "list_prop", - // Type: typeList, - // Required: true, - // ArrayItemsType: typeString, - // }, - // inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, - // remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, - // expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), - //}, - //{ - // name: "required input list doesn't match the value returned by the API (different list length)", - // property: specSchemaDefinitionProperty{ - // Name: "list_prop", - // Type: typeList, - // Required: true, - // ArrayItemsType: typeString, - // }, - // inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, - // remoteValue: []string{"inputVal2", "inputVal3", "inputVal4"}, - // expectedError: errors.New("API returned a payload containing an array property (list_prop) which does not contain the expected desired items specified by the user"), - //}, + { + name: "required input list has a value that isn't returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeString, + }, + inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, + remoteValue: []string{"inputVal2", "inputVal1"}, + expectedOutput: []interface{}{"inputVal1", "inputVal2"}, + }, + { + name: "required input list is missing a value returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeString, + }, + inputPropertyValue: []string{"inputVal1", "inputVal2"}, + remoteValue: []string{"inputVal3", "inputVal2", "inputVal1"}, + expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, + }, } for _, tc := range testCases { From 593c526d29c42ad46283c9cc711d464809a37ab2 Mon Sep 17 00:00:00 2001 From: dikhan Date: Mon, 11 May 2020 11:24:21 -0700 Subject: [PATCH 10/37] cast to array of interfaces to simplify processing of different elem types - delegate the responsibility of comparing item values to helper method compareListItems specSchemaDefinitionProperty - added --- openapi/common.go | 59 ++++++++++++------- ...pec_resource_schema_definition_property.go | 32 ++++++++++ 2 files changed, 70 insertions(+), 21 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index be6ae6e91..0a3d7497e 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -119,32 +119,31 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini if property.Required { switch property.Type { case typeList: - if property.ArrayItemsType == typeString { - for _, inputItemValue := range inputPropertyValue.([]string) { - for _, remoteItemValue := range remoteValue.([]string) { - if inputItemValue == remoteItemValue { - newPropertyValue = append(newPropertyValue, inputItemValue) - break - } + inputValueArray := castValueToArray(property, inputPropertyValue) + remoteValueArray := castValueToArray(property, remoteValue) + for _, inputItemValue := range inputValueArray { + for _, remoteItemValue := range remoteValueArray { + if property.compareListItems(inputItemValue, remoteItemValue) { + newPropertyValue = append(newPropertyValue, inputItemValue) + break } } - modifiedItems := []interface{}{} - for _, remoteItemValue := range remoteValue.([]string) { - match := false - for _, inputItemValue := range inputPropertyValue.([]string) { - if inputItemValue == remoteItemValue { - match = true - break - } - } - if !match { - modifiedItems = append(modifiedItems, remoteItemValue) + } + modifiedItems := []interface{}{} + for _, remoteItemValue := range remoteValueArray { + match := false + for _, inputItemValue := range inputValueArray { + if property.compareListItems(inputItemValue, remoteItemValue) { + match = true + break } } - for _, updatedItem := range modifiedItems { - newPropertyValue = append(newPropertyValue, updatedItem) + if !match { + modifiedItems = append(modifiedItems, remoteItemValue) } - + } + for _, updatedItem := range modifiedItems { + newPropertyValue = append(newPropertyValue, updatedItem) } return newPropertyValue } @@ -152,6 +151,24 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini return newPropertyValue } +func castValueToArray(property specSchemaDefinitionProperty, value interface{}) []interface{} { + interfaceArray := []interface{}{} + if property.Type == typeList { + switch property.ArrayItemsType { + case typeString: + for _, item := range value.([]string) { + interfaceArray = append(interfaceArray, item) + } + case typeInt: + for _, item := range value.([]int) { + interfaceArray = append(interfaceArray, item) + } + } + return interfaceArray + } + return nil +} + func convertPayloadToLocalStateDataValue(property *specSchemaDefinitionProperty, propertyValue interface{}, useString bool) (interface{}, error) { if propertyValue == nil { return nil, nil diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 02a4dd759..0a8ed193c 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -2,6 +2,7 @@ package openapi import ( "fmt" + "reflect" "github.com/dikhan/terraform-provider-openapi/openapi/terraformutils" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" @@ -300,3 +301,34 @@ func (s *specSchemaDefinitionProperty) validateFunc() schema.SchemaValidateFunc return } } + +func (s *specSchemaDefinitionProperty) compareListItems(item1, item2 interface{}) bool { + switch s.ArrayItemsType { + case typeString: + if !s.validateValueType(item1, reflect.String) && !s.validateValueType(item2, reflect.String) { + return false + } + case typeInt: + if !s.validateValueType(item1, reflect.Int) && !s.validateValueType(item2, reflect.Int) { + return false + } + case typeFloat: + if !s.validateValueType(item1, reflect.Float64) && !s.validateValueType(item2, reflect.Float64) { + return false + } + case typeBool: + if !s.validateValueType(item1, reflect.Bool) && !s.validateValueType(item2, reflect.Bool) { + return false + } + default: + return false + } + return item1 == item2 +} + +func (s *specSchemaDefinitionProperty) validateValueType(item interface{}, expectedKind reflect.Kind) bool { + if reflect.TypeOf(item).Kind() != expectedKind { + return false + } + return true +} From 29d508700a93281c9a37067699326f23fe8f6759 Mon Sep 17 00:00:00 2001 From: dikhan Date: Mon, 11 May 2020 11:24:42 -0700 Subject: [PATCH 11/37] added int use case test coverage --- openapi/common_test.go | 47 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/openapi/common_test.go b/openapi/common_test.go index f32d78851..d947263de 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -797,8 +797,9 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { remoteValue interface{} expectedOutput []interface{} }{ + // String use cases { - name: "required input list matches the value returned by the API where order of input values match", + name: "required input list (of strings) matches the value returned by the API where order of input values match", property: specSchemaDefinitionProperty{ Name: "list_prop", Type: typeList, @@ -810,7 +811,7 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, }, { - name: "required input list matches the value returned by the API where order of input values doesn't match", + name: "required input list (of strings) matches the value returned by the API where order of input values doesn't match", property: specSchemaDefinitionProperty{ Name: "list_prop", Type: typeList, @@ -822,7 +823,7 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { expectedOutput: []interface{}{"inputVal3", "inputVal1", "inputVal2"}, }, { - name: "required input list has a value that isn't returned by the API (input order maintained)", + name: "required input list (of strings) has a value that isn't returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ Name: "list_prop", Type: typeList, @@ -834,7 +835,7 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { expectedOutput: []interface{}{"inputVal1", "inputVal2"}, }, { - name: "required input list is missing a value returned by the API (input order maintained)", + name: "required input list (of strings) is missing a value returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ Name: "list_prop", Type: typeList, @@ -845,6 +846,44 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { remoteValue: []string{"inputVal3", "inputVal2", "inputVal1"}, expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, }, + + // Integer use cases + { + name: "required input list (of ints) matches the value returned by the API where order of input values doesn't match", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeInt, + Required: true, + }, + inputPropertyValue: []int{3, 1, 2}, + remoteValue: []int{2, 3, 1}, + expectedOutput: []interface{}{3, 1, 2}, + }, + { + name: "required input list (of ints) has a value that isn't returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeInt, + }, + inputPropertyValue: []int{1, 2, 3}, + remoteValue: []int{2, 1}, + expectedOutput: []interface{}{1, 2}, + }, + { + name: "required input list (of ints) is missing a value returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeInt, + }, + inputPropertyValue: []int{1, 2}, + remoteValue: []int{3, 2, 1}, + expectedOutput: []interface{}{1, 2, 3}, + }, } for _, tc := range testCases { From 381cf8fb23d30a2a9e785f803b62aea77fbf2e06 Mon Sep 17 00:00:00 2001 From: lchan Date: Mon, 11 May 2020 11:43:02 -0700 Subject: [PATCH 12/37] Add float case to castValueToArray - Update TestCompareInputPropertyValueWithPayloadPropertyValue with float cases --- openapi/common.go | 4 ++++ openapi/common_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/openapi/common.go b/openapi/common.go index 0a3d7497e..864124a76 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -163,6 +163,10 @@ func castValueToArray(property specSchemaDefinitionProperty, value interface{}) for _, item := range value.([]int) { interfaceArray = append(interfaceArray, item) } + case typeFloat: + for _, item := range value.([]float64) { + interfaceArray = append(interfaceArray, item) + } } return interfaceArray } diff --git a/openapi/common_test.go b/openapi/common_test.go index d947263de..8c21bfb1f 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -884,6 +884,44 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { remoteValue: []int{3, 2, 1}, expectedOutput: []interface{}{1, 2, 3}, }, + + // Float use cases + { + name: "required input list (of floats) matches the value returned by the API where order of input values doesn't match", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeFloat, + Required: true, + }, + inputPropertyValue: []float64{3.0, 1.0, 2.0}, + remoteValue: []float64{2.0, 3.0, 1.0}, + expectedOutput: []interface{}{3.0, 1.0, 2.0}, + }, + { + name: "required input list (of floats) has a value that isn't returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeFloat, + }, + inputPropertyValue: []float64{1.0, 2.0, 3.0}, + remoteValue: []float64{2.0, 1.0}, + expectedOutput: []interface{}{1.0, 2.0}, + }, + { + name: "required input list (of floats) is missing a value returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + Required: true, + ArrayItemsType: typeFloat, + }, + inputPropertyValue: []float64{1.0, 2.0}, + remoteValue: []float64{3.0, 2.0, 1.0}, + expectedOutput: []interface{}{1.0, 2.0, 3.0}, + }, } for _, tc := range testCases { From de6be5fce345b186327357140597fb924f722886 Mon Sep 17 00:00:00 2001 From: dikhan Date: Mon, 11 May 2020 12:57:15 -0700 Subject: [PATCH 13/37] add support for compare and compareItems in the specSchemaDefinitionProperty - compare goes ahead and given the array item types and two values checks if they are equal --- openapi/common.go | 9 +++- openapi/common_test.go | 54 +++++++++++++++++++ ...pec_resource_schema_definition_property.go | 53 ++++++++++++++++-- 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index 864124a76..703335843 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -123,7 +123,7 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini remoteValueArray := castValueToArray(property, remoteValue) for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { - if property.compareListItems(inputItemValue, remoteItemValue) { + if property.compareListItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { newPropertyValue = append(newPropertyValue, inputItemValue) break } @@ -133,7 +133,7 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini for _, remoteItemValue := range remoteValueArray { match := false for _, inputItemValue := range inputValueArray { - if property.compareListItems(inputItemValue, remoteItemValue) { + if property.compareListItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { match = true break } @@ -167,7 +167,12 @@ func castValueToArray(property specSchemaDefinitionProperty, value interface{}) for _, item := range value.([]float64) { interfaceArray = append(interfaceArray, item) } + case typeObject: + for _, item := range value.([]interface{}) { + interfaceArray = append(interfaceArray, item) + } } + return interfaceArray } return nil diff --git a/openapi/common_test.go b/openapi/common_test.go index 8c21bfb1f..5cc1c9247 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -922,6 +922,60 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { remoteValue: []float64{3.0, 2.0, 1.0}, expectedOutput: []interface{}{1.0, 2.0, 3.0}, }, + + // List of objects use cases + { + name: "required input list (of floats) matches the value returned by the API where order of input values doesn't match", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeObject, + SpecSchemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + &specSchemaDefinitionProperty{ + Name: "group", + Type: typeString, + }, + &specSchemaDefinitionProperty{ + Name: "roles", + Type: typeList, + ArrayItemsType: typeString, + }, + }, + }, + Required: true, + }, + inputPropertyValue: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + map[string]interface{}{ + "group": "someOtherGroup", + "roles": []interface{}{"role3", "role4"}, + }, + }, + remoteValue: []interface{}{ + map[string]interface{}{ + "group": "someOtherGroup", + "roles": []interface{}{"role3", "role4"}, + }, + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + }, + expectedOutput: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + map[string]interface{}{ + "group": "someOtherGroup", + "roles": []interface{}{"role3", "role4"}, + }, + }, + }, } for _, tc := range testCases { diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 0a8ed193c..28e47cb4e 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -302,8 +302,8 @@ func (s *specSchemaDefinitionProperty) validateFunc() schema.SchemaValidateFunc } } -func (s *specSchemaDefinitionProperty) compareListItems(item1, item2 interface{}) bool { - switch s.ArrayItemsType { +func (s *specSchemaDefinitionProperty) compare(arrayItemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { + switch arrayItemsType { case typeString: if !s.validateValueType(item1, reflect.String) && !s.validateValueType(item2, reflect.String) { return false @@ -316,10 +316,55 @@ func (s *specSchemaDefinitionProperty) compareListItems(item1, item2 interface{} if !s.validateValueType(item1, reflect.Float64) && !s.validateValueType(item2, reflect.Float64) { return false } - case typeBool: - if !s.validateValueType(item1, reflect.Bool) && !s.validateValueType(item2, reflect.Bool) { + case typeList: + if !s.validateValueType(item1, reflect.Slice) && !s.validateValueType(item2, reflect.Slice) { + return false + } + list1 := item1.([]interface{}) + list2 := item2.([]interface{}) + if len(list1) != len(list2) { + return false + } + for idx, _ := range list1 { + return s.compareListItems(s.ArrayItemsType, list1[idx], list2[idx]) + } + case typeObject: + if !s.validateValueType(item1, reflect.Map) && !s.validateValueType(item2, reflect.Map) { return false } + object1 := item1.(map[string]interface{}) + object2 := item1.(map[string]interface{}) + for _, objectProperty := range s.SpecSchemaDefinition.Properties { + objectPropertyValue1 := object1[objectProperty.Name] + objectPropertyValue2 := object2[objectProperty.Name] + if objectProperty.compare(objectProperty.Type, objectPropertyValue1, objectPropertyValue2) { + return true + } + } + default: + return false + } + return item1 == item2 +} + +func (s *specSchemaDefinitionProperty) compareListItems(arrayItemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { + switch arrayItemsType { + case typeString, typeInt, typeFloat: + return s.compare(arrayItemsType, item1, item2) + case typeObject: + if !s.validateValueType(item1, reflect.Map) && !s.validateValueType(item2, reflect.Map) { + return false + } + object1 := item1.(map[string]interface{}) + object2 := item2.(map[string]interface{}) + for _, objectProperty := range s.SpecSchemaDefinition.Properties { + objectPropertyValue1 := object1[objectProperty.Name] + objectPropertyValue2 := object2[objectProperty.Name] + if !objectProperty.compare(objectProperty.Type, objectPropertyValue1, objectPropertyValue2) { + return false + } + } + return true default: return false } From 81479d7f8a0029cc67dbc242a31bc0d57cbb83a7 Mon Sep 17 00:00:00 2001 From: dikhan Date: Mon, 11 May 2020 13:12:45 -0700 Subject: [PATCH 14/37] remove unnecessary duplicated code - leverage equal method for list objects too - cleaned up code --- openapi/common.go | 4 +-- ...pec_resource_schema_definition_property.go | 36 +++++-------------- 2 files changed, 10 insertions(+), 30 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index 703335843..13ec7c300 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -123,7 +123,7 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini remoteValueArray := castValueToArray(property, remoteValue) for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { - if property.compareListItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { + if property.compare(property.ArrayItemsType, inputItemValue, remoteItemValue) { newPropertyValue = append(newPropertyValue, inputItemValue) break } @@ -133,7 +133,7 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini for _, remoteItemValue := range remoteValueArray { match := false for _, inputItemValue := range inputValueArray { - if property.compareListItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { + if property.compare(property.ArrayItemsType, inputItemValue, remoteItemValue) { match = true break } diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 28e47cb4e..4efc2e654 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -302,8 +302,12 @@ func (s *specSchemaDefinitionProperty) validateFunc() schema.SchemaValidateFunc } } -func (s *specSchemaDefinitionProperty) compare(arrayItemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { - switch arrayItemsType { +func (s *specSchemaDefinitionProperty) equal(item1, item2 interface{}) bool { + return s.compare(s.Type, item1, item2) +} + +func (s *specSchemaDefinitionProperty) compare(itemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { + switch itemsType { case typeString: if !s.validateValueType(item1, reflect.String) && !s.validateValueType(item2, reflect.String) { return false @@ -326,7 +330,7 @@ func (s *specSchemaDefinitionProperty) compare(arrayItemsType schemaDefinitionPr return false } for idx, _ := range list1 { - return s.compareListItems(s.ArrayItemsType, list1[idx], list2[idx]) + return s.compare(s.ArrayItemsType, list1[idx], list2[idx]) } case typeObject: if !s.validateValueType(item1, reflect.Map) && !s.validateValueType(item2, reflect.Map) { @@ -337,7 +341,7 @@ func (s *specSchemaDefinitionProperty) compare(arrayItemsType schemaDefinitionPr for _, objectProperty := range s.SpecSchemaDefinition.Properties { objectPropertyValue1 := object1[objectProperty.Name] objectPropertyValue2 := object2[objectProperty.Name] - if objectProperty.compare(objectProperty.Type, objectPropertyValue1, objectPropertyValue2) { + if objectProperty.equal(objectPropertyValue1, objectPropertyValue2) { return true } } @@ -347,30 +351,6 @@ func (s *specSchemaDefinitionProperty) compare(arrayItemsType schemaDefinitionPr return item1 == item2 } -func (s *specSchemaDefinitionProperty) compareListItems(arrayItemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { - switch arrayItemsType { - case typeString, typeInt, typeFloat: - return s.compare(arrayItemsType, item1, item2) - case typeObject: - if !s.validateValueType(item1, reflect.Map) && !s.validateValueType(item2, reflect.Map) { - return false - } - object1 := item1.(map[string]interface{}) - object2 := item2.(map[string]interface{}) - for _, objectProperty := range s.SpecSchemaDefinition.Properties { - objectPropertyValue1 := object1[objectProperty.Name] - objectPropertyValue2 := object2[objectProperty.Name] - if !objectProperty.compare(objectProperty.Type, objectPropertyValue1, objectPropertyValue2) { - return false - } - } - return true - default: - return false - } - return item1 == item2 -} - func (s *specSchemaDefinitionProperty) validateValueType(item interface{}, expectedKind reflect.Kind) bool { if reflect.TypeOf(item).Kind() != expectedKind { return false From dd575caa771b4128e75432a2f660a5dca2a81836 Mon Sep 17 00:00:00 2001 From: dikhan Date: Mon, 11 May 2020 14:40:52 -0700 Subject: [PATCH 15/37] fix typo --- openapi/openapi_spec_resource_schema_definition_property.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 4efc2e654..3222eea68 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -337,7 +337,7 @@ func (s *specSchemaDefinitionProperty) compare(itemsType schemaDefinitionPropert return false } object1 := item1.(map[string]interface{}) - object2 := item1.(map[string]interface{}) + object2 := item2.(map[string]interface{}) for _, objectProperty := range s.SpecSchemaDefinition.Properties { objectPropertyValue1 := object1[objectProperty.Name] objectPropertyValue2 := object2[objectProperty.Name] From afc971f9f610eaac418b842ad3d421cb10fb2924 Mon Sep 17 00:00:00 2001 From: dikhan Date: Mon, 11 May 2020 14:47:18 -0700 Subject: [PATCH 16/37] fixed bug where once all the object prop values had been checked it was falling back to the regulat item check (instead of returning true) resulting into casting errors - added another test scenario where the input contains more items than remote --- openapi/common_test.go | 46 ++++++++++++++++++- ...pec_resource_schema_definition_property.go | 5 +- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/openapi/common_test.go b/openapi/common_test.go index 5cc1c9247..d4995b2ec 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -925,7 +925,7 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { // List of objects use cases { - name: "required input list (of floats) matches the value returned by the API where order of input values doesn't match", + name: "required input list (objects) matches the value returned by the API where order of input values doesn't match", property: specSchemaDefinitionProperty{ Name: "list_prop", Type: typeList, @@ -976,6 +976,50 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { }, }, }, + { + name: "required input list (objects) has a value that isn't returned by the API (input order maintained)", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeObject, + SpecSchemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + &specSchemaDefinitionProperty{ + Name: "group", + Type: typeString, + }, + &specSchemaDefinitionProperty{ + Name: "roles", + Type: typeList, + ArrayItemsType: typeString, + }, + }, + }, + Required: true, + }, + inputPropertyValue: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + map[string]interface{}{ + "group": "someOtherGroup", + "roles": []interface{}{"role3", "role4"}, + }, + }, + remoteValue: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + }, + expectedOutput: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + }, + }, } for _, tc := range testCases { diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 3222eea68..378c7ef7f 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -341,10 +341,11 @@ func (s *specSchemaDefinitionProperty) compare(itemsType schemaDefinitionPropert for _, objectProperty := range s.SpecSchemaDefinition.Properties { objectPropertyValue1 := object1[objectProperty.Name] objectPropertyValue2 := object2[objectProperty.Name] - if objectProperty.equal(objectPropertyValue1, objectPropertyValue2) { - return true + if !objectProperty.equal(objectPropertyValue1, objectPropertyValue2) { + return false } } + return true default: return false } From 66240a873889dec47a303117336f31c331f01fea Mon Sep 17 00:00:00 2001 From: lchan Date: Mon, 11 May 2020 14:57:55 -0700 Subject: [PATCH 17/37] Add test case to TestCompareInputPropertyValue - API returns additional value that isn't in the input --- openapi/common_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/openapi/common_test.go b/openapi/common_test.go index d4995b2ec..557860fb7 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -1020,6 +1020,54 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { }, }, }, + { + name: "required input list (objects) doesn't have a value returned by the API", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeObject, + SpecSchemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + &specSchemaDefinitionProperty{ + Name: "group", + Type: typeString, + }, + &specSchemaDefinitionProperty{ + Name: "roles", + Type: typeList, + ArrayItemsType: typeString, + }, + }, + }, + Required: true, + }, + inputPropertyValue: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + }, + remoteValue: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + map[string]interface{}{ + "group": "unexpectedGroup", + "roles": []interface{}{"role3", "role4"}, + }, + }, + expectedOutput: []interface{}{ + map[string]interface{}{ + "group": "someGroup", + "roles": []interface{}{"role1", "role2"}, + }, + map[string]interface{}{ + "group": "unexpectedGroup", + "roles": []interface{}{"role3", "role4"}, + }, + }, + }, } for _, tc := range testCases { From b6c430788048d0879abbd848889510e7e2f39a80 Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 11:46:02 -0700 Subject: [PATCH 18/37] Add TestCompare for specSchemaDefinitionProperty - Includes test cases for strings, ints, floats, lists, and objects where the items match and don't match --- ...esource_schema_definition_property_test.go | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/openapi/openapi_spec_resource_schema_definition_property_test.go b/openapi/openapi_spec_resource_schema_definition_property_test.go index 0eee9442d..3198af7b3 100644 --- a/openapi/openapi_spec_resource_schema_definition_property_test.go +++ b/openapi/openapi_spec_resource_schema_definition_property_test.go @@ -1654,6 +1654,135 @@ func TestValidateFunc(t *testing.T) { }) } +func TestCompare(t *testing.T) { + testCases := []struct { + name string + schemaDefProp specSchemaDefinitionProperty + propertyType schemaDefinitionPropertyType + arrayItemsPropType schemaDefinitionPropertyType + inputItem interface{} + remoteItem interface{} + expectedOutput bool + }{ + // String use cases + { + name: "string input value matches string remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeString}, + inputItem: "inputVal1", + remoteItem: "inputVal1", + expectedOutput: true, + }, + { + name: "string input value doesn't match string remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeString}, + inputItem: "inputVal1", + remoteItem: "inputVal2", + expectedOutput: false, + }, + // Integer use cases + { + name: "int input value matches int remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeInt}, + inputItem: 1, + remoteItem: 1, + expectedOutput: true, + }, + { + name: "int input value doesn't match int remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeInt}, + inputItem: 1, + remoteItem: 2, + expectedOutput: false, + }, + // Float use cases + { + name: "float input value matches float remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeFloat}, + inputItem: 1.0, + remoteItem: 1.0, + expectedOutput: true, + }, + { + name: "float input value doesn't match float remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeFloat}, + inputItem: 1.0, + remoteItem: 2.0, + expectedOutput: false, + }, + // List use cases + { + name: "list input value matches list remote value", + schemaDefProp: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeString, + }, + inputItem: []interface{}{"role1", "role2"}, + remoteItem: []interface{}{"role1", "role2"}, + expectedOutput: true, + }, + { + name: "list input value doesn't match list remote value (same list length)", + schemaDefProp: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeString, + }, + inputItem: []interface{}{"role1", "role2"}, + remoteItem: []interface{}{"role3", "role4"}, + expectedOutput: false, + }, + { + name: "list input value doesn't match list remote value (different list length)", + schemaDefProp: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeString, + }, + inputItem: []interface{}{"role1", "role2"}, + remoteItem: []interface{}{"role1"}, + expectedOutput: false, + }, + // Object use cases + { + name: "object input value matches object remote value", + schemaDefProp: specSchemaDefinitionProperty{ + Type: typeObject, + SpecSchemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + &specSchemaDefinitionProperty{ + Name: "group", + Type: typeString, + }, + }, + }, + }, + inputItem: map[string]interface{}{"group": "someGroup"}, + remoteItem: map[string]interface{}{"group": "someGroup"}, + expectedOutput: true, + }, + { + name: "object input value doesn't match object remote value", + schemaDefProp: specSchemaDefinitionProperty{ + Type: typeObject, + SpecSchemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + &specSchemaDefinitionProperty{ + Name: "group", + Type: typeString, + }, + }, + }, + }, + inputItem: map[string]interface{}{"group": "someGroup"}, + remoteItem: map[string]interface{}{"group": "someOtherGroup"}, + expectedOutput: false, + }, + } + + for _, tc := range testCases { + output := tc.schemaDefProp.compare(tc.schemaDefProp.Type, tc.inputItem, tc.remoteItem) + assert.Equal(t, tc.expectedOutput, output, tc.name) + } +} + func Test_shouldUseLegacyTerraformSDKBlockApproachForComplexObjects(t *testing.T) { Convey("shouldUseLegacyTerraformSDKBlockApproachForComplexObject", t, func() { From 28ad45dbeb4c4bbe16eec7adf765d935da122552 Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 12:14:08 -0700 Subject: [PATCH 19/37] Add bool switch case in compare method - Change TestCompare to TestEqual, which just returns the results of compare, but we don't need to pass in the schemaDefProp type when calling equal --- ...pec_resource_schema_definition_property.go | 4 ++++ ...esource_schema_definition_property_test.go | 19 +++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 378c7ef7f..a11db5325 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -320,6 +320,10 @@ func (s *specSchemaDefinitionProperty) compare(itemsType schemaDefinitionPropert if !s.validateValueType(item1, reflect.Float64) && !s.validateValueType(item2, reflect.Float64) { return false } + case typeBool: + if !s.validateValueType(item1, reflect.Bool) && !s.validateValueType(item2, reflect.Bool) { + return false + } case typeList: if !s.validateValueType(item1, reflect.Slice) && !s.validateValueType(item2, reflect.Slice) { return false diff --git a/openapi/openapi_spec_resource_schema_definition_property_test.go b/openapi/openapi_spec_resource_schema_definition_property_test.go index 3198af7b3..d59f4952f 100644 --- a/openapi/openapi_spec_resource_schema_definition_property_test.go +++ b/openapi/openapi_spec_resource_schema_definition_property_test.go @@ -1654,7 +1654,7 @@ func TestValidateFunc(t *testing.T) { }) } -func TestCompare(t *testing.T) { +func TestEqual(t *testing.T) { testCases := []struct { name string schemaDefProp specSchemaDefinitionProperty @@ -1709,6 +1709,21 @@ func TestCompare(t *testing.T) { remoteItem: 2.0, expectedOutput: false, }, + // Bool use cases + { + name: "bool input value matches bool remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeBool}, + inputItem: true, + remoteItem: true, + expectedOutput: true, + }, + { + name: "bool input value doesn't match bool remote value", + schemaDefProp: specSchemaDefinitionProperty{Type: typeBool}, + inputItem: true, + remoteItem: false, + expectedOutput: false, + }, // List use cases { name: "list input value matches list remote value", @@ -1778,7 +1793,7 @@ func TestCompare(t *testing.T) { } for _, tc := range testCases { - output := tc.schemaDefProp.compare(tc.schemaDefProp.Type, tc.inputItem, tc.remoteItem) + output := tc.schemaDefProp.equal(tc.inputItem, tc.remoteItem) assert.Equal(t, tc.expectedOutput, output, tc.name) } } From ef12e1b59824c8f1a7fa01080fded8bdd74b648a Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 12:15:18 -0700 Subject: [PATCH 20/37] Add TestValidateValueType - Include cases for strings, ints, floats, bools, list, and objects where item matches/doesn't match the expected kind --- ...esource_schema_definition_property_test.go | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/openapi/openapi_spec_resource_schema_definition_property_test.go b/openapi/openapi_spec_resource_schema_definition_property_test.go index d59f4952f..3d55b39de 100644 --- a/openapi/openapi_spec_resource_schema_definition_property_test.go +++ b/openapi/openapi_spec_resource_schema_definition_property_test.go @@ -1798,6 +1798,99 @@ func TestEqual(t *testing.T) { } } +func TestValidateValueType(t *testing.T) { + testCases := []struct { + name string + item interface{} + itemKind reflect.Kind + expectedOutput bool + }{ + // String use cases + { + name: "expect string kind and item is a string", + item: "inputVal1", + itemKind: reflect.String, + expectedOutput: true, + }, + { + name: "expect string kind and item is NOT a string", + item: 1, + itemKind: reflect.String, + expectedOutput: false, + }, + // Int use cases + { + name: "expect int kind and item is a int", + item: 1, + itemKind: reflect.Int, + expectedOutput: true, + }, + { + name: "expect int kind and item is NOT a int", + item: "not an int", + itemKind: reflect.Int, + expectedOutput: false, + }, + // Float use cases + { + name: "expect float kind and item is a float", + item: 1.0, + itemKind: reflect.Float64, + expectedOutput: true, + }, + { + name: "expect float kind and item is NOT a float", + item: "not a float", + itemKind: reflect.Float64, + expectedOutput: false, + }, + // Bool use cases + { + name: "expect bool kind and item is a bool", + item: true, + itemKind: reflect.Bool, + expectedOutput: true, + }, + { + name: "expect bool kind and item is NOT a bool", + item: "not a bool", + itemKind: reflect.Bool, + expectedOutput: false, + }, + // List use cases + { + name: "expect slice kind and item is a slice", + item: []interface{}{"item1", "item2"}, + itemKind: reflect.Slice, + expectedOutput: true, + }, + { + name: "expect slice kind and item is NOT a slice", + item: "not a slice", + itemKind: reflect.Slice, + expectedOutput: false, + }, + // Object use cases + { + name: "expect map kind and item is a map", + item: map[string]interface{}{"group": "someGroup"}, + itemKind: reflect.Map, + expectedOutput: true, + }, + { + name: "expect map kind and item is NOT a map", + item: "not a map", + itemKind: reflect.Map, + expectedOutput: false, + }, + } + for _, tc := range testCases { + s := specSchemaDefinitionProperty{} + output := s.validateValueType(tc.item, tc.itemKind) + assert.Equal(t, tc.expectedOutput, output, tc.name) + } +} + func Test_shouldUseLegacyTerraformSDKBlockApproachForComplexObjects(t *testing.T) { Convey("shouldUseLegacyTerraformSDKBlockApproachForComplexObject", t, func() { From c5cc347914be17be2231dc7b61bb749f7b0014ea Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 12 May 2020 12:54:27 -0700 Subject: [PATCH 21/37] - rename the compareInputPropertyValueWithPayloadPropertyValue to a more clear name processIgnoreOrderIfEnabled - processIgnoreOrderIfEnabled now checks whether the property has the flag IgnoreItemsOrder enabled, otherwise returns the remote list (keeping the current behaviour) - if the casting returns nil we return the remotelist too (eg< use case for list of bools with the IgnoreItemsOrder set to true, this is not supported at the moment) --- openapi/common.go | 30 +++++--- openapi/common_test.go | 153 +++++++++++++++++++++++++++-------------- 2 files changed, 123 insertions(+), 60 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index 13ec7c300..ffb37185c 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -109,18 +109,26 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str return nil } -// Use case 0: The desired state for an array property (input from user) contains items in certain order AND the remote state comes back with the same items in the same order. -// Use case 1: The desired state for an array property (input from user) contains items in certain order BUT the remote state comes back with the same items in different order. -// Use case 2: The desired state for an array property (input from user) contains items in certain order BUT the remote state comes back with the same items in different order PLUS new ones. -// Use case 3: The desired state for an array property (input from user) contains items in certain order BUT the remote state comes back with a shorter list where the remaining elems match the inputs. -// Use case 4: The desired state for an array property (input from user) contains items in certain order BUT the remote state some back with the list with the same size but some elems were updated -func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) interface{} { - newPropertyValue := []interface{}{} - if property.Required { +// processIgnoreOrderIfEnabled checks whether the property has enabled the `IgnoreItemsOrder` field and if so, goes ahead +// and returns a new list trying to match as much as possible the input order from the user (not remotes). The following use +// cases are supported: +// Use case 0: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order AND the remote state (remoteValue) comes back with the same items in the same order. +// Use case 1: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) comes back with the same items in different order. +// Use case 2: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) comes back with the same items in different order PLUS new ones. +// Use case 3: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) comes back with a shorter list where the remaining elems match the inputs. +// Use case 4: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) some back with the list with the same size but some elems were updated +func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) interface{} { + if property.IgnoreItemsOrder && property.Required { + newPropertyValue := []interface{}{} switch property.Type { case typeList: inputValueArray := castValueToArray(property, inputPropertyValue) remoteValueArray := castValueToArray(property, remoteValue) + + if inputValueArray == nil || remoteValueArray == nil { + return remoteValue + } + for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { if property.compare(property.ArrayItemsType, inputItemValue, remoteItemValue) { @@ -145,10 +153,11 @@ func compareInputPropertyValueWithPayloadPropertyValue(property specSchemaDefini for _, updatedItem := range modifiedItems { newPropertyValue = append(newPropertyValue, updatedItem) } + return newPropertyValue } } - return newPropertyValue + return remoteValue } func castValueToArray(property specSchemaDefinitionProperty, value interface{}) []interface{} { @@ -171,8 +180,9 @@ func castValueToArray(property specSchemaDefinitionProperty, value interface{}) for _, item := range value.([]interface{}) { interfaceArray = append(interfaceArray, item) } + default: + return nil } - return interfaceArray } return nil diff --git a/openapi/common_test.go b/openapi/common_test.go index 557860fb7..6fcaa1ef2 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -801,10 +801,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of strings) matches the value returned by the API where order of input values match", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeString, - Required: true, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, remoteValue: []string{"inputVal1", "inputVal2", "inputVal3"}, @@ -813,10 +814,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of strings) matches the value returned by the API where order of input values doesn't match", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeString, - Required: true, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, remoteValue: []string{"inputVal2", "inputVal3", "inputVal1"}, @@ -825,10 +827,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of strings) has a value that isn't returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - ArrayItemsType: typeString, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, remoteValue: []string{"inputVal2", "inputVal1"}, @@ -837,10 +840,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of strings) is missing a value returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - ArrayItemsType: typeString, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []string{"inputVal1", "inputVal2"}, remoteValue: []string{"inputVal3", "inputVal2", "inputVal1"}, @@ -851,10 +855,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of ints) matches the value returned by the API where order of input values doesn't match", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeInt, - Required: true, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeInt, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []int{3, 1, 2}, remoteValue: []int{2, 3, 1}, @@ -863,10 +868,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of ints) has a value that isn't returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - ArrayItemsType: typeInt, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeInt, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []int{1, 2, 3}, remoteValue: []int{2, 1}, @@ -875,10 +881,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of ints) is missing a value returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - ArrayItemsType: typeInt, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeInt, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []int{1, 2}, remoteValue: []int{3, 2, 1}, @@ -889,10 +896,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of floats) matches the value returned by the API where order of input values doesn't match", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeFloat, - Required: true, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeFloat, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []float64{3.0, 1.0, 2.0}, remoteValue: []float64{2.0, 3.0, 1.0}, @@ -901,10 +909,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of floats) has a value that isn't returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - ArrayItemsType: typeFloat, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeFloat, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []float64{1.0, 2.0, 3.0}, remoteValue: []float64{2.0, 1.0}, @@ -913,10 +922,11 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (of floats) is missing a value returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - Required: true, - ArrayItemsType: typeFloat, + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeFloat, + IgnoreItemsOrder: true, + Required: true, }, inputPropertyValue: []float64{1.0, 2.0}, remoteValue: []float64{3.0, 2.0, 1.0}, @@ -927,9 +937,10 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (objects) matches the value returned by the API where order of input values doesn't match", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeObject, + Name: "list_prop", + IgnoreItemsOrder: true, + Type: typeList, + ArrayItemsType: typeObject, SpecSchemaDefinition: &specSchemaDefinition{ Properties: specSchemaDefinitionProperties{ &specSchemaDefinitionProperty{ @@ -979,9 +990,10 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (objects) has a value that isn't returned by the API (input order maintained)", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeObject, + Name: "list_prop", + IgnoreItemsOrder: true, + Type: typeList, + ArrayItemsType: typeObject, SpecSchemaDefinition: &specSchemaDefinition{ Properties: specSchemaDefinitionProperties{ &specSchemaDefinitionProperty{ @@ -1023,9 +1035,10 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { { name: "required input list (objects) doesn't have a value returned by the API", property: specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeObject, + Name: "list_prop", + IgnoreItemsOrder: true, + Type: typeList, + ArrayItemsType: typeObject, SpecSchemaDefinition: &specSchemaDefinition{ Properties: specSchemaDefinitionProperties{ &specSchemaDefinitionProperty{ @@ -1071,7 +1084,47 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { } for _, tc := range testCases { - output := compareInputPropertyValueWithPayloadPropertyValue(tc.property, tc.inputPropertyValue, tc.remoteValue) + output := processIgnoreOrderIfEnabled(tc.property, tc.inputPropertyValue, tc.remoteValue) assert.Equal(t, tc.expectedOutput, output) } } + +func TestCompareInputPropertyValueWithPayloadPropertyValueIgnoreOrderDisabled(t *testing.T) { + Convey("Given a a list of strings property definition (with ignore order set to false) and the corresponding input/remote lists", t, func() { + property := specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: false, + Required: true, + } + inputPropertyValue := []string{"inputVal1", "inputVal2", "inputVal3"} + remoteValue := []string{"inputVal1", "inputVal2", "inputVal3"} + Convey("When processIgnoreOrderIfEnabled", func() { + output := processIgnoreOrderIfEnabled(property, inputPropertyValue, remoteValue) + Convey("Then the output should match the remote list", func() { + So(output, ShouldResemble, []string{"inputVal1", "inputVal2", "inputVal3"}) + }) + }) + }) +} + +func TestCompareInputPropertyValueWithPayloadPropertyValueBools(t *testing.T) { + Convey("Given a a list of bools property definition and the corresponding input/remote lists", t, func() { + property := specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeBool, + IgnoreItemsOrder: true, + Required: true, + } + inputPropertyValue := []bool{true} + remoteValue := []bool{false} + Convey("When processIgnoreOrderIfEnabled", func() { + output := processIgnoreOrderIfEnabled(property, inputPropertyValue, remoteValue) + Convey("Then the output should match the remote list", func() { + So(output, ShouldResemble, []bool{false}) + }) + }) + }) +} From 4dbf26695eea8ac6b3fdbe6aabd49bef9c603d29 Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 13:11:43 -0700 Subject: [PATCH 22/37] Rename compare to equalItems (more descriptive) --- openapi/common.go | 4 ++-- openapi/openapi_spec_resource_schema_definition_property.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index ffb37185c..391e719b3 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -131,7 +131,7 @@ func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPro for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { - if property.compare(property.ArrayItemsType, inputItemValue, remoteItemValue) { + if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { newPropertyValue = append(newPropertyValue, inputItemValue) break } @@ -141,7 +141,7 @@ func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPro for _, remoteItemValue := range remoteValueArray { match := false for _, inputItemValue := range inputValueArray { - if property.compare(property.ArrayItemsType, inputItemValue, remoteItemValue) { + if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { match = true break } diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index a11db5325..057084811 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -303,10 +303,10 @@ func (s *specSchemaDefinitionProperty) validateFunc() schema.SchemaValidateFunc } func (s *specSchemaDefinitionProperty) equal(item1, item2 interface{}) bool { - return s.compare(s.Type, item1, item2) + return s.equalItems(s.Type, item1, item2) } -func (s *specSchemaDefinitionProperty) compare(itemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { +func (s *specSchemaDefinitionProperty) equalItems(itemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { switch itemsType { case typeString: if !s.validateValueType(item1, reflect.String) && !s.validateValueType(item2, reflect.String) { @@ -334,7 +334,7 @@ func (s *specSchemaDefinitionProperty) compare(itemsType schemaDefinitionPropert return false } for idx, _ := range list1 { - return s.compare(s.ArrayItemsType, list1[idx], list2[idx]) + return s.equalItems(s.ArrayItemsType, list1[idx], list2[idx]) } case typeObject: if !s.validateValueType(item1, reflect.Map) && !s.validateValueType(item2, reflect.Map) { From 04c14a40f6650f3fcb3a288823574d84e0e9203d Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 13:20:39 -0700 Subject: [PATCH 23/37] Add TestCastValueToArray --- openapi/common_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/openapi/common_test.go b/openapi/common_test.go index 6fcaa1ef2..2ae8939fd 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -1128,3 +1128,29 @@ func TestCompareInputPropertyValueWithPayloadPropertyValueBools(t *testing.T) { }) }) } + +func TestCastValueToArray(t *testing.T) { + testCases := []struct { + name string + property specSchemaDefinitionProperty + inputVal interface{} + expectedOutput []interface{} + }{ + // String use cases + { + name: "array of strings", + property: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeString, + }, + inputVal: []string{"inputVal1", "inputVal2", "inputVal3"}, + expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, + }, + } + // TODO: Add more use cases + + for _, tc := range testCases { + output := castValueToArray(tc.property, tc.inputVal) + assert.Equal(t, tc.expectedOutput, output) + } +} From 17452feeec5f00266c832f90f85125be3874a2e4 Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 16:05:19 -0700 Subject: [PATCH 24/37] Add more test cases to TestCastValueToArray --- openapi/common_test.go | 50 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/openapi/common_test.go b/openapi/common_test.go index 2ae8939fd..743175422 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -1136,7 +1136,6 @@ func TestCastValueToArray(t *testing.T) { inputVal interface{} expectedOutput []interface{} }{ - // String use cases { name: "array of strings", property: specSchemaDefinitionProperty{ @@ -1146,8 +1145,55 @@ func TestCastValueToArray(t *testing.T) { inputVal: []string{"inputVal1", "inputVal2", "inputVal3"}, expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, }, + { + name: "array of ints", + property: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeInt, + }, + inputVal: []int{1, 2, 3}, + expectedOutput: []interface{}{1, 2, 3}, + }, + { + name: "array of floats", + property: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeFloat, + }, + inputVal: []float64{1.0, 2.0, 3.0}, + expectedOutput: []interface{}{1.0, 2.0, 3.0}, + }, + { + name: "array of objects", + property: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeObject, + }, + inputVal: []interface{}{ + map[string]interface{}{"group": "someGroup"}, + }, + expectedOutput: []interface{}{ + map[string]interface{}{"group": "someGroup"}, + }, + }, + { + name: "array of bools (unsupported type)", + property: specSchemaDefinitionProperty{ + Type: typeList, + ArrayItemsType: typeBool, + }, + inputVal: []bool{true}, + expectedOutput: nil, + }, + { + name: "value is not an array", + property: specSchemaDefinitionProperty{ + Type: typeString, + }, + inputVal: "someValue", + expectedOutput: nil, + }, } - // TODO: Add more use cases for _, tc := range testCases { output := castValueToArray(tc.property, tc.inputVal) From 440babcc547a565ed44abf703cc4534a25a51fb1 Mon Sep 17 00:00:00 2001 From: lchan Date: Tue, 12 May 2020 16:11:23 -0700 Subject: [PATCH 25/37] Update TestEqual to TestEqualItems (new method name) --- .../openapi_spec_resource_schema_definition_property_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi/openapi_spec_resource_schema_definition_property_test.go b/openapi/openapi_spec_resource_schema_definition_property_test.go index 3d55b39de..133a3c5ef 100644 --- a/openapi/openapi_spec_resource_schema_definition_property_test.go +++ b/openapi/openapi_spec_resource_schema_definition_property_test.go @@ -1654,7 +1654,7 @@ func TestValidateFunc(t *testing.T) { }) } -func TestEqual(t *testing.T) { +func TestEqualItems(t *testing.T) { testCases := []struct { name string schemaDefProp specSchemaDefinitionProperty From 7f1f8c0a3444e0d0941ffd45307fc96b939f7a91 Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 12 May 2020 16:17:55 -0700 Subject: [PATCH 26/37] remove non needed switch - bring to the if statement the check for the list type - added a test to validate that behaviour --- openapi/common.go | 54 +++++++++++++++++++----------------------- openapi/common_test.go | 20 +++++++++++++++- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index 391e719b3..6a2a266a2 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -118,44 +118,40 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str // Use case 3: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) comes back with a shorter list where the remaining elems match the inputs. // Use case 4: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) some back with the list with the same size but some elems were updated func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) interface{} { - if property.IgnoreItemsOrder && property.Required { + if property.IgnoreItemsOrder && property.Required && property.isArrayProperty() { newPropertyValue := []interface{}{} - switch property.Type { - case typeList: - inputValueArray := castValueToArray(property, inputPropertyValue) - remoteValueArray := castValueToArray(property, remoteValue) + inputValueArray := castValueToArray(property, inputPropertyValue) + remoteValueArray := castValueToArray(property, remoteValue) - if inputValueArray == nil || remoteValueArray == nil { - return remoteValue - } + if inputValueArray == nil || remoteValueArray == nil { + return remoteValue + } - for _, inputItemValue := range inputValueArray { - for _, remoteItemValue := range remoteValueArray { - if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { - newPropertyValue = append(newPropertyValue, inputItemValue) - break - } - } - } - modifiedItems := []interface{}{} + for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { - match := false - for _, inputItemValue := range inputValueArray { - if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { - match = true - break - } + if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { + newPropertyValue = append(newPropertyValue, inputItemValue) + break } - if !match { - modifiedItems = append(modifiedItems, remoteItemValue) + } + } + modifiedItems := []interface{}{} + for _, remoteItemValue := range remoteValueArray { + match := false + for _, inputItemValue := range inputValueArray { + if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { + match = true + break } } - for _, updatedItem := range modifiedItems { - newPropertyValue = append(newPropertyValue, updatedItem) + if !match { + modifiedItems = append(modifiedItems, remoteItemValue) } - - return newPropertyValue } + for _, updatedItem := range modifiedItems { + newPropertyValue = append(newPropertyValue, updatedItem) + } + return newPropertyValue } return remoteValue } diff --git a/openapi/common_test.go b/openapi/common_test.go index 743175422..0060ae69e 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -1089,7 +1089,25 @@ func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { } } -func TestCompareInputPropertyValueWithPayloadPropertyValueIgnoreOrderDisabled(t *testing.T) { +func TestProcessIgnoreOrderIfEnabledNonListProperty(t *testing.T) { + Convey("Given a non list property that is marked as required and for some reason also as IgnoreItemsOrder", t, func() { + property := specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeString, + IgnoreItemsOrder: true, + Required: true, + } + remoteValue := "someString" + Convey("When processIgnoreOrderIfEnabled", func() { + output := processIgnoreOrderIfEnabled(property, nil, remoteValue) + Convey("Then the output should be the string from remote", func() { + So(output, ShouldResemble, remoteValue) + }) + }) + }) +} + +func TestProcessIgnoreOrderIfEnabledIgnoreOrderDisabled(t *testing.T) { Convey("Given a a list of strings property definition (with ignore order set to false) and the corresponding input/remote lists", t, func() { property := specSchemaDefinitionProperty{ Name: "list_prop", From 488cb67d47ef69dc674e4a8117b5e3d029ce1219 Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 12 May 2020 16:18:24 -0700 Subject: [PATCH 27/37] update test name to match the method under test --- openapi/common_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi/common_test.go b/openapi/common_test.go index 0060ae69e..e7874f30e 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -789,7 +789,7 @@ func TestSetStateID(t *testing.T) { }) } -func TestCompareInputPropertyValueWithPayloadPropertyValue(t *testing.T) { +func TestProcessIgnoreOrderIfEnabled(t *testing.T) { testCases := []struct { name string property specSchemaDefinitionProperty From 172d295b6550fd9510de2661e03341a40bac523f Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 12 May 2020 16:24:08 -0700 Subject: [PATCH 28/37] add new use cases to check for input value being nil --- openapi/common_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/openapi/common_test.go b/openapi/common_test.go index e7874f30e..76ec78f85 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -1211,6 +1211,14 @@ func TestCastValueToArray(t *testing.T) { inputVal: "someValue", expectedOutput: nil, }, + { + name: "value is nil", + property: specSchemaDefinitionProperty{ + Type: typeString, + }, + inputVal: nil, + expectedOutput: nil, + }, } for _, tc := range testCases { From 0cf829506018510b59b375912746c7ab480c6bde Mon Sep 17 00:00:00 2001 From: dikhan Date: Tue, 12 May 2020 17:01:34 -0700 Subject: [PATCH 29/37] integrate processIgnoreOrderIfEnabled in updateStateWithPayloadData - added test to cover new code with the array of object and array of string use cases - created helper method shouldIgnoreOrder in specSchemaDefinitionProperty - removed no longer needed casting method castValueToArray. The input value for list will always come as []interface --- openapi/common.go | 48 ++--- openapi/common_test.go | 182 ++++++++---------- ...pec_resource_schema_definition_property.go | 4 + ...esource_schema_definition_property_test.go | 38 ++++ 4 files changed, 139 insertions(+), 133 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index 6a2a266a2..a0bd64175 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -86,7 +86,7 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str if err != nil { return err } - for propertyName, propertyValue := range remoteData { + for propertyName, propertyRemoteValue := range remoteData { property, err := resourceSchema.getProperty(propertyName) if err != nil { log.Printf("[WARN] The API returned a property that is not specified in the resource's schema definition in the OpenAPI document - error = %s", err) @@ -96,7 +96,13 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str continue } - value, err := convertPayloadToLocalStateDataValue(property, propertyValue, false) + propValue := propertyRemoteValue + if property.shouldIgnoreOrder() { + desiredValue := resourceLocalData.Get(property.getTerraformCompliantPropertyName()) + propValue = processIgnoreOrderIfEnabled(*property, desiredValue, propertyRemoteValue) + } + + value, err := convertPayloadToLocalStateDataValue(property, propValue, false) if err != nil { return err } @@ -118,10 +124,14 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str // Use case 3: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) comes back with a shorter list where the remaining elems match the inputs. // Use case 4: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) some back with the list with the same size but some elems were updated func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) interface{} { - if property.IgnoreItemsOrder && property.Required && property.isArrayProperty() { + if inputPropertyValue == nil { // treat remote as the final state if input value does not exists + return remoteValue + } + if property.shouldIgnoreOrder() && property.Required { newPropertyValue := []interface{}{} - inputValueArray := castValueToArray(property, inputPropertyValue) - remoteValueArray := castValueToArray(property, remoteValue) + + inputValueArray := inputPropertyValue.([]interface{}) + remoteValueArray := remoteValue.([]interface{}) if inputValueArray == nil || remoteValueArray == nil { return remoteValue @@ -156,34 +166,6 @@ func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPro return remoteValue } -func castValueToArray(property specSchemaDefinitionProperty, value interface{}) []interface{} { - interfaceArray := []interface{}{} - if property.Type == typeList { - switch property.ArrayItemsType { - case typeString: - for _, item := range value.([]string) { - interfaceArray = append(interfaceArray, item) - } - case typeInt: - for _, item := range value.([]int) { - interfaceArray = append(interfaceArray, item) - } - case typeFloat: - for _, item := range value.([]float64) { - interfaceArray = append(interfaceArray, item) - } - case typeObject: - for _, item := range value.([]interface{}) { - interfaceArray = append(interfaceArray, item) - } - default: - return nil - } - return interfaceArray - } - return nil -} - func convertPayloadToLocalStateDataValue(property *specSchemaDefinitionProperty, propertyValue interface{}, useString bool) (interface{}, error) { if propertyValue == nil { return nil, nil diff --git a/openapi/common_test.go b/openapi/common_test.go index 76ec78f85..9ea3ac78c 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -335,6 +335,68 @@ func TestUpdateStateWithPayloadData(t *testing.T) { }) }) + Convey("Given a resource factory containing a schema with property lists that have the IgnoreItemsOrder set to true", t, func() { + objectSchemaDefinition := &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + newIntSchemaDefinitionPropertyWithDefaults("origin_port", "", true, false, 80), + newStringSchemaDefinitionPropertyWithDefaults("protocol", "", true, false, "http"), + }, + } + arrayObjectStateValue := []interface{}{ + map[string]interface{}{ + "origin_port": 80, + "protocol": "http", + }, + map[string]interface{}{ + "origin_port": 443, + "protocol": "https", + }, + } + listOfObjectsProperty := newListSchemaDefinitionPropertyWithDefaults("slice_object_property", "", true, false, false, arrayObjectStateValue, typeObject, objectSchemaDefinition) + listOfObjectsProperty.IgnoreItemsOrder = true + + listOfStrings := newListSchemaDefinitionPropertyWithDefaults("slice_property", "", true, false, false, []interface{}{"value1", "value2"}, typeString, nil) + listOfStrings.IgnoreItemsOrder = true + + r, resourceData := testCreateResourceFactory(t, listOfStrings, listOfObjectsProperty) + Convey("When is called with a map containing all property types supported (string, int, number, bool, slice of primitives, objects, list of objects and property with nested objects)", func() { + remoteData := map[string]interface{}{ + listOfStrings.Name: []interface{}{"value2", "value1"}, + listOfObjectsProperty.Name: []interface{}{ + map[string]interface{}{ + "origin_port": 443, + "protocol": "https", + }, + map[string]interface{}{ + "origin_port": 80, + "protocol": "http", + }, + }, + } + err := updateStateWithPayloadData(r.openAPIResource, remoteData, resourceData) + Convey("Then the err returned should be nil", func() { + So(err, ShouldBeNil) + }) + Convey("And the expectedValue should equal to the expectedValue coming from remote, and also the key expectedValue should be the preferred as defined in the property", func() { + // keys stores in the resource data struct are always snake case + So(len(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})), ShouldEqual, 2) + So(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})[0], ShouldEqual, listOfStrings.Default.([]interface{})[0]) + So(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})[1], ShouldEqual, listOfStrings.Default.([]interface{})[1]) + + So(len(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})), ShouldEqual, 2) + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{}), ShouldContainKey, "origin_port") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{}), ShouldContainKey, "protocol") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{})["origin_port"], ShouldEqual, arrayObjectStateValue[0].(map[string]interface{})["origin_port"].(int)) + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{})["protocol"], ShouldEqual, arrayObjectStateValue[0].(map[string]interface{})["protocol"]) + + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{}), ShouldContainKey, "origin_port") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{}), ShouldContainKey, "protocol") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{})["origin_port"], ShouldEqual, arrayObjectStateValue[1].(map[string]interface{})["origin_port"].(int)) + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{})["protocol"], ShouldEqual, arrayObjectStateValue[1].(map[string]interface{})["protocol"]) + }) + }) + }) + Convey("Given a resource factory", t, func() { r, resourceData := testCreateResourceFactory(t, stringWithPreferredNameProperty) Convey("When is called with a map remoteData containing more properties than then ones specified in the schema (this means the API is returning more info than the one specified in the swagger file)", func() { @@ -807,8 +869,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, - remoteValue: []string{"inputVal1", "inputVal2", "inputVal3"}, + inputPropertyValue: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, + remoteValue: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, }, { @@ -820,8 +882,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []string{"inputVal3", "inputVal1", "inputVal2"}, - remoteValue: []string{"inputVal2", "inputVal3", "inputVal1"}, + inputPropertyValue: []interface{}{"inputVal3", "inputVal1", "inputVal2"}, + remoteValue: []interface{}{"inputVal2", "inputVal3", "inputVal1"}, expectedOutput: []interface{}{"inputVal3", "inputVal1", "inputVal2"}, }, { @@ -833,8 +895,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []string{"inputVal1", "inputVal2", "inputVal3"}, - remoteValue: []string{"inputVal2", "inputVal1"}, + inputPropertyValue: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, + remoteValue: []interface{}{"inputVal2", "inputVal1"}, expectedOutput: []interface{}{"inputVal1", "inputVal2"}, }, { @@ -846,8 +908,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []string{"inputVal1", "inputVal2"}, - remoteValue: []string{"inputVal3", "inputVal2", "inputVal1"}, + inputPropertyValue: []interface{}{"inputVal1", "inputVal2"}, + remoteValue: []interface{}{"inputVal3", "inputVal2", "inputVal1"}, expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, }, @@ -861,8 +923,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []int{3, 1, 2}, - remoteValue: []int{2, 3, 1}, + inputPropertyValue: []interface{}{3, 1, 2}, + remoteValue: []interface{}{2, 3, 1}, expectedOutput: []interface{}{3, 1, 2}, }, { @@ -874,8 +936,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []int{1, 2, 3}, - remoteValue: []int{2, 1}, + inputPropertyValue: []interface{}{1, 2, 3}, + remoteValue: []interface{}{2, 1}, expectedOutput: []interface{}{1, 2}, }, { @@ -887,8 +949,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []int{1, 2}, - remoteValue: []int{3, 2, 1}, + inputPropertyValue: []interface{}{1, 2}, + remoteValue: []interface{}{3, 2, 1}, expectedOutput: []interface{}{1, 2, 3}, }, @@ -902,8 +964,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []float64{3.0, 1.0, 2.0}, - remoteValue: []float64{2.0, 3.0, 1.0}, + inputPropertyValue: []interface{}{3.0, 1.0, 2.0}, + remoteValue: []interface{}{2.0, 3.0, 1.0}, expectedOutput: []interface{}{3.0, 1.0, 2.0}, }, { @@ -915,8 +977,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []float64{1.0, 2.0, 3.0}, - remoteValue: []float64{2.0, 1.0}, + inputPropertyValue: []interface{}{1.0, 2.0, 3.0}, + remoteValue: []interface{}{2.0, 1.0}, expectedOutput: []interface{}{1.0, 2.0}, }, { @@ -928,8 +990,8 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { IgnoreItemsOrder: true, Required: true, }, - inputPropertyValue: []float64{1.0, 2.0}, - remoteValue: []float64{3.0, 2.0, 1.0}, + inputPropertyValue: []interface{}{1.0, 2.0}, + remoteValue: []interface{}{3.0, 2.0, 1.0}, expectedOutput: []interface{}{1.0, 2.0, 3.0}, }, @@ -1146,83 +1208,3 @@ func TestCompareInputPropertyValueWithPayloadPropertyValueBools(t *testing.T) { }) }) } - -func TestCastValueToArray(t *testing.T) { - testCases := []struct { - name string - property specSchemaDefinitionProperty - inputVal interface{} - expectedOutput []interface{} - }{ - { - name: "array of strings", - property: specSchemaDefinitionProperty{ - Type: typeList, - ArrayItemsType: typeString, - }, - inputVal: []string{"inputVal1", "inputVal2", "inputVal3"}, - expectedOutput: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, - }, - { - name: "array of ints", - property: specSchemaDefinitionProperty{ - Type: typeList, - ArrayItemsType: typeInt, - }, - inputVal: []int{1, 2, 3}, - expectedOutput: []interface{}{1, 2, 3}, - }, - { - name: "array of floats", - property: specSchemaDefinitionProperty{ - Type: typeList, - ArrayItemsType: typeFloat, - }, - inputVal: []float64{1.0, 2.0, 3.0}, - expectedOutput: []interface{}{1.0, 2.0, 3.0}, - }, - { - name: "array of objects", - property: specSchemaDefinitionProperty{ - Type: typeList, - ArrayItemsType: typeObject, - }, - inputVal: []interface{}{ - map[string]interface{}{"group": "someGroup"}, - }, - expectedOutput: []interface{}{ - map[string]interface{}{"group": "someGroup"}, - }, - }, - { - name: "array of bools (unsupported type)", - property: specSchemaDefinitionProperty{ - Type: typeList, - ArrayItemsType: typeBool, - }, - inputVal: []bool{true}, - expectedOutput: nil, - }, - { - name: "value is not an array", - property: specSchemaDefinitionProperty{ - Type: typeString, - }, - inputVal: "someValue", - expectedOutput: nil, - }, - { - name: "value is nil", - property: specSchemaDefinitionProperty{ - Type: typeString, - }, - inputVal: nil, - expectedOutput: nil, - }, - } - - for _, tc := range testCases { - output := castValueToArray(tc.property, tc.inputVal) - assert.Equal(t, tc.expectedOutput, output) - } -} diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 057084811..d431a3e97 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -115,6 +115,10 @@ func (s *specSchemaDefinitionProperty) isArrayProperty() bool { return s.Type == typeList } +func (s *specSchemaDefinitionProperty) shouldIgnoreOrder() bool { + return s.Type == typeList && s.IgnoreItemsOrder +} + func (s *specSchemaDefinitionProperty) isArrayOfObjectsProperty() bool { return s.Type == typeList && s.ArrayItemsType == typeObject } diff --git a/openapi/openapi_spec_resource_schema_definition_property_test.go b/openapi/openapi_spec_resource_schema_definition_property_test.go index 133a3c5ef..05153bbcf 100644 --- a/openapi/openapi_spec_resource_schema_definition_property_test.go +++ b/openapi/openapi_spec_resource_schema_definition_property_test.go @@ -1891,6 +1891,44 @@ func TestValidateValueType(t *testing.T) { } } +func Test_shouldIgnoreOrder(t *testing.T) { + Convey("Given a scjema definition property that is a list and configured with ignore order", t, func() { + p := &specSchemaDefinitionProperty{ + Type: typeList, + IgnoreItemsOrder: true, + } + Convey("When shouldIgnoreOrder is called", func() { + shouldIgnoreOrder := p.shouldIgnoreOrder() + Convey("Then the err returned should be true", func() { + So(shouldIgnoreOrder, ShouldBeTrue) + }) + }) + }) + Convey("Given a scjema definition property that is NOT a list", t, func() { + p := &specSchemaDefinitionProperty{ + Type: typeString, + } + Convey("When shouldIgnoreOrder is called", func() { + shouldIgnoreOrder := p.shouldIgnoreOrder() + Convey("Then the err returned should be false", func() { + So(shouldIgnoreOrder, ShouldBeFalse) + }) + }) + }) + Convey("Given a scjema definition property that is a list but the ignore order is set to false", t, func() { + p := &specSchemaDefinitionProperty{ + Type: typeList, + IgnoreItemsOrder: false, + } + Convey("When shouldIgnoreOrder is called", func() { + shouldIgnoreOrder := p.shouldIgnoreOrder() + Convey("Then the err returned should be false", func() { + So(shouldIgnoreOrder, ShouldBeFalse) + }) + }) + }) +} + func Test_shouldUseLegacyTerraformSDKBlockApproachForComplexObjects(t *testing.T) { Convey("shouldUseLegacyTerraformSDKBlockApproachForComplexObject", t, func() { From beb4fa3e7f4808b002ed4a20d33105ba8f056e44 Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 11:51:23 -0700 Subject: [PATCH 30/37] fix integration test adding the newly supported extension x-terraform-ignore-order: true - removed constraint of property needing to be required in processIgnoreOrderIfEnabled to ignore the ordering as there may be properties that are lists and also optional but the ordering might also need to be ignored. --- openapi/common.go | 2 +- openapi/common_test.go | 27 ++++++--------------------- tests/e2e/gray_box_cdns_test.go | 1 + 3 files changed, 8 insertions(+), 22 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index a0bd64175..a8587c99b 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -127,7 +127,7 @@ func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPro if inputPropertyValue == nil { // treat remote as the final state if input value does not exists return remoteValue } - if property.shouldIgnoreOrder() && property.Required { + if property.shouldIgnoreOrder() { newPropertyValue := []interface{}{} inputValueArray := inputPropertyValue.([]interface{}) diff --git a/openapi/common_test.go b/openapi/common_test.go index 9ea3ac78c..4036f12a0 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -867,7 +867,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeString, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, remoteValue: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, @@ -880,7 +879,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeString, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{"inputVal3", "inputVal1", "inputVal2"}, remoteValue: []interface{}{"inputVal2", "inputVal3", "inputVal1"}, @@ -893,7 +891,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeString, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{"inputVal1", "inputVal2", "inputVal3"}, remoteValue: []interface{}{"inputVal2", "inputVal1"}, @@ -906,7 +903,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeString, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{"inputVal1", "inputVal2"}, remoteValue: []interface{}{"inputVal3", "inputVal2", "inputVal1"}, @@ -921,7 +917,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeInt, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{3, 1, 2}, remoteValue: []interface{}{2, 3, 1}, @@ -934,7 +929,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeInt, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{1, 2, 3}, remoteValue: []interface{}{2, 1}, @@ -947,7 +941,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeInt, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{1, 2}, remoteValue: []interface{}{3, 2, 1}, @@ -962,7 +955,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeFloat, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{3.0, 1.0, 2.0}, remoteValue: []interface{}{2.0, 3.0, 1.0}, @@ -975,7 +967,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeFloat, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{1.0, 2.0, 3.0}, remoteValue: []interface{}{2.0, 1.0}, @@ -988,7 +979,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { Type: typeList, ArrayItemsType: typeFloat, IgnoreItemsOrder: true, - Required: true, }, inputPropertyValue: []interface{}{1.0, 2.0}, remoteValue: []interface{}{3.0, 2.0, 1.0}, @@ -1016,7 +1006,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { }, }, }, - Required: true, }, inputPropertyValue: []interface{}{ map[string]interface{}{ @@ -1069,7 +1058,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { }, }, }, - Required: true, }, inputPropertyValue: []interface{}{ map[string]interface{}{ @@ -1114,7 +1102,6 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { }, }, }, - Required: true, }, inputPropertyValue: []interface{}{ map[string]interface{}{ @@ -1157,7 +1144,6 @@ func TestProcessIgnoreOrderIfEnabledNonListProperty(t *testing.T) { Name: "list_prop", Type: typeString, IgnoreItemsOrder: true, - Required: true, } remoteValue := "someString" Convey("When processIgnoreOrderIfEnabled", func() { @@ -1176,14 +1162,13 @@ func TestProcessIgnoreOrderIfEnabledIgnoreOrderDisabled(t *testing.T) { Type: typeList, ArrayItemsType: typeString, IgnoreItemsOrder: false, - Required: true, } - inputPropertyValue := []string{"inputVal1", "inputVal2", "inputVal3"} - remoteValue := []string{"inputVal1", "inputVal2", "inputVal3"} + inputPropertyValue := []interface{}{"inputVal1", "inputVal2", "inputVal3"} + remoteValue := []interface{}{"inputVal1", "inputVal2", "inputVal3"} Convey("When processIgnoreOrderIfEnabled", func() { output := processIgnoreOrderIfEnabled(property, inputPropertyValue, remoteValue) Convey("Then the output should match the remote list", func() { - So(output, ShouldResemble, []string{"inputVal1", "inputVal2", "inputVal3"}) + So(output, ShouldResemble, []interface{}{"inputVal1", "inputVal2", "inputVal3"}) }) }) }) @@ -1198,12 +1183,12 @@ func TestCompareInputPropertyValueWithPayloadPropertyValueBools(t *testing.T) { IgnoreItemsOrder: true, Required: true, } - inputPropertyValue := []bool{true} - remoteValue := []bool{false} + inputPropertyValue := []interface{}{true} + remoteValue := []interface{}{false} Convey("When processIgnoreOrderIfEnabled", func() { output := processIgnoreOrderIfEnabled(property, inputPropertyValue, remoteValue) Convey("Then the output should match the remote list", func() { - So(output, ShouldResemble, []bool{false}) + So(output, ShouldResemble, []interface{}{false}) }) }) }) diff --git a/tests/e2e/gray_box_cdns_test.go b/tests/e2e/gray_box_cdns_test.go index 6895572e1..b2ff72d24 100644 --- a/tests/e2e/gray_box_cdns_test.go +++ b/tests/e2e/gray_box_cdns_test.go @@ -508,6 +508,7 @@ definitions: type: "string" list_prop: type: "array" + x-terraform-ignore-order: true items: type: "string"` apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 890eb6b54627f8a205bc17d6847d91bca9e0e2cf Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 11:51:35 -0700 Subject: [PATCH 31/37] vet cleanup --- openapi/openapi_spec_resource_schema_definition_property.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index d431a3e97..916919ff0 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -337,7 +337,7 @@ func (s *specSchemaDefinitionProperty) equalItems(itemsType schemaDefinitionProp if len(list1) != len(list2) { return false } - for idx, _ := range list1 { + for idx := range list1 { return s.equalItems(s.ArrayItemsType, list1[idx], list2[idx]) } case typeObject: From 7378bc115cb16d12f2b741a071771fcf98dfdf61 Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 11:51:45 -0700 Subject: [PATCH 32/37] go mod dep updates --- go.mod | 9 +++++---- go.sum | 10 ++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 796cf9ca3..bc07fb415 100644 --- a/go.mod +++ b/go.mod @@ -41,13 +41,14 @@ require ( github.com/spf13/cobra v0.0.3 github.com/stretchr/testify v1.3.0 github.com/stvp/go-udp-testing v0.0.0-20191102171040-06b61409b154 + github.com/yuin/goldmark v1.1.30 // indirect github.com/zach-klippenstein/goregen v0.0.0-20160303162051-795b5e3961ea // indirect - golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59 // indirect + golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37 // indirect golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect - golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e // indirect + golang.org/x/net v0.0.0-20200506145744-7e3656a0809f // indirect golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a // indirect - golang.org/x/sys v0.0.0-20200331124033-c3d80250170d // indirect - golang.org/x/tools v0.0.0-20200331202046-9d5940d49312 // indirect + golang.org/x/sys v0.0.0-20200513112337-417ce2331b5c // indirect + golang.org/x/tools v0.0.0-20200513175351-0951661448da // indirect gopkg.in/mgo.v2 v2.0.0-20160818020120-3f83fa500528 // indirect gopkg.in/yaml.v2 v2.2.2 ) diff --git a/go.sum b/go.sum index e2a97ec55..31d8ab990 100644 --- a/go.sum +++ b/go.sum @@ -289,6 +289,7 @@ github.com/ulikunitz/xz v0.5.5/go.mod h1:2bypXElzHzzJZwzH67Y6wb67pO62Rzfn7BSiF4A github.com/vmihailenco/msgpack v3.3.3+incompatible h1:wapg9xDUZDzGCNFlwc5SqI1rvcciqcxEHac4CYj89xI= github.com/vmihailenco/msgpack v3.3.3+incompatible/go.mod h1:fy3FlTQTDXWkZ7Bh6AcGMlsjHatGryHQYUTf1ShIgkk= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= +github.com/yuin/goldmark v1.1.30/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/zach-klippenstein/goregen v0.0.0-20160303162051-795b5e3961ea h1:CyhwejzVGvZ3Q2PSbQ4NRRYn+ZWv5eS1vlaEusT+bAI= github.com/zach-klippenstein/goregen v0.0.0-20160303162051-795b5e3961ea/go.mod h1:eNr558nEUjP8acGw8FFjTeWvSgU1stO7FAO6eknhHe4= github.com/zclconf/go-cty v1.0.0 h1:EWtv3gKe2wPLIB9hQRQJa7k/059oIfAqcEkCNnaVckk= @@ -312,6 +313,8 @@ golang.org/x/crypto v0.0.0-20191227163750-53104e6ec876 h1:sKJQZMuxjOAR/Uo2LBfU90 golang.org/x/crypto v0.0.0-20191227163750-53104e6ec876/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59 h1:3zb4D3T4G8jdExgVU/95+vQXfpEPiMdCaZgmGVxjNHM= golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= +golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37 h1:cg5LA/zNPRzIXIWSCxQW10Rvpy94aQh3LT/ShoCpkHw= +golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190510132918-efd6b22b2522/go.mod h1:ZjyILWgesfNpC6sMxTJOJm9Kp84zZh5NQWvqDGG3Qr8= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= @@ -351,6 +354,8 @@ golang.org/x/net v0.0.0-20191209160850-c0dbc17a3553/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e h1:3G+cUijn7XD+S4eJFddp53Pv7+slrESplyjG25HgL+k= golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A= +golang.org/x/net v0.0.0-20200506145744-7e3656a0809f h1:QBjCr1Fz5kw158VqdE9JfI9cJnl/ymnJWAdMuinqL7Y= +golang.org/x/net v0.0.0-20200506145744-7e3656a0809f/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421 h1:Wo7BWFiOk0QRFMLYMqJGFMd9CgUAcGx7V+qEg/h5IBI= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -381,6 +386,7 @@ golang.org/x/sys v0.0.0-20191220220014-0732a990476f/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20191224085550-c709ea063b76/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200323222414-85ca7c5b95cd/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200331124033-c3d80250170d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20200513112337-417ce2331b5c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -407,6 +413,10 @@ golang.org/x/tools v0.0.0-20191227053925-7b8e75db28f4/go.mod h1:TB2adYChydJhpapK golang.org/x/tools v0.0.0-20200130002326-2f3ba24bd6e7/go.mod h1:TB2adYChydJhpapKDTa4BR/hXlZSLoq2Wpct/0txZ28= golang.org/x/tools v0.0.0-20200331202046-9d5940d49312 h1:2PHG+Ia3gK1K2kjxZnSylizb//eyaMG8gDFbOG7wLV8= golang.org/x/tools v0.0.0-20200331202046-9d5940d49312/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200513122804-866d71a3170a h1:LkBZllNM46txyXU+/e601XXm26FTs7DJr8TRZa5w35Q= +golang.org/x/tools v0.0.0-20200513122804-866d71a3170a/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200513175351-0951661448da h1:ZR1ivkcQoKXKtux9Rx3Em7iiSViMxQ5suNd5PZMUkPc= +golang.org/x/tools v0.0.0-20200513175351-0951661448da/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From 5a29d958bdb9351259d74b893aa9668c5489d66d Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 13:10:03 -0700 Subject: [PATCH 33/37] enable ignoreListOrderEnabled depending whether a resource or data source called the method - if the resource is trying to update the state then it is acceptable that the ignoreList may be set to true. However, when dealing with data sources since local state is not populated the ignore list is not applicable and the expectaion is to just use directly the remote values --- openapi/common.go | 20 ++++++- openapi/common_test.go | 77 ++++++++++++++++++++++++- openapi/data_source_factory.go | 2 +- openapi/data_source_instance_factory.go | 2 +- 4 files changed, 94 insertions(+), 7 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index a8587c99b..50725c649 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -79,9 +79,23 @@ func getParentIDs(openAPIResource SpecResource, data *schema.ResourceData) ([]st return []string{}, nil } -// updateStateWithPayloadData is in charge of saving the given payload into the state file. The property names are -// converted into compliant terraform names if needed. +// updateStateWithPayloadData is in charge of saving the given payload into the state file keeping for list properties the +// same order as the input (if the list property has the IgnoreItemsOrder set to true). The property names are converted into compliant terraform names if needed. +// The property names are converted into compliant terraform names if needed. func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[string]interface{}, resourceLocalData *schema.ResourceData) error { + return updateStateWithPayloadDataAndOptions(openAPIResource, remoteData, resourceLocalData, true) +} + +// dataSourceUpdateStateWithPayloadData is in charge of saving the given payload into the state file keeping for list properties the +// same order received by the API. The property names are converted into compliant terraform names if needed. +func dataSourceUpdateStateWithPayloadData(openAPIResource SpecResource, remoteData map[string]interface{}, resourceLocalData *schema.ResourceData) error { + return updateStateWithPayloadDataAndOptions(openAPIResource, remoteData, resourceLocalData, false) +} + +// updateStateWithPayloadDataAndOptions is in charge of saving the given payload into the state file AND if the ignoreListOrder is enabled +// it will go ahead and compare the items in the list (input vs remote) for properties of type list and the flag 'IgnoreItemsOrder' set to true +// The property names are converted into compliant terraform names if needed. +func updateStateWithPayloadDataAndOptions(openAPIResource SpecResource, remoteData map[string]interface{}, resourceLocalData *schema.ResourceData, ignoreListOrderEnabled bool) error { resourceSchema, err := openAPIResource.getResourceSchema() if err != nil { return err @@ -97,7 +111,7 @@ func updateStateWithPayloadData(openAPIResource SpecResource, remoteData map[str } propValue := propertyRemoteValue - if property.shouldIgnoreOrder() { + if ignoreListOrderEnabled && property.shouldIgnoreOrder() { desiredValue := resourceLocalData.Get(property.getTerraformCompliantPropertyName()) propValue = processIgnoreOrderIfEnabled(*property, desiredValue, propertyRemoteValue) } diff --git a/openapi/common_test.go b/openapi/common_test.go index 4036f12a0..4dd0c0bbd 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -359,7 +359,7 @@ func TestUpdateStateWithPayloadData(t *testing.T) { listOfStrings.IgnoreItemsOrder = true r, resourceData := testCreateResourceFactory(t, listOfStrings, listOfObjectsProperty) - Convey("When is called with a map containing all property types supported (string, int, number, bool, slice of primitives, objects, list of objects and property with nested objects)", func() { + Convey("When updateStateWithPayloadData is called", func() { remoteData := map[string]interface{}{ listOfStrings.Name: []interface{}{"value2", "value1"}, listOfObjectsProperty.Name: []interface{}{ @@ -377,7 +377,7 @@ func TestUpdateStateWithPayloadData(t *testing.T) { Convey("Then the err returned should be nil", func() { So(err, ShouldBeNil) }) - Convey("And the expectedValue should equal to the expectedValue coming from remote, and also the key expectedValue should be the preferred as defined in the property", func() { + Convey("And the expectedValue should maintain the order of the local input (not the order of the remote lists)", func() { // keys stores in the resource data struct are always snake case So(len(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})), ShouldEqual, 2) So(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})[0], ShouldEqual, listOfStrings.Default.([]interface{})[0]) @@ -416,6 +416,79 @@ func TestUpdateStateWithPayloadData(t *testing.T) { }) } +func TestDataSourceUpdateStateWithPayloadData(t *testing.T) { + Convey("Given a resource factory containing a schema with property lists that have the IgnoreItemsOrder set to true", t, func() { + objectSchemaDefinition := &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + newIntSchemaDefinitionPropertyWithDefaults("origin_port", "", true, false, 80), + newStringSchemaDefinitionPropertyWithDefaults("protocol", "", true, false, "http"), + }, + } + arrayObjectStateValue := []interface{}{} + listOfObjectsProperty := newListSchemaDefinitionPropertyWithDefaults("slice_object_property", "", true, false, false, arrayObjectStateValue, typeObject, objectSchemaDefinition) + listOfObjectsProperty.IgnoreItemsOrder = true + + listOfStrings := newListSchemaDefinitionPropertyWithDefaults("slice_property", "", true, false, false, []interface{}{"value1", "value2"}, typeString, nil) + listOfStrings.IgnoreItemsOrder = true + + r, resourceData := testCreateResourceFactory(t, listOfStrings, listOfObjectsProperty) + Convey("When dataSourceUpdateStateWithPayloadData is called", func() { + remoteData := map[string]interface{}{ + listOfStrings.Name: []interface{}{"value2", "value1"}, + listOfObjectsProperty.Name: []interface{}{ + map[string]interface{}{ + "origin_port": 443, + "protocol": "https", + }, + map[string]interface{}{ + "origin_port": 80, + "protocol": "http", + }, + }, + } + err := dataSourceUpdateStateWithPayloadData(r.openAPIResource, remoteData, resourceData) + Convey("Then the err returned should be nil", func() { + So(err, ShouldBeNil) + }) + Convey("And the expectedValue should equal to the expectedValue coming from remote", func() { + // keys stores in the resource data struct are always snake case + So(len(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})), ShouldEqual, 2) + So(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})[0], ShouldEqual, remoteData[listOfStrings.Name].([]interface{})[0]) + So(resourceData.Get(listOfStrings.getTerraformCompliantPropertyName()).([]interface{})[1], ShouldEqual, remoteData[listOfStrings.Name].([]interface{})[1]) + + So(len(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})), ShouldEqual, 2) + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{}), ShouldContainKey, "origin_port") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{}), ShouldContainKey, "protocol") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{})["origin_port"], ShouldEqual, remoteData[listOfObjectsProperty.Name].([]interface{})[0].(map[string]interface{})["origin_port"].(int)) + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[0].(map[string]interface{})["protocol"], ShouldEqual, remoteData[listOfObjectsProperty.Name].([]interface{})[0].(map[string]interface{})["protocol"]) + + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{}), ShouldContainKey, "origin_port") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{}), ShouldContainKey, "protocol") + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{})["origin_port"], ShouldEqual, remoteData[listOfObjectsProperty.Name].([]interface{})[1].(map[string]interface{})["origin_port"].(int)) + So(resourceData.Get(listOfObjectsProperty.getTerraformCompliantPropertyName()).([]interface{})[1].(map[string]interface{})["protocol"], ShouldEqual, remoteData[listOfObjectsProperty.Name].([]interface{})[1].(map[string]interface{})["protocol"]) + }) + }) + }) + + Convey("Given a resource factory", t, func() { + r, resourceData := testCreateResourceFactory(t, stringWithPreferredNameProperty) + Convey("When is called with a map remoteData containing more properties than then ones specified in the schema (this means the API is returning more info than the one specified in the swagger file)", func() { + remoteData := map[string]interface{}{ + stringWithPreferredNameProperty.Name: "someUpdatedStringValue", + "some_other_property_not_documented_in_openapi_doc": 15, + } + err := updateStateWithPayloadData(r.openAPIResource, remoteData, resourceData) + Convey("Then the err returned should be nil", func() { + So(err, ShouldBeNil) + }) + Convey("Then the resource state data only contains the properties and values for the documented properties", func() { + So(resourceData.Get(stringWithPreferredNameProperty.getTerraformCompliantPropertyName()), ShouldEqual, remoteData[stringWithPreferredNameProperty.Name]) + So(resourceData.Get("some_other_property_not_documented_in_openapi_doc"), ShouldBeNil) + }) + }) + }) +} + func TestConvertPayloadToLocalStateDataValue(t *testing.T) { Convey("Given a resource factory", t, func() { diff --git a/openapi/data_source_factory.go b/openapi/data_source_factory.go index 3ad40a14c..ef0977f84 100644 --- a/openapi/data_source_factory.go +++ b/openapi/data_source_factory.go @@ -125,7 +125,7 @@ func (d dataSourceFactory) read(data *schema.ResourceData, i interface{}) error return err } - return updateStateWithPayloadData(d.openAPIResource, filteredResults[0], data) + return dataSourceUpdateStateWithPayloadData(d.openAPIResource, filteredResults[0], data) } func (d dataSourceFactory) filterMatch(filters filters, payloadItem map[string]interface{}) bool { diff --git a/openapi/data_source_instance_factory.go b/openapi/data_source_instance_factory.go index fdcba06b7..b126c3e88 100644 --- a/openapi/data_source_instance_factory.go +++ b/openapi/data_source_instance_factory.go @@ -84,5 +84,5 @@ func (d dataSourceInstanceFactory) read(data *schema.ResourceData, i interface{} if err != nil { return err } - return updateStateWithPayloadData(d.openAPIResource, responsePayload, data) + return dataSourceUpdateStateWithPayloadData(d.openAPIResource, responsePayload, data) } From cec48c5b7bdc1f3fb9dcbc2026f8525f32494c42 Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 16:56:14 -0700 Subject: [PATCH 34/37] add check for nil remoteValue --- openapi/common.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/openapi/common.go b/openapi/common.go index 50725c649..d76435a33 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -138,19 +138,13 @@ func updateStateWithPayloadDataAndOptions(openAPIResource SpecResource, remoteDa // Use case 3: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) comes back with a shorter list where the remaining elems match the inputs. // Use case 4: The desired state for an array property (input from user, inputPropertyValue) contains items in certain order BUT the remote state (remoteValue) some back with the list with the same size but some elems were updated func processIgnoreOrderIfEnabled(property specSchemaDefinitionProperty, inputPropertyValue, remoteValue interface{}) interface{} { - if inputPropertyValue == nil { // treat remote as the final state if input value does not exists + if inputPropertyValue == nil || remoteValue == nil { // treat remote as the final state if input value does not exists return remoteValue } if property.shouldIgnoreOrder() { newPropertyValue := []interface{}{} - inputValueArray := inputPropertyValue.([]interface{}) remoteValueArray := remoteValue.([]interface{}) - - if inputValueArray == nil || remoteValueArray == nil { - return remoteValue - } - for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { From efe0ba42a2c8e812248c8213fe79bd739cb59a4b Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 16:56:45 -0700 Subject: [PATCH 35/37] add test TestUpdateStateWithPayloadDataAndOptions - cleaned up some tests and merged in to TestConvertPayloadToLocalStateDataValue --- openapi/common_test.go | 182 ++++++++++++++++++++++++++++------------- 1 file changed, 124 insertions(+), 58 deletions(-) diff --git a/openapi/common_test.go b/openapi/common_test.go index 4dd0c0bbd..96ac2f342 100644 --- a/openapi/common_test.go +++ b/openapi/common_test.go @@ -2,6 +2,7 @@ package openapi import ( "errors" + "fmt" "io/ioutil" "net/http" "strconv" @@ -489,6 +490,78 @@ func TestDataSourceUpdateStateWithPayloadData(t *testing.T) { }) } +func TestUpdateStateWithPayloadDataAndOptions(t *testing.T) { + Convey("Given a resource factory containing a schema with property lists that have the IgnoreItemsOrder set to true", t, func() { + specResource := &specStubResource{ + error: fmt.Errorf("some error"), + } + Convey("When updateStateWithPayloadDataAndOptions is called", func() { + err := updateStateWithPayloadDataAndOptions(specResource, nil, nil, true) + Convey("Then the err returned should match the expected one", func() { + So(err, ShouldEqual, specResource.error) + }) + }) + }) + Convey("Given a resource factory containing just a property ID", t, func() { + specResource := &specStubResource{ + schemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{idProperty}, + }, + } + Convey("When updateStateWithPayloadDataAndOptions is called", func() { + remoteData := map[string]interface{}{ + idProperty.Name: "someID", + } + var resourceLocalData *schema.ResourceData + err := updateStateWithPayloadDataAndOptions(specResource, remoteData, resourceLocalData, true) + Convey("Then the err returned should be nil", func() { + So(err, ShouldBeNil) + }) + Convey("And the resource local data should be intact since the id property is ignored when updating the resource data file behind the scenes", func() { + So(resourceLocalData, ShouldEqual, nil) + }) + }) + }) + Convey("Given a resource factory containing a property with certain type", t, func() { + r, resourceData := testCreateResourceFactory(t, &specSchemaDefinitionProperty{ + Name: "wrong_property", + Type: typeObject, + SpecSchemaDefinition: &specSchemaDefinition{}, + }) + Convey("When updateStateWithPayloadDataAndOptions is called with a remote data containing the property but the value does not match the property type", func() { + remoteData := map[string]interface{}{ + "wrong_property": "someValueNotMatchingTheType", + } + err := updateStateWithPayloadDataAndOptions(r.openAPIResource, remoteData, resourceData, true) + Convey("Then the err returned should match the expected one", func() { + So(err.Error(), ShouldEqual, "wrong_property: must be a map") + }) + }) + }) + Convey("Given a resource factory containing a property with a type that the remote value does not match", t, func() { + r := &specStubResource{ + schemaDefinition: &specSchemaDefinition{ + Properties: specSchemaDefinitionProperties{ + &specSchemaDefinitionProperty{ + Name: "not_well_configured_property", + Type: typeList, + ArrayItemsType: schemaDefinitionPropertyType("unknown"), + }, + }, + }, + } + Convey("When updateStateWithPayloadDataAndOptions", func() { + remoteData := map[string]interface{}{ + "not_well_configured_property": []interface{}{"something"}, + } + err := updateStateWithPayloadDataAndOptions(r, remoteData, nil, true) + Convey("Then the err returned should match the expected one", func() { + So(err.Error(), ShouldEqual, "property 'not_well_configured_property' is supposed to be an array objects") + }) + }) + }) +} + func TestConvertPayloadToLocalStateDataValue(t *testing.T) { Convey("Given a resource factory", t, func() { @@ -930,7 +1003,7 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { property specSchemaDefinitionProperty inputPropertyValue interface{} remoteValue interface{} - expectedOutput []interface{} + expectedOutput interface{} }{ // String use cases { @@ -1203,66 +1276,59 @@ func TestProcessIgnoreOrderIfEnabled(t *testing.T) { }, }, }, + { + name: "inputPropertyValue is nil", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: true, + }, + inputPropertyValue: nil, + remoteValue: []interface{}{}, + expectedOutput: []interface{}{}, + }, + { + name: "remoteValue is nil", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: true, + }, + inputPropertyValue: []interface{}{}, + remoteValue: nil, + expectedOutput: nil, + }, + { + name: "IgnoreItemsOrder is set to false", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeString, + IgnoreItemsOrder: false, + }, + inputPropertyValue: []interface{}{"inputVal1"}, + remoteValue: []interface{}{"inputVal1", "inputVal2"}, + expectedOutput: []interface{}{"inputVal1", "inputVal2"}, + }, + { + name: "list of bools property definition and the corresponding input/remote lists", + property: specSchemaDefinitionProperty{ + Name: "list_prop", + Type: typeList, + ArrayItemsType: typeBool, + IgnoreItemsOrder: true, + Required: true, + }, + inputPropertyValue: []interface{}{true}, + remoteValue: []interface{}{false}, + expectedOutput: []interface{}{false}, + }, } for _, tc := range testCases { output := processIgnoreOrderIfEnabled(tc.property, tc.inputPropertyValue, tc.remoteValue) - assert.Equal(t, tc.expectedOutput, output) + assert.Equal(t, tc.expectedOutput, output, tc.name) } } - -func TestProcessIgnoreOrderIfEnabledNonListProperty(t *testing.T) { - Convey("Given a non list property that is marked as required and for some reason also as IgnoreItemsOrder", t, func() { - property := specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeString, - IgnoreItemsOrder: true, - } - remoteValue := "someString" - Convey("When processIgnoreOrderIfEnabled", func() { - output := processIgnoreOrderIfEnabled(property, nil, remoteValue) - Convey("Then the output should be the string from remote", func() { - So(output, ShouldResemble, remoteValue) - }) - }) - }) -} - -func TestProcessIgnoreOrderIfEnabledIgnoreOrderDisabled(t *testing.T) { - Convey("Given a a list of strings property definition (with ignore order set to false) and the corresponding input/remote lists", t, func() { - property := specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeString, - IgnoreItemsOrder: false, - } - inputPropertyValue := []interface{}{"inputVal1", "inputVal2", "inputVal3"} - remoteValue := []interface{}{"inputVal1", "inputVal2", "inputVal3"} - Convey("When processIgnoreOrderIfEnabled", func() { - output := processIgnoreOrderIfEnabled(property, inputPropertyValue, remoteValue) - Convey("Then the output should match the remote list", func() { - So(output, ShouldResemble, []interface{}{"inputVal1", "inputVal2", "inputVal3"}) - }) - }) - }) -} - -func TestCompareInputPropertyValueWithPayloadPropertyValueBools(t *testing.T) { - Convey("Given a a list of bools property definition and the corresponding input/remote lists", t, func() { - property := specSchemaDefinitionProperty{ - Name: "list_prop", - Type: typeList, - ArrayItemsType: typeBool, - IgnoreItemsOrder: true, - Required: true, - } - inputPropertyValue := []interface{}{true} - remoteValue := []interface{}{false} - Convey("When processIgnoreOrderIfEnabled", func() { - output := processIgnoreOrderIfEnabled(property, inputPropertyValue, remoteValue) - Convey("Then the output should match the remote list", func() { - So(output, ShouldResemble, []interface{}{false}) - }) - }) - }) -} From 8110f81863ecae4fbfdab77e07e51683efa765cf Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 17:48:42 -0700 Subject: [PATCH 36/37] add x-terraform-ignore-order documentation --- docs/how_to.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/docs/how_to.md b/docs/how_to.md index 05492510f..a4123217b 100644 --- a/docs/how_to.md +++ b/docs/how_to.md @@ -1033,8 +1033,35 @@ x-terraform-sensitive | boolean | If this meta attribute is present in a definit x-terraform-id | boolean | If this meta attribute is present in an object definition property, the value will be used as the resource identifier when performing the read, update and delete API operations. The value will also be stored in the ID field of the local state file. x-terraform-field-name | string | This enables service providers to override the schema definition property name with a different one which will be the property name used in the terraform configuration file. This is mostly used to expose the internal property to a more user friendly name. If the extension is not present and the property name is not terraform compliant (following snake_case), an automatic conversion will be performed by the OpenAPI Terraform provider to make the name compliant (following Terraform's field name convention to be snake_case) x-terraform-field-status | boolean | If this meta attribute is present in a definition property, the value will be used as the status identifier when executing the polling mechanism on eligible async operations such as POST/PUT/DELETE. +[x-terraform-ignore-order](#xTerraformIgnoreOrder) | boolean | If this meta attribute is present in a definition property of type list, when the plugin is updating the state for the property it will inspect the items of the list received from remote and compare with the local values and if the lists are the same but unordered the state will keep the users input. Please go to the `x-terraform-ignore-order` section to learn more about the different behaviours supported. [x-terraform-complex-object-legacy-config](#xTerraformComplexObjectLegacyConfig) | boolean | If this meta attribute is present in an definition property of type object with value set to true, the OpenAPI terraform plugin will configure the corresponding property schema in Terraform following [Hashi maintainers recommendation](https://github.com/hashicorp/terraform/issues/22511#issuecomment-522655851) using as Schema Type schema.TypeList and limiting the max items in the list to 1 (MaxItems = 1). +###### x-terraform-ignore-order + +This extension enables the service providers to setup the 'ignore order' behaviour for a property of type list defined in +the object definition. For instance, the API may be returning the array items in lexical order but that behaviour might +not be the desired one for the terraform plugin since it would cause DIFFs for users that provided the values of the array +property in a different order. Hence, ensuring as much as possible that the order for the elements in the input list +provided by the user is maintained. + +Given the following terraform snippet where the members values are in certain desired order and assuming that the members +property is of type 'list' AND has the `x-terraform-ignore-order` extension set to true in the OpenAPI document for the `group_v1` +resource definition: + +```` +resource "openapi_group_v1" "my_iam_group_v1" { + members = ["user1", "user2", "user3"] +} +```` + +The following behaviour is applied depending on the different scenarios when processing the response received by the API +and saving the state of the property. + +- Use case 0: If the remote value for the property `members` contained the same items in the same order (eg: `{"members":["user1", "user2", "user3"]}`) as the tf input then the state saved for the property would match the input values. That is: ``members = ["user1", "user2", "user3"]`` +- Use case 1: If the remote value for the property `members` contained the same items as the tf input BUT the order of the elements is different (eg: `{"members":["user3", "user2", "user1"]}`) then state saved for the property would match the input values. That is: ``members = ["user1", "user2", "user3"]`` +- Use case 2: If the remote value for the property `members` contained the same items as the tf input in different order PLUS new ones (eg: `{"members":["user2", "user1", "user3", "user4"]}`) then state saved for the property would match the input values and also add to the end of the list the new elements received from the API. That is: ``members = ["user1", "user2", "user3", "user4"]`` +- Use case 3: If the remote value for the property `members` contained a shorter list than items in the tf input (eg: `{"members":["user3", "user1"}`) then state saved for the property would contain only the matching elements between the input and remote. That is: ``members = ["user1", "user3"]`` +- Use case 4: If the remote value for the property `members` contained the same list size as the items in the tf input but some elements inside where updated (eg: `{"members":["user1", "user5", "user9"]}`) then state saved for the property would contain the matching elements between the input and output and also keep the remote values. That is: ``members = ["user1", "user5", "user9"]`` ###### x-terraform-complex-object-legacy-config From ace46ffcfa4fbf2f0a6e4552f2d77171b81521eb Mon Sep 17 00:00:00 2001 From: dikhan Date: Wed, 13 May 2020 17:48:51 -0700 Subject: [PATCH 37/37] go mod dep updates --- go.mod | 4 ++-- go.sum | 4 ++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index bc07fb415..61fbe2617 100644 --- a/go.mod +++ b/go.mod @@ -45,10 +45,10 @@ require ( github.com/zach-klippenstein/goregen v0.0.0-20160303162051-795b5e3961ea // indirect golang.org/x/crypto v0.0.0-20200510223506-06a226fb4e37 // indirect golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect - golang.org/x/net v0.0.0-20200506145744-7e3656a0809f // indirect + golang.org/x/net v0.0.0-20200513185701-a91f0712d120 // indirect golang.org/x/sync v0.0.0-20200317015054-43a5402ce75a // indirect golang.org/x/sys v0.0.0-20200513112337-417ce2331b5c // indirect - golang.org/x/tools v0.0.0-20200513175351-0951661448da // indirect + golang.org/x/tools v0.0.0-20200513201620-d5fe73897c97 // indirect gopkg.in/mgo.v2 v2.0.0-20160818020120-3f83fa500528 // indirect gopkg.in/yaml.v2 v2.2.2 ) diff --git a/go.sum b/go.sum index 31d8ab990..d2b7683fe 100644 --- a/go.sum +++ b/go.sum @@ -356,6 +356,8 @@ golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e h1:3G+cUijn7XD+S4eJFddp53Pv7 golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A= golang.org/x/net v0.0.0-20200506145744-7e3656a0809f h1:QBjCr1Fz5kw158VqdE9JfI9cJnl/ymnJWAdMuinqL7Y= golang.org/x/net v0.0.0-20200506145744-7e3656a0809f/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A= +golang.org/x/net v0.0.0-20200513185701-a91f0712d120 h1:EZ3cVSzKOlJxAd8e8YAJ7no8nNypTxexh/YE/xW3ZEY= +golang.org/x/net v0.0.0-20200513185701-a91f0712d120/go.mod h1:qpuaurCH72eLCgpAm/N6yyVIVM9cpaDIP3A8BGJEC5A= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421 h1:Wo7BWFiOk0QRFMLYMqJGFMd9CgUAcGx7V+qEg/h5IBI= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -417,6 +419,8 @@ golang.org/x/tools v0.0.0-20200513122804-866d71a3170a h1:LkBZllNM46txyXU+/e601XX golang.org/x/tools v0.0.0-20200513122804-866d71a3170a/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200513175351-0951661448da h1:ZR1ivkcQoKXKtux9Rx3Em7iiSViMxQ5suNd5PZMUkPc= golang.org/x/tools v0.0.0-20200513175351-0951661448da/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= +golang.org/x/tools v0.0.0-20200513201620-d5fe73897c97 h1:DAuln/hGp+aJiHpID1Y1hYzMEPP5WLwtZHPb50mN0OE= +golang.org/x/tools v0.0.0-20200513201620-d5fe73897c97/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=