Allowing for validation and other errors to be returned as standard `ProblemDetails`

nhwilly commented 2 months ago

The collision of FastEndpoints (FE), FluentValidation (FV), Asp.Net Core (MVC) and Ardalis.Result (AR) in the same application begs for a standardized error response.

The Validation Issue

FE uses FV in the pipeline and returns a custom FE implementation of ProblemDetails (which does not derive from the MVC class), but does attempt to maintain 7807/9457 compatibility (see this). It also uses an internal Error class so that FV ValidationFailure details are not lost.
AR also offers an Invalid result containing a ValidationError. When using AR in the domain model, an AR ValidationError can be used or a FV ValidationFailure can be converted to an AR ValidationError using the AsErrors() extension method.
When ToMinimalApiResult() is called on the result, the client receives a 400 status code but with a completely different payload than the 400 generated by the FE pipeline validation.

Other than catching deserializing exceptions on the client there's no way to know which invalid response shape was received.

The Error Detail Issue

AR now receives an ErrorList object which contains a CorrelationId and a collection of strings. The ToMinimalApiResult() method simply takes the collection of strings and concatenates them into the Detail property. The CorrelationId is lost.
Parsing the concatenated string is less than ideal.

Possible Enhancements

Create a bespoke AR ProblemDetails class in the same fashion as FE did. This seems to be the most popular approach. In this way an AR problem details class could contain a collection of validation errors and a collection of error strings so that all error laden responses could be deserialized to a single type. With proper JSON attributes null collections could be ignored.
Derive from existing MVC classes. HttpValidationProblemDetails only supports collections of strings as errors and so additional ValidationError properties will be lost.
Populate the Extensions property of ProblemDetails. This works well and avoids new types, but the consumer will still need to know if there are simple errors or validation errors contained the JSON. Further, deserializing requires special treatment per this comment in the library -

The round-tripping behavior for is determined by the implementation of the Input \ Output formatters

As expected, a quick attempt to deserialize did not fully recreate the problem details properly.

Looking for guidance on which way to proceed.

nhwilly commented 2 months ago

I believe that creating a bespoke, AR ProblemDetails class is the best approach. And it could be modeled closely to the FE ProblemDetails class. I say this as that might help solve creating the same JSON payload returning to the client whether a FE-FV pipeline ValidationFailure occurred or an AR ValidationError was returned.

Same deserialization could occur.

Or does this create an implied dependency?

nhwilly commented 2 months ago

If we want to standardize the response, then we need to override the FE validation response. There's a hook to do that. FYI

nhwilly commented 2 months ago

Legit question:

ValidationError contains a lot of detail, while standard Error contains a collection of strings.

I can use JSON attributes to configure the response to take the shape most people are used to seeing while the deserialization still works.

Is my custom problem details better off supporting two different types of collections, or just one where many values are nullable depending on the type of content?

nhwilly commented 2 months ago

Update:

Creating a custom ProblemDetails class

As a first pass, I've written an extension method to use in place of ToMinimalApiResult.

This approach requires a replacement for MVC's ProblemDetails which carries two additional properties. One is a collection of strings and the other a collection of a new ValidationErrorDto class.

This class allows for Ardalis.Result.ValidationError and FluentValidation.ValidationFailure to be mapped to a consistent validation detail item.

This approach creates two problems:

The Asp Net Core Results.Problem method won't take a complex type. We can resort to manually constructing the response, but that means that we must supply a Json serialized string and where do we get those? A method argument works but seems nasty. That code looks like this:

private static Microsoft.AspNetCore.Http.IResult UnavailableEntity(IResult result)
{
var problem = new ProblemDetails
{
  Type = "https://tools.ietf.org/html/rfc7231#section-6.6.4",
  Title = "Critical errors occurred",
  Status = (int)HttpStatusCode.ServiceUnavailable,
  Errors = result.Errors.Any() ? result.Errors : null,
  ValidationErrors = result.ValidationErrors.Any() ? result.ValidationErrors.ToDto() : null
};

return Results.Json(problem, _jsonOptions, ProblemContentType, 500);
}

Using MVC's ProblemDetail Extensions

I like this idea best as it keeps things close to the framework. However, the validation errors and errors will end up in Extensions which looks like this:

Extensions = new Dictionary<string, object?> { { "errors", validationErrors } }

This ProblemDetails property carries this attribute: [JsonExtensionData]

Everything serializes just fine. Clients, however can't deserialize Extensions because they've been serialized as object.

It is possible to resolve this with a JsonConverter and concrete type to be used on the client.

Examples in the wild show people checking the Type property of the response (which can be obtained by deserializing to ProblemDetails). Based on the Type value, you can specify the target class, such as ProblemDetailsInvalid or ProblemDetailsWithErrors to get at the Extensions values.

Current Package Enhancement

In the current version of the ToMinimalApiResult method, IResult is cast before processing the response. In that case we are losing the CorrelationId before we can put it into a ProblemDetails.

In Any Case

A new extension method would have to be added to Ardalis.Result to alter how Errors are put into whatever we develop, making them accessible to clients as an enumerable.
Client classes will have to be supplied because JsonConverters are weird and no one wants to do that. Probably one for ValidationErrors and one Errors.

Considerations for using MVC ProblemDetails

If we are using a json converter on the client, we can put both validation error entries and string error entries in the Extensions dictionary with a key of error. In this way resulting payloads would look more normal to non C# consumers.

Working on this today...

Looking for input

ardalis commented 2 months ago

Success - 2xx ValidationErrors - 4xx Errors - 500 ProblemDetails

{
  "type": "MyValidationErrorType",
  "title": null,
  "status": 400,
  "detail": null,
  "instance": null,
  "errors": {
    "severity": "Info"
  }
}

{
  "type": "MyValidationErrorType",
  "title": null,
  "status": 400,
  "detail": null,
  "instance": null,
  "errors": [
    {
      "identifier": "PropertyName",
      "errorMessage": "you idiot!",
      "errorCode": "myCode",
      "severity": 2
    }
  ]
}

{
  "type": "MyValidationErrorType",
  "title": null,
  "status": 400,
  "detail": null,
  "instance": null,
  "validationErrors": [
    {
      "identifier": "PropertyName",
      "errorMessage": "you idiot!",
      "errorCode": "myCode",
      "severity": 2
    }
  ],
  "errors": {
    "severity": "Info"
  }
}

nhwilly commented 2 months ago

OK, I've written something that works for me. But during our dB meeting you spoke about swapping some statements around to avoid casting as IResult and losing CorrelationId and LocationId as they are not part of the interface.

Here's the awful workaround I ended up with, but I don't know how you intended it to work.

When I try to cast Result<T> to Result it returns null. I guess I'd expect that.

Am I using this Result library wrong? All of my usages are handling Result<T> Nothing calls Result.

public static Microsoft.AspNetCore.Http.IResult ToApiResult<T>(this Result<T> result)
{
  return result.Status switch
  {
    ResultStatus.Ok => result is Result ? Results.Ok() : Results.Ok(result.GetValue()),
    ResultStatus.Created => Results.Created(result.Location, result.GetValue()),
    ResultStatus.NotFound => NotFoundEntity(result),
    ResultStatus.Unauthorized => Results.Unauthorized(),
    ResultStatus.Forbidden => Results.Forbid(),
    ResultStatus.Invalid => BadEntity(result),
    ResultStatus.Error => UnprocessableEntity(result),
    ResultStatus.Conflict => ConflictEntity(result),
    ResultStatus.Unavailable => UnavailableEntity(result),
    ResultStatus.CriticalError => CriticalEntity(result),
    _ => throw new NotSupportedException($"Result {result.Status} conversion is not supported."),
  };
}

internal static Microsoft.AspNetCore.Http.IResult ToApiResult(this Result result)
{
  return result.Status switch
  {
    ResultStatus.Ok => result is Result ? Results.Ok() : Results.Ok(result.GetValue()),
    ResultStatus.Created => Results.Created(result.Location, result.GetValue()),
    ResultStatus.NotFound => NotFoundEntity(result),
    ResultStatus.Unauthorized => Results.Unauthorized(),
    ResultStatus.Forbidden => Results.Forbid(),
    ResultStatus.Invalid => BadEntity(result),
    ResultStatus.Error => UnprocessableEntity(result),
    ResultStatus.Conflict => ConflictEntity(result),
    ResultStatus.Unavailable => UnavailableEntity(result),
    ResultStatus.CriticalError => CriticalEntity(result),
    _ => throw new NotSupportedException($"Result {result.Status} conversion is not supported."),
  };
}

ardalis commented 2 months ago

I don't see where you are trying to cast Result from Result<T> in that code. Or is that the point? You couldn't, so you had to duplicate the switch statement?

nhwilly commented 2 months ago

The result of casting Result<T> to Result is null. So, yes. Had to duplicate the statemen.t

savornicesei commented 1 month ago

Hi all,

I don't think this library should handle all existing ProblemDetails objects in AspNet world. It takes arround 40 lines to map Ardalis.IResult to a ProblemDetails object. I believe that adding just some guidance/examples in the (missing 😛 ) documentation would be enough.

As a first example how I did it:

Added the ProblemDetails class from AspNetCore to my .NET framework MVC 5 project

Added some extension methods for Ardalis.Result

public static class ArdalisResultExtensions
{
public static int GetHttpStatusCode(this IResult result)
{
    return result.Status switch
    {
        ResultStatus.Error => StatusCodes.Status422UnprocessableEntity,
        ResultStatus.Forbidden => StatusCodes.Status403Forbidden,
        ResultStatus.Unauthorized => StatusCodes.Status401Unauthorized,
        ResultStatus.Invalid => StatusCodes.Status400BadRequest,
        ResultStatus.NotFound => StatusCodes.Status404NotFound,
        ResultStatus.Conflict => StatusCodes.Status409Conflict,
        ResultStatus.CriticalError => StatusCodes.Status500InternalServerError,
        ResultStatus.Unavailable => StatusCodes.Status503ServiceUnavailable,
        ResultStatus.NoContent => StatusCodes.Status204NoContent,
        _ => StatusCodes.Status200OK
    };
}
public static ProblemDetails AsProblemDetails(this IResult result)
{
    Guard.Against.Null(result, nameof(result));

    return new ProblemDetails()
    {
        Title = result.Status.ToString(),
        Detail = result.IsInvalid()
            ? JsonConvert.SerializeObject(result.ValidationErrors)
            : JsonConvert.SerializeObject(result.Errors),
        Status = result.GetHttpStatusCode(),
        Type = result.ValueType.FullName
    };
}
}

And a new Json* method in my base MVC controller
```
protected internal ActionResult JsonProblemDetails(IResult result)
{
return new JsonProblemDetailsResult(result);
}
```
Now all my ajax calls to MVC endpoints return a standard response, with error codes that enable localized messages to the user from frontend.

ardalis commented 1 month ago

Definitely could (always) use more docs...

We do have them at: https://result.ardalis.com/

ardalis / Result