From b76106b61a1352f5d5c92e661f112ec33563ca9b Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 30 Jul 2026 14:55:48 +0100 Subject: [PATCH 1/7] feat(results): add required value access Give required dependencies a non-null Value accessor with module-specific failure context while keeping ValueOrDefault for optional data. Refs #3490. --- README.md | 2 +- docs/docs/fundamentals.md | 20 +++-- docs/docs/how-to/sharing-data.md | 18 +++- src/ModularPipelines/Models/ModuleResult.cs | 68 +++++++++++---- .../Modules/ModuleResultContractTests.cs | 87 ++++++++++++++++++- 5 files changed, 167 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index aa5ea5a543..d61a556024 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ public class PublishModule : Module protected override async Task ExecuteAsync(IModuleContext context, CancellationToken cancellationToken) { var buildResult = await context.GetModule(); - var outputPath = buildResult.ValueOrDefault!.OutputPath; // Strongly-typed, compile-time checked + var outputPath = buildResult.Value.OutputPath; // Throws with module context if unavailable // Publish using the build output... return None.Value; } diff --git a/docs/docs/fundamentals.md b/docs/docs/fundamentals.md index e73ec65418..c9bbeed750 100644 --- a/docs/docs/fundamentals.md +++ b/docs/docs/fundamentals.md @@ -51,16 +51,22 @@ Modules are strongly typed, so we can return clear, concrete objects, and other // Get a module's result var myModule = await context.GetModule(); -// Access the value using pattern matching (recommended) -if (myModule is ModuleResult.Success { Value: var result }) +// Access a required dependency value directly. +// This throws with module context if it failed, was skipped, or returned null. +var requiredValue = myModule.Value; +var firstString = requiredValue.MyFirstString; +var secondString = requiredValue.MySecondString; + +// Or use pattern matching when every outcome needs different handling +if (myModule is ModuleResult.Success { Value: var successfulValue }) { - var string1 = result.MyFirstString; - var string2 = result.MySecondString; + Console.WriteLine(successfulValue.MyFirstString); + Console.WriteLine(successfulValue.MySecondString); } -// Or use ValueOrDefault for simpler access -var string1 = myModule.ValueOrDefault?.MyFirstString; -var string2 = myModule.ValueOrDefault?.MySecondString; +// ValueOrDefault remains available when missing data is expected +var optionalFirstString = myModule.ValueOrDefault?.MyFirstString; +var optionalSecondString = myModule.ValueOrDefault?.MySecondString; ``` ## Custom Types diff --git a/docs/docs/how-to/sharing-data.md b/docs/docs/how-to/sharing-data.md index ae48a1cd85..4789b41ba4 100644 --- a/docs/docs/how-to/sharing-data.md +++ b/docs/docs/how-to/sharing-data.md @@ -21,8 +21,9 @@ public class DeployModule : Module // Get the build module's result var buildResult = await context.GetModule(); - // Access the value safely - var artifact = buildResult.ValueOrDefault?.ArtifactPath; + // Access the required dependency value. This throws with module context + // if the module failed, was skipped, or returned null. + var artifact = buildResult.Value.ArtifactPath; return await Deploy(artifact); } @@ -63,9 +64,20 @@ return result.Match( ); ``` +## Accessing Required Values + +Use `Value` when the dependency must have produced a non-null value. It returns `T` +without a null-forgiveness operator. If the module failed, was skipped, or returned +`null`, it throws an `InvalidOperationException` that identifies the module and outcome: + +```csharp +var result = await context.GetModule(); +var value = result.Value; +``` + ## Safe Accessors -For simpler checks, inspect the union through its safe accessors: +When an absent value is expected, inspect the union through its non-throwing accessors: ```csharp var result = await context.GetModule(); diff --git a/src/ModularPipelines/Models/ModuleResult.cs b/src/ModularPipelines/Models/ModuleResult.cs index fd7a24d2f1..0e27c2052e 100644 --- a/src/ModularPipelines/Models/ModuleResult.cs +++ b/src/ModularPipelines/Models/ModuleResult.cs @@ -218,6 +218,25 @@ private protected ModuleResult() [JsonConverter(typeof(ModuleResultJsonConverterFactory))] public abstract record ModuleResult : ModuleResult { + /// + /// Gets the successful non-null value. + /// + /// + /// The module failed, was skipped, or succeeded with a value. + /// + [JsonIgnore] + public T Value => this switch + { + Success { Value: not null } success => success.Value, + Success => throw new InvalidOperationException($"{ModuleName} succeeded but returned null"), + FailureWrapper failure => throw new InvalidOperationException( + $"{ModuleName} failed: {failure.Exception.Message}", + failure.Exception), + SkippedWrapper skipped => throw new InvalidOperationException( + $"{ModuleName} was skipped: {skipped.Decision.Reason ?? "No reason was provided"}"), + _ => throw new InvalidOperationException($"{ModuleName} has an unknown result type"), + }; + // === Safe accessors (no exceptions) === /// @@ -298,9 +317,32 @@ public void Switch( /// /// Represents a successful module execution with a value. /// - /// The value produced by the module, which may be null. [JsonConverter(typeof(ModuleResultJsonConverterFactory))] - public sealed record Success(T? Value) : ModuleResult; + public sealed record Success : ModuleResult + { + /// + /// Initialises a new instance of the class. + /// + /// The value produced by the module, which may be null. + public Success(T? value) + { + Value = value; + } + + /// + /// Gets the value produced by the module, which may be null. + /// + public new T? Value { get; init; } + + /// + /// Deconstructs the result into its successful value. + /// + /// The value produced by the module, which may be null. + public void Deconstruct(out T? value) + { + value = Value; + } + } /// /// Represents a failed module execution with an exception. @@ -455,8 +497,6 @@ internal sealed class ExceptionJsonConverter : JsonConverter string? typeName = null; string? message = null; - string? stackTrace = null; - while (reader.Read()) { if (reader.TokenType == JsonTokenType.EndObject) @@ -478,7 +518,6 @@ internal sealed class ExceptionJsonConverter : JsonConverter message = reader.GetString(); break; case "StackTrace": - stackTrace = reader.GetString(); break; } } @@ -495,8 +534,7 @@ internal sealed class ExceptionJsonConverter : JsonConverter { try { - var ex = Activator.CreateInstance(exceptionType, message) as Exception; - if (ex != null) + if (Activator.CreateInstance(exceptionType, message) is Exception ex) { return ex; } @@ -618,10 +656,10 @@ internal sealed class ModuleResultNonGenericJsonConverter : JsonConverter : JsonConverter result = success; + success.Deconstruct(out var deconstructedValue); + + using (Assert.Multiple()) + { + await Assert.That(result.Value).IsEqualTo(42); + await Assert.That(success.Value).IsEqualTo(42); + await Assert.That(deconstructedValue).IsEqualTo(42); + } + } + [Test] public async Task Generic_Skipped_Can_Be_Pattern_Matched() { @@ -164,6 +179,62 @@ public async Task Concrete_Generic_Skipped_Serializes_Through_Json() await Assert.That(deserialized!.SkipDecisionOrDefault?.Reason).IsEqualTo("Not needed"); } + [Test] + public async Task Success_Value_SurvivesJsonRoundTrip() + { + ModuleResult result = CreateSuccess(42); + + var json = JsonSerializer.Serialize(result); + var deserialized = JsonSerializer.Deserialize>(json); + + await Assert.That(deserialized!.Value).IsEqualTo(42); + } + + [Test] + public async Task Failure_Value_ThrowsWithModuleContext() + { + var failure = new InvalidOperationException("Compilation failed"); + ModuleResult result = CreateFailure(failure); + + var exception = await Assert.That(() => result.Value) + .Throws(); + + using (Assert.Multiple()) + { + await Assert.That(exception!.Message).IsEqualTo("IntModule failed: Compilation failed"); + await Assert.That(exception.InnerException).IsSameReferenceAs(failure); + } + } + + [Test] + public async Task Skipped_Value_ThrowsWithModuleContext() + { + ModuleResult result = CreateSkipped("No source changes"); + + var exception = await Assert.That(() => result.Value) + .Throws(); + + await Assert.That(exception!.Message).IsEqualTo("IntModule was skipped: No source changes"); + } + + [Test] + public async Task NullSuccess_Value_ThrowsWithModuleContext() + { + ModuleResult result = new ModuleResult.Success(null) + { + ModuleName = "NullableModule", + ModuleDuration = TimeSpan.Zero, + ModuleStart = DateTimeOffset.UtcNow, + ModuleEnd = DateTimeOffset.UtcNow, + ModuleStatus = Status.Successful, + }; + + var exception = await Assert.That(() => result.Value) + .Throws(); + + await Assert.That(exception!.Message).IsEqualTo("NullableModule succeeded but returned null"); + } + [Test] public async Task NullSuccess_TryGetValue_ReturnsTrue() { @@ -202,9 +273,9 @@ private static ModuleResult.Success CreateSuccess(int value) }; } - private static ModuleResult CreateFailure() + private static ModuleResult CreateFailure(Exception? exception = null) { - return new ModuleResult.Failure(new InvalidOperationException("Failed")) + return new ModuleResult.Failure(exception ?? new InvalidOperationException("Failed")) { ModuleName = nameof(IntModule), ModuleTypeName = typeof(IntModule).FullName, @@ -215,6 +286,18 @@ private static ModuleResult CreateFailure() }; } + private static ModuleResult CreateSkipped(string reason) + { + return new ModuleResult.Skipped(SkipDecision.Skip(reason)) + { + ModuleName = nameof(IntModule), + ModuleDuration = TimeSpan.Zero, + ModuleStart = DateTimeOffset.UtcNow, + ModuleEnd = DateTimeOffset.UtcNow, + ModuleStatus = Status.Skipped, + }; + } + private sealed class IntModule : Module { protected internal override Task ExecuteAsync( From 553d7a01a8e502b8eabaed3adba1b4578a2a7083 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:02:22 +0100 Subject: [PATCH 2/7] fix(results): make record formatting safe Exclude the throwing required accessor from generic record formatting while preserving the successful value output. Refs #3490 --- src/ModularPipelines/Models/ModuleResult.cs | 4 ++ .../Modules/ModuleResultContractTests.cs | 50 +++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/ModularPipelines/Models/ModuleResult.cs b/src/ModularPipelines/Models/ModuleResult.cs index 0e27c2052e..e5045d44a5 100644 --- a/src/ModularPipelines/Models/ModuleResult.cs +++ b/src/ModularPipelines/Models/ModuleResult.cs @@ -1,4 +1,5 @@ using System.Diagnostics.CodeAnalysis; +using System.Text; using System.Text.Json; using System.Text.Json.Serialization; using ModularPipelines.Engine; @@ -449,6 +450,9 @@ internal static Success CreateSuccess(T? value, ModuleExecutionContext ctx) /// protected override object? GetValueOrDefault() => ValueOrDefault; + /// + protected override bool PrintMembers(StringBuilder builder) => base.PrintMembers(builder); + // Prevent external inheritance - only Success, Failure, and Skipped are valid private protected ModuleResult() { diff --git a/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs b/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs index 1ae210ad7a..9c585ed00a 100644 --- a/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs +++ b/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs @@ -235,6 +235,56 @@ public async Task NullSuccess_Value_ThrowsWithModuleContext() await Assert.That(exception!.Message).IsEqualTo("NullableModule succeeded but returned null"); } + [Test] + public async Task Failure_ToString_DoesNotEvaluateRequiredValue() + { + ModuleResult result = CreateFailure(new InvalidOperationException("Compilation failed")); + + var formatted = result.ToString(); + + await Assert.That(formatted).Contains("Compilation failed"); + } + + [Test] + public async Task Skipped_ToString_DoesNotEvaluateRequiredValue() + { + ModuleResult result = CreateSkipped("No source changes"); + + var formatted = result.ToString(); + + await Assert.That(formatted).Contains("No source changes"); + } + + [Test] + public async Task NullSuccess_ToString_DoesNotEvaluateRequiredValue() + { + ModuleResult result = new ModuleResult.Success(null) + { + ModuleName = "NullableModule", + ModuleDuration = TimeSpan.Zero, + ModuleStart = DateTimeOffset.UtcNow, + ModuleEnd = DateTimeOffset.UtcNow, + ModuleStatus = Status.Successful, + }; + + var formatted = result.ToString(); + + await Assert.That(formatted).Contains(nameof(ModuleResult.Success)); + } + + [Test] + public async Task Success_ToString_PrintsValueOnce() + { + ModuleResult result = CreateSuccess(42); + + var formatted = result.ToString(); + var valueOccurrences = formatted + .Split("Value = 42", StringSplitOptions.None) + .Length - 1; + + await Assert.That(valueOccurrences).IsEqualTo(1); + } + [Test] public async Task NullSuccess_TryGetValue_ReturnsTrue() { From a631ef73355a7b1c127a11a34e8aa4e1cc6b2623 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:25:19 +0100 Subject: [PATCH 3/7] fix(results): preserve named argument Keep the public Success constructor parameter named Value so existing named-argument call sites continue compiling. Refs #3490 --- src/ModularPipelines/Models/ModuleResult.cs | 6 +++--- .../Modules/ModuleResultContractTests.cs | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/ModularPipelines/Models/ModuleResult.cs b/src/ModularPipelines/Models/ModuleResult.cs index e5045d44a5..fc80a4600b 100644 --- a/src/ModularPipelines/Models/ModuleResult.cs +++ b/src/ModularPipelines/Models/ModuleResult.cs @@ -324,10 +324,10 @@ public sealed record Success : ModuleResult /// /// Initialises a new instance of the class. /// - /// The value produced by the module, which may be null. - public Success(T? value) + /// The value produced by the module, which may be null. + public Success(T? Value) { - Value = value; + this.Value = Value; } /// diff --git a/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs b/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs index 9c585ed00a..857ca67d0e 100644 --- a/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs +++ b/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs @@ -179,6 +179,21 @@ public async Task Concrete_Generic_Skipped_Serializes_Through_Json() await Assert.That(deserialized!.SkipDecisionOrDefault?.Reason).IsEqualTo("Not needed"); } + [Test] + public async Task Success_Constructor_PreservesValueNamedArgument() + { + var success = new ModuleResult.Success(Value: 42) + { + ModuleName = nameof(IntModule), + ModuleDuration = TimeSpan.Zero, + ModuleStart = DateTimeOffset.UtcNow, + ModuleEnd = DateTimeOffset.UtcNow, + ModuleStatus = Status.Successful, + }; + + await Assert.That(success.Value).IsEqualTo(42); + } + [Test] public async Task Success_Value_SurvivesJsonRoundTrip() { From a1d4ca9f33d1fac9ecf64a52a0479f6c20184e7a Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:47:42 +0100 Subject: [PATCH 4/7] fix(results): preserve deconstruct API Keep the positional record's public Value parameter names for source compatibility. --- src/ModularPipelines/Models/ModuleResult.cs | 8 +++++--- .../Modules/ModuleResultContractTests.cs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/ModularPipelines/Models/ModuleResult.cs b/src/ModularPipelines/Models/ModuleResult.cs index fc80a4600b..a5a81ae0b8 100644 --- a/src/ModularPipelines/Models/ModuleResult.cs +++ b/src/ModularPipelines/Models/ModuleResult.cs @@ -321,6 +321,7 @@ public void Switch( [JsonConverter(typeof(ModuleResultJsonConverterFactory))] public sealed record Success : ModuleResult { +#pragma warning disable SA1313 // Preserve the public parameter names generated by the former positional record. /// /// Initialises a new instance of the class. /// @@ -338,11 +339,12 @@ public Success(T? Value) /// /// Deconstructs the result into its successful value. /// - /// The value produced by the module, which may be null. - public void Deconstruct(out T? value) + /// The value produced by the module, which may be null. + public void Deconstruct(out T? Value) { - value = Value; + Value = this.Value; } +#pragma warning restore SA1313 } /// diff --git a/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs b/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs index 857ca67d0e..ed7e9cb46f 100644 --- a/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs +++ b/test/ModularPipelines.UnitTests/Modules/ModuleResultContractTests.cs @@ -67,7 +67,7 @@ public async Task Success_Value_ReturnsValue() { var success = CreateSuccess(42); ModuleResult result = success; - success.Deconstruct(out var deconstructedValue); + success.Deconstruct(Value: out var deconstructedValue); using (Assert.Multiple()) { From a70c43d242b2bc2397c124a722b9ce14880fc45a Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 30 Jul 2026 17:38:56 +0100 Subject: [PATCH 5/7] docs(readme): sync required value example --- README_Template.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README_Template.md b/README_Template.md index e015f18974..16753ef86d 100644 --- a/README_Template.md +++ b/README_Template.md @@ -91,7 +91,7 @@ public class PublishModule : Module protected override async Task ExecuteAsync(IModuleContext context, CancellationToken cancellationToken) { var buildResult = await context.GetModule(); - var outputPath = buildResult.ValueOrDefault!.OutputPath; // Strongly-typed, compile-time checked + var outputPath = buildResult.Value.OutputPath; // Throws with module context if unavailable // Publish using the build output... return None.Value; } From c7b94051dac3ead5f457060b652c194fea2edf4b Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:43:55 +0100 Subject: [PATCH 6/7] docs(results): explain success value hiding --- src/ModularPipelines/Models/ModuleResult.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ModularPipelines/Models/ModuleResult.cs b/src/ModularPipelines/Models/ModuleResult.cs index a5a81ae0b8..bf8e9d7eea 100644 --- a/src/ModularPipelines/Models/ModuleResult.cs +++ b/src/ModularPipelines/Models/ModuleResult.cs @@ -334,6 +334,11 @@ public Success(T? Value) /// /// Gets the value produced by the module, which may be null. /// + /// + /// This property intentionally hides the required + /// accessor to preserve the nullable value carried by a known successful result. + /// Access through uses the required accessor instead. + /// public new T? Value { get; init; } /// From 365ef1d24f49438b6bfe2cf29ddd2b5ab5ac806e Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:11:49 +0100 Subject: [PATCH 7/7] fix(results): match generic result variants --- src/ModularPipelines/Models/ModuleResult.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ModularPipelines/Models/ModuleResult.cs b/src/ModularPipelines/Models/ModuleResult.cs index bf8e9d7eea..4f9e0865ab 100644 --- a/src/ModularPipelines/Models/ModuleResult.cs +++ b/src/ModularPipelines/Models/ModuleResult.cs @@ -230,10 +230,10 @@ public abstract record ModuleResult : ModuleResult { Success { Value: not null } success => success.Value, Success => throw new InvalidOperationException($"{ModuleName} succeeded but returned null"), - FailureWrapper failure => throw new InvalidOperationException( + Failure failure => throw new InvalidOperationException( $"{ModuleName} failed: {failure.Exception.Message}", failure.Exception), - SkippedWrapper skipped => throw new InvalidOperationException( + Skipped skipped => throw new InvalidOperationException( $"{ModuleName} was skipped: {skipped.Decision.Reason ?? "No reason was provided"}"), _ => throw new InvalidOperationException($"{ModuleName} has an unknown result type"), };