Request Input/Output pointer usage.

[jasdel]

We have experimented with a few different solutions to address the pain points mentioned of the service Input Output structs using pointers for fields. Due to the size of the SDK and our commitment to not making breaking changes within a major version once the SDK is released, we want to make sure we provide the best experience while still maintaining major version API stability.

In the end we decided to keep field pointers as they are. None of the alternatives we explored provided adequate improvements for their cost. Additional helper functions should address the pain point of retrieving the primitive pointer's value. Because of this the Config.Merge proposals (#276) will be updated to use pointer primitive fields for consistency.

Any feedback you have on this issue would be much appreciated.

The following are some of the methods we explored:

Values for all fields

Using just primitive values wasn't much of an options because there is no way to identify zero value from fields not set.

Required fields value and optional fields pointer

This wasn't much of an options also, because it would be a breaking change if a service operation changed a field to be no longer required. In addition to this the SDK wouldn't be able to distinguish required field's zero value from a value not set during validation. Also, this isn't as obvious for strings, but would produce unexpected hidden bugs for non string fields such as bools, int64s, and float64s. e.g, service/ec2: CancelSpotFleetRequestsInput.TerminateInstances if the field was mistakenly not set.

Fine grained Input/Output struct primitive struct field pointer vs value

Playing around with this idea we quickly saw fine grained Input/Output struct primitive struct field pointer vs value is not maintainable. An example: A service API initially does not allow empty string for a field. The associated Input struct could a string with omitempty. Later on the API is updated to allow an empty string for the field the SDK would be unable to satisfy that request without a breaking change to the SDK converting the field to *string type.

Value - Struct wrapped primitive with set state

We experimented with using struct values wrapping the pointers, but this method seemed pretty verbose. While it does solve the pain point of doing nil checks, these fields cannot be excluded from the marshaled output using encoding/json or xml packages. golang/go#10648, golang/go#5452, and golang/go#4357 each mention this problem.

type StringType struct{
    val string
    set bool
}
func String(v string) StringType {
    return StringType{v, true}
}
func (s StringType) IsSet() bool {
    return s.set
}
func (s StringType) Value() string {
    return s.String()
}
func (s StringType) String() string {
    if !s.set {
        return ""
    }
    return s.val
}

type GetBucketLocationInput struct {
    Bucket aws.StringType
    // ...
}

params := GetBucketLocationInput{
    Bucket: aws.String("myBucket"}),
}

// Bucket.Value() will return empty string if not set.
if params.Bucket.IsSet() {
    fmt.Println(params.Bucket)
    myBucket := params.Bucket.Value()
}

Pointer - Struct wrapped primitive

We investigated using a pointer to a struct which wrapped the primitive, but the only value this technique seemed to provide over primitive pointer was the discoverability of aws.String() helper func. Whereas this method made it obvious a string could not be directly assigned to the field. Nil checks were still needed, and an additional accessor method or field is needed to be used to retrieve the primitive value.

type String struct {
    string
}
func (s String) String() string { return s.string }

type GetBucketLocationInput struct {
    Bucket *aws.String
    // ...
}

params := GetBucketLocationInput{
    Bucket: &aws.String{"myBucket"},
}

if params.Bucket != nil {
    fmt.Println(params.Bucket)
    myBucket := params.Bucket.string
}

Related to: #276, #271, #265, #157, #124, #114
Pings: @fatih, @clbanning, @stevenh, @foresmac, @conslo, @pdalinis, @mitchellh, @bmatsuo, @LukeMauldin, @dhawal55, @cbroglie, @pges, @guregu, @drombosky, @ngauthier

[ThisGuyCodes]

This is incredibly disappointing. As I've said before, the arguments in favor of this design make sense, but basically seem centered around "We don't want to have to change the SDK if the API changes". Personally Required fields value and optional fields pointer would have been my prefered approach, but that's not the point I want to make, so instead I'll make said point:

I interpret this post as this: We have heard your complaints about the design of this API, but all these suggestions require us to actually maintain the SDK when the API it's used for changes, so we don't care, deal with it.

That may sound childish, but I see this response as childish. You've heard fairly substantial arguments and complaints that this doesn't fit into the ecosystem, and have responded stubbornly with no compromise.

[lsegal]

@conslo I'm not sure the attribution of "We don't want to have to change the SDK if the API changes" is an accurate one. We're not opposed to changing the SDK when updates come. The distinction here is that we do not want to introduce breaking changes into a stable library. It is pretty standard practice for stable libraries not to break their downstream consumers with minor updates. In fact, it would probably extremely un-Go-like to follow the proposed path of breaking the SDK at each update, since even Golang understands the strong importance of backward compatibility.

That said, if you can offer suggestions that does not incur significant backwards incompatible changes, we are all for looking at them.

[ThisGuyCodes]

@lsegal yes, I'll be more constructive, that was largely an emotional response. There is an idea I've been toying with:
https://gist.github.com/conslo/497f985dd74fafaffdaa

Using a request constructing paradigm you could accomplish a very simple interface for the vast majority of requests: svc.Request(param1, param2).Send() and if someone needs newer features (like another optional field being null) they can do something like this: svc.Request("", param2).SetFirst(nil).Send()

[mitchellh]

I don't think this issue is asking anything, however I can give my feedback on pointers.

I completely understand the need for pointers. With the recent helper methods to print the values of structs with pointers within this library, we're actually quite happy with this in Terraform and Packer.

I'm actually not a big fan of "required fields are values and optional fields pointers" since it ties the structure of an API call pretty closely to the validation semantics, which seems odd to me. i.e. What if a field becomes optional later? That wouldn't break BC at an API level, but it would at an SDK level. Or, what if a field is conditionally optional/required? Pretty weird to put that knowledge into the memory layout of a thing.

Overall, I judge an API (using "API" in this context as SDK API, not HTTP API) by my ability to be productive with it. A major blocker was the issue I reported where we couldn't log.Printf("%#v") values because of pointers. This seriously hampered productivity in debugging. However, that was resolved. And now we're shipping software, and we're shipping it fast. Pointers aren't in the way of that. They're weird sure, but practically speaking, we don't care. Everything is consistent: everything is a pointer. We expect it, we're used to it, we can debug it. We're happy.

The only practical problem we've had with the pointers is that it certainly has increased the surface area of crash-ability into our software since you have to nil check every output value. I recommended it in some other issue, but a ValueOrZero type method would be helpful: return the value it is, or return the zero value if it doesn't have a value. That would get rid of a lot of if conditionals for us.

Overall, I'm more supportive of maintaining as much stability as possible. The very regular/consistent approach of the SDK currently is fantastic, and we're very happy with the speed of new API calls becoming available in the lib (it is regenerated often!). We'd be very upset if there was a sudden major backwards incompat change.

Keep up the great work.

[jasdel]

Thank you for the feedback, I updated the OP to clarify our commitment to maintaining API major version stability after release. Apologizes for not making that as clear as it could be.

@conslo I took a look at your gist, it seems similar to the idea proposed by @pges where a builder like pattern would be used to construct the request parameters. This is an interesting idea. I think we would loose our compile-time validation and sanity checking provided by the compiler though. Compile-time checking is one of the strongest benefits we get from request structure initialization. Please let us know any additional thoughts or ideas you have though. We are always interested in better ways to implement the SDK that are maintainable, and won't break API stability when the services add, or make minor changes to their API fields.

@mitchellh On your point about crash surface area being an issue since pointers are commonly used, would helper functions which return the primitive's value, or zero value if the pointer is nil be helpful? I have been playing around with it in the ConfigMergeViaPointer branch. aws.StringPtrValue() would return the value pointed to by *string or the zero value.

In addition to this would adding String() and GoString() to the service Input/Output structs simplify the usage of `log.Printf("%#v") even more? With this you would not need to to call aws.StringValue() directly. addStringers branch of my fork experiments with this idea.

[lsegal]

Perhaps it might be worth looking into having an optional builder pattern structure that users could use when the heavy struct initialization becomes burdensome. For example, we could theoretically allow for:

params := &dynamodb.PutItemInput{}.WithTableName("foo").WithItem(
    &dynamodb.AttributeValue{}.WithS("key", "value").WithS("key2", "value2"))

Or for the simple S3 case:

params := &s3.PutObjectInput{}.WithBucket("bucket").WithKey("key").WithBody(reader)

Reading output values would still use the same pointer structure, but could be improved with some of @mitchellh's suggestions.

[foresmac]

I think the helper function to build common job input scenarios would go a long way to address the pain points I mentioned regarding Elastic Transcoder, for instance. There are really only like 5 things that I think would honestly be passed in in the 80% use case, while about 10 things would be default values and the remaining 30 or so can generally be ignored.

[jasdel]

To clarify the builder pattern we would be considering would not provide common job helper funcs due to the size and breadth of the sdk. Common helper functions would not be maintainable. Even if we provided helper functions for required parameters per operation there are many cases where the common usage would be to also provide optional parameters. For example s3.PutObject is commonly used with ACL settings in the same API call. Or even worse if a required parameter changes to optional the SDK would need to make a breaking change, and this is something we want to avoid.

To expand on the examples provided by @lsegal using Elastic Transcoder to create a Pipeline using the builder pattern would look something like the following. Disappointingly go fmt will remove extra indentions added to WithThumbnailConfig(&elastictranscoder.PipelineOutputConfig{}'s nested builder methods. Though i think the formating is a bug in go fmt due to nested builders not being as common.

client := elastictranscoder.New(nil)

params := &elastictranscoder.CreatePipelineInput{}.
    WithInputBucket("InputBucket").
    WithOutputBucket("OutputBucket").
    WithName("PipelineName").
    WithRole("RoleToUse").
    WithThumbnailConfig(&elastictranscoder.PipelineOutputConfig{}.
    WithBucket("ThumbnailBucket")).
    WithNotification(elastictranscoder.Notifications{}.
    WithCompleted("SnsCompletedTopic").
    Witherror("SNSErrorTopic"))

result, err := client.CreatePipeline(params)
// ...

The same example using the struct initialization method:

client := elastictranscoder.New(nil)

params := &elastictranscoder.CreatePipelineInput{
    InputBucket:  aws.String("InputBucket"),
    OutputBucket: aws.String("OutputBucket"),
    Name:         aws.String("PipelineName"),
    Role:         aws.String("RoleToUse"),
    ThumbnailConfig: &elastictranscoder.PipelineOutputConfig{
        Bucket: aws.String("ThumbnailBucket"),
    },
    Notification: &elastictranscoder.Notifications{
        Completed: aws.String("SNSCompletedTopic"),
        Error:     aws.String("SNSErrorTopic"),
    },
}

result, err := client.CreatePipeline(params)
// ...

[ThisGuyCodes]

I don't think a builder pattern needs to be strictly based on required vs optional, which would, as discussed, cause regular breaking changes. Using the sort of pattern I suggested you could have:

req := ec2Svc.RunInstances(count int, imageID string, instanceType string)

(there could be different parameters decided on, and yes I'm mixing usage and declaration syntax, work with me for a second. In addition I'm also posing that in most instances for someone both max and min count are the same)

req would be a type with helper methods for modifying each of its fields, and returning itself, which would allow you to do this:

req := ec2Svc.RunInstances(count int, imageID string, instanceType string).UserData(userData *string)

and if you somehow needed to set the initial parameters to the null values (why we use pointers):

req := ec2Svc.RunInstances(count int, "" string, instanceType string).ImageID(nil *string)

then (and this is my favorite part) the request type would have a method to send the request inline:

resp, err := ec2Svc.RunInstances(count int, imageID string, instanceType string).Send()

If the constructor method is too verbose or gets in the way you can declare the struct type it creates literally:

req := &ec2Svc.RunInstancesRequest{
    // some values and stuff here, pointers, not literals
}
resp, err := req.Send()

which looks very similar to the current syntax.

The one thing I want to stress here, is that the arguments to the constructor method are decided on by usage not requirements. This is asking "how are most people using this when starting out" not "what are people allowed to do with this." And as such, the arguments to the constructor would not need to change when the api changes a field requirement, and this would be ok because both the helper methods and struct declaration allow (and would always have allowed) users to set these fields to null values.

If this approach is used, I'd be happy with both 'common sense' approach to decisions for what arguments to present in the constructor, and a data centric approach (something I'm sure AWS could dig up from their logs ;)) so long as it's aggregated on a per-customer basis, not a per-call basis, as the problem a constructor would be trying to solve is for people starting out and/or most users, not angled towards power-users (who I imagine are making the most raw calls, but I could be wrong).

[jasdel]

Improving our documentation and examples would go a long way in addressing pain points new beginner customers experience. Through this we think we can greatly improve the experience using the SDK for both new and experienced users. The SDK it self targets long term usability and maintainability, because after a few weeks or months of using the SDK a user will no longer be a beginner and will want or need more flexibly and customizability in how they use the SDK.

With services with a static unchanging API using the idea of function helpers with positional arguments would make perfect sense. This is not the case with AWS services. The services all iterating, adding new features, and extending the capability of existing features. While AWS services are dedicated to backward compatibility, a SDK focusing on specific current use cases would be introducing breaking changes often.

One of the biggest downside about function helpers like this is that as the API grows these function helpers start to develop holes as parameters become optional, or alternative new fields become the preferred way of using the API.

As an example using a fictitious service Foo. Foo starts out with an API operation Bar. Bar requires a JobID and can be given an optional Name field also. The overwhelming common use case is to provide name also even though it's optional. The generic form of foo.BarRequest(p *foo.BarInput)(*BarOutput, err)) would still exist, but would not be commonly used.

foo.Bar(id JobID, name string) (*BarOutput, err)

This works great until Foo's Bar operation is updated to instead of taking a Name string an Identiy struct could be provided. Which creates a conflict with the current SDK. In order to not create a breaking change additional method needs to be added.

foo.Bar(id JobID, name string) (*BarOutput, err)
foo.BarWithIdentity(id JobID, ident Identity) (*BarOutput, err)

Now the capabilities of Identity have been expanded to include information about the job so explicitly passing JobID is no longer required. This puts the users in a situation where operation functions start having holes in them because nil would be provided for jobID. To workaround users needed to use nil parameters in the function additional forms of the function would be created just for Identity.

foo.BarWithOnlydentity(ident Identity) (*BarOutput, err)

With these changes users are faced the conundrum of do they update the application to the new form of bar.BarWithOnlyIdentity or do they simplify and switch to the basic method of foo.BarRequest(p *foo.BarInput)(*BarOutput, err)) to reduce churn.

As the service APIs evolve and grow the surface area of the SDK would grow exponentially quickly become a mess of helper functions for slightly different use cases. Some of which, may no longer be preferred, or even wise to use.

[ThisGuyCodes]

@jasdel I don't see how your post is a response to my suggestion at all. At no point did I suggest static methods for every possible combination, nor did I suggest additional base methods be provided for new functionality (in fact, I expressly said you shouldn't). Are you not familiar with implementing a dot-chaining syntax for constructors? Taking your example:

func (*foo) Bar(id JobID, name string) *BarRequest {}

method Bar returns a request type. Which has some neat methods:

func (*BarRequest) Send() (*BarOutput, error) {}

func (*BarRequest) SetIdentity(ident *Identity) *BarRequest {}

func (*BarRequest) SetName(name *string) *BarRequest {}

func (*BarRequest) SetJobID(id *JobID) *BarRequest {}

So then to send a request I can do:

resp, err := foo.Bar(id, name).Send()

Then, new feature comes along, I want to use an identity struct concept that's available to me now, I have two types of options:

resp, err := foo.Bar(id, "").SetIdentity(&ident).Send()
// Assuming the presence of an identity overrides an empty name, which seems sane to me, or:
resp, err := foo.Bar(id, "").SetIdentity(&ident).SetName(nil).Send()

or

req := &BarRequest{
    Id: &id,
    Identity: &ident,
}
resp, err := req.Send()

Another new feature comes around, I can now bake the job id concept into the identity! (not sure why we're moving concern around so frequently and loosely, but ok):

req := &BarRequest{
    Identity: &ident,
}
resp, err := req.Send()

or

resp, err := foo.Bar(0, "").SetIdentity(&ident).Send()
// Again, assuming sane interpretation, or:
resp, err := foo.Bar(0, "").SetIdentity(&ident).SetName(nil).SetJobID(nil).Send()

The default methods are just that, defaults, sane defaults, like disabling SSLv2 on a webserver program by default instead of requiring anyone who uses it to be versed in every aspect of the technology before they can do anything. They're meant to assist with the simple case where one doesn't need advanced functionality or fine grained control, or even necessarily wants to know about it; but not get in the way when, as the developer learns more functionality over time, they want to do more in-depth things, at which point they would move away from the helper syntax and begin constructing requests on their own, because you've given them that option.

It's very idealistic to expose 100% of the raw functionality of something to people off the bat. Fact of the matter is, most people just don't care about most of what you're throwing at them.

Give us sane defaults. Give people the ability to get something simple now, and the tools to become power users later.

[lsegal]

This is the breaking change that @jasdel was referring to:

Then, new feature comes along, I want to use an identity struct concept that's available to me now, I have two types of options:

resp, err := foo.Bar(id, "").SetIdentity(&ident).Send()
// Assuming the presence of an identity overrides an empty name, which seems sane to me, or:
resp, err := foo.Bar(id, "").SetIdentity(&ident).SetName(nil).Send()

If Bar's previously required Name member becomes conditionally optional, the above operation would likely fail on the AWS side because the SDK cannot determine that "" is a zero-value that should not be serialized. From the perspective of the SDK, passing the empty string to the Name parameter of an AWS service call is a valid thing that the SDK should support. In order to know that in this case, Name cannot be empty, would mean having to special case a whole host of parameters so as to not serialize their zero-values-- and this too would cause breaking changes, since services can relax a requirement to begin allowing empty strings for certain parameters (and this has happened).

The default methods are just that, defaults, sane defaults, like disabling SSLv2 on a webserver program by default instead of requiring anyone who uses it to be versed in every aspect of the technology before they can do anything ...

We might be having to different conversations here. Defaults are great, you won't find any argument from our side about having defaults. The reality is, though, that defaults should come from the API, not an SDK. Defaults, in fact, are provided by AWS APIs, and this relates back to the required vs. optional argument. In other words-- the way that our SDKs surface "default values" is by making a parameter optional. You will find this behavior fairly consistent across our other SDKs.

If you're asking for defaults, what you are effectively asking for is optional arguments from the perspective of the SDK and API. Most of our AWS APIs already do a good job of (a) making secondary parameters optional, and, (b) providing these sane defaults when the parameters are optional. If you're running into scenarios where you are providing too many parameters for a given service / operation and believe that the service or operation should be providing defaults, please let us know about those or open a forum / support issue with that specific service requesting those parameters to provide defaults in the API. If you can provide concrete examples of an operation that has this problem we can discuss that, too.

[ThisGuyCodes]

@lsegal How is that a breaking change? Did you read the comment in the code block you copied? Or the second line of code under it where I set said field to nil? Or the second syntax I provided under it? I've seen the statement about "empty vs nil" a dozen times already and agree with it, and I accounted for its necessity in both forms of syntax I provided (as you may notice, the actual struct fields are pointers, as are the arguments to the modifying methods). Here's the full syntax of one of the methods to be as clear as possible:

func (br *BarRequest) SetName(name *string) *BarRequest {
    br.Name = name
    return br
}

Again, I iterate the name field is a pointer. See where I did .SetName(nil)?

And I'm not talking about default values, but default functionality. Which is why I said default methods, not default values. That entire paragraph was trying to say that the current syntax is useful for power users, but frustrating for those looking to get something simple done. But most users are by definition not power users, and do not need access to every possible combination of parameters (as @jasdel seemed to think I was asking for). I am not suggesting that access to that flexibility be removed, but it doesn't need to be made easier than it is currently because people who need that are already power users, and the syntax/mental overhead/whatever isn't going to be noticed much by them.

I'm asking for a simple syntax for the more common ways things are done (which again, could likely be backed up by data). Yes, the API may now, or in the future, support a (*EC2) DeleteVolume command without a VolumeID being sent, and the SDK should be capable of doing that should I need it, but considering all these methods do is form a request and send it:

func (c *EC2) DeleteVolume(input *DeleteVolumeInput) (*DeleteVolumeOutput, error) {
    req, out := c.DeleteVolumeRequest(input)
    err := req.Send()
    return out, err
}

Some refactoring of the request type (returning things instead of writing to pointers by moving the return value allocation out of the request formation and into the .Send() method), could turn it into something like this:

func (c *EC2) DeleteVolume(input *DeleteVolumeInput) (*DeleteVolumeOutput, error) {
    req := c.DeleteVolumeRequest(input)
    return req.Send()
}

which at this point is just:

func (c *EC2) DeleteVolume(input *DeleteVolumeInput) (*DeleteVolumeOutput, error) {
    return c.DeleteVolumeRequest(input).Send()
}

Which seems... a bit silly to put into it's own method?

(as a slight aside, I just had a journey through the request abstraction here in an attempt to more specifically suggest how to change to the syntax I just did above, and I must say that at a high level, it's quite beautiful. Further down I started having a bit of difficulty following the execution path, but the way you set the Data interface{} to a pointer to a static type to get type safety is pretty cool, if a bit magical)

(but I did figure out how to do it):

type DeleteVolumeRequest struct {
    c     *EC2
    input *aws.request
}

func (r *DeleteVolumeRequest) Send() (*DeleteVolumeOutput, error) {
    op := &aws.Operation{
        Name:       opDeleteVolume,
        HTTPMethod: "POST",
        HTTPPath:   "/",
    } // This could be statically declared outside the method
    // I saw the way it previously was lazy loaded, which required a mutex, but
    // really you can just declare it statically outside the function scope without a mutex.

    if r.input == nil {
        r.input = &DeleteVolumeInput{}
    }

    req = r.c.newRequest(opDeleteVolume, r.input, output)
    output = &DeleteVolumeOutput{}
    req.Data = output // is there a reason this isn't:
    // output = &DeleteVolumeOutput{}
    // req = r.c.newRequest(op, r.input, output)
    // without the need to do "req.Data = output" after?

    err := req.Send()

    return output, err
}

func (c *EC2) DeleteVolumeRequest(input *DeleteVolumeInput) *DeleteVolumeRequest {
    return &DeleteVolumeRequest{
        input: input,
        c:     c,
    }
}

Using a syntax like this would pave the way to (possibly) replace the current (*EC2) DeleteVolume with something like:

func (c *EC2) DeleteVolume(volume string) (*DeleteVolumeOutput, error) {
    return (&DeleteVolumeInput{
        VolumeID: volume,
    }).Send()
}

I'm not actually suggesting DryRun is a value worth omitting, but the example I'm working with only has two fields. It'd be a qualitative (or again, data driven) analysis of each request to decide on a signature. A more concrete suggestion:

func (*EC2) DescribeAddresses(DryRun bool, filters ...Filter) (*DescribeAddressesOutput, error)

Yes, being able to only search certain allocations or IPs is useful, and important, but are likely not nearly as common a use. (the whole slice of pointers thing is still confusing though, I'm also interested in a response to @drombosky's post in this regard).

Does this make sense yet? I acknowledge that you may not agree with my idea, but I've not yet gotten a response that communicates a disagreement with the design, and instead they all seem to misconstrue what I'm trying to say.

[jasdel]

Thank you very much for your feedback. The design of pointers, style, and calling method is complex and very important to the long term maintainability of the SDK. We have a few issues which are duplicating each other and I'd like to collapse the discussion down to a single issue. Lets use #363 for further discussion.

Duplicates: #363
Related: #265, #284, #306