Bug: serverless.Function_S3Location's Version field should be string type, not int

[ryan-ju]

This code is wrong: https://github.com/awslabs/goformation/blob/d56929b269373078275877578e19f2f554ac6eff/cloudformation/serverless/aws-serverless-function_s3location.go#L24

This is the official doc. It should be string, not int. https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/sam-property-function-functioncode.html#sam-function-functioncode-version

[PaulMaddox]

Hey Ryan, Thanks for reporting this - you're 100% correct. I've had a look into possible solutions for this, but i'm struggling to find something that works well for all use cases.

Because of the large number of SAM/CFN templates that specify as a number, we need to support parsing from both number:

MyFunction:
  Type: AWS::Serverless::Function
  Properties:
    Runtime: nodejs
    CodeUri: 
      Bucket: testbucket
      Key: testkey.zip
      Version: 5

and string:

MyFunction:
  Type: AWS::Serverless::Function
  Properties:
    Runtime: nodejs
    CodeUri: 
      Bucket: testbucket
      Key: testkey.zip
      Version: "5"

If we simply change the S3Location struct to use String, like so:

type Function_S3Location struct {
	Bucket string `json:"Bucket,omitempty"`
	Key string `json:"Key,omitempty"`
	Version string `json:"Version,omitempty"`
}

Then any SAM/CFN templates that specify the value as a JSON/YAML number (such as the first example above), will fail to parse: json: cannot unmarshal number into Go struct field Function_S3Location.Version of type string

The only solution I can think of would be to create an intermediate type with a custom JSON unmarshaller that can accept both number and string formats. Something like this StackOverflow solution.

This has a pretty big downside though - it means that when composing templates in Go code, you can no longer just do:

resource := &serverless.Function{
	Runtime: "nodejs6.10",
	CodeUri: &serverless.Function_CodeUri{
		S3Location: &serverless.Function_S3Location{
			Bucket:  "test-bucket",
			Key:     "test-key",
			Version: "123",
		},
	},
}

Instead for any place in the goformation API where a number is used (which can be specified as a string or number in JSON/YAML due to CFN's relaxed typing), we would need to do something like this:

type Function_S3Location struct {
	Bucket StringOrNumber `json:"Bucket,omitempty"`
	Key StringOrNumber `json:"Key,omitempty"`
	Version StringOrNumber `json:"Version,omitempty"`
}

resource := &serverless.Function{
	Runtime: "nodejs6.10",
	CodeUri: &serverless.Function_CodeUri{
		S3Location: &serverless.Function_S3Location{
			Bucket:  cloudformation.String("test-bucket"),
			Key:     cloudformation.String("test-key"),
			Version: cloudformation.Number(123),
		},
	},
}

... which would be a major change to the API of the library - although not necessarily a bad one, as it would allow us to move to using pointers instead of values, and therefore solve a number of open issues in this library to do with null values.

Unless you can think of a smarter way of handling this?