Fix spurious duplicate activity registry warnings during startup (#7183)
* Initial plan * Fix: Remove bogus duplicate activity registry warnings during refresh The ActivityRegistry.RefreshDescriptorsAsync() method was starting with existing descriptors, causing false warnings when intentionally repopulating the registry. Now starts fresh but preserves manual descriptors. Co-authored-by: KnibbsyMan <23156317+KnibbsyMan@users.noreply.github.com> * Add tests for ActivityRegistry refresh behavior Added comprehensive tests to verify: 1. No warnings logged during intentional registry refresh 2. Manual descriptors preserved during refresh 3. True duplicates from different providers still logged Co-authored-by: KnibbsyMan <23156317+KnibbsyMan@users.noreply.github.com> * Apply code review feedback: track manual descriptors in providersDictionary and clarify comments - Track _manualActivityDescriptors in providersDictionary under GetType() key for consistency - Clarify comments to be more specific about GetType() provider and RegisterAsync(Type activityType) method - Avoid duplicate entries in provider list when descriptor already preserved Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Minor style improvement: use Count > 0 instead of !Count.Equals(0) Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: KnibbsyMan <23156317+KnibbsyMan@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
This commit is contained in:
Copilot
GitHub
parent
ad87ab96fd
commit
40a7583f62
|
|
@ -75,7 +75,41 @@ public class ActivityRegistry(IActivityDescriber activityDescriber, IEnumerable<
|
|||
public async Task RefreshDescriptorsAsync(IEnumerable<IActivityProvider> activityProviders, CancellationToken cancellationToken = default)
|
||||
{
|
||||
var providersDictionary = new ConcurrentDictionary<Type, ICollection<ActivityDescriptor>>();
|
||||
var activityDescriptors = new ConcurrentDictionary<(string Type, int Version), ActivityDescriptor>(_activityDescriptors);
|
||||
var activityDescriptors = new ConcurrentDictionary<(string Type, int Version), ActivityDescriptor>();
|
||||
|
||||
// First, preserve manually registered descriptors from Register/RegisterAsync(Type) calls, which are stored under the ActivityRegistry type (GetType()) as the provider key, without logging warnings (since we're starting fresh).
|
||||
if (_providedActivityDescriptors.TryGetValue(GetType(), out var manualDescriptors))
|
||||
{
|
||||
var preservedManualDescriptors = new List<ActivityDescriptor>();
|
||||
providersDictionary[GetType()] = preservedManualDescriptors;
|
||||
|
||||
foreach (var manualDescriptor in manualDescriptors)
|
||||
{
|
||||
activityDescriptors[(manualDescriptor.TypeName, manualDescriptor.Version)] = manualDescriptor;
|
||||
preservedManualDescriptors.Add(manualDescriptor);
|
||||
}
|
||||
}
|
||||
|
||||
// Also add descriptors from _manualActivityDescriptors (from RegisterAsync(Type activityType) calls).
|
||||
// These should also be tracked under the GetType() provider key to keep providersDictionary consistent.
|
||||
if (_manualActivityDescriptors.Count > 0)
|
||||
{
|
||||
if (!providersDictionary.TryGetValue(GetType(), out var manualProviderDescriptors))
|
||||
{
|
||||
manualProviderDescriptors = new List<ActivityDescriptor>();
|
||||
providersDictionary[GetType()] = manualProviderDescriptors;
|
||||
}
|
||||
|
||||
foreach (var manualDescriptor in _manualActivityDescriptors)
|
||||
{
|
||||
activityDescriptors[(manualDescriptor.TypeName, manualDescriptor.Version)] = manualDescriptor;
|
||||
|
||||
// Avoid adding duplicates to the provider list if the descriptor was already preserved.
|
||||
if (!manualProviderDescriptors.Contains(manualDescriptor))
|
||||
manualProviderDescriptors.Add(manualDescriptor);
|
||||
}
|
||||
}
|
||||
|
||||
foreach (var activityProvider in activityProviders)
|
||||
{
|
||||
var descriptors = (await activityProvider.GetDescriptorsAsync(cancellationToken)).ToList();
|
||||
|
|
|
|||
|
|
@ -0,0 +1,187 @@
|
|||
using Elsa.Workflows;
|
||||
using Elsa.Workflows.Models;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Moq;
|
||||
|
||||
namespace Elsa.Workflows.Core.UnitTests.Services;
|
||||
|
||||
public class ActivityRegistryTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task RefreshDescriptorsAsync_CalledTwice_DoesNotLogWarnings()
|
||||
{
|
||||
// Arrange
|
||||
var mockLogger = new Mock<ILogger<ActivityRegistry>>();
|
||||
var mockActivityDescriber = new Mock<IActivityDescriber>();
|
||||
var modifiers = Array.Empty<IActivityDescriptorModifier>();
|
||||
|
||||
var registry = new ActivityRegistry(mockActivityDescriber.Object, modifiers, mockLogger.Object);
|
||||
|
||||
var mockProvider = new Mock<IActivityProvider>();
|
||||
var descriptor1 = new ActivityDescriptor
|
||||
{
|
||||
TypeName = "TestActivity1",
|
||||
Version = 1,
|
||||
Kind = ActivityKind.Task,
|
||||
Category = "Test",
|
||||
Description = "Test Activity 1",
|
||||
IsBrowsable = true
|
||||
};
|
||||
|
||||
var descriptor2 = new ActivityDescriptor
|
||||
{
|
||||
TypeName = "TestActivity2",
|
||||
Version = 1,
|
||||
Kind = ActivityKind.Task,
|
||||
Category = "Test",
|
||||
Description = "Test Activity 2",
|
||||
IsBrowsable = true
|
||||
};
|
||||
|
||||
mockProvider.Setup(p => p.GetDescriptorsAsync(It.IsAny<CancellationToken>()))
|
||||
.ReturnsAsync(new[] { descriptor1, descriptor2 });
|
||||
|
||||
var providers = new[] { mockProvider.Object };
|
||||
|
||||
// Act - First refresh
|
||||
await registry.RefreshDescriptorsAsync(providers);
|
||||
|
||||
// Act - Second refresh (simulates the intentional repopulation in DefaultRegistriesPopulator)
|
||||
await registry.RefreshDescriptorsAsync(providers);
|
||||
|
||||
// Assert - Verify no warning logs were made
|
||||
mockLogger.Verify(
|
||||
x => x.Log(
|
||||
LogLevel.Warning,
|
||||
It.IsAny<EventId>(),
|
||||
It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("was already registered")),
|
||||
It.IsAny<Exception>(),
|
||||
It.IsAny<Func<It.IsAnyType, Exception?, string>>()),
|
||||
Times.Never,
|
||||
"Expected no duplicate registration warnings during refresh");
|
||||
|
||||
// Verify descriptors are still registered
|
||||
var allDescriptors = registry.ListAll().ToList();
|
||||
Assert.Equal(2, allDescriptors.Count);
|
||||
Assert.Contains(allDescriptors, d => d.TypeName == "TestActivity1");
|
||||
Assert.Contains(allDescriptors, d => d.TypeName == "TestActivity2");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task RefreshDescriptorsAsync_PreservesManualDescriptors()
|
||||
{
|
||||
// Arrange
|
||||
var mockLogger = new Mock<ILogger<ActivityRegistry>>();
|
||||
var mockActivityDescriber = new Mock<IActivityDescriber>();
|
||||
var modifiers = Array.Empty<IActivityDescriptorModifier>();
|
||||
|
||||
var registry = new ActivityRegistry(mockActivityDescriber.Object, modifiers, mockLogger.Object);
|
||||
|
||||
// Create a manual descriptor
|
||||
var manualDescriptor = new ActivityDescriptor
|
||||
{
|
||||
TypeName = "ManualActivity",
|
||||
Version = 1,
|
||||
Kind = ActivityKind.Task,
|
||||
Category = "Manual",
|
||||
Description = "Manually registered activity",
|
||||
IsBrowsable = true
|
||||
};
|
||||
|
||||
registry.Register(manualDescriptor);
|
||||
|
||||
// Create a provider descriptor
|
||||
var mockProvider = new Mock<IActivityProvider>();
|
||||
var providerDescriptor = new ActivityDescriptor
|
||||
{
|
||||
TypeName = "ProviderActivity",
|
||||
Version = 1,
|
||||
Kind = ActivityKind.Task,
|
||||
Category = "Provider",
|
||||
Description = "Provider activity",
|
||||
IsBrowsable = true
|
||||
};
|
||||
|
||||
mockProvider.Setup(p => p.GetDescriptorsAsync(It.IsAny<CancellationToken>()))
|
||||
.ReturnsAsync(new[] { providerDescriptor });
|
||||
|
||||
var providers = new[] { mockProvider.Object };
|
||||
|
||||
// Act - Refresh with provider
|
||||
await registry.RefreshDescriptorsAsync(providers);
|
||||
|
||||
// Assert - Both manual and provider descriptors should be present
|
||||
var allDescriptors = registry.ListAll().ToList();
|
||||
Assert.Equal(2, allDescriptors.Count);
|
||||
Assert.Contains(allDescriptors, d => d.TypeName == "ManualActivity");
|
||||
Assert.Contains(allDescriptors, d => d.TypeName == "ProviderActivity");
|
||||
|
||||
// Verify no warnings about manual descriptor being replaced
|
||||
mockLogger.Verify(
|
||||
x => x.Log(
|
||||
LogLevel.Warning,
|
||||
It.IsAny<EventId>(),
|
||||
It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("ManualActivity")),
|
||||
It.IsAny<Exception>(),
|
||||
It.IsAny<Func<It.IsAnyType, Exception?, string>>()),
|
||||
Times.Never,
|
||||
"Expected no warnings about manual descriptor");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task RefreshDescriptorsAsync_LogsWarning_WhenDifferentProvidersRegisterSameActivity()
|
||||
{
|
||||
// Arrange
|
||||
var mockLogger = new Mock<ILogger<ActivityRegistry>>();
|
||||
var mockActivityDescriber = new Mock<IActivityDescriber>();
|
||||
var modifiers = Array.Empty<IActivityDescriptorModifier>();
|
||||
|
||||
var registry = new ActivityRegistry(mockActivityDescriber.Object, modifiers, mockLogger.Object);
|
||||
|
||||
// Create two different providers with the same activity
|
||||
var mockProvider1 = new Mock<IActivityProvider>();
|
||||
var mockProvider2 = new Mock<IActivityProvider>();
|
||||
|
||||
var descriptor1 = new ActivityDescriptor
|
||||
{
|
||||
TypeName = "DuplicateActivity",
|
||||
Version = 1,
|
||||
Kind = ActivityKind.Task,
|
||||
Category = "Test",
|
||||
Description = "From Provider 1",
|
||||
IsBrowsable = true
|
||||
};
|
||||
|
||||
var descriptor2 = new ActivityDescriptor
|
||||
{
|
||||
TypeName = "DuplicateActivity",
|
||||
Version = 1,
|
||||
Kind = ActivityKind.Task,
|
||||
Category = "Test",
|
||||
Description = "From Provider 2",
|
||||
IsBrowsable = true
|
||||
};
|
||||
|
||||
mockProvider1.Setup(p => p.GetDescriptorsAsync(It.IsAny<CancellationToken>()))
|
||||
.ReturnsAsync(new[] { descriptor1 });
|
||||
|
||||
mockProvider2.Setup(p => p.GetDescriptorsAsync(It.IsAny<CancellationToken>()))
|
||||
.ReturnsAsync(new[] { descriptor2 });
|
||||
|
||||
var providers = new[] { mockProvider1.Object, mockProvider2.Object };
|
||||
|
||||
// Act
|
||||
await registry.RefreshDescriptorsAsync(providers);
|
||||
|
||||
// Assert - Should log a warning for the duplicate
|
||||
mockLogger.Verify(
|
||||
x => x.Log(
|
||||
LogLevel.Warning,
|
||||
It.IsAny<EventId>(),
|
||||
It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("DuplicateActivity") && v.ToString()!.Contains("was already registered")),
|
||||
It.IsAny<Exception>(),
|
||||
It.IsAny<Func<It.IsAnyType, Exception?, string>>()),
|
||||
Times.Once,
|
||||
"Expected one warning for duplicate activity from different providers");
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue