Get rid of string pointers, int pointers, aws.String(), etc

[veqryn]

The use of pointers (string pointers, integer pointers, etc) all over the SDK really goes against how Golang is supposed to be written.

I realize that pointers (or an optional-type) may still need to be used in places where a value or field is optional and the zero-value is actually meaningfully different than a nil pointer. However, the vast majority of the time there is no difference between the zero-value and a nil pointer.

This is really ugly:

svc.PutObjectRequest(&s3.PutObjectInput{
	Bucket: aws.String("myBucket"),
	Key:    aws.String("myKey"),
	ACL:    s3.ObjectCannedACLPublicRead,
	Body:   body,
})

It should be:

svc.PutObjectRequest(&s3.PutObjectInput{
	Bucket: "myBucket",
	Key:    "myKey",
	ACL:    s3.ObjectCannedACLPublicRead,
	Body:   body,
})

[veqryn]

I'd like to add that using a pointer communicates something.

In the case of builtin types like strings and integers, using a pointer communicates that there is a difference between s3.PutObjectInput{ Bucket: aws.String("") } and s3.PutObjectInput{}.
It says that a nil/missing-field is perfectly acceptable and legal input for that field.
It also says that a nil/missing field and a pointer to an empty string are different and will result in objects being put into different buckets.

If you accepted non-pointer values, you are communicating that there is no difference between s3.PutObjectInput{ Bucket: "" } and s3.PutObjectInput{}.
This says that an empty string is the same as not filling in this Bucket field.

If the Bucket field is a required field that must be filled in with something, then an empty string or missing field or string pointer are not valid inputs. (Therefore it doesn't make sense to use string pointers for this field.)

This information gets lost when you make every single string field into a pointer-to-a-string.

It isn't just ugly, its also a lost chance to prevent users from making mistakes and misusing the API.
This should be fixed/changed for the same reason you're moving towards typed enumerations for other fields.

Any string/scalar field where a nil pointer is not a legal input, OR where there is no effective difference between a nil pointer and an empty string, should be changed to not use pointers.

[shawnsmithdev]

I suspect this is due to generated code. The REST API this is backed by obviously doesn't know anything about Go and the zero value. As you mention, we may only consider using value fields instead of pointers when the field is required or the empty value can be unambiguously ignored because it is invalid. That requires encoding somewhere what fields are required and what fields don't accept empty values. I suspect the former is more likely to be available for code generation than the latter.

[xibz]

Hello @veqryn, thank you for reaching out to us and providing the feedback. Generally users have an issue with the amount of pointers at first, but realize that there is a reason for it. Mostly it has to do with nil meaning not set. If fields were not pointers, you may be sending an empty value, which may be valid for the request, when it wasn't meant to be sent. Our team has discussed various options other than pointers, but we felt that pointers is the most idiomatic to Golang. When something is optional, use a pointer. I hope you can understand where we are coming from, but if you have any more questions regarding this please let us know.

[veqryn]

I strenuously disagree.

If fields were not pointers, you may be sending an empty value, which may be valid for the request, when it wasn't meant to be sent.

If a nil pointer is valid, then a string pointer must be kept for that field.

When something is optional, use a pointer.

I do not disagree with this. If a field is optional, a pointer is the only way.

However, when the field is required (or a nil pointer and an empty string are equivalent), then the library should not be using pointers for those fields.

[xibz]

If a nil pointer is valid, then a string pointer must be kept for that field.

Was this a response to, If fields were not pointers, you may be sending an empty value, which may be valid for the request, when it wasn't meant to be sent.? If so, not really following the comparison there. Could you clarify?

However, when the field is required (or a nil pointer and an empty string are equivalent), then the library should not be using pointers for those fields.

And when something goes from required to optional? This would break users code because it is now a different type. That is something we do our best not to do, breaking users code when using the SDK. If you have an alternative solution to that, we would love to hear it!

[veqryn]

If fields were not pointers, you may be sending an empty value, which may be valid for the request, when it wasn't meant to be sent.

In the case above, it appears that the hypothetical field is optional (a nil pointer is valid), and an empty string is a valid input that is meaningfully different from a nil pointer. Therefore, string pointers must be kept for this field.

And when something goes from required to optional?

In any other library, this would be considered a change in the API.
This is important information that should be communicated to the users of this library through bumping of the major version number (you do appear to have semantic versioning, even though you aren't using it) and properly changing the API from a string to a string pointer to show that it is now optional.

[xibz]

@veqryn - While I understand your frustration on using pointers, I want to say we did make this decision with the issues pointed out. I am going to go ahead and close this issue since any changes now would be a breaking change and again, something we don't do without good reason.

[veqryn]

That is too bad.
With this library still in alpha/beta/preview stage, it is a good opportunity and time to get this right.

[xibz]

Reopening as I got a lot of feedback on this

[daveadams]

Having used the Go AWS SDK for a few years now, I had this same initial reaction to all the pointers--it does make working with the API feel messy. But I've come to realize that such is the cost of interfacing across language boundaries with APIs that have existed for over a decade in some cases, while providing a usable module.

Idiomatic Go is nice to insist on when you are interfacing only with Go all compiled into the same binary. But this SDK is an interface between Go and the AWS API. And unfortunately idiomatic Go cannot fully represent the remote API's interface on a one-to-one basis. Some compromises have to be made to provide a consistent and reliable SDK for us to use. This seems like a very minor one, all things considered.

The last thing I'd want to happen is for the few required fields to become plain strings, and everything optional to remain pointers. That's a recipe for massive confusion, IMO.

[xibz]

@daveadams - Thanks for the feedback! An idea we've been looking into is optional types. Meaning fields would by optional types in a structure. This idea has come up multiple times and we wanted to get users feedback before we closed this issue! What are your thoughts on something like

type Foo struct
    Bar OptionInt
    Baz *QuxStruct
}

type QuxStruct struct {
    Moo OptionalString
}

func OptionTypes() {
    f := Foo{
        Bar: aws.Int(10),
        Baz: &QuxStruct{
            Moo: aws.String("Hello"),
        },
    }

    if f.Bar.Int() < 15 {
        fmt.Println("Hello world!")
    }
}

Everything would be an option type instead of a pointer, and it is a deviation of how other Go libraries handle optional types, but it is a potential option. The benefit here is that atomic types will always be on the stack, while structs will still be pointers, but allows for the code to avoid all the nil checks.

[veqryn]

@daveadams I get that the aws sdk's must be created in many, quite different, languages. If aws used grpc, then any of my complaints over the style of generated libraries and their usage would more likely be with the makers of grpc and not really with aws. Because aws isn't using proto/grpc, and instead is generating their own client libraries, they have quite a bit more control over how something gets generated. And with their move to a version 2 of the golang sdk, with some of the expressed reasons being stronger typing (enums, etc) and ease of use / discoverability, I think it is good to bring this up and have it addressed. I am neither new to golang nor the aws sdk, and I believe I have a very valid concern.

When I see an api that takes a struct like this:

type FooRequest struct {
    Name string
    Metadata *string
}

I know immediately that Name is a required field (more specifically, that there is no difference between a name that isn't set, and a name that is empty), and Metadata is an optional field.
The API documentation already specifies this, so the actual SDK's API should match.
This isn't just proper golang, it is proper use and communication of an API.

@xibz What would OptionInt and OptionalString actually be? (I am assuming something similar to sql.NullString?) What would be used for required/non-optional fields, just a regular string/int?