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

[AOT] Fixing the problems with ProblemDetails #45646

Merged
merged 9 commits into from
Dec 20, 2022

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Dec 17, 2022

ProblemDetails.Extensions is problematic. Dictionary<string, object> can contain any value, and the type needs to be serializable.

Simply annotating all places that use ProblemDetails.Extensions as unsafe isn't a good solution. The developer exception middleware serializes problem details with extensions, and the middleware is extremely commonly used and is registered by default in WebApplication:

app.UseDeveloperExceptionPage();

This PR tries to find a pragmatic middle ground that warns people about using extensions, but also makes some values safe so a user such as developer exception middleware can use it safely.

A short summary of what this PR tries to do:

  • Warn when someone uses extensions
  • Have a list of types that will work as extension values
  • Update usage of extensions in aspnetcore to use compatible types.

Note: this PR builds on #45604. I split it out to make it clearer what changes are focused on ProblemDetails.

Changes in PR:

  1. ProblemDetails.Extensions property now has warnings.
  2. Warnings in converters are suppressed. These converters are in [JsonConverter] attributes on the types, so are never referenced in user code anyway.
  3. Serialize/deserialize Dictionary<string, string[]> Errors { get; } in HttpValidationProblemDetailsJsonConverter manually. It's not difficult and avoids the problem of calling generated serialization inside a converter.
  4. DeveloperExceptionPageMiddlewareImpl adds an anonymous type to extensions. This will never serialize, so it's been changed to a real type with a corresponding source generated serializer. It's converted into a JsonElement which is added to extensions. The problem details writer supports JsonElement extension values.

@JamesNK JamesNK added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Dec 17, 2022
@JamesNK JamesNK requested a review from a team as a code owner December 17, 2022 23:31
// Use source generation serialization in two scenarios:
// 1. There are no extensions. Source generation is faster and works well with trimming.
// 2. Native AOT. In this case only the data types specified on ProblemDetailsJsonContext will work.
if (context.ProblemDetails.Extensions is { Count: 0 } || !RuntimeFeature.IsDynamicCodeSupported)
Copy link
Member Author

Choose a reason for hiding this comment

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

@eerhardt @jkotas @vitek-karas @davidfowl Like #45604 (comment), this is a place where dotnet/runtime#39806 would be useful.

Developers being able to run the "AOT path" while writing and debugging their app would help them find incompatible extension values during development instead of when they publish as AOT for production.

Copy link
Member

Choose a reason for hiding this comment

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

Would it fail to serialize if we did that in "build"? Or would the reflection based serializer kick in?

Copy link
Member

Choose a reason for hiding this comment

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

It would fail at runtime. This is what we're going to be doing manually today for AOT based JSON in ASP.NET Core. The reflection fallback would be disabled and our calls to resolve the JsonTypeInfo would return null.

Copy link
Member

Choose a reason for hiding this comment

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

In that case - let's do dotnet/runtime#39806.
We just need to "hide" it enough so that people don't use it as the first option to solve AOT problems (still better than current behavior, but ideally we want them to really fix things)

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks good to me. I just had 2 nit comments.

It may be a good idea to get some AOT tests set up in the repo in a future PR. That way we can validate our workarounds for suppressions work correctly.

src/Shared/ProblemDetails/ProblemDetailsJsonConverter.cs Outdated Show resolved Hide resolved
// Use source generation serialization in two scenarios:
// 1. There are no extensions. Source generation is faster and works well with trimming.
// 2. Native AOT. In this case only the data types specified on ProblemDetailsJsonContext will work.
if (context.ProblemDetails.Extensions is { Count: 0 } || !RuntimeFeature.IsDynamicCodeSupported)
Copy link
Member

Choose a reason for hiding this comment

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

This would probably be a good candidate for an "AOT-specific" test, similar to how we test trimming and AOT in dotnet/runtime. For example: https://github.com/dotnet/runtime/tree/main/src/libraries/System.Diagnostics.DiagnosticSource/tests/NativeAotTests.

@JamesNK JamesNK merged commit 3de17af into jamesnk/Aot Dec 20, 2022
@JamesNK JamesNK deleted the jamesnk/Aot-ProblemDetails branch December 20, 2022 14:42
@davidfowl
Copy link
Member

FYI @eerhardt, I spoke to @JamesNK about this offline and I think we'll end up rewriting this implementation as I think there's a better way to solve this problem given how we're approaching JSON generally across the stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants