diff --git a/Elsa.sln b/Elsa.sln index 9def60352..eba811766 100644 --- a/Elsa.sln +++ b/Elsa.sln @@ -327,6 +327,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Elsa.Http.IntegrationTests" EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Elsa.Tenants.UnitTests", "test\unit\Elsa.Tenants.UnitTests\Elsa.Tenants.UnitTests.csproj", "{DC476900-D836-4920-A696-CF8796668723}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Elsa.Features.UnitTests", "test\unit\Elsa.Features.UnitTests\Elsa.Features.UnitTests.csproj", "{F77B17C4-B722-402F-A238-48CDC5AE7565}" +EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "shells", "shells", "{FD31E565-A5D9-4F25-B484-2F27FAB99B17}" EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Elsa.Shells.Api", "src\modules\Elsa.Shells.Api\Elsa.Shells.Api.csproj", "{23BBF2CE-D2BA-43D4-8C78-A0B7B4AD6219}" @@ -1333,6 +1335,18 @@ Global {DC476900-D836-4920-A696-CF8796668723}.Release|x64.Build.0 = Release|Any CPU {DC476900-D836-4920-A696-CF8796668723}.Release|x86.ActiveCfg = Release|Any CPU {DC476900-D836-4920-A696-CF8796668723}.Release|x86.Build.0 = Release|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Debug|Any CPU.Build.0 = Debug|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Debug|x64.ActiveCfg = Debug|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Debug|x64.Build.0 = Debug|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Debug|x86.ActiveCfg = Debug|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Debug|x86.Build.0 = Debug|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Release|Any CPU.ActiveCfg = Release|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Release|Any CPU.Build.0 = Release|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Release|x64.ActiveCfg = Release|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Release|x64.Build.0 = Release|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Release|x86.ActiveCfg = Release|Any CPU + {F77B17C4-B722-402F-A238-48CDC5AE7565}.Release|x86.Build.0 = Release|Any CPU {23BBF2CE-D2BA-43D4-8C78-A0B7B4AD6219}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {23BBF2CE-D2BA-43D4-8C78-A0B7B4AD6219}.Debug|Any CPU.Build.0 = Debug|Any CPU {23BBF2CE-D2BA-43D4-8C78-A0B7B4AD6219}.Debug|x64.ActiveCfg = Debug|Any CPU @@ -2382,6 +2396,7 @@ Global {A3C07D5B-2A30-494E-B9BC-4B1594B31ABC} = {18453B51-25EB-4317-A4B3-B10518252E92} {8C4F6A2D-1E9F-4B3C-9D8E-7F5A6B4C3D2E} = {1B8D5897-902E-4632-8698-E89CAF3DDF54} {DC476900-D836-4920-A696-CF8796668723} = {18453B51-25EB-4317-A4B3-B10518252E92} + {F77B17C4-B722-402F-A238-48CDC5AE7565} = {18453B51-25EB-4317-A4B3-B10518252E92} {FD31E565-A5D9-4F25-B484-2F27FAB99B17} = {5BA4A8FA-F7F4-45B3-AEC8-8886D35AAC79} {23BBF2CE-D2BA-43D4-8C78-A0B7B4AD6219} = {FD31E565-A5D9-4F25-B484-2F27FAB99B17} {85E81AC4-10EF-4A93-A7A3-930087621ACC} = {D92BEAB2-60D6-4BB4-885A-6BA681C6CCF1} diff --git a/doc/wiki/module-system.md b/doc/wiki/module-system.md index 352afa351..0af524ceb 100644 --- a/doc/wiki/module-system.md +++ b/doc/wiki/module-system.md @@ -23,6 +23,8 @@ Feature classes usually use three lifecycle methods: `Module.Apply()` topologically sorts configured features and dependencies, configures them once, filters features with missing optional dependencies, registers hosted services, applies services, and finally registers installed-feature metadata. +A feature may introduce another feature from `Apply()`, for example by calling `Module.Configure()` directly or through a helper such as `AddActivity()`. `Module.Apply()` keeps applying until no new features show up, so the introduced feature (and its dependencies) are configured and applied as well. + ## Entry Points The common public path is: @@ -93,4 +95,4 @@ When adding a new module, follow this shape: - Do not register services in extension methods when the module already has a feature class. Put service registration in `Apply()`. - Do not bypass dependencies with direct service provider access in unrelated modules. Add a contract and dependency if the relationship is real. - Use `TryAdd*` for overridable defaults and normal `Add*` for deliberate multiple registrations such as handlers, validators, and descriptors. -- If a feature uses `Module.Configure()`, verify that the other feature is already a dependency or that optional behavior is intentional. +- If a feature uses `Module.Configure()`, verify that the other feature is already a dependency or that optional behavior is intentional. Introducing a feature this way works from `Configure()` and from `Apply()`, but declaring `[DependsOn(typeof(OtherFeature))]` keeps the relationship visible and gives the dependency the normal ordering. diff --git a/src/common/Elsa.Features/Implementations/Module.cs b/src/common/Elsa.Features/Implementations/Module.cs index ba9cc1d2e..0153c6f1e 100644 --- a/src/common/Elsa.Features/Implementations/Module.cs +++ b/src/common/Elsa.Features/Implementations/Module.cs @@ -106,13 +106,23 @@ public class Module : IModule // Filter out features that depend on other features that are not installed. _features = ExcludeFeaturesWithMissingDependencies(_features.Values).ToDictionary(x => x.GetType(), x => x); - // Add hosted services in order of priority. - foreach (var hostedServiceDescriptor in _hostedServiceDescriptors.OrderBy(x => x.Order)) - Services.TryAddEnumerable(ServiceDescriptor.Singleton(typeof(IHostedService), hostedServiceDescriptor.Type)); + // Hosted services are registered after the features have been applied, because applying a feature can contribute more of them. They still belong + // at this position in the service collection though, ahead of whatever the features register from their Apply method, so remember where that is. + var hostedServiceIndex = Services.Count; - // Make sure to use the complete list of features when applying them. - foreach (var feature in _features.Values) - feature.Apply(); + // Make sure to use the complete list of features when applying them. Applying a feature can introduce additional features (a feature calling + // Module.Configure() from its Apply method, directly or through a helper such as AddActivity()), so keep going until nothing new shows up. + var appliedFeatures = new HashSet(); + while (GetFeaturesPendingApply(appliedFeatures) is { Count: > 0 } pendingFeatures) + { + appliedFeatures.UnionWith(pendingFeatures); + + foreach (var feature in pendingFeatures) + feature.Apply(); + } + + // Add hosted services in order of priority. + RegisterHostedServices(hostedServiceIndex); // Add a registry of enabled features to the service collection for client applications to reflect on what features are installed. var registry = new InstalledFeatureRegistry(); @@ -141,6 +151,40 @@ public class Module : IModule select feature; } + // Registers the configured hosted services in order of priority, starting at the specified index. + private void RegisterHostedServices(int index) + { + var appendIndex = Services.Count; + + foreach (var hostedServiceDescriptor in _hostedServiceDescriptors.OrderBy(x => x.Order)) + Services.TryAddEnumerable(ServiceDescriptor.Singleton(typeof(IHostedService), hostedServiceDescriptor.Type)); + + // TryAddEnumerable appends, so move what it added back to where hosted services configured through this module belong. Doing it this way rather + // than inserting directly keeps the de-duplication behaviour of TryAddEnumerable, which also considers what the features registered themselves. + var appendedDescriptors = Services.Skip(appendIndex).ToList(); + + for (var i = 0; i < appendedDescriptors.Count; i++) + Services.RemoveAt(appendIndex); + + for (var i = 0; i < appendedDescriptors.Count; i++) + Services.Insert(index + i, appendedDescriptors[i]); + } + + // Returns the features that have not been applied yet, sorted so that dependencies are applied before the features that depend on them. + private List GetFeaturesPendingApply(IReadOnlySet appliedFeatures) + { + var pendingFeatureTypes = _features.Where(x => !appliedFeatures.Contains(x.Value)).Select(x => x.Key).ToList(); + var pendingFeatureTypeLookup = pendingFeatureTypes.ToHashSet(); + + // Sorting pulls in dependencies that are not pending themselves, so filter those out again. + return pendingFeatureTypes + .TSort(GetDeclaredDependencyTypes) + .Where(pendingFeatureTypeLookup.Contains) + .Select(x => _features[x]) + .Distinct() + .ToList(); + } + private void ConfigureFeature(IFeature feature) { if (_configuredFeatures.Contains(feature)) @@ -161,13 +205,18 @@ public class Module : IModule { var featureTypes = _features.Keys.ToHashSet(); var featureTypesWithDependencies = featureTypes.Concat(featureTypes.SelectMany(GetDependencyTypes)).ToHashSet(); - return featureTypesWithDependencies.TSort(x => x.GetCustomAttributes(true).Select(dependsOn => dependsOn.Type)).ToHashSet(); + return featureTypesWithDependencies.TSort(GetDeclaredDependencyTypes).ToHashSet(); + } + + private static IEnumerable GetDeclaredDependencyTypes(Type type) + { + return type.GetCustomAttributes(true).Select(dependsOn => dependsOn.Type); } // Recursively get dependency types. private IEnumerable GetDependencyTypes(Type type) { - var dependencies = type.GetCustomAttributes(true).Select(dependsOn => dependsOn.Type).ToList(); + var dependencies = GetDeclaredDependencyTypes(type).ToList(); return dependencies.Concat(dependencies.SelectMany(GetDependencyTypes)); } } \ No newline at end of file diff --git a/src/common/Elsa.Features/Services/IModule.cs b/src/common/Elsa.Features/Services/IModule.cs index da070d3f6..bbba12b59 100644 --- a/src/common/Elsa.Features/Services/IModule.cs +++ b/src/common/Elsa.Features/Services/IModule.cs @@ -51,5 +51,9 @@ public interface IModule /// /// Will apply all configured features, causing the collection to be populated. /// + /// + /// Features are allowed to introduce additional features from their own method, e.g. by calling . + /// Those features are configured and applied as part of the same call. + /// void Apply(); } \ No newline at end of file diff --git a/test/unit/Elsa.Features.UnitTests/Elsa.Features.UnitTests.csproj b/test/unit/Elsa.Features.UnitTests/Elsa.Features.UnitTests.csproj new file mode 100644 index 000000000..ab31ccaf8 --- /dev/null +++ b/test/unit/Elsa.Features.UnitTests/Elsa.Features.UnitTests.csproj @@ -0,0 +1,16 @@ + + + + [Elsa.Features]* + 0 + + + + + + + + + + + diff --git a/test/unit/Elsa.Features.UnitTests/ModuleTests.cs b/test/unit/Elsa.Features.UnitTests/ModuleTests.cs new file mode 100644 index 000000000..2f1017e71 --- /dev/null +++ b/test/unit/Elsa.Features.UnitTests/ModuleTests.cs @@ -0,0 +1,240 @@ +using Elsa.Features.Abstractions; +using Elsa.Features.Attributes; +using Elsa.Features.Contracts; +using Elsa.Features.Implementations; +using Elsa.Features.Services; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; + +namespace Elsa.Features.UnitTests; + +/// +/// Tests , with an emphasis on features that introduce additional features from their own method. +/// +public class ModuleTests +{ + private const string AppliedFeaturesKey = "AppliedFeatures"; + + private readonly ServiceCollection _services = new(); + private readonly Module _module; + private readonly List _appliedFeatures = new(); + + public ModuleTests() + { + _module = new(_services); + _module.Properties[AppliedFeaturesKey] = _appliedFeatures; + } + + [Fact] + public void Apply_AppliesFeatureIntroducedFromApply() + { + _module.Configure(); + + _module.Apply(); + + Assert.Contains(typeof(IntroducedFeature), _appliedFeatures); + Assert.Contains(_services, x => x.ServiceType == typeof(IntroducedMarker)); + } + + [Fact] + public void Apply_AppliesEntireChainOfFeaturesIntroducedFromApply() + { + _module.Configure(); + + _module.Apply(); + + Assert.Contains(typeof(ChainMiddleFeature), _appliedFeatures); + Assert.Contains(typeof(ChainLeafFeature), _appliedFeatures); + } + + [Fact] + public void Apply_AppliesDependenciesOfFeatureIntroducedFromApplyBeforeThatFeature() + { + _module.Configure(); + + _module.Apply(); + + Assert.Contains(typeof(IntroducedDependencyFeature), _appliedFeatures); + Assert.True(_appliedFeatures.IndexOf(typeof(IntroducedDependencyFeature)) < _appliedFeatures.IndexOf(typeof(IntroducedDependentFeature))); + } + + [Fact] + public void Apply_RegistersHostedServicesOfFeatureIntroducedFromApply() + { + _module.Configure(); + + _module.Apply(); + + Assert.Contains(_services, x => x.ServiceType == typeof(IHostedService) && x.ImplementationType == typeof(IntroducedHostedService)); + } + + [Fact] + public void Apply_ListsFeatureIntroducedFromApplyInTheInstalledFeatureRegistry() + { + _module.Configure(); + + _module.Apply(); + + Assert.NotNull(GetInstalledFeatureRegistry().Find("Elsa.Introduced")); + } + + [Fact] + public void Apply_AppliesEachFeatureOnlyOnce() + { + _module.Configure(); + + _module.Apply(); + + Assert.Equal(_appliedFeatures.Distinct().Count(), _appliedFeatures.Count); + } + + [Fact] + public void Apply_AppliesFeaturesInDependencyOrder() + { + _module.Configure(); + + _module.Apply(); + + Assert.Equal([typeof(DependencyFeature), typeof(DependentFeature)], _appliedFeatures); + } + + [Fact] + public void Apply_RegistersHostedServicesInPriorityOrder() + { + _module.ConfigureHostedService(2); + _module.ConfigureHostedService(1); + _module.Configure(); + + _module.Apply(); + + Assert.Equal([typeof(FirstHostedService), typeof(SecondHostedService), typeof(IntroducedHostedService)], GetHostedServiceTypes()); + } + + [Fact] + public void Apply_OrdersHostedServiceOfFeatureIntroducedFromApplyByPriority() + { + _module.ConfigureHostedService(10); + _module.Configure(); + + _module.Apply(); + + // The introduced feature configures its hosted service at priority 3, so it has to come first even though it shows up last. + Assert.Equal([typeof(IntroducedHostedService), typeof(SecondHostedService)], GetHostedServiceTypes()); + } + + [Fact] + public void Apply_RegistersConfiguredHostedServicesBeforeThoseRegisteredFromApply() + { + _module.ConfigureHostedService(); + _module.Configure(); + + _module.Apply(); + + Assert.Equal([typeof(FirstHostedService), typeof(SelfRegisteredHostedService)], GetHostedServiceTypes()); + } + + private List GetHostedServiceTypes() + { + return _services.Where(x => x.ServiceType == typeof(IHostedService)).Select(x => x.ImplementationType).ToList(); + } + + private IInstalledFeatureRegistry GetInstalledFeatureRegistry() + { + return (IInstalledFeatureRegistry)_services.Single(x => x.ServiceType == typeof(IInstalledFeatureRegistry)).ImplementationInstance!; + } + + /// + /// Records the order in which features are applied so that tests can assert on it. + /// + public abstract class RecordingFeature(IModule module) : FeatureBase(module) + { + public override void Apply() => ((List)Module.Properties[AppliedFeaturesKey]).Add(GetType()); + } + + public class IntroducingFeature(IModule module) : RecordingFeature(module) + { + public override void Apply() + { + base.Apply(); + Module.Configure(); + } + } + + public class IntroducedFeature(IModule module) : RecordingFeature(module) + { + public override void ConfigureHostedServices() => ConfigureHostedService(3); + + public override void Apply() + { + base.Apply(); + Services.AddSingleton(); + } + } + + /// + /// Registers a hosted service straight with the service collection, the way WorkflowRuntimeFeature does, rather than through the module. + /// + public class HostedServiceRegisteringFeature(IModule module) : RecordingFeature(module) + { + public override void Apply() + { + base.Apply(); + Services.AddHostedService(); + } + } + + public class ChainIntroducingFeature(IModule module) : RecordingFeature(module) + { + public override void Apply() + { + base.Apply(); + Module.Configure(); + } + } + + public class ChainMiddleFeature(IModule module) : RecordingFeature(module) + { + public override void Apply() + { + base.Apply(); + Module.Configure(); + } + } + + public class ChainLeafFeature(IModule module) : RecordingFeature(module); + + public class IntroducingDependentFeature(IModule module) : RecordingFeature(module) + { + public override void Apply() + { + base.Apply(); + Module.Configure(); + } + } + + [DependsOn(typeof(IntroducedDependencyFeature))] + public class IntroducedDependentFeature(IModule module) : RecordingFeature(module); + + public class IntroducedDependencyFeature(IModule module) : RecordingFeature(module); + + [DependsOn(typeof(DependencyFeature))] + public class DependentFeature(IModule module) : RecordingFeature(module); + + public class DependencyFeature(IModule module) : RecordingFeature(module); + + public class IntroducedMarker; + + public abstract class NoopHostedService : IHostedService + { + public Task StartAsync(CancellationToken cancellationToken) => Task.CompletedTask; + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; + } + + public class IntroducedHostedService : NoopHostedService; + + public class SelfRegisteredHostedService : NoopHostedService; + + public class FirstHostedService : NoopHostedService; + + public class SecondHostedService : NoopHostedService; +}