dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.4k stars 10k forks source link

Improve IContractResolver usage in Json Patch library #2972

Closed HappyNomad closed 5 years ago

HappyNomad commented 6 years ago

The Json.Net contract resolver that's first passed to JsonPatchDocument's constructor is then passed among numerous methods and objects throughout the code. Three problems relate to this.

  1. The serializer's contract resolver is often not the one passed in. When JsonPatchDocument's parameterless constructor is called, it defaults to passing new DefaultContractResolver() for the contract resolver. Even if the library user provides one via another constructor, he'll probably opt for the easy new DefaultContractResolver() instead of pulling one from the serializer. As for patch docs deserialized by Asp.Net Core, JsonPatchInputFormatter only works for JsonPatchDocument's generic version due to this line.
  2. JsonPatchDocument constructors don't allow a null contract resolver. The parameterless constructor will use new DefaultContractResolver(). The constructor that accepts a contract resolver throws ArgumentNullException if it's null. This doesn't make sense for scenarios that never use a contract resolver. None of my scenarios require a contract resolver, and usages in the library could be replaced by reflection or left to customization by other means. For example, the usage here can be replaced with targetObject is and reflection. It's used to allow customization of how a path segment resolves to a dictionary key here and allow customization of how a path segment resolves to a dynamic property name here. Those uncommon customizations can be accomplished by other means like modifying the path segments before calling ApplyTo or customizing the overall ApplyTo behavior. That property usage is also haphazard since it's used there but not here. Anyway, I digress.
  3. The contract resolver appears as a parameter in all of IAdapter's methods and their implementations. This makes the various method signatures of IAdapter and its implementations unnecessarily verbose, and thus harder to understand and cumbersome to use.

I propose solving these problems as follows.

  1. Pass in the serializer's contract resolver in JsonPatchDocumentConverter. Then eliminate JsonPatchInputFormatter too, which doesn't work for untyped JsonPatchDocuments anyway.
  2. Make optional the contract resolver parameter of JsonPatchDocument's constructor. Don't throw an exception if it's null.
  3. Eliminate all those IContractResolver method parmaters in IAdapter. Instead, have each IAdapter implementation that wants to use a contract resolver just have its constructor accept it.
mkArtakMSFT commented 5 years ago

Hi. Thanks for contacting us. We're closing this issue as there was no much community interest in this ask for quite a while now.