Add generic method overloads to Microsoft.AspNetCore.Http.Results static class

[DamianEdwards]

As a follow-up to making the in-box IResult-implementing types public and the introduction of typed results like Ok<TValue>, we should add new generic overloads to the methods on the Microsoft.AspNetCore.Http.Results static factory class that accept a value that is ultimately passed to the equivalent generic method on Microsoft.AspNetCore.Http.TypedResults, thus preserving the concrete type of the passed object. With the concrete type preserved, logic inspecting the runtime return types (e.g. unit tests, filters) can use pattern matching techniques to check for the desired type.

These new methods should continue to return IResult so that all methods on the Results class are easy to use in inferred IResult returning lambdas where multiple return statements exist. The TypedResults class will remain the way to create instances of in-box IResult-implementing types while preserving the concrete type (primarily for use with the Results<TResult1, TResultN> union type).

Note that these new methods will be bound by the compiler for existing source code in projects updated from .NET 6 to .NET 7. We don't treat this as a breaking change however given the declared return type will remain IResult and the runtime types in .NET 6 were not public, so any reference to them must have used unsupported approaches anyway (e.g. reflection).

Examples

// Before change: result is typed as IResult and the value has a runtime type of Ok<object>
var result = Results.Ok(new Todo(1, "Pick up the groceries"));

// After change: result is typed as IResult and the value has a runtime type of Ok<Todo>
var result = Results.Ok(new Todo(1, "Pick up the groceries"));

record Todo(int Id, string Title);

public static class TodosApi
{
    public static IEndpointRouteBuilder MapTodosApi(this IEndpointRouteBuilder routes)
    {
        routes.MapGet("/todos", GetAllTodos);
    }

    public static async Task<IResult> GetAllTodos(TodoDb db)
    {
        return Results.Ok(await db.Todos.ToArrayAsync());
    }
}

// A test method
[Fact]
public async Task GetAllTodos_ReturnsOkResultOfIEnumerableTodo()
{
    // Arrange
    var db = CreateDbContext();

    // Act
    var result = await TodosApi.GetAllTodos(db);

    // Assert: Check the returned result type is correct
    Assert.IsType<Ok<Todo[]>>(result);
}

Suggested API

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Accepted<TValue?>(TValue? value);
+    public static IResult AcceptedAtRoute<TValue?>(string? routeName, object? routeValues = null, TValue? value = null);
+    public static IResult BadRequest<TValue?>(TValue? error);
+    public static IResult Conflict<TValue?>(TValue? error);
+    public static IResult Created<TValue?>(string uri, TValue? value);
+    public static IResult Created<TValue?>(Uri uri, TValue? value);
+    public static IResult CreatedAtRoute<TValue?>(string? routeName, object? routeValues = null, TValue? value = null);
+    public static IResult Json<TValue>(TValue value, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null);
+    public static IResult NotFound<TValue?>(TValue? value);
+    public static IResult Ok<TValue?>(TValue? value);
+    public static IResult UnprocessableEntity<TValue?>(TValue? error);
}

[ghost]

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.

[davidfowl]

cc @JamesNK for trim friendliness.

[ghost]

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.

[halter73]

API review notes:

1. Can we return typed arguments from the new generic Results methods?
   No. This would be source breaking to existing apps (and annoying to new apps) with multiple returns in a lambda. And it's more consistent.
2. Risks?
   It changes the methods some existing apps call when they're recompiled. This only changes the internal runtime type we returned.
3. What about polymorphism?
   We should make sure that if I was returning Results.Ok(myBaseTypedVariable) that it still serializes all the same properties it would before and not just those on the declared base type.
   We think this would be breaking, so the types like Ok<TValue> will need a mode to continue serializing all properties even if a base type is declared. We should ALSO CHANGE TypedResults to match this behavior for consistency. If you want to exclude certain properties, you can use DTO, attribute or custom IResult.

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Accepted<TValue>(TValue? value);
+    public static IResult AcceptedAtRoute<TValue>(string? routeName, object? routeValues = null, TValue? value = null);
+    public static IResult BadRequest<TValue>(TValue? error);
+    public static IResult Conflict<TValue>(TValue? error);
+    public static IResult Created<TValue>(string uri, TValue? value);
+    public static IResult Created<TValue>(Uri uri, TValue? value);
+    public static IResult CreatedAtRoute<TValue>(string? routeName, object? routeValues = null, TValue? value = null);
+    public static IResult Json<TValue>(TValue value, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null);
+    public static IResult NotFound<TValue>(TValue? value);
+    public static IResult Ok<TValue>(TValue? value);
+    public static IResult UnprocessableEntity<TValue>(TValue? error);
}

• @JamesNK for trimmability discussion. I imagine it would be similar to the other existing Results or TypedResults methods.

Given the nuanced breaking change discussion, we'll continue this next time.

[brunolins16]

3. We should make sure that if I was returning Results.Ok(myBaseTypedVariable) that it still serializes all the same properties it would before and not just those on the declared base type.

I just tested this behavior and TypedResults and Results are behaving differently. Eg.:

public class BaseModel
{
    public int BaseProperty { get; set; }
}

public class ChildModel : BaseModel
{
    public int ChildProperty { get; set; }
}

// response: {"baseProperty":1}
app.MapGet("/typed-results", () =>
{
    BaseModel model = new ChildModel() { BaseProperty = 1, ChildProperty = 2 };
    return TypedResults.Ok(model);
});

// response: {"childProperty":2,"baseProperty":1}
app.MapGet("/results", () =>
{
    BaseModel model = new ChildModel() { BaseProperty = 1, ChildProperty = 2 };
    return Results.Ok(model);
});

[davidfowl]

@halter73 another one!

[DamianEdwards]

@davidfowl yeah he was the one who brought it up in API review. It's a little concerning to me how unintuitive this feels but being consistent is the most important thing.

[davidfowl]

Consistently unintuitive.

[brunolins16]

At this point ( not questioning what is correct) , should we update the TypedResults or any generic method to behave as Results?

[halter73]

We think this would be breaking, so the types like Ok<TValue> will need a mode to continue serializing all properties even if a base type is declared. We should ALSO CHANGE TypedResults to match this behavior for consistency. If you want to exclude certain properties, you can use DTO, attribute or custom IResult.

This is still my position. We should always serialize all properties to JSON which is what we have been doing when you return the object directly or you've used the untyped Results methods. We were actually quite consistent with this in NET 6 with minimal APIs with the exception of objects returned via a Task or ValueTask, but that was fixed in .NET 7 preview 2 by #39858.

[DamianEdwards]

Yeah I meant more the inconsistency between what System.Text.Json does by default. What do our JSON helpers for HttpContext do?

[halter73]

What do our JSON helpers for HttpContext do?

They pass the type argument directly through to System.Text.Json, so it will only serialize the properties on the declared type (unless it's object).

[DamianEdwards]

Right, that's the kind of inconsistency I'm talking about: the extensions on HttpRequest and HttpResponse preserve the type data, but RequestDelegateFactory and TypedResults won't. TBC I'm not saying we should leave TypedResults the way it is, just that the inconsistency makes it hard to remember which behavior you're getting from a given API.

[halter73]

TypedResults are new, so we can choose to give the generic IResult implementations like Ok<TValue> the S.T.J behavior by default. As @brunolins16 demonstrated this is what we're doing right now.

But then, if we want to return the same generic result types from the new generic Results methods proposed in this PR, we'd have to give all the generic result types a mode to serialize as object because otherwise it'd be source breaking. Old calls to Results.Ok(myValue) would still compile, but they'd produce different responses without the alternate mode if the declared type of myValue was a non-object base type.

I don't like having an inconsistency between TypedResults and Results though, and giving modes to all the generic IResult implementations seems overly complicated.

Here are the options I see:

1. Update the generic IResult implementations like Ok<TValue> to always serialize as object to get consistent behavior between Results and TypedResults and returning the value directly.
2. Do the crazy modal thing I describe above. The generic Results methods would return the same type as its TypedResults counterpart so request filters can pattern match against it, but the results would serialize differently. TypedResults would behave like S.T.J and the generic Results methods proposed in this issue would behave like the existing non-generic Results methods.
3. Leave the behavior of the generic IResult implementations as is. Just don't add the new generic methods proposed in the issue. Basically, this means we do nothing. Filters would always see TValue as object if returned from a Results method, but that's probably fine. This only affects the runtime type anyway so it cannot contribute to any endpoint metadata.

I think my vote is for option 3 right now. Option 1 was my previous vote, but now that's a close second for me.
On second thought, my vote is still for option 1, but option 3 is a close second.

[DamianEdwards]

Yep I agree, make Results and TypedResults behave the same WRT serialization, despite it being different to what S.T.J, and by extension the request extension methods, do.

[davidfowl]

We should talk about trim ability as well?

Calling @JamesNK and @eiriktsarpalis and @eerhardt

[halter73]

Is anything about serializing the return type of an arbitrary Delegate trimmable? We already liberally applying [RequiresUnreferencedCode(MapEndpointTrimmerWarning)] to all the MapXXX methods that take Delegate. I guess this comes up if you call IResult.ExecuteAsync(HttpContext) directly instead of having the RequestDelegateFactory call it for you. But that's relatively rare, so also applying [RequiresUnreferencedCode] to that makes sense, no?

[davidfowl]

I'm wondering if we want more overloads like we did for Json to support the System.Text.Json source generator.

[JamesNK]

Yes. Methods like these will never be 100% trim safe:

public static IResult Ok<TValue>(TValue? value);
public static IResult Json<TValue>(TValue value, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null)

Don't worry about trimming behavior with them.

Overloads that take JsonSerializerContext (i.e. untyped) or JsonTypeInfo<T> (i.e. typed, more relevant to the API at hand) are needed. And if someone specifies a JsonTypeInfo<T> then they're being specific about the type they're serializing.

app.MapGet("/typed-results", () =>
{
    BaseModel model = new ChildModel() { BaseProperty = 1, ChildProperty = 2 };

    // Note the generated context. I don't think this would compile.
    // I think the compiler would complain that you're passing it a BaseModel and say that a ChildModel is required.
    return TypedResults.Ok(model, MyJsonContext.ChildModel);
});

More info: https://devblogs.microsoft.com/dotnet/try-the-new-system-text-json-source-generator/#introducing-jsontypeinfot-jsontypeinfo-and-jsonserializercontext

But, this is only making the response trim safe. Request parameter args types aren't preserved and will only work if placed in an assembly that's excluded from trimming.

[brunolins16]

Updating the proposed API to reflect some additional proposed changes:

namespace Microsoft.AspNetCore.Http;

public static class Results
{
-    public static IResult Accepted<TValue>(TValue? value);
+    public static IResult Accepted<TValue>(string? uri = null, TValue? value = default);

-    public static IResult AcceptedAtRoute<TValue>(string? routeName, object? routeValues = null, TValue? value = null);
+    public static IResult AcceptedAtRoute<TValue>((string? routeName = null, object? routeValues = null, TValue? value = default);

+    public static IResult BadRequest<TValue>(TValue? error);
+    public static IResult Conflict<TValue>(TValue? error);
+    public static IResult Created<TValue>(string uri, TValue? value);
+    public static IResult Created<TValue>(Uri uri, TValue? value);

-    public static IResult CreatedAtRoute<TValue>(string? routeName, object? routeValues = null, TValue? value = null);
+    public static IResult CreatedAtRoute<TValue>(string? routeName = null, object? routeValues = null, TValue? value = default);

+    public static IResult Json<TValue>(TValue value, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null);
+    public static IResult NotFound<TValue>(TValue? value);
+    public static IResult Ok<TValue>(TValue? value);
+    public static IResult UnprocessableEntity<TValue>(TValue? error);
}

[ghost]

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]

Overloads that take JsonSerializerContext (i.e. untyped) or JsonTypeInfo<T> (i.e. typed, more relevant to the API at hand) are needed. And if someone specifies a JsonTypeInfo<T> then they're being specific about the type they're serializing.

I don't see overloads taking either of these types. Should we add them?

[brunolins16]

Overloads that take JsonSerializerContext (i.e. untyped) or JsonTypeInfo<T> (i.e. typed, more relevant to the API at hand) are needed. And if someone specifies a JsonTypeInfo<T> then they're being specific about the type they're serializing.

I don't see overloads taking either of these types. Should we add them?

I think we should, also add to the TypedResults, but I prefer no include them as part of this issues and have another one where we can discuss exactly what is needed to get Results and TypedResults trim "safe" (at least the response as state by James). Are you ok with that?

[ghost]

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.

[BrennanConroy]

API review notes:

• We are assuming all object accepting Results methods are having new APIs added here, lets double check when making the PR
• Lets make sure the TypedResults behavior matches

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Accepted<TValue>(string? uri = null, TValue? value = default);

+    public static IResult AcceptedAtRoute<TValue>(string? routeName = null, object? routeValues = null, TValue? value = default);

+    public static IResult BadRequest<TValue>(TValue? error);
+    public static IResult Conflict<TValue>(TValue? error);
+    public static IResult Created<TValue>(string uri, TValue? value);
+    public static IResult Created<TValue>(Uri uri, TValue? value);

+    public static IResult CreatedAtRoute<TValue>(string? routeName = null, object? routeValues = null, TValue? value = default);

+    public static IResult Json<TValue>(TValue value, JsonSerializerOptions? options = null, string? contentType = null, int? statusCode = null);
+    public static IResult NotFound<TValue>(TValue? value);
+    public static IResult Ok<TValue>(TValue? value);
+    public static IResult UnprocessableEntity<TValue>(TValue? error);
}

API approved!