feat: EDM-driven OData open types on complex types, opt-in (#389)

[engenb]

Implements OData open types (dynamic property bags) on complex types, driven entirely by the EDM — the consumer's model needs no [JsonExtensionData] or any other attribute.

Closes #389.

What it does

ODataConventionModelBuilder — the builder OhData already uses — infers a dynamic-property container from an IDictionary<string, object>-assignable member, marks the containing type OpenType="true" in the CSDL, omits the member from the declared properties, and records the backing PropertyInfo as a DynamicPropertyDictionaryAnnotation. This PR reads that annotation back at MapOhData() and marks exactly that member as System.Text.Json extension data, so dynamic keys serialize and bind flat — siblings of the declared properties rather than nested under the container's name.

The same registration that produces the CSDL produces the wire shape, so the two cannot drift. Nothing is matched by property name or by convention.

public record ExternalReferenceMetadata
{
    public IDictionary<string, object?>? KeyValuePairs { get; set; }   // no attributes
}

"Metadata": { "organizationCreatedDate": "2026-01-01T00:00:00.0000000+00:00", "tier": 3 }

KeyValuePairs appears nowhere on the wire and nowhere in $metadata.

Opt-in — and why

builder.Services.AddOhData(b => b.WithOpenTypes());   // default: off

The first cut of this applied unconditionally, on the reasoning that the CSDL already said OpenType="true" so we were only making the JSON agree with its own metadata. Adversarial review showed why that is wrong: the CSDL was the half nobody was reading, and the wire was the contract in practice.

Once the container is extension data it is no longer a declared property, so an existing adopter's body

{ "Meta": { "Region": "r", "Bag": { "a": 1 } } }

binds "Bag" as a dynamic key holding the old dictionary — the handler persists Bag = { "Bag": {"a":1} } — and the response echo is byte-identical to the previous one. Silent write corruption with no runtime signal.

Off by default ⇒ the container map is never built, Build returns the base JsonSerializerOptions reference-equal, no per-request validation runs, and behaviour is byte-identical to develop. OpenTypeOptInTests pins both halves.

Review findings addressed

Two-pass local review on the first commit returned 1 critical + 3 high, all invisible in the response body — which is why the original 21 tests passed. Each is fixed and pinned:

	Finding	Fix
1	CRITICAL silent re-bind of existing payloads	opt-in switch, above
2	HIGH @odata.* and non-identifier keys persist into the bag and echo on every read — a stored deserialization-fault vector against other OData consumers	FindInvalidDynamicKey walks the raw JsonElement against JsonTypeInfo; 400 + OData envelope with target = the offending key, on POST/PUT/PATCH and property-route writes
3	HIGH a bag key equal to a declared property name emits duplicate JSON keys, and every .NET reader tested resolves in the bag's favour — making the declared value unreachable	container getter wrapped: declared wins, shadowed key dropped, warning logged. Same reference returned when there is no collision, so the deserialize path is untouched
4	HIGH PATCH is whole-value replace; docs claimed undeclared keys survive	semantics unchanged (pre-existing, and a recorded 1.0.0 scope decision); docs rewritten with a read-modify-write recipe, and the misleading test renamed to Patch_ReplacesTheWholeComplexValue_SeededDynamicKeysAreLost — it previously observed the wipe and read as a pass
5	ValidateOrThrow did not cover what its docstring promised	broader catch + wrap, probes derived open types, explicit extension-member count; docstring now honest about the instance-dependent fault it cannot see
6	getter-only containers silently skipped, reproducing the exact EDM/wire mismatch the feature declines to ship for entity roots	throws at MapOhData() naming type, member and fix
7	modifier-ordering rationale in CLAUDE.md and the factory was false — Ignore() cannot reach a complex type, so the two modifiers can never see the same JsonTypeInfo	removed from all four places; composition test relabelled honestly
8	docs stated the opposite of actual behaviour for new-shadowed containers	confirmed it is flattened; docs and comment corrected, pinning test added
9	HasSameMetadataDefinitionAs comment claimed an identity match it does not provide (GBag<int>/GBag<string> match)	comment rewritten to state the real invariant — base-chain walk against a DeclaringType-keyed map — and to warn that re-keying would convert a declared property into a bag
10	null/empty container asymmetry undocumented	documented + test

Three review findings were pushed back on with evidence rather than accepted:

• Finding 3's "also reject at write time" is unreachable — STJ binds a body key matching a declared name to the declared property; it never reaches the bag. The serialization-side guard is the whole fix.
• Finding 5 understated the hole: GetTypeInfo does not catch the competing-[JsonExtensionData] case at all (it resolves fine; only Serialize throws), so the method's stated primary purpose was entirely unmet rather than under-covered.
• Finding 6's "keep the type-assignability check" needed a justification, and there is one: it is unreachable. The convention builder only infers IDictionary<string,object>-assignable members, and a consumer cannot write the annotation by hand — EdmAnnotationExtensions exposes only a getter and DynamicPropertyDictionaryAnnotation is internal. It stays as a defensive guard with that reason recorded.

A bug in the fix itself was found and fixed during implementation: returning a plain Dictionary from the collision-filtering getter throws InvalidCastException when the container is declared as a custom subclass, because STJ casts back to the declared type. The clone now preserves the runtime type and degrades with a logged error rather than faulting on a read path.

Scope

Complex types only. Entity-root dynamic containers are deliberately not handled: the PATCH delta loop resolves body members through FindClrPropertyByEdmName and skips what it cannot resolve, so a root-level undeclared key would be silently dropped on write. Half-working is worse than absent. Documented as a known limitation in docs/open-types.md.

#325/#326 clause-bounded serialization is not widened. Review confirmed against a develop baseline that bag values already reached System.Text.Json before this change — they were simply written one level deeper — and that no code path added here can reach ResolveNavTreatment, TryBuildEngagedExpand, or the pushdown gate. Complex types carry no EDM navigation properties, so delegate safety is untouched.

Review rounds

Three adversarial rounds ran on this branch. Each is recorded because the pattern matters more than any single finding: the code was broadly right every time; the prose describing it was not.

round	target	outcome
1	b65154c (feature)	1 CRITICAL + 3 HIGH → opt-in switch, key validation, collision guard, PATCH doc correction (a2f0536)
2	a2f0536 (round-1 fix)	2 HIGH + 3 MEDIUM → the @odata.type mitigation was one level deep and never recursed into a dynamic key's value; nav-POST and action routes were unwired; a GET could 500; the identifier check rejected valid non-ASCII identifiers (9cb2524)
3	9cb2524 (round-2 fix)	clean above LOW → one remaining coverage gap through dictionary-valued members, plus prose/message corrections (f31ab8f)

Round 2 also found that round 1's justification comment for an untested branch was false, and round 3 found that round 2's replacement justification was false as well — the real reason that branch is unreachable is that AddDynamicPropertyDictionary self-validates the container type, measured by reflection rather than reasoned about.

Round 3 verified, empirically rather than by reading: every one of the eight body-reading write routes rejects first-level and nested reserved keys (including the two the tests did not cover — entity-level bound action and unbound action); $ref writes are correctly excluded; JsonDocument's 64-depth cap terminates before the walk is entered; cost is linear (50,000 sibling keys → 106 ms); the ABNF leading/following split, surrogate handling and 128-code-point cap are exact; and named registrations do not leak opt-in state — v1 with the flag rejects while v2 over the same CLR model returns 201 unflattened.

Verification

• 1669 passing in the core suite, 2130 across the solution, 0 failures (baseline 1582, +87). Build clean, 0 warnings, net8.0 and net10.0.
• Merged with develop (0552f34, including the five dependabot bumps); merge touched only .csproj and workflow files.
• Zero-delta when off, verified byte-for-byte across a 25-case transcript — status and full response body identical to develop@0552f34 for malformed, empty, array, null, trailing-garbage and 70-level-deep bodies, wrong Content-Type, well-formed writes on every route, plus $metadata and the service document.
• New tests were checked for revert-sensitivity: reverting the nested-key recursion, the dictionary-member walk, the DropShadowedKeys fix and the OpenTypesActive gate each fails specific tests. A test that still passes with its fix removed is not a regression test.
• One earlier solution-wide run showed ObservabilityTests.Metadata_Operation_IsTagged failing. It passes in isolation on both this branch and clean develop, and the full project passes on consecutive runs — a load-dependent flake, filed as Flaky test: ObservabilityTests.Metadata_Operation_IsTagged captures no activity under solution-wide parallel load #394, not a regression from this change.

Performance

No benchmarks were run for this PR, and here is the honest reason rather than a silent omission: with the feature off — the default, and what every existing deployment gets — Build returns the base JsonSerializerOptions reference-equal and no code added here executes, so there is nothing to measure. OpenTypesActive narrows this further: a registration that opts in but whose model has no open complex types also does no per-request work.

With the feature on, two costs are added:

• one JsonElement walk per write request against the already-resolved JsonTypeInfo — measured linear, 50,000 sibling keys under one dynamic key in 106 ms, and JsonDocument's 64-level cap rejects deep bodies before the walk is entered;
• one pass over the bag's keys per open-complex-type instance serialized (O(bag size), HashSet lookups, no allocation unless a collision is found) — unmeasured.

Neither is on the collection read path for a model that does not opt in. If the opt-in path becomes hot for an adopter, the serialization-side pass should be benchmarked then.

Follow-ups filed

• $filter/$orderby over a dynamic (open-type) property returns 500 instead of 400 #390 — $filter/$orderby over a dynamic property returns 500 instead of 400. Pre-existing behaviour surfaced by this work, not a regression from it.

[codecov]

Codecov Report

❌ Patch coverage is 87.63736% with 45 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/OhData.AspNetCore/OpenTypeJsonOptions.cs	82.42%	22 Missing and 23 partials ⚠️

📢 Thoughts on this report? Let us know!