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

Support optionality via nullability and default values #34505

Merged
merged 8 commits into from
Jul 26, 2021

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Jul 19, 2021

  • Adds codegen to validate required parameter if they are not nullable or don't have a default value
  • Allows developers to use [FromBody(AllowEmpty = true)] to override required parameters defined in method
  • Favors implicit services over body parameters when a non-string or non-parseable argument is typed as optional

Fixes #32375

@ghost ghost added the area-runtime label Jul 19, 2021
@captainsafia captainsafia force-pushed the safia/optional-params branch from 1eb4b65 to c746a3a Compare July 20, 2021 00:38
@captainsafia captainsafia marked this pull request as ready for review July 20, 2021 16:55
@captainsafia captainsafia requested review from halter73, davidfowl and pranavkm and removed request for Tratcher and BrennanConroy July 20, 2021 16:58
factoryContext.JsonRequestBodyType = parameter.ParameterType;
factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional;

var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local");
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Can't we just null-check BodyValueExpr?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need this because the way the try-parse logic is generated. From what I've been able to reason from removing this argument and tracing through the code, the try-parse logic assembles a code block that looks like this when mapped to a lambda.

(target, httpContext, bodyValue) => {
  (param1_local, param2_local) {
    // if statements for validation here
   // check tryParseFailedValueHere
   // call action here
  }
}

If we try to do something like:

(target, httpContext, bodyValue) => {
  (param1_local, bodyValue) {
    // if statements for validation here
   // check tryParseFailedValueHere
   // call action here
  }
}

Then the original value of bodyValue is shadowed and set to null so the null-check doesn't behave as we'd expect.

Copy link
Member

Choose a reason for hiding this comment

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

The try-parse logic shouldn't touch bodyValue since the body is never TryParsed.

Copy link
Member Author

Choose a reason for hiding this comment

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

try-parse logic

try-parse here refers to the expression blocks that are added to the TryParseParams list and executed. Without this argument the checkBodyParamRequired expression would've shadowed bodyValue.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused here too. The TryParse should never matter for the body argument.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to check if the body is null after casting, and don't want to cast twice, storing the casted value in the local helps. Not sure if that's necessary, but one extra local doesn't seem that bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a bit confused here too. The TryParse should never matter for the body argument.

Previously, it didn't. This PR updates the TryParse logic to be a generic codepath for validating parameters so now we are using it for body arguments.

If you want to check if the body is null after casting, and don't want to cast twice, storing the casted value in the local helps. Not sure if that's necessary, but one extra local doesn't seem that bad.

Yeah, it's not the worse. Can you validate my understanding of what is happening mentioned in #34505 (comment)?

@captainsafia captainsafia force-pushed the safia/optional-params branch 2 times, most recently from f6e1d0c to d566d30 Compare July 21, 2021 18:02
@captainsafia captainsafia force-pushed the safia/optional-params branch from d566d30 to b2333d7 Compare July 21, 2021 18:21
src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved
src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved
src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved
factoryContext.JsonRequestBodyType = parameter.ParameterType;
factoryContext.AllowEmptyRequestBody = allowEmpty || isOptional;

var argument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local");
Copy link
Member

Choose a reason for hiding this comment

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

If you want to check if the body is null after casting, and don't want to cast twice, storing the casted value in the local helps. Not sure if that's necessary, but one extra local doesn't seem that bad.

src/Http/Http.Extensions/src/RequestDelegateFactory.cs Outdated Show resolved Hide resolved
@captainsafia captainsafia merged commit cb4ceb8 into main Jul 26, 2021
@captainsafia captainsafia deleted the safia/optional-params branch July 26, 2021 20:17
@ghost ghost added this to the 6.0-rc1 milestone Jul 26, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use nullibility to infer optionality of service and body parameters
6 participants