Properly deserialize source on BT during expand

[remi-stripe]

This PR adds support for deserializing source on a Balance Transaction properly. It used to map to a Source class which is a "parent class" for payment methods and has nothing to do with balance transaction.

There's a new custom deserializer and type for this that looks at the balance transaction's source object property and deserializes the right type based on this.

This PR also adds support for local JSON files as fixtures for the new test suite to properly test deserializing all source types.

Fixes #1162
cc @stripe/api-libraries

[remi-stripe]

@ob-stripe Can you have a quick look and confirm this is the right direction? Some questions

• How can I handle testing this? Should I create BTs for each type and confirm they get deserialized properly on expand like I do in the Charge one?
• How can I handle deserializing new types into a generic StripeObject? StripeEntityWithId is abstract so I could create a new type that would be StripeObject that inherits from it but not sure it's the right direction. The goal would be to be able to access the raw JSON if you want things on types we have not yet added to stripe-dotnet (or older versions of the lib). For now I set the type to Unknown.
• I modeled this on the Source.cs implementation. C# does not seem to have union support but is this the best approach?

[ob-stripe]

This looks mostly on the right track to me. Thanks for taking a stab at this @remi-stripe!

How can I handle testing this? Should I create BTs for each type and confirm they get deserialized properly on expand like I do in the Charge one?

Ugh. I'd rather avoid adding complex tests that use the real API. The test suite already takes forever to run, and these tests will either be painful to port to stripe-mock or will be scraped entirely. Since it's unlikely stripe-mock will support returning different source objects under balance_transaction in the near future, I suppose the right thing to do would be to test the various cases using a local set of JSON fixtures, but right now stripe-dotnet does not support stubbing requests...

@brandur-stripe Any thoughts here?

How can I handle deserializing new types into a generic StripeObject? StripeEntityWithId is abstract so I could create a new type that would be StripeObject that inherits from it but not sure it's the right direction. The goal would be to be able to access the raw JSON if you want things on types we have not yet added to stripe-dotnet (or older versions of the lib). For now I set the type to Unknown.

Honestly, I'd rather throw an exception with a message saying to contact support so we can update the library with missing types. Just returning the raw JSON does not sound like a great developer experience. I understand that updating stripe-dotnet can be painful because of the pinned API version and semi-frequent major version bumps, but I don't think we're going to add new types often enough that we should have an escape hatch specifically for this case.

(Worst case, the JSON of the entire response is always accessible via the StripeResponse object.)

I modeled this on the Source.cs implementation. C# does not seem to have union support but is this the best approach?

Not a C# expert™, but that seems fine to me. C unions are not really compatible with classes anyway (even in C++, you cannot have any kind of virtual functions in a union).

[remi-stripe]

Honestly, I'd rather throw an exception with a message saying to contact support so we can update the library with missing types. Just returning the raw JSON does not sound like a great developer experience. I understand that updating stripe-dotnet can be painful because of the pinned API version and semi-frequent major version bumps, but I don't think we're going to add new types often enough that we should have an escape hatch specifically for this case.

We can not do that. If we did this it'd mean that as soon as we add a new type, anyone could end up with an exception or a crash in their production application right? New types are not a breaking change and could be introduced.
I'm fine leaving as is without a custom type but at least putting Unknown as the enum value so that you can do a case on it in the code and look into it as a developer.

[ob-stripe]

If we did this it'd mean that as soon as we add a new type, anyone could end up with an exception or a crash in their production application right?

Ah, good point. Sigh, who decided that an expandable polymorphic attribute was a good idea anyway :shakes_fist:

So yeah, Type=Unknown sounds good to me then.

[jaymedavis]

this is interesting:
https://social.msdn.microsoft.com/Forums/en-US/60150e7b-665a-49a2-8e2e-2097986142f3/c-equivalent-to-c-union?forum=csharplanguage

Do you think StructLayout has promise here?

[remi-stripe]

@jaymedavis Yeah I looked into it but it seems a bit hacky/complex for the sake of saving a little bit of memory

[jaymedavis]

Thought that might be the case. To my knowledge, .net developers rarely ever use the struct keyword anyway.

[remi-stripe]

@ob-stripe I took another stab at this PR now that I better understand the test suite based on stripe-mock. Can you have a look as I think it's ready to merge.

It's a breaking change, but also it never worked so not sure what we want to do here?

[ob-stripe]

Left a minor comment, but looks great otherwise.

[ob-stripe]

I would just add a couple of asserts here:

Assert.NotNull(balanceTransactions.Data)
Assert.IsTrue(balanceTransactions.Data.Count >= 9)

just to avoid exceptions when trying to access something that doesn't exist if the deserialization goes wrong (or if something is wrong with the fixture itself).

[remi-stripe]

@ob-stripe done! PTAL

[ob-stripe]

It's a breaking change, but also it never worked so not sure what we want to do here?

I think it's fine to release this as a minor version. Technically, the API changed, but as you pointed out no integration could use the previous API anyway, so in practice it's more like a new feature.

[ob-stripe]

Released as 17.4.0.