fix: preserve single errors through Combine and mint validation failures as validation errors (#324)

Author: Vulthil

docs/articles/result-pattern.md

@@ -24,8 +24,8 @@ Result<User> failed = Result.Failure<User>(error);
 
 // Validation failure
 var validation = new ValidationError([
-    Error.Problem("Email.Required", "Email is required"),
-    Error.Problem("Name.TooLong", "Name exceeds 100 characters")
+    Error.Validation("Email.Required", "Email is required"),
+    Error.Validation("Name.TooLong", "Name exceeds 100 characters")
 ]);
 Result<User> invalid = Result.ValidationFailure<User>(validation);
 ```
@@ -34,13 +34,17 @@ Result<User> invalid = Result.ValidationFailure<User>(validation);
 
 `ErrorType` determines how the error maps to HTTP status codes in the API layer:
 
-| ErrorType | Factory Method | HTTP Status |
-|---|---|---|
-| `Failure` | `Error.Failure(...)` | 500 Internal Server Error |
-| `NotFound` | `Error.NotFound(...)` | 404 Not Found |
-| `Problem` | `Error.Problem(...)` | 500 Internal Server Error |
-| `Conflict` | `Error.Conflict(...)` | 409 Conflict |
-| `Validation` | `ValidationError(...)` | 400 Bad Request |
+| ErrorType | Factory Method | HTTP Status | Meaning |
+|---|---|---|---|
+| `Failure` | `Error.Failure(...)` | 500 Internal Server Error | Unclassified/unexpected failure |
+| `NotFound` | `Error.NotFound(...)` | 404 Not Found | The requested resource doesn't exist |
+| `Problem` | `Error.Problem(...)` | 400 Bad Request | Client-addressable business-rule violation; full error detail is returned |
+| `Conflict` | `Error.Conflict(...)` | 409 Conflict | The request conflicts with current state |
+| `Validation` | `Error.Validation(...)` / `ValidationError` | 400 Bad Request | Input validation failure; `ValidationError.Errors` carries the per-field details |
+
+`Error.Validation` mints the *inner* errors of a `ValidationError` (this is what `ValidationPipelineBehavior` uses for
+FluentValidation failures). `Error.Problem` is for a single, standalone business-rule error — it is a different
+classification from `Validation`, not a supertype of it.
 
 ## Defining Domain Errors
 
@@ -94,6 +98,44 @@ string message = result.Match(
 
 All extensions have synchronous and asynchronous overloads so they compose naturally with `Task<Result<T>>`.
 
+### Combine / Zip – aggregate multiple results
+
+`Combine` (a sequence of `Result`) and `Zip` (exactly two `Result<T>`) both follow the same aggregation rule:
+
+- **All succeed** → success.
+- **Exactly one fails** → that failure's **original error** propagates unwrapped. Combining a single `NotFound`
+  failure still surfaces as `NotFound`, not as a validation failure.
+- **More than one fails** → the failures are aggregated into a single `ValidationError` whose `Errors` list holds
+  every failed error in order.
+
+```csharp
+Result combined = ResultExtensions.Combine(CheckName(), CheckEmail(), CheckAge());
+// combined.Error is the original NotFound/Conflict/etc. error if only one check failed,
+// or a ValidationError aggregating all of them if more than one failed.
+
+Result<(User, Order)> zipped = GetUser(userId).Zip(GetOrder(orderId));
+// same rule: a single failure propagates as-is; two failures aggregate into a ValidationError.
+```
+
+## Sharp Edges
+
+A few behaviors are easy to trip over. They are intentional for now; genuinely breaking fixes (marked below) are
+deferred to a future major version.
+
+- **Implicit `TValue? → Result<TValue>` conversion swallows `null` as a failure.** Assigning a `null` value to a
+  `Result<TValue>` compiles and silently produces a *failed* result carrying `Error.NullValue` — the conversion
+  picks the error for you, and there is no compiler warning at the call site. Prefer constructing the result
+  explicitly (`Result.Failure<TValue>(someDomainError)`) when the caller should choose the error. *(Making this
+  conversion explicit is a tracked future breaking change.)*
+- **`Result` and `Result<T>` have no value equality.** Both are plain classes with no `Equals`/`GetHashCode`
+  override, so two results are only equal by reference — two separately-constructed successes (or two
+  identically-failed results) are never `Equals`-equal. Compare `IsSuccess`/`Error`/`Value` instead of the result
+  itself. *(Adding value equality is a tracked future breaking change.)*
+- **A custom error that "looks like" `Error.None` is a valid failure error.** The `Result` constructor rejects a
+  failure whose error is literally the `Error.None` singleton, but a custom error with the same empty code and
+  description (e.g. `Error.Failure(string.Empty, string.Empty)`) is a distinct object and is accepted — it is not
+  treated as "no error" just because its field values match.
+
 ## Mapping to HTTP Responses
 
 `Vulthil.SharedKernel.Api` provides helpers that turn a `Result` into the correct HTTP response with typed results for OpenAPI documentation:

src/Vulthil.Results/Error.cs

@@ -64,7 +64,7 @@ protected internal Error(string code, string description, ErrorType type)
     /// <returns>A new <see cref="Error"/> with <see cref="ErrorType.Problem"/> classification.</returns>
     public static Error Problem(string code, string description) => new(code, description, ErrorType.Problem);
     /// <summary>
-    /// Creates a problem error.
+    /// Creates a validation error.
     /// </summary>
     /// <param name="code">The error code.</param>
     /// <param name="description">The error description.</param>
@@ -144,28 +144,34 @@ public override int GetHashCode()
 }
 
 /// <summary>
-/// Specifies values for ErrorType.
+/// Classifies an <see cref="Error"/> so that presentation layers (e.g. <c>Vulthil.SharedKernel.Api</c>) can map it
+/// to the appropriate HTTP status code without inspecting its code or description.
 /// </summary>
 public enum ErrorType
 {
     /// <summary>
-    /// Specifies the Failure value.
+    /// An unclassified or unexpected failure with no more specific meaning. Maps to HTTP 500 Internal Server Error.
+    /// This is the default classification produced by <see cref="Error.Failure(string, string)"/>.
     /// </summary>
     Failure = 0,
     /// <summary>
-    /// Specifies the Validation value.
+    /// One or more input values failed validation (e.g. FluentValidation rule failures). Carried by
+    /// <see cref="ValidationError"/>, whose <see cref="ValidationError.Errors"/> lists every individual violation.
+    /// Maps to HTTP 400 Bad Request as a validation-problem response with per-field details.
     /// </summary>
     Validation = 1,
     /// <summary>
-    /// Specifies the Problem value.
+    /// A client-addressable business-rule violation the caller can act on (distinct from a plain validation
+    /// failure). Maps to HTTP 400 Bad Request with the error's full detail in the problem response.
     /// </summary>
     Problem = 2,
     /// <summary>
-    /// Specifies the NotFound value.
+    /// The requested resource does not exist. Maps to HTTP 404 Not Found.
     /// </summary>
     NotFound = 3,
     /// <summary>
-    /// Specifies the Conflict value.
+    /// The request conflicts with the current state of the resource (e.g. a uniqueness violation). Maps to
+    /// HTTP 409 Conflict.
     /// </summary>
     Conflict = 4,
 }

src/Vulthil.Results/Extensions/ResultExtensions.Combine.cs

@@ -6,21 +6,30 @@ namespace Vulthil.Results;
 public static partial class ResultExtensions
 {
     /// <summary>
-    /// Combines multiple results, returning success when all succeed; otherwise a failure with a <see cref="ValidationError"/> aggregating every failed result's error.
+    /// Combines multiple results, returning success when all succeed; a single failure's original error when exactly
+    /// one fails; otherwise a failure with a <see cref="ValidationError"/> aggregating every failed result's error.
     /// </summary>
     public static Result Combine(params Result[] results) =>
         Combine((IEnumerable<Result>)results);
     /// <summary>
-    /// Combines a sequence of results, returning success when all succeed; otherwise a failure with a <see cref="ValidationError"/> aggregating every failed result's error.
+    /// Combines a sequence of results, returning success when all succeed; a single failure's original error when
+    /// exactly one fails; otherwise a failure with a <see cref="ValidationError"/> aggregating every failed result's error.
     /// </summary>
     public static Result Combine(this IEnumerable<Result> results)
     {
         Error[] failedErrors = [.. results.Where(result => result.IsFailure).Select(result => result.Error)];
-        return failedErrors.Length == 0 ? Result.Success() : Result.Failure(new ValidationError(failedErrors));
+        return failedErrors.Length switch
+        {
+            0 => Result.Success(),
+            1 => Result.Failure(failedErrors[0]),
+            _ => Result.Failure(new ValidationError(failedErrors))
+        };
     }
 
     /// <summary>
-    /// Asynchronously awaits and combines a sequence of result tasks, returning success when all succeed; otherwise a failure with a <see cref="ValidationError"/> aggregating every failed result's error.
+    /// Asynchronously awaits and combines a sequence of result tasks, returning success when all succeed; a single
+    /// failure's original error when exactly one fails; otherwise a failure with a <see cref="ValidationError"/>
+    /// aggregating every failed result's error.
     /// </summary>
     public static async Task<Result> CombineAsync(this IEnumerable<Task<Result>> resultTasks) =>
         (await Task.WhenAll(resultTasks)).Combine();

src/Vulthil.Results/Result.cs

@@ -4,6 +4,11 @@ namespace Vulthil.Results;
 /// <summary>
 /// Represents the outcome of an operation that may succeed or fail.
 /// </summary>
+/// <remarks>
+/// <see cref="Result"/> does not define value equality: two instances compare by reference, so two successful (or
+/// two identically-failed) results built separately are never <c>Equals</c>-equal. Compare <see cref="IsSuccess"/>
+/// and <see cref="Error"/> instead of the result itself. Value equality is tracked as a future breaking change.
+/// </remarks>
 public class Result
 {
     /// <summary>
@@ -25,11 +30,14 @@ public class Result
     /// </summary>
     /// <param name="isSuccess">Whether the result represents success.</param>
     /// <param name="error">The error, or <see cref="Error.None"/> for success.</param>
-    /// <exception cref="ArgumentException">Thrown when the success state and error are inconsistent.</exception>
+    /// <exception cref="ArgumentException">Thrown when the success state and error are inconsistent: a success
+    /// carrying an error other than the <see cref="Error.None"/> instance, or a failure carrying the
+    /// <see cref="Error.None"/> instance itself. The check is an identity check, so a custom error that merely
+    /// looks like <see cref="Error.None"/> (e.g. an empty code and description) is accepted as a valid failure.</exception>
     protected internal Result(bool isSuccess, Error error)
     {
-        if (isSuccess && error != Error.None ||
-            !isSuccess && error == Error.None)
+        if (isSuccess && !ReferenceEquals(error, Error.None) ||
+            !isSuccess && ReferenceEquals(error, Error.None))
         {
             throw new ArgumentException("Invalid error", nameof(error));
         }
@@ -78,6 +86,12 @@ protected internal Result(bool isSuccess, Error error)
 /// Represents the outcome of an operation that returns a value of type <typeparamref name="TValue"/> on success.
 /// </summary>
 /// <typeparam name="TValue">The type of the success value.</typeparam>
+/// <remarks>
+/// Like <see cref="Result"/>, <see cref="Result{TValue}"/> does not define value equality: two instances compare
+/// by reference, even when they carry the same value or error. Compare <see cref="Result.IsSuccess"/> and
+/// <see cref="Value"/>/<see cref="Result.Error"/> instead of the result itself. Value equality is tracked as a
+/// future breaking change.
+/// </remarks>
 public class Result<TValue> : Result
 {
 #pragma warning disable IDE0032 // Use auto property
@@ -105,6 +119,13 @@ protected internal Result(TValue? value, bool isSuccess, Error error)
     /// <summary>
     /// Implicitly converts a value to a successful result, or a failed result if the value is <see langword="null"/>.
     /// </summary>
+    /// <remarks>
+    /// <b>Sharp edge:</b> a <see langword="null"/> value silently converts to a <em>failed</em> result carrying
+    /// <see cref="Error.NullValue"/> — the conversion picks the error for you, and the failure is easy to miss at
+    /// the call site since it looks like an ordinary assignment. Prefer an explicit <see cref="Result.Failure{TValue}"/>
+    /// call with a domain-specific error when the caller should choose the error. Making this conversion explicit
+    /// (removing the implicit null-to-failure behavior) is tracked as a future breaking change.
+    /// </remarks>
     /// <param name="value">The value to convert.</param>
     public static implicit operator Result<TValue>(TValue? value) =>
         value is not null ? Success(value) : Failure<TValue>(Error.NullValue);

src/Vulthil.SharedKernel.Application/Behaviors/ValidationPipelineBehavior.cs

@@ -80,5 +80,5 @@ private async Task<ValidationFailure[]> ValidateAsync(TCommand command, Cancella
     }
 
     private static ValidationError CreateValidationError(ValidationFailure[] validationFailures) =>
-        new(validationFailures.Select(f => Error.Problem(f.ErrorCode, f.ErrorMessage)));
+        new(validationFailures.Select(f => Error.Validation(f.ErrorCode, f.ErrorMessage)));
 }

tests/Vulthil.Results.Tests/Errors/ErrorTests.cs

@@ -37,6 +37,7 @@ public void ErrorNullValueShouldBeFailure()
         { Error.Problem("C", "D"), ("C", "D"), ErrorType.Problem },
         { Error.Conflict("C", "D"), ("C", "D"), ErrorType.Conflict },
         { Error.Failure("C", "D"), ("C", "D"), ErrorType.Failure },
+        { Error.Validation("C", "D"), ("C", "D"), ErrorType.Validation },
         { new ValidationError([Error.NullValue]), ("Validation.General", "One or more validation errors occurred"), ErrorType.Validation },
     };
 

tests/Vulthil.Results.Tests/Extensions/CombinatorTests.cs

@@ -293,7 +293,32 @@ public void CombineAggregatesFailedErrors()
     }
 
     [Fact]
-    public async Task CombineAsyncAggregatesFailedErrors()
+    public void CombinePropagatesOriginalErrorForSingleFailure()
+    {
+        // Arrange
+        var results = new[] { Result.Success(), Result.Failure(FirstError) };
+
+        // Act
+        var combined = results.Combine();
+
+        // Assert
+        combined.IsFailure.ShouldBeTrue();
+        combined.Error.ShouldBeSameAs(FirstError);
+    }
+
+    [Fact]
+    public void CombineParamsPropagatesOriginalErrorForSingleFailure()
+    {
+        // Act
+        var combined = ResultExtensions.Combine(Result.Success(), Result.Failure(FirstError));
+
+        // Assert
+        combined.IsFailure.ShouldBeTrue();
+        combined.Error.ShouldBeSameAs(FirstError);
+    }
+
+    [Fact]
+    public async Task CombineAsyncPropagatesOriginalErrorForSingleFailure()
     {
         // Arrange
         var resultTasks = new[]
@@ -305,10 +330,27 @@ public async Task CombineAsyncAggregatesFailedErrors()
         // Act
         var combined = await resultTasks.CombineAsync();
 
+        // Assert
+        combined.IsFailure.ShouldBeTrue();
+        combined.Error.ShouldBeSameAs(FirstError);
+    }
+
+    [Fact]
+    public async Task CombineAsyncAggregatesMultipleFailures()
+    {
+        // Arrange
+        var resultTasks = new[]
+        {
+            Task.FromResult(Result.Failure(FirstError)),
+            Task.FromResult(Result.Failure(SecondError))
+        };
+
+        // Act
+        var combined = await resultTasks.CombineAsync();
+
         // Assert
         combined.Error.ShouldBeOfType<ValidationError>()
-            .Errors.ShouldHaveSingleItem()
-            .ShouldBe(FirstError);
+            .Errors.ShouldBe([FirstError, SecondError]);
     }
 
     [Fact]

tests/Vulthil.Results.Tests/Results/ResultTests.cs

@@ -72,6 +72,20 @@ public void ResultShouldThrowIfFailureIsNone()
         act.ShouldThrow<ArgumentException>();
     }
 
+    [Fact]
+    public void ResultShouldAllowFailureWithCustomErrorThatLooksLikeNone()
+    {
+        // Arrange
+        var lookAlike = Error.Failure(string.Empty, string.Empty);
+
+        // Act
+        var result = Result.Failure(lookAlike);
+
+        // Assert
+        result.IsFailure.ShouldBeTrue();
+        result.Error.ShouldBeSameAs(lookAlike);
+    }
+
     [Fact]
     public void ResultShouldThrowFailureWithValue()
     {

tests/Vulthil.SharedKernel.Application.Tests/Pipeline/ValidationPipelineBehaviorTests.cs

@@ -65,7 +65,7 @@ public async Task WithInvalidRequestReturnsValidationError()
         // Assert
         Assert.True(result.IsFailure);
         var validationError = Assert.IsType<ValidationError>(result.Error);
-        Assert.Contains(validationError.Errors, e => e.Code == "Name" && e.Description == "Name is required");
+        Assert.Contains(validationError.Errors, e => e.Code == "Name" && e.Description == "Name is required" && e.Type == ErrorType.Validation);
     }
 }
 
@@ -125,7 +125,7 @@ public async Task WithInvalidRequestReturnsFailedResultCarryingTheValidationErro
         // Assert
         Assert.True(result.IsFailure);
         var validationError = Assert.IsType<ValidationError>(result.Error);
-        Assert.Contains(validationError.Errors, e => e.Code == "Name" && e.Description == "Name is required");
+        Assert.Contains(validationError.Errors, e => e.Code == "Name" && e.Description == "Name is required" && e.Type == ErrorType.Validation);
     }
 }