elsa-workflows / elsa-workflows/elsa-foundation

Investigate retiring / narrowing the polymorphic object converter — unify the serialization type story

Open
#552 0 comments 0 reactions 0 assignees View on GitHub
ready-for-human tech-debt
Dominant language
C#
Stars
5
Forks
1
Avg merge
3h 56m
Merged PRs (30d)
210

Description

## Motivation

The `_type`-tagging polymorphic converter machinery (`PolymorphicObjectConverter`, `PolymorphicObjectConverterFactory`, `PolymorphicDictionaryConverter`, wire ids `_type`/`_items`/`_island`/`$ref`/`$values`) is the most complex part of Elsa's serialization, carries a `Type.GetType()` deserialization-gadget risk, and is the source of a latent stack overflow (#551). It predates the typed-value model and is inherited from Elsa 3. With the typed-argument model (spec 081) now live, it's worth asking: **do we still need it, and can we shrink or unify the serialization story?**

This is a scoping/decision issue (candidate ADR), not a code task. Surfaced while implementing deterministic serialization (#549 / spec 086).

## What the investigation found (with evidence)

**The converter is load-bearing, but only in bounded places** — it is *not* the general default it appears to be:

1. **Runtime durable values have already moved off it.** `DurableValueState` stores `InlineValue` as `JsonElement?` plus a separate `RuntimeValueTypeDescriptor` (alias-based type, out-of-band) — a typed envelope, not an `object` graph with embedded `_type`. Runtime projections (`RuntimeInputBindingStateProjection`, `RuntimeWorkflowOutputStateProjection`) keep value (JSON) and type (descriptor) separate. Type resolution goes through `IWellKnownTypeRegistry` / `TypeJsonConverter`, which is **alias-only with no `Type.GetType` fallback** (spec 081 FR-004a). → For core runtime state, the `_type` polymorphism is essentially vestigial.

2. **Design-time `StateSource` still depends on it — narrowly.** `WorkflowDefinitionState` is strongly-typed *except* for `IDictionary? PropertyInfo` and `UISpecifications` on `InputDefinition`/`OutputDefinition` (`src/Elsa/Activities/Design/Core/Models/{Input,Output}Definition.cs:22-23`). These arbitrary designer bags route through `PolymorphicDictionaryConverter` + `_type` tagging on the canonical StateSource write path (the very form spec 086 makes deterministic). `ActivityNode.Structure.Payload` is already an opaque `JsonElement` island and does *not* need polymorphism.

3. **HTTP / dynamic JSON genuinely needs open polymorphism.** `JsonHttpContentParser.Deserialize(...)` (when the response type is `object`/`ExpandoObject`) is the one clearly load-bearing open-object case. Scripting/ExpandoObject paths are similar.

4. **Two parallel type systems coexist.** (a) `TypeJsonConverter` + `WellKnownTypeRegistry` — alias-based, registry-only, safe, the go-forward model. (b) `PolymorphicObjectConverter._type` — simple-assembly-qualified name + `Type.GetType(typeName)` fallback (`PolymorphicObjectConverter.cs:307`), a legacy gadget-risk path. TypeJsonConverter is always registered alongside, so the raw `Type.GetType` fallback is largely dead in production but still reachable.

## Options (menu, roughly increasing effort)

- **A. Kill the `Type.GetType` fallback** (`PolymorphicObjectConverter.cs:307`) and route *all* type resolution through the alias registry. Removes the deserialization-gadget risk with minimal behavior change. Cheap, high-value, independent of everything else.
- **B. Make the designer bags opaque.** If the Studio doesn't need CLR-typed round-trip of `PropertyInfo`/`UISpecifications`, model them as `JsonElement`/`JsonObject` instead of `IDictionary`. This removes the polymorphic surface from the canonical StateSource entirely.
- **C. Scope the converter to the dynamic path.** Stop making the polymorphic factory a global default of `IPayloadSerializer`; provide a distinct "loose/dynamic" options variant for HTTP/scripting `Deserialize`, and let core state use plain STJ + typed envelopes.
- **D. Fix #551 as part of the above** — the stack overflow lives in exactly the legacy read branch options A–C would remove or simplify, so don't fix it in isolation if B/C land.

## Non-goals / caveats

- Unreleased software — no back-compat shim needed ([memory: unreleased-no-backcompat]), so we can change the wire format freely.
- The `_type`/`_items`/`$ref`/`$values` ids are currently pinned as a frozen contract by `PolymorphicObjectConverterReferenceTests` (constitution §E6) — any change re-baselines those tests deliberately.
- Per the constitution, StateSource scope (§E2.9) is architectural; option B touches authored-content modeling and likely needs an architecture-meeting decision → hence `ready-for-human`.

## Deliverable

An ADR that decides the target serialization type story (unify on the alias registry; where open polymorphism is still permitted; fate of the `_type` wire format), then incremental PRs per the chosen options.

Related: #549 (deterministic serializer / spec 086), #551 (stack overflow in the legacy read branch), spec 081 (typed argument model).

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading PolymorphicObjectConverter.cs, especially the type-resolution branch near line 307, and PolymorphicObjectConverterReferenceTests. Then inspect the StateSource models and JsonHttpContentParser paths named in the issue. Done means producing an ADR that decides the alias-registry, open-polymorphism, and legacy wire-format strategy, with incremental PR boundaries.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp, json
Domain
backend, infrastructure
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.