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.

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

DisableInferBodyFromParameters in RDG. #48269

Merged
merged 7 commits into from
May 23, 2023
Merged

DisableInferBodyFromParameters in RDG. #48269

merged 7 commits into from
May 23, 2023

Conversation

mitchdenny
Copy link
Member

Addresses #47239

@mitchdenny mitchdenny self-assigned this May 17, 2023
@mitchdenny mitchdenny added this to the 8.0-preview5 milestone May 17, 2023
captainsafia
captainsafia approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

I think we can get away with doing the lookup per request. We do the lookup once at compile-time for RDF but I think the impact is negligible.

@mitchdenny mitchdenny force-pushed the disable-infer-body-from-parameters-support branch from 8b40fa5 to 9f4d4c6 Compare May 23, 2023 03:53
@mitchdenny mitchdenny merged commit 6c49e7b into dotnet:main May 23, 2023
davidfowl
davidfowl reviewed May 23, 2023
View reviewed changes
...st/RequestDelegateGenerator/Baselines/MapAction_ImplicitQuery_StringArrayParam.generated.txt
}
try
{
var bodyValue = await httpContext.Request.ReadFromJsonAsync<T>();
Copy link
Member

Choose a reason for hiding this comment

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

@captainsafia Why aren't we passing the JSON type info here?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch
Context for why: #45359

Copy link
Member

Choose a reason for hiding this comment

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

Context for why: #45359

Are you identifying the perf ramifications of querying the DI container for the JsonSerializerOptions to resolve the type info on every request? If so, that shouldn't be a problem here since we can cache the type info for the parameter type in the same way that we do for the response type.

@BrennanConroy BrennanConroy mentioned this pull request May 31, 2023
Merged
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@captainsafia captainsafia captainsafia approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

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

Assignees

@mitchdenny mitchdenny

Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg
Projects
None yet
Milestone
8.0-preview5
Development

Successfully merging this pull request may close these issues.
5 participants
@mitchdenny @davidfowl @captainsafia @BrennanConroy @amcasey