Parse zero-val pointers

[natdm]

If a boolean value is meant to be true by default, stripe-go uses a pointer to a bool, and uses nil to render no url parameter, which ends up being true to the stripe-api.

Unfortunately, there was no ability to pass a pointer to a boolean of true. This attempts to fix it in a non-breaking, non-major change or refactor way.

I made some smaller changes as well to assist in the readability (since I had to do a lot of that -- reading).

[natdm]

@remi-stripe or someone, since I can't review or assign.

[remi-stripe]

@natdm thanks for the PR! Don't worry it pings the whole team whenever one is created. I'll leave this one to Brandur (assigning him) as it's about the internals of form.go

[brandur-stripe]

@natdm Thanks and sorry about the trouble here!

This patch is looking pretty good. Looking through this though, I think it might be a good idea to shift the behavior a little more broadly because although booleans are mostly what was broken here, it looks like they weren't the only thing. Quantity on OrderItemParams would have previously allowed a zero, but that will also no longer be encoded.

What do you think about tweaking this slightly so that if a field is a pointer and if that pointer has a kind that's a scalar value (i.e. bool, int, string, unit, float, etc., not array, interface, struct), we encode the value even if it's zero value? This would address Quantity as well, and I think the change would be a little nicer.

I realize this is a fairly tall order though, so if you like I can take over the branch and work it through to completion. Thoughts?

[brandur-stripe]

@remi-stripe BTW, I'd totally overlooked that we had a few places that do use pointers to encode params information. I think what happened is that as the Relay stuff was going in, it kind of deviated from the normal conventions randomly because no one was familiar enough with the kit at the time to let the author know about it.

This does represent some precedent though ... since we've got both types of use, it might be worthwhile considering a pointer-based approach elsewhere in the library too (e.g. Prorate *bool instead of Prorate bool + ProrateNo bool). We'd probably want to add some helpers as well (e.g. func Bool(b bool) *bool {) to make usage more convenient than it is now, but it's plausible at least.

[natdm]

@brandur-stripe I noticed that too, and I appreciate the opportunity to take a stab at it. I'll look tonight and see what I can come up with.

[brandur-stripe]

I noticed that too, and I appreciate the opportunity to take a stab at it. I'll look tonight and see what I can come up with.

Thank you! Let me know if you want any other kind of support.

[remi-stripe]

@brandur

This does represent some precedent though ... since we've got both types of use, it might be worthwhile considering a pointer-based approach elsewhere in the library too (e.g. Prorate *bool instead of Prorate bool + ProrateNo bool). We'd probably want to add some helpers as well (e.g. func Bool(b bool) *bool {) to make usage more convenient than it is now, but it's plausible at least.

Agreed that's what I was trying to raise internally on deciding for the right approach on the pointer. You pointed out that passing the value in code becomes harder this way though which rang true.

I'm happy to take any approach though I agree that consolidating this once and for all across the library would be valuable. The ProrateNo option is not the greatest but I'm not sure that the Prorate *bool is cleaner Go-wise. Do you have a personal preference?

[brandur-stripe]

@remi-stripe

Agreed that's what I was trying to raise internally on deciding for the right approach on the pointer. You pointed out that passing the value in code becomes harder this way though which rang true.

Ah sorry. I may not have been reading carefully enough — I didn't realize at the time that we were already doing pointer stuff in some places.

I'm happy to take any approach though I agree that consolidating this once and for all across the library would be valuable. The ProrateNo option is not the greatest but I'm not sure that the Prorate *bool is cleaner Go-wise. Do you have a personal preference?

That's where it gets a little harder. Honestly, if we were doing this from scratch, I'd absolutely just say: pointers.

However, given that we have quite a few existing installations, we should try to minimize upgrade churn so it's not obscenely painful.

I just did some accounting and right now we have:

• 11 form:invert fields for booleans (e.g. NoProrate bool).
• 4 form:empty fields for strings (e.g. CouponEmpty bool).
• 11 form:zero fields for integers (e.g. QuantityZero bool).

If we switched to pointers, we could get rid of all of these and they'd become just x *bool, x *string, and x *int/uint. We could also remove the form special casing, and we could also get everything on the same system (as opposed to Relay parameters doing their own thing).

All in all, I'm pretty on the fence right now. In total there are ~600 parameter fields in the library, so changing these would affect ~5-10%. It'd be painful for existing users, but probably an improvement for future users. What do you think?

[remi-stripe]

@brandur-stripe with the scope of #459 and how widely it changes struct and params names I'm assuming those would be as painful (or as straightforward) to change as the rest for users right?

As in, as long as we tackle this in the same major version it's one big hurdle (for the best) and then the library is cleaner?

If so, I'm definitely in favor of changing this

[brandur-stripe]

If so, I'm definitely in favor of changing this

Cool. Yeah I can get behind that.

Just for inspiration, here's how the AWS Golang SDK looks with an invocation (note aws.String):

// Uploads the object to S3. The Context will interrupt the request if the
// timeout expires.
_, err := svc.PutObjectWithContext(ctx, &s3.PutObjectInput{
	Bucket: aws.String(bucket),
	Key:    aws.String(key),
	Body:   os.Stdin,
 })

The first Go-based GitHub SDK to appear in my Google results also does something very similar (note github.String):

// create a new private repository named "foo"
repo := &github.Repository{
	Name:    github.String("foo"),
	Private: github.Bool(true),
}
client.Repositories.Create(ctx, "", repo)

I think people are relatively used to this pattern in Go and could probably easily get behind it.

[natdm]

For what it's worth, here at Brightcove, we use the AWS libraries for everything and use aws.Bool (or what have you) everywhere. I also use it with Stripe for dealing with these issues.

How I found this particular bug:

// CreateProduct creates a product based on a database item.
func (b *Billing) CreateProduct(acct string, i database.Item) (*stripe.Product, error) {
	meta := make(map[string]string)
	var desc string
	if i.Description != nil {
		desc = *i.Description
	}
	meta["event_id"] = strconv.Itoa(i.EventID)
	params := stripe.ProductParams{Name: i.Name, Desc: desc, Shippable: aws.Bool(false)}
	params.Meta = meta
	params.SetStripeAccount(acct)
	return product.New(&params)
}

[brandur-stripe]

For what it's worth, here at Brightcove, we use the AWS libraries for everything and use aws.Bool (or what have you) everywhere. I also use it with Stripe for dealing with these issues.

Ah, good to know! Well, we should probably be shipping our own, but there's no particular reason that the aws package couldn't also do the job.

[natdm]

Absolutely. Just pointing out that I believe you're correct -- it's an adopted pattern to use a helper library of sorts to create boolean values.

[natdm]

Ok, took a quick stab at it.

I added a lot of {"sometype_ptr", &testStruct{}, ""} tests, mainly for documentation, although doing it once would suffice. I can remove them if you wish.

[natdm]

@brandur-stripe - Let me know how you would do this to clean it up. Feel free to branch the repo or create another PR, no feelings hurt -- I don't feel that this is as clean as it can get (as you said correctly, it's a patch), and I'm not sure how far I should be going with it.

[brandur-stripe]

Hi Nathan, great work on this one. Thanks a lot for taking the fix!

I think it's 95% great. Maybe the only thing I'd change is that I have a slight preference that EncodeZeroVal be passed as an argument instead of in formOptions. Every other member of this struct is an option that was parsed from a form:"xxx" tag, so I'm not sure that it really belongs here.

As an alternative, what do you think about changing the signature of encoderFunc to look something like this?

type encoderFunc func(values *Values, v reflect.Value, keyParts []string, encodeZero bool, options *formOptions)

It makes it a little uglier, but given that it's not publicly exposed, I don't think it matters all that much.

Otherwise, looks great to me!

[brandur-stripe]

It looks like "typ" for types is the abbreviation used in Go core's source code too. Nice!

[brandur-stripe]

Nice improvement. Somehow I'd convinced myself that you weren't allowed to specify an expression like this.

[brandur-stripe]

I love the *0 convention. Thanks for the considerable number of new tests too!

[natdm]

Ok, @brandur-stripe, I took a go at it. It got a little tricky but I was able to do it without touching the tests. I also added a note to the encoderFunc type since I think the explanation was necessary, and that seemed to be the only place for it. This seems to be a much better implementation -- thanks for the hint, and the chance for another go at it.

I agree it didn't seem like it belonged in formOptions. The only reason I stuck it there was because it wasn't called formTags, so I convinced myself it could be allowed to collect more information. :-X

[brandur-stripe]

Nice one @natdm! Thanks again for the hard work on this one.

[natdm]

Glad to commit to one of my favorite products to work with.

[brandur-stripe]

Glad to commit to one of my favorite products to work with.

Just glad you're still happy after running into all these bugs :)

Released as 28.0.1!