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

Obsolete JsonSerializerOptions.AddContext #83280

Closed
Tracked by #79122
eiriktsarpalis opened this issue Mar 10, 2023 · 2 comments · Fixed by #84022
Closed
Tracked by #79122

Obsolete JsonSerializerOptions.AddContext #83280

eiriktsarpalis opened this issue Mar 10, 2023 · 2 comments · Fixed by #84022
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Mar 10, 2023

Background & Motivation

The JsonSerializerOptions.AddContext<TContext>() method was introduced in .NET 6 as a means to associate JsonSerializerOptions instances with a given JsonSerializerContext type. This method was largely superseded in .NET 7 with the introduction of contract customization and the JsonSerializerOptions.TypeInfoResolver property.

We should obsolete AddContext for the following reasons:

  1. Contrary to what the name suggests, it replaces user configuration instead of appending to it.
  2. It can be confused with the new JsonSerializerOptions.TypeInfoResolverChain when manipulating resolver chains.
  3. The method only works with JsonSerializerContext type parameters, not with arbitrary IJsonTypeInfoResolver instances.
  4. The method freezes the JsonSerializerOptions for further modification:
    JsonSerializerOptions options = new();
    options.AddContext<Context1>(); // success
    options.AddContext<Context2>(); // InvalidOperationException

API Proposal

public class JsonSerializerOptions
{
     [Obsolete(/* SYLIB code TBD? */)]
     public void AddContext<TContext>() : where TContext : JsonSerializerContext { }
}

cc @eerhardt

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 10, 2023
@ghost
Copy link

ghost commented Mar 10, 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 & Motivation

The JsonSerializerOptions.AddContext<TContext>() method was introduced in .NET 6 as a means to associate JsonSerializerOptions instances with a given JsonSerializerContext type. This method was largely superseded in .NET 7 with the introduction of contract customization and the JsonSerializerOptions.TypeInfoResolver property.

We should obsolete AddContext for the following reasons:

  1. Contrary to what the name suggests, it replaces user configuration instead of appending to it.
  2. It can be confused with the new JsonSerializerOptions.TypeInfoResolverChain when manipulating resolver chains.
  3. The method only works with JsonSerializerContext type parameters, not with arbitrary IJsonTypeInfoResolver instances.
  4. The method freezes the JsonSerializerOptions for further modification:
    JsonSerializerOptions options = new();
    options.AddContext<Context1>(); // success
    options.AddContext<Context2>(); // InvalidOperationException

API Proposal

public class JsonSerializerOptions
{
     [Obsolete(/* SYLIB code TBD? */)]
     public void AddContext<TContext>() : where TContext : JsonSerializerContext
}
Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

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

Looks good as proposed.

Confirm with @terrajobst that System.Text.Json should use SYSLIB codes.

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 16, 2023
eerhardt added a commit to eerhardt/aspnetcore that referenced this issue Mar 27, 2023
- Removes all usages of JsonSerializerOptions.AddContext, which will be obsoleted by dotnet/runtime#83280
- Update ProblemDetailsJsonContext to be added just before the DefaultJsonTypeInfoResolver if it is the last resolver in the chain. Otherwise, add it to the end of a non-empty resolver chain.
- Small clean up in the API template's usings
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 28, 2023
eerhardt added a commit to dotnet/aspnetcore that referenced this issue Mar 29, 2023
* Use Json TypeInfoResolverChain

- Removes all usages of JsonSerializerOptions.AddContext, which will be obsoleted by dotnet/runtime#83280
- Update ProblemDetailsJsonContext to be added in a Configure<JsonOptions>() call back and to always be added to the beginning of the resolver chain at that time
    - This gives us the simplest, most understandable pattern for all libraries to follow.
- Small clean up in the API template's usings

* Change ProblemDetailsJsonOptionsSetup to use Configure instead of PostConfigure.

When adding the ProblemDetailsJsonContext, we always prepend it to the beginning of the chain at the time the configure step is executed, no matter the state of the chain.

This allows for a simpler, more understandable policy for all libraries that want to add their JsonContext into the JsonSerializerOptions resolver chain. The order of the chain is now determined by the order that the configure steps were registered in DI (for example when AddProblemDetails() was called).
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants