elsa-workflows / elsa-workflows/elsa-core
Retire obsolete WorkflowStorageDriver dual dictionary / dual registration (PersistentVariablesDictionary vs Variables)
- Dominant language
- C#
- Stars
- 7.9k
- Forks
- 1.5k
- Avg merge
- 15h 22m
- Merged PRs (30d)
- 114
Description
## Problem
`WorkflowStorageDriver` is marked `[Obsolete]` and documented as unused, but it is still registered in DI, aliased for serialization, and special-cased in persistence extraction. It keeps a **second** in-workflow variable store (`PersistentVariablesDictionary` of `object`) beside `WorkflowInstanceStorageDriver`'s `Variables` (`JsonNode`) dictionary — dual paths, dual semantics.
This is leftover follow-through from #8129 (fail-open Write/Read fixed in #8139). Item 4 there (“consider retiring obsolete `WorkflowStorageDriver`”) was not delivered and has no open tracking issue.
## Evidence (main)
1. **Obsolete type still present** — `src/modules/Elsa.Workflows.Core/VariableStorageDrivers/WorkflowStorageDriver.cs`
- `[Obsolete("… Use the WorkflowInstanceStorageDriver instead.")]`
- State key: `PersistentVariablesDictionary` / `IDictionary`
- Contrast: `WorkflowInstanceStorageDriver.VariablesDictionaryStateKey = "Variables"` / `Dictionary`
2. **Still registered (classic + shell features)**
- `Features/WorkflowsFeature.cs` and `ShellFeatures/WorkflowsFeature.cs`:
- `.AddStorageDriver()`
- `.AddStorageDriver()`
- `AddTypeAliasWithLegacyName(…)` alongside the modern driver
3. **Persistence still dual-aware** — `WorkflowStateExtractor.GetPersistableInput`:
```csharp
// TODO: This is a temporary solution. We need to find a better way to handle this.
var persistableInput = …Inputs.Where(x =>
x.StorageDriverType == typeof(WorkflowStorageDriver)
|| x.StorageDriverType == typeof(WorkflowInstanceStorageDriver));
```
4. **UI/API surface** — `StorageDriverManager.List()` returns every registered driver; obsolete type still has `[Display(Name = "Workflow")]`, so hosts/Studio can keep selecting a driver the code says is unused.
5. **Legacy JSON fixtures** still embed the old type name (integration/component tests) — expected for migration, not a reason to keep the live DI registration forever.
## Why it matters architecturally
- Two stores for “persist with the workflow instance” invites silent split-brain (write via one driver, read/extract via the other).
- After #8129 tightened fail-open policy on the modern driver only, the obsolete path remains an untested/unaligned twin.
- Subtractive clarity: one instance-backed storage driver, one dictionary key, one extraction rule.
## Proposed subtractive direction (not a full design)
1. Stop registering `WorkflowStorageDriver` in new hosts (or gate behind an explicit legacy opt-in if migration still needs the type resolve).
2. Map legacy type aliases / stored `StorageDriverType` names → `WorkflowInstanceStorageDriver` on load (definitions + instance state), then remove the live `IStorageDriver` implementation when safe.
3. Collapse `GetPersistableInput` to a single driver (or a shared “instance-persisted” tag/contract) and delete the TODO dual check.
4. Update fixtures gradually; keep type-name aliases only as long as deserialization of old documents requires them.
Prefer deletion/mapping over a new abstraction layer.
## Not a duplicate of
- #8129 (closed) — Write fail-open `Remove` / Read poison `JsonNode` (fixed in #8139); retirement was only a suggested follow-through.
- #8159 — `WithVariable(name, value)` never sets a storage driver (silent memory-only) — related persistence footgun, different surface.
- #6750 / #6780 — default / flexible storage-driver selection — complementary; do not expand this issue into a defaults feature.
- #6978 — ExpandoObject / `List<>` deserialize on the modern driver.
## Milestone
Unset — Triage / Crew Lead place. Do not auto-assign Engineer. Do not add to Ready/projects from this pass.
Contributor guide
Research direction
Start with WorkflowStorageDriver.cs and compare its state with WorkflowInstanceStorageDriver, then inspect registrations in Features/WorkflowsFeature.cs and ShellFeatures/WorkflowsFeature.cs. Trace WorkflowStateExtractor.GetPersistableInput and StorageDriverManager.List(), along with legacy JSON fixtures. Done means obsolete registrations and dual extraction are retired while legacy documents still deserialize through the intended mappings.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend, databases
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100