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

Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup #46716

Merged
merged 3 commits into from
Mar 1, 2023

Conversation

brunolins16
Copy link
Member

Fixes #46143

@brunolins16 brunolins16 requested a review from a team February 16, 2023 22:59
@ghost ghost added the area-runtime label Feb 16, 2023
brunolins16 added a commit to brunolins16/aspnetcore that referenced this pull request Feb 16, 2023
break;
case not null:
// Combine the current resolver with our internal problem details context (adding last)
options.SerializerOptions.AddContext<ProblemDetailsJsonContext>();
Copy link
Member

Choose a reason for hiding this comment

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

This is broken when someone calls AddContext before us right? So it'll be:
custom context -> default context -> problem details context

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean because of https://github.com/brunolins16/aspnetcore/blob/9bb6ab75a30e65e4c530d647a7798c1a96b3e92e/src/Http/Http.Extensions/src/JsonOptions.cs#L37, right? If so, it will actually be:

default context -> custom context -> problem details context

Unless someone called the Combine api, JsonTypeInfoResolver.Combine(MyCustomContext.Default, options.SerializerOptions.TypeInfoResolver);, to prepend instead of append

This is kind of the issue we are discussing with S.T.J team and I am trying to fix here #46490

@brunolins16
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@brunolins16 brunolins16 merged commit 572993c into dotnet:main Mar 1, 2023
@ghost ghost added this to the 8.0-preview3 milestone Mar 1, 2023
@brunolins16 brunolins16 deleted the brunolins16/issues/pd/46143 branch March 1, 2023 18:12
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup
5 participants