`[]*Type` vs. `[]Type`

[drombosky]

I was wondering if there is a technical reason to use slices of pointers (e.g., []*string) rather than just slices (e.g., []string). Passing around []*Type feels a bit unnatural in Go and is forcing me to either propagate this typing convention or convert before AWS calls.

I understand that pointers to scalers are required scalers are not nilable, but I am not sure why individual elements of a slice need to be nilable.

[ThisGuyCodes]

I second this comment. everything is a pointer, why? In fact the style guide (technically "common code review comments") pushes you towards using values: https://github.com/golang/go/wiki/CodeReviewComments#pass-values

[ThisGuyCodes]

I would also like to add that using pointers everywhere forces people to use the helper methods you declared in the aws package (aws.Long(10)) which just feels silly. (also, Go calls them int64, not long)

[lsegal]

@drombosky the decision to use a list of pointers mostly stems around trying to maintain consistency in the API usage. Having Foo: aws.String("x"), Bars: []Bar{"not-a-ptr"} could potentially be confusing to differentiate. It also means that any values in the list are passed by value when the rest of the input is not, which may have behavioral effects on plugins that operate on data mid-way through the request.

@conslo there is a good discussion of this issue in #114, and I would recommend reading through that to understand the context of this issue. Note that using []Type would not solve the problem of requiring the helper methods, since scalars must be nilable or at the very least support a tri-state style "unset" value. You can read more about this in the related issue.

[ThisGuyCodes]

@lsegal if you're passing a pointer to the struct around (which you are) doesn't that solve the behavioral effects issue you describe?

And I did read through that, and would like to reraise a some examples that were given:
guregu specifically raised 5 instances where the zero-value vs omitted requirement (which I do understand) doesn't make sense. I do actually agree pointers are a clean way to allow distinguishing between omitted and zero values (most relevantly with singular struct values), but there are many cases where it is not required (or realistically ever required, like a list of ID's I'm requesting information on, or the list of information I get in response).

I argue that using pointers vs values selectively where it's needed (rather than everywhere) would actually add to code clarity. If it's a value I know I actually have to put something there, if it's a pointer, I know the API will accept (possibly only under qualifying circumstances) the value being omitted.

[lsegal]

If it's a value I know I actually have to put something there, if it's a pointer, I know the API will accept (possibly only under qualifying circumstances) the value being omitted.

Unfortunately, designing around this assumption would force us to break the API any time a service changed the required-ness of a parameter, which happens fairly often. For example, services often add new parameters that cause a previously required parameter to now be conditionally required, based on other inputs. Alternatively, services occasionally just make parameters optional by introducing defaults computed on the server side. The general assumption is that any parameter can be omitted, even if it is marked as required today.

[drombosky]

@lsegal I'm a bit confused on the plugin use case. I skimmed through the code for hooks and didn't see any. I'm not sure how a plugin wouldn't be able to modify a request in the []Type case, expect if it was trying to act across the API, which I'm not sure is a good design pattern to support.

I think Foo: aws.String("x"), Bars: []string{"not-a-ptr"} is less confusing than Foo: aws.String("x"), Bars: []*string{aws.String(NotAPtr[0]), aws.String(NotAPtr[1])} or having to add additional boiler plate to convert from []Type to []*Type before every call.

If []*Type is a requirement, I would request helper functions for converting between []Type and []*Type, then your example becomes Foo: aws.String("x"), Bars: aws.Strings(NotAPtr). This would be especially useful since there are many types that follow this pattern, not just string.

[jasdel]

We've investigated if we can switch the []*T to []T, but found instances where a service could return sparse arrays in operation responses. Because of this we're unable to remove pointers generically from slices and maps since the service response may have sparse arrays and map elements values which are null.

[ThisGuyCodes]

@jasdel could you give an example of a service that could return sparse arrays; specifically where the existence of said null members is important information for the user? (meaning: transparently removing them as part of the encoding/decoding process would break correct applications)

Related to this, there's an explicit change that may be coming up in 1.5 (if they don't decide to revert) that is related to this idea of null members in encoding/decoding, discussed here: https://groups.google.com/forum/#!topic/golang-dev/3w2QlLDBEjc

Though I don't know if this will actually affect this library (this is talking about json, and I don't know if a similar change was made to xml, and off the top of my head I think you guys are using xml?), it's worth looking at.

[lsegal]

could you give an example of a service that could return sparse arrays; specifically where the existence of said null members is important information for the user?

@conslo the Metadata field in the TrustedAdvisorCheckDescription type in AWS Support API retrieves tabular-style column values in the response, some of which can be null. Omitting these values affects the output, and representing these values as empty strings may not be valid (in this specific case it might be, in a future case it might not).

[ThisGuyCodes]

@lsegal Seems odd to me that you'd have a null values as header fields, but it does make sense that the TrustedAdvisorResourceDetail type that said headers are for might need to be able to do null values.... except in this case this api call is for a service is specifically mirroring an Excel download (and I may be reading it wrong but it looks like that field is documented as a list of strings and required), so I highly doubt empty string vs null will come into play at this endpoint in the future.

I feel like a broken record, but I didn't suggest substituting with empty strings, I suggested omitting them. However tabular data is an instance where omitting these nulls would be incorrect, when you don't manage these header-data associations for the user (if this isn't a concept you're familiar with, I'd suggest checking out the DictReader in python's csv parser, it allows you to iterate over a csv and deal with fields by their header name rather than their position, sorta like how you can deal with values when ranging over a slice instead of doing pointer arithmetic on the location of the first array member) and instead simply mirror the raw data output exactly; but it does also seem like an instance where empty strings would actually be more correct to return (that's more subjective).

More importantly, did you view the discussion on CL 9376 / have you tested this library against this sort of data output on Go 1.5 yet? The decision for that CL seems to directly go against this philosophy of maintaining nulls, and weather you agree with my interpretation or not it may break your implementation.

[lsegal]

I feel like a broken record, but I didn't suggest substituting with empty strings, I suggested omitting them.

I understand you did not suggest this, I was simply laying out the problem with the only possible alternative. Omitting the values entirely is not an option since the columns have significance. This was actually explicitly fixed in the JS SDK recently due to a bug report specifically about omitting nulls. Omitting values shifts the columns and yields incorrect results.

The issue here isn't about the specific API, but the fact that AWS APIs can contain sparse arrays (there is a precedent and there can be other instances that either already exist or APIs that may be created in the future). We therefore cannot make the assumption that arrays are non-null.

More importantly, did you view the discussion on CL 9376 / have you tested this library against this sort of data output on Go 1.5 yet?

I believe @jasdel has already tested against Go 1.5, but he can confirm that.

[ThisGuyCodes]

We don't use encoding/json. I believe jasdel has already tested against Go 1.5, but he can confirm that.

Good to hear, though the idea of the standard library treating nulls as non-data for encoding/decoding purposes may be something to keep an eye on.

The issue here isn't about the specific API...

No, but I did request a specific example, to which you provided one that had a correct alternative. Though I wasn't aware the API served any data in a tabular fashion like this (seems... archaic, to refer to fields by number rather than name, hence my reference to pointer arithmetic), but I accept this library's responsibility is to mirror the API rather than provide a productive abstraction. The fact that endpoints that serve tabular data like this exist is valid enough.