Update multitenancy logic and improve `Result` handling: by sfmskywalker · Pull Request #7281 · elsa-workflows/elsa-core

sfmskywalker · 2026-02-10T20:20:18Z

Introduce TenantsOptions with IsEnabled flag to conditionally apply tenant-specific logic.
Refactor Result class to support strongly-typed operations and async handlers.
Implement tenant filters respecting multitenancy enablement.
Enhance error logging for workflow definition addition, upgrading error handling.
Refactor tests and storage drivers to use IsSuccess from Result.

- Introduce `TenantsOptions` with `IsEnabled` flag to conditionally apply tenant-specific logic. - Refactor `Result` class to support strongly-typed operations and async handlers. - Implement tenant filters respecting multitenancy enablement. - Enhance error logging for workflow definition addition, upgrading error handling. - Refactor tests and storage drivers to use `IsSuccess` from `Result`.

greptile-apps · 2026-02-10T20:24:13Z

Greptile Overview

Greptile Summary

This PR refactors the codebase to improve error handling and add conditional multitenancy support:

Key Changes:

Consolidated Result class into Elsa.Common.Models with strongly-typed generic support and async handlers (OnSuccessAsync, OnFailureAsync)
Added TenantsOptions.IsEnabled flag to conditionally enable/disable multitenancy features
Updated ApplyTenantId and SetTenantIdFilter entity handlers to respect the IsEnabled flag
Changed IWorkflowDefinitionStorePopulator.AddAsync to return Result<WorkflowDefinition> instead of WorkflowDefinition directly for better error handling
Added error logging in DefaultWorkflowDefinitionStorePopulator when workflow addition fails
Added backwards compatibility logic in SetTenantIdFilter to include records with null TenantId when context TenantId is empty string
Updated all existing usages of Result.Success to Result.IsSuccess throughout the codebase

Impact:
The IWorkflowDefinitionStorePopulator interface change is breaking and requires all implementations and callers to be updated.

Confidence Score: 3/5

This PR has a critical bug where DefaultWorkflowRegistry.RegisterAsync doesn't handle the new Result return type, causing errors to be silently swallowed
The PR introduces a breaking interface change that wasn't fully propagated to all callers. The DefaultWorkflowRegistry.RegisterAsync method still treats AddAsync as if it returns void, ignoring the new Result<WorkflowDefinition> return value. This means exceptions will be wrapped in the Result but never handled, causing silent failures.
Pay close attention to src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowRegistry.cs which has not been updated to handle the new Result return type

Important Files Changed

Filename	Overview
src/modules/Elsa.Common/Models/Result.cs	New strongly-typed Result monad implementation with async support. Clean implementation with proper generic type support and helper factory methods.
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/ApplyTenantId.cs	Added TenantsOptions dependency injection and conditional tenant ID application based on IsEnabled flag. Logic refactored with early returns for better readability.
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs	Added TenantsOptions check to conditionally apply tenant filters. Added backwards compatibility logic for null TenantId with empty context TenantId.
src/modules/Elsa.Workflows.Runtime/Contracts/IWorkflowDefinitionStorePopulator.cs	Changed AddAsync methods to return Result<WorkflowDefinition> instead of WorkflowDefinition directly. Breaking interface change that requires all callers to be updated.
src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs	Implemented Result return types for AddAsync methods with error handling and logging. Uses OnSuccess pattern to handle successful workflow additions and trigger indexing.

Sequence Diagram

sequenceDiagram
    participant Client
    participant PopulateStore as DefaultWorkflowDefinitionStorePopulator
    participant AddOrUpdate as AddOrUpdateAsync
    participant Store as WorkflowDefinitionStore
    participant TriggerIndexer

    Client->>PopulateStore: PopulateStoreAsync()
    loop For each workflow provider
        PopulateStore->>PopulateStore: Get workflows from provider
        loop For each workflow
            PopulateStore->>PopulateStore: Check tenant ID match
            alt Tenant matches or is agnostic
                PopulateStore->>PopulateStore: AddAsync(workflow)
                PopulateStore->>AddOrUpdate: AddOrUpdateAsync()
                alt Success
                    AddOrUpdate->>Store: SaveManyAsync()
                    AddOrUpdate-->>PopulateStore: Result.Success(definition)
                    PopulateStore->>PopulateStore: OnSuccess handler
                    alt Is published and indexTriggers=true
                        PopulateStore->>TriggerIndexer: IndexTriggersAsync()
                    end
                    PopulateStore->>PopulateStore: Add to workflowDefinitions list
                else Exception
                    AddOrUpdate-->>PopulateStore: Result.Failure(exception)
                    Note over PopulateStore: Error logged, workflow skipped
                end
            else Tenant mismatch
                Note over PopulateStore: Workflow skipped with debug log
            end
        end
    end
    PopulateStore-->>Client: List of WorkflowDefinition

greptile-apps

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-10T20:24:21Z

Additional Comments (1)

src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowRegistry.cs
The return value from AddAsync is now Result<WorkflowDefinition> but this code doesn't handle the result. The error will be silently swallowed if the workflow addition fails.

        var result = await populator.AddAsync(materializedWorkflow, cancellationToken);
        result.OnFailure(ex => throw ex);

Copilot

Pull request overview

This pull request refactors the Result class to provide strongly-typed error handling and introduces conditional multitenancy support. The changes move Result from Elsa.Expressions.Models to Elsa.Common.Models, making it generic (Result<T>) with async handler support, while maintaining backwards compatibility through inheritance. Additionally, a new TenantsOptions.IsEnabled flag allows conditional application of tenant-specific logic in EF Core entity handlers.

Changes:

Refactored Result to a generic Result<T> class with async handler methods (OnSuccessAsync, OnFailureAsync) and static factory methods (Success<T>, Failure<T>)
Renamed Success property to IsSuccess across all usages
Added TenantsOptions.IsEnabled flag to conditionally enable/disable multitenancy features
Enhanced error logging in workflow definition store population with rich contextual information
Updated EF Core entity handlers to respect multitenancy enablement flag and added backwards compatibility for null tenant IDs

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/modules/Elsa.Common/Models/Result.cs	New strongly-typed Result class with generic support, async handlers, and factory methods
src/modules/Elsa.Expressions/Models/Result.cs	Removed old non-generic Result class (moved to Elsa.Common)
src/modules/Elsa.Expressions/Helpers/ObjectConverter.cs	Updated to import Result from Elsa.Common.Models
src/modules/Elsa.Workflows.Runtime/Contracts/IWorkflowDefinitionStorePopulator.cs	Changed AddAsync methods to return Result instead of WorkflowDefinition
src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs	Implemented Result-based error handling with detailed logging and async handlers
src/modules/Elsa.Workflows.Core/VariableStorageDrivers/WorkflowInstanceStorageDriver.cs	Updated Success to IsSuccess property
src/modules/Elsa.Workflows.Core/Extensions/DictionaryExtensions.cs	Updated Success to IsSuccess and simplified code structure
src/modules/Elsa.Workflows.Api/Endpoints/WorkflowDefinitions/Execute/Models.cs	Updated Success to IsSuccess property
src/modules/Elsa.Tenants/Options/TenantsOptions.cs	Added IsEnabled property to control multitenancy features
src/modules/Elsa.Tenants/Features/TenantsFeature.cs	Sets IsEnabled to true when TenantsFeature is applied
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs	Conditionally applies tenant filters based on IsEnabled flag, added backwards compatibility for null TenantId
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/ApplyTenantId.cs	Conditionally applies tenant IDs based on IsEnabled flag
test/unit/Elsa.Workflows.Core.UnitTests/ObjectConversion/Tests.cs	Updated test assertions from Success to IsSuccess

src/modules/Elsa.Common/Models/Result.cs

src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs

…initionStorePopulator.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…oduce `ValueOrDefault` property for better null support.

…y' into bug/tenancy-backward-compatibilty

…d use it in `DefaultWorkflowRegistry` to ensure operation success.

sfmskywalker · 2026-02-10T20:37:17Z

Greptile Overview

Greptile Summary

This PR refactors the codebase to improve error handling and add conditional multitenancy support:

Key Changes:

Consolidated Result class into Elsa.Common.Models with strongly-typed generic support and async handlers (OnSuccessAsync, OnFailureAsync)

Added TenantsOptions.IsEnabled flag to conditionally enable/disable multitenancy features

Updated ApplyTenantId and SetTenantIdFilter entity handlers to respect the IsEnabled flag

Changed IWorkflowDefinitionStorePopulator.AddAsync to return Result<WorkflowDefinition> instead of WorkflowDefinition directly for better error handling

Added error logging in DefaultWorkflowDefinitionStorePopulator when workflow addition fails

Added backwards compatibility logic in SetTenantIdFilter to include records with null TenantId when context TenantId is empty string

Updated all existing usages of Result.Success to Result.IsSuccess throughout the codebase

Impact: The IWorkflowDefinitionStorePopulator interface change is breaking and requires all implementations and callers to be updated.

Confidence Score: 3/5

This PR has a critical bug where DefaultWorkflowRegistry.RegisterAsync doesn't handle the new Result return type, causing errors to be silently swallowed

The PR introduces a breaking interface change that wasn't fully propagated to all callers. The DefaultWorkflowRegistry.RegisterAsync method still treats AddAsync as if it returns void, ignoring the new Result<WorkflowDefinition> return value. This means exceptions will be wrapped in the Result but never handled, causing silent failures.

Pay close attention to src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowRegistry.cs which has not been updated to handle the new Result return type

Important Files Changed

Filename Overview
src/modules/Elsa.Common/Models/Result.cs New strongly-typed Result monad implementation with async support. Clean implementation with proper generic type support and helper factory methods.
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/ApplyTenantId.cs Added TenantsOptions dependency injection and conditional tenant ID application based on IsEnabled flag. Logic refactored with early returns for better readability.
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs Added TenantsOptions check to conditionally apply tenant filters. Added backwards compatibility logic for null TenantId with empty context TenantId.
src/modules/Elsa.Workflows.Runtime/Contracts/IWorkflowDefinitionStorePopulator.cs Changed AddAsync methods to return Result instead of WorkflowDefinition directly. Breaking interface change that requires all callers to be updated.
src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs Implemented Result return types for AddAsync methods with error handling and logging. Uses OnSuccess pattern to handle successful workflow additions and trigger indexing.

Sequence Diagram
sequenceDiagram
    participant Client
    participant PopulateStore as DefaultWorkflowDefinitionStorePopulator
    participant AddOrUpdate as AddOrUpdateAsync
    participant Store as WorkflowDefinitionStore
    participant TriggerIndexer

    Client->>PopulateStore: PopulateStoreAsync()
    loop For each workflow provider
        PopulateStore->>PopulateStore: Get workflows from provider
        loop For each workflow
            PopulateStore->>PopulateStore: Check tenant ID match
            alt Tenant matches or is agnostic
                PopulateStore->>PopulateStore: AddAsync(workflow)
                PopulateStore->>AddOrUpdate: AddOrUpdateAsync()
                alt Success
                    AddOrUpdate->>Store: SaveManyAsync()
                    AddOrUpdate-->>PopulateStore: Result.Success(definition)
                    PopulateStore->>PopulateStore: OnSuccess handler
                    alt Is published and indexTriggers=true
                        PopulateStore->>TriggerIndexer: IndexTriggersAsync()
                    end
                    PopulateStore->>PopulateStore: Add to workflowDefinitions list
                else Exception
                    AddOrUpdate-->>PopulateStore: Result.Failure(exception)
                    Note over PopulateStore: Error logged, workflow skipped
                end
            else Tenant mismatch
                Note over PopulateStore: Workflow skipped with debug log
            end
        end
    end
    PopulateStore-->>Client: List of WorkflowDefinition
Loading

Fixed via fdda5b1

sfmskywalker · 2026-02-10T20:37:56Z

@greptile Please review my fixes based on your feedback.

greptile-apps · 2026-02-10T20:42:21Z

Greptile Overview

Greptile Summary

This PR refactors the Result monad pattern and introduces conditional multitenancy logic.

Key Changes:

Consolidated Result class into Elsa.Common.Models with strongly-typed generic support (Result<T>)
Added async handler methods (OnSuccessAsync, OnFailureAsync), ValueOrDefault property, and ThrowIfFailure() method
Introduced TenantsOptions.IsEnabled flag to conditionally enable/disable multitenancy features
Updated EF Core entity handlers (ApplyTenantId, SetTenantIdFilter) to respect the IsEnabled flag
Added backwards compatibility filter for null TenantId records when context TenantId is empty string
Refactored IWorkflowDefinitionStorePopulator.AddAsync() to return Result<WorkflowDefinition> with proper error logging
Updated all usages of Result.Success to Result.IsSuccess across the codebase

Benefits:

Improved error handling with structured Result pattern and detailed logging
More flexible multitenancy configuration with ability to disable when not needed
Better backwards compatibility for existing data without tenant IDs
Strongly-typed result handling reduces runtime errors

Confidence Score: 4/5

This PR is generally safe to merge with minor considerations for the multitenancy feature flag and backwards compatibility logic.
The refactoring is well-structured with proper error handling and logging. The Result<T> pattern is correctly implemented and all usages have been updated. However, the score is 4/5 due to: (1) the backwards compatibility filter logic in SetTenantIdFilter needs careful testing to ensure it doesn't inadvertently expose data across tenants, and (2) the multitenancy IsEnabled flag will affect existing deployments and should be verified to default correctly for backwards compatibility.
Pay close attention to src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs for the backwards compatibility filter logic to ensure no tenant data leakage.

Important Files Changed

Filename	Overview
src/modules/Elsa.Common/Models/Result.cs	Added new strongly-typed `Result<T>` monad with async handlers, `ValueOrDefault` property, and `ThrowIfFailure` method for improved error handling.
src/modules/Elsa.Tenants/Options/TenantsOptions.cs	Added `IsEnabled` flag to conditionally apply multitenancy logic throughout the system.
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/ApplyTenantId.cs	Added multitenancy enablement check to conditionally apply tenant IDs; refactored to early-return pattern.
src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs	Added multitenancy enablement check and backwards compatibility filter to include records with null `TenantId` when context `TenantId` is empty string.
src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs	Refactored to return `Result<WorkflowDefinition>`, added try-catch with error logging, and used `OnSuccess` handler in `PopulateStoreAsync`.

Sequence Diagram

sequenceDiagram
    participant Client
    participant DefaultWorkflowRegistry
    participant DefaultWorkflowDefinitionStorePopulator
    participant WorkflowDefinitionStore
    participant TriggerIndexer
    participant ApplyTenantId
    participant SetTenantIdFilter

    Client->>DefaultWorkflowRegistry: RegisterAsync(workflow)
    DefaultWorkflowRegistry->>DefaultWorkflowDefinitionStorePopulator: AddAsync(materializedWorkflow)
    
    Note over DefaultWorkflowDefinitionStorePopulator: Checks TenantsOptions.IsEnabled
    
    DefaultWorkflowDefinitionStorePopulator->>DefaultWorkflowDefinitionStorePopulator: AddOrUpdateAsync()
    
    alt Success
        DefaultWorkflowDefinitionStorePopulator->>WorkflowDefinitionStore: SaveManyAsync()
        WorkflowDefinitionStore->>ApplyTenantId: HandleAsync() [if IsEnabled]
        ApplyTenantId-->>WorkflowDefinitionStore: Apply TenantId
        WorkflowDefinitionStore->>SetTenantIdFilter: Handle() [if IsEnabled]
        SetTenantIdFilter-->>WorkflowDefinitionStore: Apply query filter
        WorkflowDefinitionStore-->>DefaultWorkflowDefinitionStorePopulator: WorkflowDefinition
        DefaultWorkflowDefinitionStorePopulator->>DefaultWorkflowDefinitionStorePopulator: Result.Success(workflowDefinition)
        DefaultWorkflowDefinitionStorePopulator->>TriggerIndexer: IndexTriggersAsync() [if published]
        DefaultWorkflowDefinitionStorePopulator-->>DefaultWorkflowRegistry: Result<WorkflowDefinition>
        DefaultWorkflowRegistry->>DefaultWorkflowRegistry: result.ThrowIfFailure()
        DefaultWorkflowRegistry-->>Client: Success
    else Failure
        DefaultWorkflowDefinitionStorePopulator->>DefaultWorkflowDefinitionStorePopulator: Log error
        DefaultWorkflowDefinitionStorePopulator->>DefaultWorkflowDefinitionStorePopulator: Result.Failure(exception)
        DefaultWorkflowDefinitionStorePopulator-->>DefaultWorkflowRegistry: Result<WorkflowDefinition>
        DefaultWorkflowRegistry->>DefaultWorkflowRegistry: result.ThrowIfFailure()
        DefaultWorkflowRegistry-->>Client: Throws exception
    end

greptile-apps

_{5 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs

j03y-nxxbz

Backwards compatibility issue seems resolved.
Improvement of error handling is clear and adds value.

…ltWorkflowDefinitionStorePopulator`.

…izeTenantId()` in `DefaultWorkflowDefinitionStorePopulatorTests`.

sfmskywalker requested a review from Copilot February 10, 2026 20:20

Copilot started reviewing on behalf of sfmskywalker February 10, 2026 20:21 View session

greptile-apps bot reviewed Feb 10, 2026

View reviewed changes

Copilot AI reviewed Feb 10, 2026

View reviewed changes

src/modules/Elsa.Common/Models/Result.cs Outdated Show resolved Hide resolved

src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDefinitionStorePopulator.cs Outdated Show resolved Hide resolved

sfmskywalker and others added 4 commits February 10, 2026 21:26

Update src/modules/Elsa.Workflows.Runtime/Services/DefaultWorkflowDef…

0c5f84f

…initionStorePopulator.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Add exception handling when accessing Value on failed results; intr…

a1d3f03

…oduce `ValueOrDefault` property for better null support.

Merge remote-tracking branch 'origin/bug/tenancy-backward-compatibilt…

bc42d48

…y' into bug/tenancy-backward-compatibilty

Add ThrowIfFailure method to handle exceptions in Result model an…

fdda5b1

…d use it in `DefaultWorkflowRegistry` to ensure operation success.

greptile-apps bot reviewed Feb 10, 2026

View reviewed changes

src/modules/Elsa.Persistence.EFCore.Common/EntityHandlers/SetTenantIdFilter.cs Show resolved Hide resolved

j03y-nxxbz approved these changes Feb 11, 2026

View reviewed changes

sfmskywalker added 2 commits February 11, 2026 09:33

Normalize tenant ID handling by using NormalizeTenantId() in `Defau…

ff164ac

…ltWorkflowDefinitionStorePopulator`.

Normalize tenant ID usage in unit tests by returning `tenantId.Normal…

3cc51e8

…izeTenantId()` in `DefaultWorkflowDefinitionStorePopulatorTests`.

sfmskywalker merged commit 1b8083a into release/3.6.0 Feb 11, 2026
3 checks passed

sfmskywalker deleted the bug/tenancy-backward-compatibilty branch February 11, 2026 09:11

This was referenced Mar 5, 2026

Bump the elsa group with 4 updates futugyou/CodeFragments#1324

Merged

Bump the all-nuget group with 19 updates bc-solutions-coder/Foundry#38

Open

Conversation

sfmskywalker commented Feb 10, 2026

Uh oh!

greptile-apps bot commented Feb 10, 2026

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot commented Feb 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

sfmskywalker commented Feb 10, 2026

Greptile Overview

Greptile Summary

Confidence Score: 3/5

Important Files Changed

Sequence Diagram

Uh oh!

sfmskywalker commented Feb 10, 2026

Uh oh!

greptile-apps bot commented Feb 10, 2026

Greptile Overview

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j03y-nxxbz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants