STJ converter review round: streamed write, declared-type writer, modernization - #212
Open
manuc66 wants to merge 5 commits into
Open
STJ converter review round: streamed write, declared-type writer, modernization#212manuc66 wants to merge 5 commits into
manuc66 wants to merge 5 commits into
Conversation
The generator runs in metadata mode, not fast-path mode: System.Text.Json only fast-paths types without a custom converter. And unlike native [JsonDerivedType] polymorphism (which needs AllowOutOfOrderMetadataProperties for a mid-object discriminator), the converter reads the discriminator from anywhere.
…-level LINQ Code review follow-ups: - RegisterDynamicSubtype now rejects null, abstract, interface or non-assignable types instead of silently breaking the converter's invariants. - Cache the discriminator key type per converter (the generated converter compiles it) instead of scanning the mapping keys on every object, and make it volatile so a concurrent registration is visible. A registration racing a deserialization only affects the cache, never the mapping. - GetTypeResolver scans the converter array directly with an excluded resolver instead of rebuilding a LINQ Where iterator on every multi-level walk step.
…ument The payload is written into a compact buffer we just produced, so re-reading it token by token with a Utf8JsonReader avoids materializing a JsonDocument DOM. The discriminator is injected first or last and the payload property of the same name is skipped. Values are copied with a small recursive copier; numbers use WriteRawValue to preserve the exact token (decimals, exponents, big ints). Measured (BenchmarkDotNet, net10, DefaultJob): Single_Converter_Serialize 1.15us -> 1.00us, Col_Converter_Serialize 4.23us -> 3.41us. All 200 STJ tests pass.
…scriminator types Code review follow-ups: - BuildBaseTypeWriter serialized each property as object (runtime type); STJ uses the declared property type so a polymorphic converter on that type applies. The reader already used the declared type, so the writer now matches. - RegisterDynamicSubtype rejected abstract/interface types; it now also rejects a discriminator whose type differs from the existing keys, which would make the cached key type inconsistent.
…fixes
- JsonDiscriminatorPropertyName is private (was protected); no internal code
or test derives-and-uses it.
- Write takes T? and ReadPlainObject returns T (non-nullable), matching their
actual contracts.
- Replace verbose conditions with pattern matching (is {...}, [..^], ?.ConverterType).
- Drop the volatile on _mappingKeyType: it only protected a cache field whose
stale-read risk is benign, and RegisterDynamicSubtype documents its setup-time
contract instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The converter review round addressed: README claims about two STJ behaviors, missing RegisterDynamicSubtype validation, a write path still materializing JSON strings, a base writer that ignored the declared property type, and stale code style.
Fix
Tests
Honest note(s)