Use IHasObject interface instead of dynamic for data.object attribute of event objects

[ob-stripe]

r? @remi-stripe
cc @stripe/api-libraries @fred-stripe

This PR changes the declaration of the Object property in EventData to be an IHasObject instead of dynamic, and adds a new StripeObjectConverter that can instantiate any Stripe model class based on the value of the object attribute.

Replaces #1129.

Some notes:

• technically, not all objects can be found in events, so this could have been implemented by adding a new interface (IEventDataObject), tagging classes for resources that can be found in events with this interface, and adding a converter that only handles those resources. But this works just as well and means that the library won't have to be updated if we suddenly decide to add new event types for existing resources.

• I added a test for PreviousAttributes as well, mostly because I was curious how Newtonsoft.Json handled deserialization of dynamic properties. It's actually quite nifty: it gives you an object that lets you access all properties of the JSON via methods (.metadata) or indexes (["foo"]). Of course you don't have any type safety and have to cast manually, but we can't make any assumptions on the shape of previous_attributes so that seems reasonable. (To be clear, this is already how the library works today -- I just added a test.)

[ob-stripe]

Had to add this because of the new test that uses the dynamic attribute.

[remi-stripe]

I'm approving because it's definitely a lot cleaner but I have some feedback still.

1/ The name StripeObjectConverter seems a bit too generic and not Event specific. I know if I were to look for the first time I'd assume it does the entire deserialization logic but it doesn't
2/ It is really easy for that class to become out of sync. Is there a way to add tests to avoid that?
3/ Could we use something like this in general to deserialize any object instead of relying on the type declared in the resource (for example on expansion or such) and detect bad situation (like when we have the wrong type/resource (I've done that a few times in go by mistake :( )

Sorry a bit of this is quite vague in general, that's why I'm approving anyway. I just know some of my feedback often trigger some new ideas/improvements for you.

[ob-stripe]

1/ The name StripeObjectConverter seems a bit too generic and not Event specific. I know if I were to look for the first time I'd assume it does the entire deserialization logic but it doesn't

The naming was deliberate -- this converter can deserialize any Stripe object based on the object value. It is only specific to events in the sense that the converter is only invoked if the property is declared directly as IHasObject, and EventData.Object is currently the only place where we do that, but that may change in the future.

2/ It is really easy for that class to become out of sync. Is there a way to add tests to avoid that?

I agree (though fwiw, all our other libraries have this particular pitfall). I think it might be possible to add a test that uses reflection to find all classes that implement IHasObject and ensure they have an entry in the converter's mapping. I'll give it a try.

3/ Could we use something like this in general to deserialize any object instead of relying on the type declared in the resource (for example on expansion or such) and detect bad situation (like when we have the wrong type/resource (I've done that a few times in go by mistake :( )

I see what you mean and I agree it would be nice to detect inconsistencies (e.g. if a property is declared as a Charge but the API returns object=customer, I think currently the deserialization will just instantiate a Charge and fill the properties that the two resources have in common). I'm not sure if/how we could leverage this new converter for this, but I'll think about it.

[remi-stripe]

@ob-stripe Thanks a lot. I think we can totally merge as is. I know how your 🧠 works and there'll be more PRs in a few days :)

[fred-stripe]

I like the direction these changes are headed! I share Remi's concern that it might be hard to keep that list in sync. It might be possible to add a static class initializer to each individual class that we expect to deserialize that registers the string:Class mapping. Alternatively we could add something to the StripeEntity base class (maybe a required parameter for the base constructor?) to ensure that a string:Class mapping exists for any new entity added.