Skip to content

Adding support for custom JsonConverter(s) when deserializing JSON Patch value object #30974

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

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

onegoodsausage
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

PR Title
Adding support for custom JsonConverter(s) when deserializing JSON Patch value object

PR Description
When we started using the JSON Patch library we hit an issue with heterogenous collections. To specify what type an object in a heterogenous collection is, we added a "Type" property to the JSON value object. However, regardless of what we supplied in our custom ContractResolver, nothing was getting used when the JObject was converted to a concrete object.

Once we looked at the sources we found out that the JsonPatchDocument.ContractResolver is not used at all when deserializing the value object. So we added an overload that takes an IContractResolver and wired it up for the ListAdapter.

That solved our issue as now our custom JsonConverter is used to read the "Type" property and create the correct object instance. We also added an integration unit test that covers this scenario.

onegoodsausage and others added 3 commits March 10, 2021 08:35
…kes an IContractResolver and hooking it up for ListAdapter

- Unit test to verify above works for heterogenous collections
…viderUsesContractResolver

Andremi/conversion result provider uses contract resolver
@dnfadmin
Copy link

dnfadmin commented Mar 16, 2021

CLA assistant check
All CLA requirements met.

@pranavkm pranavkm added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews community-contribution Indicates that the PR has been added by a community member labels Mar 16, 2021
@ghost
Copy link

ghost commented Mar 16, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm pranavkm added this to the 6.0-preview3 milestone Mar 16, 2021
@pranavkm pranavkm requested a review from javiercn March 16, 2021 16:34
@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 16, 2021
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

How do you feel about this? It avoids an additional public API

@pranavkm pranavkm merged commit dec7382 into dotnet:main Mar 17, 2021
@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants