Skip to content

Add TypedResults static factory class for creating typed IResult objects #41161

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

Merged
merged 35 commits into from
Apr 18, 2022

Conversation

DamianEdwards
Copy link
Member

@DamianEdwards DamianEdwards commented Apr 13, 2022

  • Reinstate the HttpResult suffix on IResult types except for where the short name has recognized value, e.g. those types that implement IEndpointMetadataProvider and will be used in conjunction with Results<T1, TN>, e.g. Results<Ok<Todo>, NotFound>
  • Change from Results.Typed.xxx to TypedResults.xxx
  • Added HttpResults.ValidationProblem type that represents 400 responses with validation errors (equiv to BadRequest<HttpValidationProblemDetails>
  • Changed TypedResults.ValidationProblem to return new HttpResults.ValidationProblem type
  • Moved Results<TResult1, TResultN> types into the Microsoft.AspNetCore.Http.HttpResults namespace to match where the other results types are
  • Changed Results.xxx methods to call through to TypedResults.xxx methods
  • Explicitly implemented IEndpointMetadataProvider on following IResult types:
    • Accepted
    • Accepted<TValue>
    • AcceptedAtRoute
    • AcceptedAtRoute<TValue>
    • BadRequest
    • BadRequest<TValue>
    • Conflict
    • Conflict<TValue>
    • Created
    • Created<TValue>
    • CreatedAtRoute
    • CreatedAtRoute<TValue>
    • NoContent
    • NotFound
    • NotFound<TValue>
    • Ok
    • Ok<TValue>
    • UnprocessableEntity
    • UnprocessableEntity<TValue>
  • Order using statements before namespace declarations
  • Added tests for Microsoft.AspNetCore.Http.Results and Microsoft.AspNetCore.Http.TypedResults

Fixes #41009

@Pilchie Pilchie added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Apr 13, 2022
@DamianEdwards DamianEdwards force-pushed the damianedwards/results.typed branch from 1b75667 to 2c93eca Compare April 15, 2022 20:05
@DamianEdwards DamianEdwards marked this pull request as ready for review April 16, 2022 01:08
@DamianEdwards DamianEdwards requested a review from a team April 16, 2022 01:08
@davidfowl
Copy link
Member

Here's a bit of a compromise on what I said about conflicts. We change all of the classes to verbs instead of nouns and keep the all without the result/httpresult.

Thoughts?

@DamianEdwards
Copy link
Member Author

Here's a bit of a compromise on what I said about conflicts. We change all of the classes to verbs instead of nouns and keep the all without the result/httpresult.

Example? Like ReturnValidationProblem, ReturnOk?

The other thought that came up last week was keeping the HttpResult suffix and just letting folks use type aliasing (global if they choose) if they want the short names, but that requires closing any generic unfortunately, e.g.:

using OkTodo = Microsoft.AspNetCore.Http.HttpResults.OkHttpResult<Todo>;
using NotFound = Microsoft.AspNetCore.Http.HttpResults.NotFoundHttpResult;

app.MapGet("/todo/{id}", async Task<Results<OkTodo, NotFound>> (int id, TodoDb db) =>
    await db.Todos.FindAsync(id) is Todo todo
        ? new OkTodo(todo)
        : new NotFound();

@brunolins16
Copy link
Member

brunolins16 commented Apr 16, 2022

Example? Like ReturnValidationProblem, ReturnOk?

Seems a little bit weird have verbs in the class names and it does not sound better that *HttpResult. I know that might be less clear but what about an alternative name? Eg. Ok -> Succeeded

@DamianEdwards
Copy link
Member Author

Seems a little bit weird have verbs in the class names and do not sound better that *HttpResult. I know that might be less clear but what about an alternative name? Eg. Ok -> Succeeded

Kinda hate it 😄 Sticking with well established names for HTTP statuses is the only logical thing to do I think.

@brunolins16
Copy link
Member

Seems a little bit weird have verbs in the class names and do not sound better that *HttpResult. I know that might be less clear but what about an alternative name? Eg. Ok -> Succeeded

Kinda hate it 😄 Sticking with well established names for HTTP statuses is the only logical thing to do I think.

To be honest I hate it as well 😂

brunolins16 and others added 14 commits April 18, 2022 11:47
WIP
- Reinstate the `HttpResult` suffix on IResult types except for where the short name has recognized value, e.g. those types that implement `IEndpointMetadataProvider` and will be used in conjunction with `Results<T1, TN>`
- Order `using` statements before `namespace` declarations
- Change from `Results.Typed.xxx` to `TypedResults.xxx`
- Added `HttpResults.ValidationProblem` type that represents 400 responses with validation errors (equiv to `BadRequest<HttpValidationProblemDetails>`
- Changed `TypedResults.ValidationProblem` to return new `HttpResults.ValidationProblem` type
- Moved `Results<TResult1, TResultN>` types into the `Microsoft.AspNetCore.Http.HttpResults` namespace to match where the other results types are
@DamianEdwards DamianEdwards force-pushed the damianedwards/results.typed branch from 2a5c30f to f551d7b Compare April 18, 2022 18:47
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't go over the new APIs with a fine-toothed comb, but I'm assuming they are what we approved.

Thanks for the new tests for the existing types :)

@DamianEdwards DamianEdwards enabled auto-merge (squash) April 18, 2022 23:12
@DamianEdwards DamianEdwards merged commit 64ccf82 into main Apr 18, 2022
@DamianEdwards DamianEdwards deleted the damianedwards/results.typed branch April 18, 2022 23:21
@ghost ghost added this to the 7.0-preview4 milestone Apr 18, 2022
/// <summary>
/// An <see cref="IResult"/> that on execution will write an object to the response
/// with Unprocessable Entity (422) status code.
/// </summary>
public sealed class UnprocessableEntityObjectHttpResult : IResult
public sealed class UnprocessableEntity : IResult, IEndpointMetadataProvider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DamianEdwards Is it too late to rename these to Unprocessable Content to match the new official reason phrase from RFC9110?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also filed #42302

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
6 participants