diff --git a/.forgejo/workflows/default.yml b/.forgejo/workflows/default.yml index 51c054c..2796b83 100644 --- a/.forgejo/workflows/default.yml +++ b/.forgejo/workflows/default.yml @@ -1,6 +1,7 @@ name: default on: + workflow_dispatch: push: branches: [ "main", "develop" ] paths-ignore: diff --git a/CHANGELOG.md b/CHANGELOG.md index 240c251..a7052a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,19 @@ To have a consistent experience across all packages, some public interfaces have ### Changed +- **request.result:** Document that `Result`/`Result` equality treats any two failures as equal, ignoring error content; branch on the `Error` directly for failure-specific logic + +### Fixed + +- **request.result:** Correct the namespace in the `Prelude` doc comment (`Geekeey.Extensions.Result` → `Geekeey.Request.Result`) +- **request.result:** Fold `IsSuccess` into `Result.GetHashCode` to avoid collisions between a failure and a success value whose hash is `0` +- **request.validation:** Guard null comparison bounds in `GreaterThan`/`LessThan`/`Between` etc. so a `null` bound no longer throws (`NullReferenceException`) +- **request.validation:** Return no problems when validating a `null` instance of a reference type instead of dereferencing it (`NullReferenceException`) +- **request.validation:** Make `Severity` meaningful — `Validation.IsValid` now only fails on `Error` problems; `Warning`/`Info` no longer invalidate. Added `Validation.IsValidFor(Severity)` to set the failing-severity threshold +- **request.dispatcher:** Throw `InvalidOperationException` listing candidates when multiple handlers match a request instead of silently resolving via `.First()`. Also throws when no handler is registered +- **request.dispatcher:** Resolve handlers/behaviors in registration order across closed and open generics instead of the fixed closed-before-open reflection order +- **request.dispatcher:** Add `AddBehavior(order:)` registration-time API so pipeline behavior ordering is explicit and deterministic (lower order runs first, registration order as tie-break) instead of relying on reflection/discovery order + ### Removed [1.0.0]: https://code.geekeey.de/geekeey/request/releases/tag/1.0.0 diff --git a/src/request.dispatcher.tests/ScalarBehaviourTests.cs b/src/request.dispatcher.tests/ScalarBehaviourTests.cs index 9c10e4b..d2a6dd6 100644 --- a/src/request.dispatcher.tests/ScalarBehaviourTests.cs +++ b/src/request.dispatcher.tests/ScalarBehaviourTests.cs @@ -87,6 +87,26 @@ internal sealed class ScalarBehaviourTests await Assert.That(tracker.Executed).IsTrue(); } + [Test] + public async Task I_can_order_behaviours_explicitly_by_order_property() + { + var sc = new ServiceCollection(); + sc.AddSingleton(); + sc.AddRequestDispatcher(builder => builder + .Add(typeof(ScalarTestHandler)) + .AddBehavior(2) + .AddBehavior(1)); + var provider = sc.BuildServiceProvider(); + var dispatcher = provider.GetRequiredService(); + var tracker = provider.GetRequiredService(); + + var request = new ScalarTestRequest { Value = "Hello" }; + await dispatcher.DispatchAsync(request); + + // Registered A (order 2) then B (order 1) via AddBehavior; explicit order overrides discovery order. + await Assert.That(tracker.Log).IsEquivalentTo(["OrderedB", "OrderedA"]); + } + [Test] public async Task I_can_maintain_the_ordering_between_open_and_closed_behaviours() { diff --git a/src/request.dispatcher.tests/ScalarDispatcherTests.cs b/src/request.dispatcher.tests/ScalarDispatcherTests.cs index 5c10998..f274da9 100644 --- a/src/request.dispatcher.tests/ScalarDispatcherTests.cs +++ b/src/request.dispatcher.tests/ScalarDispatcherTests.cs @@ -116,7 +116,7 @@ internal sealed class ScalarDispatcherTests } [Test] - public async Task I_can_dispatch_a_request_async_with_an_interface_constrained_handler() + public async Task I_can_see_it_throw_on_ambiguous_concrete_and_constrained_scalar_handlers() { var sc = new ServiceCollection(); sc.AddRequestDispatcher(builder => builder @@ -126,13 +126,18 @@ internal sealed class ScalarDispatcherTests var dispatcher = provider.GetRequiredService(); var request = new InterfaceInheritedScalarRequest { Name = "Constrained" }; - var result = await dispatcher.DispatchAsync(request); // Both InterfaceInheritedScalarHandler and InterfaceConstrainedScalarHandler could match. // InterfaceInheritedScalarHandler is a concrete match for InterfaceInheritedScalarRequest. // InterfaceConstrainedScalarHandler is an open generic match. - // Currently Dispatcher.SendAsync checks concrete handlers first. - await Assert.That(result).IsEquivalentTo("Constrained-InterfaceHandled"); + // Ambiguity is no longer resolved silently with .First(); it throws instead. + var ex = await Assert.That(async () => await dispatcher.DispatchAsync(request)).Throws(); + + using (Assert.Multiple()) + { + await Assert.That(ex?.Message).Contains(nameof(InterfaceInheritedScalarHandler)); + await Assert.That(ex?.Message).Contains("InterfaceConstrainedScalarHandler"); + } } [Test] @@ -234,6 +239,49 @@ internal sealed class ScalarDispatcherTests } } + [Test] + public async Task I_can_see_it_throw_on_ambiguous_scalar_handlers() + { + var sc = new ServiceCollection(); + sc.AddRequestDispatcher(builder => builder + .Add(typeof(DuplicateScalarHandlerA)) + .Add(typeof(DuplicateScalarHandlerB))); + var provider = sc.BuildServiceProvider(); + var dispatcher = provider.GetRequiredService(); + + var request = new DuplicateScalarRequest(); + var ex = await Assert.That(async () => await dispatcher.DispatchAsync(request)).Throws(); + + using (Assert.Multiple()) + { + await Assert.That(ex?.Message).Contains(nameof(DuplicateScalarHandlerA)); + await Assert.That(ex?.Message).Contains(nameof(DuplicateScalarHandlerB)); + } + } + + [Test] + public async Task I_can_see_candidates_ordered_by_registration_not_closed_first() + { + var sc = new ServiceCollection(); + sc.AddRequestDispatcher(builder => builder + .Add(typeof(InterfaceConstrainedScalarHandler<>)) + .Add(typeof(InterfaceInheritedScalarHandler))); + var provider = sc.BuildServiceProvider(); + var dispatcher = provider.GetRequiredService(); + + var request = new InterfaceInheritedScalarRequest { Name = "Constrained" }; + + // The open generic handler is registered before the closed handler, so candidates must be + // listed in registration order (open first), not the old fixed closed-before-open order. + var ex = await Assert.That(async () => await dispatcher.DispatchAsync(request)).Throws(); + + var message = ex?.Message ?? string.Empty; + var openIndex = message.IndexOf("InterfaceConstrainedScalarHandler", StringComparison.Ordinal); + var closedIndex = message.IndexOf("InterfaceInheritedScalarHandler", StringComparison.Ordinal); + + await Assert.That(openIndex >= 0 && closedIndex >= 0 && openIndex < closedIndex).IsTrue(); + } + [Test] public async Task I_can_see_it_throw_if_dispatcher_options_are_modified_after_build() { diff --git a/src/request.dispatcher.tests/StreamBehaviourTests.cs b/src/request.dispatcher.tests/StreamBehaviourTests.cs index c96b530..7e9377f 100644 --- a/src/request.dispatcher.tests/StreamBehaviourTests.cs +++ b/src/request.dispatcher.tests/StreamBehaviourTests.cs @@ -66,6 +66,26 @@ internal sealed class StreamBehaviourTests await Assert.That(tracker.Log[1]).IsEquivalentTo("Behaviour2"); } + [Test] + public async Task I_can_order_behaviours_explicitly_by_order_property() + { + var sc = new ServiceCollection(); + sc.AddSingleton(); + sc.AddRequestDispatcher(builder => builder + .Add(typeof(StreamTestHandler)) + .AddBehavior(2) + .AddBehavior(1)); + var provider = sc.BuildServiceProvider(); + var dispatcher = provider.GetRequiredService(); + var tracker = provider.GetRequiredService(); + + var request = new StreamTestRequest { Value = "Hello" }; + await dispatcher.DispatchAsync(request).ToListAsync(); + + // Registered A (order 2) then B (order 1) via AddBehavior; explicit order overrides discovery order. + await Assert.That(tracker.Log).IsEquivalentTo(["OrderedB", "OrderedA"]); + } + [Test] public async Task I_can_work_with_a_generic_wrapper_request_and_the_open_behaviour() { diff --git a/src/request.dispatcher.tests/StreamDispatcherTests.cs b/src/request.dispatcher.tests/StreamDispatcherTests.cs index c06cb36..5fcc7b2 100644 --- a/src/request.dispatcher.tests/StreamDispatcherTests.cs +++ b/src/request.dispatcher.tests/StreamDispatcherTests.cs @@ -7,6 +7,26 @@ namespace Geekeey.Request.Dispatcher.Tests; public class StreamDispatcherTests { + [Test] + public async Task I_can_see_it_throw_on_ambiguous_stream_handlers() + { + var sc = new ServiceCollection(); + sc.AddRequestDispatcher(builder => builder + .Add(typeof(DuplicateStreamHandlerA)) + .Add(typeof(DuplicateStreamHandlerB))); + var provider = sc.BuildServiceProvider(); + var dispatcher = provider.GetRequiredService(); + + var request = new DuplicateStreamRequest(); + var ex = await Assert.That(async () => await dispatcher.DispatchAsync(request).ToListAsync()).Throws(); + + using (Assert.Multiple()) + { + await Assert.That(ex?.Message).Contains(nameof(DuplicateStreamHandlerA)); + await Assert.That(ex?.Message).Contains(nameof(DuplicateStreamHandlerB)); + } + } + [Test] public async Task I_can_dispatch_a_request_async_with_an_open_generic_handler() { @@ -121,13 +141,18 @@ public class StreamDispatcherTests var dispatcher = provider.GetRequiredService(); var request = new InterfaceInheritedStreamRequest { Name = "Constrained" }; - var results = await dispatcher.DispatchAsync(request).ToListAsync(); // Both InterfaceInheritedStreamHandler and InterfaceConstrainedStreamHandler could match. // InterfaceInheritedStreamHandler is a concrete match for InterfaceInheritedStreamRequest. // InterfaceConstrainedStreamHandler is an open generic match. - // Currently Dispatcher checks concrete handlers first. - await Assert.That(results).IsEquivalentTo(["Constrained-InterfaceHandled-0", "Constrained-InterfaceHandled-1"]); + // Ambiguity is no longer resolved silently with .First(); it throws instead. + var ex = await Assert.That(async () => await dispatcher.DispatchAsync(request).ToListAsync()).Throws(); + + using (Assert.Multiple()) + { + await Assert.That(ex?.Message).Contains(nameof(InterfaceInheritedStreamHandler)); + await Assert.That(ex?.Message).Contains("InterfaceConstrainedStreamHandler"); + } } [Test] diff --git a/src/request.dispatcher.tests/_fixtures/DuplicateScalarHandler.cs b/src/request.dispatcher.tests/_fixtures/DuplicateScalarHandler.cs new file mode 100644 index 0000000..6a0504e --- /dev/null +++ b/src/request.dispatcher.tests/_fixtures/DuplicateScalarHandler.cs @@ -0,0 +1,24 @@ +// Copyright (c) The Geekeey Authors +// SPDX-License-Identifier: EUPL-1.2 + +namespace Geekeey.Request.Dispatcher.Tests; + +public sealed class DuplicateScalarRequest : IScalarRequest +{ +} + +public sealed class DuplicateScalarHandlerA : IScalarRequestHandler +{ + public Task HandleAsync(DuplicateScalarRequest request, CancellationToken cancellationToken) + { + return Task.FromResult("A"); + } +} + +public sealed class DuplicateScalarHandlerB : IScalarRequestHandler +{ + public Task HandleAsync(DuplicateScalarRequest request, CancellationToken cancellationToken) + { + return Task.FromResult("B"); + } +} diff --git a/src/request.dispatcher.tests/_fixtures/DuplicateStreamHandler.cs b/src/request.dispatcher.tests/_fixtures/DuplicateStreamHandler.cs new file mode 100644 index 0000000..d3f0414 --- /dev/null +++ b/src/request.dispatcher.tests/_fixtures/DuplicateStreamHandler.cs @@ -0,0 +1,24 @@ +// Copyright (c) The Geekeey Authors +// SPDX-License-Identifier: EUPL-1.2 + +namespace Geekeey.Request.Dispatcher.Tests; + +public sealed class DuplicateStreamRequest : IStreamRequest +{ +} + +public sealed class DuplicateStreamHandlerA : IStreamRequestHandler +{ + public async IAsyncEnumerable HandleAsync(DuplicateStreamRequest request, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken) + { + yield return "A"; + } +} + +public sealed class DuplicateStreamHandlerB : IStreamRequestHandler +{ + public async IAsyncEnumerable HandleAsync(DuplicateStreamRequest request, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken) + { + yield return "B"; + } +} diff --git a/src/request.dispatcher.tests/_fixtures/OrderedScalarBehavior.cs b/src/request.dispatcher.tests/_fixtures/OrderedScalarBehavior.cs new file mode 100644 index 0000000..1eab048 --- /dev/null +++ b/src/request.dispatcher.tests/_fixtures/OrderedScalarBehavior.cs @@ -0,0 +1,22 @@ +// Copyright (c) The Geekeey Authors +// SPDX-License-Identifier: EUPL-1.2 + +namespace Geekeey.Request.Dispatcher.Tests; + +public class OrderedScalarBehaviorA(ScalarTestTracker tracker) : IScalarRequestBehavior +{ + public async Task HandleAsync(ScalarTestRequest request, ScalarHandlerDelegate next, CancellationToken cancellationToken) + { + tracker.Log.Add("OrderedA"); + return await next(request, cancellationToken); + } +} + +public class OrderedScalarBehaviorB(ScalarTestTracker tracker) : IScalarRequestBehavior +{ + public async Task HandleAsync(ScalarTestRequest request, ScalarHandlerDelegate next, CancellationToken cancellationToken) + { + tracker.Log.Add("OrderedB"); + return await next(request, cancellationToken); + } +} diff --git a/src/request.dispatcher.tests/_fixtures/OrderedStreamBehavior.cs b/src/request.dispatcher.tests/_fixtures/OrderedStreamBehavior.cs new file mode 100644 index 0000000..0439f21 --- /dev/null +++ b/src/request.dispatcher.tests/_fixtures/OrderedStreamBehavior.cs @@ -0,0 +1,28 @@ +// Copyright (c) The Geekeey Authors +// SPDX-License-Identifier: EUPL-1.2 + +namespace Geekeey.Request.Dispatcher.Tests; + +public class OrderedStreamBehaviorA(StreamTestTracker tracker) : IStreamRequestBehavior +{ + public async IAsyncEnumerable HandleAsync(StreamTestRequest request, StreamHandlerDelegate next, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken) + { + tracker.Log.Add("OrderedA"); + await foreach (var item in next(request, cancellationToken)) + { + yield return item; + } + } +} + +public class OrderedStreamBehaviorB(StreamTestTracker tracker) : IStreamRequestBehavior +{ + public async IAsyncEnumerable HandleAsync(StreamTestRequest request, StreamHandlerDelegate next, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken) + { + tracker.Log.Add("OrderedB"); + await foreach (var item in next(request, cancellationToken)) + { + yield return item; + } + } +} diff --git a/src/request.dispatcher/RequestDispatcherBuilderExtensions.cs b/src/request.dispatcher/RequestDispatcherBuilderExtensions.cs index 8d53e3b..c2b6976 100644 --- a/src/request.dispatcher/RequestDispatcherBuilderExtensions.cs +++ b/src/request.dispatcher/RequestDispatcherBuilderExtensions.cs @@ -70,6 +70,27 @@ public static class RequestDispatcherBuilderExtensions return builder; } + /// + /// Adds the specified pipeline behavior to the request dispatcher configuration, with an explicit + /// pipeline used to determine its position in the pipeline. Lower values run + /// first (outermost); behaviors with equal order run in registration order. + /// + /// The behavior type to add. Must implement a request behavior interface. + /// The to configure. + /// The pipeline order for this behavior. Defaults to 0. + /// The instance for further configuration. + public static IRequestDispatcherBuilder AddBehavior(this IRequestDispatcherBuilder builder, int order = 0) + where TBehavior : class + { + ArgumentNullException.ThrowIfNull(builder); + ValidateNoNestedRequestHandlers([typeof(TBehavior)]); + + builder.Services.AddOptions() + .Configure(options => options.Inspect([typeof(TBehavior)], order)); + + return builder; + } + /// /// Adds the specified type to the request dispatcher configuration for inspection. /// diff --git a/src/request.dispatcher/RequestDispatcherOptions.cs b/src/request.dispatcher/RequestDispatcherOptions.cs index d18def9..f7f8c68 100644 --- a/src/request.dispatcher/RequestDispatcherOptions.cs +++ b/src/request.dispatcher/RequestDispatcherOptions.cs @@ -10,24 +10,27 @@ namespace Geekeey.Request.Dispatcher; internal sealed class RequestDispatcherOptions { - private readonly List _search = []; + private readonly List<(Type Type, int? Order)> _search = []; private readonly Lazy _behaviorsTypeIndex; private readonly Lazy _handlersTypeIndex; public RequestDispatcherOptions() { - _behaviorsTypeIndex = new Lazy(() => new BehaviorTypeIndex(_search.Distinct())); - _handlersTypeIndex = new Lazy(() => new HandlerTypeIndex(_search.Distinct())); + _behaviorsTypeIndex = new Lazy(() => new BehaviorTypeIndex(_search.DistinctBy(x => x.Type))); + _handlersTypeIndex = new Lazy(() => new HandlerTypeIndex(_search.DistinctBy(x => x.Type))); } - public void Inspect(IEnumerable assembly) + public void Inspect(IEnumerable assembly, int? order = null) { if (_behaviorsTypeIndex.IsValueCreated || _handlersTypeIndex.IsValueCreated) { throw new InvalidOperationException("The type index has already been created. Cannot inspect new assemblies."); } - _search.AddRange(assembly); + foreach (var type in assembly) + { + _search.Add((type, order)); + } } public IEnumerable GetRequestBehaviors(IServiceProvider services) @@ -46,11 +49,22 @@ internal sealed class RequestDispatcherOptions protected readonly Dictionary> _closedTypeInfo = []; protected readonly List _openTypeInfo = []; + protected readonly Dictionary _order = []; + protected readonly Dictionary _explicitOrder = []; - protected TypeIndex(IEnumerable collection, Func predicate) + protected TypeIndex(IEnumerable<(Type Type, int? Order)> collection, Func predicate) { - foreach (var type in collection) + var index = 0; + + foreach (var (type, order) in collection) { + _order[type] = index++; + + if (order is { } explicitOrder) + { + _explicitOrder[type] = explicitOrder; + } + if (type.IsGenericTypeDefinition) { if (type.GetInterfaces().Any(predicate)) @@ -68,6 +82,11 @@ internal sealed class RequestDispatcherOptions } } + protected int EffectiveOrder(Type type) + { + return _explicitOrder.TryGetValue(type, out var order) ? order : _order[type]; + } + public IEnumerable Resolve(IServiceProvider services) { return (IEnumerable)_cache.GetOrAdd(typeof(T), CreateResolverFactory)(services); @@ -97,16 +116,19 @@ internal sealed class RequestDispatcherOptions type.GetGenericTypeDefinition() == typeof(IStreamRequestHandler<,>)); } - private sealed class HandlerTypeIndex(IEnumerable collection) + private sealed class HandlerTypeIndex(IEnumerable<(Type Type, int? Order)> collection) : TypeIndex(collection, IsRequestHandlerType) { protected override IReadOnlyList IsAssignableTo(Type @interface) { - var result = new List(); + var result = new List<(int Order, Type Type)>(); if (_closedTypeInfo.TryGetValue(@interface, out var list)) { - result.AddRange(list); + foreach (var type in list) + { + result.Add((EffectiveOrder(type), type)); + } } var requestType = @interface.GetGenericArguments()[0]; @@ -121,7 +143,7 @@ internal sealed class RequestDispatcherOptions var impl = type.MakeGenericType(requestType); if (impl.IsAssignableTo(@interface)) { - result.Add(impl); + result.Add((EffectiveOrder(type), impl)); } } catch (ArgumentException) @@ -137,7 +159,7 @@ internal sealed class RequestDispatcherOptions var impl = type.MakeGenericType(requestType.GetGenericArguments()); if (impl.IsAssignableTo(@interface)) { - result.Add(impl); + result.Add((EffectiveOrder(type), impl)); } } } @@ -146,7 +168,7 @@ internal sealed class RequestDispatcherOptions } } - return result; + return [.. result.OrderBy(static item => item.Order).Select(static item => item.Type)]; } } @@ -157,16 +179,19 @@ internal sealed class RequestDispatcherOptions type.GetGenericTypeDefinition() == typeof(IStreamRequestBehavior<,>)); } - private sealed class BehaviorTypeIndex(IEnumerable collection) + private sealed class BehaviorTypeIndex(IEnumerable<(Type Type, int? Order)> collection) : TypeIndex(collection, IsRequestBehaviorType) { protected override IReadOnlyList IsAssignableTo(Type @interface) { - var result = new List(); + var result = new List<(int Order, Type Type)>(); if (_closedTypeInfo.TryGetValue(@interface, out var list)) { - result.AddRange(list); + foreach (var type in list) + { + result.Add((EffectiveOrder(type), type)); + } } var requestType = @interface.GetGenericArguments()[0]; @@ -180,7 +205,7 @@ internal sealed class RequestDispatcherOptions var impl = behaviour.MakeGenericType(requestType, responseType); if (impl.IsAssignableTo(@interface)) { - result.Add(impl); + result.Add((EffectiveOrder(behaviour), impl)); } } catch (ArgumentException) @@ -193,7 +218,7 @@ internal sealed class RequestDispatcherOptions var impl = behaviour.MakeGenericType(requestType.GetGenericArguments()); if (impl.IsAssignableTo(@interface)) { - result.Add(impl); + result.Add((EffectiveOrder(behaviour), impl)); } } catch (ArgumentException) @@ -201,7 +226,7 @@ internal sealed class RequestDispatcherOptions } } - return result; + return [.. result.OrderBy(static item => item.Order).Select(static item => item.Type)]; } } } diff --git a/src/request.dispatcher/ScalarRequestInvoker.cs b/src/request.dispatcher/ScalarRequestInvoker.cs index 63de03e..2c6d129 100644 --- a/src/request.dispatcher/ScalarRequestInvoker.cs +++ b/src/request.dispatcher/ScalarRequestInvoker.cs @@ -43,7 +43,20 @@ internal sealed class ScalarRequestInvoker : ScalarRequestI Task Head(IScalarRequest r, CancellationToken ct) { - return options.GetRequestHandlers>(serviceProvider).First().HandleAsync((TRequest)r, ct); + var handlers = options.GetRequestHandlers>(serviceProvider).ToArray(); + + if (handlers.Length is 0) + { + throw new InvalidOperationException($"No handler is registered for request '{typeof(TRequest).FullName}'."); + } + + if (handlers.Length > 1) + { + var candidates = string.Join(", ", handlers.Select(handler => handler.GetType().FullName)); + throw new InvalidOperationException($"Multiple handlers are registered for request '{typeof(TRequest).FullName}'. Ambiguous handlers: {candidates}"); + } + + return handlers[0].HandleAsync((TRequest)r, ct); } } } diff --git a/src/request.dispatcher/StreamRequestInvoker.cs b/src/request.dispatcher/StreamRequestInvoker.cs index 882d538..817b5b2 100644 --- a/src/request.dispatcher/StreamRequestInvoker.cs +++ b/src/request.dispatcher/StreamRequestInvoker.cs @@ -47,7 +47,20 @@ internal sealed class StreamRequestInvoker : StreamRequestI IAsyncEnumerable Head(IStreamRequest r, CancellationToken ct) { - return options.GetRequestHandlers>(serviceProvider).First().HandleAsync((TRequest)r, ct); + var handlers = options.GetRequestHandlers>(serviceProvider).ToArray(); + + if (handlers.Length is 0) + { + throw new InvalidOperationException($"No handler is registered for request '{typeof(TRequest).FullName}'."); + } + + if (handlers.Length > 1) + { + var candidates = string.Join(", ", handlers.Select(handler => handler.GetType().FullName)); + throw new InvalidOperationException($"Multiple handlers are registered for request '{typeof(TRequest).FullName}'. Ambiguous handlers: {candidates}"); + } + + return handlers[0].HandleAsync((TRequest)r, ct); } } } diff --git a/src/request.result.tests/ResultEqualityTests.cs b/src/request.result.tests/ResultEqualityTests.cs index 67233dd..99d7094 100644 --- a/src/request.result.tests/ResultEqualityTests.cs +++ b/src/request.result.tests/ResultEqualityTests.cs @@ -166,11 +166,48 @@ internal sealed class ResultEqualityTests } [Test] - public async Task I_can_get_hashcode_and_get_zero_for_failure() + public async Task I_can_get_hashcode_and_get_non_zero_for_failure() { var result = Prelude.Failure("error"); - await Assert.That(result.GetHashCode()).IsZero(); + await Assert.That(result.GetHashCode()).IsNotEqualTo(0); + } + + [Test] + public async Task I_can_get_hashcode_and_not_collide_non_generic_success_and_failure() + { + await Assert.That(Prelude.Success().GetHashCode()) + .IsNotEqualTo(Prelude.Failure("error").GetHashCode()); + } + + [Test] + public async Task I_can_get_hashcode_and_not_collide_success_value_zero_with_failure() + { + var success = Prelude.Success(0); + var failure = Prelude.Failure("error"); + + await Assert.That(success.Equals(failure)).IsFalse(); + await Assert.That(success.GetHashCode()).IsNotEqualTo(failure.GetHashCode()); + } + + [Test] + public async Task I_can_equal_failure_and_failure_ignoring_error_content() + { + var a = Prelude.Failure("error 1"); + var b = Prelude.Failure("error 2"); + + await Assert.That(a.Equals(b)).IsTrue(); + await Assert.That(a.Error).IsNotEqualTo(b.Error); + } + + [Test] + public async Task I_can_equal_non_generic_failure_and_failure_ignoring_error_content() + { + var a = Prelude.Failure("error 1"); + var b = Prelude.Failure("error 2"); + + await Assert.That(a.Equals(b)).IsTrue(); + await Assert.That(a.Error).IsNotEqualTo(b.Error); } [Test] @@ -199,4 +236,53 @@ internal sealed class ResultEqualityTests await Assert.That(a.Equals(b)).IsFalse(); } + + [Test] + public async Task I_can_use_equality_operator_and_get_false_for_success_and_failure() + { + var success = Prelude.Success(2); + var failure = Prelude.Failure("error"); + + await Assert.That(success == failure).IsFalse(); + await Assert.That(success != failure).IsTrue(); + } + + [Test] + public async Task I_can_use_equality_operator_and_get_true_for_successes_with_equal_value() + { + await Assert.That(Prelude.Success(2) == Prelude.Success(2)).IsTrue(); + await Assert.That(Prelude.Success(2) != Prelude.Success(3)).IsTrue(); + } + + [Test] + public async Task I_can_use_equality_operator_and_compare_result_to_value() + { + await Assert.That(Prelude.Success(2) == 2).IsTrue(); + await Assert.That(Prelude.Success(2) != 3).IsTrue(); + await Assert.That(Prelude.Failure("x") == 2).IsFalse(); + } + + [Test] + public async Task I_can_equal_object_and_get_false_for_null() + { + object result = Prelude.Success(2); + + await Assert.That(result.Equals(null)).IsFalse(); + } + + [Test] + public async Task I_can_equal_object_and_get_false_for_wrong_type() + { + object result = Prelude.Success(2); + + await Assert.That(result.Equals("not a result")).IsFalse(); + } + + [Test] + public async Task I_can_equal_object_and_get_true_for_boxed_equal_result() + { + object result = Prelude.Success(2); + + await Assert.That(result.Equals(Prelude.Success(2))).IsTrue(); + } } diff --git a/src/request.result/Prelude.cs b/src/request.result/Prelude.cs index de48354..e59349d 100644 --- a/src/request.result/Prelude.cs +++ b/src/request.result/Prelude.cs @@ -9,7 +9,7 @@ namespace Geekeey.Request.Result; /// A class containing various utility methods, a 'prelude' to the rest of the library. /// /// -/// This class is meant to be imported statically, e.g. using static Geekeey.Extensions.Result.Prelude;. +/// This class is meant to be imported statically, e.g. using static Geekeey.Request.Result.Prelude;. /// Recommended to be imported globally via a global using statement. /// public static class Prelude diff --git a/src/request.result/Result.Equality.cs b/src/request.result/Result.Equality.cs index 0253b88..da9372d 100644 --- a/src/request.result/Result.Equality.cs +++ b/src/request.result/Result.Equality.cs @@ -9,7 +9,15 @@ namespace Geekeey.Request.Result; public partial class Result : IEquatable { - /// + /// + /// Checks whether two results are equal. Results are equal if both are success or both are failure. + /// + /// + /// Two failures are considered equal regardless of their content. Equality therefore + /// ignores the error; if you need to branch on a specific failure, compare the values + /// directly instead of relying on equality. + /// + /// The result to check for equality with the current result. [Pure] public bool Equals(Result? other) { @@ -49,7 +57,8 @@ public partial class Result : IEquatable public partial class Result : IEqualityOperators { /// - /// Checks whether two results are equal. Results are equal if they are both success or both failure. + /// Checks whether two results are equal. Results are equal if they are both success or both failure. Two + /// failures are equal regardless of their error content. /// /// The first result to compare. /// The second result to compare. @@ -79,6 +88,11 @@ public partial class Result : IEquatable>, IEquatable /// Checks whether the result is equal to another result. Results are equal if both results are success values and /// the success values are equal, or if both results are failures. /// + /// + /// Two failures are considered equal regardless of their content. Equality therefore + /// ignores the error; if you need to branch on a specific failure, compare the values + /// directly instead of relying on equality. + /// /// The result to check for equality with the current result. [Pure] public bool Equals(Result? other) @@ -181,7 +195,12 @@ public partial class Result : IEquatable>, IEquatable return comparer.GetHashCode(result.Value); } - return 0; + if (result is { IsSuccess: true, Value: null }) + { + return 0; + } + + return result.Error?.GetHashCode() ?? 0; } } @@ -189,7 +208,7 @@ public partial class Result : IEqualityOperators, Result, bool>, { /// /// Checks whether two results are equal. Results are equal if both results are success values and the success - /// values are equal, or if both results are failures. + /// values are equal, or if both results are failures. Two failures are equal regardless of their error content. /// /// The first result to compare. /// The second result to compare. diff --git a/src/request.validation.tests/RuleBuilderExtensionsTests.cs b/src/request.validation.tests/RuleBuilderExtensionsTests.cs index 5b5271a..c0d2cae 100644 --- a/src/request.validation.tests/RuleBuilderExtensionsTests.cs +++ b/src/request.validation.tests/RuleBuilderExtensionsTests.cs @@ -314,6 +314,71 @@ internal sealed class RuleBuilderExtensionsTests .Throws(); } + [Test] + public async Task I_can_validate_greater_than_with_null_bound_without_throwing() + { + var validator = new PropertyValidator(model + => model.Value, rule => rule.GreaterThan(null)); + + var valid = validator.Validate(new ComparableModel { Value = new ComparableValue { Number = 1 } }); + var ignoredNull = validator.Validate(new ComparableModel()); + + await Assert.That(valid.IsValid).IsTrue(); + await Assert.That(ignoredNull.IsValid).IsTrue(); + } + + [Test] + public async Task I_can_validate_greater_than_or_equal_to_with_null_bound_without_throwing() + { + var validator = new PropertyValidator(model + => model.Value, rule => rule.GreaterThanOrEqualTo(null)); + + var valid = validator.Validate(new ComparableModel { Value = new ComparableValue { Number = 1 } }); + var ignoredNull = validator.Validate(new ComparableModel()); + + await Assert.That(valid.IsValid).IsTrue(); + await Assert.That(ignoredNull.IsValid).IsTrue(); + } + + [Test] + public async Task I_can_validate_less_than_with_null_bound_without_throwing() + { + var validator = new PropertyValidator(model + => model.Value, rule => rule.LessThan(null)); + + var valid = validator.Validate(new ComparableModel { Value = new ComparableValue { Number = 1 } }); + var ignoredNull = validator.Validate(new ComparableModel()); + + await Assert.That(valid.IsValid).IsTrue(); + await Assert.That(ignoredNull.IsValid).IsTrue(); + } + + [Test] + public async Task I_can_validate_less_than_or_equal_to_with_null_bound_without_throwing() + { + var validator = new PropertyValidator(model + => model.Value, rule => rule.LessThanOrEqualTo(null)); + + var valid = validator.Validate(new ComparableModel { Value = new ComparableValue { Number = 1 } }); + var ignoredNull = validator.Validate(new ComparableModel()); + + await Assert.That(valid.IsValid).IsTrue(); + await Assert.That(ignoredNull.IsValid).IsTrue(); + } + + [Test] + public async Task I_can_validate_between_with_null_bounds_without_throwing() + { + var validator = new PropertyValidator(model + => model.Value, rule => rule.Between(null, null)); + + var valid = validator.Validate(new ComparableModel { Value = new ComparableValue { Number = 1 } }); + var ignoredNull = validator.Validate(new ComparableModel()); + + await Assert.That(valid.IsValid).IsTrue(); + await Assert.That(ignoredNull.IsValid).IsTrue(); + } + [Test] public async Task I_can_see_it_throw_for_invalid_between_configuration() { diff --git a/src/request.validation.tests/ValidationSeverityTests.cs b/src/request.validation.tests/ValidationSeverityTests.cs new file mode 100644 index 0000000..6f24c8d --- /dev/null +++ b/src/request.validation.tests/ValidationSeverityTests.cs @@ -0,0 +1,49 @@ +// Copyright (c) The Geekeey Authors +// SPDX-License-Identifier: EUPL-1.2 + +namespace Geekeey.Request.Validation.Tests; + +internal sealed class ValidationSeverityTests +{ + [Test] + public async Task I_can_have_warning_problems_not_invalidate_by_default() + { + var validator = new PropertyValidator(person + => person.Name, rule => rule.Must(value => !string.IsNullOrWhiteSpace(value), "Name is required.") + .WithSeverity(Severity.Warning)); + + var result = validator.Validate(new Person { Name = "" }); + + await Assert.That(result.Problems).Count().IsEqualTo(1); + await Assert.That(result.IsValid).IsTrue(); + await Assert.That(result.IsValidFor(Severity.Warning)).IsFalse(); + await Assert.That(result.IsValidFor(Severity.Info)).IsFalse(); + } + + [Test] + public async Task I_can_have_info_problems_only_invalidate_at_info_threshold() + { + var validator = new PropertyValidator(person + => person.Name, rule => rule.Must(value => !string.IsNullOrWhiteSpace(value), "Name is required.") + .WithSeverity(Severity.Info)); + + var result = validator.Validate(new Person { Name = "" }); + + await Assert.That(result.Problems).Count().IsEqualTo(1); + await Assert.That(result.IsValid).IsTrue(); + await Assert.That(result.IsValidFor(Severity.Warning)).IsTrue(); + await Assert.That(result.IsValidFor(Severity.Info)).IsFalse(); + } + + [Test] + public async Task I_can_have_error_problems_invalidate_by_default() + { + var validator = new PropertyValidator(person + => person.Name, rule => rule.Must(value => !string.IsNullOrWhiteSpace(value), "Name is required.")); + + var result = validator.Validate(new Person { Name = "" }); + + await Assert.That(result.IsValid).IsFalse(); + await Assert.That(result.IsValidFor(Severity.Error)).IsFalse(); + } +} diff --git a/src/request.validation.tests/ValidatorTests.cs b/src/request.validation.tests/ValidatorTests.cs index 14239f7..820edc4 100644 --- a/src/request.validation.tests/ValidatorTests.cs +++ b/src/request.validation.tests/ValidatorTests.cs @@ -14,7 +14,8 @@ internal sealed class ValidatorTests var result = validator.Validate(new Person { Name = "" }); - await Assert.That(result.IsValid).IsFalse(); + await Assert.That(result.IsValid).IsTrue(); + await Assert.That(result.IsValidFor(Severity.Warning)).IsFalse(); await Assert.That(result.Problems).Count().IsEqualTo(1); var problem = result.Problems.Single(); @@ -39,12 +40,24 @@ internal sealed class ValidatorTests using (Assert.Multiple()) { - await Assert.That(invalid.IsValid).IsFalse(); + await Assert.That(invalid.IsValid).IsTrue(); + await Assert.That(invalid.IsValidFor(Severity.Warning)).IsFalse(); await Assert.That(valid.IsValid).IsTrue(); await Assert.That(valid.Problems).IsEmpty(); } } + [Test] + public async Task I_can_validate_with_a_null_instance_without_throwing() + { + var validator = new PropertyValidator(person + => person!.Name, rule => rule.Must(value => value is not null, "Name is required.")); + + var result = validator.Validate((Person?)null); + + await Assert.That(result.IsValid).IsTrue(); + } + [Test] public async Task I_can_compose_nested_validators_and_aggregate_property_paths() { diff --git a/src/request.validation.tests/_fixtures/ComparableModel.cs b/src/request.validation.tests/_fixtures/ComparableModel.cs new file mode 100644 index 0000000..9178228 --- /dev/null +++ b/src/request.validation.tests/_fixtures/ComparableModel.cs @@ -0,0 +1,19 @@ +// Copyright (c) The Geekeey Authors +// SPDX-License-Identifier: EUPL-1.2 + +namespace Geekeey.Request.Validation.Tests; + +internal sealed class ComparableModel +{ + public ComparableValue? Value { get; init; } +} + +internal sealed class ComparableValue : IComparable +{ + public int Number { get; init; } + + public int CompareTo(ComparableValue? other) + { + return Number.CompareTo(other!.Number); + } +} diff --git a/src/request.validation/Rule.cs b/src/request.validation/Rule.cs index 0c65cc0..a6a397c 100644 --- a/src/request.validation/Rule.cs +++ b/src/request.validation/Rule.cs @@ -46,7 +46,7 @@ internal abstract record Rule : Rule if (context.Instance is null && default(T) is null) { - return Validate((T)context.Instance!, context); + return []; } var actualType = context.Instance?.GetType().FullName ?? "null"; diff --git a/src/request.validation/RuleBuilderExtensions.cs b/src/request.validation/RuleBuilderExtensions.cs index 528b387..7e1a5db 100644 --- a/src/request.validation/RuleBuilderExtensions.cs +++ b/src/request.validation/RuleBuilderExtensions.cs @@ -170,7 +170,7 @@ public static class RuleBuilderExtensions { ArgumentNullException.ThrowIfNull(rule); - return rule.Must(value => IsNull(value) || value.CompareTo(comparisonValue) > 0, + return rule.Must(value => IsNull(value) || IsNull(comparisonValue) || value.CompareTo(comparisonValue) > 0, $"Value must be greater than {comparisonValue}."); } @@ -189,7 +189,7 @@ public static class RuleBuilderExtensions { ArgumentNullException.ThrowIfNull(rule); - return rule.Must(value => IsNull(value) || value.CompareTo(comparisonValue) >= 0, + return rule.Must(value => IsNull(value) || IsNull(comparisonValue) || value.CompareTo(comparisonValue) >= 0, $"Value must be greater than or equal to {comparisonValue}."); } @@ -208,7 +208,7 @@ public static class RuleBuilderExtensions { ArgumentNullException.ThrowIfNull(rule); - return rule.Must(value => IsNull(value) || value.CompareTo(comparisonValue) < 0, + return rule.Must(value => IsNull(value) || IsNull(comparisonValue) || value.CompareTo(comparisonValue) < 0, $"Value must be less than {comparisonValue}."); } @@ -227,7 +227,7 @@ public static class RuleBuilderExtensions { ArgumentNullException.ThrowIfNull(rule); - return rule.Must(value => IsNull(value) || value.CompareTo(comparisonValue) <= 0, + return rule.Must(value => IsNull(value) || IsNull(comparisonValue) || value.CompareTo(comparisonValue) <= 0, $"Value must be less than or equal to {comparisonValue}."); } @@ -254,7 +254,9 @@ public static class RuleBuilderExtensions "Maximum value must be greater than or equal to minimum value."); } - return rule.Must(value => IsNull(value) || (value.CompareTo(minValue) >= 0 && value.CompareTo(maxValue) <= 0), + return rule.Must(value => IsNull(value) + || ((minValue is null || value.CompareTo(minValue) >= 0) + && (maxValue is null || value.CompareTo(maxValue) <= 0)), $"Value must be between {minValue} and {maxValue}."); } diff --git a/src/request.validation/Validation.cs b/src/request.validation/Validation.cs index fadadff..8e9c7c7 100644 --- a/src/request.validation/Validation.cs +++ b/src/request.validation/Validation.cs @@ -23,7 +23,27 @@ public sealed class Validation /// /// Whether the validation was successful. /// - public bool IsValid => Problems.Count is 0; + /// + /// A validation is considered successful unless it contains at least one problem with + /// . Problems of lower severity ( or + /// ) do not invalidate the result. Use + /// to control which severities are treated as failures. + /// + public bool IsValid => IsValidFor(Severity.Error); + + /// + /// Whether the validation was successful, treating problems at or above the given severity as failures. + /// + /// The minimum severity that should be considered a failure. + /// + /// A problem invalidates the validation when its is at least as severe as + /// . For example, IsValidFor(Severity.Warning) fails on errors and + /// warnings but not on informational problems. + /// + public bool IsValidFor(Severity minimum) + { + return !Problems.Any(problem => problem.Severity <= minimum); + } /// /// The problems that were found during validation.