Method argument types require lots of boiler plate

[stevenh]

I'm looking at this new sdk as a replacement for goamz as its the official SDK; however the current structure, in particular the use of custom types, is far from ideal as it means anything interacting with the library needs to do have a significant amount of boiler plate all the time.

Here's a comparison
aws-sdk-go

svc := s3.New(&s3.Config{Credentials: auth, Region: region})
params := &s3.ListObjectsInput{
    Bucket:     aws.String(bucketName),
    Delimiter:  aws.String(delimiter),
    Prefix:     aws.String(prefix),
    MaxKeys:    aws.Long(max),
}
list, err := svc.ListObjects(params)

goamz

svc := s3.New(auth, region)
bucket := s3o.Bucket(bucketName)
list, err := svc.bucket.List(prefix, delimiter, marker, max)

I'm guessing the justification for the use of input types where multi values are required is API flexibility given the lack of overloading in go which is understandable, but the use of custom base types and structs with only a single field seems overkill.

As you can see from the above the amount of type conversion, hence boiler plate code, needed is significant in comparison to competing libraries.

Is there any way to make this so we can do the following, avoiding all the type conversion:

svc := s3.New(&s3.Config{Credentials: auth, Region: region})
params := &s3.ListObjectsInput{Bucket: bucketName,  Delimiter:  delimiter,  Prefix: prefix, MaxKeys: max}
list, err := svc.ListObjects(params)

Similarly for single value types:

svc := s3.New(nil)
params := &s3.GetBucketLocationInput{Bucket: aws.String(bucketName)}
resp, err := svc.GetBucketLocation(params)
if err != nil {
   return nil
}
fmt.Println(resp.LocationConstraint)

Would be much nicer if we could:

svc := s3.New(nil)
location, err := svc.GetBucketLocation(bucketName)
if err != nil {
   return nil
}
fmt.Println(location)

[foresmac]

I agree with the point here; if the SDK abstracted some of the complicated configuration set up, it would alleviate a lot of the need for explanations requested in #262, for instance.

I think part of this is a result of the verbose XML origins of the configuration parameters for the AWS API and the automated code generation of the SDKs. The later makes it easier to keep the SDKs up-to-date with the API, but the trade-off is that the official SDKs seem to be harder to use.

[lsegal]

the use of custom base types and structs with only a single field seems overkill.

@stevenh the big problem is that although these structures may have a single field now, it's very possible they won't in the future. It's also very possible that even required parameters become conditionally optional with the introduction of new fields, so you can't avoid the possibility of a nil on any field. In any case where we add a singular argument method replacement, it would have to be a separate method name so as to not break compatibility in an update, and even that isn't even a great experience, since those methods would effectively become deprecated (on a case-by-case basis) when this happened.

[lsegal]

I agree with the point here; if the SDK abstracted some of the complicated configuration set up, it would alleviate a lot of the need for explanations requested in #262, for instance.

@foresmac I'm not sure solving the main points of this issue would necessarily resolve yours in #262. Specifically, the request here is only for structures with a single input, which is not the case for Elastic Transcoder. We could setup abstracted helper methods for various "common cases" with these services, which I think is what you're getting at, but that falls into the same compatibility burden as pointed out above. When the service changes (and it eventually always will) to introduce new features, those new features will not be reflected in these helper methods, and if we were to rely on simplistic methods with positional arguments, supporting any new service feature for those targeted common cases would require a breaking change in the SDK. I think the right way forward for #262 is better documentation and examples, but that's a discussion for that issue.

@stevenh @foresmac

As a sidenote, a lot of the complexity around using these structures mainly comes from the lack of syntactic support in the language for dealing with nested structures. A lot of things could transparently become much simpler in the SDK if Go were to improve its inference on structural and literal types. With Go 1.5's introduction of map key inference it seems more and more likely that Go is heading in that direction. A few very simple syntactic changes in the language could remove quite a bit of the complexity in the API for free.

[foresmac]

@lsegal Agree with all your points and I understand why it is the way it is.

I'm happy to make my own library of convenience functions and accept the responsibility of keeping up to date for my own use; and that's probably what others would do as well.

[lsegal]

@foresmac if you are willing to open source your helpers we'd be happy to look at creating a section in the wiki to advertise these kinds of helper libraries with the "backward compatibility enthusiasts beware" caveat.

[foresmac]

Once I understand enough from #262, I'm happy to! 👍

[ThisGuyCodes]

@lsegal

~stevenh the big problem is that although these structures may have a single field now, it's very possible they won't in the future. It's also very possible that even required parameters become conditionally optional with the introduction of new fields, so you can't avoid the possibility of a nil on any field. In any case where we add a singular argument method replacement, it would have to be a separate method name so as to not break compatibility in an update, and even that isn't even a great experience, since those methods would effectively become deprecated (on a case-by-case basis) when this happened.

In the compatibility situation you describe, when you introduce new fields, aren't you breaking compatibility anyway? The function signature may not need to change, but if additional fields are required, or response types change, that's still a breaking change. I still need to change my code to add this field to the struct. I disagree with the return types as well, but have a less concrete argument against them.

But this whole "declare a request object before making every request" thing this library makes us do feels... dirty, and unlike what I'm used to expecting when using Go. Actually, it feels like java. As does the extreme verbosity of types/field names:

statuses, err := route53Svc.GetHealthCheckStatus(req)
// handle errors
for status := range statuses.HealthCheckObservations {
    // code
}

I just called a method named GetHealthCheckStatus, what else could I possibly expect as output than a list of statuses? If there's an insistence on this single-return-type model, maybe:

statuses, err := route53Svc.GetHealthCheckStatus(req)
// handle errors
for status := range statuses.Observations {
    // code
}

Is more prudent? https://github.com/golang/go/wiki/CodeReviewComments#variable-names

[lsegal]

In the compatibility situation you describe, when you introduce new fields, aren't you breaking compatibility anyway?

No, adding new fields to a structure does not break compatibility. Additional fields would never be required. We actually pay quite a lot of attention to ensure that we never introduce new required fields. The scenario I described actually shows the opposite case, where the introduction of a new member would come alongside removing the required trait on another member.

But this whole "declare a request object before making every request" thing this library makes us do

I'm not quite sure what you are referring to here. You're not required to declare a request object. The two code examples you listed shows that this is not the case.

I just called a method named GetHealthCheckStatus, what else could I possibly expect as output than a list of statuses? If there's an insistence on this single-return-type model, maybe:

We actually looked at doing this but backed off on the change. In some services this makes some sense, but it does not always make that much sense. In your example, an API update in Route53 could introduce another "type" of "Observations" list that would collide with this singular field making the "Observations" name ambiguous. HealthCheckObservations might be one of the easier examples to list, but there are a few more cases (in AutoScaling, for example) where removing the seemingly redundant prefix actually makes the structure name fairly ambiguous.

[ThisGuyCodes]

@lsegal actually the two examples I gave show it is the case. I just omitted the declaration of the req variable. In fact all/most (haven't actually read every page) of the documentation uses this methodology in examples: http://godoc.org/github.com/aws/aws-sdk-go/service/route53#pkg-examples (though you're using the variable name params instead of req).

I would continue to argue against the verbosity, and I'll say that when I use a method with HealthCheck in it it's reasonable to expect the developer to assume non-explicit values are related to HealthChecks, or a method with the name AutoScaling is assumed to have output related to AutoScaling unless expressly stated otherwise. But that's a less concrete argument, more emotional and style driven, so I'll drop it.

[lsegal]

@lsegal actually the two examples I gave show it is the case. I just omitted the declaration of the req variable. In fact all/most (haven't actually read every page) of the documentation uses this methodology in examples: http://godoc.org/github.com/aws/aws-sdk-go/service/route53#pkg-examples (though you're using the variable name params instead of req).

I think we might be having an issue with terminology then. Those are not considered "request objects", they are simply input parameters to the request, and should not be prefixed as "req". There is an actual request object (see http://godoc.org/github.com/aws/aws-sdk-go/service/route53#Route53.GetHealthCheckCountRequest which actually returns a request object that can be sent later) and this object is a first-class object in the SDK.

Regardless, you are still not required to "declare input before making the request". The examples are generated this way for clarity only. Nothing stops you from passing parameters inline:

resp, err := route53Svc.GetHealthCheckStatus(&route53.GetHealCheckStatusInput{...})

And, of course, if you have no parameters to pass, you can also just pass nil. Most operations have some required parameters, of course, so it's hard to avoid having to pass in arguments to an operation.

[ThisGuyCodes]

It's not literally required, enforcement of that level isn't possible. And yes I could do it all in one line, but does the line of code you just gave me really not look absurdly verbose and cumbersome to you?

resp, err := route53Svc.GetHealthCheckStatus(&route53.GetHealthCheckStatusInput{...})

This issue, and others (#284, #114, #124), I feel are all comments on the style of this library. I tend to come off fairly aggressive, so upfront I want to apologize if that is/becomes the case, but this library does not seem to fit with what my experience with the Go ecosystem is so far. I see the arguments in favor of most of the complaints, and these arguments make sense, but I would personally rather deal with upgrading the version of my sdk to get new optional/required parameters rather than deal with the syntactic salt my code receives from trying to use the officially supported library (that is a specific example, but I think you can see what I'm getting at). It feels... clunky, and incredibly verbose, and when I'm working on/reading my code that uses it the input and output of things feels disjointed, and I don't feel productive.

Arguments in favor of infinite flexibility (which I feel is exactly what's going on here) I find are difficult to argue against. Not because I agree with them, but because I've not yet found a way to succinctly communicate the costs associated with designing towards that. But there are costs. I want to use the officially supported library, but these design decisions have made every time I sit down with this library frustrating.

[foresmac]

I agree with @conslo that the official SDK is very un-Go-like. Like I said earlier in the thread, I understand that this is a result of the Java origins of the AWS APIs and the auto-generation of code from the API. But the side effect is that non-Java SDKs are always going to "feel" wrong because they just don't really follow common idioms or patterns for those other languages.

I'm not sure there's a "good" way to address this other than to build an idiomatic library on top of the official SDK. Otherwise users are left to choose between an official SDK that conforms perfectly to the API but is difficult to use, or something like goamz that's easy to use but doesn't always conform to the current API.

[stevenh]

@conslo makes a very good point here, in that dealing with upgrades when the parameters change would indeed be much more preferable than having to deal with all the unnessaray syntactic sugar thats currently required.

From a support perspective using tags and branches for each version that introduces incompatible changes would work just fine.

As @foresmac says the un-Go-like nature of the library is a huge turn off, and currently we've chosen to not use this library due to that even though helping maintain competing libraries is quite a large overhead, its still preferable, which gives you an idea the overhead that this library introduces.