Better handling of API version mismatch when deserializing events

[ob-stripe]

r? @remi-stripe

Opening the PR for early feedback, but totally open to changes here.

As it is, this PR introduces two changes:

• support for event objects formatted with older API versions (< 2017-05-25), by handling the request attribute as if it were expandable (it's not, but it's functionally the same thing: pre 2017-05-25, request was a string, post 2017-05-25, request is a hash with an id)
• when using Stripe.EventUtility.ConstructEvent or ParseEvent to deserialize event objects (presumably, in a webhook handler), by default the library raises an exception if the API version on the event object doesn't match the API version the library is pinned to. This is a breaking change in the sense the code that used to "work" will now throw an exception ("work" in quotes because when the API versions don't match, the deserialization may fail silently if the format of the nested object changed: older attributes returned by the API will be silently ignored, newer attributes declared in the library will be null). This behavior can be disabled by passing throwOnApiVersionMismatch = false to both methods

Tests fail right now because I haven't fixed them yet. I'll fix / add more tests if you think we should move forward with this change.

Tentative fix for #1402.

[remi-stripe]

I like this approach a lot, especially since the impact on the resource itself is quite minimal. The issue is what happens the day we change the event shape a lot (which could happen)?

Crazy idea: Can we deserialize to an InternalEvent class instead that specifically just handles the request property and nothing else?

[ob-stripe]

I like this approach a lot, especially since the impact on the resource itself is quite minimal. The issue is what happens the day we change the event shape a lot (which could happen)?

Yeah, we're kind of lucky here that the change can be handled like an expandable attribute. In the future, we may need to write a custom deserializer.

Crazy idea: Can we deserialize to an InternalEvent class instead that specifically just handles the request property and nothing else?

I'm not sure I follow. Do you mean we'd keep the existing Event class (with its fixed shape) to be used with EventService (i.e. when sending event retrieve or list requests), and use a different class for EventUtility (i.e. when receiving webhooks)? I don't really see an upside to this.

[remi-stripe]

I mean having an InternalEvent class that you use to deserialize the JSON on first to get the API version and then decide if you should raise an exception or deserialize a second time on the right class. This avoids "leaking" the expandable logic on request just for the sake of supporting 2 API versions. And in the future if there are more changes, we'd break only the internal representation

[ob-stripe]

Gotcha. I think a custom deserializer is a cleaner approach than an InternalEvent class. I'll update the PR.

[ob-stripe]

@remi-stripe I've updated the PR to use a custom deserializer for event objects, so that the Event model class only exposes the attributes matching the current API version, and whatever mapping is necessary is done internally. I've also added some tests. ptal

[ob-stripe]

@remi-stripe I made another change (which I should have added as a second commit but I didn't because I'm stupid): EventData now has an additional RawObject property containing an untyped copy of the data in data.object. This makes it easier to deal with resources for which the library does not have a concrete type (e.g. checkout_beta).

[remi-stripe]

realizing this might affect your version mismatch error handling no?

[ob-stripe]

Hm, good point. I don't think this is going to be a huge problem in practice since EventUtility.ParseEvent should only be used when receiving a webhook though.

[remi-stripe]

Ugh I don't like this wording. I know it's the official API docs one but we really need to automate the comments generation at least at some point in the future.

[ob-stripe]

+1000. I started looking into this, it's not a trivial problem but it's definitely doable.