dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.21k stars 9.95k forks source link

Use ValueTuple element names in minimal route handlers #40510

Closed halter73 closed 1 year ago

halter73 commented 2 years ago

Is there an existing issue for this?

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

Take the following minimal route handler:

app.MapPut("/todos/{id}", async ((int Id, string Title, bool IsComplete) inputTodo, TodoDb db) =>

Today, to upload an inputTodoo to this endpoint, you would need a JSON request body like the following because we don't look for any TupleElementNameAttributes on the parameter:

{
    "Item1": 1,
    "Item2": "Support named items in ValueTuples",
    "Item3": false
}

Describe the solution you'd like

If we took the names of the ValueTuple elements into account, the request body could look like you'd expect:

{
    "Id": 1,
    "Title": "Support named items in ValueTuples",
    "IsComplete": false
}

Additional context

We have the same problem with ValueTuples returned from minimal route handlers. Today, the following would be serialized using Item1, Item2 and Item3 as the property values instead of the specified names.

app.MapGet("/todos/{id}", async (int Id, string Title, bool IsComplete)  (int id, TodoDb db) =>
DamianEdwards commented 2 years ago

We'd also need to consider what the generated ApiExplorer schema looks like, i.e. it should match what we'd deserialize from/serialize to.

This is an enticing idea when one considers it as a way to enable inline definition of DTOs. This is useful when wanting to explicitly control the shape of an API body argument to prevent mass-assignment/over-binding to the concrete type, e.g.

app.MapPost("/todos", async ((string Title, bool IsComplete) newTodo, TodoDb db) =>
{
    // We don't want to bind to Id or CreatedOn when creating new todos
    var todo = new Todo { Title = newTodo.Title, IsComplete = newTodo.IsComplete, CreatedOn = DateTimeOffset.Now };

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

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

public class Todo
{
    public int Id { get; set; }
    public string? Title { get; set; }
    public bool IsComplete { get; set; }
    public DateTimeOffset CreatedOn? { get; set; }
}
davidfowl commented 2 years ago

This is a very cool idea but I have concerns about implementing a custom serializer for tuples. There are also lots of challenges around nested tuples and how that would interact with the serializer.

This to me feels like it should work everywhere, not just minimal APIs (like IAsyncEnumerable<T>). The problem is that the schema for the lives with the methods and not with the type. This means that we need to somehow feed this information into a single unit the JSON serializer can understand. This gets more complex with nested tuples see this and this for an example. Assuming we could figure out which names are paired with with which nested tuples, the only reasonable way to do this would be a custom JsonConverter. That would allow us to not lose the benefits of the serializer (recursion depth checks, nested serialization etc etc). This would mean that we'd need to allocate a JsonSerializationOptions per callsite that captured the required information to handle tuples.

JsonConverters don't give you access to the member being deserialized, just the type. This makes that more challenging.

Or we could say it only works for top level properties and nested isn't supported...(no VT<T0, VT<T1>>)

I'd like to hear some thoughts from @layomia and @eiriktsarpalis.

eiriktsarpalis commented 2 years ago

I don't see how this might be possible to solve in the general case, for the reasons @davidfowl already mentioned. I'm also not sure how a custom JsonConverter might be able to work with ambiguity such as ((int foo, int bar), (int baz, int qux)). As far as the STJ converter infrastructure is concerned, both nested values are of type ValueTuple<int, int> and as such they will be dispatched to the same converter instance for serialization without any contextual information.

It might be worth pointing out that the source generator can in fact retrieve tuple label metadata from Roslyn, however such an approach would be problematic for a couple of reasons:

  1. Contract divergence from the reflection-based serializer.
  2. Converters either source generated or otherwise are still indexed by Type, and as such it would not be possible to address ambiguities like the one highlighted above.
ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 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 commented 2 years ago

@eiriktsarpalis yep I came to the same conclusion.

zyofeng commented 1 year ago

This would work quite nicely with CQRS pattern by splitting up the request DTOs instead of having to pollute them with Parameter Binding attributes or creating new ones

var group = app.MapGroup("/accounts")
    .WithTags("Accounts")
    .RequireAuthorization();

group.MapPost("{accountNumber}/transactions",
    (ISender sender, string accountNumber,
        (DateOnly From, DateOnly To, int CurrentPage, int PageSize) body) => sender.Send(
        new GetTransactionsQuery
        {
            AccountNumber = accountNumber,
            PageSize = body.PageSize,
            From = body.From,
            To = body.To,
            CurrentPage = body.CurrentPage
        }))
    .WithName("Transactions");
davidfowl commented 1 year ago

I'm not sure why this issue is still open. We should close this out. Tuples are unsuitable for this.

@zyofeng you can use AsParameters for this usecase:

group.MapPost("{accountNumber}/transactions",
    ([AsParameters]RequestDto dto) => dto.Sender.Send(
        new GetTransactionsQuery
        {
            AccountNumber = dto.AccountNumber,
            PageSize = dto.Body.PageSize,
            From = dto.Body.From,
            To = dto.Body.To,
            CurrentPage = dto.Body.CurrentPage
        }))
    .WithName("Transactions");

record struct RequestBody(DateOnly From, DateOnly To, int CurrentPage, int PageSize); 
record struct RequestDto(ISender Sender, string AccountNumber, RequestBody Body);
zyofeng commented 1 year ago

Thanks @davidfowl. That's similar to what I am doing atm,

        group.MapPost("{accountNumber}/transactions",
                (ISender sender, string accountNumber, GetTransactionsRequest body) => sender.Send(
                    new GetTransactionsQuery
                    {
                        AccountNumber = accountNumber,
                        PageSize = body.PageSize,
                        From = body.From,
                        To = body.To,
                        CurrentPage = body.CurrentPage
                    }))
            .WithName("Transactions");

        return app;
    }
    public record GetTransactionsRequest(DateOnly From, DateOnly To, int CurrentPage, int PageSize = 20);

Although I am kinda hoping I don't have to create another request dto. Cheers