Skip to content
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

ResultHelper.MergeWithValue enumerates results parameter multiple times #148

Closed
mtyski opened this issue Sep 15, 2022 · 2 comments
Closed

Comments

@mtyski
Copy link
Contributor

mtyski commented Sep 15, 2022

Extension method IEnumerable<Result<TValue>>.Merge() enumerates passed results collection multiple times. This might cause side effects to be introduced multiple times; the following code sample throws an InvalidOperationException:

using FluentResults;

var sth = new Something(
    Enumerable.Range(1, 5)
        .ToHashSet());

var result = Enumerable.Range(6, 2)
    .Select(number => sth.AddNumber(number))
    .Merge(); // Exception is thrown here;

Console.WriteLine($"Merger is a {(result.IsSuccess ? "success" : "failure")}");

internal class Something
{
    private readonly HashSet<int> numbers;

    public Something(HashSet<int> numbers)
    {
        this.numbers = numbers;
    }

    public Result<Something> AddNumber(int number) =>
        Result.OkIf(numbers.Add(number), "Can't add a duplicate!")
            .ToResult(this);
}

The workaround is to materialize the results enumerable before merging. Fixing that requires change in the implementation of MergeWithValue to one that enumerates the collection once; the simplest (and the most dumbed-down) refactor would resemble this:

public static Result<IEnumerable<TValue>> MergeWithValue<TValue>(IEnumerable<Result<TValue>> results)
{
    var values = new List<TValue>();
    var finalResult = Result.Ok<IEnumerable<TValue>>(Array.Empty<TValue>());

    foreach (var result in results)
    {
        finalResult.Reasons.AddRange(result.Reasons);
        if (result.IsSuccess)
        {
            values.Add(result.Value);
        }
    }

    return finalResult.IsSuccess ? finalResult.WithValue(values) : finalResult;
}

Alternatively, results can be materialized first and then the rest can remain mostly unchanged:

public static Result<IEnumerable<TValue>> MergeWithValue<TValue>(IEnumerable<Result<TValue>> results)
{
    var resultList = results.ToList();

    var finalResult = Result.Ok<IEnumerable<TValue>>(Array.Empty<TValue>())
        .WithReasons(resultList.SelectMany(result => result.Reasons));

    return finalResult.IsSuccess
        ? finalResult.WithValue(
            resultList.Select(r => r.Value)
                .ToList())
        : finalResult;
}
@altmann
Copy link
Owner

altmann commented Sep 18, 2022

Thanks for your feedback. I will have a look at it.

@altmann
Copy link
Owner

altmann commented Sep 18, 2022

Thanks again for creating the issue. The new fixed version will be released in the next hour... https://www.nuget.org/packages/FluentResults/3.13.0

@altmann altmann closed this as completed Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants