Fix DefaultProblemDetailsWriter polymorphism #45906
Conversation
ghost
added
the
brunolins16
added
old-area-web-frameworks-do-not-use
brunolins16
changed the title
brunolins16
requested review from
Tratcher,
BrennanConroy and
halter73
as code owners
src/Http/Http.Extensions/test/ProblemDetailsDefaultWriterTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Halter <[email protected]>
src/Http/Http.Extensions/src/ProblemDetailsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/src/ProblemDetailsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
| { | ||
| _options = options.Value; | ||
| _serializerOptions = new JsonSerializerOptions(jsonOptions.Value.SerializerOptions); | ||
|
|
||
| if (_serializerOptions.TypeInfoResolver is not null) |
There was a problem hiding this comment.
What happens if TypeInfoResolver is null?
There was a problem hiding this comment.
In non-aot scenario it will use the reflection-based resolver, so, we don't really need to include our internal context. In AOT scenario, unless your application only serializes Problem Details, it should not be null and if it is null not adding our internal context seems might be bad but probably it will fail elsewhere first. Make sense?
There was a problem hiding this comment.
But it seems that if TypeInfoResolver is null and you're in AOT, then this type can't work.
Should there be an error if IsDynamicCodeSupported is false (which means context is required) and TypeInfoResolver is null?
Either way, I think a comment here would be useful for future devs.
There was a problem hiding this comment.
Interesting idea. Do you think error is enough or should we set our context in this case? I believe setting our context could hide the configuration issue and make it harder to understand.
There was a problem hiding this comment.
I don't know. If the problem is configuration, then IMO throwing here with a helpful error message is better than pretending things are ok and then later failing when serialization happens with an obscure error message.
There was a problem hiding this comment.
@JamesNK I have been thinking about this and maybe we shouldn't throw during the configuration because it could be too earlier to make sure a problem details will not be able to be serializer. I am saying that because in a situation where the user added a custom IProblemDetailsWriter that could serializer a ProblemDetails in a different format (eg, XML) throwing because the JsonContext was not configured correctly might be wrong. What do you think?
There was a problem hiding this comment.
I agree with @brunolins16, I don't think we should throw because the TypeInfoResolver is null. That shouldn't come from this code. Feels like something more top level and central as a whole. Its part of the "how does the user know they need to set a context" in the first place.
src/Http/Http.Extensions/src/ProblemDetailsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
…lins16/aspnetcore into brunolins16/issues/45680-2
| [JsonSerializable(typeof(HttpValidationProblemDetails))] | ||
| // Additional values are specified on JsonSerializerContext to support some values for extensions. | ||
| // For example, the DeveloperExceptionMiddleware serializes its complex type to JsonElement, which problem details then needs to serialize. | ||
| [JsonSerializable(typeof(JsonElement))] |
There was a problem hiding this comment.
This should be removed eventually. Can you file an issue for this?
Fix #45680