-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Added string arrays to simple binding #63072
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
Conversation
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 change looks like a good start -- can you add some test coverage to RequestDelegateFactoryTests in the repo for this case?
Added unit test for handling string array |
metadata.Name == "formValues") as ParameterBindingMetadata; | ||
|
||
Assert.NotNull(parameterBindingMetadata); | ||
Assert.Equal(typeof(string[]), parameterBindingMetadata.ParameterInfo.ParameterType); |
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.
Can we update the test to send a payload in the HTTP context and verify that context.Items["form"]
is populated correctly?
|
||
await requestDelegate(httpContext); | ||
|
||
Assert.Equal(new StringValues(new[] { "1", "2", "3" }), httpContext.Items["form"]!); |
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.
Assert.Equal(new StringValues(new[] { "1", "2", "3" }), httpContext.Items["form"]!); | |
var parameterBindingMetadata = factoryResult.EndpointMetadata | |
.FirstOrDefault(e => e is ParameterBindingMetadata metadata && | |
metadata.Name == "formValues") as ParameterBindingMetadata; | |
Assert.NotNull(parameterBindingMetadata); | |
Assert.Equal(typeof(string[]), parameterBindingMetadata.ParameterInfo.ParameterType); | |
Assert.Equal(new StringValues(new[] { "1", "2", "3" }), httpContext.Items["form"]!); |
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.
Commited suggestion
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! Thanks for addressing all the feedback points.
@captainsafia, do I need to do something else to merge this pull-request? |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Added string arrays to simple binding
Description
Fixed condition for using a string array with simple binding.
Fixes #62326