STJ converter review round: streamed write, declared-type writer, modernization by manuc66 · Pull Request #212 · manuc66/JsonSubTypes

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 · 2026-08-16T06:14:34Z

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

Clarify two STJ behaviors in the README.
Validate RegisterDynamicSubtype, cache the mapping key type, drop per-level LINQ.
Stream the discriminator write with Utf8JsonReader instead of JsonDocument.
Use the declared property type in the base writer and reject mixed discriminator types.
Modernize JsonSubtypes.cs: private field, pattern matching, nullable fixes.

Tests

New ReviewBugTests; full STJ suite passes (net8.0 and net10.0).

Honest note(s)

The streamed write changes the MaxDepth + 1 note and the allocation numbers for the converter; the follow-up review-fixes PR corrects the docs and re-measures the benchmarks.

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.

manuc66 added 5 commits August 15, 2026 21:35

Uh oh!

Conversation

manuc66 commented Aug 16, 2026

Problem

Fix

Tests

Honest note(s)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

manuc66 commented Aug 16, 2026

Problem

Fix

Tests

Honest note(s)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

manuc66 commented Aug 16, 2026

Problem

Fix

Tests

Honest note(s)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

manuc66 commented Aug 16, 2026

Problem

Fix

Tests

Honest note(s)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

manuc66 commented Aug 16, 2026

Problem

Fix

Tests

Honest note(s)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant