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

Fix code generation for parsing optional array parameters in RequestDelegateGenerator #57336

Merged
merged 8 commits into from
Sep 23, 2024

Conversation

ithline
Copy link
Contributor

@ithline ithline commented Aug 14, 2024

Fix code generation for parsing optional array parameters in RequestDelegateGenerator

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

This PR aims to fix the invalid code generated for optional arrays in RequestDelegateGenerator (RDG), where the generated code tried to assign single element value into array variable.

The EndpointParameter.ParsingBlockEmitter property was lifted out of EndpointParameter.TryGetParsability into EndpointParameterEmitter.EmitParsingBlock method, to be able to properly condition all the parsing paths.

I added 4 tests to cover the code path, but commented out VerifyAgainstBaselineUsingFile check. I can add them, but will need guidance on how to use this part of test infrastructure.

This PR also contains a simple fix to SupportsSingleKeyedServiceWithPrimitiveKeyTypes test, that failed to compile on my machine due to culture specific handling of decimal point. I replaced simple .ToString() call with Convert.ToString({value}, CultureInfo.InvariantCulture) to force the value to be consistent across cultures.

Fixes #57175

…pointParameterEmitter.EmitParsingBlock` to be able to properly emit parsing logic for optional arrays.
…l point, because string interpolation made `12.3` as `12,3` failing the compilation.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 14, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 14, 2024
@ithline
Copy link
Contributor Author

ithline commented Aug 14, 2024

@dotnet-policy-service agree

align array creation more closely with the reflection delegate factory
@captainsafia
Copy link
Member

captainsafia commented Aug 20, 2024

I added 4 tests to cover the code path, but commented out VerifyAgainstBaselineUsingFile check. I can add them, but will need guidance on how to use this part of test infrastructure.

This is our bespoke snapshot testing infrastructure. In order to use it, you'll have to add the call to the target unit test method, set this property in the test base class to true, run the test suite, reset the value of the property, then run the test suite again to observe the resulting behavior without this Assert.Fail spoiling your fun.

Since we create a snapshot for each test method, I like to create a single test method that contains a test source with multiple endpoints with the conditions to test.

This PR also contains a simple fix to SupportsSingleKeyedServiceWithPrimitiveKeyTypes test, that failed to compile on my machine due to culture specific handling of decimal point. I replaced simple .ToString() call with Convert.ToString({value}, CultureInfo.InvariantCulture) to force the value to be consistent across cultures.

Thanks for this fix! We've got a few of these kinds of issues in the codebase since our team/CI machines primarily use an en-US culture. We should probably figure out a way to be resilient to this that doesn't rely on people with different locales running the test suite... 😅

@ithline
Copy link
Contributor Author

ithline commented Aug 22, 2024

@captainsafia Alright, I've added the missing baseline tests and this PR should be ready for review.

@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Aug 22, 2024
@dotnet-policy-service dotnet-policy-service bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 22, 2024

await endpoint.RequestDelegate(httpContext);
await VerifyResponseBodyAsync(httpContext, "[]");
await VerifyAgainstBaselineUsingFile(compilation);
Copy link
Member

Choose a reason for hiding this comment

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

We can probably exclude the baseline here since the generated code between MapAction_ExplicitQuery_IntArrayParam_Optional_QueryNotPresent and MapAction_ExplicitQuery_IntArrayParam_Optional is the same.

Alternatively, we could do one test method that uses a [Theory] to test the scenario where a query is present and the one where it is not.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Aug 29, 2024
@captainsafia
Copy link
Member

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 5, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mkArtakMSFT mkArtakMSFT removed the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Sep 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 17, 2024
@captainsafia captainsafia removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 17, 2024
@captainsafia captainsafia enabled auto-merge (squash) September 17, 2024 09:15
@captainsafia captainsafia merged commit 0da45ac into dotnet:main Sep 23, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview1 milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member feature-rdg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RequestDelegateGenerator generates invalid code for optional array parameters
5 participants