[Enhancement] Add extension methods for to convert to IActionResult (mvc) IResult (minimal API)

[RHaughton]

First of all, I love this extension.
I would like to see extension methods to convert an ErrorOr (domain result) to an IActionResult or IResult.

The conversion should send 200OK or 204NoContent if the operation is successfull.
If the result is an Error the conversion would take the type of error and return an appropriate ProblemDetails with the correct status Code.

This enhancement was inspired by this repository https://github.com/AKlaus/DomainResult

[florentmsl]

Maybe as a workaround (if this won't get implemented) you could use the really convenient Match method.

e.g

 private async Task<IResult> GetBookById(long id, IBookRepo bookRepo)
    {
        var book = await bookRepo.GetBookByIdAsync(id);

        return book.Match(
            success => Results.Ok(success),
            failure => Results.Problem()
        );
    }

where the IBookRepo returns ErrorOr<Book>. Just a quick demo, you should process the list of errors (failure) and return matching results. 500 UnexpectedError, 400 BadRequest, ...

[amantinband]

I'm not opposed to a adding a feature like this.

I have two follow up questions:

1. Do you think this is something that should be part of the ErrorOr NuGet? Perhaps another NuGet like ErrorOr.AspNetCore?
2. The problem with this type of static conversion, is bypassing the problem details factory that may be used to add custom error details to the response. From the repo you gave as an example, you can see the problem details factory isn't used:

That's the reson I usually have a base controller that has the logic of converting the error or list to an error response.

This is what I usually go with:

[ApiController]
public class ApiController : ControllerBase
{
    protected ActionResult Problem(List<Error> errors)
    {
        if (errors.Count is 0)
        {
            return Problem();
        }

        if (errors.All(error => error.Type == ErrorType.Validation))
        {
            return ValidationProblem(errors);
        }

        return Problem(errors[0]);
    }

    private ObjectResult Problem(Error error)
    {
        var statusCode = error.Type switch
        {
            ErrorType.Conflict => StatusCodes.Status409Conflict,
            ErrorType.Validation => StatusCodes.Status400BadRequest,
            ErrorType.NotFound => StatusCodes.Status404NotFound,
            ErrorType.Unauthorized => StatusCodes.Status403Forbidden,
            _ => StatusCodes.Status500InternalServerError,
        };

        return Problem(statusCode: statusCode, title: error.Description);
    }

    private ActionResult ValidationProblem(List<Error> errors)
    {
        var modelStateDictionary = new ModelStateDictionary();

        errors.ForEach(error => modelStateDictionary.AddModelError(error.Code, error.Description));

        return ValidationProblem(modelStateDictionary);
    }
}

Perhaps if we introduce ErrorOr.AspNetCore, it's possible to have something similar to the base controller above available for devs to derive from

[amirhessampourhossein]

Hello everyone
talking about having a separate nuget called 'ErrorOr.AspNetCore', what are your thoughts on the following extension methods 👇

public static partial class ErrorOrAspNetCoreExtensions
{
    public static IActionResult ToActionResult(this Error error) => error switch
    {
        { Type: not(ErrorType.Failure or ErrorType.Unexpected) } => MatchCommonErrorResults(error),
        _ => new StatusCodeResult(StatusCodes.Status500InternalServerError)
    };

    public static IActionResult ToActionResult(
        this Error error,
        int? statusCode = null,
        string? title = null,
        string? detail = null,
        string? instance = null,
        string? type = null) => error switch
        {
            { Type: not(ErrorType.Failure or ErrorType.Unexpected) } => MatchCommonErrorResults(error),
            _ => MatchProblemResult(
                statusCode,
                title,
                detail,
                instance,
                type),
        };

    public static OkObjectResult ToOk<TValue>(this ErrorOr<TValue> errorOr)
    {
        EnsureStateIsNotError(errorOr, nameof(OkObjectResult));

        return new(errorOr.Value);
    }

    public static CreatedResult ToCreated<TValue>(
        this ErrorOr<TValue> errorOr,
        string uri)
    {
        EnsureStateIsNotError(errorOr, nameof(CreatedResult));

        return new(uri, errorOr.Value);
    }

    public static CreatedResult ToCreated<TValue>(
        this ErrorOr<TValue> errorOr,
        Uri uri)
    {
        EnsureStateIsNotError(errorOr, nameof(CreatedResult));

        return new(uri, errorOr);
    }

    public static CreatedAtActionResult ToCreatedAtAction<TValue>(
        this ErrorOr<TValue> errorOr,
        string actionName,
        string controllerName,
        object routeValues)
    {
        EnsureStateIsNotError(errorOr, nameof(CreatedAtActionResult));

        return new(actionName, controllerName, routeValues, errorOr);
    }

    public static CreatedAtRouteResult ToCreatedAtRoute<TValue>(
        this ErrorOr<TValue> errorOr,
        object routeValues)
    {
        EnsureStateIsNotError(errorOr, nameof(CreatedAtRouteResult));

        return new(routeValues, errorOr);
    }

    public static CreatedAtRouteResult ToCreatedAtRoute<TValue>(
        this ErrorOr<TValue> errorOr,
        string routeName,
        object routeValues)
    {
        EnsureStateIsNotError(errorOr, nameof(CreatedAtRouteResult));

        return new(routeName, routeValues, errorOr);
    }

    public static AcceptedResult ToAccepted<TValue>(this ErrorOr<TValue> errorOr)
    {
        EnsureStateIsNotError(errorOr, nameof(AcceptedResult));

        return new(location: null, errorOr);
    }

    public static AcceptedResult ToAccepted<TValue>(
        this ErrorOr<TValue> errorOr,
        string uri)
    {
        EnsureStateIsNotError(errorOr, nameof(AcceptedResult));

        return new(uri, errorOr);
    }

    public static AcceptedResult ToAccepted<TValue>(
        this ErrorOr<TValue> errorOr,
        Uri uri)
    {
        EnsureStateIsNotError(errorOr, nameof(AcceptedResult));

        return new(uri, errorOr);
    }

    private static void EnsureStateIsNotError<TValue>(
        ErrorOr<TValue> errorOr,
        string targetTypeName)
    {
        if (errorOr.IsError)
        {
            string message = $"Cannot convert to '{targetTypeName}' because the state is error";

            throw new InvalidOperationException(message);
        }
    }

    private static IActionResult MatchCommonErrorResults(Error error) => error switch
    {
        { Type: ErrorType.NotFound } => new NotFoundResult(),
        { Type: ErrorType.Validation } => new BadRequestResult(),
        { Type: ErrorType.Unauthorized } => new UnauthorizedResult(),
        { Type: ErrorType.Conflict } => new ConflictResult(),
        _ => throw new ArgumentException("Invalid error type!", nameof(error)),
    };

    private static ObjectResult MatchProblemResult(
    int? statusCode,
    string? title = null,
    string? detail = null,
    string? instance = null,
    string? type = null)
    {
        ProblemDetails problem = new()
        {
            Status = statusCode ?? StatusCodes.Status500InternalServerError,
            Instance = instance,
            Detail = detail,
            Title = title,
            Type = type,
        };

        return new(problem);
    }
}

[amirhessampourhossein]

By the way, I was thinking about having extension methods for minimal APIs. but there is a problem,
we would have methods like below👇

    public static IActionResult ToOk<TValue>(this ErrorOr<TValue> errorOr)
    {
        return new OkObjectResult(errorOr.Value);
    }

    public static IResult ToOk<TValue>(this ErrorOr<TValue> errorOr)
    {
        return Results.Ok(errorOr.value);
    }

how do you think we should work this around ?

[xavierjohn]

This is how I solved it.

https://github.com/xavierjohn/FunctionalDDD/tree/main/Asp

[stewartcelani]

The method I've settled on is to create an Error extension method ToActionResult that sits in the API layer.

Love the library!

  [Authorize(Roles = AuthConstants.Roles.Jobs.Read)]
  [HttpGet(ApiEndpoints.Jobs.GetJobByCode.Url)]
  public async Task<IActionResult> GetJobByCode(string? jobCode, CancellationToken token)
  {
      var getJobResult =
          await _mediator.Send(new GetJobQuery(HttpContext.ToExecutionContext(), string.Empty, jobCode), token);
      if (getJobResult.IsError) return getJobResult.FirstError.ToActionResult();
      var job = getJobResult.Value;
      var jobResponse = job.ToJobResponse();
      return Ok(jobResponse);
  }

public static class ActionResultMapper
{
    public static IActionResult ToActionResult(this Error error)
    {
        var problemDetails = new ProblemDetails
        {
            Title = error.Type.ToString(),
            Extensions = { ["code"] = error.Code },
            Detail = error.Description
        };

        return error.Type switch
        {
            ErrorType.Validation => new ObjectResult(problemDetails) { StatusCode = StatusCodes.Status400BadRequest },
            ErrorType.NotFound => new ObjectResult(problemDetails) { StatusCode = StatusCodes.Status404NotFound },
            ErrorType.Unauthorized => new ObjectResult(problemDetails)
                { StatusCode = StatusCodes.Status401Unauthorized },
            ErrorType.Forbidden => new ObjectResult(problemDetails) { StatusCode = StatusCodes.Status403Forbidden },
            _ => GenerateGeneric500Error()
        };
    }

    private static IActionResult GenerateGeneric500Error()
    {
        var genericProblemDetails = new ProblemDetails
        {
            Title = "Internal Server Error",
            Detail = "An unexpected error occurred.",
            Status = StatusCodes.Status500InternalServerError
        };

        return new ObjectResult(genericProblemDetails) { StatusCode = StatusCodes.Status500InternalServerError };
    }
}

[amantinband]

Thanks for the suggestions everyone.

I started looking into implementing this feature, as I think it can be very useful for ASP.NET apps.

For minimal APIs the solution is simple and could look something like this:

public static class ErrorExtensions
{
    public static IResult ToProblemResult(this List<Error> errors)
    {
        if (errors.Count is 0)
        {
            return TypedResults.Problem();
        }

        if (errors.All(error => error.Type == ErrorType.Validation))
        {
            return errors.ToValidationProblemResult();
        }

        return errors[0].ToProblemResult();
    }

    private static ProblemHttpResult ToProblemResult(this Error error)
    {
        var statusCode = error.Type switch
        {
            ErrorType.Conflict => StatusCodes.Status409Conflict,
            ErrorType.Validation => StatusCodes.Status400BadRequest,
            ErrorType.NotFound => StatusCodes.Status404NotFound,
            ErrorType.Unauthorized => StatusCodes.Status403Forbidden,
            _ => StatusCodes.Status500InternalServerError,
        };

        return TypedResults.Problem(statusCode: statusCode, title: error.Description);
    }

    private static ValidationProblem ToValidationProblemResult(this List<Error> errors)
    {
        Dictionary<string, string[]> validationErrors = errors
            .GroupBy(error => error.Code)
            .ToDictionary(
                group => group.Key,
                group => group.Select(error => error.Description).ToArray());

        return TypedResults.ValidationProblem(validationErrors);
    }
}

Which takes into account any custom extensions or configurations defined for the problem details:

    builder.Services.AddProblemDetails(options =>
    {
        options.CustomizeProblemDetails = (context) =>
        {
            context.ProblemDetails.Extensions.Add("foo", "asdfasdfasdf");
        };
    });

The problem still remains for controllers, since the problem details customizations are applied via the ProblemDetailsFactory. The only way to get around this issue is calling the method Problem in the ControllerBase, or somehow getting hold of the factory.

One option is doing what @xavierjohn did, which is passing the controller base to the ToProblemResult method:

which doesn't sit right with me. A better option in this case IMO is passing the HttpContext (which we would then use to get hold of the problem details factory/service)

Here are the options I'm thinking about

Options 1: Making an extension method that turns the errors into a ProblemDetails. The caller would then use it however they wish

[HttpGet]
public IActionResult Foo()
{
    var problemDetails = errors.ToProblemDetails();
    return Problem(problemDetails.Title, ...);
}

app.MapGet("foo", () => Results.Problem(errors.ToProblemDetails()))

Options 2: Creating 2 methods, one for Minimal APIs and one for MVC Controllers

[HttpGet]
public IActionResult Foo()
{
    return result.Match(movie => Ok(movie), errors => errors.ToProblemResult(HttpContext))
}

app.MapGet("foo", () => result.Match(movie => Results.Ok(movie), errors => errors.ToProblemResult())

Feedback would be appreciated

What option above would you prefer?

• Option 1
• Option 2

If we go with option 2 (which I would personally prefer as the consumer of the ErrorOr package), what naming would you prefer?

Notice the methods can have the same name since there would be two overloads, one that takes the http context and one that doesn't.

• ToResult (minimal apis), ToActionResult (MVC controllers)
• ToProblemResult (minimal apis), ToProblemActionResult (MVC controllers)
• ToResult (for both)
• ToProblemResult (for both)
• ToHttpError (for both)
• something else

[amantinband]

Here is a full working implementation for both MVC controllers and minimal APIs:

public static class ErrorExtensions
{
    public static IActionResult ToProblemResult(this List<Error> errors, HttpContext httpContext)
    {
        ProblemDetails problemDetails = errors.ToProblemDetails();

        if (httpContext.RequestServices.GetService<ProblemDetailsFactory>() is ProblemDetailsFactory factory)
        {
            problemDetails = factory.CreateProblemDetails(
                httpContext,
                problemDetails.Status,
                problemDetails.Title,
                problemDetails.Type,
                problemDetails.Detail,
                problemDetails.Instance);
        }

        return new ObjectResult(problemDetails) { StatusCode = problemDetails.Status };
    }

    public static IResult ToProblemResult(this List<Error> errors)
    {
        return Results.Problem(errors.ToProblemDetails());
    }

    public static ProblemDetails ToProblemDetails(this List<Error> errors)
    {
        if (errors.Count is 0)
        {
            return TypedResults.Problem().ProblemDetails;
        }

        if (errors.All(error => error.Type == ErrorType.Validation))
        {
            return errors.ToValidationProblemResult().ProblemDetails;
        }

        return errors[0].ToProblemResult().ProblemDetails;
    }

    private static ProblemHttpResult ToProblemResult(this Error error)
    {
        var statusCode = error.Type switch
        {
            ErrorType.Conflict => StatusCodes.Status409Conflict,
            ErrorType.Validation => StatusCodes.Status400BadRequest,
            ErrorType.NotFound => StatusCodes.Status404NotFound,
            ErrorType.Unauthorized => StatusCodes.Status403Forbidden,
            _ => StatusCodes.Status500InternalServerError,
        };

        return TypedResults.Problem(statusCode: statusCode, title: error.Description);
    }

    private static ValidationProblem ToValidationProblemResult(this List<Error> errors)
    {
        Dictionary<string, string[]> validationErrors = errors
            .GroupBy(error => error.Code)
            .ToDictionary(
                group => group.Key,
                group => group.Select(error => error.Description).ToArray());

        return TypedResults.ValidationProblem(validationErrors);
    }
}

Waiting to hear some feedback before I move forward with this 🙂

[jackpercy-acl]

@amantinband I think that looks great 👍

In regards to your earlier question about this being in a separate ErrorOr.AspNetCore package, I think that makes a lot of sense and will avoid AspNetCore references in either domain/application projects. This new package can just be installed in the AspNetCore projects.

[chrisrlewis]

@amantinband

For me option 2, and ToProblemResult (for both)

For context I should mention that I haven't used ErrorOr yet, but have used the Ardalis Result library.

[shanerogers]

@amantinband looks good. I wonder, maybe this is a bigger question.

In my scenario for conflicts and bad requests I need for each error three properties.

1. Error code
2. Error message, and
3. Property name

This is so we can inform our customers what is wrong, and each consumer of the API can choose what error message to display. They don't have to parse the error message to understand what the issue is.

So to my question:

Can we have the ability to change the structure of the errors. and or introduce each error in your system to include a 'code' property?

[Andreas-Klapproth]

ErrorOr.AspNetCore

Yes, this package would be great!

[ahmtsen]

@amantinband looks great!

IMO, ErrorOr.AspNetCore should also use the Metadata property of Error and add the key/values to ProblemDetails as extensions.

[amantinband]

Hey guys, here's an early draft for the new APIs I'm thinking of adding as part of ErrorOr.AspNetCore:

Convert an Error or List<Error> to IActionResult

error.ToActionResult(httpContext);
errors.ToActionResult(httpContext);

Convert an Error or List<Error> to IResult

error.ToResult();
errors.ToResult();

Convert an Error or List<Error> to ProblemDetails

error.ToProblemDetails();
errors.ToProblemDetails();

Convert an Error or List<Error> to an http status code

error.ToHttpStatusCode();
errors.ToHttpStatusCode();

Configurations

serviceCollection.AddErrorOr(options =>
{
    options.IncludeMetadataInProblemDetails = true; // default is false
    options.ErrorsToProblemDetailsMapper.Add(errors =>
    {
        if (someCheck)
        {
            return null; // will use default mapping
        }

        return new ProblemDetails(..); // will not use default mapping, will respect IncludeMetadataInProblemDetails
    });
});

Thoughts?

[amantinband]

initial draft #107

[amantinband]

I see that the PR is barely readable (will fix it up), so here's a tl;dr of the planned changes and the APIs that will be available as part of ErrorOr.AspNetCore:

ErrorOr configurations

Small example

builder.Services.AddErrorOr(options =>
{
    options.IncludeMetadataInProblemDetails = true;

    options.ErrorToResultMapper.Add(error =>
    {
        return error.NumericType == 421
            ? Results.Ok("this is actually not an error")
            : null;
    });

    options.ErrorToStatusCodeMapper.Add(error =>
    {
        return error.NumericType switch
        {
            421 => StatusCodes.Status307TemporaryRedirect,
            68 => StatusCodes.Status202Accepted,
            _ => null,
        };
    });
});

All options

public class ErrorOrOptions
{
    public bool IncludeMetadataInProblemDetails { get; set; } = false; // whether error's metadata will be included in the problem details response
    public List<Func<List<Error>, IResult?>> ErrorListToResultMapper { get; set; } = [];
    public List<Func<Error, IResult?>> ErrorToResultMapper { get; set; } = [];
    public List<Func<List<Error>, IActionResult?>> ErrorListToActionResultMapper { get; set; } = [];
    public List<Func<Error, IActionResult?>> ErrorToActionResultMapper { get; set; } = [];
    public List<Func<List<Error>, ProblemDetails?>> ErrorListToProblemDetailsMapper { get; set; } = [];
    public List<Func<Error, ProblemDetails?>> ErrorToProblemDetailsMapper { get; set; } = [];
    public List<Func<Error, int?>> ErrorToStatusCodeMapper { get; set; } = [];
    public List<Func<Error, string?>> ErrorToTitleMapper { get; set; } = [];
}

Error conversions (imagine the equivalent for List<Error> in the examples below)

error.ToActionResult();
error.ToResult();
error.ToHttpStatusCode();
error.ToProblemDetails();
error.ToTitle();

Where basically the heuristics will be

1. custom mapper? use it
2. use default

for example:

    public static int ToHttpStatsCode(this Error error)
    {
        foreach (var mapping in ErrorOrOptions.Instance.ErrorToStatusCodeMapper)
        {
            if (mapping(error) is int statusCode)
            {
                return statusCode;
            }
        }

        return error.Type switch
        {
            ErrorType.Conflict => StatusCodes.Status409Conflict,
            ErrorType.Validation => StatusCodes.Status400BadRequest,
            ErrorType.NotFound => StatusCodes.Status404NotFound,
            ErrorType.Unauthorized => StatusCodes.Status403Forbidden,
            _ => StatusCodes.Status500InternalServerError,
        };
    }

That's more or less it. More implementation details in the spaghetti PR above.

Would love to hear some feedback before moving forward with this 🙏

[feO2x]

Dear Amichai,

I had a quick look at your implementation yesterday evening and I would like to put some implementation details up for discussion.

First of all, having this ASP.NET Core integration will be a huge win! We should also strife for other frameworks like GRPC's RpcException integration or some MassTransit integration - but we can discuss this in other issues.

About the current implementation:

1. The options are referenced via a static property. In my opinion, we should have an optional parameter in the corresponding ToProblemResult, ToActionResult, etc. methods so that callers are not coupled to a singleton that cannot be circumvented.
2. In my opinion, the properties on the options object are not intuitive and easy to comprehend. The first time I read the source code yesterday evening, it took me about 10 minutes to really grasp what each property is exactly for. Especially that there is a distinction between converting a single error and a list of errors was irritating to me. If we keep this design, we should carefully document it.
3. Personally, I would drastically reduce the properties on the options. Let's just have one delegate for MVC and one for Minimal APIs which converts a list of errors to the corresponding IActionResult or IResult. This would be easy to understand. Surely, passing a delegate would mean that we circumvent our default implementation completely, but this mapping logic shouldn't be too complicated anyway and we would avoid having all these foreach loops to pick a matching delegate before our default implementation kicks in.
4. Speaking of the default implementation: it should be so good that users wouldn't want to switch anyway. For this, we should increase the number of ErrorTypes to better map to HTTP 400 - 599 codes. I would consider adding 415 Unsupported Media Type, 451 Unavailable for Legal Reasons, 502 Bad Gateway, 503 Service Unavailable, 504 Gateway Timeout (and yes, I know they are niche response status codes).

Enough from me, now I'm really interested in your opinion. How do you feel about this?

[amantinband]

Fantastic feedback, thanks for taking the time.

1. Yes, that's part of the plan as well. If you have a different approach all together, feel free to suggest.
   2, 3. I feel you. Both on the naming and the over-configurability.
   The reason why implementing this is a bit cumbersome is because problem details can be configured on a service level, and ASP.NET's underlying implementation is quite different between minimal APIs and MVC. I want to continue respecting the service-wide problem details configuration if possible.
2. That's a good idea.

Let me play around with the syntax and naming a bit. A major selling point of ErrorOr is the intuitive naming, we don't want to be out of character 🤙

Please drop any suggestions you may have. Thanks!

[feO2x]

I will soon provide a PR to you and then we can discuss the details.

[feO2x]

After further inspecting the source code and the ProblemDetailsFactory functionality, I think I found an issue with the current implementation. Here's the ProblemDetailsFactory definition:

public abstract class ProblemDetailsFactory
{
    public abstract ProblemDetails CreateProblemDetails(
        HttpContext httpContext,
        int? statusCode = null,
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null
    );

    public abstract ValidationProblemDetails CreateValidationProblemDetails(
        HttpContext httpContext,
        ModelStateDictionary modelStateDictionary,
        int? statusCode = null,
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null
    );
}

The only out-of-the-box implementation is DefaultProblemDetailsFactory:

internal sealed class DefaultProblemDetailsFactory : ProblemDetailsFactory
{
    private readonly ApiBehaviorOptions _options;
    private readonly Action<ProblemDetailsContext>? _configure;

    public DefaultProblemDetailsFactory(
        IOptions<ApiBehaviorOptions> options,
        IOptions<ProblemDetailsOptions>? problemDetailsOptions = null)
    {
        _options = options?.Value ?? throw new ArgumentNullException(nameof(options));
        _configure = problemDetailsOptions?.Value?.CustomizeProblemDetails;
    }

    public override ProblemDetails CreateProblemDetails(
        HttpContext httpContext,
        int? statusCode = null,
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null)
    {
        statusCode ??= 500;

        var problemDetails = new ProblemDetails
        {
            Status = statusCode,
            Title = title,
            Type = type,
            Detail = detail,
            Instance = instance,
        };

        ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);

        return problemDetails;
    }

    public override ValidationProblemDetails CreateValidationProblemDetails(
        HttpContext httpContext,
        ModelStateDictionary modelStateDictionary,
        int? statusCode = null,
        string? title = null,
        string? type = null,
        string? detail = null,
        string? instance = null)
    {
        ArgumentNullException.ThrowIfNull(modelStateDictionary);

        statusCode ??= 400;

        var problemDetails = new ValidationProblemDetails(modelStateDictionary)
        {
            Status = statusCode,
            Type = type,
            Detail = detail,
            Instance = instance,
        };

        if (title != null)
        {
            // For validation problem details, don't overwrite the default title with null.
            problemDetails.Title = title;
        }

        ApplyProblemDetailsDefaults(httpContext, problemDetails, statusCode.Value);

        return problemDetails;
    }

    private void ApplyProblemDetailsDefaults(HttpContext httpContext, ProblemDetails problemDetails, int statusCode)
    {
        problemDetails.Status ??= statusCode;

        if (_options.ClientErrorMapping.TryGetValue(statusCode, out var clientErrorData))
        {
            problemDetails.Title ??= clientErrorData.Title;
            problemDetails.Type ??= clientErrorData.Link;
        }

        var traceId = Activity.Current?.Id ?? httpContext?.TraceIdentifier;
        if (traceId != null)
        {
            problemDetails.Extensions["traceId"] = traceId;
        }

        _configure?.Invoke(new() { HttpContext = httpContext!, ProblemDetails = problemDetails });
    }
}

As we can see, a ProblemDetails instance is created in CreateProblemDetails. In CreateValidationProblemDetails, a ValidationProblemDetails instance is created and the main difference is that there is a ModelStateDictionary which is converted to a Dictionary<string, string[]> for the validation problems.

Our ToActionResult is currently implemented in the following way:

public static IActionResult ToActionResult(this List<Error> errors, HttpContext? httpContext = null)
{
    foreach (var mapping in ErrorOrOptions.Instance.ErrorListToActionResultMapper)
    {
        if (mapping(errors) is IActionResult actionResult)
        {
            return actionResult;
        }
    }

    ProblemDetails problemDetails = errors.ToProblemDetails();

    if (httpContext?.RequestServices.GetService<ProblemDetailsFactory>() is ProblemDetailsFactory factory)
    {
        problemDetails = factory.CreateProblemDetails(
            httpContext,
            problemDetails.Status,
            problemDetails.Title,
            problemDetails.Type,
            problemDetails.Detail,
            problemDetails.Instance);
    }

    return new ObjectResult(problemDetails)
    {
        StatusCode = problemDetails.Status,
    };
}

If errors.ToProblemDetails() returns an HttpValidationProblemDetails instance, and we have access to a ProblemDetailsFactory via HttpContext, the instance will be replaced with a ProblemDetails instance, causing loss of errors previously appended to HttpValidationProblemDetails. This is a clear bug.

The more important thing is: if we want to incorporate ProblemDetailsFactory correctly, we need to call it when we instantiate the problem details instances. For validation errors, this would imply that our List<Error> would need to be projected to a ModelStateDictionary because ProblemDetailsFactory.CreateValidationProblemDetails is tightly coupled to the latter. My performance heart is bleeding: we go from List<Error> to ModelStateDictionary to Dictionary<string, string[] which is a lot unnecessary conversion. ModelStateDictionary is not as optimized as Dictionary<TKey, TValue> as it is a tree of single-linked nodes.

I strongly suggest that our default implementation does not go this route. In MVC, I would only do this if we have access to such a factory via a provided HttpContext. In Minimal APIs, I would not use ProblemDetailsFactory at all, especially because Results and TypedResults do not use them under the covers either. If our clients want to, they can opt-in via their own customization. I have worked on many ASP.NET Core MVC-based backends and almonst none of them actually customized ProblemDetailsFactory. Of course, we should make it easy for clients to opt-in but also warn them about potential performance issues.

I'm currently working on a PR - I hope I can finish it today.

[feO2x]

Hey guys,

I created an alternate implementation of this feature, you can see it here: #110

I'm interested in your feedback - I will try to enhance this PR in the upcoming days with more tests.

[amantinband]

After further inspecting the source code and the ProblemDetailsFactory functionality, I think I found an issue with the current implementation. Here's the ProblemDetailsFactory definition:

just to be clear - the PR was more of a POC to get some feedback, it's still missing many features and safe checks.

Thanks for creating the PR, will take a look now! 🫶

[celluj34]

@amantinband I am really looking forward to using this library and the ASPNet extensions, will they be available soon?