AnyOf<> generic class to handle polymorphic parameters

[ob-stripe]

Here's a little experiment for better handling of polymorphic request parameters. The AnyOf<T1, T2> generic class uses implicit operators to seamlessly accept a value of either type.

One major inconvenience is that this only works for two types. In order to work with three types, we'd need to define another AnyOf<T1, T2, T3> overload, etc. Hopefully we don't have too many parameters that accept many different types!

cc @remi-stripe @brandur-stripe What do you think of this approach?

[brandur-stripe]

Wow, this is so cool!! I had no idea you could do this from user space, haha.

I'm slightly neutral on the change. I love the technical acumen, but I think the old Created / CreatedRange style usage is quite a bit easier to understand. However, one you understand the trick to AnyOf fields, I could see how they'd be pretty handy/fast. I'll leave the decision up to you!

One major inconvenience is that this only works for two types. In order to work with three types, we'd need to define another AnyOf<T1, T2, T3> overload, etc. Hopefully we don't have too many parameters that accept many different types!

That part seems okay to me. Some of the built-in C# classes even do that like Tuple for example.

[remi-stripe]

This looks really cool!

One major inconvenience is that this only works for two types. In order to work with three types, we'd need to define another AnyOf<T1, T2, T3> overload, etc. Hopefully we don't have too many parameters that accept many different types!

I'm on the opposite side where I think it's not good because the ones we (or at least I) wanted to actively clean up are the ones that are not as obvious and take multiple inputs like external_account which can be a string (of multiple things on top of it) or a hash (of multiple different shapes).

[ob-stripe]

Okay, I've updated the PR to make it production-ready:

• AnyOf<> now exists in two versions: AnyOf<T1, T2> and AnyOf<T1, T2, T3> (I certainly hope we never introduce a parameter than can take four different shapes)
• Every polymorphic parameter now uses AnyOf<>
• I've added XML comments to explain that different types can be assigned
• I've added a wholesome test to ensure we don't reuse the same JsonProperty name for different properties
• I've added a JSON converter to serialize and deserialize AnyOf<> values. This is completely unnecessary for the library itself (AnyOf<> is only used in options classes, and options classes are serialized to form-encoding, not JSON, and are never deserialized) but it can maybe help users (cf. Duplicate jsonproperty "external_account" in StripeAccountSharedOptions #648)
  • The deserialization works by attempting to deserialize with each of the possible type. If the deserialization succeeds, then the type is used. This can result in the wrong type being used if the two types have similar JSON shapes. E.g. AnyOf<SomeOptionsClass, AnotherOptionsClass> will always result in SomeOptionsClass being used, because both classes look like hashes when serialized, and the deserializer is not smart enough to look at the hash's keys and figure out the best match. I don't think this is very important, especially since all but one of our polymorphic attributes are of the form AnyOf<string, SomeOptionsClass> so there's no ambiguity.
• Added a bunch of tests

@remi-stripe wdyt? Should we merge this?

[remi-stripe]

So the fact that it works is fairly magnificent, like really. Because we went from "Can we stop having to copy paste all of this and be inconsistent with naming" to "oh wow". So kudos here. It also makes the code (as a user) look so much cleaner, when I look at the test using Created and it just magically handles a string or a hash. Makes me re-excited that we even did this in the API in the first place.

Now there are 2 things that "bother me". Mostly because they are clear unknowns for me:

• What does it look like to a .NET developer using their IDE to figure out what to send. Is it really clear that they can do both, is it explicit about this?
• In the past, we changed some of the serialization/deserialization logic and broke real users doing things we did not expect. Some of it is because we don't think they represent a normal usage of the library but another part is being less familiar with the eco-system surrounding .NET. How do we make sure this is okay? I think part of it is that "this is on parameters so people should not do that magic" but wanted to raise it.

E.g. AnyOf<SomeOptionsClass, AnotherOptionsClass> will always result in SomeOptionsClass being used, because both classes look like hashes when serialized, and the deserializer is not smart enough to look at the hash's keys and figure out the best match. I don't think this is very important, especially since all but one of our polymorphic attributes are of the form AnyOf<string, SomeOptionsClass> so there's no ambiguity.

I did not fully grasp that one. How does it work for External Account where it takes a string or one hash or one other hash? Same for source right?

Separately, Stripe as a whole is moving away (or trying to) from param: string | {} in favour of param: string and param_data: {} so that also should, over time, decrease in usage.

[ob-stripe]

What does it look like to a .NET developer using their IDE to figure out what to send. Is it really clear that they can do both, is it explicit about this?

I think both the type's name and the XML comments do a pretty good job of making it clear that different types can be used. Here's what it looks like in VS Code (with the C# extension) when you hover over the Created property in ListOptionsWithCreated:

And here's what it looks like in Visual Studio for Mac when you hover over the ExternalAccount property in AccountCreateOptions:

Presumably other C# IDEs can also display XML documentation in tooltips.

In the past, we changed some of the serialization/deserialization logic and broke real users doing things we did not expect. Some of it is because we don't think they represent a normal usage of the library but another part is being less familiar with the eco-system surrounding .NET. How do we make sure this is okay? I think part of it is that "this is on parameters so people should not do that magic" but wanted to raise it.

I think it's very unlikely we'd break anyone here, because right now options classes that use the same JsonProperty name for different attributes simply cannot be serialized to JSON, as the JSON serializer raises an exception when the same name is reused for different properties (see #648).

With this PR, serialization will work as users expect. The only potential issue happens if you try to deserialize an AccountCreateOptions or AccountUpdateOptions instance where ExternalAccount is an AccountCardOptions. The JSON string will look "external_account": {"object": "card", "exp_month": 1, "exp_year": 2020, ...}. The deserializer will instead instantiate an AccountBankAccountOptions where every property will be null (except the ones that exist in both classes -- I think Currency is the only one).

I think this is acceptable for now (though I'll add a test to showcase this specific edge-case and document it). Few users provide raw card details, and with any luck the intersection of such users with those who will serialize then deserialize options class will be empty.

That said, if you disagree, here are some alternatives for handling this edge case:

• instead of stopping at the first successful deserialization, we try to deserialize with all types and raise an exception if more than one deserialization succeeded to explain that the type is ambiguous. This will make it impossible to deserialize AccountCreateOptions / AccountUpdateOptions when external_account is a hash (but not when it is a token string), but that's arguably better than potentially deserializing with the wrong type.
• use Newtonsoft.Json's TypeNameHandling setting to add a key to the JSON when serializing, which will allow for unambiguous deserialization. (I don't like this because I'd prefer to keep the JSON strings "clean".)
• use the object key to unambiguously choose the correct type. This is arguably the cleaner solution, but it would require a lot of refactoring to introduce something similar to the IHasObject interface in options classes, which feels like a lot of work for a rare edge case.
• heuristically try to determine the best type, e.g. by counting the number of non-null properties after deserializing with each type.

[remi-stripe]

@ob-stripe Thanks for the screenshots, this feels perfect (though I never use IDEs so hard to say if it really is!)

My main FUD is not on serialization but on deserialization. We have seen developers build their own JSON version of the options because it's easier to reason with for them and then want to pass it in the API. Here's an example in stripe-go where @brandur-stripe explained why we did not want this: stripe/stripe-go#654

I am fully in favour of the change, I think it makes it a lot cleaner and easier to understand than our "magic" around naming fields in a way that tries to make sense of it.

[remi-stripe]

assigning to @brandur-stripe for remaining thoughts.

[brandur-stripe]

Thanks @remi-stripe! LGTM if OB wants to go forward with it.

[ob-stripe]

Thank you both!

My main FUD is not on serialization but on deserialization.

Sure, my point was that it's unlikely anybody is deserializing options classes with polymorphic parameters right now, seeing as it's impossible to serialize them in the first place ;)

Here's an example in stripe-go where @brandur-stripe explained why we did not want this: stripe/stripe-go#654

Thanks for sharing this! Brandur raises a very valid point about adding code that is not used by the library itself -- that's exactly the case of AnyOfConverter that I added in this PR. There's definitely a risk that this adds some maintenance burden on us: with this PR, we're "officially" adding the ability to do JSON serialization/deserialization with options classes to the public contract of the library even though that's completely unneeded for communicating with Stripe's API.

I think the main difference between stripe-go and stripe-dotnet on this particular issue is that stripe-dotnet is already using JSON annotations in options classes, as this was how Jayme originally designed them. Options classes without any polymorphic parameter (i.e. the vast majority) already support JSON serialization/deserialization. In light of this, I think it's fair to consider #648 as a bug rather than a feature request.

Seeing as there are no objections, I will pull this in after giving it a little more polish (going to add some more comments and tests).

[ob-stripe]

There was an issue with the initial implementation. In order to decide which value to return in GetValue(), it compared the value of each of the container attributes (value1 and value2) to their default values, and returned the first one with a non-default value or null if both had default values.

This works fine for reference types where the default value is null, but not for value types. E.g. if you have AnyOf<string, int> x = 0', with the previous implementation x.GetValue() would return null instead of 0 because 0 is the default value for an int.

I fixed this by explicitly storing which of the container values was set in the constructor, in an additional attribute setValue. This way we know exactly which value to return.

This makes it possible to use AnyOf with value types such as enums, allowing us to cover more polymorphic params like the infamous "Unix timestamp or now".

@brandur-stripe @remi-stripe Please take a look at the second commit and let me know what you think. If you're fine with the approach I'll update the PR to cover all remaining polymorphic parameters of the form "timestamp or some magic values".

Also cc @rattrayalex-stripe since we were chatting about this the other day and you seemed interested :)

[remi-stripe]

Damn, I'm glad you caught this issue upfront. I'm curious if we should add some explicit tests to confirm this behaviour (the int 0 value) is not broken by mistake in the future.

[brandur-stripe]

Nice catch Olivier. +1 Remi that some tests would be nice if possible. The fix's implementation looks pretty clean to me.

[rattrayalex-stripe]

You're a wizard, ob! ✨🚀