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

JsonSerializerOptions should provide explicit control over chained IJsonTypeInfoResolvers #83095

Closed
Tracked by #79122
eiriktsarpalis opened this issue Mar 7, 2023 · 2 comments · Fixed by #83358
Closed
Tracked by #79122
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Mar 7, 2023

Background and motivation

When shipped in .NET 7, the contract customization feature added support for chaining resolvers by means of the JsonTypeInfoResolver.Combine method:

var options = new JsonTypeInfoResolver
{
    TypeInfoResolver = JsonTypeInfoResolver.Combine(ContextA.Default, ContextB.Default, ContextC.Default);
};

Based on feedback we've received, this approach has a couple of usability issues:

  1. It necessitates specifying all chained components at one call site -- resolvers cannot be prepended or appended to the chain after the fact.
  2. Because the chaining implementation is abstracted behind a IJsonTypeInfoResolver implementation, there is no way for users to introspect the chain or remove components from it.

API Proposal

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public IJsonTypeInfoResolver? TypeInfoResolver { get; set; }
+   public IList<IJsonTypeInfoResolver> TypeInfoResolverChain { get; }
}

With this API added, we should also revert the recent change to AddContext in #80698 since it provided an inadequate attempt to address the same underlying issue:

  1. It only allows appending resolvers, prepending is not possible
  2. It only works with JsonSerializerContext, not resolvers in general.
  3. It introduces a breaking change compared to the AddContext method in .NET 7.

API Usage

The new property should complement and be mutually consistent with the existing TypeInfoResolver property. Here are a few examples:

Setting a resolver to TypeInfoResolver

JsonSerializerOptions options = new();
options.TypeInfoResolver = new DefaultJsonTypeInfoResolver();

Assert.Equal(new IJsonTypeInfoResolver[] { options.TypeInfoResolver }, options.ChainedTypeInfoResolvers);

Setting a combined resolver to TypeInfoResolver

JsonSerializerOptions options = new();
options.TypeInfoResolver = JsonTypeInfoResolver.Combine(CtxA.Default, CtxB.Default, CtxC.Default);

Assert.Equal(new IJsonTypeInfoResolver[] { CtxA.Default, CtxB.Default, CtxC.Default }, options.ChainedTypeInfoResolvers);
Assert.Same(options.TypeInfoResolver, options.ChainedTypeInfoResolvers);

Appending a resolver to ChainedTypeInfoResolvers

JsonSerializerOptions options = new();
options.ChainedTypeInfoResolvers.Add(CtxA.Default);
options.ChainedTypeInfoResolvers.Add(CtxB.Default);
options.ChainedTypeInfoResolvers.Add(CtxC.Default);

Assert.Equal(new IJsonTypeInfoResolver[] { CtxA.Default, CtxB.Default, CtxC.Default }, options.ChainedTypeInfoResolvers);
Assert.Same(options.TypeInfoResolver, options.ChainedTypeInfoResolvers);

Prepending a resolver to ChainedTypeInfoResolvers:

JsonSerializerOptions options = new();
DefaultJsonTypeInfoResolver defaultResolver = new();
options.TypeInfoResolver = defaultResolver ;

Assert.Equal(new IJsonTypeInfoResolver[] { defaultResolver }, options.ChainedTypeInfoResolvers);
options.ChainedTypeInfoResolvers.Insert(0, CtxA.Default);
Assert.Equal(new IJsonTypeInfoResolver[] { CtxA.Default, defaultResolver }, options.ChainedTypeInfoResolvers);

cc @brunolins16 @davidfowl @eerhardt

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 7, 2023
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Mar 7, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 7, 2023
@ghost
Copy link

ghost commented Mar 7, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

When shipped in .NET 7, the contract customization feature added support for chaining resolvers by means of the JsonTypeInfoResolver.Combine method:

var options = new JsonTypeInfoResolver
{
    TypeInfoResolver = JsonTypeInfoResolver.Combine(ContextA.Default, ContextB.Default, ContextC.Default);
};

Based on feedback we've received, this approach has a couple of usability issues:

  1. It necessitates specifying all chained components at one call site -- resolvers cannot be prepended or appended to the chain after the fact.
  2. Because the chaining implementation is abstracted behind a IJsonTypeInfoResolver implementation, there is no way for users to introspect the chain or remove components from it.

API Proposal

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public IJsonTypeInfoResolver? TypeInfoResolver { get; set; }
+   public IList<IJsonTypeInfoResolver> ChainedTypeInfoResolvers { get; set; }
}

With this API added, we should also revert the recent change to AddContext in #80698 since it is a breaking change and provides an idaquate attempt to solve the same problem.

API Usage

The new property should complement and be mutually consistent with the existing TypeInfoResolver property. Here are a few examples:

Setting a resolver to TypeInfoResolver

JsonSerializerOptions options = new();
options.TypeInfoResolver = new DefaultJsonTypeInfoResolver();

Assert.Equal(new IJsonTypeInfoResolver[] { options.TypeInfoResolver }, options.ChainedTypeInfoResolvers);

Setting a combined resolver to TypeInfoResolver

JsonSerializerOptions options = new();
options.TypeInfoResolver = JsonTypeInfoResolver.Combine(CtxA.Default, CtxB.Default, CtxC.Default);

Assert.Equal(new IJsonTypeInfoResolver[] { CtxA.Default, CtxB.Default, CtxC.Default }, options.ChainedTypeInfoResolvers);
Assert.Same(options.TypeInfoResolver, options.ChainedTypeInfoResolvers);

Appending a resolver to ChainedTypeInfoResolvers

JsonSerializerOptions options = new();
options.ChainedTypeInfoResolvers.Add(CtxA.Default);
options.ChainedTypeInfoResolvers.Add(CtxB.Default);
options.ChainedTypeInfoResolvers.Add(CtxC.Default);

Assert.Equal(new IJsonTypeInfoResolver[] { CtxA.Default, CtxB.Default, CtxC.Default }, options.ChainedTypeInfoResolvers);
Assert.Same(options.TypeInfoResolver, options.ChainedTypeInfoResolvers);

Prepending a resolver to ChainedTypeInfoResolvers:

JsonSerializerOptions options = new();
DefaultJsonTypeInfoResolver defaultResolver = new();
options.TypeInfoResolver = defaultResolver ;

Assert.Equal(new IJsonTypeInfoResolver[] { defaultResolver }, options.ChainedTypeInfoResolvers);
options.ChainedTypeInfoResolvers.Insert(0, CtxA.Default);
Assert.Equal(new IJsonTypeInfoResolver[] { CtxA.Default, defaultResolver }, options.ChainedTypeInfoResolvers);

Alternative Designs

No response

Risks

No response

Author: eiriktsarpalis
Assignees: -
Labels:

api-suggestion, area-System.Text.Json

Milestone: 8.0.0

@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 7, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Mar 7, 2023
@eiriktsarpalis eiriktsarpalis added the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 8, 2023
@terrajobst
Copy link
Member

terrajobst commented Mar 9, 2023

Video

  • Should be get-only
  • Should we rename it to just TypeInfoResolverChain?
namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public IList<IJsonTypeInfoResolver> TypeInfoResolverChain { get; }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 9, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 13, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants