Optimize ActivityRegistry.Find to prefer tenant-specific descriptors without performance regression#7227
Conversation
… single-pass iteration Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes the ActivityRegistry.Find(string type) method to correctly prefer tenant-specific activity descriptors over tenant-agnostic ones without introducing performance regression. The original implementation used LINQ with MaxBy which could incorrectly select a higher-version tenant-agnostic descriptor over a lower-version tenant-specific one.
Changes:
- Replaced LINQ-based lookup with single-pass iteration that separately tracks tenant-specific and tenant-agnostic descriptors
- Added explicit preference logic that returns tenant-specific descriptors when available, falling back to tenant-agnostic
- Maintained O(n) performance while fixing the tenant resolution logic
| if (descriptor.TenantId == tenantId) | ||
| { | ||
| if (tenantSpecific == null || descriptor.Version > tenantSpecific.Version) | ||
| tenantSpecific = descriptor; | ||
| } | ||
| else if (descriptor.TenantId == null) | ||
| { | ||
| if (tenantAgnostic == null || descriptor.Version > tenantAgnostic.Version) | ||
| tenantAgnostic = descriptor; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
| if (descriptor.TenantId == tenantId) | |
| { | |
| if (tenantSpecific == null || descriptor.Version > tenantSpecific.Version) | |
| tenantSpecific = descriptor; | |
| } | |
| else if (descriptor.TenantId == null) | |
| { | |
| if (tenantAgnostic == null || descriptor.Version > tenantAgnostic.Version) | |
| tenantAgnostic = descriptor; | |
| } | |
| if (descriptor.TenantId == tenantId && (tenantSpecific == null || descriptor.Version > tenantSpecific.Version)) | |
| tenantSpecific = descriptor; | |
| else if (descriptor.TenantId == null && (tenantAgnostic == null || descriptor.Version > tenantAgnostic.Version)) | |
| tenantAgnostic = descriptor; |
There was a problem hiding this comment.
Applied in commit 203e2ef. The nested if statements have been combined into single-line conditionals for improved readability.
|
@copilot apply changes based on the comments in this thread |
…it tests Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Applied all review feedback in commit 203e2ef:
|
sfmskywalker
left a comment
There was a problem hiding this comment.
Optimize the code that was created for DRYness using theories and extracting private helpers if and where appropriate.
|
@copilot Optimize the code that was created for DRYness using theories and extracting private helpers if and where appropriate. |
Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Refactored in commit b1bd4fa. Changes include:
|
Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Already completed in commits b1bd4fa and 70d096f. The test code has been optimized with:
|
55460bd
into
enh/tenant-agnostic-entities
* Add ADR for Null Tenant ID, implement tenant-agnostic logic Introduce ADR-0009 to document the use of `null` for tenant-agnostic entities, enhancing multitenancy handling. Update multitenancy features across the codebase, including EF Core query filters and ActivityRegistry, to handle null as a tenant ID, ensuring tenant-agnostic entities are accessible across all tenants. * Add multitenancy support in `ActivityTestFixture` by registering `ITenantAccessor`. * Update src/modules/Elsa.Common/Multitenancy/Implementations/DefaultTenantAccessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Remove unused `using Elsa.Common.Multitenancy;` from WorkflowDefinitionActivityDescriptorFactory (#7230) * Initial plan * Remove unused using Elsa.Common.Multitenancy statement 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: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Optimize ActivityRegistry.Find to prefer tenant-specific descriptors without performance regression (#7227) * Initial plan * Optimize Find(string type) to prefer tenant-specific descriptors with single-pass iteration Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Apply review feedback: combine if statements and add comprehensive unit tests Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Refactor tests for DRYness using theories and helper methods Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Clean up extra whitespace in test file 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: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Fix default tenant data visibility leak by removing NullIfEmpty conversion (#7229) * Initial plan * Remove NullIfEmpty conversion to align with ADR-0008 and ADR-0009 - Updated ElsaDbContextBase to use empty string for default tenant - Updated ApplyTenantId to stop converting empty string to null - Updated TenantAwareDbContextFactory to preserve empty string for default tenant - Updated Store.cs to preserve empty string for default tenant - This ensures: null = tenant-agnostic (visible to all), "" = default tenant Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Add database migration to convert null TenantId to empty string for SqlServer - Added Management migration to convert null to "" for WorkflowDefinitions and WorkflowInstances - Added Runtime migration to convert null to "" for all runtime entities - This ensures existing default tenant data is properly migrated per ADR-0008 - Note: Similar migrations needed for PostgreSql, MySql, Sqlite, and Oracle providers Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> * Clarify tenant handling logic in `ElsaDbContextBase` with new ADR references. * Add tenant ID convention analysis documents and quick reference guide * Implement tenant-agnostic functionality across modules - Introduce `AgnosticTenantId` constant to manage tenant-agnostic entities. - Modify entity handling logic to respect tenant-agnostic designations. - Adjust workflow processing to include tenant-agnostic workflows. - Update caching and activity descriptor logic to accommodate the `AgnosticTenantId`. * Refactor tenant management and registry logic in `ActivityRegistry` for improved clarity and separation of tenant-specific and tenant-agnostic activity descriptors. Remove `TestTenantResolver` and update workflow definition handling for tenant support. * Refactor `ActivityRegistry`: prioritize tenant-specific descriptors over tenant-agnostic and simplify descriptor retrieval logic. * Improve async handling in `CommandHandlerInvokerMiddleware` to await tasks without blocking * Update ADR to use asterisk as sentinel value for tenant-agnostic entities Replace the previous convention of using `null` for tenant-agnostic entities with an asterisk (`"*"`) for improved clarity and system architecture. Updated ADR documentation, TOC, and dependency graph accordingly. * Remove migration `ConvertNullTenantIdToEmptyString` and its associated designer file to clean up the codebase. * Refactor `ActivityRegistry`: streamline activity descriptor removal logic and simplify tenant ID checks. * Update Elsa.sln Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Simplify `RefreshDescriptorsAsync` by removing unnecessary local variable `currentTenantId`. * Remove unused `currentTenantId` variable from `ActivityRegistry`. * Add detailed semantic flow and key points to ADR 0009 Document the tenant ID flow from entity creation to query, emphasizing normalization and tenant-agnostic workflows. Update semantic flow diagrams and provide testing considerations for preserving `"*"` values in multi-tenant scenarios. * Remove outdated Tenant ID Analysis and associated documents * Add security-by-default design for tenant-agnostic entities in ADR Enhance Architecture Decision Record to detail explicit requirements for tenant-agnostic database entities, highlighting differences between in-memory activity descriptors and persistent entities. Emphasize importance of setting `TenantId = "*"` to prevent accidental data leakage. * Normalize tenant ID grouping in `ActivityRegistry` to unify null and agnostic IDs, reducing redundant processing. * Refactor `SignalManager`: improve timeout handling and streamline signal task cancellation. * Update src/modules/Elsa.Workflows.Core/Models/TenantRegistryData.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update src/modules/Elsa.Workflows.Core/Services/ActivityRegistry.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refactor tests to use `Tenant.AgnosticTenantId` instead of `null` for tenant-agnostic descriptors. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com> Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Enhance logging in recurring tasks: add error handling and logger support to prevent crashes in scheduled timers. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Find(string type)method to avoid performance regression for single-tenant/no-tenancy installationsImplementation Summary
Successfully optimized
ActivityRegistry.Find(string type)to address both performance and correctness concerns:Key Improvements
Test Refactoring
Optimized unit tests for DRYness:
CreateDescriptor(),RegisterDescriptors(),AssertDescriptor()TestActivityTypeandCurrentTenantto avoid string duplication[Theory]with multiple[InlineData]setsValidation
Find(type, version)method💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.