PayoutDestination Marshaling only returns ID

[mrayonnaise]

go1.12.6 darwin/amd64
stripe-go v62.10.0

When retrieving a PayoutList with expanded Destination ('data.destination') and returning as JSON in our orchestrator, destination is coded to only marshal the ID regardless of expansion.

Line 190+ in payout.go

func (d *PayoutDestination) MarshalJSON() ([]byte, error) {
      return json.Marshal(d.ID)
}

Expected it to return data much like the CURL does, with properly expanded detail.

[brandur-stripe]

So spent a little time looking into this one because a couple of us over here were a little confused as to why these even exist. The comment isn't really explanatory enough to tell us what it's for:

// MarshalJSON handles serialization of a PayoutDestination.
// This custom marshaling is needed because we can only send a string
// ID as a destination, even though it can be expanded to a full
// object when retrieving

Obviously, we don't marshal parameters using JSON (they use form) and we don't really re-encode API resources at all (parameters all use separate structs), so it's not clear what these might be for. I even looked for uses of json.Marshal and there are only three non-test uses in the entire library (error's Error implementation, X-Stripe-User-Agent serialization, and telemetry serialization) — none of them related to serializing API resources.

Aside from PayoutDestination, we have a few others like it too on:

• BalanceTransactionSource
• PaymentSource
• RecipientTransferDestination
• TransferDestination

The one on PaymentSource is by far the most elaborate, and tracking back its implementation, we come to a really old contribution in #88 where a user wanted to proxy requests from stripe-go, and therefore needed JSON marshaling. That solves at least one riddle, and the other implementations might be there for similar reasons.

So I guess first off, I'm not totally sure that this is a case we want to support anyway — even if these four resources currently work, there's almost certainly a whole bunch of other ones that don't. Any resource that has a custom UnmarshalJSON implementation to help unpack a possibly expanded object and which doesn't have a companion MarshalJSON isn't going to re-marshal correctly.

Secondly, if we do want to support it, we should try to do it right by marshaling unexpanded objects back to strings and expanded objects back to objects. We could achieve this by setting some internal property in our custom UnmarshalJSON implementations like:

func (t *Transfer) UnmarshalJSON(data []byte) error {
	if id, ok := ParseID(data); ok {
		t.ID = id
		return nil
	}

	type transfer Transfer
	var v transfer
	if err := json.Unmarshal(data, &v); err != nil {
		return err
	}

	*t = Transfer(v)

	// NEW!!!
	t.expanded = true

	return nil
}

And then introduce MarshalJSON overrides like:

func (t *Transfer) MarshalJSON() ([]byte, error) {
	if !t.expanded {
		return json.Marshal(t.ID)
	}

	type transfer Transfer
	return json.Marshal(transfer(t))
}

(That code doesn't work, but hopefully you get the idea.)

I'd probably lean toward just taking these MarshalJSON overrides out and not supporting this, and then falling back to supporting it if we see a lot of complaints. Maybe we could leave the one for PaymentSource in just because that struct is a bit of a mess in that it specifies most of its fields as json:"-" because its UnmarshalJSON implementation is so custom.

type PaymentSource struct {
	BankAccount     *BankAccount      `json:"-"`
	BitcoinReceiver *BitcoinReceiver  `json:"-"`
	Card            *Card             `json:"-"`
	Deleted         bool              `json:"deleted"`
	ID              string            `json:"id"`
	SourceObject    *Source           `json:"-"`
	Type            PaymentSourceType `json:"object"`
}

@remi-stripe Sorry for the wall of text. Any thoughts on this?

[remi-stripe]

Thanks for the detailed writeup. I'd be happy to remove this until we get real asks about this. I kind of assumed things worked "magically" for mot resources and that we had those extra ones because they are custom where a nested object is polymorphic and we use a union-like approach so we needed a custom serializer.

Is this entirely separate from how fmt.Printf() outputs the content (partially I might say) of a given Stripe resource?

[brandur-stripe]

@remi-stripe

Is this entirely separate from how fmt.Printf() outputs the content (partially I might say) of a given Stripe resource?

Yeah it's totally separate. If you want, you can define how Go should print your type by overriding the special Stringer interface and giving it a custom implementation.

Currently, stripe-go isn't even doing that, and is just using the default implementation for a struct in Go which just iterates through each field and prints its values. You can see that its even a little bit different for %v versus %+v:

// %v
plan = &{true  5000 5000 per_unit 1568669529 usd false plan_Fp5zpehKLdej7C month 1 false map[]  0xc000156500 []  <nil> 0 licensed}

// %+v
plan = &{Active:true AggregateUsage: Amount:5000 AmountDecimal:5000 BillingScheme:per_unit Created:1568669529 Currency:usd Deleted:false ID:plan_Fp5zpehKLdej7C Interval:month IntervalCount:1 Livemode:false Metadata:map[] Nickname: Product:0xc000156500 Tiers:[] TiersMode: TransformUsage:<nil> TrialPeriodDays:0 UsageType:licensed}

Okay then, would you be comfortable if I sent a PR then that removed the MarshalJSON for PayoutDestination, BalanceTransactionSource, RecipientTransferDestination, and TransferDestination? (The only one we'd leave in is PaymentSource, which as mentioned above, is kind of a special case and likely not harmful.)

There is some risk in that it's possible there's some other reason its there which I wasn't able to figure out, but I think it's going to be safe for the vast majority of users anyway.

The other question is if you're comfortable making such a change as a minor — technically, this could be a breaking change for someone, but again, it probably isn't for the vast majority of users, and it seems a little extreme to release a major for this. Thoughts?

[remi-stripe]

Okay then, would you be comfortable if I sent a PR then that removed the MarshalJSON for PayoutDestination, BalanceTransactionSource, RecipientTransferDestination, and TransferDestination? (The only one we'd leave in is PaymentSource, which as mentioned above, is kind of a special case and likely not harmful.)

Sure.

The other question is if you're comfortable making such a change as a minor — technically, this could be a breaking change for someone, but again, it probably isn't for the vast majority of users, and it seems a little extreme to release a major for this. Thoughts?

I'd rather we did a major to be safe here. There are other TODOs in the library that we should clean-up or it could also wait the next API version?
I'm not opposed to a minor, but I don't think it's worth the risk to break users to remove this.

[brandur-stripe]

I'd rather we did a major to be safe here. There are other TODOs in the library that we should clean-up or it could also wait the next API version?

I'm not opposed to a minor, but I don't think it's worth the risk to break users to remove this.

Alright, I don't really have strong feelings about it. I opened #950 and will leave the the PR open until we're reading to bring in a few more things for a major version release.

[remi-stripe]

Closing as #950 was merged alread