Handle unmarshaling OrderItem to account for parent expansion

[spastorelli-stripe]

Attempts to fix the unmarshalling error that can occur when expanding parent of an Order Item object, when calling order.List and expanding on data.items.parent.

r? @brandur-stripe
cc @stripe/api-libraries

[brandur-stripe]

Great work @spastorelli-stripe! Thanks so much for tackling the fix.

Added a few comments below.

[brandur-stripe]

@spastorelli-stripe Would you mind taking a look at the implementation for ExternalAccount (another fairly close polymorphic resource) and seeing if we can put together an implementation a little more like that one?

Namely, a few differences:

1. The custom UnmarshalJSON moves from OrderItem to OrderItemParent.
2. Type on OrderItemParent picks up a JSON annotation of json:"object".

I think that other way has a few advantages:

1. You move the custom implementation right onto the polymorphic object itself, which is better conceptually.
2. You need a one or two fewer json.Unmarshal invocations.
3. You can avoid the boilerplate of assigning Type yourself like oi.Parent.Type = OrderItemParentTypeCoupon.

[spastorelli-stripe]

Thanks for the suggestion! Just to make sure I'm not overlooking something, were you suggesting to take an approach similar to this:
https://gist.github.com/spastorelli-stripe/a368768498d0607c3c20c1bddbf0169b#file-order-go-L52-L57
https://gist.github.com/spastorelli-stripe/a368768498d0607c3c20c1bddbf0169b#file-order-go-L218-L245

One issue I would see with moving the custom UnmarshalJSON method from OrderItem to OrderItemParent is that the data byte array provided to the call would only include the bytes for the "parent": .... property of the order_item object [1].

As this parent value isn't consistently returned as a subhash, it doesn't always contain an object property and as a result Type on OrderItemParent may not get consistently populated [1] and from my understanding we wouldn't be able to backfill it from the OrderItem since it isn't accessible from OrderItemParent.

My concern would be that if we provide a Type on OrderItemParent there could be expectation at the call site that this value is populated whenever the parent is requested to be expanded.

It may not be an issue but wanted to double check with you just in case.
Let me know what you think.

[1]
For SKU order items, parent would be expanded nicely:

{
      object: "order_item",
      amount: 499,
      currency: "usd",
      description: "test name",
      parent: {
        id: "TEST-SKU-qSvWbSxdyAHQxEje",
        object: "sku",
        ...
        updated: 1535639219
      },
      quantity: 1,
      type: "sku"
}

But for items like shipping or discount where parent doesn't expand (despite requesting to expand it) only the ID string is returned:

{
      object: "order_item",
      amount: -399,
      currency: "usd",
      description: "50usdoffonce2: $50.00 off",
      parent: "50usdoffonce2",
      quantity: null,
      type: "discount"
}

[brandur-stripe]

Thanks for the suggestion! Just to make sure I'm not overlooking something, were you suggesting to take an approach similar to this:

Yeah, that's exactly it!

One issue I would see with moving the custom UnmarshalJSON method from OrderItem to OrderItemParent is that the data byte array provided to the call would only include the bytes for the "parent": .... property of the order_item object [1].

Ah, I see why you went the way you did.

My preference would still be to change this. I think your justification makes sense, but it feels like we're trying to paper over a broken API with this approach rather than fix the source.

I was just looking at the backend code, and although apparently only SKUs are expandable according to your implementation, it looks at least like the intent was that discounts should be expandable as well, and it's bug if they're not expanding correctly. The lack of expansion on other types is hopefully an oversight that we can correct — ideally, if one type is expandable, than they all are.

While waiting for those fixes though, I don't think it's that bad to just have users check the Type on OrderItem because they'll be holding a reference to that anyway. e.g.:

if orderItem.Parent == OrderItemParentTypeSKU {
    // Do something with orderItem.Parent.SKU

This keeps the implementation more symmetric with how the API actually looks, and we can expand it if more parents become expandable in the future. What do you think?

ptal @spastorelli-stripe

[spastorelli-stripe]

Thanks. Makes sense. Changed as per suggestions. PTAL

[brandur-stripe]

Great tests!

Would you mind wrapping each section in {} braces? I find this looks a little cleaner and is a bit easier to follow. For example:

func TestOrderItem_UnmarshalJSON(t *testing.T) {
    // Try unmarshaling a SKU
    {
        ...
    }

    // Try unmarshaling a coupon
    {
        ...
    }

    // Try unmarshaling a shipment
    {
        ...
    }

    ...
}

[spastorelli-stripe]

Thanks for the suggestion. Done

[brandur-stripe]

That's actually how the API works?? Horrible :$

[brandur-stripe]

Nit, but we generally order "computer alphabetically", and uppercase letters sort before lowercase ones. Would you mind putting SKU here above Shipping?

[spastorelli-stripe]

Done

[spastorelli-stripe]

Thanks @brandur-stripe for the review and suggestions.
I followed up on the suggestion about moving the custom UnmarshalJSON method from OrderItem to OrderItemParent. Would you mind taking a look at it and let me know what you think?

[spastorelli-stripe]

Thanks for your follow @brandur-stripe. I believe I have addressed all suggestions. PTAL

[brandur-stripe]

Thank you @spastorelli-stripe! This is a pretty complicated case, so thanks for tackling it.

LGTM.

[brandur-stripe]

Released as 48.0.0.

[rothskeller]

Thanks to you both for your work on this. (And it's nice to see a more effective software development flow than the one I deal with at my company! :-)