Add transport-agnostic engine contracts and adapters - #128
Add transport-agnostic engine contracts and adapters#128SamuelMcAravey wants to merge 2 commits into
Conversation
…port-agnostic-modules
There was a problem hiding this comment.
Pull request overview
This PR introduces a transport-agnostic engine architecture for the Bravellian platform, enabling modules to expose UI and webhook processing logic independently of HTTP, Razor Pages, or other transport concerns.
Key changes:
- Adds core engine contracts (
IUiEngine<TInput, TViewModel>,IWebhookEngine<TPayload>) with corresponding result types and metadata structures - Implements discovery and registry services for engine resolution across modules
- Provides reference adapter implementations for UI (navigation tokens) and webhooks (signature validation, outcome mapping)
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
src/Bravellian.Platform.Modularity.Core/IEngineModule.cs |
Defines marker interface for modules that expose engines via DescribeEngines method |
src/Bravellian.Platform.Modularity.Core/IUiEngine.cs |
Core UI engine contract accepting input DTO and returning view model with navigation tokens |
src/Bravellian.Platform.Modularity.Core/IWebhookEngine.cs |
Core webhook engine contract processing payloads and returning outcome directives |
src/Bravellian.Platform.Modularity.Core/EngineKind.cs |
Enum distinguishing UI vs webhook engine types |
src/Bravellian.Platform.Modularity.Core/ModuleEngineDescriptor.cs |
Descriptor capturing engine manifest, contract type, and factory for DI resolution |
src/Bravellian.Platform.Modularity.Core/ModuleEngineManifest.cs |
Rich metadata structure declaring engine capabilities, schemas, security, and adapter hints |
src/Bravellian.Platform.Modularity.Core/ModuleEngineSchema.cs |
Schema hint record capturing CLR types for inputs/outputs |
src/Bravellian.Platform.Modularity.Core/ModuleEngineCapabilities.cs |
Declares actions/events and async/streaming support flags |
src/Bravellian.Platform.Modularity.Core/ModuleEngineAdapterHints.cs |
Transport-level hints for raw body, headers, auth, and tenancy requirements |
src/Bravellian.Platform.Modularity.Core/ModuleEngineCompatibility.cs |
Versioning and breaking change metadata |
src/Bravellian.Platform.Modularity.Core/ModuleEngineSecurity.cs |
Security configuration for signature algorithm, secret scope, and idempotency window |
src/Bravellian.Platform.Modularity.Core/ModuleEngineWebhookMetadata.cs |
Webhook event registration with provider, event type, and retry policy |
src/Bravellian.Platform.Modularity.Core/ModuleEngineNavigationHints.cs |
Navigation token collection for abstract routing |
src/Bravellian.Platform.Modularity.Core/UiEngineResult.cs |
Result record containing view model, navigation tokens, and domain events |
src/Bravellian.Platform.Modularity.Core/WebhookRequest.cs |
Webhook request envelope with provider, event type, payload, and idempotency key |
src/Bravellian.Platform.Modularity.Core/WebhookOutcome.cs |
Outcome record directing adapter to acknowledge, retry, or enqueue event |
src/Bravellian.Platform.Modularity.Core/WebhookOutcomeType.cs |
Enum for webhook outcome types |
src/Bravellian.Platform.Modularity.Core/ModuleEngineRegistry.cs |
Internal static registry managing engine descriptors with thread-safe registration |
src/Bravellian.Platform.Modularity.Core/ModuleEngineDiscoveryService.cs |
Public discovery service for listing, filtering, and resolving engines |
src/Bravellian.Platform.Modularity.Core/ModuleRegistry.cs |
Enhanced to auto-register engines when modules implement IEngineModule |
src/Bravellian.Platform.Modularity.FullStack/UiEngineAdapter.cs |
Reference UI adapter resolving engines and mapping results to navigation responses |
src/Bravellian.Platform.Modularity.FullStack/UiAdapterResponse.cs |
Transport-ready response with view model and navigation targets |
src/Bravellian.Platform.Modularity.FullStack/FullStackModuleServiceCollectionExtensions.cs |
Registers ModuleEngineDiscoveryService in DI container |
src/Bravellian.Platform.Modularity.Api/WebhookEngineAdapter.cs |
Reference webhook adapter with signature validation and outcome mapping |
src/Bravellian.Platform.Modularity.Api/WebhookAdapterRequest.cs |
HTTP-style webhook request envelope with headers, raw body, and signature |
src/Bravellian.Platform.Modularity.Api/WebhookAdapterResponse.cs |
Transport response indicating outcome and optional reason |
src/Bravellian.Platform.Modularity.Api/IWebhookSignatureValidator.cs |
Pluggable signature validation interface |
src/Bravellian.Platform.Modularity.Api/ApiModuleServiceCollectionExtensions.cs |
Registers ModuleEngineDiscoveryService for API modules |
src/Bravellian.Platform.Modularity.Core/BackgroundModuleServiceCollectionExtensions.cs |
Registers ModuleEngineDiscoveryService for background modules |
tests/Bravellian.Platform.Tests/EngineRefactoringTests.cs |
Headless integration tests demonstrating UI invocation, webhook dispatch, and discovery filtering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| var descriptor = discoveryService.ResolveById(moduleKey, engineId) | ||
| ?? throw new InvalidOperationException($"No UI engine registered with id '{engineId}' for module '{moduleKey}'."); | ||
|
|
There was a problem hiding this comment.
The UiEngineAdapter.ExecuteAsync method does not validate that the resolved engine's Manifest.Kind is EngineKind.Ui before casting to IUiEngine. While the descriptor lookup should ensure this, defensive validation would make the error clearer if a module incorrectly registers a webhook engine with a UI-looking ID. Consider adding a check for descriptor.Manifest.Kind == EngineKind.Ui and throwing a more descriptive exception if the kind doesn't match.
| if (descriptor.Manifest.Kind != EngineKind.Ui) | |
| { | |
| throw new InvalidOperationException( | |
| $"Engine '{engineId}' for module '{moduleKey}' is of kind '{descriptor.Manifest.Kind}' but a UI engine was expected."); | |
| } |
| yield return new ModuleEngineDescriptor( | ||
| Key, | ||
| new ModuleEngineManifest( | ||
| "ui.login", | ||
| "1.0", | ||
| "Login page engine", | ||
| EngineKind.Ui, | ||
| "Auth", | ||
| new ModuleEngineCapabilities(new[] { "login" }, new[] { "login.loggedIn" }, SupportsStreaming: false), | ||
| new[] { new ModuleEngineSchema("command", typeof(LoginCommand)) }, | ||
| new[] { new ModuleEngineSchema("viewModel", typeof(LoginViewModel)) }, | ||
| new[] { "route:dashboard" }, | ||
| new[] { nameof(LoginUiEngine) }, | ||
| new ModuleEngineAdapterHints(false, false, false, false, true), | ||
| null, | ||
| new ModuleEngineCompatibility("1.0", null)), | ||
| typeof(IUiEngine<LoginCommand, LoginViewModel>), | ||
| sp => sp.GetRequiredService<LoginUiEngine>()); | ||
|
|
||
| yield return new ModuleEngineDescriptor( | ||
| Key, | ||
| new ModuleEngineManifest( | ||
| "webhook.postmark", | ||
| "1.0", | ||
| "Postmark bounce webhook handler", | ||
| EngineKind.Webhook, | ||
| "Notifications", | ||
| new ModuleEngineCapabilities(new[] { "handle" }, new[] { "bounce.received" }), | ||
| new[] { new ModuleEngineSchema("payload", typeof(PostmarkBouncePayload)) }, | ||
| Array.Empty<ModuleEngineSchema>(), | ||
| Array.Empty<string>(), | ||
| new[] { nameof(PostmarkWebhookEngine) }, | ||
| new ModuleEngineAdapterHints(true, true, true, false, true), | ||
| new ModuleEngineSecurity("HMAC-SHA256", "postmark", TimeSpan.FromMinutes(10)), | ||
| new ModuleEngineCompatibility("1.0", "Initial"), | ||
| new[] | ||
| { | ||
| new ModuleEngineWebhookMetadata("postmark", "bounce", new ModuleEngineSchema("payload", typeof(PostmarkBouncePayload)), new[] { "logging" }, Retries: 3), | ||
| }), | ||
| typeof(IWebhookEngine<PostmarkBouncePayload>), | ||
| sp => sp.GetRequiredService<PostmarkWebhookEngine>()); | ||
| } |
There was a problem hiding this comment.
The ModuleEngineDescriptor is constructed with the module Key in the DescribeEngines method (line 137, 156), but then overridden again in ModuleRegistry.cs (line 87) using a with expression. This results in redundant work and potential confusion. The module Key should either be passed as empty string or null in DescribeEngines, or the override in ModuleRegistry should be removed. The current implementation works but creates unnecessary object allocations.
| /// <summary> | ||
| /// Resolves an engine instance for a descriptor. | ||
| /// </summary> | ||
| public object ResolveEngine(ModuleEngineDescriptor descriptor, IServiceProvider serviceProvider) => descriptor.Factory(serviceProvider); |
There was a problem hiding this comment.
The descriptor parameter is not validated for null before being passed to the Factory. If a null descriptor is passed, this will throw a NullReferenceException when accessing descriptor.Factory. Add a guard clause to throw ArgumentNullException if descriptor is null, or use the null-forgiving operator if null is expected to be caught earlier in the call chain.
| public object ResolveEngine(ModuleEngineDescriptor descriptor, IServiceProvider serviceProvider) => descriptor.Factory(serviceProvider); | |
| public object ResolveEngine(ModuleEngineDescriptor descriptor, IServiceProvider serviceProvider) | |
| { | |
| if (descriptor is null) | |
| { | |
| throw new ArgumentNullException(nameof(descriptor)); | |
| } | |
| return descriptor.Factory(serviceProvider); | |
| } |
| private sealed class TestSignatureValidator : IWebhookSignatureValidator | ||
| { | ||
| public bool Validate(ModuleEngineSecurity security, IReadOnlyDictionary<string, string> headers, string rawBody, string? providedSignature) | ||
| { | ||
| headers.TryGetValue("X-Signature", out var headerSignature); | ||
| var signature = providedSignature ?? headerSignature; | ||
| return signature == $"{security.SecretScope}:{rawBody}"; | ||
| } |
There was a problem hiding this comment.
The test uses a simplified validation that concatenates SecretScope and RawBody with a colon. However, real webhook signature validation typically uses HMAC algorithms where the secret is used as a key, not concatenated. This test implementation could give developers a misleading example of how webhook signatures work. Consider adding a comment clarifying that this is a simplified test validator and not a production implementation pattern.
| /// Executes the engine using the provided command DTO. | ||
| /// </summary> | ||
| /// <param name="command">Command DTO.</param> | ||
| /// <param name="cancellationToken">Cancellation token.</param> | ||
| /// <returns>A view model and any navigation tokens emitted.</returns> | ||
| Task<UiEngineResult<TViewModel>> ExecuteAsync(TInput command, CancellationToken cancellationToken); |
There was a problem hiding this comment.
The parameter name 'command' in ExecuteAsync is misleading when the TInput type parameter could represent any input DTO, not just commands. For example, in a read-only UI engine, the input might be a query DTO. Consider renaming to 'input' to match the generic type parameter TInput, which would make the API more semantically accurate and avoid confusion about CQRS command patterns.
| /// Executes the engine using the provided command DTO. | |
| /// </summary> | |
| /// <param name="command">Command DTO.</param> | |
| /// <param name="cancellationToken">Cancellation token.</param> | |
| /// <returns>A view model and any navigation tokens emitted.</returns> | |
| Task<UiEngineResult<TViewModel>> ExecuteAsync(TInput command, CancellationToken cancellationToken); | |
| /// Executes the engine using the provided input DTO. | |
| /// </summary> | |
| /// <param name="input">Input DTO.</param> | |
| /// <param name="cancellationToken">Cancellation token.</param> | |
| /// <returns>A view model and any navigation tokens emitted.</returns> | |
| Task<UiEngineResult<TViewModel>> ExecuteAsync(TInput input, CancellationToken cancellationToken); |
| public static void Register(string moduleKey, IEnumerable<ModuleEngineDescriptor> descriptors) | ||
| { | ||
| var list = Engines.GetOrAdd(moduleKey, _ => new List<ModuleEngineDescriptor>()); | ||
| lock (list) | ||
| { | ||
| list.AddRange(descriptors); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential race condition in the Register method. While individual lists are locked during AddRange, another thread could call GetEngines during registration and read a partially populated list. This could cause engines from the same module to appear as separate lists if GetEngines is called between GetOrAdd and lock acquisition. Consider using a ReaderWriterLockSlim around the entire Engines dictionary to ensure consistent reads during concurrent registration and enumeration.
| new WebhookRequest<TPayload>(request.Provider, request.EventType, request.Payload, request.IdempotencyKey, request.Attempt), | ||
| cancellationToken).ConfigureAwait(false); | ||
|
|
||
| return new WebhookAdapterResponse(outcome.Outcome, outcome.Reason); |
There was a problem hiding this comment.
The Outcome from WebhookOutcome.Retry and WebhookOutcome.Enqueue should result in different handling, but the WebhookAdapterResponse only captures Outcome and Reason. The EnqueuedEvent field from WebhookOutcome is being silently dropped. If the transport layer needs to enqueue events, it won't have access to the event payload. Consider adding an EnqueuedEvent property to WebhookAdapterResponse and mapping outcome.EnqueuedEvent to it.
| return new WebhookAdapterResponse(outcome.Outcome, outcome.Reason); | |
| return new WebhookAdapterResponse(outcome.Outcome, outcome.Reason, outcome.EnqueuedEvent); |
| public sealed class EngineRefactoringTests | ||
| { | ||
| public EngineRefactoringTests() | ||
| { | ||
| FullStackModuleRegistry.RegisterFullStackModule<FakeEngineModule>(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The test module FakeEngineModule is registered in the constructor which runs once per test class instantiation. If xUnit instantiates this class multiple times or if tests run in parallel, this could lead to duplicate registrations in the FullStackModuleRegistry. Consider moving the registration to a class fixture or using the IAsyncLifetime interface to ensure proper setup and teardown, or add [Collection] attribute to serialize test execution.
| public sealed class EngineRefactoringTests | |
| { | |
| public EngineRefactoringTests() | |
| { | |
| FullStackModuleRegistry.RegisterFullStackModule<FakeEngineModule>(); | |
| } | |
| public sealed class EngineRefactoringTests : IClassFixture<EngineRefactoringTests.FullStackModuleFixture> | |
| { | |
| public sealed class FullStackModuleFixture : IAsyncLifetime | |
| { | |
| private static bool _initialized; | |
| public Task InitializeAsync() | |
| { | |
| if (!_initialized) | |
| { | |
| FullStackModuleRegistry.RegisterFullStackModule<FakeEngineModule>(); | |
| _initialized = true; | |
| } | |
| return Task.CompletedTask; | |
| } | |
| public Task DisposeAsync() | |
| { | |
| // No teardown required for FakeEngineModule registration in these tests. | |
| return Task.CompletedTask; | |
| } | |
| } | |
| public EngineRefactoringTests(FullStackModuleFixture _) | |
| { | |
| // Module registration is handled once by FullStackModuleFixture. | |
| } |
| public sealed record ModuleEngineManifest( | ||
| string Id, | ||
| string Version, | ||
| string Description, | ||
| EngineKind Kind, | ||
| string? FeatureArea = null, | ||
| ModuleEngineCapabilities? Capabilities = null, | ||
| IReadOnlyCollection<ModuleEngineSchema>? Inputs = null, | ||
| IReadOnlyCollection<ModuleEngineSchema>? Outputs = null, | ||
| IReadOnlyCollection<string>? NavigationHints = null, | ||
| IReadOnlyCollection<string>? RequiredServices = null, | ||
| ModuleEngineAdapterHints? AdapterHints = null, | ||
| ModuleEngineSecurity? Security = null, | ||
| ModuleEngineCompatibility? Compatibility = null, | ||
| IReadOnlyCollection<ModuleEngineWebhookMetadata>? WebhookMetadata = null); |
There was a problem hiding this comment.
The ModuleEngineManifest accepts nullable collections for Inputs, Outputs, NavigationHints, RequiredServices, and WebhookMetadata, but there is no validation to ensure these are non-empty when they should contain values. For instance, a webhook engine should probably require at least one WebhookMetadata entry, and a UI engine should likely have at least one Output schema. Consider adding factory methods or validation that enforces logical constraints based on the EngineKind.
| if (descriptor.Manifest.Security is { } security) | ||
| { | ||
| if (!signatureValidator.Validate(security, request.Headers, request.RawBody, request.Signature)) | ||
| { | ||
| return new WebhookAdapterResponse(WebhookOutcomeType.Retry, "Signature validation failed"); | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (descriptor.Manifest.Security is { } security) | |
| { | |
| if (!signatureValidator.Validate(security, request.Headers, request.RawBody, request.Signature)) | |
| { | |
| return new WebhookAdapterResponse(WebhookOutcomeType.Retry, "Signature validation failed"); | |
| } | |
| if (descriptor.Manifest.Security is { } security && | |
| !signatureValidator.Validate(security, request.Headers, request.RawBody, request.Signature)) | |
| { | |
| return new WebhookAdapterResponse(WebhookOutcomeType.Retry, "Signature validation failed"); |
Summary
Testing
Codex Task