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

Stop Calling JSON GetTypeInfo with the runtime type #47859

Merged
merged 6 commits into from
Apr 25, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Apr 24, 2023

Calling GetTypeInfo with the object's runtime type doesn't work when using unspeakable types and using JSON source generation. There is no way to mark the unspeakable type (like a C# compiler implemented IAsyncEnumerable iterator) as JsonSerializable.

Instead, when the "declared type"'s JsonTypeInfo isn't compatible with the runtime type w.r.t. polymorphism, serialize the value "as object", letting System.Text.Json's serialization take over to serialize the value.

Fix #47548

@eerhardt eerhardt requested a review from eiriktsarpalis April 24, 2023 16:32
@eerhardt eerhardt added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Apr 24, 2023
Calling GetTypeInfo with the object's runtime type doesn't work when using unspeakable types and using JSON source generation. There is no way to mark the unspeakable type (like a C# compiler implemented IAsyncEnumerable iterator) as JsonSerializable.

Instead, when the "declared type"'s JsonTypeInfo isn't compatible with the runtime type w.r.t. polymorphism, serialize the value "as object", letting System.Text.Json's serialization take over to serialize the value.

Fix dotnet#47548
@eerhardt eerhardt force-pushed the FixGetTypeInfoOnUnspeakableTypes branch from c8e1046 to c771a2a Compare April 25, 2023 00:34
@eerhardt eerhardt marked this pull request as ready for review April 25, 2023 00:51
@eerhardt eerhardt requested review from JamesNK and mitchdenny April 25, 2023 00:51
@@ -108,9 +109,7 @@ public static partial class HttpResponseJsonExtensions
/// <param name="contentType">The content-type to set on the response.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param>
/// <returns>The task object representing the asynchronous operation.</returns>
#pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters
Copy link
Member

Choose a reason for hiding this comment

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

Why do these no longer apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

IDK. It builds without them... 🤷

src/Http/Http.Results/src/HttpResultsHelper.cs Outdated Show resolved Hide resolved
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

LGTM! TY for migrating the test over.

Remove redundant JsonSerializerOptions.
@eerhardt eerhardt requested a review from javiercn as a code owner April 25, 2023 16:22
Rename JsonTypeInfo extension method IsValid to ShouldUseWith.
@eerhardt
Copy link
Member Author

OK, I have addressed all feedback. I'm going to merge this once it is green to make the 8.0-preview4 cutoff.

@eerhardt eerhardt enabled auto-merge (squash) April 25, 2023 18:36
@eerhardt eerhardt merged commit 08f6eea into dotnet:main Apr 25, 2023
@ghost ghost added this to the 8.0-preview4 milestone Apr 25, 2023
eerhardt added a commit to eerhardt/aspnetcore that referenced this pull request Apr 26, 2023
This was missed in dotnet#47859 and is causing trimming/AOT warnings from generated code.
eerhardt added a commit that referenced this pull request Apr 26, 2023
This was missed in #47859 and is causing trimming/AOT warnings from generated code.
github-actions bot pushed a commit that referenced this pull request Apr 26, 2023
This was missed in #47859 and is causing trimming/AOT warnings from generated code.
wtgodbe pushed a commit that referenced this pull request Apr 27, 2023
This was missed in #47859 and is causing trimming/AOT warnings from generated code.

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@eerhardt eerhardt deleted the FixGetTypeInfoOnUnspeakableTypes branch April 28, 2023 19:23
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.

Stop Calling JSON GetTypeInfo with the runtime type
3 participants