Skip to content

Commit 32a1ff6

Browse files
committed
fix: enforce non-null successful result values
1 parent bcd7254 commit 32a1ff6

7 files changed

Lines changed: 94 additions & 13 deletions

File tree

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ If I tell you to remember something, you do the same, update
99

1010

1111
## Rules to follow
12+
- A successful `Result<T>` must always carry a non-null `Value`, matching its `MemberNotNullWhen` annotations. Reject null success payloads at the owning result setter/factory, including nullable type arguments and initializer overrides. Use non-generic `Result.Succeed()` for a success without a payload; never preserve a successful null value to accommodate a consumer null guard.
1213
- Communication Orleans integration uses native generated Orleans serialization and its typed surrogates only. Do not add Newtonsoft.Json, JSON grain-storage adapters/converters, or automatic JSON storage registrations. Applications own their storage-provider serialization.
1314
- For failure logging, use Warning with failure details when there is no exception, and Error with the original stack trace when an exception is supplied.
1415
- For automatic Result failure telemetry, record the error and original exception immediately in the failure/catch path; never retain an exception reference in Result or Problem for later diagnostics.

‎Directory.Build.props‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@
2727
<RepositoryUrl>https://github.com/managedcode/Communication</RepositoryUrl>
2828
<PackageProjectUrl>https://github.com/managedcode/Communication</PackageProjectUrl>
2929
<Product>Managed Code - Communication</Product>
30-
<Version>10.3.5</Version>
31-
<PackageVersion>10.3.5</PackageVersion>
30+
<Version>10.3.6</Version>
31+
<PackageVersion>10.3.6</PackageVersion>
3232

3333
</PropertyGroup>
3434
<PropertyGroup Condition="'$(GITHUB_ACTIONS)' == 'true'">

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -215,18 +215,11 @@ public void ToActionResult_WithComplexObject_ReturnsCorrectValue()
215215
}
216216

217217
[Test]
218-
public void ToHttpResult_WithNullValue_HandlesGracefully()
218+
public void ToHttpResult_WithNullTypedSuccess_RejectsInvalidPayload()
219219
{
220-
// Arrange
221220
string? nullValue = null;
222-
var result = Result<string?>.Succeed(nullValue);
223-
224-
// Act
225-
var httpResult = result.ToHttpResult();
226221

227-
// Assert
228-
httpResult.ShouldNotBeNull();
229-
httpResult.GetType().Name.ShouldContain("Ok");
222+
Should.Throw<ArgumentNullException>(() => Result<string?>.Succeed(nullValue).ToHttpResult());
230223
}
231224

232225
[Test]
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
using System;
2+
using System.Text.Json;
3+
using Shouldly;
4+
5+
namespace ManagedCode.Communication.Tests.Results;
6+
7+
public class ResultSuccessValueContractTests
8+
{
9+
private const string Payload = "payload";
10+
private const string Failure = "missing";
11+
private const string ValueParameter = "value";
12+
private const string NullSuccessJson = "{\"isSuccess\":true,\"value\":null}";
13+
14+
[Test]
15+
public void Succeed_WithNullReference_RejectsNullPayload()
16+
{
17+
Should.Throw<ArgumentNullException>(() => Result<string>.Succeed((string)null!))
18+
.ParamName.ShouldBe(ValueParameter);
19+
}
20+
21+
[Test]
22+
public void Succeed_WithNullableNull_RejectsNullPayload()
23+
{
24+
Should.Throw<ArgumentNullException>(() => Result<string?>.Succeed((string?)null));
25+
Should.Throw<ArgumentNullException>(() => Result<int?>.Succeed((int?)null));
26+
}
27+
28+
[Test]
29+
public void Succeed_WithDefaultReference_RejectsNullPayload()
30+
{
31+
Should.Throw<ArgumentNullException>(() => Result<string>.Succeed());
32+
}
33+
34+
[Test]
35+
public void Succeed_WithDefaultValueType_PreservesValidDefault()
36+
{
37+
var result = Result<int>.Succeed();
38+
result.IsSuccess.ShouldBeTrue();
39+
result.Value.ShouldBe(0);
40+
}
41+
42+
[Test]
43+
public void Initializer_WithNullSuccessPayload_RejectsOverride()
44+
{
45+
var success = Result<string>.Succeed(Payload);
46+
Should.Throw<ArgumentNullException>(() => { _ = success with { Value = null }; });
47+
success.IsSuccess.ShouldBeTrue();
48+
success.Value.ShouldBe(Payload);
49+
}
50+
51+
[Test]
52+
public void Failure_WithNullPayload_PreservesFailure()
53+
{
54+
var result = Result<string>.FailNotFound(Failure) with { Value = null };
55+
result.IsFailed.ShouldBeTrue();
56+
result.Value.ShouldBeNull();
57+
result.Problem.ShouldNotBeNull();
58+
}
59+
60+
[Test]
61+
public void Deserialize_WithNullSuccessPayload_RejectsInvalidResult()
62+
{
63+
Should.Throw<ArgumentNullException>(() => JsonSerializer.Deserialize<Result<string>>(NullSuccessJson));
64+
}
65+
}

‎ManagedCode.Communication/ResultT/Result.cs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ namespace ManagedCode.Communication;
1818
[DebuggerDisplay("IsSuccess: {IsSuccess}; Problem: {Problem?.Title}")]
1919
public partial struct Result<T> : IResult<T>, IResultFactory<Result<T>>, IResultValueFactory<Result<T>, T>
2020
{
21+
private T? _value;
22+
2123
/// <summary>
2224
/// Initializes a new instance of the Result struct.
2325
/// </summary>
@@ -127,7 +129,19 @@ public bool TryGetProblem([MaybeNullWhen(false)] out Problem problem)
127129
/// </remarks>
128130
[JsonPropertyName(CommunicationJsonNames.Value)]
129131
[JsonPropertyOrder(2)]
130-
public T? Value { get; init; }
132+
public T? Value
133+
{
134+
get => _value;
135+
init
136+
{
137+
if (IsSuccess)
138+
{
139+
ArgumentNullException.ThrowIfNull(value);
140+
}
141+
142+
_value = value;
143+
}
144+
}
131145

132146
[JsonInclude]
133147
[JsonPropertyName(CommunicationJsonNames.Problem)]

‎ManagedCode.Communication/ResultT/ResultT.Succeed.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ namespace ManagedCode.Communication;
66
public partial struct Result<T>
77
{
88
/// <summary>
9-
/// Creates a success carrying the default value.
9+
/// Creates a success carrying the default value when that value is non-null.
1010
/// </summary>
11+
/// <exception cref="ArgumentNullException">The default value of T is null. Use non-generic Result for a success without a payload.</exception>
1112
public static Result<T> Succeed() => CreateSuccess(default!);
1213

1314
/// <summary>
1415
/// Creates a success carrying the value.
1516
/// </summary>
17+
/// <exception cref="ArgumentNullException">The supplied value is null.</exception>
1618
public static Result<T> Succeed(T value) => CreateSuccess(value);
1719

1820
/// <summary>

‎README.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,12 @@ A failed untyped `Result` or a `Problem` can become a typed failure. A successfu
120120
value to supply to `Result<T>`; converting it to a typed result produces a failure. Create typed successes
121121
from their actual values.
122122

123+
`Result<T>.IsSuccess` guarantees a non-null `Value`. Passing null to a typed success factory, or
124+
replacing a successful payload with null in an initializer, throws `ArgumentNullException`; a nullable
125+
C# type argument does not allow a successful null payload. `Result<int>.Succeed()` still carries zero,
126+
while `Result<string>.Succeed()` has no valid default payload and throws. For an operation with no
127+
data, return `Result.Succeed()`; for optional data, return a real value or an appropriate failure.
128+
123129
`Result` and `Result<T>` are structs. Their state is fixed after construction, but referenced payloads and
124130
`Problem` objects remain mutable. Avoid changing a shared problem after returning or publishing it.
125131
`default(Result)` is not a successful result; use a factory to state the intended outcome.

0 commit comments

Comments
 (0)