Skip to content

Unify JSON handling across RDF and RDG #48573

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 14, 2023
Merged

Unify JSON handling across RDF and RDG #48573

merged 4 commits into from
Jun 14, 2023

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Jun 1, 2023 •
edited
Loading

RDG currently resolves the JsonTypeInfo when deserializing a request body parameter once per request. This applies an update to the codegen so that the JsonTypeInfo is resolved once at delegate construction and then re-used for each request.

RDG's implementation for response writing is currently slightly different from what RDF uses. There's a work item to make a public API (#47973) here but for now we unify the two implementations.

@ghost ghost added the area-runtime label Jun 1, 2023
@captainsafia captainsafia mentioned this pull request Jun 1, 2023
Merged
@captainsafia captainsafia changed the title Reslove JsonTypeInfo<T> once per request body parameter Unify JSON handling across RDF and RDG Jun 2, 2023
@captainsafia captainsafia added feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
@captainsafia captainsafia requested a review from eerhardt June 2, 2023 21:48
eerhardt
eerhardt reviewed Jun 2, 2023
View reviewed changes
...Http/Http.Extensions/test/RequestDelegateGenerator/RequestDelegateCreationTests.Responses.cs Outdated
@@ -276,7 +277,7 @@ async ValueTask<Todo> ValueTaskTestActionAwaited()
}
}

[Theory]
[ConditionalTheory(Skip = "https://github.com/dotnet/runtime/issues/87073")]
Copy link
Member

Choose a reason for hiding this comment

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

Your change is regressing this scenario because we are now ALWAYS calling GetTypeInfo(typeof(object))? Is that correct? (this test is passing in main) Maybe we shouldn't be doing this change if it is going to regress scenarios. I just thought you were hitting issues in new tests you were converting (for filters).

Copy link
Member Author

Choose a reason for hiding this comment

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

Your change is regressing this scenario because we are now ALWAYS calling GetTypeInfo(typeof(object))? Is that correct?

Yes, although it's only regressing the scenario when a JsonContext is used.

Maybe we shouldn't be doing this change if it is going to regress scenarios.

I think we should still do it, but I'm comfortable waiting until the bug fix in the runtime is merged to merge this in to our repo.

I just thought you were hitting issues in new tests you were converting (for filters).

The filters scenario was to showcase that the bug was happening in RDF as well and wasn't a side-effect of RDG's codegen, but as we eventually realized, any invocation of GetTypeInfo(typeof(object)) will trigger this at the moment.

@captainsafia captainsafia mentioned this pull request Jun 6, 2023
Merged
@captainsafia captainsafia force-pushed the safia/read-json-type-info branch from b0644fb to 1a5e6bb Compare June 8, 2023 21:43
@captainsafia captainsafia force-pushed the safia/read-json-type-info branch from 1a5e6bb to fba3643 Compare June 9, 2023 16:29
eerhardt
eerhardt approved these changes Jun 12, 2023
View reviewed changes
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.

LGTM. Just a couple small nits/questions

...DelegateGenerator/Baselines/MapAction_ExplicitBodyParam_ComplexReturn_Snapshot.generated.txt Outdated
var runtimeType = value?.GetType();
if (runtimeType is null || jsonTypeInfo.Type == runtimeType || jsonTypeInfo.PolymorphismOptions is not null)
{
return httpContext.Response.WriteAsJsonAsync(value!, jsonTypeInfo);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return httpContext.Response.WriteAsJsonAsync(value!, jsonTypeInfo);
return httpContext.Response.WriteAsJsonAsync(value, jsonTypeInfo);

This seems wrong because when value is null, we get here.

Maybe the right fix is to take a JsonTypeInfo<T?> jsonTypeInfo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I tweaked the nullability in a9b8d8b

@captainsafia captainsafia merged commit ce696ca into main Jun 14, 2023
@captainsafia captainsafia deleted the safia/read-json-type-info branch June 14, 2023 21:47
@ghost ghost added this to the 8.0-preview6 milestone Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BrennanConroy BrennanConroy BrennanConroy left review comments

@eerhardt eerhardt eerhardt approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees
No one assigned
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg
Projects
None yet
Milestone
8.0-preview6
Development

Successfully merging this pull request may close these issues.
3 participants
@captainsafia @BrennanConroy @eerhardt