Merge pull request #261 from dikhan/feature/cache-improvements

[TechDebt: Issue #260] Plugin execution improvements

openapi/common.go

@@ -121,7 +121,7 @@ func updateStateWithPayloadDataAndOptions(openAPIResource SpecResource, remoteDa
 			return err
 		}
 		if value != nil {
-			if err := setResourceDataProperty(openAPIResource, propertyName, value, resourceLocalData); err != nil {
+			if err := setResourceDataProperty(*property, value, resourceLocalData); err != nil {
 				return err
 			}
 		}
@@ -268,12 +268,7 @@ func convertPayloadToLocalStateDataValue(property *SpecSchemaDefinitionProperty,
 }
 
 // setResourceDataProperty sets the expectedValue for the given schemaDefinitionPropertyName using the terraform compliant property name
-func setResourceDataProperty(openAPIResource SpecResource, schemaDefinitionPropertyName string, value interface{}, resourceLocalData *schema.ResourceData) error {
-	resourceSchema, _ := openAPIResource.GetResourceSchema()
-	schemaDefinitionProperty, err := resourceSchema.getProperty(schemaDefinitionPropertyName)
-	if err != nil {
-		return fmt.Errorf("could not find schema definition property name %s in the resource data: %s", schemaDefinitionPropertyName, err)
-	}
+func setResourceDataProperty(schemaDefinitionProperty SpecSchemaDefinitionProperty, value interface{}, resourceLocalData *schema.ResourceData) error {
 	return resourceLocalData.Set(schemaDefinitionProperty.GetTerraformCompliantPropertyName(), value)
 }
 

openapi/common_test.go

@@ -847,11 +847,11 @@ func TestConvertPayloadToLocalStateDataValue(t *testing.T) {
 }
 
 func TestSetResourceDataProperty(t *testing.T) {
-	Convey("Given a resource factory initialized with a spec resource with some schema definition", t, func() {
-		r, resourceData := testCreateResourceFactory(t, stringProperty, stringWithPreferredNameProperty)
-		Convey("When setResourceDataProperty is called with a schema definition property name that exists in terraform resource data object and with a new expectedValue", func() {
+	Convey("Given a resource data (state) loaded with couple propeprties", t, func() {
+		_, resourceData := testCreateResourceFactory(t, stringProperty, stringWithPreferredNameProperty)
+		Convey("When setResourceDataProperty is called with a schema definition property that exists in terraform resource data object and with a new expectedValue", func() {
 			expectedValue := "newValue"
-			err := setResourceDataProperty(r.openAPIResource, stringProperty.Name, expectedValue, resourceData)
+			err := setResourceDataProperty(*stringProperty, expectedValue, resourceData)
 			Convey("Then the result returned should be the expected one", func() {
 				So(err, ShouldBeNil)
 				// keys stores in the resource data struct are always snake case
@@ -860,7 +860,7 @@ func TestSetResourceDataProperty(t *testing.T) {
 		})
 		Convey("When setResourceDataProperty is called with a schema definition property preferred name that exists in terraform resource data object and with a new expectedValue", func() {
 			expectedValue := "theNewValue"
-			err := setResourceDataProperty(r.openAPIResource, stringWithPreferredNameProperty.Name, expectedValue, resourceData)
+			err := setResourceDataProperty(*stringWithPreferredNameProperty, expectedValue, resourceData)
 			Convey("Then the result returned should be the expected one", func() {
 				So(err, ShouldBeNil)
 				// keys stores in the resource data struct are always snake case
@@ -869,11 +869,11 @@ func TestSetResourceDataProperty(t *testing.T) {
 			})
 		})
 		Convey("When setResourceDataProperty is called with a schema definition property name does NOT exist", func() {
-			err := setResourceDataProperty(r.openAPIResource, "nonExistingKey", "", resourceData)
+			err := setResourceDataProperty(SpecSchemaDefinitionProperty{Name: "nonExistingKey"}, "", resourceData)
 			Convey("Then the result returned should be the expected one", func() {
 				So(err, ShouldNotBeNil)
 				// keys stores in the resource data struct are always snake case
-				So(err.Error(), ShouldEqual, "could not find schema definition property name nonExistingKey in the resource data: property with name 'nonExistingKey' not existing in resource schema definition")
+				So(err.Error(), ShouldEqual, `Invalid address to set: []string{"non_existing_key"}`)
 			})
 		})
 	})

openapi/openapi_spec_resource.go

@@ -14,7 +14,7 @@ type SpecResource interface {
 	getResourceOperations() specResourceOperations
 	getTimeouts() (*specTimeouts, error)
 	// GetParentResourceInfo returns a struct populated with relevant ParentResourceInfo if the resource is considered
-	// a subresource; nil otherwise.
+	// a sub-resource; nil otherwise.
 	GetParentResourceInfo() *ParentResourceInfo
 }
 

openapi/openapi_v2_resource.go

@@ -87,6 +87,16 @@ type SpecV2Resource struct {
 	SchemaDefinitions map[string]spec.Schema
 
 	Paths map[string]spec.PathItem
+
+	// Cached objects that are loaded once (when the corresponding function that loads the object is called the first time) and
+	// on subsequent method calls the cached object is returned instead saving executing time.
+
+	// specSchemaDefinitionCached is cached in GetResourceSchema() method
+	specSchemaDefinitionCached *SpecSchemaDefinition
+	// parentResourceInfoCached is cached in GetParentResourceInfo() method
+	parentResourceInfoCached *ParentResourceInfo
+	// resolvedPathCached is cached in getResourcePath() method
+	resolvedPathCached string
 }
 
 // newSpecV2Resource creates a SpecV2Resource with no region and default host
@@ -211,13 +221,19 @@ func (o *SpecV2Resource) buildResourceNameFromPath(resourcePath, preferredName s
 // resource path "/v1/cdns/{cdn_id}/v1/firewalls" and the []strin{"cdnID"} the returned path will be "/v1/cdns/cdnID/v1/firewalls".
 // If the resource path is not parameterised, then regular path will be returned accordingly
 func (o *SpecV2Resource) getResourcePath(parentIDs []string) (string, error) {
+	if o.resolvedPathCached != "" {
+		log.Printf("[DEBUG] getResourcePath hit the cache for '%s'", o.Name)
+		return o.resolvedPathCached, nil
+	}
 	resolvedPath := o.Path
 
 	pathParameterRegex, _ := regexp.Compile(pathParameterRegex)
 	pathParamsMatches := pathParameterRegex.FindAllStringSubmatch(resolvedPath, -1)
 
 	switch {
 	case len(pathParamsMatches) == 0:
+		o.resolvedPathCached = resolvedPath
+		log.Printf("[DEBUG] getResourcePath cache loaded for '%s'", o.Name)
 		return resolvedPath, nil
 
 	case len(parentIDs) > len(pathParamsMatches):
@@ -235,6 +251,8 @@ func (o *SpecV2Resource) getResourcePath(parentIDs []string) (string, error) {
 		resolvedPath = strings.Replace(resolvedPath, pathParamsMatches[idx][1], parentIDs[idx], 1)
 	}
 
+	o.resolvedPathCached = resolvedPath
+	log.Printf("[DEBUG] getResourcePath cache loaded for '%s'", o.Name)
 	return resolvedPath, nil
 }
 
@@ -282,6 +300,10 @@ func (o *SpecV2Resource) ShouldIgnoreResource() bool {
 
 // GetParentResourceInfo returns the information about the parent resources
 func (o *SpecV2Resource) GetParentResourceInfo() *ParentResourceInfo {
+	if o.parentResourceInfoCached != nil {
+		log.Printf("[DEBUG] GetParentResourceInfo hit the cache for '%s'", o.Name)
+		return o.parentResourceInfoCached
+	}
 	resourceParentRegex, _ := regexp.Compile(resourceParentNameRegex)
 	parentMatches := resourceParentRegex.FindAllStringSubmatch(o.Path, -1)
 	if len(parentMatches) > 0 {
@@ -328,14 +350,26 @@ func (o *SpecV2Resource) GetParentResourceInfo() *ParentResourceInfo {
 			parentURIs:             parentURIs,
 			parentInstanceURIs:     parentInstanceURIs,
 		}
+		o.parentResourceInfoCached = sub
+		log.Printf("[DEBUG] GetParentResourceInfo cache loaded for '%s'", o.Name)
 		return sub
 	}
 	return nil
 }
 
 // GetResourceSchema returns the resource schema
 func (o *SpecV2Resource) GetResourceSchema() (*SpecSchemaDefinition, error) {
-	return o.getSchemaDefinitionWithOptions(&o.SchemaDefinition, true)
+	if o.specSchemaDefinitionCached != nil {
+		log.Printf("[DEBUG] GetResourceSchema hit the cache for '%s'", o.Name)
+		return o.specSchemaDefinitionCached, nil
+	}
+	specSchemaDefinition, err := o.getSchemaDefinitionWithOptions(&o.SchemaDefinition, true)
+	if err != nil {
+		return nil, err
+	}
+	o.specSchemaDefinitionCached = specSchemaDefinition
+	log.Printf("[DEBUG] GetResourceSchema cache loaded for '%s'", o.Name)
+	return o.specSchemaDefinitionCached, nil
 }
 
 func (o *SpecV2Resource) getSchemaDefinition(schema *spec.Schema) (*SpecSchemaDefinition, error) {
@@ -379,6 +413,12 @@ func (o *SpecV2Resource) getSchemaDefinitionWithOptions(schema *spec.Schema, add
 func (o *SpecV2Resource) createSchemaDefinitionProperty(propertyName string, property spec.Schema, requiredProperties []string) (*SpecSchemaDefinitionProperty, error) {
 	schemaDefinitionProperty := &SpecSchemaDefinitionProperty{}
 
+	schemaDefinitionProperty.Name = propertyName
+	propertyType, err := o.getPropertyType(property)
+	if err != nil {
+		return nil, fmt.Errorf("failed to process property '%s': %s", propertyName, err)
+	}
+	schemaDefinitionProperty.Type = propertyType
 	schemaDefinitionProperty.Description = property.Description
 
 	if isObject, schemaDefinition, err := o.isObjectProperty(property); isObject || err != nil {
@@ -418,14 +458,6 @@ func (o *SpecV2Resource) createSchemaDefinitionProperty(propertyName string, pro
 		log.Printf("[DEBUG] found array type property '%s' with items of type '%s'", propertyName, itemsType)
 	}
 
-	propertyType, err := o.getPropertyType(property)
-	if err != nil {
-		return nil, err
-	}
-	schemaDefinitionProperty.Type = propertyType
-
-	schemaDefinitionProperty.Name = propertyName
-
 	if preferredPropertyName, exists := property.Extensions.GetString(extTfFieldName); exists {
 		schemaDefinitionProperty.PreferredName = preferredPropertyName
 	}
@@ -619,7 +651,7 @@ func (o *SpecV2Resource) isObjectProperty(property spec.Schema) (bool, *spec.Sch
 			}
 			return true, schema, nil
 		}
-		return true, nil, fmt.Errorf("object is missing the nested schema definition or the ref is poitning to a non existing schema definition")
+		return true, nil, fmt.Errorf("object is missing the nested schema definition or the ref is pointing to a non existing schema definition")
 	}
 	return false, nil, nil
 }

openapi/openapi_v2_resource_test.go

@@ -727,6 +727,7 @@ func TestParentResourceInfo(t *testing.T) {
 			parentResourceInfo := r.GetParentResourceInfo()
 			Convey("Then the result returned should be the expected one", func() {
 				So(parentResourceInfo, ShouldNotBeNil)
+				So(parentResourceInfo, ShouldPointTo, r.parentResourceInfoCached) // checking cache is populated
 				// the parentResourceNames should not be empty and contain the right items
 				So(len(parentResourceInfo.parentResourceNames), ShouldEqual, 1)
 				So(parentResourceInfo.parentResourceNames[0], ShouldEqual, "cdns_v1")
@@ -1136,7 +1137,15 @@ func TestParentResourceInfo(t *testing.T) {
 			})
 		})
 	})
-
+	Convey("Given a SpecV2Resource with parentResourceInfoCached populated", t, func() {
+		r := SpecV2Resource{parentResourceInfoCached: &ParentResourceInfo{}}
+		Convey("When GetParentResourceInfo is called", func() {
+			p := r.GetParentResourceInfo()
+			Convey("Then the returned ParentResourceInfo should point to parentResourceInfoCached", func() {
+				So(p, ShouldPointTo, r.parentResourceInfoCached)
+			})
+		})
+	})
 }
 
 func assertSchemaProperty(actualSpecSchemaDefinition *SpecSchemaDefinition, expectedName string, expectedType schemaDefinitionPropertyType, expectedRequired, expectedReadOnly, expectedComputed bool) {
@@ -1198,6 +1207,7 @@ func TestGetResourceSchema(t *testing.T) {
 			Convey("And the SpecSchemaDefinition returned should be configured as expected", func() {
 				So(err, ShouldBeNil)
 				So(len(specSchemaDefinition.Properties), ShouldEqual, len(r.SchemaDefinition.SchemaProps.Properties))
+				So(specSchemaDefinition, ShouldPointTo, r.specSchemaDefinitionCached) // checking cache is populated
 				assertSchemaProperty(specSchemaDefinition, "string_readonly_prop", TypeString, false, true, true)
 				assertSchemaProperty(specSchemaDefinition, "int_optional_computed_prop", TypeInt, false, false, true)
 				assertSchemaProperty(specSchemaDefinition, "number_required_prop", TypeFloat, true, false, false)
@@ -1440,6 +1450,42 @@ func TestGetResourceSchema(t *testing.T) {
 			})
 		})
 	})
+
+	Convey("Given a SpecV2Resource configured with a miss configured schema (eg: schema contains a property that is missing the type", t, func() {
+		r := &SpecV2Resource{
+			SchemaDefinition: spec.Schema{
+				SchemaProps: spec.SchemaProps{
+					Properties: map[string]spec.Schema{
+						"bad_property": {
+							SchemaProps: spec.SchemaProps{
+								// Type: Missing the type
+							},
+						},
+					},
+				},
+			},
+		}
+		Convey("When GetResourceSchema is called", func() {
+			specSchemaDefinition, err := r.GetResourceSchema()
+			Convey("Then the schema definition returned should nil and the error should be the expected one", func() {
+				So(err.Error(), ShouldEqual, "failed to process property 'bad_property': non supported '[]' type")
+				So(specSchemaDefinition, ShouldBeNil)
+			})
+		})
+	})
+
+	Convey("Given a SpecV2Resource containing a cached specSchemaDefinitionCached", t, func() {
+		r := &SpecV2Resource{
+			specSchemaDefinitionCached: &SpecSchemaDefinition{},
+		}
+		Convey("When GetResourceSchema is called", func() {
+			specSchemaDefinition, err := r.GetResourceSchema()
+			Convey("Then the schema definition returned should be the cached one and the error should be nil", func() {
+				So(err, ShouldBeNil)
+				So(specSchemaDefinition, ShouldPointTo, r.specSchemaDefinitionCached)
+			})
+		})
+	})
 }
 
 func TestGetSchemaDefinition(t *testing.T) {
@@ -1668,13 +1714,15 @@ func TestGetResourcePath(t *testing.T) {
 			Convey("Then the returned resource path should match the expected one", func() {
 				So(err, ShouldBeNil)
 				So(resourcePath, ShouldEqual, "/v1/cdns")
+				So(r.resolvedPathCached, ShouldEqual, "/v1/cdns")
 			})
 		})
 		Convey("When getResourcePath is called with a nil list of IDs", func() {
 			resourcePath, err := r.getResourcePath(nil)
 			Convey("Then the returned resource path should match the expected one", func() {
 				So(err, ShouldBeNil)
 				So(resourcePath, ShouldEqual, "/v1/cdns")
+				So(r.resolvedPathCached, ShouldEqual, "/v1/cdns")
 			})
 		})
 	})
@@ -1689,29 +1737,38 @@ func TestGetResourcePath(t *testing.T) {
 			Convey("Then the returned resource path should match the expected one", func() {
 				So(err, ShouldBeNil)
 				So(resourcePath, ShouldEqual, "/v1/cdns/parentID/v1/firewalls")
+				So(r.resolvedPathCached, ShouldEqual, "/v1/cdns/parentID/v1/firewalls")
 			})
 		})
 		Convey("When getResourcePath is called with an empty list of IDs", func() {
-			_, err := r.getResourcePath([]string{})
+			resourcePath, err := r.getResourcePath([]string{})
 			Convey("Then the error returned should not be nil", func() {
+				So(resourcePath, ShouldBeEmpty)
+				So(r.resolvedPathCached, ShouldBeEmpty)
 				So(err.Error(), ShouldEqual, "could not resolve sub-resource path correctly '/v1/cdns/{cdn_id}/v1/firewalls' with the given ids - missing ids to resolve the path params properly: []")
 			})
 		})
 		Convey("When getResourcePath is called with an nil list of IDs", func() {
-			_, err := r.getResourcePath(nil)
+			resourcePath, err := r.getResourcePath(nil)
 			Convey("Then the error returned should not be nil", func() {
+				So(resourcePath, ShouldBeEmpty)
+				So(r.resolvedPathCached, ShouldBeEmpty)
 				So(err.Error(), ShouldEqual, "could not resolve sub-resource path correctly '/v1/cdns/{cdn_id}/v1/firewalls' with the given ids - missing ids to resolve the path params properly: []")
 			})
 		})
 		Convey("When getResourcePath is called with a list of IDs that is bigger than the parameterised params in the path", func() {
-			_, err := r.getResourcePath([]string{"cdnID", "somethingThatDoesNotBelongHere"})
+			resourcePath, err := r.getResourcePath([]string{"cdnID", "somethingThatDoesNotBelongHere"})
 			Convey("Then the error returned should not be nil", func() {
+				So(resourcePath, ShouldBeEmpty)
+				So(r.resolvedPathCached, ShouldBeEmpty)
 				So(err.Error(), ShouldEqual, "could not resolve sub-resource path correctly '/v1/cdns/{cdn_id}/v1/firewalls' with the given ids - more ids than path params: [cdnID somethingThatDoesNotBelongHere]")
 			})
 		})
 		Convey("When getResourcePath is called with a list of IDs twhere some IDs contain forward slashes", func() {
-			_, err := r.getResourcePath([]string{"cdnID/somethingElse"})
+			resourcePath, err := r.getResourcePath([]string{"cdnID/somethingElse"})
 			Convey("Then the error returned should not be nil", func() {
+				So(resourcePath, ShouldBeEmpty)
+				So(r.resolvedPathCached, ShouldBeEmpty)
 				So(err.Error(), ShouldEqual, "could not resolve sub-resource path correctly '/v1/cdns/{cdn_id}/v1/firewalls' due to parent IDs ([cdnID/somethingElse]) containing not supported characters (forward slashes)")
 			})
 		})
@@ -1727,6 +1784,20 @@ func TestGetResourcePath(t *testing.T) {
 			Convey("And the returned resource path should match the expected one", func() {
 				So(err, ShouldBeNil)
 				So(resourcePath, ShouldEqual, "/v1/cdns/cdnID/v1/firewalls/fwID/rules")
+				So(r.resolvedPathCached, ShouldEqual, "/v1/cdns/cdnID/v1/firewalls/fwID/rules")
+			})
+		})
+	})
+
+	Convey("Given a SpecV2Resource with resolvedPathCached populated", t, func() {
+		r := SpecV2Resource{
+			resolvedPathCached: "/v1/cdns",
+		}
+		Convey("When getResourcePath is called with a nil list of IDs", func() {
+			resourcePath, err := r.getResourcePath(nil)
+			Convey("Then the returned resource path should match the expected one", func() {
+				So(err, ShouldBeNil)
+				So(resourcePath, ShouldEqual, "/v1/cdns")
 			})
 		})
 	})
@@ -1824,7 +1895,7 @@ func TestCreateSchemaDefinitionProperty(t *testing.T) {
 			schemaDefinitionProperty, err := r.createSchemaDefinitionProperty(propertyName, propertySchema, requiredProperties)
 			Convey("Then the error returned should be the expected one and the schemaDefinitionProperty should be nil", func() {
 				So(err, ShouldNotBeNil)
-				So(err.Error(), ShouldEqual, "non supported '[]' type")
+				So(err.Error(), ShouldEqual, "failed to process property 'propertyName': non supported '[]' type")
 				So(schemaDefinitionProperty, ShouldBeNil)
 			})
 		})
@@ -1866,8 +1937,7 @@ func TestCreateSchemaDefinitionProperty(t *testing.T) {
 			requiredProperties := []string{}
 			schemaDefinitionProperty, err := r.createSchemaDefinitionProperty(propertyName, propertySchema, requiredProperties)
 			Convey("Then the error returned should be the expected one and the schemaDefinitionProperty should be nil", func() {
-				So(err, ShouldNotBeNil)
-				So(err.Error(), ShouldEqual, "failed to process object type property 'propertyName': object is missing the nested schema definition or the ref is poitning to a non existing schema definition")
+				So(err.Error(), ShouldEqual, "failed to process property 'propertyName': object is missing the nested schema definition or the ref is pointing to a non existing schema definition")
 				So(schemaDefinitionProperty, ShouldBeNil)
 			})
 		})

openapi/resource_factory.go

@@ -510,7 +510,7 @@ func (r resourceFactory) createPayloadFromLocalStateData(resourceLocalData *sche
 			continue
 		}
 		if !property.IsParentProperty {
-			if dataValue, ok := r.getResourceDataOKExists(propertyName, resourceLocalData); ok {
+			if dataValue, ok := r.getResourceDataOKExists(*property, resourceLocalData); ok {
 				err := r.populatePayload(input, property, dataValue)
 				if err != nil {
 					log.Printf("[ERROR] [resource='%s'] error when creating the property payload for property '%s': %s", r.openAPIResource.GetResourceName(), propertyName, err)
@@ -646,11 +646,6 @@ func (r resourceFactory) getStatusValueFromPayload(payload map[string]interface{
 }
 
 // getResourceDataOK returns the data for the given schemaDefinitionPropertyName using the terraform compliant property name
-func (r resourceFactory) getResourceDataOKExists(schemaDefinitionPropertyName string, resourceLocalData *schema.ResourceData) (interface{}, bool) {
-	resourceSchema, _ := r.openAPIResource.GetResourceSchema()
-	schemaDefinitionProperty, err := resourceSchema.getProperty(schemaDefinitionPropertyName)
-	if err != nil {
-		return nil, false
-	}
+func (r resourceFactory) getResourceDataOKExists(schemaDefinitionProperty SpecSchemaDefinitionProperty, resourceLocalData *schema.ResourceData) (interface{}, bool) {
 	return resourceLocalData.GetOkExists(schemaDefinitionProperty.GetTerraformCompliantPropertyName())
 }