Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deserialization logic for polymorphic types #1396

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

ob-stripe
Copy link
Contributor

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

Fixes the deserialization logic to properly handle polymorphic types, i.e. properties that are declared with an interface as their type rather than a concrete class.

Summary of changes:

  • new Stripe.Infrastructure.Util class that contains:
    • a mapping of object values to concrete types, e.g. "charge" => typeof(Stripe.Charge)
    • a GetConcreteType() method that accepts a "potential type" and an object value:
      • if the potential type is a concrete type (not an interface), return it
      • otherwise, use the object value to find the matching concrete type, and return it (if it's compatible with the interface)
  • AbstractStripeObjectConverter, BalanceTransactionSourceConverter, ExternalAccountConverter, PaymentSourceConverter and StripeObjectConverter are all gone
  • new StripeObjectConverter deserializer (same name, but completely different logic) that uses Util.GetConcreteType() to figure out which concrete type to instantiate and populate
    • the converter is directly applied to properties with interface types (e.g. EventData.Object), which ensures that the class can be deserialized with JsonConvert.DeserializeObject()
    • the converter is also used as the ItemConverterType for StripeList.Data so that StripeList can be used with interfaces (e.g. StripeList<IPaymentSource> on Customer)
  • StringOrObject class used for expandable fields also uses Util.GetConcreteType() to decide which type to pass to JObject.ToObject() for deserialization
  • Stripe.Mapper now exposes SerializerSettings property that can be customized by users. Default settings are:

Test changes:

  • Use JsonConvert.DeserializeObject<> instead of Mapper<>.MapFromJson wherever possible (i.e. when the type is not an interface)
  • Add a new wholesome test to ensure that all model classes that implement IHasObject have an entry in the Util.ObjectsToTypes dictionary
  • Add a new test to ensure that strings that contain ISO dates are not changed
  • Add a test to ensure that serializing/deserializing/reserializing produces consistent results

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor feedback but that looks great to me.

src/Stripe.net/Infrastructure/Util.cs Outdated Show resolved Hide resolved
@ob-stripe ob-stripe force-pushed the ob-exp-stripe-converter branch 2 times, most recently from 7333a74 to d8c06a2 Compare November 28, 2018 15:19
@ob-stripe ob-stripe force-pushed the ob-exp-stripe-converter branch from d8c06a2 to 4640ae6 Compare December 4, 2018 10:33
@ob-stripe
Copy link
Contributor Author

r? @brandur-stripe

Hey Brandur, mind taking a look at this one? I know you haven't been too involved in stripe-dotnet recently, but I'd like to get as many eyes as possible on this PR. Let me know if you have any questions or need more context.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay @ob-stripe! This looks like a huge improvement to me.

I'd be temped to just add a couple basic tests for the new Util class and method, especially for the logic in GetConcreteType at the bottom, but other than that this looks great to me!

src/Stripe.net/Infrastructure/Util.cs Outdated Show resolved Hide resolved
@ob-stripe ob-stripe force-pushed the ob-exp-stripe-converter branch from 4640ae6 to 04da400 Compare December 11, 2018 01:25
@ob-stripe ob-stripe changed the base branch from master to integration-v23 December 11, 2018 01:25
@ob-stripe ob-stripe force-pushed the ob-exp-stripe-converter branch from 04da400 to a9a1aec Compare December 11, 2018 16:52
@ob-stripe ob-stripe mentioned this pull request Dec 11, 2018
29 tasks
@ob-stripe ob-stripe force-pushed the ob-exp-stripe-converter branch from a9a1aec to 83879f5 Compare December 11, 2018 17:13
@ob-stripe
Copy link
Contributor Author

I've renamed Util to StripeTypeRegistry and added some tests for GetConcreteType.

Pulling this in!

@ob-stripe ob-stripe merged commit 992f889 into integration-v23 Dec 11, 2018
@ob-stripe ob-stripe deleted the ob-exp-stripe-converter branch December 11, 2018 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants