Ensure failed results always have a Problem (#40)

* Partial rework to disallow invalid Result states

#32
#35

Begin work to improve Result state safety, ie disallow a Result to be both successful & failed, and to ensure failed results are never missing a Problem.
Made constructors for Result types private.
Replaced Result Create methods with CreateSuccess & CreateFailed to force parameters for either case.
Result.Problem will never be null when the result is unsuccessful. If no problem was recorded, it falls back to returning Problem.GenericError.

* CollectionResult changes, less reliance on HasProblem

Repeated changes from last commit in the CollectionResult type.
IsFailed and HasProblem properties just return the opposite of IsSuccess, since failed results now always have a Problem by design.
ThrowIfFail implementations don't rely on HasProblem. HasProblem is now functionally equivalent to IsFailed and is only necessary for the IResultProblem interface.

* Fix json deserialization

Added [JsonInclude] attribute to IsSuccess properties, as init setter is now private.
Moved Json-related attributes from Problem properties to _problem backing fields.
Added [JsonIgnore] attribute to Problem properties, as the backing field should now be serialized.
Added [JsonInclude] attribute to _problem fields.

* Corrected 'HasProblem' test conditions

Changed several tests that expect 'HasProblem' to be false in the case of Fail methods that previously did not assign a Problem.
This change has been made under the assumption that this is indeed an oversight: #32 (comment)

* Additional fixes

Change tests that use the AddInvalidMessage methods to operate on a failed result, since said methods now throw when invoked on a successful result.
Restore a private unused constructor for Result<T>, which is accessed via Activator.CreateInstance in CommunicationOutgoingGrainCallFilter

* Add tests for AddInvalidMessage throws

Added two new tests that assert the AddInvalidMessage methods should throw an InvalidOperationException when the result is successful

* Apply suggestion from @Copilot

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Improve 'AddInvalidMessage' usage

Moved main implementation of AddInvalidMessage methods to the Problem class (these were previously duplicated identically in all 3 Result types).
The same is done for InvalidField & InvalidFieldError methods.
AddInvalidMessage methods on Result types are now obsolete and no longer throw an exception when the result is successful.
Moved AddInvalidMessage test methods for CollectionResult type to instead test directly on the Problem type.

* Test fixes

Removed Problem.AddInvalidMessage methods added in last commit. Use the existing AddValidationError method instead which does the same thing, except safer.
All unit tests now passing again.

---------

Co-authored-by: ksemenenko <KSemenenko@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: 3 people

ManagedCode.Communication.AspNetCore/WebApi/Extensions/ControllerExtensions.cs

@@ -10,7 +10,7 @@ public static IActionResult ToActionResult<T>(this Result<T> result)
         if (result.IsSuccess)
             return new OkObjectResult(result.Value);
 
-        var problem = result.Problem ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
+        var problem = result.GetProblemNoFallback() ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
         return new ObjectResult(problem)
         {
             StatusCode = problem.StatusCode
@@ -22,7 +22,7 @@ public static IActionResult ToActionResult(this Result result)
         if (result.IsSuccess)
             return new NoContentResult();
 
-        var problem = result.Problem ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
+        var problem = result.GetProblemNoFallback() ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
         return new ObjectResult(problem)
         {
             StatusCode = problem.StatusCode
@@ -34,7 +34,7 @@ public static Microsoft.AspNetCore.Http.IResult ToHttpResult<T>(this Result<T> r
         if (result.IsSuccess)
             return Results.Ok(result.Value);
 
-        var problem = result.Problem ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
+        var problem = result.GetProblemNoFallback() ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
         return Results.Problem(
             title: problem.Title,
             detail: problem.Detail,
@@ -50,7 +50,7 @@ public static Microsoft.AspNetCore.Http.IResult ToHttpResult(this Result result)
         if (result.IsSuccess)
             return Results.NoContent();
 
-        var problem = result.Problem ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
+        var problem = result.GetProblemNoFallback() ?? Problem.Create("Operation failed", "Unknown error occurred", 500);
         return Results.Problem(
             title: problem.Title,
             detail: problem.Detail,

ManagedCode.Communication.Orleans/Converters/CollectionResultTSurrogateConverter.cs

@@ -9,8 +9,10 @@ public sealed class CollectionResultTSurrogateConverter<T> : IConverter<Collecti
 {
     public CollectionResult<T> ConvertFromSurrogate(in CollectionResultTSurrogate<T> surrogate)
     {
-        return CollectionResult<T>.Create(surrogate.IsSuccess, surrogate.Collection, surrogate.PageNumber, surrogate.PageSize, surrogate.TotalItems,
-            surrogate.Problem);
+        if (surrogate.IsSuccess)
+            return CollectionResult<T>.CreateSuccess(surrogate.Collection, surrogate.PageNumber, surrogate.PageSize, surrogate.TotalItems);
+
+        return CollectionResult<T>.CreateFailed(surrogate.Problem ?? Problem.GenericError(), surrogate.Collection);
     }
 
     public CollectionResultTSurrogate<T> ConvertToSurrogate(in CollectionResult<T> value)

ManagedCode.Communication.Orleans/Converters/ResultSurrogateConverter.cs

@@ -8,7 +8,10 @@ public sealed class ResultSurrogateConverter : IConverter<Result, ResultSurrogat
 {
     public Result ConvertFromSurrogate(in ResultSurrogate surrogate)
     {
-        return Result.Create(surrogate.IsSuccess, surrogate.Problem);
+        if (surrogate.IsSuccess)
+            return Result.Succeed();
+
+        return Result.CreateFailed(surrogate.Problem ?? Problem.GenericError());
     }
 
     public ResultSurrogate ConvertToSurrogate(in Result value)

ManagedCode.Communication.Orleans/Converters/ResultTSurrogateConverter.cs

@@ -8,7 +8,10 @@ public sealed class ResultTSurrogateConverter<T> : IConverter<Result<T>, ResultT
 {
     public Result<T> ConvertFromSurrogate(in ResultTSurrogate<T> surrogate)
     {
-        return Result<T>.Create(surrogate.IsSuccess, surrogate.Value, surrogate.Problem);
+        if (surrogate.IsSuccess)
+            return Result<T>.Succeed(surrogate.Value!);
+
+        return Result<T>.CreateFailed(surrogate.Problem ?? Problem.GenericError(), surrogate.Value);
     }
 
     public ResultTSurrogate<T> ConvertToSurrogate(in Result<T> value)

ManagedCode.Communication.Tests/AspNetCore/Extensions/ControllerExtensionsTests.cs

@@ -86,7 +86,7 @@ public void ToActionResult_WithValidationError_Returns400WithProblemDetails()
     public void ToActionResult_WithNoProblem_ReturnsDefaultError()
     {
         // Arrange - manually create failed result without problem
-        var result = new Result<string> { IsSuccess = false, Problem = null };
+        var result = (Result<string>)default;
 
         // Act
         var actionResult = result.ToActionResult();

ManagedCode.Communication.Tests/CollectionResults/CollectionResultFailMethodsTests.cs

@@ -28,7 +28,7 @@ public void Fail_NoParameters_ShouldCreateFailedResult()
         result.PageSize.Should().Be(0);
         result.TotalItems.Should().Be(0);
         result.TotalPages.Should().Be(0);
-        result.HasProblem.Should().BeFalse();
+        result.HasProblem.Should().BeTrue();
     }
 
     #endregion
@@ -48,7 +48,7 @@ public void Fail_WithEnumerable_ShouldCreateFailedResultWithItems()
         result.IsFailed.Should().BeTrue();
         result.Collection.Should().BeEquivalentTo(items);
         result.Collection.Should().HaveCount(5);
-        result.HasProblem.Should().BeFalse();
+        result.HasProblem.Should().BeTrue();
     }
 
     [Fact]
@@ -84,7 +84,7 @@ public void Fail_WithArray_ShouldCreateFailedResultWithItems()
         result.IsFailed.Should().BeTrue();
         result.Collection.Should().BeEquivalentTo(items);
         result.Collection.Should().HaveCount(3);
-        result.HasProblem.Should().BeFalse();
+        result.HasProblem.Should().BeTrue();
     }
 
     [Fact]

ManagedCode.Communication.Tests/CollectionResults/CollectionResultFromMethodsTests.cs

@@ -460,7 +460,7 @@ public void From_FailedCollectionResultWithoutProblem_ShouldReturnFailedResult()
 
         // Assert
         result.IsFailed.Should().BeTrue();
-        result.HasProblem.Should().BeFalse();
+        result.HasProblem.Should().BeTrue();
     }
 
     #endregion
@@ -507,7 +507,7 @@ public void From_GenericFailedCollectionResultWithoutProblem_ShouldReturnFailedR
 
         // Assert
         result.IsFailed.Should().BeTrue();
-        result.HasProblem.Should().BeFalse();
+        result.HasProblem.Should().BeTrue();
     }
 
     #endregion

ManagedCode.Communication.Tests/Extensions/ProblemExtensionsTests.cs

@@ -286,4 +286,43 @@ public void ToFailedResult_WithComplexExtensions_ShouldPreserveAllData()
         errors["email"].Should().Contain("Already exists");
         errors["password"].Should().Contain("Too short");
     }
-}
+
+    [Fact]
+    public void AddInvalidMessage_ShouldAddValidationError()
+    {
+        // Arrange
+        var problem = new Problem();
+
+        // Act
+        problem.AddValidationError("email", "Email is required");
+        problem.AddValidationError("email", "Email format is invalid");
+
+        // Assert
+        problem.InvalidField("email")
+            .Should()
+            .BeTrue();
+        var emailErrors = problem.InvalidFieldError("email");
+        emailErrors.Should()
+            .Contain("Email is required");
+        emailErrors.Should()
+            .Contain("Email format is invalid");
+    }
+
+    [Fact]
+    public void AddInvalidMessage_WithGeneralMessage_ShouldAddToGeneralErrors()
+    {
+        // Arrange
+        var problem = new Problem();
+
+        // Act
+        problem.AddValidationError("General error occurred");
+
+        // Assert
+        problem.InvalidField("_general")
+            .Should()
+            .BeTrue();
+        var generalErrors = problem.InvalidFieldError("_general");
+        generalErrors.Should()
+            .Be("General error occurred");
+    }
+}

ManagedCode.Communication.Tests/Results/CollectionResultTests.cs

@@ -292,45 +292,6 @@ public void InvalidFieldError_WithValidationProblem_ShouldReturnErrorMessage()
             .BeEmpty();
     }
 
-    [Fact]
-    public void AddInvalidMessage_ShouldAddValidationError()
-    {
-        // Arrange
-        var result = CollectionResult<string>.Empty();
-
-        // Act
-        result.AddInvalidMessage("email", "Email is required");
-        result.AddInvalidMessage("email", "Email format is invalid");
-
-        // Assert
-        result.InvalidField("email")
-            .Should()
-            .BeTrue();
-        var emailErrors = result.InvalidFieldError("email");
-        emailErrors.Should()
-            .Contain("Email is required");
-        emailErrors.Should()
-            .Contain("Email format is invalid");
-    }
-
-    [Fact]
-    public void AddInvalidMessage_WithGeneralMessage_ShouldAddToGeneralErrors()
-    {
-        // Arrange
-        var result = CollectionResult<string>.Empty();
-
-        // Act
-        result.AddInvalidMessage("General error occurred");
-
-        // Assert
-        result.InvalidField("_general")
-            .Should()
-            .BeTrue();
-        var generalErrors = result.InvalidFieldError("_general");
-        generalErrors.Should()
-            .Be("General error occurred");
-    }
-
     [Fact]
     public void ThrowIfFail_WithSuccessfulResult_ShouldNotThrow()
     {

ManagedCode.Communication.Tests/Results/ResultFailMethodsTests.cs

@@ -20,7 +20,7 @@ public void Result_Fail_NoParameters_ShouldCreateFailedResult()
         // Assert
         result.IsFailed.Should().BeTrue();
         result.IsSuccess.Should().BeFalse();
-        result.HasProblem.Should().BeFalse();
+        result.HasProblem.Should().BeTrue();
     }
 
     #endregion