-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Remove JsonSerializerOptions copy in ProblemDetailsJsonOptionsSetup #46716
Conversation
src/Http/Http.Extensions/test/ProblemDetailsServiceCollectionExtensionsTest.cs
Outdated
Show resolved
Hide resolved
break; | ||
case not null: | ||
// Combine the current resolver with our internal problem details context (adding last) | ||
options.SerializerOptions.AddContext<ProblemDetailsJsonContext>(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
…xtensionsTest.cs Co-authored-by: Brennan <[email protected]>
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Fixes #46143