Merge pull request #110 from dikhan/feature/ref-first-level-expansion…

…-support

Feature/ref first level expansion support

Gopkg.toml

@@ -29,7 +29,7 @@
   name = "github.com/go-openapi/loads"
 
 [[constraint]]
-  branch = "master"
+  version = "v0.19.0"
   name = "github.com/go-openapi/spec"
 
 [[constraint]]

docs/how_to.md

@@ -215,20 +215,23 @@ as the service provider will have less paths to maintain overall. if there are m
 should not affect the way consumer interacts with the APIs whatsoever and the namespace should remain as is.
 
 - POST operation should have a body payload referencing a schema object (see example below) defined at the root level 
-[definitions](#swaggerDefinitions) section. Payload schema should not be defined inside the path’s configuration. This 
-is so the same definition can be shared across different operations for a given version (e,g: $ref: "#/definitions/resource) 
-and consistency in terms of data model for a given resource version is maintained throughout all the operations. 
-This helps keeping the swagger file well structured and encourages object definition re-usability. 
-Different end point versions should their own payload definitions as the example below, path ```/v1/resource``` has a corresponding
-```resourceV1``` definition object:
+[definitions](#swaggerDefinitions) section. The $ref can be a link to a local model definition or a definition hosted
+externally. Payload schema should not be defined inside the path’s configuration; however, if it is defined the schema
+must be the same as the GET and PUT operations, including the expected input properties as well as the computed ones.
+The reason for this is to make sure the model for the the resource state is shared across different operations (POST, GET, PUT)
+ensuring no diffs with terraform will happen at runtime due to inconsistency with properties. It is suggested to use the same
+definition shared across the resource operations for a given version (e,g: $ref: "#/definitions/resource) so consistency 
+in terms of data model for a given resource version is maintained throughout all the operations. This helps keeping the 
+swagger file well structured and encourages object definition re-usability. Different end point versions should their own 
+payload definitions as the example below, path ```/v1/resource``` has a corresponding ```resourceV1``` definition object:
 
 ````
   /v1/resource:
     post:
       - in: "body"
         name: "body"
         schema:
-          $ref: "#/definitions/resourceV1"    
+          $ref: "#/definitions/resourceV1" # this can be a link to an external definition hostead somewhere else (e.g: $ref:"http://another-host.com/#/definitions/ContentDeliveryNetwork")
 
 definitions:   
   resourceV1:     
@@ -246,10 +249,11 @@ definitions:  
 Refer to [readOnly](#attributeDetails) attributes to learn more about how to define an object that has computed properties 
 (value auto-generated by the API).
 
-- The schema object definition must be described on the root level [definitions](#swaggerDefinitions) section and must 
+- The schema object definition should be described on the root level [definitions](#swaggerDefinitions) section and must 
 not be embedded within the API definition. This is enforced to keep the swagger file well structured and to encourage
-object re-usability across the CRUD operations. Operations such as POST/GET/PUT are expected to have a 'schema' property
-with a link to the actual definition (e,g: `$ref: "#/definitions/resource`)
+object re-usability across the resource CRUD operations. Operations such as POST/GET/PUT are expected to have a 'schema' property
+with a link to the same definition (e,g: `$ref: "#/definitions/resource`). The ref can be a link to an external source
+as described in the [OpenAPI documentation for $ref](https://swagger.io/docs/specification/using-ref/).
 
 - The schema object must have a property that uniquely identifies the resource instance. This can be done by either
 having a computed property (readOnly) called ```id``` or by adding the ```x-terraform-id``` extension to one of the

examples/goa/api/Dockerfile

@@ -9,6 +9,8 @@ WORKDIR $WORK_DIR
 COPY swagger/swagger.json /opt/goa/swagger/
 COPY swagger/swagger.yaml /opt/goa/swagger/
 
+RUN git clone --branch v1 https://github.com/goadesign/goa.git $GOPATH/src/github.com/goadesign/goa
+
 RUN go get
 RUN go build -o goa-service-provider .
 

examples/swaggercodegen/api/resources/swagger.yaml

@@ -530,11 +530,11 @@ definitions:
         type: boolean
       newStatus:
         $ref: "#/definitions/Status"
-        x-terraform-field-status: true # identifies the field that should be used as status for async operations. This is handy when the field name is not status but some other name the service provider might have chosen and enables the provider to identify the field as the status field that will be used to track progress for the async operations
-        readOnly: true
 
   Status:
     type: object
+    readOnly: true
+    x-terraform-field-status: true # identifies the field that should be used as status for async operations. This is handy when the field name is not status but some other name the service provider might have chosen and enables the provider to identify the field as the status field that will be used to track progress for the async operations
     properties:
       message:
         type: string

openapi/openapi_spec_analyser_test.go

@@ -1,10 +1,7 @@
 package openapi
 
 import (
-	"encoding/json"
 	. "github.com/smartystreets/goconvey/convey"
-	"io/ioutil"
-	"log"
 	"os"
 	"testing"
 )
@@ -48,16 +45,3 @@ func TestCreateSpecAnalyser(t *testing.T) {
 		})
 	})
 }
-
-func initAPISpecFile(swaggerContent string) *os.File {
-	file, err := ioutil.TempFile("", "testSpec")
-	if err != nil {
-		log.Fatal(err)
-	}
-	swagger := json.RawMessage([]byte(swaggerContent))
-	_, err = file.Write(swagger)
-	if err != nil {
-		log.Fatal(err)
-	}
-	return file
-}

openapi/openapi_spec_resource_schema_definition_property_test.go

@@ -316,7 +316,7 @@ func TestSchemaDefinitionPropertyIsComputed(t *testing.T) {
 			Required: false,
 			ReadOnly: false,
 			Computed: true,
-			Default: nil,
+			Default:  nil,
 		}
 		Convey("When isComputed method is called", func() {
 			isReadOnly := s.isComputed()
@@ -356,11 +356,11 @@ func TestSchemaDefinitionPropertyIsComputed(t *testing.T) {
 func TestSchemaDefinitionPropertyIsOptionalComputed(t *testing.T) {
 	Convey("Given a property that is optional, not readOnly, is computed and does not have a default value (optional-computed of property where value is not known at plan time)", t, func() {
 		s := &specSchemaDefinitionProperty{
-			Type: typeString,
+			Type:     typeString,
 			Required: false,
 			ReadOnly: false,
 			Computed: true,
-			Default: nil,
+			Default:  nil,
 		}
 		Convey("When isOptionalComputed method is called", func() {
 			isOptionalComputed := s.isOptionalComputed()
@@ -371,7 +371,7 @@ func TestSchemaDefinitionPropertyIsOptionalComputed(t *testing.T) {
 	})
 	Convey("Given a property that is not optional", t, func() {
 		s := &specSchemaDefinitionProperty{
-			Type: typeString,
+			Type:     typeString,
 			Required: true,
 		}
 		Convey("When isOptionalComputed method is called", func() {
@@ -383,7 +383,7 @@ func TestSchemaDefinitionPropertyIsOptionalComputed(t *testing.T) {
 	})
 	Convey("Given a property that is optional but readOnly", t, func() {
 		s := &specSchemaDefinitionProperty{
-			Type: typeString,
+			Type:     typeString,
 			Required: false,
 			ReadOnly: true,
 		}
@@ -396,11 +396,11 @@ func TestSchemaDefinitionPropertyIsOptionalComputed(t *testing.T) {
 	})
 	Convey("Given a property that is optional, not readOnly and it's not computed (purely optional use case)", t, func() {
 		s := &specSchemaDefinitionProperty{
-			Type: typeString,
+			Type:     typeString,
 			Required: false,
 			ReadOnly: false,
 			Computed: false,
-			Default: nil,
+			Default:  nil,
 		}
 		Convey("When isOptionalComputed method is called", func() {
 			isOptionalComputed := s.isOptionalComputed()
@@ -411,11 +411,11 @@ func TestSchemaDefinitionPropertyIsOptionalComputed(t *testing.T) {
 	})
 	Convey("Given a property that is optional, not readOnly, computed but has a default value (optional-computed use case, but as far as terraform is concerned the default will be set om the terraform schema, making it available at plan time - this is by design in terraform)", t, func() {
 		s := &specSchemaDefinitionProperty{
-			Type: typeString,
+			Type:     typeString,
 			Required: false,
 			ReadOnly: false,
 			Computed: true,
-			Default: "something",
+			Default:  "something",
 		}
 		Convey("When isOptionalComputed method is called", func() {
 			isOptionalComputed := s.isOptionalComputed()

openapi/openapi_v2_resource.go

@@ -227,7 +227,7 @@ func (o *SpecV2Resource) createSchemaDefinitionProperty(propertyName string, pro
 
 		// Only set to true if property is computed OR optional-computed, purely optional properties are not computed since
 		// API is not expected to auto-generate any value by default if value is not provided
-		schemaDefinitionProperty.Computed = schemaDefinitionProperty.ReadOnly || optionalComputed
+		schemaDefinitionProperty.Computed = property.ReadOnly || optionalComputed
 	}
 
 	// A readOnly property is the one that is not used to create a resource (property is not exposed to the user); but

openapi/openapi_v2_spec_analyser.go

@@ -3,13 +3,14 @@ package openapi
 import (
 	"errors"
 	"fmt"
-	"github.com/dikhan/terraform-provider-openapi/openapi/openapiutils"
-	"github.com/go-openapi/loads"
-	"github.com/go-openapi/spec"
 	"log"
 	"regexp"
 	"strings"
 	"time"
+
+	"github.com/dikhan/terraform-provider-openapi/openapi/openapiutils"
+	"github.com/go-openapi/loads"
+	"github.com/go-openapi/spec"
 )
 
 const extTfResourceRegionsFmt = "x-terraform-resource-regions-%s"
@@ -24,17 +25,21 @@ type specV2Analyser struct {
 
 // newSpecAnalyserV2 creates an instance of specV2Analyser which implements the SpecAnalyser interface
 // This implementation provides an analyser that understands an OpenAPI v2 document
-func newSpecAnalyserV2(openAPIDocumentURL string) (*specV2Analyser, error) {
-	if openAPIDocumentURL == "" {
-		return nil, errors.New("open api document url empty, please provide the url of the OpenAPI document")
+func newSpecAnalyserV2(openAPIDocumentFilename string) (*specV2Analyser, error) {
+	if openAPIDocumentFilename == "" {
+		return nil, errors.New("open api document filename argument empty, please provide the url of the OpenAPI document")
+	}
+	apiSpec, err := loads.JSONSpec(openAPIDocumentFilename)
+	if err != nil {
+		return nil, fmt.Errorf("failed to retrieve the OpenAPI document from '%s' - error = %s", openAPIDocumentFilename, err)
 	}
-	apiSpec, err := loads.JSONSpec(openAPIDocumentURL)
+	apiSpec, err = apiSpec.Expanded()
 	if err != nil {
-		return nil, fmt.Errorf("failed to retrieve the OpenAPI document from '%s' - error = %s", openAPIDocumentURL, err)
+		return nil, fmt.Errorf("failed to expand the OpenAPI document from '%s' - error = %s", openAPIDocumentFilename, err)
 	}
 	return &specV2Analyser{
 		d:                  apiSpec,
-		openAPIDocumentURL: openAPIDocumentURL,
+		openAPIDocumentURL: openAPIDocumentFilename,
 	}, nil
 }
 
@@ -246,7 +251,7 @@ func (specAnalyser *specV2Analyser) validateRootPath(resourcePath string) (strin
 	resourceRootPathItem, _ := specAnalyser.d.Spec().Paths.Paths[resourceRootPath]
 	resourceRootPostOperation := resourceRootPathItem.Post
 
-	resourceRootPostSchemaDef, err := specAnalyser.getResourcePayloadSchemaDef(resourceRootPostOperation)
+	resourceRootPostSchemaDef, err := specAnalyser.getBodyParameterBodySchema(resourceRootPostOperation)
 	if err != nil {
 		return "", nil, nil, fmt.Errorf("resource root path '%s' POST operation validation error: %s", resourceRootPath, err)
 	}
@@ -282,63 +287,39 @@ func (specAnalyser *specV2Analyser) postDefined(resourceRootPath string) bool {
 	return true
 }
 
-func (specAnalyser *specV2Analyser) getResourcePayloadSchemaDef(resourceRootPostOperation *spec.Operation) (*spec.Schema, error) {
-	ref, err := specAnalyser.getResourcePayloadSchemaRef(resourceRootPostOperation)
-	if err != nil {
-		return nil, err
-	}
-	payloadDefName, err := specAnalyser.getPayloadDefName(ref)
-	if err != nil {
-		return nil, err
-	}
-	payloadDefinition, exists := specAnalyser.d.Spec().Definitions[payloadDefName]
-	if !exists {
-		return nil, fmt.Errorf("missing schema definition in the swagger file with the supplied ref '%s'", ref)
-	}
-	return &payloadDefinition, nil
-}
-
-func (specAnalyser *specV2Analyser) getResourcePayloadSchemaRef(resourceRootPostOperation *spec.Operation) (string, error) {
-	if len(resourceRootPostOperation.Parameters) <= 0 {
-		return "", fmt.Errorf("operation does not have parameters defined")
+func (specAnalyser *specV2Analyser) getBodyParameterBodySchema(resourceRootPostOperation *spec.Operation) (*spec.Schema, error) {
+	if resourceRootPostOperation == nil {
+		return nil, fmt.Errorf("resource root operation does not have a POST operation")
 	}
-
-	// A given operation might have multiple parameters, looking for required 'body' parameter type
+	bodyCounter := 0
 	var bodyParameter spec.Parameter
-	var bodyParamCounter int
 	for _, parameter := range resourceRootPostOperation.Parameters {
 		if parameter.In == "body" {
-			bodyParamCounter++
+			bodyCounter = bodyCounter + 1
 			bodyParameter = parameter
 		}
 	}
-	if bodyParamCounter == 0 {
-		return "", fmt.Errorf("operation is missing required 'body' type parameter")
-	}
-	if bodyParamCounter > 1 {
-		return "", fmt.Errorf("operation contains multiple 'body' parameters")
+
+	if bodyCounter <= 0 {
+		return nil, fmt.Errorf("resource root operation missing the body parameter")
 	}
-	payloadDefinitionSchemaRef := bodyParameter.Schema
-	if payloadDefinitionSchemaRef == nil {
-		return "", fmt.Errorf("operation is missing the ref to the schema definition")
+
+	if bodyCounter > 1 {
+		return nil, fmt.Errorf("resource root operation contains multiple 'body' parameters")
 	}
-	if payloadDefinitionSchemaRef.Ref.String() == "" {
-		return "", fmt.Errorf("operation has an invalid schema definition ref empty")
+
+	if bodyParameter.Schema == nil {
+		return nil, fmt.Errorf("resource root operation missing the schema for the POST operation body parameter")
 	}
-	return payloadDefinitionSchemaRef.Ref.String(), nil
-}
 
-// getPayloadDefName only supports references to the same document. External references like URLs is not supported at the moment
-func (specAnalyser *specV2Analyser) getPayloadDefName(ref string) (string, error) {
-	reg, err := regexp.Compile(swaggerResourcePayloadDefinitionRegex)
-	if err != nil {
-		return "", fmt.Errorf("an error occurred while compiling the swaggerResourcePayloadDefinitionRegex regex '%s': %s", swaggerResourcePayloadDefinitionRegex, err)
+	if bodyParameter.Schema.Ref.String() != "" {
+		return nil, fmt.Errorf("the operation ref was not expanded properly, check that the ref is valid (no cycles, bogus, etc)")
 	}
-	payloadDefName := reg.FindStringSubmatch(ref)[0]
-	if payloadDefName == "" {
-		return "", fmt.Errorf("could not find a valid definition name for '%s'", ref)
+
+	if len(bodyParameter.Schema.Properties) > 0 {
+		return bodyParameter.Schema, nil
 	}
-	return payloadDefName, nil
+	return nil, fmt.Errorf("POST operation contains an schema with no properties")
 }
 
 // resourceInstanceRegex loads up the regex specified in const resourceInstanceRegex