RFC: Pointer-based structures for parameters

[brandur-stripe]

We're currently thinking about making a fairly large breaking change (see #507) in the stripe-go interface that would move parameter structures from the "scalars" that they are today:

type ChargeParams struct {
    Customer string `form:"customer"`
}

To pointers instead:

type ChargeParams struct {
    Customer *string `form:"customer"`
}

The rationale behind the move is that we have quite a bit of difficulty modeling empty values to be sent to the API. Today, while we encode a structure, the only way we can tell which of a parameter's fields were set by a user and which were not is by checking whether a value is the type's zero value; so in the above, we'd know that we should encode Customer if its value is anything other than an empty string (the zero value for a string in Go).

The problem with this approach is that in many cases a value's zero value is actually meaningful. You might want to set a Description to an empty string, set a boolean Closed value to false, or an integer Quantity to 0. These cases don't work by default because each of them is the zero value for the type, so we won't encode them with an API request even if the user set them explicitly.

As a workaround, we've traditionally used a secondary field that allows a user to flag that they really wanted the zero value, so something like:

DescriptionEmpty bool `form:"description,empty"`
NoClosed bool `bool:"closed,invert"`
QuantityZero bool `bool:"quantity,zero"`

This works, but is hacky and very unintuitive. By moving to pointers, we could use a nil to determine whether or not a field has been set, and allow each of a type's zero values to become part of our API.

Before pulling the trigger though, we wanted to experiment a little to make sure there isn't a better way to do this. Pointers probably aren't too bad, but have a few problems in that they (1) introduce the possibility of accidental segmentation faults, and (2) require the use of a helper function to coerce a real value into a pointer like Description: stripe.String("foo").

To start, I wrote a simple demonstration of what a pointer-based approach would look like in candidate 1.

We had a bit of a lead in that because unlike most primitives, any interface allows nil, so it might be possible to write a simple interface for every target type and then implement it for the type we're interested in:

type Int64Convertible interface {
	Convert() interface{}
}

func (v int64) Convert() interface{} {
	return v
}

I wrote a demonstration of this up in candidate 2, and while it's supremely elegant, it unfortunately doesn't compile because we're not allowed to add a function on a type outside of our package like int64.

You are allowed to do something similar with your own type, and I've written up that version in candidate 3:

type Int64 int64

func (v Int64) Convert() *string {
	s := strconv.FormatInt(int64(v), 10)
	return &s
}

The problem is that because it's a new type, you still need to do a type conversion in order to use it like Amount: stripe.Int64(123), so in practice this looks no different than candidate 1 that uses pointers. There's some argument to be made that having the custom type is advantageous because it's not interoperable with basic types and you'd need an explicit conversion to use it (which would convey some additional type safety), but it doesn't strike me as a major win in any way.

So in summary, we're thinking about staying course and using pointers as seen in candidate 1 above, but I'm opening this issue just to solicit feedback and/or ideas for better options. If anyone has any opinions on this, let us know.

[brendensoares]

Have you considered existing solutions to this common issue?

A lib like the above may help keep consuming code free of boilerplate types.

Another thought is you could re-export the above null/zero lib types so that the use of them is more obvious/encouraged instead of it becoming an issue to the uninitiated golang developer.

It's a bummer we can't do &(int64(3.14)) . There are many options, but none are concise. I wonder if the Go team will ever address this e.g. by allowing the above syntax.

[brandur-stripe]

Have you considered existing solutions to this common issue?

Can you be more specific about how this would help us? (Ideally with a code sample?) I know about the null package, but I guess I'm not seeing how it would significantly change anything here.

It's a bummer we can't do &(int64(3.14)) . There are many options, but none are concise. I wonder if the Go team will ever address this e.g. by allowing the above syntax.

Yeah, it'd be nice if this worked.

[brendensoares]

Using existing libs like the null lib could simply stripe's implementation.

Just found an article on how github uses pointers in go which confirms the direction you're considering.

Have you considered doing a test branch to see what changes would be required to convert to a pointer-based implementation?

Also, do you need to use pointers for all fields or just optional fields?

[remi-stripe]

@brendensoares You can see the ongoing PR where I implemented all those changes here: #544

[brandur-stripe]

Just found an article on how github uses pointers in go which confirms the direction you're considering.

Good to see another data point here (the GitHub SDK, but also golang/protobuf as mentioned in the article.

Also, do you need to use pointers for all fields or just optional fields?

It's all fields. We think this is the right approach because (1) it's very easy to know where you have to use the pointer helpers — you use them everywhere, and (2) there is some possibility that parameters might flip from optional to required or vice versa. Technically, the latter is not a breaking change from a web API perspective, and it'd be nice not to have to introduce a breaking change to the Go API in order to support it.

[brandur-stripe]

We just released 32.0.0 which moves fields to pointers. Closing out this RFC.