Fix ActivityNodeId column truncation for deeply nested workflows#7338
Conversation
…ncation for deeply nested workflows Add HasMaxLength(1024) for ActivityNodeId on ActivityExecutionRecord and WorkflowExecutionLogRecord entities. Create V3_7 migrations for all 5 database providers (SqlServer, MySql, PostgreSql, Sqlite, Oracle). Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
…odeId Update V3_6 migrations to change ActivityNodeId from fixed-length columns to unlimited types: nvarchar(max) for SQL Server, longtext for MySQL, NCLOB for Oracle. PostgreSQL (text) and SQLite (TEXT) already used unlimited types. Drop ActivityNodeId indexes on both ActivityExecutionRecords and WorkflowExecutionLogRecords tables since unlimited columns cannot be indexed. Remove HasIndex for ActivityNodeId from Configurations.cs. Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Greptile SummaryThis PR fixes a truncation error on Key concerns found during review:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| src/modules/Elsa.Persistence.EFCore/Modules/Runtime/Configurations.cs | Removes HasIndex on ActivityNodeId for both ActivityExecutionRecord and WorkflowExecutionLogRecord — correctly reflects that unlimited-length columns cannot carry a B-tree index. |
| src/modules/Elsa.Persistence.EFCore.SqlServer/Migrations/Runtime/20251204150326_V3_6.cs | Widens ActivityNodeId to nvarchar(max) and drops its index; however the DropIndex calls have no IF EXISTS guard (unlike the existing Trigger-index pattern in the same file), and the Down() migration narrows back to nvarchar(450) which will truncate or fail for long values. The migration also mutates the existing V3_6 file instead of being a new incremental migration. |
| src/modules/Elsa.Persistence.EFCore.MySql/Migrations/Runtime/20251204150235_V3_6.cs | Widens ActivityNodeId to longtext and drops its index; Down() narrows back to varchar(255) risking silent truncation of long values, and DropIndex calls have no existence guard. |
| src/modules/Elsa.Persistence.EFCore.Oracle/Migrations/Runtime/20251204150355_V3_6.cs | Widens ActivityNodeId to NCLOB and drops its index; Down() narrows back to NVARCHAR2(450) risking data truncation, and DropIndex calls have no existence guard. |
| src/modules/Elsa.Persistence.EFCore.PostgreSql/Migrations/Runtime/20251204150341_V3_6.cs | PostgreSQL column is already text (unbounded), so only the index is dropped; Down() recreates the index correctly. DropIndex still has no IF EXISTS guard. |
| src/modules/Elsa.Persistence.EFCore.Sqlite/Migrations/Runtime/20251204150006_V3_6.cs | SQLite column is already TEXT (unbounded), so only the index is dropped; Down() recreates the index correctly. DropIndex still has no IF EXISTS guard. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[EF Core Migration: V3_6 Up] --> B{Provider?}
B --> |SQL Server| C[DropIndex ActivityNodeId\nnvarchar 450 → nvarchar max]
B --> |MySQL| D[DropIndex ActivityNodeId\nvarchar 255 → longtext]
B --> |Oracle| E[DropIndex ActivityNodeId\nNVARCHAR2 450 → NCLOB]
B --> |PostgreSQL| F[DropIndex ActivityNodeId\ncolumn already text - no AlterColumn]
B --> |SQLite| G[DropIndex ActivityNodeId\ncolumn already TEXT - no AlterColumn]
C --> H[ActivityExecutionRecords\nActivityNodeId: nvarchar max]
D --> I[ActivityExecutionRecords\nActivityNodeId: longtext]
E --> J[ActivityExecutionRecords\nActivityNodeId: NCLOB]
F --> K[Index removed only]
G --> K
H --> L[WorkflowExecutionLogRecords\nActivityNodeId: nvarchar max]
I --> M[WorkflowExecutionLogRecords\nActivityNodeId: longtext]
J --> N[WorkflowExecutionLogRecords\nActivityNodeId: NCLOB]
L --> O[No index on ActivityNodeId]
M --> O
N --> O
K --> O
style C fill:#f9f,stroke:#333
style D fill:#f9f,stroke:#333
style E fill:#f9f,stroke:#333
style F fill:#bbf,stroke:#333
style G fill:#bbf,stroke:#333
Last reviewed commit: 6c73d5d
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| migrationBuilder.DropIndex( | ||
| name: "IX_WorkflowExecutionLogRecord_ActivityNodeId", | ||
| schema: _schema.Schema, | ||
| table: "WorkflowExecutionLogRecords"); | ||
|
|
||
| migrationBuilder.DropIndex( | ||
| name: "IX_ActivityExecutionRecord_ActivityNodeId", | ||
| schema: _schema.Schema, | ||
| table: "ActivityExecutionRecords"); |
There was a problem hiding this comment.
DropIndex calls lack an existence guard
The SQL Server Up() method already uses an IF EXISTS guard when dropping the Trigger index (lines 22–29). The two new DropIndex calls for ActivityNodeId do not apply the same pattern. Because this PR mutates an existing migration (rather than adding a new one), any database where those indexes were already manually dropped — or where a partial migration was applied — will throw a "Cannot drop the index … because it does not exist" error and abort the migration entirely.
The same issue exists in all five provider migration files (Oracle, MySQL, PostgreSQL, SQLite).
For SQL Server the safest fix mirrors the existing pattern already in this file:
migrationBuilder.Sql($@"
IF EXISTS (SELECT * FROM sys.indexes WHERE name = 'IX_WorkflowExecutionLogRecord_ActivityNodeId'
AND object_id = OBJECT_ID('{_schema.Schema}.WorkflowExecutionLogRecords'))
BEGIN
DROP INDEX [IX_WorkflowExecutionLogRecord_ActivityNodeId]
ON [{_schema.Schema}].[WorkflowExecutionLogRecords]
END
");
migrationBuilder.Sql($@"
IF EXISTS (SELECT * FROM sys.indexes WHERE name = 'IX_ActivityExecutionRecord_ActivityNodeId'
AND object_id = OBJECT_ID('{_schema.Schema}.ActivityExecutionRecords'))
BEGIN
DROP INDEX [IX_ActivityExecutionRecord_ActivityNodeId]
ON [{_schema.Schema}].[ActivityExecutionRecords]
END
");For MySQL, PostgreSQL, Oracle, and SQLite equivalent IF EXISTS SQL clauses (or provider-specific conditional DDL) should be applied.
| migrationBuilder.AlterColumn<string>( | ||
| name: "ActivityNodeId", | ||
| schema: _schema.Schema, | ||
| table: "WorkflowExecutionLogRecords", | ||
| type: "varchar(255)", | ||
| nullable: false, | ||
| oldClrType: typeof(string), | ||
| oldType: "longtext") | ||
| .Annotation("MySql:CharSet", "utf8mb4") | ||
| .OldAnnotation("MySql:CharSet", "utf8mb4"); | ||
|
|
||
| migrationBuilder.AlterColumn<string>( | ||
| name: "ActivityNodeId", | ||
| schema: _schema.Schema, | ||
| table: "ActivityExecutionRecords", | ||
| type: "varchar(255)", | ||
| nullable: false, | ||
| oldClrType: typeof(string), | ||
| oldType: "longtext") | ||
| .Annotation("MySql:CharSet", "utf8mb4") | ||
| .OldAnnotation("MySql:CharSet", "utf8mb4"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_WorkflowExecutionLogRecord_ActivityNodeId", | ||
| schema: _schema.Schema, | ||
| table: "WorkflowExecutionLogRecords", | ||
| column: "ActivityNodeId"); | ||
|
|
||
| migrationBuilder.CreateIndex( | ||
| name: "IX_ActivityExecutionRecord_ActivityNodeId", | ||
| schema: _schema.Schema, | ||
| table: "ActivityExecutionRecords", | ||
| column: "ActivityNodeId"); |
There was a problem hiding this comment.
Down migration will silently truncate or fail on existing long-value rows
The MySQL Down() migration narrows ActivityNodeId back from longtext to varchar(255). Any row that was inserted with a value longer than 255 characters (the exact scenario this PR was created to fix) will either be silently truncated (when sql_mode does not include STRICT_TRANS_TABLES) or cause the rollback to fail entirely (when strict mode is on).
The same risk applies to the Oracle Down() migration (NCLOB → NVARCHAR2(450)) and the SQL Server Down() migration (nvarchar(max) → nvarchar(450)).
Because data loss on rollback is irreversible, the Down() migration should at minimum document this limitation with a comment, or guard the column change with an explicit truncation/assertion step that makes the data-loss risk visible rather than silent.
Additional Comments (1)
This PR amends the The safer approach is to introduce a new migration file (e.g. |
|
@copilot Take a look at the comments on this PR (from @graptile-apps, specifically). However, ignore its comments about existing DBs - we're working on unreleased code, so it's OK to have been updating existing migrations. What I am mostly interested in, is: are we using any (DB) queries on NodeId anywhere where the DB index would have been important in terms of perf? Can you check? |
I checked every place in the codebase where
In every case, |
…oviders Replace bare migrationBuilder.DropIndex() calls with provider-specific conditional DDL to prevent errors when indexes have already been dropped: - SQL Server: IF EXISTS (SELECT * FROM sys.indexes ...) pattern - PostgreSQL: DROP INDEX IF EXISTS (native support) - SQLite: DROP INDEX IF EXISTS (native support) - MySQL: information_schema.statistics check with PREPARE/EXECUTE - Oracle: PL/SQL BEGIN...EXCEPTION WHEN OTHERS (SQLCODE -1418) Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Deeply nested workflows produce
ActivityNodeIdvalues that exceed fixed column sizes (nvarchar(450)on SQL Server,varchar(255)on MySQL,NVARCHAR2(450)on Oracle), causingString or binary data would be truncatederrors on insert.Column type changes
Widen
ActivityNodeIdto the provider-native unlimited type in V3_6 migrations:nvarchar(450)nvarchar(max)varchar(255)longtextNVARCHAR2(450)NCLOBtextTEXTIndex removal
IX_ActivityExecutionRecord_ActivityNodeIdandIX_WorkflowExecutionLogRecord_ActivityNodeId— unlimited-length columns cannot be B-tree indexedHasIndexcalls fromConfigurations.csActivityNodeIdalso filters byWorkflowInstanceId, which has its own indexDefensive migration DDL
DropIndexcalls use provider-specific existence guards to avoid failures on partial re-runs:DROP INDEX IF EXISTS(native syntax)information_schema.statisticscheck withPREPARE/EXECUTEORA-01418(index does not exist)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.