StringValue used instead of String type

[dhawal55]

One big issue migration to this SDK from other community SDKs is that most of the string types are represented as string pointers. This make porting existing code difficult. Also, simple initialization become cumbersome:

i := Instance { InstanceId: "i-123456" }
become
id:= "i-123456"
i:= Instance { InstanceId: &id }

Now, if you have to do this for 10 more properties, it starts to get annoying. You can use a function, but its its not as clean as just passing string literal.

[lsegal]

Thanks for opening an issue. We're still looking into the best behavior for this and the API may change.

One important thing to note that I'm not sure how other SDKs handle, is the distinction in AWS APIs between an "unset value"-- this is why we've been ensuring that all types are pointers, even for primitive values. For example, many AWS operations differ between providing an empty string or 0-value number and an actual value. If we didn't allow for explicit nil values, our marshalers would not be able to tell if you wanted to reset a field or set it with 0. How do other community implementations currently handle this? Maybe we can take some solutions there.

I personally would love to support string literal initialization, but without solving this issue it would be difficult.

FWIW you can use a function to alleviate most of the ugliness:

id := Instance{ InstanceId: aws.String("i-123456") }

[cbroglie]

This won't help with integer types, but you can leverage the omitempty json struct tag to omit empty strings from serialization. Since DynamoDB doesn't allow empty strings, there is no need to disambiguate. Not sure if other services need to allow for strings to be present but empty.

[lsegal]

@cbroglie

This won't help with integer types, but you can leverage the omitempty json struct tag to omit empty strings from serialization

The issue is that "empty string" in Go does not imply that it should not be sent over the wire. This also applies to any value coming back from a response-- getting a "" from a service often means something different from getting a null or omitted value, and some services can actually return with a literal null.

Since DynamoDB doesn't allow empty strings, there is no need to disambiguate.

Not now, but if this ever changed this SDK would be seriously broken, which is why we would want to avoid designing around this assumption.

Not sure if other services need to allow for strings to be present but empty.

There are quite a few services out there in which an empty string is meaningful and different from an omitted value.

[cbroglie]

I was primarily thinking of empty strings being omitted from request serialization, since that's where the pain point around pointers comes from. Responses could easily remain as pointers. And I was thinking of the DynamoDB requirement being a contract rather than a current implementation detail. But if there is any chance of that changing, then it certainly shouldn't be relied upon.

[lsegal]

In many cases we share the same structure shapes on input and output to allow for roundtripping requests, so it wouldn't always be possible to have literals on input and pointers on output, unless we separated the shapes and disallowed roundtripping, which I think would reduce usability quite a bit.

The alternate option would be to have separate methods that accepted separate copies of the structures with non-pointer types when users wanted the convenience, but that would explode the API surface quite considerably, so I'm not a huge fan of that option.

[lsegal]

For what it's worth, a lot of the ugliness around pointers and string literals have a lot to do with Go's current state of type inference rules. Theoretically, Go could infer the string literal as a pointer type in some future version, which would allow for the initially suggested syntax without any changes to this SDK. I'm not sure if type inference improvements like that are even being considered in Go, but if so, this could be solved without us having to do anything at all. I suppose that is worth noting.

[pges]

I think that using interfaces would avoid the need of using pointers in this case too.

See #125 for a solution for this same case.

Using pointers to basic data types is prone to errors in my opinion, and adds an extra layer of complexity.

[guregu]

Let's look at dynamodb.GetItemInput.

http://godoc.org/github.com/awslabs/aws-sdk-go/service/dynamodb#GetItemInput

AttributesToGet []*string `type:"list"`

Why is it a slice of *string? AFAIK mixing a nil in there makes no sense and will just result in an error.

ConsistentRead *bool `type:"boolean"`

The default is false. Nil and false mean the exact same thing. Why not just make this a boolean with omitempty?

Key *map[string]*AttributeValue `type:"map" required:"true"`

Why is this a pointer? Maps can be nil without making them a pointer. I can think of very few use cases for a pointer to a map in Go and this isn't one of them. Also, why are maps made into pointers but slices (like AttributesToGet) not?

ProjectionExpression *string `type:"string"`

A blank ProjectionExpression is defined to return all results, so this should be a string with omitempty. It's perfectly idiomatic Go to check if a string is blank with str != "", there is no need to make this a pointer.

TableName *string `type:"string" required:"true"`

This is required. There is no reason to make it nullable.

I understand that not all Amazon APIs work the same way as DynamoDB's, but I think it would be worth it to make everything idiomatic where possible. I would be happy to volunteer effort towards this. At the very least get rid of the map pointers, please.

Of course, for situations like "a string that may be null in the response" (i.e. blank strings and null have a different meaning), I think a pointer is totally fine.

[pges]

@lsegal , my suggestion is to take a different approach. Instead of using null pointers for null values to set a default value, I suggest using constructors to set default values and initialize structures that otherwise you have to do manually.

For example, for CreateTableInput type for CreateTable function, I have crated a constructor and added some methods to help in initialization.

API for creating the DynamoDB request struct would be as follow:

    svc := dynamodb.New(nil) // Taken from documentation reference for illustration

    var request *CreateTableInput

    request = NewCreateTableInput("test_table_name").
        WithAttributeDefinitions(
        AttributeDefinition{"name_1", "S"},
        AttributeDefinition{"name_2", "N"},
        AttributeDefinition{"name_3", "B"},
    ).WithKeySchema(
        KeySchemaElement{"uuid", "S"},
    )

    resp, err := svc.CreateTable(request)  // Taken from documentation reference for illustration

The API will create a struct which produces a result Json like this:

{"AttributeDefinitions":[{"AttributeName":"name_1","AttributeType":"S"},{"AttributeName":"name_2","AttributeType":"N"},{"AttributeName":"name_3","AttributeType":"B"}],"GlobalSecondaryIndexes":[],"KeySchema":[{"AttributeName":"uuid","KeyType":"S"}],"ProvisionedThroughput":{"ReadCapacityUnits":2,"WriteCapacityUnits":1},"TableName":"test_table_name"}

Constructor enforces to set a table name wich is compulsory. See that the constructor has initialized every list to an empty list, and has assigned a default value to ReadCapacityUnits and WriteCapacityUnits, which can very useful to initialize boolean values too.

I think that this above makes the need of pointers to basic types to crate null values irrelevant.

Quite likely it makes testing far easier as well.

And this is how the API above works:

// http://godoc.org/github.com/awslabs/aws-sdk-go/service/dynamodb#CreateTableInput
package main

import (
    "encoding/json"
    "fmt"
)

type CreateTableInput struct {
    AttributeDefinitions   []AttributeDefinition  `type:"list" required:"true"`
    GlobalSecondaryIndexes []GlobalSecondaryIndex `type:"list"`
    KeySchema              []KeySchemaElement     `type:"list" required:"true"`
    ProvisionedThroughput  ProvisionedThroughput  `type:"structure" required:"true"`
    TableName              string                 `type:"string" required:"true"`
}

// Handy method to create lists of elements
// Do the same for GlobalSecondaryIndexes, KeySchema, and others
func (c *CreateTableInput) WithAttributeDefinitions(ad ...AttributeDefinition) *CreateTableInput {
    c.AttributeDefinitions = ad
    return c
}

func (c *CreateTableInput) WithKeySchema(ks ...KeySchemaElement) *CreateTableInput {
    c.KeySchema = ks
    return c
}

// Constructor builds the request with default values
func NewCreateTableInput(tableName string) *CreateTableInput {
    return &CreateTableInput{
        AttributeDefinitions:   []AttributeDefinition{},
        GlobalSecondaryIndexes: []GlobalSecondaryIndex{},
        KeySchema:              []KeySchemaElement{},
        ProvisionedThroughput: ProvisionedThroughput{
            ReadCapacityUnits:  2, // Default value
            WriteCapacityUnits: 1, // Default value
        },
        TableName: tableName,
    }
}

type AttributeDefinition struct {
    AttributeName string
    AttributeType string
}

type GlobalSecondaryIndex struct {
    //
}

type KeySchemaElement struct {
    AttributeName string
    KeyType       string
}

type ProvisionedThroughput struct {
    ReadCapacityUnits  uint // If not int, better uint or uint64 than int64: negative value is no sense
    WriteCapacityUnits uint
}

func main() {

    var request *CreateTableInput

    request = NewCreateTableInput("test_table_name").
        WithAttributeDefinitions(
        AttributeDefinition{"name_1", "S"},
        AttributeDefinition{"name_2", "N"},
        AttributeDefinition{"name_3", "B"},
    ).WithKeySchema(
        KeySchemaElement{"uuid", "S"},
    )

    b, err := json.Marshal(request)
    if err != nil {
        fmt.Println("error:", err)
    }

    fmt.Println(string(b))
}

[drombosky]

@guregu There are some cases where pointers are needed because there is a very big difference in the AWS API between non-zero, zero, and omitted. Take ModifyVpcAttribute for example. If EnableDneHostnames and EnableDnsSupport where both literal booleans that were unconditionally marshaled, you would need to specify both values all the time or risk the chance of a true value flipping. Likewise, using omitempty prevents false from being sent over the wire.

@lsegal I do still wonder about []*string and *map[string]*AttributeValue. I'm not sure what those buy technically over []string and map[string]AttributeValue. I'm just getting around to moving my code over from Stripe's implementation and finding it a bit tiring to constantly convert []<type> to []*<type>.

[ngauthier]

👍 to @drombosky. Builtin pointers make sense to me, and also we don't need pointers on nillable types.

[pges]

@drombosky @ngauthier please give me your feedback on the below:

When you want to make a difference when values are set and unset, this is better achieved with a dictionary. In this case because this is are url values, Go type url.Values may be the best candidate.

For ModifyVpcAttribute I have written the following example whose API would be as follows:

    query1 := NewModifyVpcAttribute().
        WithEnableDnsHostnames(false)
    fmt.Println("With one parameter, others omitted: ", query1.UrlValues().Encode())

    query2 := NewModifyVpcAttribute().
        WithEnableDnsHostnames(false).
        WithEnableDnsSupport(true).
        WithVpcId("vpc-1a2b3c4d")
    fmt.Println("With all the paremeters: ", query2.UrlValues().Encode())

This would produce the following output:

With one parameter, others omitted:  EnableDnsHostnames.Value=false
With all the paremeters:  EnableDnsHostnames.Value=false&EnableDnsSupport.Value=true&VpcId=vpc-1a2b3c4d

Full example code:

// Example for omited values
package main

import (
    "fmt"
    "net/url"
    "strconv"
)

// UrlValuer is an interface for creating URL values
type UrlValuer interface {
    UrlValues() url.Values
}

// ModifyVpcAttribute
// Do not create the struct directly. Use the constructor instead.
// http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_ModifyVpcAttribute.html
type ModifyVpcAttribute struct {
    values url.Values
}

func NewModifyVpcAttribute() *ModifyVpcAttribute {
    return &ModifyVpcAttribute{
        values: url.Values{},
    }
}

func (m *ModifyVpcAttribute) UrlValues() url.Values {
    return m.values
}

func (m *ModifyVpcAttribute) WithEnableDnsHostnames(v bool) *ModifyVpcAttribute {
    m.values.Set("EnableDnsHostnames.Value", strconv.FormatBool(v))
    return m
}

func (m *ModifyVpcAttribute) WithEnableDnsSupport(v bool) *ModifyVpcAttribute {
    m.values.Set("EnableDnsSupport.Value", strconv.FormatBool(v))
    return m
}

func (m *ModifyVpcAttribute) WithVpcId(v string) *ModifyVpcAttribute {
    m.values.Set("VpcId", v)
    return m
}

func main() {
    query1 := NewModifyVpcAttribute().
        WithEnableDnsHostnames(false)
    fmt.Println("With one parameter, others omitted: ", query1.UrlValues().Encode())

    query2 := NewModifyVpcAttribute().
        WithEnableDnsHostnames(false).
        WithEnableDnsSupport(true).
        WithVpcId("vpc-1a2b3c4d")
    fmt.Println("With all the paremeters: ", query2.UrlValues().Encode())
}

[ngauthier]

That is a nice solution but I worry it's more complicated and non idiomatic go to have a DSL instead of structs whose unset value is the default. Also I'd imagine this would be harder to generate and maintain, but I am not an expert in go generate.

I also worry that since ur.Values is a map that one could accidentally set a param that was not actually a valid one per the API. Although if it was private and all methods were generated this probably isn't an issue.

url.Values makes sense for many parts of the AWS API since they use query params. But don't others use json and xml payloads? I don't know, but I am not sure we can always omit options. We would need defaults in some cases. Structs are good at that.

I think in the end I prefer structs with pointers. It is a much clearer interface even if it means you have to do a nil check and deference. With your example one would still need nil checks on responses (and you have not shown how you would handle responses, only requests). IMO it is valuable for the request and response API to be symmetric (i.e. both structs) vs having different slightly optimized APIs (like url.Values for requests, then structs or something else for responses).

In my usage, I am more dealing with nils in responses than I am in requests.

Hope that helps!

[drombosky]

I'll echo the points above. Stuct initialization is one of the better idiomatic features of Go.

Building the data structures via function calls would look significantly different compared to standard struct initialization. Also, and this is a personal preference, the justification gofmt provides for stuct initialization makes the code feel cleaner.

The Go compiler also prevents double initialization of struct fields. I'm not sure if that would be possible in this case.

The solution does look like it would work nicely at a higher level. The first thing I thought of when I saw this was chaining together parameterized data transformations in a concise way.

I don't see a problem with continuing to pass pointers to non-nillable types and using helper functions to get the job done (e.g., aws.String and aws.Boolean). My headaches come from transforming []<type> to []*<type> where most of the cases <type> is string or a filter type but may also be another aws-sdk-go type that is passed in a list.

Hope this helps as well!

[ngauthier]

Yes, I do think @pges's solution is very nice at a high level. Reminds me of squirrel. It would be interesting to provide such a package build upon this one. But for the core IMO we should stick to the basics.

[pges]

Thank you for sharing your feedback and raising your concerns; it gives me the opportunity to explain better what my proposition is.

In my experience with the AWS API, three types of data encapsulation are used: Json or XML, headers, and URL parameters. These are better served using the following Go types respectively: struct types, http.Header, and url.Values.

My proposition is not to avoid structs when it is natural to use them, but making initialization easier and safer. I think you may have missed the struct inizialization example above. Struct constructor creates and initialize a struct that you can manipulate later on like any other struct.

As logic is contained in the struct and its constructor, it is easy to test and extend safely if future API would require so.

For the other two use cases, url.Values example would be a better fit. For users the API is pretty similar, and in my opinion it is a better fit than using structs for this too.

[jasdel]

Why is this a pointer? Maps can be nil without making them a pointer. I can think of very few use cases for a pointer to a map in Go and this isn't one of them. Also, why are maps made into pointers but slices (like AttributesToGet) not?

d993b41 was just pushed which addresses the *map usage in the SDK. The *map have been replaced with just normal maps and should improve the usage of maps throughout the SDK.

[guregu]

@jasdel Thank you very much! I think this change will make a lot of people happy 👍

[dhawal55]

Awesome!! When can we do the same for slices?

[jasdel]

@dhawal55 Slices should already be by value not pointer. If there are *[] instances that we missed please let us know so we can correct them also.

Also I'd like to note that this change did not impact the usage of pointer element types within maps and slices. The element types are still pointers.

[jasdel]

Thank you very much for the in depth discussion on this issue. Because of it, we've added conversion utility funcs for []T <-> []_T, and map[string]string <-> map[string]_string. These are available in the aws package. These conversion funcs should help deal with the pain points of working with []_T fields in the SDK. E.g aws.StringSlice will convert []string to []_string. aws.StringValueSlice performs the reverse.

On the issue of replacing []*T throughout the SDK with []T, lets continue that discussion in #284.

[jasdel]

Closing as pointer and value conversion utilities are now available.