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

Use nullibility to infer optionality of service and body parameters #32375

Closed
halter73 opened this issue May 4, 2021 · 5 comments · Fixed by #34505
Closed

Use nullibility to infer optionality of service and body parameters #32375

halter73 opened this issue May 4, 2021 · 5 comments · Fixed by #34505
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc blocked The work on this issue is blocked due to some dependency enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing help wanted Up for grabs. We would accept a PR to help resolve this issue Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@halter73
Copy link
Member

halter73 commented May 4, 2021

app.MapPost("/todos", (Todo? todo) => { 
// ...
app.MapGet("/something", (IService? service) => { 
// ...

In cases like the above, we could use the nullability of the Todo and IService parameters to determine weather to request body and service are optional.

See #31658 (comment)

@halter73 halter73 added area-runtime feature-minimal-actions Controller-like actions for endpoint routing enhancement This issue represents an ask for new feature or an enhancement to an existing one labels May 4, 2021
@halter73 halter73 added this to the Next sprint planning milestone May 4, 2021
@ghost
Copy link

ghost commented May 4, 2021

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl
Copy link
Member

davidfowl commented May 4, 2021

Also detect default values:

app.MapPost("/todos", (Todo todo = null) => 
{ 
});

Essentially signals from the type system that the user is willing to tolerate null.

@halter73 halter73 added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 11, 2021
@JamesNK JamesNK self-assigned this May 18, 2021
@JamesNK JamesNK added the blocked The work on this issue is blocked due to some dependency label May 18, 2021
@JamesNK
Copy link
Member

JamesNK commented May 18, 2021

Required reflection metadata about whether parameters have default values and are nullable is not available on private and compiler-generated methods.

Also semi-blocked on dotnet/runtime#29723 (having this API is nice, but we can do this ourselves if API is not available)

@davidfowl
Copy link
Member

davidfowl commented Jul 7, 2021

We want to determine if these parameters should fail to bind if there's no value provided for them from the determined source (inferred or explicit). The question is, how does the developer indicate that a parameter is optional (should allow default value) or not. For example

string Hello(string name = "John") => $"Hello {name}";
app.MapGet("/hi", Hello);

In the above, if no name is provided (via the query string), the value will be "John".

string Hello(string name = null) => $"Hello {name}";
app.MapGet("/hi", Hello);

In the above, if no name is provided, the value will be null.

string Hello(string name) => $"Hello {name}";
app.MapGet("/hi", Hello);

What does the above mean? Should it mean non-null? Today if no name is provided the value will be null.

We need a way to opt-into "required-ness":

  • Nullable annotations:
  • Validation Attributes
  • Absence of default values could mean required
  • Type system (e.g. F# option)
// Required name
string Hello(string name) => $"Hello {name}";

// Required name
string Hello([Required]string name) => $"Hello {name}";

// Optional name
string Hello(string name = null) => $"Hello {name}";

// Optional name
string Hello(string? name) => $"Hello {name}";

The same would apply for body parameters:

// Required TodoItem
async Task<IResult> PostTodo(TodoItem todo)
{
    await db.Todos.AddAsync(todo);
    await db.SaveChangesAsync();

    return Created($"/todo/{todo.Id}", todo);
}

// Required TodoItem
async Task<IResult> PostTodo([Required]TodoItem todo)
{
    await db.Todos.AddAsync(todo);
    await db.SaveChangesAsync();

    return Created($"/todo/{todo.Id}", todo);
}

// Optional TodoItem (can be null)
async Task<IResult> PostTodo(TodoItem todo = null)
{
    if (todo is null)
    {
       return UnprocessableEntity();
    }

    await db.Todos.AddAsync(todo);
    await db.SaveChangesAsync();

    return Created($"/todo/{todo.Id}", todo);
}

// Optional TodoItem (can be null)
async Task<IResult> PostTodo(TodoItem? todo)
{
    if (todo is null)
    {
       return UnprocessableEntity();
    }
    await db.Todos.AddAsync(todo);
    await db.SaveChangesAsync();

    return Created($"/todo/{todo.Id}", todo);
}

@halter73 halter73 assigned captainsafia and unassigned JamesNK Jul 7, 2021
@davidfowl
Copy link
Member

We've decided to avoid [Required] and just support default parameter values and nullability.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc blocked The work on this issue is blocked due to some dependency enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing help wanted Up for grabs. We would accept a PR to help resolve this issue Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants