Suggest using parameter binding instead of manual parsing of HttpContext

davidfowl commented 3 years ago

Background and Motivation

Although we support scenarios where users can manual bind parameters from the HttpContext associated with their route handler, it is generally recommended to leverage the existing binding logic in ASP.NET to support this.

Note: we prototyped this analyzer as part of the 2022 hackathon.

Proposed Analyzer

Analyzer Behavior and Message

When we determine that the user has accessed the HttpContext.HttpRequest value to extract parameters, provide a warning with the following message.

Recommend using built-in parameter binding instead of manually processing HTTP request.

Severity Level

[ ] Error
[X] Warning
[ ] Info
[ ] Hidden

Usage Scenarios

Some patterns we can detect:

Untyped parsing

app.MapGet("/hey/{name}", (HttpRequest req) => $"Hello {req.RouteValues["name"]}");

Suggested fix:

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

We should can remove the HttpRequest if it's not referenced by anything in the method.

Typed parsing

If we can detect the parsing of those members:

app.MapGet("/todos/{id}", (HttpRequest req) =>
{
    if (int.TryParse((string?)req.RouteValues["id"], out var id))
    {
        return Results.BadRequest();
    }

    var todo = db.Find(id);
    if (todo is null)
    {
        return Results.BadRequest();
    }

    return Results.Ok(todo);
});

To

app.MapGet("/todos/{id}", (int id) =>
{
    var todo = db.Find(id);
    if (todo is null)
    {
        return Results.BadRequest();
    }

    return Results.Ok(todo);
});

This should also work for the query string:

app.MapGet("/todos", async (HttpContext context) =>
{
    if (!int.TryParse(context.Request.Query["pageIndex"], out var pageIndex))
    {
        context.Response.StatusCode = 400;
        return;
    }

    if (!int.TryParse(context.Request.Query["pageSize"], out var pageSize))
    {
        context.Response.StatusCode = 400;
        return;
    }

    await context.Response.WriteAsJsonAsync(db.GetTodos().Take(pageIndex).Skip(pageSize));
});

Suggested change:

app.MapGet("/todos", (int pageIndex, int pageSize, HttpContext context) =>
{
    await context.Response.WriteAsJsonAsync(db.GetTodos().Take(pageIndex).Skip(pageSize));
});

Risks

There are some scenarios where users may prefer to manually parse parameters from HttpContext.HttpRequest but the warning level of this analyzer is sufficient.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

captainsafia commented 1 year ago

Updated this to match our API template and added API review label.

ghost commented 1 year ago

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

halter73 commented 1 year ago

API Review Notes:

What do we think about "Recommend using built-in parameter binding instead of manually processing HTTP request." verbiage? It does seem like we have a bunch of analyzer messages that start with "Suggest", but it feels better to just make the suggestion rather than use the word "Suggest" or "Recommend".
What about custom error handling logic? Or are using a shared component to read the route value? Can we detect that? Would it be annoying to get a usage warning when the code you already have is necessary for your custom error handling logic?
Is this useful enough without a fixer? It might not be clear enough what to do without really god documentation. We should link to documentation that has good before and after examples.

Analyzer Approved with modifications.

Category: Usage
Level: Information
Message: Use built-in parameter binding instead of manually processing HTTP request.

dotnet / aspnetcore