"Add utility method to combine multiple ErrorOr instances"

[alnaimi-github]

Feature Request

Summary: Add a utility method to combine multiple ErrorOr instances, collecting all errors into a single ErrorOr if any operation fails.

Details:

using System.Collections.Generic;

namespace ErrorOr
{
    public static class ErrorOrAggregationExtensions
    {
        public static ErrorOr<TValue> Combine<TValue>(params ErrorOr<TValue>[] errorOrs)
        {
            List<Error> allErrors = new();
            foreach (var errorOr in errorOrs)
            {
                if (errorOr.IsError)
                {
                    allErrors.AddRange(errorOr.Errors);
                }
            }

            return allErrors.Count > 0 ? ErrorOr<TValue>.From(allErrors) : errorOrs[0];
        }
    }
}

Use Case:
This method will allow users to easily aggregate errors from multiple operations into a single ErrorOr instance, simplifying error handling.

Example:

var success1 = new ErrorOr<int>(1);
var success2 = new ErrorOr<int>(2);
var failure = ErrorOr<int>.From(new List<Error> { Error.Validation("ValidationError", "Validation failed") });

var result = ErrorOrAggregationExtensions.Combine(success1, success2, failure);

if (result.IsError)
{
    foreach (var error in result.Errors)
    {
        Console.WriteLine(error.Description);
    }
}
else
{
    Console.WriteLine(result.Value);
}

[davidferneding]

I fell like an extension method would help to communicate which value will be used if no ErrorOr-object has any errors:

public static ErrorOr<TValue> AppendErrors<TValue>(this ErrorOr<TValue> errorOr, params ErrorOr<object>[] errors)
{
    List<Error> combinedErrors = errorOr.ErrorsOrEmptyList;
    foreach (ErrorOr<object> error in errors)
    {
        combinedErrors.AddRange(error.ErrorsOrEmptyList);
    }

    return combinedErrors.Count == 0 ? errorOr : combinedErrors;
}

var success1 = new ErrorOr<int>(1);
var success2 = new ErrorOr<int>(2);
var failure = ErrorOr<int>.From(new List<Error> { Error.Validation("ValidationError", "Validation failed") });

var result1 = success1.AppendErrors(success2);
var result2 = success1.AppendErrors(success2, failure);

result1.ThenDo(x => Console.WriteLine(x)).ElseDo(x => { foreach (var error in x) Console.WriteLine(error.Description); }); // writes 1
result2.ThenDo(x => Console.WriteLine(x)).ElseDo(x => { foreach (var error in x) Console.WriteLine(error.Description); }); // writes Validation failed

What do you think? If that looks good to you, I'll open a PR

[alnaimi-github]

The idea is really good, and the changes you've made are clear. However, I noticed that you're using ErrorOr<object> in the parameters of the AppendErrors method. It would be better to use ErrorOr<TValue> instead of ErrorOr<object>. The reason is that the method is originally designed to work with the TValue type, and using ErrorOr<object> could introduce unnecessary complications.

Here’s how you can modify the method to work with the correct type:

public static ErrorOr<TValue> AppendErrors<TValue>(this ErrorOr<TValue> errorOr, params ErrorOr<TValue>[] errors)
{
    List<Error> combinedErrors = errorOr.ErrorsOrEmptyList;
    foreach (ErrorOr<TValue> error in errors)
    {
        combinedErrors.AddRange(error.ErrorsOrEmptyList);
    }

    return combinedErrors.Count == 0 ? errorOr : combinedErrors;
}

If these changes look good to you, you can open a PR, and I'll review it. Thanks for your work!

[davidferneding]

I agree that ErrorOr<object>[] is not optimal. The reason I chose that instead of ErrorOr<TValue>[] was to allow combining ErrorOr instances of multiple types. Since on success, only the first instance is returned, the types of the other instances don't necessarily have to match. In most scenarios I can think of, the types will not be the same for all instances. E.g.:

public ErrorOr<User> AuthorizeAsAdmin() 
{
    ErrorOr<User> user = GetUser();
    ErrorOr<Success> hasPermission = CheckPermissions(user, "admin");
    ErrorOr<int> calcResult = (1 + 1).ToErrorOr();
    return user.AppendErrors(hasPermission, calcResult);
}

IErrorOr[] should probably be correct here, simply ignoring the type for everything but the first instance.

public static ErrorOr<TValue> AppendErrors<TValue>(this ErrorOr<TValue> errorOr, params IErrorOr[] errors)
{
    ...
}

[alnaimi-github]

Thanks for the explanation! I see your point about wanting to combine different types of ErrorOr instances into one call. You're right that in many cases, the types of the instances being combined won't match, and using IErrorOr[] allows for more flexibility in handling this scenario.

The idea of using IErrorOr[] makes sense because it provides a way to handle multiple different types without being restricted to a single type parameter. This would allow you to append errors across different types like ErrorOr, ErrorOr, and ErrorOr in the same operation.

However, one consideration I have is that using IErrorOr[] would mean we lose the type safety that comes with generics, since we would be dealing with a non-generic interface. This could potentially introduce runtime issues if someone were to accidentally mix incompatible types, though I understand that this is less of an issue when you only care about the errors and not the values.

I think this approach could be quite useful in many scenarios, but I'd also like to make sure that we maintain type safety where possible, especially if this becomes a commonly used extension method.

Would you be open to perhaps adding an overload for the AppendErrors method that allows both ErrorOr[] and IErrorOr[]? This way, we can have both flexible multi-type support and still maintain the option for type-safe handling when needed.

Thanks again for the suggestion!

[davidferneding]

Sure, I can add both versions, great suggestion 👍

[zbyszekprasak]

Combining multiple results would be very nice feature, but perhaps maybe there is more general approach possible.

I am afraid that use cases presented above - where only first result type matters - might rarely be the case in real world. I can imagine a lot of use cases when you have multiple results with different types and want to combine them to give either comosite object or list of all errors encountered so far. Is it doable with AggregateErrors somehow?

See classic example above:

record Person(string FirstName, string LastName, DateOnly DateOfBirth);

// Inputs from user - potentially invalid
ErrorOr<string> firstName = "John";
ErrorOr<string> lastName = "Doe";
ErrorOr<DateOnly> dateOfBirth = DateOnly.Parse("1990-03-19");

// Domain object - must be either valid or list of errors
ErrorOrFactory.Combine(firstName, lastName, dateOfBirth)
    .Then(t =>
    {
        // Tuple deconstruction into lambda args currently is not seem to be supported at the moment.
        var (firstName, lastName, dateOfBirth) = t;
        return new Person(firstName, lastName, dateOfBirth);
    })
    .Switch(
        person => WritePerson(Console.Out, person),
        errors => WriteErrors(Console.Error, errors));

In order to achieve above effect one can add bunch of methods like this:

public static ErrorOr<(T1, T2, T3)> Combine<T1, T2, T3>(ErrorOr<T1> v1, ErrorOr<T2> v2, ErrorOr<T3> v3)
{
    IErrorOr[] args = [v1, v2, v3];
    List<Error> errors = [];

    foreach (var arg in args)
    {
        if (arg.IsError)
        {
            errors.AddRange(arg.Errors!);
        }
    }

    return errors.Count > 0 ? errors : (v1.Value, v2.Value, v3.Value);
}

Downside of this solution is that only limited amout of such methods can be added i.e. up to 8-tuples.

@amantinband It would be nice to get some design guidelines and decisions in this issue before merging #125. Is there more elegant and general solution for this?

[davidferneding]

I agree, that would be a nice feature. The eight tuple restriction should probably not be an issue in most scenarios - I can't think of a situation where I had to construct a result object from more than eight different sources without questionable design decisions.

I think the previous proposal is still valid though. In most projects I've worked on, we had command handlers that call utility methods returning ErrorOr<Success>. With AppendErrors / #125, we'd be able to skip the explicit IsError-Checks for those return values if an early return is not necessary.

Some input from @amantinband would be great, yes. I don't think there is a rush to get this feature merged quickly.