diff --git a/openapi/common.go b/openapi/common.go index 4b98b68f6..962fca52b 100644 --- a/openapi/common.go +++ b/openapi/common.go @@ -168,7 +168,7 @@ func processIgnoreOrderIfEnabled(property SpecSchemaDefinitionProperty, inputPro for _, inputItemValue := range inputValueArray { for _, remoteItemValue := range remoteValueArray { if property.equalItems(property.ArrayItemsType, inputItemValue, remoteItemValue) { - newPropertyValue = append(newPropertyValue, inputItemValue) + newPropertyValue = append(newPropertyValue, remoteItemValue) break } } diff --git a/openapi/openapi_spec_resource_schema_definition_property.go b/openapi/openapi_spec_resource_schema_definition_property.go index 57d8c32eb..5695573c7 100644 --- a/openapi/openapi_spec_resource_schema_definition_property.go +++ b/openapi/openapi_spec_resource_schema_definition_property.go @@ -328,20 +328,27 @@ func (s *SpecSchemaDefinitionProperty) equal(item1, item2 interface{}) bool { } func (s *SpecSchemaDefinitionProperty) equalItems(itemsType schemaDefinitionPropertyType, item1, item2 interface{}) bool { + var defaultValueForType interface{} = nil switch itemsType { case TypeString: + defaultValueForType = "" if !s.validateValueType(item1, reflect.String) || !s.validateValueType(item2, reflect.String) { return false } case TypeInt: + item1 = terraformutils.CastToIntegerIfFloat(item1) // deal with API responses mapping all numbers to float64 + item2 = terraformutils.CastToIntegerIfFloat(item2) + defaultValueForType = 0 if !s.validateValueType(item1, reflect.Int) || !s.validateValueType(item2, reflect.Int) { return false } case TypeFloat: + defaultValueForType = 0.0 if !s.validateValueType(item1, reflect.Float64) || !s.validateValueType(item2, reflect.Float64) { return false } case TypeBool: + defaultValueForType = false if !s.validateValueType(item1, reflect.Bool) || !s.validateValueType(item2, reflect.Bool) { return false } @@ -349,8 +356,16 @@ func (s *SpecSchemaDefinitionProperty) equalItems(itemsType schemaDefinitionProp if !s.validateValueType(item1, reflect.Slice) || !s.validateValueType(item2, reflect.Slice) { return false } + if item1 == nil || item2 == nil { + return s.isOptional() + } list1 := item1.([]interface{}) list2 := item2.([]interface{}) + + if s.isOptional() && (len(list1) == 0 || len(list2) == 0) { + return true + } + if len(list1) != len(list2) { return false } @@ -373,14 +388,16 @@ func (s *SpecSchemaDefinitionProperty) equalItems(itemsType schemaDefinitionProp return s.equalItems(s.ArrayItemsType, list1[idx], list2[idx]) } case TypeObject: + item1, _ = terraformutils.CastTerraformSliceToMap(item1) // deal with terraform schema returning nested objects as slices + item2, _ = terraformutils.CastTerraformSliceToMap(item2) 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] + objectPropertyValue1 := objectProperty.getPropertyValueFromMap(object1) + objectPropertyValue2 := objectProperty.getPropertyValueFromMap(object2) if !objectProperty.equal(objectPropertyValue1, objectPropertyValue2) { return false } @@ -389,12 +406,31 @@ func (s *SpecSchemaDefinitionProperty) equalItems(itemsType schemaDefinitionProp default: return false } + + if s.isOptional() { + if item1 == nil || item2 == nil { // deals with cases where optional properties aren't returned by the API but Terraform sets a default value for them + return true + } + if item1 == defaultValueForType || item2 == defaultValueForType { // deals with cases where optional properties are returned by the API, but these properties are not defined in the tf config and Terraform gives sets them to default values + return true + } + } return item1 == item2 } func (s *SpecSchemaDefinitionProperty) validateValueType(item interface{}, expectedKind reflect.Kind) bool { + if item == nil { + return true // API responses can skip optional properties, resulting in nils, which are technically valid for the given type + } if reflect.TypeOf(item).Kind() != expectedKind { return false } return true } + +func (s *SpecSchemaDefinitionProperty) getPropertyValueFromMap(mapItem map[string]interface{}) interface{} { + if mapItem[s.Name] != nil { + return mapItem[s.Name] + } + return mapItem[s.GetTerraformCompliantPropertyName()] +} diff --git a/openapi/terraformutils/terraform_utils.go b/openapi/terraformutils/terraform_utils.go index 06b0032f5..d4b848efc 100644 --- a/openapi/terraformutils/terraform_utils.go +++ b/openapi/terraformutils/terraform_utils.go @@ -158,3 +158,42 @@ func MultiEnvDefaultString(ks []string, defaultValue interface{}) (string, error } return dv.(string), nil } + +// CastTerraformSliceToMap tries to cast the provided input to a map and returns an unaltered shallow +// copy of the input if it fails to cast. It exists to deal with the fact the Terraform defines nested +// objects as single element lists, and this method normalises the structure so that it can be +// compared against non-Terraform produced models. +func CastTerraformSliceToMap(item interface{}) (interface{}, bool) { + if item == nil { + return make(map[string]interface{}), true + } + sliceItem, successfulCast := item.([]interface{}) + if successfulCast { + if len(sliceItem) == 0 { + return make(map[string]interface{}), true + } + if len(sliceItem) == 1 { + mapItem, successfulCast := sliceItem[0].(map[string]interface{}) + if successfulCast { + return mapItem, true + } + } + } + return item, false +} + +// CastToIntegerIfFloat tries to cast the input to an integer if it can be represented as an integer. +// Failure to cast to an integer will return an unaltered shallow copy of the input. +func CastToIntegerIfFloat(item interface{}) interface{} { + if item == nil { + return 0 + } + var floatValue, successfulCast = item.(float64) + if successfulCast { + var integerValue = int(floatValue) + if item == float64(integerValue) { // check if float is really an integer + return integerValue + } + } + return item +} diff --git a/openapi/terraformutils/terraform_utils_test.go b/openapi/terraformutils/terraform_utils_test.go index 1ee17df43..15f1dae73 100644 --- a/openapi/terraformutils/terraform_utils_test.go +++ b/openapi/terraformutils/terraform_utils_test.go @@ -242,3 +242,68 @@ func TestEnvDefaultFunc(t *testing.T) { }) }) } + +func TestCastTerraformSliceToMap(t *testing.T) { + Convey("Given a nil input when CastTerraformSliceToMap is called then the result should be successfully cast an empty map", t, func() { + var input interface{} = nil + Convey("When CastTerraformSliceToMap is called", func() { + result, success := CastTerraformSliceToMap(input) + Convey("Then the result should be successfully cast", func() { + checkedResult, checkSuccess := result.(map[string]interface{}) + So(success, ShouldBeTrue) + So(checkSuccess, ShouldBeTrue) + Convey("And the result should be an empty map", func() { + So(len(checkedResult), ShouldEqual, 0) + }) + }) + }) + }) + + Convey("Given an empty slice input", t, func() { + var input []interface{} + Convey("When CastTerraformSliceToMap is called", func() { + result, success := CastTerraformSliceToMap(input) + Convey("Then the result should be successfully cast", func() { + checkedResult, checkSuccess := result.(map[string]interface{}) + So(success, ShouldBeTrue) + So(checkSuccess, ShouldBeTrue) + Convey("And the result should be an empty map", func() { + So(len(checkedResult), ShouldEqual, 0) + }) + }) + }) + }) + + Convey("Given a single element slice input", t, func() { + var input []interface{} + Convey("And the element is a map", func() { + mapItem := make(map[string]interface{}) + mapItem["some_property"] = "some_value" + input := append(input, mapItem) + Convey("When CastTerraformSliceToMap is called", func() { + result, success := CastTerraformSliceToMap(input) + Convey("Then the result should be successfully cast", func() { + checkedResult, checkSuccess := result.(map[string]interface{}) + So(success, ShouldBeTrue) + So(checkSuccess, ShouldBeTrue) + Convey("And the result should be the same map from the slice", func() { + So(checkedResult, ShouldEqual, mapItem) + }) + }) + }) + }) + + Convey("And the element is a non-map", func() { + input := append(input, "some_value", "other_value") + Convey("When CastTerraformSliceToMap is called", func() { + result, success := CastTerraformSliceToMap(input) + Convey("Then the result should not be successfully cast", func() { + So(success, ShouldBeFalse) + Convey("And the result should be the same as the input", func() { + So(len(result.([]interface{})), ShouldEqual, len(input)) + }) + }) + }) + }) + }) +} diff --git a/tests/e2e/data/gray_box_test_data/ignore_order/openapi.yaml b/tests/e2e/data/gray_box_test_data/ignore_order/openapi.yaml new file mode 100644 index 000000000..97fec0f83 --- /dev/null +++ b/tests/e2e/data/gray_box_test_data/ignore_order/openapi.yaml @@ -0,0 +1,104 @@ +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" + x-terraform-ignore-order: true + items: + type: "string" + integer_list_prop: + type: "array" + x-terraform-ignore-order: true + items: + type: "integer" + nestedListProp: + type: "array" + x-terraform-ignore-order: true + items: + type: "object" + properties: + someProperty: + type: "string" + x-terraform-force-new: true + otherPropertyStr: + type: "string" + otherPropertyInt: + type: "integer" + otherPropertyFloat: + type: "number" + otherPropertyBool: + type: "boolean" + otherPropertyList: + type: "array" + x-terraform-computed: true + items: + type: "string" + otherPropertyObject: + type: "object" + x-terraform-computed: true + properties: + deeplyNestedProperty: + type: "string" \ No newline at end of file diff --git a/tests/e2e/data/gray_box_test_data/ignore_order/test_stage_1.tf b/tests/e2e/data/gray_box_test_data/ignore_order/test_stage_1.tf new file mode 100644 index 000000000..a248d0538 --- /dev/null +++ b/tests/e2e/data/gray_box_test_data/ignore_order/test_stage_1.tf @@ -0,0 +1,34 @@ +# URI /v1/cdns/ +resource "openapi_cdn_v1" "my_cdn" { + label = "some label" + list_prop = ["value1", "value2", "value3"] + integer_list_prop = [1, 2, 3] + + nested_list_prop { + some_property = "value1" + other_property_str = "otherValue1" + other_property_int = 5 + other_property_float = 3.14 + other_property_bool = true + other_property_list = ["someValue1"] + other_property_object { + deeply_nested_property = "someDeeplyNestedValue1" + } + } + + nested_list_prop { + some_property = "value2" + other_property_str = "otherValue2" + other_property_int = 10 + other_property_float = 1.23 + other_property_bool = false + other_property_list = ["someValue2"] + other_property_object { + deeply_nested_property = "someDeeplyNestedValue2" + } + } + + nested_list_prop { + some_property = "value3" + } +} \ No newline at end of file diff --git a/tests/e2e/data/gray_box_test_data/ignore_order/test_stage_2.tf b/tests/e2e/data/gray_box_test_data/ignore_order/test_stage_2.tf new file mode 100644 index 000000000..04dc40b25 --- /dev/null +++ b/tests/e2e/data/gray_box_test_data/ignore_order/test_stage_2.tf @@ -0,0 +1,34 @@ +# URI /v1/cdns/ +resource "openapi_cdn_v1" "my_cdn" { + label = "some label" + list_prop = ["value1", "value3", "value2"] + integer_list_prop = [1, 3, 2] + + nested_list_prop { + some_property = "value1" + other_property_str = "otherValue1" + other_property_int = 5 + other_property_float = 3.14 + other_property_bool = true + other_property_list = ["someValue1"] + other_property_object { + deeply_nested_property = "someDeeplyNestedValue1" + } + } + + nested_list_prop { + some_property = "value3" + } + + nested_list_prop { + some_property = "value2" + other_property_str = "otherValue2" + other_property_int = 10 + other_property_float = 1.23 + other_property_bool = false + other_property_list = ["someValue2"] + other_property_object { + deeply_nested_property = "someDeeplyNestedValue2" + } + } +} \ No newline at end of file diff --git a/tests/e2e/gray_box_cdns_test.go b/tests/e2e/gray_box_cdns_test.go index 9add2430e..ccad9ab6b 100644 --- a/tests/e2e/gray_box_cdns_test.go +++ b/tests/e2e/gray_box_cdns_test.go @@ -427,85 +427,30 @@ func (a *api) apiResponse(t *testing.T, responseBody string, httpResponseStatusC } func TestAccCDN_CreateResourceWithIgnoreListOrderExtension(t *testing.T) { - swagger := `swagger: "2.0" -host: %s -schemes: -- "http" + swagger := getFileContents(t, "data/gray_box_test_data/ignore_order/openapi.yaml") -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" - x-terraform-ignore-order: true - items: - type: "string"` + resourceStateRemote := make([]byte, 0) apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodDelete { + resourceStateRemote = make([]byte, 0) w.WriteHeader(http.StatusNotFound) return } - body := `{"id": "someid", "label":"some label", "list_prop": ["value1", "value2", "value3"]}` + if r.Method == http.MethodPost { + body, err := ioutil.ReadAll(r.Body) + assert.Nil(t, err) + bodyJSON := map[string]interface{}{} + err = json.Unmarshal(body, &bodyJSON) + assert.Nil(t, err) + bodyJSON["id"] = "someid" + if len(resourceStateRemote) > 0 { + assert.Fail(t, "POST request triggered more than once where the resource is only expected to be created once.") + } + resourceStateRemote, err = json.Marshal(bodyJSON) + assert.Nil(t, err) + } w.WriteHeader(http.StatusOK) - w.Write([]byte(body)) + w.Write(resourceStateRemote) })) apiHost := apiServer.URL[7:] swaggerServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -513,38 +458,64 @@ definitions: w.Write([]byte(swaggerReturned)) })) - tfFileContents := `# URI /v1/cdns/ -resource "openapi_cdn_v1" "my_cdn" { - label = "some label" - list_prop = ["value3", "value1", "value2"] -}` + tfFileContentsStage1 := getFileContents(t, "data/gray_box_test_data/ignore_order/test_stage_1.tf") + tfFileContentsStage2 := getFileContents(t, "data/gray_box_test_data/ignore_order/test_stage_2.tf") p := openapi.ProviderOpenAPI{ProviderName: providerName} provider, err := p.CreateSchemaProviderFromServiceConfiguration(&openapi.ServiceConfigStub{SwaggerURL: swaggerServer.URL}) assert.NoError(t, err) resource.Test(t, resource.TestCase{ - IsUnitTest: true, - ProviderFactories: testAccProviders(provider), - PreCheck: func() { testAccPreCheck(t, swaggerServer.URL) }, + IsUnitTest: true, + ProviderFactories: testAccProviders(provider), + PreCheck: func() { testAccPreCheck(t, swaggerServer.URL) }, + PreventPostDestroyRefresh: true, Steps: []resource.TestStep{ { - ExpectNonEmptyPlan: false, - Config: tfFileContents, + Config: tfFileContentsStage1, Check: resource.ComposeTestCheckFunc( // check resource resource.TestCheckResourceAttr( openAPIResourceStateCDN, "label", "some label"), resource.TestCheckResourceAttr( - openAPIResourceStateCDN, "list_prop.#", "3"), + openAPIResourceStateCDN, "list_prop.0", "value1"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "list_prop.2", "value3"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "integer_list_prop.0", "1"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "integer_list_prop.2", "3"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "nested_list_prop.0.some_property", "value1"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "nested_list_prop.2.some_property", "value3"), + ), + }, + { + Config: tfFileContentsStage2, + Check: resource.ComposeTestCheckFunc( + // check resource resource.TestCheckResourceAttr( - openAPIResourceStateCDN, "list_prop.0", "value3"), + openAPIResourceStateCDN, "label", "some label"), resource.TestCheckResourceAttr( - openAPIResourceStateCDN, "list_prop.1", "value1"), + openAPIResourceStateCDN, "list_prop.0", "value1"), resource.TestCheckResourceAttr( openAPIResourceStateCDN, "list_prop.2", "value2"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "integer_list_prop.0", "1"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "integer_list_prop.2", "2"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "nested_list_prop.0.some_property", "value1"), + resource.TestCheckResourceAttr( + openAPIResourceStateCDN, "nested_list_prop.2.some_property", "value2"), ), }, + { + Config: tfFileContentsStage2, + PlanOnly: true, + ExpectNonEmptyPlan: false, + }, }, }) } diff --git a/tests/e2e/test_api_utils_test.go b/tests/e2e/test_api_utils_test.go index 79e69c22a..e856c0c36 100644 --- a/tests/e2e/test_api_utils_test.go +++ b/tests/e2e/test_api_utils_test.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/stretchr/testify/assert" + "io/ioutil" "net/http" "strings" "testing" @@ -104,3 +105,13 @@ func testAccPreCheck(t *testing.T, swaggerURLEndpoint string) { t.Fatalf("GET %s returned not expected response status code %d", swaggerURLEndpoint, res.StatusCode) } } + +func getFileContents(t *testing.T, filePath string) string { + return string(getFileContentsBytes(t, filePath)) +} + +func getFileContentsBytes(t *testing.T, filePath string) []byte { + fileContents, err := ioutil.ReadFile(filePath) + assert.Nil(t, err) + return fileContents +}