Honor default value provided in parameters for route handlers

[amcasey]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

There are a number of ways to make it unnecessary to pass an argument corresponding to a given parameter. We should think about what we want to do in these scenarios with RDG.

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/{purple:int}", HandleRequest);

app.Run();

static string HandleRequest(int purple, int orange = 1) => "Hello World!";
static string HandleRequest(int purple, [System.Runtime.InteropServices.OptionalAttribute] int orange) => "Hello World!";
static string HandleRequest(int purple, params int[] orange) => "Hello World!";

Describe the solution you'd like

I see that parameters can't already be flagged as optional:

// Endpoint Parameter: purple (Type = int, IsOptional = False, IsParsable = True, Source = RouteOrQuery)

Maybe we just need to recognize more forms of optionality?

Additional context

No response

[mitchdenny]

Thanks for bringing this up, it is something I hadn't considered. Just looking into the errors in the generated code now for these scenarios.

[mitchdenny]

OK for the params scenario I don't think we need to worry. The generated code is going to look at the signature for the method and identify that it is an array, and an array is always going to have a value, even if it is empty. I just tested it and the code it emits works fine (I will add some additional tests for it though!).

[mitchdenny]

In the case of this delegate signature:

static string HandleRequest(int purple, [System.Runtime.InteropServices.OptionalAttribute] int orange) => "Hello World!";

Omitting the orange variable fails with a 400 (predictably). So, RDF doesn't appear to do anything special with the [Optional] attribute.

The remaining case (int orange = 1) we don't handle at all - and the generator blows up (@captainsafia heads up that we'll need this captured in the known issues Gist). RDF works fine so its clearly some test cases we need to bring across ;)

[captainsafia]

OK for the params scenario I don't think we need to worry. The generated code is going to look at the signature for the method and identify that it is an array, and an array is always going to have a value, even if it is empty. I just tested it and the code it emits works fine (I will add some additional tests for it though!).

@amcasey had brought up that it might make sense for params to treat the array parameter as optional since that matches the behavior for methods in general (zero, one, or many values are supported). params in inline lambdas was only recently added to C# (I believe it is part of C#12) and while both RDF and RDG respect the array type of the parameter, they don't set a params argument as optional in anyway.

Omitting the orange variable fails with a 400 (predictably). So, RDF doesn't appear to do anything special with the [Optional] attribute.

Yep, don't think this is a scenario we'd support. If we did, it would be via the DataAnnotations attributes and the minimal validation support.

The remaining case (int orange = 1) we don't handle at all - and the generator blows up (@captainsafia heads up that we'll need this captured in the known issues Gist). RDF works fine so its clearly some test cases we need to bring across ;)

Yeah, it looks like this is a result of the fact that code generated by RDF is oblivious to nullability while RDG code is not. I think we should create a separate issue for this to link in the doc. I dug into this a little bit further and it looks like for the given code:

string HandleRequest(int purple, int orange = 1) => "Hello World!";
app.MapGet("/hello", HandleRequest);

We end up generating a parsing block that looks like this:

if (GeneratedRouteBuilderExtensionsCore.TryParseExplicit<int>(orange_temp!, CultureInfo.InvariantCulture, out var orange_temp_parsed_non_nullable))
{
    orange_parsed_temp = orange_temp_parsed_non_nullable;
}
else if (string.IsNullOrEmpty(orange_temp))
{
    orange_parsed_temp = null;
}
else
{
    wasParamCheckFailure = true;
}

With the problematic code in question being:

else if (string.IsNullOrEmpty(orange_temp))
{
    orange_parsed_temp = null;
}

orange_parsed_temp is emitted as a non-nullable int to match the signature of the route handler delegate. I believe the fix is to change this line of code to emit a nullable type for the argument:

aspnetcore/src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

Line 232 in 7b61996

writer.WriteLine($"""{parameter.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)} {outputArgument} = default;""");

using a strategy similar to what we do for the delegate cast:

aspnetcore/src/Http/Http.Extensions/gen/StaticRouteHandlerModel/StaticRouteHandlerModel.Emitter.cs

Lines 36 to 37 in 7b61996

	var parameterTypeList = string.Join(", ", endpoint.Parameters.Select(
	p => p.Type.ToDisplayString(p.IsOptional ? NullableFlowState.MaybeNull : NullableFlowState.NotNull, EmitterConstants.DisplayFormat)));

Edit: Correcting myself. We don't need to use an inline conditional like we do in the delegate cast code. We're already in the IsOptional case:

aspnetcore/src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

Line 228 in 7b61996

if (IsOptional)

So we should always emit _parsed_temp as a nullable type.

[captainsafia]

I ended up addressing this in #47657 since I imported some tests from RDF that were failing because we didn't handle default values.

[mitchdenny]

Thanks @captainsafia

[captainsafia]

I'll keep this open until the PR is merged...

[eerhardt]

@captainsafia - can this be closed now? It is in the "Done" column for both projects, but this issue is still open.