Grandiras/ATProto.NET

Fork 0

UnionJsonConverterFactory breaks serialization of any IAtProtoUnion implementor (CanConvert true, CreateConverter null) #46

New issue

Closed

opened 2026-07-05 17:33:58 +00:00 by Grandiras · 2 comments

Grandiras commented

2026-07-05 17:33:58 +00:00

Owner

AtProtoJsonDefaults.Options registers UnionJsonConverterFactory, whose CanConvert returns true for every type assignable to IAtProtoUnion while CreateConverter returns null ("let the default serializer handle it"). System.Text.Json treats a null return from a claimed factory as an error:

System.InvalidOperationException: The converter 'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value.

So the moment a consumer marks a custom union base with IAtProtoUnion (as suggested for open unions), all (de)serialization of records containing it throws. The SDK's own union types never hit this because they use [JsonPolymorphic]/[JsonDerivedType] without implementing the marker.

Fix options: make CanConvert return false (the attribute-based polymorphism handles unions), or actually implement the converter. Found while building Mise's RecipeAttribution union for exchange.recipe.defs#attribution* — Mise works around it by not implementing the marker interface.

`AtProtoJsonDefaults.Options` registers `UnionJsonConverterFactory`, whose `CanConvert` returns true for every type assignable to `IAtProtoUnion` while `CreateConverter` returns `null` ("let the default serializer handle it"). System.Text.Json treats a null return from a claimed factory as an error: ``` System.InvalidOperationException: The converter 'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value. ``` So the moment a consumer marks a custom union base with `IAtProtoUnion` (as suggested for open unions), all (de)serialization of records containing it throws. The SDK's own union types never hit this because they use `[JsonPolymorphic]`/`[JsonDerivedType]` without implementing the marker. Fix options: make `CanConvert` return false (the attribute-based polymorphism handles unions), or actually implement the converter. Found while building Mise's `RecipeAttribution` union for `exchange.recipe.defs#attribution*` — Mise works around it by not implementing the marker interface.

claude-bot was assigned by Grandiras

2026-07-25 02:10:36 +00:00

claude-bot commented

2026-07-25 02:10:41 +00:00

Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/35); the reply will link the full session transcript.

claude-bot referenced this issue from a commit

2026-07-25 02:14:49 +00:00

fix: remove UnionJsonConverterFactory so IAtProtoUnion is serializable (closes #46)

claude-bot referenced this issue

2026-07-25 02:15:05 +00:00

Claude: changes for issue #46 #55

claude-bot commented

2026-07-25 02:15:06 +00:00

Collaborator

Fixed and pushed to branch claude/issue-46.

Diagnosis

Confirmed exactly as reported. UnionJsonConverterFactory.CanConvert returned true for anything assignable to IAtProtoUnion, while CreateConverter returned null — which System.Text.Json rejects outright:

System.InvalidOperationException : The converter
'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value.

I reproduced it first with a RecipeAttribution-shaped union (mirroring Mise's exchange.recipe.defs#attribution*): 5 tests, all 5 throwing before the change, on both serialize and deserialize, and on the containing record as well as the union itself.

The factory was registered in two places, not one — AtProtoJsonDefaults.CreateOptions() and LexiconTypeRegistry.CreateOptions() — so the plugin/registry path was equally affected.

Fix

Took the "make it not claim the type" option, in its strongest form: the factory is deleted rather than neutered. It never had any behaviour — union discrimination comes entirely from [JsonPolymorphic]/[JsonDerivedType] plus LexiconTypeRegistry.RegisterUnionVariant, exactly as its own comment admitted. A CanConvert => false factory would just be dead weight in every options instance. It was internal, so this is not a public API change.

IAtProtoUnion stays public, now XML-documented as what it actually is: a behaviour-free documentation marker that is optional and has no effect on serialization. Removing it would have broken anyone who took the original advice; leaving it undocumented would leave the next person guessing what it does.

Also added a "Union Types" section to docs/custom-records.md — the gap that produced the misleading advice in the first place. It covers closed unions via attributes, the <nsid>#<defName> discriminator convention, that the marker is optional, and open unions via RegisterUnionVariant.

Verification

Installed the .NET 10 SDK in the container (none was present), then:

dotnet build across the solution — succeeded, 0 errors, no new warnings (remaining ones are pre-existing CS1591/NU1902); no broken cref in the new XML docs.
dotnet test tests/ATProtoNet.Tests/ — 1000/1000 passed, up from 999. The 6 new tests in Serialization/AtProtoUnionMarkerTests.cs cover serialize, deserialize, the union as a root value, out-of-order $type on a marker union (interaction with the #50 fix), registry options, and the documented open-union runtime-registration round-trip. All of them fail on main.

CHANGELOG.md updated under [Unreleased] → Fixed in the same commit.

One note for Mise: the workaround (not implementing the marker) was correct and remains correct — it stays correct after this fix too, since the interface is now genuinely inert. You can add it back for documentation value whenever convenient, but nothing requires it.

⏱ 4m 16s · 33 turns · tokens: 40.2k in / 11.8k out (+1198.7k cache-read)
📜 Full session transcript
→ Opened PR: #55

Fixed and pushed to branch **`claude/issue-46`**. ## Diagnosis Confirmed exactly as reported. `UnionJsonConverterFactory.CanConvert` returned true for anything assignable to `IAtProtoUnion`, while `CreateConverter` returned `null` — which System.Text.Json rejects outright: ``` System.InvalidOperationException : The converter 'ATProtoNet.Serialization.UnionJsonConverterFactory' cannot return a null value. ``` I reproduced it first with a `RecipeAttribution`-shaped union (mirroring Mise's `exchange.recipe.defs#attribution*`): 5 tests, all 5 throwing before the change, on both serialize and deserialize, and on the containing record as well as the union itself. The factory was registered in **two** places, not one — `AtProtoJsonDefaults.CreateOptions()` *and* `LexiconTypeRegistry.CreateOptions()` — so the plugin/registry path was equally affected. ## Fix Took the "make it not claim the type" option, in its strongest form: the factory is **deleted** rather than neutered. It never had any behaviour — union discrimination comes entirely from `[JsonPolymorphic]`/`[JsonDerivedType]` plus `LexiconTypeRegistry.RegisterUnionVariant`, exactly as its own comment admitted. A `CanConvert => false` factory would just be dead weight in every options instance. It was `internal`, so this is not a public API change. `IAtProtoUnion` **stays public**, now XML-documented as what it actually is: a behaviour-free documentation marker that is optional and has no effect on serialization. Removing it would have broken anyone who took the original advice; leaving it undocumented would leave the next person guessing what it does. Also added a **"Union Types"** section to `docs/custom-records.md` — the gap that produced the misleading advice in the first place. It covers closed unions via attributes, the `<nsid>#<defName>` discriminator convention, that the marker is optional, and open unions via `RegisterUnionVariant`. ## Verification Installed the .NET 10 SDK in the container (none was present), then: - **`dotnet build`** across the solution — succeeded, 0 errors, no new warnings (remaining ones are pre-existing `CS1591`/`NU1902`); no broken `cref` in the new XML docs. - **`dotnet test tests/ATProtoNet.Tests/`** — **1000/1000 passed**, up from 999. The 6 new tests in `Serialization/AtProtoUnionMarkerTests.cs` cover serialize, deserialize, the union as a root value, out-of-order `$type` on a marker union (interaction with the #50 fix), registry options, and the documented open-union runtime-registration round-trip. All of them fail on `main`. `CHANGELOG.md` updated under `[Unreleased]` → `Fixed` in the same commit. One note for Mise: the workaround (not implementing the marker) was correct and remains correct — it stays correct after this fix too, since the interface is now genuinely inert. You can add it back for documentation value whenever convenient, but nothing requires it. --- ⏱ 4m 16s · 33 turns · tokens: 40.2k in / 11.8k out (+1198.7k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/issue-46/run-35.md) → Opened PR: https://git.grandiras.net/Grandiras/ATProto.NET/pulls/55

claude-bot referenced this issue

2026-07-25 02:22:09 +00:00

Claude: changes for issue #46 #55

Grandiras referenced this issue from a commit

2026-07-25 02:22:47 +00:00

Claude: changes for issue #46 (#55)

Grandiras closed this issue

2026-07-25 02:23:04 +00:00

claude-bot referenced this issue

2026-07-25 02:26:33 +00:00

Claude: changes for issue #49 #57

claude-bot referenced this issue