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

Support JSON serialization with System.Text.Json #2495

Open
cecilphillip-stripe opened this issue May 26, 2022 · 20 comments
Open

Support JSON serialization with System.Text.Json #2495

cecilphillip-stripe opened this issue May 26, 2022 · 20 comments

Comments

@cecilphillip-stripe
Copy link

Is your feature request related to a problem? Please describe.

Stripe.NET takes a hard dependency on the Netwonsoft.Json serializer which is fine for some cases. Starting with ASP.NET Core 3.0, the default JSON serializer has changed to System.Text.JSON.

For ASP.NET Core applications running on v3.0+ that use our Stripe.NET nuget package, developers have to be concerned with the inconsistencies between the serializers when working with Stripe Objects like Products, Customers, etc.

Serialized single Stripe Product w/ Newtonsoft

{
  "object": "product",
  "active": true,
  "attributes": [],
  "caption": null,
  "created": 1650997792,
  "deactivate_on": null,
  "default_price": null,
  "description": "This is such an awesome product made out of Metal",
  "images": [
    "https://picsum.photos/640/480/?image=279"
  ],
  "livemode": false,
  "metadata": {
    "code": "XXFYF3V"
  },
  "name": "Gorgeous Plastic Pants",
  "package_dimensions": null,
  "shippable": true,
  "statement_descriptor": null,
  "tax_code": null,
  "type": "service",
  "unit_label": null,
  "updated": 1650997792,
  "url": null
}

Serialized single Stripe Product w/ System.Text.Json

{
  "Object": "product",
  "Active": true,
  "Attributes": [],
  "Caption": null,
  "Created": "2022-04-26T18:29:52Z",
  "DeactivateOn": null,
  "DefaultPriceId": null,
  "DefaultPrice": null,
  "Deleted": null,
  "Description": "This is such an awesome product made out of Metal",
  "Images": [
    "https://picsum.photos/640/480/?image=279"
  ],
  "Livemode": false,
  "Metadata": {
    "code": "XXFYF3V"
  },
  "Name": "Gorgeous Plastic Pants",
  "PackageDimensions": null,
  "Shippable": true,
  "StatementDescriptor": null,
  "TaxCodeId": null,
  "TaxCode": null,
  "Type": "service",
  "UnitLabel": null,
  "Updated": "2022-04-26T18:29:52Z",
  "Url": null,
  "RawJObject": null,
  "StripeResponse": null
}

You'll notice that the expected casing is different as well as the number of returned properties. Below are examples of serializing a StripeList

Serialized StripeList of Product w/ Newtonsoft.Json

{
  "object": "list",
  "data": [
    {
      "id": "prod_La3FD0M3LXeUPE",
      "object": "product",
      "active": true,
      "attributes": [],
      "caption": null,
      "created": 1650997792,
      "deactivate_on": null,
      "default_price": null,
      "description": "This is such an awesome product made out of Metal",
      "images": [
        "https://picsum.photos/640/480/?image=279"
      ],
      "livemode": false,
      "metadata": {
        "code": "XXFYF3V"
      },
      "name": "Gorgeous Plastic Pants",
      "package_dimensions": null,
      "shippable": true,
      "statement_descriptor": null,
...
  ],
  "has_more": true,
  "url": "/v1/products"
}

Serialized StripeList of Product w/ System.text.json

[
  {
    "Id": "prod_La3FD0M3LXeUPE",
    "Object": "product",
    "Active": true,
    "Attributes": [],
    "Caption": null,
    "Created": "2022-04-26T18:29:52Z",
    "DeactivateOn": null,
    "DefaultPriceId": null,
    "DefaultPrice": null,
    "Deleted": null,
    "Description": "This is such an awesome product made out of Metal",
    "Images": [
      "https://picsum.photos/640/480/?image=279"
    ],
    "Livemode": false,
    "Metadata": {
      "code": "XXFYF3V"
    },
    "Name": "Gorgeous Plastic Pants",
    "PackageDimensions": null,
    "Shippable": true,
    "StatementDescriptor": null,
...
    "RawJObject": null,
    "StripeResponse": null
  }
]

With lists you can see the data nesting between the serializers are also different.

Describe the solution you'd like

Since System.Text.JSON is the default serializer for asp.net core web applications, It would be great if there could be some consistency with the expected responses when serializing/deserializing Stripe.NET object.

Can we have Stripe.NET use System.Text.JSON if available and fallback to Newtonsoft.JSON? Otherwise, I've seen other package authors create separate packages for each serializer but I'm not sure if we'd want to do that here.

Describe alternatives you've considered

This can be mitigated by using the setting Newtonsoft.Json as the default serializer by using the Microsoft.AspNetCore.Mvc.NewtonsoftJson package.

Another option would be to return a custom response type instead of the plain Stripe object. For example, see here.

Additional context

No response

@clement911
Copy link
Contributor

I'm not sure if you're aware but every stripe entity in the Stripe.Net lib inherits from the StripeEntity base class, which has a ToJson and FromJson methods. This way if tomorrow Stripe.Net starts using System.Text.Json, these methods should keep working as they do now.

@cecilphillip-stripe
Copy link
Author

@clement911 all the Stripe objects use attributes from Newtonsoft.json => example.

This would have to be updated if another serializer is adopted so that the shape of the payload is consistent.

@emorell96
Copy link

I've been porting this library to System.Text.Json these last few days, it has quite its challenges, specially how much polymorphism is used which is always a weak point for STJ but I have been able to get most of it working on my branch:

https://github.com/StockDrops/stripe-dotnet/tree/without-stripe-entity

Would you all be interested in a PR for a future version or at least some discussion on how to better port it? Specially if you ever decide to do the switch, it might come in handy to have most of the converters out of the way.

@richardm-stripe
Copy link
Contributor

Hi @emorell96, sorry for the delayed response. Thank you for preparing that pull request!

cc @cecilphillip-stripe, I think this is worth discussing. I want to specifically talk about whether we could do this non-breakingly? If not; how disruptive will this change be to users who are upgrading? Is it possible that changing default serialization libraries would break them in subtle ways?

@cecilphillip-stripe
Copy link
Author

@richardm-stripe started a conversation with some folks on this. There's also a migration document we can reference.

To not break existing users, we'd have to find a way to support both and gradually migrate away.

@AraHaan
Copy link
Contributor

AraHaan commented Apr 8, 2023

I think a better option would be the following:

  • for .NET Framework compatibility use Newtonsoft.Json on netstandard2.0 target.
  • for .NET 5+ compatibility use System.Text.Json on netstandard2.1 target.
  • for targets between .NET Core 3.0 and .NET 5 are EOL by Microsoft (even .NET 5 itself) so I see little need for people to be using .NET 5 either, as for Core 3.0 and 3.1 they are more likely to be using .NET 6 until .NET 8 is released this November.

For used Newtonsoft.Json attributes:

  • I would recommend new dummy attributes be created that basically inherit from the attributes used, that way different configurations on the project would seamlessly use the correct attributes in both System.Text.Json and Newtonsoft.Json depending on the configured framework used to build.

Alternatively, one could look into making ASP.NET Framework use System.Text.Json instead as well so then it can be unified for both cases too.

@iamcarbon
Copy link
Contributor

To facilitate a migration, would it be possible to double up attributes / serialization converters and begin using System.Text.Json for all internal serialization methods?

@AraHaan
Copy link
Contributor

AraHaan commented Jul 10, 2023

I guess another option would be to define an interface in stripe-dotnet and then have separate json "plugins" that implement said interface for newtonsoft and system.text.json that then get loaded at runtime depending on what the user determine that they want to use (it gets loaded into something similar to DependencyInjection but in a way usable in all of the supported frameworks of .NET)

@ismkdc
Copy link

ismkdc commented Feb 12, 2024

We definitely need system.text.json version of stripe.net

@btecu
Copy link

btecu commented Mar 1, 2024

Any news? Would rather use System.Text.Json than Newtonsoft.Json, which has not seen a release in almost a year.

@ramya-stripe ramya-stripe changed the title Support JSON serialization with System.Net.Json Support JSON serialization with System.Text.Json May 10, 2024
@ramya-stripe
Copy link
Contributor

Update: We will be revisiting this in August.

hootanht added a commit to hootanht/stripe-dotnet that referenced this issue Sep 13, 2024
Fixes stripe#2495

Add support for JSON serialization with System.Text.Json.

* Add `JsonSerializer` interface and implement `NewtonsoftJsonSerializer` and `SystemTextJsonSerializer` classes in `src/Stripe.net/Infrastructure/JsonSerializer.cs`.
* Update `StripeEntity` class in `src/Stripe.net/Entities/_base/StripeEntity.cs` to use the `JsonSerializer` interface for `ToJson` and `FromJson` methods.
* Replace `Newtonsoft.Json` attributes with `System.Text.Json` attributes in `src/Stripe.net/Entities/Customers/Customer.cs`, `src/Stripe.net/Entities/_common/Address.cs`, `src/Stripe.net/Entities/_common/AddressJapan.cs`, `src/Stripe.net/Entities/_common/Shipping.cs`, `src/Stripe.net/Entities/AccountLinks/AccountLink.cs`, and `src/Stripe.net/Entities/Accounts/Account.cs`.
* Add conditional compilation to use appropriate attributes based on the selected JSON serializer in the above files.
@jar-stripe
Copy link
Contributor

Howdy folks! I'm starting to work on this issue in earnest and I wanted to get some feedback from the people interested in the solution here. As a first pass, I am considering a version of @iamcarbon 's suggestion, copied here for context:

To facilitate a migration, would it be possible to double up attributes / serialization converters and begin using System.Text.Json for all internal serialization methods?

This feels like a good approach, but I'm considering an even slimmer implementation where we:

  • add System.Text.Json attributes and converters to our existing SDK
  • leave Json.NET in place for internal serialization
  • only enable this for .NET 6 and above

I'm considering this because: it solves the original issue, and ensures Stripe.NET models serialize to JSON correctly with STJ; replacing Json.NET with STJ internally would require a lot of testing and validation; and STJ is not officially supported on .NET versions that we do support. I'm considering restricting it to .NET 6 and above because it is supported out of the box in those runtime versions (i.e. without any additional dependencies).

My question is, if we just add the necessary attributes and converters to ensure Stripe models serialize equivalently with STJ as they do with Json.NET, does that solve your issue? If it does, a thumbs up would be awesome; if it doesn't, please describe your use case, describe how this might not work in your case, and please include the .NET version you're using. Thanks y'all!

@clement911
Copy link
Contributor

@jar-stripe that sounds like a good approach indeed.

Please expose an equivalent to StripeEntity.RawJObject so that we can continue accessing properties that are not directly exposed by Stripe's .NET

@mwadams
Copy link

mwadams commented Nov 12, 2024

Howdy folks! I'm starting to work on this issue in earnest and I wanted to get some feedback from the people interested in the solution here. As a first pass, I am considering a version of @iamcarbon 's suggestion, copied here for context:

To facilitate a migration, would it be possible to double up attributes / serialization converters and begin using System.Text.Json for all internal serialization methods?

This feels like a good approach, but I'm considering an even slimmer implementation where we:

  • add System.Text.Json attributes and converters to our existing SDK
  • leave Json.NET in place for internal serialization
  • only enable this for .NET 6 and above

I'm considering this because: it solves the original issue, and ensures Stripe.NET models serialize to JSON correctly with STJ; replacing Json.NET with STJ internally would require a lot of testing and validation; and STJ is not officially supported on .NET versions that we do support. I'm considering restricting it to .NET 6 and above because it is supported out of the box in those runtime versions (i.e. without any additional dependencies).

My question is, if we just add the necessary attributes and converters to ensure Stripe models serialize equivalently with STJ as they do with Json.NET, does that solve your issue? If it does, a thumbs up would be awesome; if it doesn't, please describe your use case, describe how this might not work in your case, and please include the .NET version you're using. Thanks y'all!

Can I ask which .NET versions you support that STJ doesn't.

@aaronhudon-pq
Copy link

Some of us are stuck on .netstandard, so please don’t break us.

@jar-stripe
Copy link
Contributor

jar-stripe commented Nov 12, 2024

@clement911 Thanks for pointing that out! Do you mind if I ask what properties you use that for today?

@mwadams .NET 4.6.1 is still supported by Stripe.net but not supported by System.Text.Json; for any .NET before .NET 6, we have to add an additional dependency. We're not opposed to doing this but it does add complexity and overhead to manage -- STJ updates a lot more frequently than Json.NET. What .NET version would you want to use with Stripe.NET and STJ?

@aaronhudon-pq Don't worry! We are definitely not changing which .NET versions we support as part of this.

Thanks y'all for the comments so far. Keep em coming!

@jar-stripe jar-stripe self-assigned this Nov 12, 2024
@clement911
Copy link
Contributor

@jar-stripe we use it to pull "on_behalf_of" on subscrptions:

s.RawJObject.Value<string>("on_behalf_of")

update: I just checked the latest Stripe.net and I can see that this property is now exposed so I guess we can live without RawJObject 👍

@jar-stripe
Copy link
Contributor

Thanks @clement911 ; I think its still a good idea to try and support it just to make sure we don't break any other usage but I'm glad to hear we now support the fields you need as first class citizens !

@k3davis
Copy link

k3davis commented Nov 12, 2024

To the point made by @clement911 we have no immediate-imminent need for a RawJsonDocument or whatever the STJ equivalent of RawJObject would be, but his use case is still legitimate imo - periodically there is some (temporary) drift between the SDK and the API, so having access to the raw object would still be incredibly useful at times.

@jar-stripe
Copy link
Contributor

Thanks everyone for the feedback so far! I am going to try and and finalize the approach and start implementing tomorrow, so please post your thoughts, feedback, etc before then if you haven't already!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.