dotnet / dotnet/aspnetcore

Suggest using parameter binding instead of manual parsing of HttpContext

Open
#35,760 5 comments 2 reactions 0 assignees View on GitHub
analyzer api-approved area-minimal feature-minimal-actions Priority:2
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 6h
Merged PRs (30d)
290

Description

## 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.

### Category

- [ ] Design
- [ ] Documentation
- [ ] Globalization
- [ ] Interoperability
- [ ] Maintainability
- [ ] Naming
- [ ] Performance
- [ ] Reliability
- [ ] Security
- [ ] Style
- [X] Usage

### Severity Level

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

## Usage Scenarios

Some patterns we can detect:

**Untyped parsing**

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

Suggested fix:

```C#
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:

```C#
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

```C#
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:

```C#
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:

```C#
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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.