Merge pull request #606 from lilla28/fix/module-loader-registration-vol2

fix(module-loader) - Fixed ModuleLoader behavior with multiple modulecatalogs

architecture/adr-014-module-catalog.md

@@ -0,0 +1,28 @@
+<!-- Morgan Stanley makes this available to you under the Apache License, Version 2.0 (the "License"). You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0. See the NOTICE file distributed with this work for additional information regarding copyright ownership. Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. -->
+
+| Type          | ID            | Status        | Title                   |
+| ------------- | ------------- | ------------- | ----------------------- |
+| ADR           | adr-014       | Accepted      | Multiple ModuleCatalogs |
+
+
+# Architecture Decision Record: Multiple ModuleCatalogs
+
+## Context
+ComposeUI aims to support many different kind of modules. These modules, that can be started by the ModuleLoader should be stored in ModuleCatalogs and every module should have its unique module id. The user should have the ability to handle multiple ModuleCatalogs with the ModuleLoader.
+
+General problem statement:
+- When multiple ModuleCatalogs are used, some modules could have the same module id in different ModuleCatalogs which could cause issues while starting or stopping the module via the ModuleLoader.
+
+## Decision
+- If the same module id is being used for multiple modules (likely in different ModuleCatalogs), the first occurrence will be used. In case of multiple ModuleCatalog registrations, the order of the registration determines which module can be used.
+
+# Alternative solutions that we considered
+- Throwing an exception when multiple modules have the same moduleId either in one or multiple ModuleCatalogs. This might cause an issue that is unresolvable by the developer or the enduser.
+- Prefixing the modules with the name of the ModuleCatalog, but prefixing ids would be non-standard.
+- Aggregating the registered ModuleCatalogs should be implemented by the developer of the application.
+
+## Consequences
+- Our implementation is more permissive than the standard. 
+- We must ensure keeping the order of the ModuleCatalog registrations as the order of registrations will determine which module is contained by our aggregated ModuleCatalog in case of duplicate moduleIds.
+- We must ensure keeping the order of the modules within the invidual ModuleCatalogs as the order of the registered modules will determine which module is contained by our aggregated ModuleCatalog in case of duplicate moduleIds.
+- We must ensure this order when fetching modules asynchronously from the ModuleCatalogs.

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader.Abstractions/IModuleCatalog.cs

@@ -15,6 +15,7 @@ namespace MorganStanley.ComposeUI.ModuleLoader;
 /// <summary>
 /// Represents a collection of modules that can be started in the scope of the application.
 /// </summary>
+/// <exception cref="ModuleNotFoundException">The requested module was not found in the catalog</exception>
 public interface IModuleCatalog
 {
     /// <summary>

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader.Abstractions/ModuleLoaderException.cs

@@ -0,0 +1,36 @@
+/*
+* Morgan Stanley makes this available to you under the Apache License,
+* Version 2.0 (the "License"). You may obtain a copy of the License at
+*
+*      http://www.apache.org/licenses/LICENSE-2.0.
+*
+* See the NOTICE file distributed with this work for additional information
+* regarding copyright ownership. Unless required by applicable law or agreed
+* to in writing, software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+* or implied. See the License for the specific language governing permissions
+* and limitations under the License.
+*/
+
+using System.Runtime.Serialization;
+
+namespace MorganStanley.ComposeUI.ModuleLoader;
+
+public class ModuleLoaderException : Exception
+{
+    public ModuleLoaderException()
+    {
+    }
+
+    protected ModuleLoaderException(SerializationInfo info, StreamingContext context) : base(info, context)
+    {
+    }
+
+    public ModuleLoaderException(string? message) : base(message)
+    {
+    }
+
+    public ModuleLoaderException(string? message, Exception? innerException) : base(message, innerException)
+    {
+    }
+}

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader.Abstractions/ModuleNotFoundException.cs

@@ -0,0 +1,22 @@
+/*
+* Morgan Stanley makes this available to you under the Apache License,
+* Version 2.0 (the "License"). You may obtain a copy of the License at
+*
+*      http://www.apache.org/licenses/LICENSE-2.0.
+*
+* See the NOTICE file distributed with this work for additional information
+* regarding copyright ownership. Unless required by applicable law or agreed
+* to in writing, software distributed under the License is distributed on an
+* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+* or implied. See the License for the specific language governing permissions
+* and limitations under the License.
+*/
+
+namespace MorganStanley.ComposeUI.ModuleLoader;
+
+public class ModuleNotFoundException : ModuleLoaderException
+{
+    public ModuleNotFoundException(string moduleId) : base($"Unknown module id: {moduleId}")
+    {
+    }
+}

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader/AggregateModuleCatalog.cs

@@ -0,0 +1,83 @@
+// Morgan Stanley makes this available to you under the Apache License,
+// Version 2.0 (the "License"). You may obtain a copy of the License at
+// 
+//      http://www.apache.org/licenses/LICENSE-2.0.
+// 
+// See the NOTICE file distributed with this work for additional information
+// regarding copyright ownership. Unless required by applicable law or agreed
+// to in writing, software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+// or implied. See the License for the specific language governing permissions
+// and limitations under the License.
+
+using System.Collections.Concurrent;
+using System.Reflection;
+using System.Runtime.CompilerServices;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
+
+namespace MorganStanley.ComposeUI.ModuleLoader;
+
+internal class AggregateModuleCatalog : IModuleCatalog
+{
+    private readonly IEnumerable<IModuleCatalog> _moduleCatalogs;
+    private readonly ILogger _logger;
+
+    public AggregateModuleCatalog(
+        IEnumerable<IModuleCatalog> moduleCatalogs,
+        ILogger? logger = null)
+    {
+        _moduleCatalogs = moduleCatalogs;
+        _logger = logger ?? NullLogger.Instance;
+        _lazyEnumerateModules = new Lazy<Task>(EnumerateModulesCore);
+    }
+
+    public async Task<IModuleManifest> GetManifest(string moduleId)
+    {
+        await EnumerateModules();
+
+        return _moduleIdToCatalog.TryGetValue(moduleId, out var catalog)
+            ? await catalog.GetManifest(moduleId)
+            : throw new ModuleNotFoundException(moduleId);
+    }
+
+    public async Task<IEnumerable<string>> GetModuleIds()
+    {
+        await EnumerateModules();
+
+        return _moduleIdToCatalog.Keys;
+    }
+
+    private readonly Lazy<Task> _lazyEnumerateModules;
+    private readonly ConcurrentDictionary<string, IModuleCatalog> _moduleIdToCatalog = new();
+
+    private Task EnumerateModules() => _lazyEnumerateModules.Value;
+
+    private async Task EnumerateModulesCore()
+    {
+        var moduleCatalogs = _moduleCatalogs.ToDictionary(x => x, y => y.GetModuleIds());
+        // Services/DI registrations appear in the order they were registered when resolved via IEnumerable<{SERVICE}>
+        // https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection
+        foreach (var moduleCatalog in moduleCatalogs)
+        {
+            var moduleIds = await moduleCatalog.Value;
+
+            foreach (var moduleId in moduleIds)
+            {
+                if (!_moduleIdToCatalog.TryGetValue(moduleId, out var catalog))
+                {
+                    _moduleIdToCatalog[moduleId] = moduleCatalog.Key;
+                }
+                else
+                {
+                    if (_logger.IsEnabled(LogLevel.Warning))
+                    {
+                        var moduleManifest = await catalog.GetManifest(moduleId);
+                        _logger.LogWarning(
+                            $"ModuleId: {moduleId} is already contained by an another {nameof(IModuleCatalog)} with name {moduleManifest.Name}. Please consider using unique ids for modules. The first occurrence of the module will be saved and used by the {nameof(ModuleLoader)}.");
+                    }
+                }
+            }
+        }
+    }
+}

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader/ModuleLoader.cs

@@ -12,40 +12,42 @@
 
 using System.Collections.Concurrent;
 using System.Reactive.Subjects;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 
 namespace MorganStanley.ComposeUI.ModuleLoader;
 
 internal sealed class ModuleLoader : IModuleLoader, IAsyncDisposable
 {
+    private readonly ILogger<ModuleLoader> _logger;
     private readonly Subject<LifetimeEvent> _lifetimeEvents = new();
     private readonly ConcurrentDictionary<Guid, IModuleInstance> _modules = new();
     private readonly Dictionary<string, IModuleRunner> _moduleRunners;
     private readonly IModuleCatalog _moduleCatalog;
     private readonly IReadOnlyList<IStartupAction> _startupActions;
 
     public ModuleLoader(
-        IModuleCatalog moduleCatalog,
+        IEnumerable<IModuleCatalog> moduleCatalogs,
         IEnumerable<IModuleRunner> moduleRunners,
-        IEnumerable<IStartupAction> startupActions)
+        IEnumerable<IStartupAction> startupActions,
+        ILogger<ModuleLoader>? logger = null)
     {
-        ArgumentNullException.ThrowIfNull(moduleCatalog);
+        ArgumentNullException.ThrowIfNull(moduleCatalogs);
         ArgumentNullException.ThrowIfNull(moduleRunners);
         ArgumentNullException.ThrowIfNull(startupActions);
-
-        _moduleCatalog = moduleCatalog;
+
+        _logger = logger ?? NullLogger<ModuleLoader>.Instance;
+        _moduleCatalog = new AggregateModuleCatalog(moduleCatalogs, _logger);
         _moduleRunners = moduleRunners.GroupBy(runner => runner.ModuleType).ToDictionary(g => g.Key, g => g.First());
         _startupActions = new List<IStartupAction>(startupActions);
+
     }
 
     public IObservable<LifetimeEvent> LifetimeEvents => _lifetimeEvents;
 
     public async Task<IModuleInstance> StartModule(StartRequest request)
     {
         var manifest = await _moduleCatalog.GetManifest(request.ModuleId);
-        if (manifest == null)
-        {
-            throw new Exception($"Unknown Module id: {request.ModuleId}");
-        }
 
         if (!_moduleRunners.TryGetValue(manifest.ModuleType, out var moduleRunner))
         {

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader/MorganStanley.ComposeUI.ModuleLoader.csproj

@@ -18,6 +18,7 @@ See the License for the specific language governing permissions and limitations
 
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
     <PackageReference Include="System.Reactive" />
   </ItemGroup>
 

src/module-loader/dotnet/src/MorganStanley.ComposeUI.ModuleLoader/Runners/NativeModuleRunner.cs

@@ -22,7 +22,7 @@ public async Task Start(StartupContext startupContext, Func<Task> pipeline)
     {
         if (!startupContext.ModuleInstance.Manifest.TryGetDetails(out NativeManifestDetails details))
         {
-            throw new Exception("Unable to get native maifest details");
+            throw new Exception("Unable to get native manifest details");
         }
 
         startupContext.AddProperty(new EnvironmentVariables(details.EnvironmentVariables));

src/module-loader/dotnet/tests/MorganStanley.ComposeUI.ModuleLoader.Abstractions.Tests/MorganStanley.ComposeUI.ModuleLoader.Abstractions.Tests.csproj

@@ -10,6 +10,7 @@
 	</PropertyGroup>
 
 	<ItemGroup>
+		<PackageReference Include="FluentAssertions" />
 		<PackageReference Include="Microsoft.NET.Test.Sdk" />
 		<PackageReference Include="Moq" />
 		<PackageReference Include="xunit" />

src/module-loader/dotnet/tests/MorganStanley.ComposeUI.ModuleLoader.Abstractions.Tests/StartupContextTests.cs

@@ -10,6 +10,7 @@
 // or implied. See the License for the specific language governing permissions
 // and limitations under the License.
 
+using FluentAssertions;
 using Moq;
 
 namespace MorganStanley.ComposeUI.ModuleLoader.Abstractions.Tests;
@@ -25,20 +26,21 @@ public void WhenAdd_AddedValuesCanBeRetrieved()
             new MyContextInfo { Name = "Test2" }
         };
 
-        StartupContext context = new StartupContext(new StartRequest("test"), Mock.Of<IModuleInstance>());
+        var context = new StartupContext(new StartRequest("test"), Mock.Of<IModuleInstance>());
         context.AddProperty(expected[0]);
         context.AddProperty(expected[1]);
 
         var result = context.GetProperties();
-        Assert.NotNull(result);
-        Assert.Equal(expected, result);
+        result.Should().NotBeNull();
+        result.Should().BeEquivalentTo(expected);
     }
 
     [Fact]
     public void GivenNullArgument_WhenAdd_ThrowsArgumentNullException()
     {
-        StartupContext context = new StartupContext(new StartRequest("test"), Mock.Of<IModuleInstance>());
-        Assert.Throws<ArgumentNullException>(() => context.AddProperty<object>(null!));
+        var context = new StartupContext(new StartRequest("test"), Mock.Of<IModuleInstance>());
+        var action = () => context.AddProperty<object>(null!);
+        action.Should().Throw<ArgumentNullException>();
     }
 
     private class MyContextInfo