ardalis / Result

A result abstraction that can be mapped to HTTP response codes if needed.
MIT License
866 stars 107 forks source link

āœØ Result Status Helpers #176

Closed danielmackay closed 5 months ago

danielmackay commented 6 months ago

Pain

When checking the result status (for example a service called from a minimal API). The code seems more verbose that it could be.

For example:

app
  .MapPost("/",
      async Task<Results<Created, BadRequest<ValidationProblemDetails>, NotFound>> (
          [FromServices] ISender sender,
          [FromBody] CreateLeaveCommand command, CancellationToken ct) =>
      {
          var result = await sender.Send(command, ct);
          if (result.Status == ResultStatus.BadRequest) // šŸ‘ˆ here
              return TypedResults.BadRequest(result.ToValidationProblem());

          if (result.Status == ResultStatus.NotFound) // šŸ‘ˆ here
              return TypedResults.NotFound();

          return TypedResults.Created();
      })
  .WithName("CreateLeave");

Solution

Add helper fields to IResult so that the code is more concise. The result would look something like:

app
  .MapPost("/",
      async Task<Results<Created, BadRequest<ValidationProblemDetails>, NotFound>> (
          [FromServices] ISender sender,
          [FromBody] CreateLeaveCommand command, CancellationToken ct) =>
      {
          var result = await sender.Send(command, ct);
          if (result.IsInvalid) // šŸ‘ˆ here
              return TypedResults.BadRequest(result.ToValidationProblem());

          if (result.IsNotFound) // šŸ‘ˆ here
              return TypedResults.NotFound();

          return TypedResults.Created();
      })
  .WithName("CreateLeave");

The above reads much cleaner and removes unneeded code.

@ardalis:

  1. What do you think of the above?
  2. Would you be happy for me to create a PR for this?
  3. Do you think IResult is the best place to put this?
ardalis commented 5 months ago
  1. I like it.
  2. Yes!
  3. I'm not sure whether IResult or Result is better. The only thing that makes me unsure is that IResult has been co-opted by the dotnet team(https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.iresult?view=aspnetcore-8.0), so there might be confusion with its usage (either by developers or by the compiler or both).

If you don't see an issue with the IResult type collision, then let's try that.

If you opt to use IResult, create a new class IResultExtensions for the new methods. Otherwise just add them to the existing ResultExtensions type.

Confirm you're still interested in the PR and I'll assign this to you (or just submit the PR if that's quick).

Thanks!

sunecko commented 5 months ago

@ardalis @danielmackay This could be solved by adding the methods to ResultExtensions class?

public static bool IsUnauthorized(this Result result)
    => result.Status == ResultStatus.Unauthorized;
public static bool IsForbidden(this Result result)
     => result.Status == ResultStatus.Forbidden;

These two are examples, if you think it is the correct way to do it I would be happy to make my contribution.

danielmackay commented 5 months ago
  1. I like it.
  2. Yes!
  3. I'm not sure whether IResult or Result is better. The only thing that makes me unsure is that IResult has been co-opted by the dotnet team(https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.iresult?view=aspnetcore-8.0), so there might be confusion with its usage (either by developers or by the compiler or both).

If you don't see an issue with the IResult type collision, then let's try that.

If you opt to use IResult, create a new class IResultExtensions for the new methods. Otherwise just add them to the existing ResultExtensions type.

Confirm you're still interested in the PR and I'll assign this to you (or just submit the PR if that's quick).

Thanks!

Hey @ardalis. Thanks for the reply! Yes I am still interesting in actioning this PR. Please assign to me and I will aim to get it done over the next week.

Cheers!

danielmackay commented 5 months ago

@ardalis I ended up opting for the extension methods to be agains IResult as that is the lowest common denominator. I don't believe there will be a problem with the type collision.