Skip to content

fix(mongodb): preserve and resolve tenant-agnostic ("*") entities#155

Open
arledesma wants to merge 2 commits into
elsa-workflows:mainfrom
arledesma:fix/mongodb-agnostic-tenant-visibility
Open

fix(mongodb): preserve and resolve tenant-agnostic ("*") entities#155
arledesma wants to merge 2 commits into
elsa-workflows:mainfrom
arledesma:fix/mongodb-agnostic-tenant-visibility

Conversation

@arledesma

Copy link
Copy Markdown

The MongoDb store overwrote the tenant-agnostic marker on write and excluded agnostic rows on read, so tenant-agnostic entities (e.g. CLR workflow definitions stamped by ClrWorkflowsProvider with "*") were persisted with a tenant-specific id and became unreachable when resolved under a specific tenant, throwing WorkflowDefinitionNotFoundException.

Bring MongoDbStore to parity with the EFCore provider:

  • ApplyTenantId: don't overwrite TenantId when it is already "*"
  • GetQueryableCollection: include TenantId == "*" in the tenant filter

Behavior change / migration

This is an internal store fix (private methods only, no public API change), but it does change observable behavior to match the EFCore provider and the documented tenant-agnostic ("*") contract:

  • Read: queries under a specific tenant now also return TenantId == "*" (global) rows, instead of strict TenantId == current only.
  • Write: ApplyTenantId no longer overwrites an entity whose TenantId is already "*", so agnostic entities (e.g. CLR workflow definitions) stay agnostic instead of being stamped with the ambient tenant.

Migration note for existing MongoDb deployments. Because of the previous behavior, agnostic entities were already persisted with TenantId = null (the marker was clobbered on write). The new read filter adds || TenantId == "*"; it does not match those legacy null rows. As a result:

  • New writes after this change are correct ("*") and resolve under any tenant.
  • Pre-existing agnostic rows remain null and stay invisible under a specific tenant until corrected.

To fix existing data, set TenantId = "*" on the affected documents, for example for workflow definitions:

js db.workflow_definitions.updateMany( { TenantId: null }, { $set: { TenantId: "*" } } ) ​

Alternatively, re-run workflow definition population so the agnostic marker is written under the corrected logic. Deployments that never used multitenancy, or that started fresh on this version, need no migration.

Refs #158

The MongoDb store overwrote the tenant-agnostic marker on write and
excluded agnostic rows on read, so tenant-agnostic entities (e.g. CLR
workflow definitions stamped by ClrWorkflowsProvider with "*") were
persisted with a tenant-specific id and became unreachable when resolved
under a specific tenant, throwing WorkflowDefinitionNotFoundException.

Bring MongoDbStore to parity with the EFCore provider:
- ApplyTenantId: don't overwrite TenantId when it is already "*"
- GetQueryableCollection: include TenantId == "*" in the tenant filter

Refs elsa-workflows/elsa-core#7691
@arledesma

Copy link
Copy Markdown
Author

@dotnet-policy-service agree company="ContraForce"

@sfmskywalker

Copy link
Copy Markdown
Member

@greptile review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MongoDB multitenancy behavior so tenant-agnostic ("*") entities are preserved on write and resolvable from within a specific tenant on read, aligning MongoDB persistence behavior with the EFCore provider and the documented tenant-agnostic contract.

Changes:

  • Update tenant-scoped query filtering to also include tenant-agnostic (TenantId == "*") documents.
  • Prevent ApplyTenantId from overwriting documents already marked as tenant-agnostic ("*"), preserving the marker during inserts/updates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +439 to +442
var tenantId = tenant?.Id.EmptyToNull();
queryable = queryable.Where(x => (x as Entity)!.TenantId == tenantId);
// Include tenant-agnostic ("*") rows so global entities (e.g. CLR workflow
// definitions) remain visible under a specific tenant, matching the EFCore provider.
queryable = queryable.Where(x => (x as Entity)!.TenantId == tenantId || (x as Entity)!.TenantId == Tenant.AgnosticTenantId);
@greptile-apps

greptile-apps Bot commented Jun 3, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes MongoDB multi-tenancy handling to align with the EFCore provider's treatment of tenant-agnostic ("*") entities. The changes are confined to two private methods in MongoDbStore<TDocument>.

  • Write path (ApplyTenantId): entities already stamped with "*" are no longer overwritten with the ambient tenant ID, so CLR-registered global workflow definitions keep their agnostic marker.
  • Read path (GetQueryableCollection): tenant-scoped queries now include TenantId == "*" via a new includeTenantAgnostic parameter (defaulting to true), making global entities visible under any specific tenant.
  • Delete path (DeleteWhereAsync): both terminal delete overloads explicitly pass includeTenantAgnostic: false, ensuring a tenant-scoped delete predicate cannot match and destroy globally-shared entities. The second commit in this PR (362c5d1) directly addresses the concern raised in the prior review about deletes inadvertently removing "*" entities.

Confidence Score: 5/5

Safe to merge — the two private methods are changed consistently, all delete overloads protect global entities, and the fix matches documented EFCore provider behaviour.

The write fix, read fix, and delete guard are all internally consistent. The second commit in this PR directly addresses the delete-path concern raised in the prior review. Every DeleteWhereAsync call chain routes through one of the two patched terminal overloads, so no delete path regressed. Read and count operations correctly include '*' entities, keeping counts in sync with result sets.

No files require special attention.

Important Files Changed

Filename Overview
src/modules/persistence/Elsa.Persistence.MongoDb/Common/MongoDbStore.cs Three targeted fixes: ApplyTenantId now skips stamping entities already marked ""; GetQueryableCollection includes TenantId == "" in tenant-scoped reads (via new includeTenantAgnostic param, default true); both DeleteWhereAsync implementations explicitly pass includeTenantAgnostic: false so global entities cannot be destroyed by a tenant-scoped delete. All delete call chains route through the two patched overloads.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[GetQueryableCollection called] --> B{tenantAgnostic == true?}
    B -- Yes --> C[Return unfiltered queryable\nall tenants + all '*' entities]
    B -- No --> D{TDocument is Entity?}
    D -- No --> E[Return unfiltered queryable]
    D -- Yes --> F{includeTenantAgnostic?}
    F -- true\ndefault reads --> G["Filter: TenantId == tenantId OR TenantId == '*'"]
    F -- false\ndeletes --> H["Filter: TenantId == tenantId only"]

    subgraph Callers
        I[FindAsync / FindManyAsync / CountAsync / AnyAsync / ListAsync] --> |includeTenantAgnostic=true| F
        J[DeleteWhereAsync both overloads] --> |includeTenantAgnostic=false| F
    end

    subgraph Write path
        K[ApplyTenantId] --> L{entity.TenantId == '*'?}
        L -- Yes --> M[Skip: preserve agnostic marker]
        L -- No --> N[Stamp with tenantId.EmptyToNull]
    end
Loading

Reviews (2): Last reviewed commit: "fix(mongodb): exclude tenant-agnostic ("..." | Re-trigger Greptile

…ed deletes

GetQueryableCollection is shared by the read and delete paths. The read fix
added an OR-branch for TenantId == "*", which also widened DeleteWhereAsync: a
tenant-scoped delete could match and remove a shared "*" entity for every
tenant (e.g. a CLR workflow definition deleted under one tenant).

Add an includeTenantAgnostic flag (default true). Reads keep including "*"
rows; both DeleteWhereAsync terminals pass includeTenantAgnostic: false so a
tenant-scoped delete stays strict to the current tenant, matching the Dapper
store. An explicit tenantAgnostic: true delete is unchanged, and the agnostic
context (Tenant.Id == "*") can still manage "*" entities.
@arledesma

Copy link
Copy Markdown
Author

Addressed the delete-path concern in 362c5d1.

GetQueryableCollection is shared by the read and delete paths, so the read-side || TenantId == "*" branch had also widened DeleteWhereAsync — a tenant-scoped delete could match and remove a shared "*" entity for every tenant.

Fix: added an includeTenantAgnostic flag (default true). Reads still include "*" rows, but both DeleteWhereAsync terminals now pass includeTenantAgnostic: false, so tenant-scoped deletes stay strict to the current tenant — matching the Dapper store. An explicit tenantAgnostic: true delete is unchanged, and the agnostic context (Tenant.Id == "*") can still manage "*" entities.

@sfmskywalker

Copy link
Copy Markdown
Member

@greptile review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants