diff --git a/AGENTS.md b/AGENTS.md index f7f9c9f5..e070cf82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,5 +28,3 @@ Read ./ai-plans/AGENTS.md for details on how to write plans. ## Here is Your Space If you encounter something worth noting while you are working on this code base, write it down here in this section. Once you are finished, I will discuss it with you, and we can decide where to put your notes. - -- `MetadataValueReconstructor` preserves `OperationCanceledException` through its exception filters, but this is not behaviorally testable through the generator's public surface: a pre-cancelled token is intercepted by Roslyn before reconstruction, and none of the whitelisted framework evaluations can throw cancellation. The contract therefore rests on the filters' structure unless an accepted evaluation gains a reachable cancellation path. diff --git a/ai-plans/0080-0-result-default-guard.md b/ai-plans/0080-0-result-default-guard.md new file mode 100644 index 00000000..a39de085 --- /dev/null +++ b/ai-plans/0080-0-result-default-guard.md @@ -0,0 +1,79 @@ +# Guard against default result instances at the write boundaries + +## Rationale + +`default(Result)` and `default(Result)` are neither a success nor a failure: `IsValid` is `false` while `Errors` is empty. The constructors enforce the "a failure carries at least one error" invariant, but C# hands out the default instance for free through array elements, uninitialized fields, `out` parameters and `default` literals, so the struct cannot defend itself. The library even produces one deliberately: `Validator.CheckForErrors` assigns `failure = default` on its success path, where the `false` return value tells the caller not to look at it. + +The damage is at the write boundaries. `default(Result).ToCloudEvent(...)` silently emits `{"lproutcome":"failure", ..., "data":{"errors":[]}}`, which this library's own reader then rejects with `JsonException`. For a library built on round-trip integrity, producing a message the consumer side cannot read is the worst failure shape — worse than throwing. The HTTP write path already fails, but only incidentally and with messages that never name the cause. Every write boundary should reject the default instance up front, with one exception type and one message that names the defect and the remedy. + +## Acceptance Criteria + +- [x] The public API of `Result`, `Result` and `IResultObject` is unchanged. +- [x] Serializing a default result over CloudEvents throws instead of emitting a payload, for the generic and non-generic result and for every public entry point: the byte-array, pooled, `Utf8JsonWriter` and envelope-factory overloads. *(see `0080-1-plan-deviations.md`: only `Result` with a nullable value can take the default shape, so the non-generic sites are guarded but unreachable.)* +- [x] A hand-constructed `CloudEventsEnvelopeForWriting`/`CloudEventsEnvelopeForWriting` carrying a default result is rejected by the JSON writing path, so the guard cannot be bypassed by skipping the envelope factory. *(see `0080-1-plan-deviations.md`: only `Result` with a nullable value can take the default shape, so the non-generic sites are guarded but unreachable.)* +- [x] Writing a default result over HTTP throws for both the wrapper factories and the JSON converters, including when a custom `CreateProblemDetailsInfo` delegate is configured. *(see `0080-1-plan-deviations.md`: only `Result` with a nullable value can take the default shape, so the non-generic sites are guarded but unreachable.)* +- [x] `HttpExtensions.SetStatusCodeFromResult`, `SetContentTypeFromResult` and `SetMetadataValuesAsHeadersIfNecessary` reject a default result, so a Minimal API or MVC endpoint returning one — directly or through an `IHttpResultEnricher` — fails before any status code, header or body byte reaches the response. +- [x] Every guarded site routes through one shared guard clause, throwing the same exception type and a message naming the default instance as the cause and `Result.Ok`/`Result.Fail` as the remedy. `Errors.GetLeadingCategory` and `ProblemDetailsInfo.CreateDefault` are no longer the observable failure for a default result. +- [x] Automated tests cover every guarded entry point, assert that no bytes are written when the guard trips, and pin the round-trip property that the library never writes a failure payload its own reader rejects. +- [x] The XML documentation states that a default result is neither a success nor a failure and cannot be written, and `Validator.CheckForErrors` documents that `failure` is meaningful only when it returns `true`. *(see `0080-1-plan-deviations.md`: the non-generic `Result` documents the opposite, because its default instance is a valid success.)* +- [x] Affected packages update ``. +- [x] The Stryker figures in `tests/AGENTS.md` are re-measured where this change invalidates them: the `Light.PortableResults` smoke-check test count, and the `AspNetCore.Shared` baseline row, whose mutant inventory grows with the `HttpExtensions` guards. + +## Technical Details + +### The guard clause + +One guard clause in the core package next to `IResultObject`, so the core write paths and `AspNetCore.Shared` share it, and no change to `Result`, `Result` or `IResultObject` at all: + +```csharp +public static TResult MustNotBeDefaultInstance( + this TResult result, + [CallerArgumentExpression("result")] string? parameterName = null +) where TResult : struct, IResultObject; +``` + +`IResultObject` already exposes `IsValid` and `Errors`, so the guard tests `!IsValid && Errors.Count == 0` with what is there today. The condition is exact: no non-default instance can be invalid with an empty `Errors` collection. Keeping the invariant in exactly one place is the point — a companion `IsDefaultInstance` property on the structs would restate it and could drift from the guard. `Error.IsDefaultInstance` stays the odd one out; a non-throwing predicate on the result structs is a cheap follow-up if a consumer need appears, but nothing in this change wants one. + +Returning `TResult` follows the `MustXxx` guard-clause convention and lets the guard sit where the value is captured rather than on a preceding line. + +Take the result by value, not by `in`. `in` is not expressible here in any case: a generic receiver rejects it for both classic extension methods (CS8338) and C# 14 extension blocks (CS9301), so `in` would cost the extension syntax outright. It would also buy nothing, because every guarded site already holds a by-value copy — `ToCloudEventsEnvelopeForWriting`, `ToHttpResultForWriting` and the `HttpExtensions` methods all take the result by value today, and the two converter sites read it out of a record-struct property getter. This also matches `CheckIfMetadataShouldBeWrittenForValidResult`, the existing generic struct receiver at these boundaries. Should the copies ever prove to matter, the lever is the enclosing signatures, not the guard. + +What does decide the codegen is inlining. Mark the guard `[MethodImpl(MethodImplOptions.AggressiveInlining)]` and outline the throw into a separate `[MethodImpl(MethodImplOptions.NoInlining)]` helper, so the inlineable body stays at two field reads and a branch and the argument copy disappears through forward substitution. Both result types are `readonly struct`, so member access adds no defensive copies. + +Throw `ArgumentException`. At every guarded site the offending result arrives as a parameter — including the converters, where the wrapper is the argument — so `ArgumentException` is accurate and keeps a single type across the boundary. + +Do not name the guard for validity. `IsValid` means "is a success", so a name built on it reads as rejecting failures, which is the opposite of the contract: any failure carrying at least one error must pass. + +### Where to guard + +The rule is: every public API that consumes a result in order to write it out. Two sites per transport, because the intermediate wrapper types are public and constructible by callers: + +| Transport | Fail-fast site | Non-bypassable site | +| --- | --- | --- | +| CloudEvents | `CloudEventsResultExtensions.ToCloudEventsEnvelopeForWriting` (both overloads) | `JsonCloudEventsExtensions.WriteCloudEvents` (both overloads) | +| HTTP body | `HttpResultForWritingExtensions.ToHttpResultForWriting` (all four overloads) | `HttpResultForWritingJsonConverter` / `HttpResultForWritingJsonConverter` `Write` | +| HTTP headers | — | `HttpExtensions.SetStatusCodeFromResult`, `SetContentTypeFromResult`, `SetMetadataValuesAsHeadersIfNecessary` | + +`ToCloudEvent`, `ToCloudEventPooled` and `WriteCloudEvent` need no guard of their own; all six funnel through the envelope factory. The `WriteCloudEvents` guard is what covers a caller who builds `CloudEventsEnvelopeForWriting` directly and hands it to the converter. + +Do not guard the `LightResult`, `LightResult`, `LightActionResult` and `LightActionResult` constructors. `IHttpResultEnricher.Enrich` runs during `ExecuteAsync` and can substitute a default result after construction, so a constructor guard is not sound on its own, while `SetHeaders` — which calls the guarded `HttpExtensions` methods on the enriched result — runs before anything is written to the response. One guard at the sound chokepoint is preferable to two that together still need the second one. + +Once inlined as described above, the guard is two field reads and a predictable branch on paths that immediately perform JSON serialization or HTTP header work. No microbenchmark is warranted. + +### Current behavior being replaced + +Worth knowing while writing the negative tests, since only one of the three is a silent corruption: + +- CloudEvents write emits `"data":{"errors":[]}` with no error at all; `CloudEventsDataJsonReader` rejects it on the way back in. +- HTTP body write throws `ArgumentException` from `ProblemDetailsInfo.CreateDefault` — but only with default problem-details creation. A configured `CreateProblemDetailsInfo` delegate bypasses that check and emits an empty `errors` array. +- `SetStatusCodeFromResult` throws `InvalidOperationException("Errors collection must contain at least one error.")` from `Errors.GetLeadingCategory`. It runs first in the ASP.NET Core pipeline, so this is the message users actually see today, and it names neither the result nor the default instance. + +Leave the checks in `Errors.GetLeadingCategory` and `ProblemDetailsInfo.CreateDefault` in place. They are general-purpose `Errors` guards, not result write boundaries; they simply stop being the first thing a default result hits. + +### Deliberately out of scope + +- **The functional extensions.** They are the hot in-memory path and none of them can manufacture a default result: `Result.Fail(Errors)` rejects an empty collection, so `Map`, `Bind`, `MapError` and friends already throw when fed one. They are inconsistent about it — `Match` and `Switch` hand the empty `Errors` to the caller's failure callback, `MatchFirst` throws from `FirstError` — but no path produces a corrupt payload, and guarding roughly forty methods buys nothing for round-trip integrity. +- **The struct itself.** No guard belongs in `Result`/`Result` members. `Validator.CheckForErrors` assigns `failure = default` on its success path by design, and a struct cannot intercept its own default construction anyway. +- **The read paths.** Readers build results through `Result.Ok`/`Result.Fail` and cannot produce a default instance. +- **Default instances of the wrapper structs.** `default(CloudEventsEnvelopeForWriting)` carries null `Type`, `Source` and `Id` — a separate defect in the same family, not part of this change. +- **The remaining v0.7.0 preparations.** This is item 2 of #77; Native AOT compatibility is #78 and `EnablePackageValidation` is item 3, tracked separately. diff --git a/ai-plans/0080-1-plan-deviations.md b/ai-plans/0080-1-plan-deviations.md new file mode 100644 index 00000000..ef5d3d07 --- /dev/null +++ b/ai-plans/0080-1-plan-deviations.md @@ -0,0 +1,98 @@ +# Plan Deviations for the Guard Against Default Result Instances + +## Referenced Plans + +- `0080-0-result-default-guard.md` introduced `MustNotBeDefaultInstance` next to `IResultObject` and applied it at + the CloudEvents, HTTP body, and HTTP header write boundaries, so that a result which is neither a success nor a + failure can never reach a transport. + +## Deviations + +### `default(Result)` is a success, not a defective instance + +The Rationale states that `default(Result)` **and** `default(Result)` are neither a success nor a failure. That +holds only for `Result` whose `default(T)` is null. + +`Result` encapsulates a `Result`, and `Unit` is a struct. `Result.IsValid` is `_value is not null && +_errors.Count is 0`, and for a value type `T` the first operand is always true. Measured on the current code: + +| Expression | `IsValid` | Equal to | +| --- | --- | --- | +| `default(Result)` | `true` | `Result.Ok()` | +| `default(Result)` | `true` | `Result.Ok(0)` | +| `default(Result)` | `false` | — (the defective instance) | +| `default(Result)` | `false` | — (the defective instance) | + +So `default(Result)` is bit-for-bit an ordinary success without metadata, and writes and round-trips like one. +The corruption the plan describes — `{"lproutcome":"failure", ..., "data":{"errors":[]}}` — is reachable only +through `Result` with a reference type or a nullable value type, which is the common case for typed results and +the case the plan's own example uses. + +This changes nothing about the guard: `!IsValid && Errors.Count == 0` still identifies the defective instance +exactly, and it remains the only condition needed. It changes what the guard can be observed to reject, and what +the documentation may claim. + +### The guards on the `Result`-typed sites are unreachable and were kept anyway + +Because no `Result` value can be invalid while carrying no errors, four guarded sites can never throw: + +- `CloudEventsResultExtensions.ToCloudEventsEnvelopeForWriting(this Result, ...)` +- `JsonCloudEventsExtensions.WriteCloudEvents(this Utf8JsonWriter, CloudEventsEnvelopeForWriting, ...)` +- `HttpResultForWritingExtensions.ToHttpResultForWriting(this Result, ...)` (both overloads) +- `HttpResultForWritingJsonConverter.Write` + +They are kept for uniformity: the invariant lives in one place, the inlined guard costs two field reads on a path +that immediately serializes JSON, and the sites become live if `Result` ever stops encapsulating a `Result`. +Expect their throw branches to show up as surviving mutants and as uncovered branches; that is a property of the +`Result` representation, not a missing test. The generic counterparts of all four are covered. + +The three `HttpExtensions` guards are a different case. They are constrained to `TResult : struct, IResultObject`, +and `IResultObject` is public, so a caller's own struct can be invalid with no errors. They are reachable +independently of `Result` and are tested both with `default(Result)` and with a hand-crafted +`IResultObject` implementation. + +### Documentation wording + +The acceptance criterion asks the XML documentation to state that "a default result is neither a success nor a +failure and cannot be written". `Result` qualifies that statement by the value type, while +`MustNotBeDefaultInstance` documents the exact rejected state and identifies a default nullable `Result` as its +usual source. On the non-generic `Result` the opposite is documented instead: its default instance is a valid +success, indistinguishable from `Result.Ok()`. Documenting the criterion verbatim there would have been false and +would invite a later "fix" that breaks the successful default. + +The exception message follows the same distinction. It names the invalid-without-errors state as the defect and a +default instance as its usual source, rather than asserting that every custom `IResultObject` rejected by the +guard must be its CLR default. It still gives `Result.Ok` and `Result.Fail` as the remedy for built-in results. + +`Validator.CheckForErrors` documents that `failure` is meaningful only when the method returns `true`, as required, +without repeating the claim that the assigned `default` is neither a success nor a failure — on `Result` it is a +success. + +### Mutation testing baselines + +The `AspNetCore.Shared` baseline row and the `Light.PortableResults` smoke-check test count in `tests/AGENTS.md` +were re-measured as required. Unrelated observation while doing so: the mutant inventory table's +`Light.PortableResults` row (4,867 mutants, 520 `CompileError`) was already stale before this change — the +smoke-check run reports 5,735 created mutants and 507 `CompileError` for the project. That row was left untouched, +because re-measuring it is not part of this change and attributing the drift to it would be misleading. + +## Housekeeping alongside this change + +Not deviations from the plan, but recorded here because they touch shared documents in the same branch. + +Two observations from this work are recorded where the knowledge is used: + +- The finding that `default(Result)` is a valid success is the first section of this file and is referenced from + the commit that introduces the guard. +- The existing `AGENTS.md` scratch note on `MetadataValueReconstructor` and `OperationCanceledException` was split, + leaving the scratch section empty. Why the two evaluation catch filters exclude cancellation is now a comment at + both filters in the generator, because that is where a reader needs it. That the contract is not observable + through the generator's public surface became a bullet in the blind spots section of `tests/AGENTS.md`, next to + the other "do not read as adequate coverage" entries, and it now names the trigger that makes it testable: an + accepted evaluation gaining a reachable cancellation path. + +A `README.md` for `Light.PortableResults.Validation.OpenApi.SourceGeneration` was considered and rejected for now. +The existing folder READMEs under `Numbers/`, `Text/` and `Metadata/` each carry a substantial, multi-faceted +concern, and a document holding a single caveat would sit one directory away from the code it explains. Revisit it +if the generator accumulates more design notes, such as the evaluation whitelist policy, the recursion depth limit, +or the incremental pipeline caching choices; the source comment stays regardless. diff --git a/src/Light.PortableResults.AspNetCore.Shared/HttpExtensions.cs b/src/Light.PortableResults.AspNetCore.Shared/HttpExtensions.cs index 8d80d6ec..3b8eadba 100644 --- a/src/Light.PortableResults.AspNetCore.Shared/HttpExtensions.cs +++ b/src/Light.PortableResults.AspNetCore.Shared/HttpExtensions.cs @@ -31,6 +31,9 @@ public static class HttpExtensions /// /// The concrete result struct implementing . /// Thrown when is null. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static void SetStatusCodeFromResult( this HttpResponse httpResponse, TResult result, @@ -40,6 +43,7 @@ public static void SetStatusCodeFromResult( where TResult : struct, IResultObject { ArgumentNullException.ThrowIfNull(httpResponse); + result.MustNotBeDefaultInstance(); HttpStatusCode statusCode; if (result.IsValid) @@ -62,6 +66,9 @@ public static void SetStatusCodeFromResult( /// Controls when metadata should be serialized. /// The concrete result struct implementing . /// Thrown when is null. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static void SetContentTypeFromResult( this HttpResponse httpResponse, TResult result, @@ -70,6 +77,7 @@ MetadataSerializationMode metadataSerializationMode where TResult : struct, IResultObject { ArgumentNullException.ThrowIfNull(httpResponse); + result.MustNotBeDefaultInstance(); if (!result.IsValid) { @@ -105,6 +113,9 @@ MetadataSerializationMode metadataSerializationMode /// Thrown when or /// is null. /// + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static void SetMetadataValuesAsHeadersIfNecessary( this HttpResponse httpResponse, TResult result, @@ -114,6 +125,7 @@ IHttpHeaderConversionService conversionService { ArgumentNullException.ThrowIfNull(httpResponse); ArgumentNullException.ThrowIfNull(conversionService); + result.MustNotBeDefaultInstance(); if (result.Metadata is null) { diff --git a/src/Light.PortableResults.AspNetCore.Shared/Light.PortableResults.AspNetCore.Shared.csproj b/src/Light.PortableResults.AspNetCore.Shared/Light.PortableResults.AspNetCore.Shared.csproj index 2aa162f6..8188c50d 100644 --- a/src/Light.PortableResults.AspNetCore.Shared/Light.PortableResults.AspNetCore.Shared.csproj +++ b/src/Light.PortableResults.AspNetCore.Shared/Light.PortableResults.AspNetCore.Shared.csproj @@ -12,6 +12,17 @@ - Compatible with .NET Native AOT. - Suppresses response headers when header conversion produces no values, including empty primitive arrays and deliberate empty output from custom converters. + + Breaking changes + --------------------------------- + + - HttpExtensions.SetStatusCodeFromResult, SetContentTypeFromResult, and SetMetadataValuesAsHeadersIfNecessary + now reject a result that is invalid while carrying no errors - usually the default instance of + Result<T> for a reference type or nullable value type - with an ArgumentException that names the invalid + state, identifies a default instance as its usual source, and gives Result.Ok/Result.Fail as the remedy. An + endpoint returning such a result now fails before any status code, header, or body byte reaches the + response, instead of throwing an InvalidOperationException from the errors collection that named neither + the result nor the default instance. diff --git a/src/Light.PortableResults.Validation.OpenApi.SourceGeneration/MetadataValueReconstructor.cs b/src/Light.PortableResults.Validation.OpenApi.SourceGeneration/MetadataValueReconstructor.cs index 98db897b..dc6f6512 100644 --- a/src/Light.PortableResults.Validation.OpenApi.SourceGeneration/MetadataValueReconstructor.cs +++ b/src/Light.PortableResults.Validation.OpenApi.SourceGeneration/MetadataValueReconstructor.cs @@ -163,6 +163,10 @@ out var arguments value = EvaluateConstructor(supportedConstructor, arguments); return MetadataReconstructionResult.Success; } + // The broad catch funnels every evaluation failure into Unsupported. Cancellation is excluded so that an + // aborted generation pass propagates instead of being downgraded to an unsupported value, which would bake + // the cancellation into the generator output. No whitelisted evaluation observes the token today, so the + // filter is structural rather than reachable - see the blind spot recorded in tests/AGENTS.md. catch (Exception exception) when (exception is not OperationCanceledException) { value = null; @@ -207,6 +211,7 @@ out var arguments value = EvaluateFactory(supportedFactory, arguments); return MetadataReconstructionResult.Success; } + // See TryReconstructObjectCreation for why cancellation is excluded from this filter. catch (Exception exception) when (exception is not OperationCanceledException) { value = null; diff --git a/src/Light.PortableResults.Validation/Validator.cs b/src/Light.PortableResults.Validation/Validator.cs index 42fa9247..a250b9bb 100644 --- a/src/Light.PortableResults.Validation/Validator.cs +++ b/src/Light.PortableResults.Validation/Validator.cs @@ -85,7 +85,11 @@ public Result Validate( /// Validates the value and materializes failures as a non-generic . /// /// The value to validate. - /// The failure result when validation fails. + /// + /// The failure result when validation fails. This value is only meaningful when this method returns + /// ; on success it is set to the default instance, which carries + /// no information about the validated value and must not be passed on to a caller as a result of its own. + /// /// The optional display name. Defaults to the caller-expression of . /// when validation failed; otherwise, . public bool CheckForErrors( @@ -105,7 +109,11 @@ public bool CheckForErrors( /// Validates the value and materializes failures as a non-generic using an explicit target descriptor. /// /// The value to validate. - /// The failure result when validation fails. + /// + /// The failure result when validation fails. This value is only meaningful when this method returns + /// ; on success it is set to the default instance, which carries + /// no information about the validated value and must not be passed on to a caller as a result of its own. + /// /// The explicit target descriptor. /// The optional display name. /// when validation failed; otherwise, . @@ -117,7 +125,11 @@ public bool CheckForErrors(T? value, out Result failure, ValidationTarget target /// /// The value to validate. /// The validation context. - /// The failure result when validation fails. + /// + /// The failure result when validation fails. This value is only meaningful when this method returns + /// ; on success it is set to the default instance, which carries + /// no information about the validated value and must not be passed on to a caller as a result of its own. + /// /// The optional display name. Defaults to the caller-expression of . /// when validation failed; otherwise, . public bool CheckForErrors( @@ -139,7 +151,11 @@ public bool CheckForErrors( /// /// The value to validate. /// The validation context. - /// The failure result when validation fails. + /// + /// The failure result when validation fails. This value is only meaningful when this method returns + /// ; on success it is set to the default instance, which carries + /// no information about the validated value and must not be passed on to a caller as a result of its own. + /// /// The explicit target descriptor. /// The optional display name. /// when validation failed; otherwise, . diff --git a/src/Light.PortableResults/CloudEvents/Writing/CloudEventsResultExtensions.cs b/src/Light.PortableResults/CloudEvents/Writing/CloudEventsResultExtensions.cs index 127098a0..81c068f0 100644 --- a/src/Light.PortableResults/CloudEvents/Writing/CloudEventsResultExtensions.cs +++ b/src/Light.PortableResults/CloudEvents/Writing/CloudEventsResultExtensions.cs @@ -39,7 +39,11 @@ public static class CloudEventsResultExtensions /// to keep downstream consumers idempotent and support routing and observability. /// /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// public static byte[] ToCloudEvent( this Result result, string? successType = null, @@ -91,7 +95,11 @@ public static byte[] ToCloudEvent( /// to keep downstream consumers idempotent and support routing and observability. /// /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// public static IRentedArray ToCloudEventPooled( this Result result, string? successType = null, @@ -141,7 +149,11 @@ public static IRentedArray ToCloudEventPooled( /// Customization options for serialization and metadata conversion. /// Thrown when is . /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// /// /// The required CloudEvents attributes type and source are resolved from the supplied arguments, the result metadata, /// or the configured defaults in . An is thrown when @@ -212,7 +224,11 @@ public static void WriteCloudEvent( /// to keep downstream consumers idempotent and support routing and observability. /// /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// public static CloudEventsEnvelopeForWriting ToCloudEventsEnvelopeForWriting( this Result result, string? successType = null, @@ -225,6 +241,7 @@ public static CloudEventsEnvelopeForWriting ToCloudEventsEnvelopeForWriting( PortableResultsCloudEventsWriteOptions? options = null ) { + result.MustNotBeDefaultInstance(); options ??= PortableResultsCloudEventsWriteOptions.Default; var convertedAttributes = ConvertMetadataToCloudEventsAttributes(result.Metadata, options.ConversionService); @@ -281,7 +298,11 @@ public static CloudEventsEnvelopeForWriting ToCloudEventsEnvelopeForWriting( /// to keep downstream consumers idempotent. /// /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// public static byte[] ToCloudEvent( this Result result, string? successType = null, @@ -334,7 +355,11 @@ public static byte[] ToCloudEvent( /// to keep downstream consumers idempotent and support routing and observability. /// /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// public static IRentedArray ToCloudEventPooled( this Result result, string? successType = null, @@ -382,7 +407,11 @@ public static IRentedArray ToCloudEventPooled( /// Customization options for serialization and metadata conversion. /// Thrown when is . /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// /// /// The required CloudEvents attributes type and source are resolved from the supplied arguments, the result metadata, /// or the configured defaults in . An is thrown when @@ -451,7 +480,11 @@ public static void WriteCloudEvent( /// to keep downstream consumers idempotent and support routing and observability. /// /// Thrown when the CloudEvents type or source cannot be resolved. - /// Thrown when , , or has an invalid format. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written, or + /// when , , or + /// has an invalid format. + /// public static CloudEventsEnvelopeForWriting ToCloudEventsEnvelopeForWriting( this Result result, string? successType = null, @@ -464,6 +497,7 @@ public static CloudEventsEnvelopeForWriting ToCloudEventsEnvelopeForWritingThe envelope whose metadata and error details will be emitted. /// The serializer options used for writing complex values. /// Thrown when is null. + /// + /// Thrown when the result in is invalid while carrying no errors and thus cannot + /// be written. + /// public static void WriteCloudEvents( this Utf8JsonWriter writer, CloudEventsEnvelopeForWriting envelope, @@ -30,6 +34,8 @@ JsonSerializerOptions serializerOptions throw new ArgumentNullException(nameof(writer)); } + envelope.Data.MustNotBeDefaultInstance(nameof(envelope)); + var shouldWriteMetadataToCloudEventsDataSectionWhenResultIsValid = envelope.CheckIfMetadataShouldBeWrittenForValidResult(); var shouldWriteData = !envelope.Data.IsValid || shouldWriteMetadataToCloudEventsDataSectionWhenResultIsValid; @@ -82,6 +88,10 @@ JsonSerializerOptions serializerOptions /// The envelope containing the typed payload and metadata. /// The serializer options used when writing the payload. /// Thrown when is null. + /// + /// Thrown when the result in is invalid while carrying no errors and thus cannot + /// be written. + /// public static void WriteCloudEvents( this Utf8JsonWriter writer, CloudEventsEnvelopeForWriting envelope, @@ -93,6 +103,8 @@ JsonSerializerOptions serializerOptions throw new ArgumentNullException(nameof(writer)); } + envelope.Data.MustNotBeDefaultInstance(nameof(envelope)); + WriteEnvelopeStart( writer, envelope.Type, diff --git a/src/Light.PortableResults/Http/Writing/HttpResultForWritingExtensions.cs b/src/Light.PortableResults/Http/Writing/HttpResultForWritingExtensions.cs index e5539866..7d0079d0 100644 --- a/src/Light.PortableResults/Http/Writing/HttpResultForWritingExtensions.cs +++ b/src/Light.PortableResults/Http/Writing/HttpResultForWritingExtensions.cs @@ -1,3 +1,5 @@ +using System; + namespace Light.PortableResults.Http.Writing; /// @@ -12,11 +14,14 @@ public static class HttpResultForWritingExtensions /// The result to wrap. /// The mutable options to freeze into the wrapper. /// The wrapper struct ready for JSON serialization. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static HttpResultForWriting ToHttpResultForWriting( this Result result, PortableResultsHttpWriteOptions options ) => - new (result, options.ToResolvedHttpWriteOptions()); + new (result.MustNotBeDefaultInstance(), options.ToResolvedHttpWriteOptions()); /// /// Creates an wrapper from the result and options. @@ -25,11 +30,14 @@ PortableResultsHttpWriteOptions options /// The result to wrap. /// The mutable options to freeze into the wrapper. /// The wrapper struct ready for JSON serialization. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static HttpResultForWriting ToHttpResultForWriting( this Result result, PortableResultsHttpWriteOptions options ) => - new (result, options.ToResolvedHttpWriteOptions()); + new (result.MustNotBeDefaultInstance(), options.ToResolvedHttpWriteOptions()); /// /// Creates an wrapper from the result and already-resolved options. @@ -37,11 +45,14 @@ PortableResultsHttpWriteOptions options /// The result to wrap. /// The already-frozen options. /// The wrapper struct ready for JSON serialization. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static HttpResultForWriting ToHttpResultForWriting( this Result result, ResolvedHttpWriteOptions resolvedOptions ) => - new (result, resolvedOptions); + new (result.MustNotBeDefaultInstance(), resolvedOptions); /// /// Creates an wrapper from the result and already-resolved options. @@ -50,9 +61,12 @@ ResolvedHttpWriteOptions resolvedOptions /// The result to wrap. /// The already-frozen options. /// The wrapper struct ready for JSON serialization. + /// + /// Thrown when is invalid while carrying no errors and thus cannot be written. + /// public static HttpResultForWriting ToHttpResultForWriting( this Result result, ResolvedHttpWriteOptions resolvedOptions ) => - new (result, resolvedOptions); + new (result.MustNotBeDefaultInstance(), resolvedOptions); } diff --git a/src/Light.PortableResults/Http/Writing/Json/HttpResultForWritingJsonConverter.cs b/src/Light.PortableResults/Http/Writing/Json/HttpResultForWritingJsonConverter.cs index 9675e7b4..8d528fc6 100644 --- a/src/Light.PortableResults/Http/Writing/Json/HttpResultForWritingJsonConverter.cs +++ b/src/Light.PortableResults/Http/Writing/Json/HttpResultForWritingJsonConverter.cs @@ -29,13 +29,17 @@ JsonSerializerOptions options ); /// + /// + /// Thrown when the result in is invalid while carrying no errors and thus cannot + /// be written. + /// public override void Write( Utf8JsonWriter writer, HttpResultForWriting wrapper, JsonSerializerOptions options ) { - var result = wrapper.Data; + var result = wrapper.Data.MustNotBeDefaultInstance(nameof(wrapper)); var resolvedOptions = wrapper.ResolvedOptions; if (result.IsValid) @@ -83,13 +87,17 @@ JsonSerializerOptions options ); /// + /// + /// Thrown when the result in is invalid while carrying no errors and thus cannot + /// be written. + /// public override void Write( Utf8JsonWriter writer, HttpResultForWriting wrapper, JsonSerializerOptions options ) { - var result = wrapper.Data; + var result = wrapper.Data.MustNotBeDefaultInstance(nameof(wrapper)); var resolvedOptions = wrapper.ResolvedOptions; if (result.IsValid) diff --git a/src/Light.PortableResults/Light.PortableResults.csproj b/src/Light.PortableResults/Light.PortableResults.csproj index fc426a03..e8f19926 100644 --- a/src/Light.PortableResults/Light.PortableResults.csproj +++ b/src/Light.PortableResults/Light.PortableResults.csproj @@ -37,6 +37,17 @@ Breaking changes --------------------------------- + - Every write boundary now rejects a result that is invalid while carrying no errors - usually the default + instance of Result<T> for a reference type or nullable value type - with an ArgumentException that names + the invalid state, identifies a default instance as its usual source, and gives Result.Ok/Result.Fail as the + remedy. This covers the CloudEvents entry points ToCloudEvent, ToCloudEventPooled, WriteCloudEvent, and + ToCloudEventsEnvelopeForWriting, the HTTP entry point ToHttpResultForWriting, and the JSON writing paths for + hand-constructed + CloudEventsEnvelopeForWriting<T> and HttpResultForWriting<T> values. Previously, CloudEvents writing + emitted a failure payload with an empty errors array that this library's own reader rejects, and HTTP + writing threw only when default problem-details creation was in use - a configured + CreateProblemDetailsInfo delegate emitted the empty errors array instead. + - Missing serialization metadata for a result value type now throws an InvalidOperationException that names the type and the remedy at every CloudEvents and HTTP entry point, instead of the NotSupportedException that System.Text.Json raised for the unresolved contract. @@ -75,6 +86,9 @@ - MetadataValue.TryGetInt64 now accepts canonical signed-integer text from any string metadata value, including values originating in headers, JSON, or caller code. Noncanonical forms such as a leading plus, negative zero, leading zeros, surrounding whitespace, and out-of-range text remain rejected. + - Adds the guard clause ResultObjectExtensions.MustNotBeDefaultInstance for any struct implementing + IResultObject. It is the single place where the write boundaries of this library reject a result that is + neither a success nor a failure. diff --git a/src/Light.PortableResults/Result.cs b/src/Light.PortableResults/Result.cs index b8e95c75..dbc585a7 100644 --- a/src/Light.PortableResults/Result.cs +++ b/src/Light.PortableResults/Result.cs @@ -8,7 +8,17 @@ namespace Light.PortableResults; /// +/// /// Represents either a successful value of or one or more errors. +/// +/// +/// Do not use the default instance of this struct. When is a reference type or a +/// nullable value type, the default instance is neither a success nor a failure: it reports +/// as while its collection is empty. Such an instance carries no +/// information and cannot be written to any transport - every write boundary of this library rejects it with an +/// . Always create results with or one of the Fail +/// overloads. +/// /// [DebuggerDisplay("{DebuggerDisplay,nq}")] public readonly struct Result : IResultObject, IEquatable>, ICanReplaceMetadata> @@ -253,6 +263,12 @@ public Result ReplaceMetadata(MetadataObject? metadata) => /// /// This is a convenience type for with as the value type. /// +/// +/// Unlike with a nullable value type, the default instance of this struct is a valid +/// success: the encapsulated Result<Unit> value can never be null, thus default(Result) is +/// indistinguishable from without metadata. Prefer and the Fail +/// overloads anyway, because they state the intent at the call site. +/// /// [DebuggerDisplay("{DebuggerDisplay,nq}")] public readonly struct Result : IResultObject, IEquatable, ICanReplaceMetadata diff --git a/src/Light.PortableResults/ResultObjectExtensions.cs b/src/Light.PortableResults/ResultObjectExtensions.cs new file mode 100644 index 00000000..240090c0 --- /dev/null +++ b/src/Light.PortableResults/ResultObjectExtensions.cs @@ -0,0 +1,59 @@ +using System; +using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; + +namespace Light.PortableResults; + +/// +/// Provides guard clauses for types implementing . +/// +public static class ResultObjectExtensions +{ + /// + /// + /// Ensures that the specified result represents either a success or a failure. + /// + /// + /// A result that reports as while its + /// collection is empty is neither a success nor a failure and carries no + /// information that could be written to a transport. Every write boundary of this library rejects that state + /// up front. default(Result<T>) takes this shape whenever T is a reference type or a nullable + /// value type; default results with non-nullable value types, including default(Result), are successes. + /// + /// + /// The result to check. + /// + /// The name of the parameter the result was passed to (optional). This value is automatically set to the + /// caller expression of when you do not specify it. + /// + /// The concrete result struct implementing . + /// The unchanged . + /// + /// Thrown when is invalid while carrying no errors. + /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static TResult MustNotBeDefaultInstance( + this TResult result, + [CallerArgumentExpression("result")] string? parameterName = null + ) + where TResult : struct, IResultObject + { + // No built-in result created via Result.Ok or Result.Fail can be invalid and carry no errors at the same + // time. Custom IResultObject implementations must uphold the same invariant at the write boundaries. + if (result is { IsValid: false, Errors.Count: 0 }) + { + ThrowInvalidWithoutErrors(parameterName); + } + + return result; + } + + [DoesNotReturn] + [MethodImpl(MethodImplOptions.NoInlining)] + private static void ThrowInvalidWithoutErrors(string? parameterName) => + throw new ArgumentException( + "The result is invalid while carrying no errors and thus cannot be written. This usually indicates " + + "the default instance. Create results with Result.Ok or Result.Fail.", + parameterName + ); +} diff --git a/tests/AGENTS.md b/tests/AGENTS.md index 763b9ca0..6663359d 100644 --- a/tests/AGENTS.md +++ b/tests/AGENTS.md @@ -36,14 +36,14 @@ Revisit `coverage-analysis: off` when Stryker's MTP per-test coverage support ([ # One project — sufficient for the four small projects dotnet stryker -p Light.PortableResults.AspNetCore.Shared.csproj -# One-file configuration smoke check (~4 minutes) -# Expect: 2,401 tests, 67 killed, 0 NoCoverage, 100.00% +# One-file configuration smoke check (~5 minutes) +# Expect: 2,675 tests, 67 killed, 0 NoCoverage, 100.00% dotnet stryker -p Light.PortableResults.csproj -m '**/Result.cs' ``` The smoke-check vector is tied to the current `Result.cs` and its tests; update it after intentional changes alter the mutant inventory. All mutants surviving with a 0.00% score suggests fallback to the `vstest` runner, while any non-zero `NoCoverage` count suggests `coverage-analysis: off` was lost. -`-p` selects the mutated project, not the tests: Stryker runs every test project transitively referencing it (`AspNetCore.Shared` → 337 tests, `Light.PortableResults` → all 2,401). Cross-project kills are legitimate (sociable tests) and nearly free. Never pass `-tp` (it does not narrow tests) or `--since` (any non-C# file in the diff, e.g. an `ai-plans/` document, degrades it to a full run). Reports go to `StrykerOutput//reports/` (gitignored): JSON for agents (filter `"status"` for both `"Survived"` and `"Timeout"`; investigate the timeout cause before survivor triage), HTML for humans. +`-p` selects the mutated project, not the tests: Stryker runs every test project transitively referencing it (`AspNetCore.Shared` → 415 tests, `Light.PortableResults` → all 2,675). Cross-project kills are legitimate (sociable tests) and nearly free. Never pass `-tp` (it does not narrow tests) or `--since` (any non-C# file in the diff, e.g. an `ai-plans/` document, degrades it to a full run). Reports go to `StrykerOutput//reports/` (gitignored): JSON for agents (filter `"status"` for both `"Survived"` and `"Timeout"`; investigate the timeout cause before survivor triage), HTML for humans. ### Cost and baseline @@ -56,7 +56,7 @@ The smoke-check vector is tied to the current `Result.cs` and its tests; update | `Validation.OpenApi.SourceGeneration` | 1,272 | 204 | | `AspNetCore.OpenApi` | 848 | 65 | | `Validation.OpenApi` | 114 | 2 | -| `AspNetCore.Shared` | 44 | 3 | +| `AspNetCore.Shared` | 47 | 3 | | `AspNetCore.Mvc` | 33 | 11 | | `AspNetCore.MinimalApis` | 32 | 11 | @@ -66,7 +66,7 @@ Baselines carry per-row provenance, because rows are re-measured individually as | --- | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: | | `AspNetCore.Mvc` | `#66` | 103 | 0:43 | 17 | 0 | 0 | 11 | 5 | 0 | | `AspNetCore.MinimalApis` | `04aee20` | 237 | 1:22 | 16 | 0 | 0 | 11 | 5 | 0 | -| `AspNetCore.Shared` | `04aee20` | 337 | 2:29 | 31 | 0 | 0 | 3 | 10 | 0 | +| `AspNetCore.Shared` | `#80` | 415 | 2:44 | 34 | 0 | 0 | 3 | 10 | 0 | | `Validation.OpenApi` | `#66` | 165 | 2:20 | 59 | 0 | 14 | 2 | 39 | 0 | Both `Validation.OpenApi` survivor groups are accounted for and neither is a missing test: twelve are the `target`-provided branch of the typed helpers, deferred to the bug in #57 so that tests are written against the corrected contract, and two are the known-false static-initializer mutants described in the blind spots below. Its 39 `Ignored` are 36 block-removal plus the three triage suppressions. @@ -99,5 +99,6 @@ If a survivor can only be killed by asserting on something incidental — exact When changing a method in that set, mutation score carries no information about it and line coverage only proves execution. Adequacy has to be argued by hand: enumerate the behaviors the method promises and point at the test constraining each one. State that reasoning in the pull request, because no tool in this repository can check it. - Mutants reachable only during static initialization are reported as survived even when the suite kills them. Measured on `BuiltInValidationErrorContracts`: emptying the `Contracts` registry fails 46 of 112 tests and blanking the built-in schema id string fails 52 of 112, yet Stryker reported both as `Survived`. Both sites run only while a static property initializer executes, which a reused test host runs once per process, independently of mutant activation. Verify any survivor in a static initializer, a static constructor, or a helper called only from one by applying the mutation to the source by hand and running the affected test project — the report cannot settle it. Do not add tests for such a survivor before that check: the two above already had covering assertions. +- `MetadataValueReconstructor` keeps `OperationCanceledException` out of its two evaluation catch filters, and that contract cannot be observed through the generator's public surface: Roslyn intercepts a pre-cancelled token before reconstruction runs, and none of the whitelisted framework constructors and factories can throw cancellation. No test can distinguish the filter from a plain `catch (Exception)`, so the contract rests on the filters' structure. It becomes testable — and needs a test — as soon as an accepted evaluation gains a reachable cancellation path. - `Timeout` counts as killed. The pinned 30,000 ms additional timeout reduced `Validation.OpenApi` from 21 timeouts to zero in two consecutive concurrency-8 runs, but no finite value makes classification independent of hardware and load. Investigate any future timeout as either a genuine hang or insufficient headroom; do not assume it represents a killed mutant. - The MTP runner is a preview (stryker-mutator/stryker-net#3094); verify surprising results against a plain `dotnet test` run. diff --git a/tests/Light.PortableResults.AspNetCore.Shared.Tests/HttpExtensionsTests.cs b/tests/Light.PortableResults.AspNetCore.Shared.Tests/HttpExtensionsTests.cs index 1b31202e..0f94303e 100644 --- a/tests/Light.PortableResults.AspNetCore.Shared.Tests/HttpExtensionsTests.cs +++ b/tests/Light.PortableResults.AspNetCore.Shared.Tests/HttpExtensionsTests.cs @@ -294,6 +294,85 @@ public void SetMetadataValuesAsHeadersIfNecessaryShouldLetRegisteredConverterSup response.Headers.ContainsKey("X-Suppressed").Should().BeFalse(); } + [Fact] + public void SetStatusCodeFromResultShouldRejectDefaultResultWithoutSettingAStatusCode() + { + var response = new DefaultHttpContext().Response; + var initialStatusCode = response.StatusCode; + + var act = () => response.SetStatusCodeFromResult(default(Result)); + + AssertInvalidWithoutErrorsRejected(act); + response.StatusCode.Should().Be(initialStatusCode); + } + + [Fact] + public void SetStatusCodeFromResultShouldRejectAnyResultObjectThatIsInvalidWithoutErrors() + { + var response = new DefaultHttpContext().Response; + + var act = () => response.SetStatusCodeFromResult(new InvalidResultObjectStub(nonDefaultMarker: 1)); + + AssertInvalidWithoutErrorsRejected(act); + } + + [Fact] + public void SetContentTypeFromResultShouldRejectDefaultResultWithoutSettingAContentType() + { + var response = new DefaultHttpContext().Response; + + var act = () => + response.SetContentTypeFromResult(default(Result), MetadataSerializationMode.Always); + + AssertInvalidWithoutErrorsRejected(act); + response.ContentType.Should().BeNull(); + } + + [Fact] + public void SetContentTypeFromResultShouldRejectAnyResultObjectThatIsInvalidWithoutErrors() + { + var response = new DefaultHttpContext().Response; + + var act = () => + response.SetContentTypeFromResult( + new InvalidResultObjectStub(nonDefaultMarker: 1), + MetadataSerializationMode.Always + ); + + AssertInvalidWithoutErrorsRejected(act); + response.ContentType.Should().BeNull(); + } + + [Fact] + public void SetMetadataValuesAsHeadersIfNecessaryShouldRejectDefaultResultWithoutAddingHeaders() + { + var response = new DefaultHttpContext().Response; + var conversionService = new CapturingHttpHeaderConversionService(); + + var act = () => response.SetMetadataValuesAsHeadersIfNecessary(default(Result), conversionService); + + AssertInvalidWithoutErrorsRejected(act); + conversionService.PreparedHeaders.Should().BeEmpty(); + response.Headers.Should().BeEmpty(); + } + + [Fact] + public void SetMetadataValuesAsHeadersIfNecessaryShouldRejectAnyResultObjectThatIsInvalidWithoutErrors() + { + var response = new DefaultHttpContext().Response; + var conversionService = new CapturingHttpHeaderConversionService(); + + var act = () => + response.SetMetadataValuesAsHeadersIfNecessary( + new InvalidResultObjectStub(nonDefaultMarker: 1), + conversionService + ); + + AssertInvalidWithoutErrorsRejected(act); + conversionService.PreparedHeaders.Should().BeEmpty(); + response.Headers.Should().BeEmpty(); + } + [Fact] public void ResolvePortableResultsHttpWriteOptions_ShouldThrow_WhenHttpContextIsNull() { @@ -345,6 +424,12 @@ public void ResolvePortableResultsHttpWriteOptions_ShouldThrow_WhenNeitherOverri .WithMessage("No PortableResultsHttpWriteOptions are configured in the DI container"); } + private static void AssertInvalidWithoutErrorsRejected(Action act) => + act.Should() + .Throw() + .WithParameterName("result") + .WithMessage("*invalid while carrying no errors*default instance*Result.Ok or Result.Fail*"); + private static DefaultHttpContext CreateHttpContext(PortableResultsHttpWriteOptions? options = null) { var services = new ServiceCollection(); @@ -360,6 +445,23 @@ private static DefaultHttpContext CreateHttpContext(PortableResultsHttpWriteOpti }; } + /// + /// A non-default result object that is invalid and carries no errors. cannot take that + /// shape, because its encapsulated value can never be null, but a custom implementation of + /// can violate the success-or-errors invariant independently of its CLR default. + /// + private readonly struct InvalidResultObjectStub : IResultObject + { + private readonly int _nonDefaultMarker; + + public InvalidResultObjectStub(int nonDefaultMarker) => _nonDefaultMarker = nonDefaultMarker; + + public bool IsValid => false; + public Errors Errors => default; + public bool HasValue => _nonDefaultMarker < 0; + public MetadataObject? Metadata => null; + } + private sealed class CapturingHttpHeaderConversionService : IHttpHeaderConversionService { public List> PreparedHeaders { get; } = new (); diff --git a/tests/Light.PortableResults.Tests/CloudEvents/Writing/DefaultResultCloudEventsGuardTests.cs b/tests/Light.PortableResults.Tests/CloudEvents/Writing/DefaultResultCloudEventsGuardTests.cs new file mode 100644 index 00000000..a7573646 --- /dev/null +++ b/tests/Light.PortableResults.Tests/CloudEvents/Writing/DefaultResultCloudEventsGuardTests.cs @@ -0,0 +1,112 @@ +using System; +using System.IO; +using System.Text.Json; +using FluentAssertions; +using Light.PortableResults.CloudEvents.Writing; +using Light.PortableResults.CloudEvents.Writing.Json; +using Light.PortableResults.SharedJsonSerialization; +using Xunit; + +namespace Light.PortableResults.Tests.CloudEvents.Writing; + +/// +/// +/// Covers the CloudEvents write boundaries for the default result instance. +/// +/// +/// Only Result<T> with a reference type or a nullable value type can take that shape. +/// default(Result) and default(Result<int>) are ordinary successes, because their encapsulated +/// value can never be null. +/// +/// +public sealed class DefaultResultCloudEventsGuardTests +{ + private static readonly PortableResultsCloudEventsWriteOptions WriteOptions = new () + { + Source = "urn:test:source", + SuccessType = "app.success", + FailureType = "app.failure" + }; + + [Fact] + public void ToCloudEventShouldRejectDefaultResult() + { + Action act = () => default(Result).ToCloudEvent(options: WriteOptions); + + AssertDefaultInstanceRejected(act); + } + + [Fact] + public void ToCloudEventPooledShouldRejectDefaultResult() + { + Action act = () => default(Result).ToCloudEventPooled(options: WriteOptions); + + AssertDefaultInstanceRejected(act); + } + + [Fact] + public void WriteCloudEventShouldRejectDefaultResultWithoutWritingBytes() + { + using var stream = new MemoryStream(); + using var writer = new Utf8JsonWriter(stream); + + Action act = () => default(Result).WriteCloudEvent(writer, options: WriteOptions); + + AssertNothingWasWritten(act, writer, stream); + } + + [Fact] + public void ToCloudEventsEnvelopeForWritingShouldRejectDefaultResult() + { + Action act = () => default(Result).ToCloudEventsEnvelopeForWriting(options: WriteOptions); + + AssertDefaultInstanceRejected(act); + } + + [Fact] + public void WriteCloudEventsShouldRejectHandConstructedEnvelopeWithoutWritingBytes() + { + using var stream = new MemoryStream(); + using var writer = new Utf8JsonWriter(stream); + var envelope = new CloudEventsEnvelopeForWriting( + "app.failure", + "urn:test:source", + "evt-1", + default, + new ResolvedCloudEventsWriteOptions(MetadataSerializationMode.Always) + ); + + Action act = () => writer.WriteCloudEvents(envelope, WriteOptions.SerializerOptions); + + AssertNothingWasWritten(act, writer, stream, "envelope"); + } + + [Fact] + public void ToCloudEventShouldAcceptDefaultNonGenericResultAsASuccess() + { + var json = default(Result).ToCloudEvent(options: WriteOptions); + + using var document = JsonDocument.Parse(json); + document.RootElement.GetProperty("lproutcome").GetString().Should().Be("success"); + } + + private static void AssertDefaultInstanceRejected(Action act, string parameterName = "result") => + act.Should() + .Throw() + .WithParameterName(parameterName) + .WithMessage("*default instance*Result.Ok or Result.Fail*"); + + private static void AssertNothingWasWritten( + Action act, + Utf8JsonWriter writer, + MemoryStream stream, + string parameterName = "result" + ) + { + AssertDefaultInstanceRejected(act, parameterName); + + writer.BytesPending.Should().Be(0); + writer.BytesCommitted.Should().Be(0); + stream.Length.Should().Be(0); + } +} diff --git a/tests/Light.PortableResults.Tests/CloudEvents/Writing/FailurePayloadRoundTripTests.cs b/tests/Light.PortableResults.Tests/CloudEvents/Writing/FailurePayloadRoundTripTests.cs new file mode 100644 index 00000000..d76f4270 --- /dev/null +++ b/tests/Light.PortableResults.Tests/CloudEvents/Writing/FailurePayloadRoundTripTests.cs @@ -0,0 +1,102 @@ +using System; +using System.Text; +using System.Text.Json; +using FluentAssertions; +using Light.PortableResults.CloudEvents.Reading; +using Light.PortableResults.CloudEvents.Writing; +using Light.PortableResults.Metadata; +using Xunit; + +namespace Light.PortableResults.Tests.CloudEvents.Writing; + +/// +/// Pins the round-trip property that this library never writes a failure payload its own reader rejects. +/// +public sealed class FailurePayloadRoundTripTests +{ + private static readonly PortableResultsCloudEventsWriteOptions WriteOptions = new () + { + Source = "urn:test:source", + SuccessType = "app.success", + FailureType = "app.failure" + }; + + public static TheoryData NonGenericFailures => + new () + { + Result.Fail(new Error { Message = "Something went wrong" }), + Result.Fail( + new[] + { + new Error { Message = "First", Category = ErrorCategory.Conflict }, + new Error { Message = "Second", Category = ErrorCategory.Conflict } + } + ), + Result.Fail( + new Error { Message = "With metadata", Category = ErrorCategory.NotFound }, + MetadataObject.Create( + ( + "traceId", + MetadataValue.FromString("trace-42", MetadataValueAnnotation.SerializeInCloudEventsData) + ) + ) + ) + }; + + [Theory] + [MemberData(nameof(NonGenericFailures))] + public void EveryFailurePayloadTheLibraryWritesCanBeReadBackByItsOwnReader(Result failure) + { + var cloudEvent = failure.ToCloudEvent(options: WriteOptions); + + var roundTripped = ((ReadOnlyMemory) cloudEvent).ReadResult(); + + roundTripped.IsValid.Should().BeFalse(); + roundTripped.Errors.Count.Should().Be(failure.Errors.Count); + roundTripped.FirstError.Message.Should().Be(failure.FirstError.Message); + } + + [Fact] + public void GenericFailurePayloadCanBeReadBackByItsOwnReader() + { + var failure = Result.Fail(new Error { Message = "Not found", Category = ErrorCategory.NotFound }); + + var cloudEvent = failure.ToCloudEvent(options: WriteOptions); + var roundTripped = ((ReadOnlyMemory) cloudEvent).ReadResult(); + + roundTripped.IsValid.Should().BeFalse(); + roundTripped.FirstError.Message.Should().Be("Not found"); + } + + [Fact] + public void TheOnlyResultThatWouldProduceAnUnreadablePayloadIsRejectedBeforeAnyByteIsWritten() + { + // The default result is invalid while carrying no errors. Writing it would emit an empty errors array, + // which the reader used above rejects, so the write boundary must refuse it instead. + Action act = () => default(Result).ToCloudEvent(options: WriteOptions); + + act.Should().Throw().WithParameterName("result"); + } + + [Fact] + public void AnEmptyErrorsArrayIsIndeedRejectedByTheReader() + { + var cloudEventWithEmptyErrors = Encoding.UTF8.GetBytes( + """ + { + "specversion": "1.0", + "type": "app.failure", + "source": "urn:test:source", + "id": "evt-1", + "lproutcome": "failure", + "datacontenttype": "application/json", + "data": { "errors": [] } + } + """ + ); + + Action act = () => ((ReadOnlyMemory) cloudEventWithEmptyErrors).ReadResult(); + + act.Should().Throw(); + } +} diff --git a/tests/Light.PortableResults.Tests/Http/Writing/DefaultResultHttpWriteGuardTests.cs b/tests/Light.PortableResults.Tests/Http/Writing/DefaultResultHttpWriteGuardTests.cs new file mode 100644 index 00000000..597a259e --- /dev/null +++ b/tests/Light.PortableResults.Tests/Http/Writing/DefaultResultHttpWriteGuardTests.cs @@ -0,0 +1,110 @@ +using System; +using System.IO; +using System.Net; +using System.Text.Json; +using System.Text.Json.Serialization; +using FluentAssertions; +using Light.PortableResults.Http; +using Light.PortableResults.Http.Writing; +using Light.PortableResults.Http.Writing.Json; +using Xunit; + +namespace Light.PortableResults.Tests.Http.Writing; + +/// +/// +/// Covers the HTTP body write boundaries for the default result instance. +/// +/// +/// Only Result<T> with a reference type or a nullable value type can take that shape. +/// default(Result) and default(Result<int>) are ordinary successes, because their encapsulated +/// value can never be null. +/// +/// +public sealed class DefaultResultHttpWriteGuardTests +{ + private static readonly ResolvedHttpWriteOptions DefaultOptions = + new PortableResultsHttpWriteOptions().ToResolvedHttpWriteOptions(); + + private static readonly ResolvedHttpWriteOptions OptionsWithCustomProblemDetailsFactory = + new PortableResultsHttpWriteOptions + { + CreateProblemDetailsInfo = static (_, _) => new ProblemDetailsInfo + { + Type = "https://example.com/problem", + Status = HttpStatusCode.InternalServerError, + Title = "Custom", + Detail = "Custom problem details" + } + }.ToResolvedHttpWriteOptions(); + + [Fact] + public void ToHttpResultForWritingShouldRejectDefaultResult() + { + Action act = () => default(Result).ToHttpResultForWriting(new PortableResultsHttpWriteOptions()); + + AssertDefaultInstanceRejected(act); + } + + [Fact] + public void ToHttpResultForWritingWithResolvedOptionsShouldRejectDefaultResult() + { + Action act = () => default(Result).ToHttpResultForWriting(DefaultOptions); + + AssertDefaultInstanceRejected(act); + } + + [Fact] + public void ConverterShouldRejectHandConstructedWrapperWithoutWritingBytes() + { + var wrapper = new HttpResultForWriting(default, DefaultOptions); + + AssertNothingWasWritten(wrapper, new HttpResultForWritingJsonConverter()); + } + + [Fact] + public void ConverterShouldRejectDefaultResultWhenCustomProblemDetailsFactoryIsConfigured() + { + var wrapper = new HttpResultForWriting(default, OptionsWithCustomProblemDetailsFactory); + + AssertNothingWasWritten(wrapper, new HttpResultForWritingJsonConverter()); + } + + [Fact] + public void ToHttpResultForWritingShouldAcceptFailureResults() + { + var result = Result.Fail(new Error { Message = "Something went wrong" }); + + var wrapper = result.ToHttpResultForWriting(DefaultOptions); + + wrapper.Data.Should().Be(result); + } + + [Fact] + public void ToHttpResultForWritingShouldAcceptDefaultNonGenericResultAsASuccess() + { + var wrapper = default(Result).ToHttpResultForWriting(DefaultOptions); + + wrapper.Data.IsValid.Should().BeTrue(); + } + + private static void AssertDefaultInstanceRejected(Action act, string parameterName = "result") => + act.Should() + .Throw() + .WithParameterName(parameterName) + .WithMessage("*default instance*Result.Ok or Result.Fail*"); + + private static void AssertNothingWasWritten(T wrapper, JsonConverter converter) + { + using var stream = new MemoryStream(); + using var writer = new Utf8JsonWriter(stream); + var serializerOptions = new JsonSerializerOptions(); + + Action act = () => converter.Write(writer, wrapper, serializerOptions); + + AssertDefaultInstanceRejected(act, "wrapper"); + writer.BytesPending.Should().Be(0); + writer.BytesCommitted.Should().Be(0); + stream.Length.Should().Be(0); + } +} diff --git a/tests/Light.PortableResults.Tests/ResultObjectExtensionsTests.cs b/tests/Light.PortableResults.Tests/ResultObjectExtensionsTests.cs new file mode 100644 index 00000000..104a8dba --- /dev/null +++ b/tests/Light.PortableResults.Tests/ResultObjectExtensionsTests.cs @@ -0,0 +1,124 @@ +using System; +using FluentAssertions; +using Light.PortableResults.Metadata; +using Xunit; + +namespace Light.PortableResults.Tests; + +public sealed class ResultObjectExtensionsTests +{ + [Fact] + public void MustNotBeDefaultInstanceShouldRejectDefaultResultWithReferenceTypeValue() + { + var result = default(Result); + + var act = () => result.MustNotBeDefaultInstance(); + + act.Should() + .Throw() + .WithParameterName("result") + .WithMessage("*invalid while carrying no errors*default instance*Result.Ok or Result.Fail*"); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldRejectDefaultResultWithNullableValueTypeValue() + { + var result = default(Result); + + var act = () => result.MustNotBeDefaultInstance(); + + act.Should() + .Throw() + .WithParameterName("result") + .WithMessage("*invalid while carrying no errors*default instance*Result.Ok or Result.Fail*"); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldRejectAnyResultObjectThatIsInvalidWithoutErrors() + { + var result = new ResultObjectStub(isValid: false, default, nonDefaultMarker: 1); + + result.Should().NotBe(default(ResultObjectStub)); + var act = () => result.MustNotBeDefaultInstance(); + + act.Should() + .Throw() + .WithParameterName("result") + .WithMessage("*invalid while carrying no errors*default instance*Result.Ok or Result.Fail*"); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldUseTheSpecifiedParameterName() + { + var act = () => default(Result).MustNotBeDefaultInstance("envelope"); + + act.Should().Throw().WithParameterName("envelope"); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldReturnSuccessfulResultUnchanged() + { + var result = Result.Ok("value"); + + var returnValue = result.MustNotBeDefaultInstance(); + + returnValue.Should().Be(result); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldReturnFailedResultUnchanged() + { + var result = Result.Fail(new Error { Message = "Something went wrong" }); + + var returnValue = result.MustNotBeDefaultInstance(); + + returnValue.Should().Be(result); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldAcceptSuccessfulNonGenericResult() + { + var result = Result.Ok(); + + var returnValue = result.MustNotBeDefaultInstance(); + + returnValue.Should().Be(result); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldAcceptDefaultNonGenericResultBecauseItIsASuccess() + { + var result = default(Result); + + var returnValue = result.MustNotBeDefaultInstance(); + + returnValue.Should().Be(Result.Ok()); + } + + [Fact] + public void MustNotBeDefaultInstanceShouldAcceptDefaultResultWithNonNullableValueTypeBecauseItIsASuccess() + { + var result = default(Result); + + var returnValue = result.MustNotBeDefaultInstance(); + + returnValue.Should().Be(Result.Ok(0)); + } + + private readonly struct ResultObjectStub : IResultObject + { + private readonly int _nonDefaultMarker; + + public ResultObjectStub(bool isValid, Errors errors, int nonDefaultMarker) + { + IsValid = isValid; + Errors = errors; + _nonDefaultMarker = nonDefaultMarker; + } + + public bool IsValid { get; } + public Errors Errors { get; } + public bool HasValue => _nonDefaultMarker < 0; + public MetadataObject? Metadata => null; + } +}