-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Only invoke 'IsService' check once in resolution #47097
Conversation
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/gen/RequestDelegateGeneratorSources.cs
Outdated
Show resolved
Hide resolved
{ | ||
foreach (var parameter in endpoint.Parameters) | ||
{ | ||
if (parameter.Source == EndpointParameterSource.JsonBodyOrService) | ||
{ | ||
codeWriter.WriteLine("var serviceProviderIsService = options?.ServiceProvider?.GetService<IServiceProviderIsService>();"); | ||
codeWriter.Write($@"var {parameter.Name}_JsonBodyOrServiceResolver = "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This includes the parameter name in the variable name. What is the parameter name here? Is it the C# method parameter name or the route parameter name? Need to be careful about using the route name because it could contain invalid C# name characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this, and I believe the parameter name here can be overridden by something like [FromRoute(Name = "1")]
. The generated source wouldn't compile because a variable can't start with a number.
Consider using the parameter index instead, e.g. param1_JsonBodyOrServiceResolver
Edit: Issue #47129. Don't need to fix it in this PR, but afterward, someone should test if it's a problem or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be based on the parameter name even if it can be overridden. That should just change the lookup key, not the variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'd like someone who's spent more time in this space to also review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Safia and I looked at this together yesterday when looking at dealing with some issues related to array handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mitchdenny. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
Follow-up to #47079 (comment).
Closes #47224.