feat(serialization)!: forward-compatible models — open unions and unknown-field round-tripping #149

Merged

Grandiras merged 1 commit from issue-115-open-unions into main 2026-09-24 18:16:59 +00:00

Conversation 4 Commits 1 Files changed 56 +2356 -664

Grandiras commented 2026-09-24 18:11:42 +00:00

Owner

Copy link

Closes #115.

Lexicon unions are open by spec and Lexicons grow optional fields, but the SDK's unions were closed and its models dropped unknown fields. One unrecognized $type failed the whole response, and read-modify-write lost data. This PR puts the fix in one shared layer that later PRs can build on. "Unions and unknown fields" in docs/custom-records.md documents the pattern.

Open unions

• [AtProtoUnion] (Serialization/AtProtoUnionAttribute.cs) marks an abstract union base. Variants are still declared with [JsonDerivedType], so the discriminators stay next to the base.
  • An open union names an Unknown{Base} variant, a sealed subclass implementing IUnknownUnionVariant (Type + Raw, with a (string, JsonElement) constructor).
  • [AtProtoUnion(Closed = true)] keeps strict behavior.
  • A misdeclared base (open with no unknown variant, duplicate discriminators, and so on) fails with a clear InvalidOperationException the first time it is used.
• AtProtoUnionConverterFactory (Serialization/AtProtoUnionConverter.cs) reads and writes these bases.
  • Read: it scans a copy of the reader for the top-level $type, wherever it sits (the appview often puts it last). It then resolves in order: the declared variants, then the registry (checked on every miss, so late registration works), then Unknown{Base} holding JsonElement.ParseValue. A closed union throws JsonException instead.
  • Write: an unknown variant writes its original bytes via JsonMarshal.GetRawUtf8Value + WriteRawValue, so number formatting, escapes and whitespace survive byte for byte. A known variant is written through its own contract.
• Why a converter and not IgnoreUnrecognizedTypeDiscriminators: that flag falls back to the abstract base and loses the discriminator. The contract modifier LexiconTypeRegistry.ApplyUnionContracts makes the converter approach work:
  • It removes STJ's PolymorphismOptions from [AtProtoUnion] bases, because STJ refuses polymorphism metadata on a type with a custom converter.
  • It gives every variant a leading $type property that is written but never read. A variant therefore always writes its discriminator first, and an incoming $type is matched and skipped, so it never allocates or lands in ExtensionData.
  • It still adds registered variants to plain [JsonPolymorphic] bases.
• AllowOutOfOrderMetadataProperties = true stays, for plain [JsonPolymorphic] types. Tests cover a trailing $type for both kinds, and a JSON-escaped $type property name.
• Converted to open unions: EmbedBase, EmbedView, FacetFeature, ThreadNode, ReportSubject, and Ozone's ModEventType and ModerationSubject (Ozone emits 11 event types the SDK doesn't model). Each gets an Unknown* variant.
• Closed: ApplyWriteOperation, the only closed union upstream.
• Gallery: added app.bsky.embed.gallery (GalleryEmbed/GalleryImage with the open GalleryItem union, GalleryView/GalleryViewImage with the open GalleryViewItem union), registered on EmbedBase and EmbedView. That covers feed.post#embed, feed.defs#postView.embed, and recordWithMedia#media/#view.media, since both use those bases. embed.record#viewRecord.embeds sits inside RecordView.Record, which is still a raw JsonElement, so it is covered once the lexicon-fidelity issue types it as List<EmbedView>.
• Guard test: reflection over the assembly checks that every $type union base is an [AtProtoUnion] with a valid shape, and that only ApplyWriteOperation is closed. The exceptions are Spaces and FirehoseMessage; see "Deferred".

Unknown fields

• LexObject (Models/LexObject.cs) provides [JsonExtensionData] IDictionary<string, JsonElement>? ExtensionData. It stays null unless an undeclared field shows up.
• Which types derive from it (a scripted, mechanical change): AtProtoRecord and every SDK model of a Lexicon record, view or object, in App.Bsky, Chat, Com.AtProto, Site.Standard, Tools.Ozone and Models/CommonModels.cs.
  • Excluded: XRPC *Request/*Response envelopes, the fixed data-model shapes BlobRef/BlobLink/CidLink, IdentityModels, the firehose frame models, and Spaces.
• $type handling:
  • ApplyRecordTypeDiscriminator works with extension data unchanged. The record's $type is a get-only contract property, so an incoming $type is matched and skipped rather than routed to ExtensionData. Tested: no duplication, and a mismatched incoming $type is dropped while the record's own wins on write.
  • The SDK's record models (PostRecord etc.) behave the same way through their get-only Type.

LexiconTypeRegistry / AtProtoJsonDefaults

• AtProtoJsonDefaults.Options now carries the union factory and modifier bound to LexiconTypeRegistry.Instance. Registered variants therefore take effect in every client, in RecordCollection<T>, and in Jetstream GetRecord<T>(), which now uses these options.
• The options are frozen with MakeReadOnly(). The wrong XML remark is gone, and the new one points to copying: new JsonSerializerOptions(AtProtoJsonDefaults.Options) keeps all of the behavior above.
• Removed:
  • the write-only record-type registry (RegisterRecordType/GetRecordType/RecordTypes)
  • _plugins
  • the duplicate CreateOptions()
  • [LexiconPlugin] + LoadPluginsFromAssembly (which did nothing LoadPlugin<T>() doesn't)
  • the public constructor (a second registry had no way to take effect)
  • the no-op IAtProtoUnion (its test became CustomUnionTests, the custom open-union test)
• Kept: ILexiconPlugin + LoadPlugin<T>(). That is still a meaningful packaging convention for "variants my package adds to unions it doesn't own".
• Validation: RegisterUnionVariant now rejects a non-union base, and a discriminator already declared or registered for another type.

Bytes

• Label.Sig has [JsonConverter(typeof(LexBytesJsonConverter))]. The test uses the mod.bsky.app queryLabels shape with a 64-byte unpadded $bytes.
• A new LexBase64.Decode (padding optional) is used by LexBytesJsonConverter, JetstreamEventParser and JetstreamSegmentReader. The latter two used Convert.FromBase64String, which rejects the unpadded base64 the data model specifies.
• DagCborEncoder.PadBase64 was left alone as instructed.

UpdateProfileAsync

• New shape: Task<RecordRef> UpdateProfileAsync(Action<ProfileRecord> update, CancellationToken).
  1. It reads app.bsky.actor.profile/self (or starts from an empty record with createdAt when there is none).
  2. It lets the callback edit the record in place.
  3. It writes with swapRecord: <cid>.
  4. On InvalidSwap it re-reads and re-runs the callback, up to 3 attempts in total, then rethrows.
• Untouched fields, ExtensionData included, are written back unchanged.
• ProfileRecord gains Pronouns, Website, Labels (SelfLabels) and JoinedViaStarterPack.
• Deviation from the suggested Func<ProfileRecord, ProfileRecord>: the models are classes, and LexObject must be a class because users write class TodoItem : AtProtoRecord. So with is unavailable, and a Func would force callers to copy every field and ExtensionData by hand. ProfileRecord's properties are therefore settable, and the callback is an Action: p => p.DisplayName = "x". Setting null clears a field, which the old API could not do.
• Only this method and its private helpers changed in AtProtoClient.cs.

Breaking changes (all in CHANGELOG with migration notes)

• Union bases: [JsonPolymorphic] becomes [AtProtoUnion], the bases derive from LexObject, and new Unknown* variants appear. Migration: add an Unknown* or _ arm to switches; the compiler won't flag it.
• Models: they now derive from LexObject, a binary break. A consumer record type with its own [JsonExtensionData] must drop it.
• UpdateProfileAsync: new signature, and ProfileRecord setters change from init to set. Migration: UpdateProfileAsync(displayName: "x") becomes UpdateProfileAsync(p => p.DisplayName = "x").
• AtProtoJsonDefaults.Options is read-only. Migration: copy it, or register variants on the registry.
• LexiconTypeRegistry removals as listed above, plus IAtProtoUnion and [LexiconPlugin].
• Wire change (listed under Changed): a union variant now writes $type also outside union position, for example RecordWithMediaEmbed.Record. The data model allows it, and a $type read there is now written back rather than dropped.

Verification

• Build: dotnet build --no-incremental for the whole solution gives 0 warnings, 0 errors.

• Unit tests: 1775 passed, 0 failed, 5 skipped (1780 total). New tests:

  • OpenUnionTests: every open union reads an unknown variant as Unknown* and re-serializes byte for byte, both through the base and as its own type. Also: an unknown embed inside a timeline page, an unknown thread node among replies, an unknown Ozone event, closed-union rejection, missing or non-object $type, null, $type leading on write and never in ExtensionData, read-only and copied options, plain options, a JSON-escaped $type name, and the assembly-wide guard.
  • GalleryEmbedTests: a timeline page built from the upstream appview snapshots (packages/bsky/tests/views/__snapshots__/posts.test.ts.snap, "embeds gallery" and "embeds gallery with record") deserializes. Also: the gallery record reads and round-trips (JsonElement.DeepEquals), and a new gallery embed writes the Lexicon shape.
  • UnknownFieldTests: unknown fields at every nesting level of a PostRecord, AtProtoRecord subclasses (no $type duplication), ProfileRecord with all upstream fields plus an unknown one, and view extension data.
  • AtProtoClientUpdateProfileTests (fake PDS): unknown and all other fields preserved with swapRecord sent, null clears a field, retry after one InvalidSwap, give up after 3, create without a guard, and unauthenticated.
  • CustomUnionTests / LexiconTypeRegistryTests: the custom open union, registration on custom and SDK unions through AtProtoJsonDefaults.Options, late registration, plain [JsonPolymorphic] registration, conflicts, plugins, and a misdeclared base.
  • Signed-label and Sig round-trip, LexBase64 vectors, and a Jetstream unpadded $bytes regression.

• Timeline deserialization benchmark: BenchmarkDotNet 0.14, in-process, 5 warmup + 20 iterations, Ryzen 5 7600X, .NET 10.0.12. It deserializes FeedResponse with AtProtoJsonDefaults.Options, using the performance auditor's fixture generator.

  50-item timeline	before (origin/main)	after	Δ
  auditor fixture, 185 KB, every field modeled	298.8 µs, 389.5 KB	309.7 µs, 416.0 KB	+3.6 % time, +6.8 % alloc
  same plus 3 fields the appview sends but the SDK doesn't model (author.associated, post.bookmarkCount, viewer.bookmarked), 199 KB	322.9 µs, 405.9 KB	358.1 µs, 494.9 KB	+10.9 % time, +22 % alloc

  • First row: each union member now deserializes through a nested JsonSerializer.Deserialize, which allocates its own read-stack frames (about 0.5 KB per embed). There is no public STJ API that avoids it.
  • Second row: this is the cost of keeping unknown fields: each one is captured as a JsonElement in a per-object dictionary. It shrinks as the lexicon-fidelity work models those fields. Before this PR, a page with a single gallery post didn't deserialize at all.

Deliberately deferred

• Spaces: Lexicon/Com/AtProto/Space/* and SimpleSpace/* unions and models are untouched. A parallel PR owns them, and the guard test excludes them.
• FirehoseMessage: stays a strict [JsonPolymorphic] union, and its frame models don't derive from LexObject. These are event-stream frames transcoded from CBOR by FirehoseEventParser (owned by a parallel PR). That parser deliberately returns null for unknown frame types, as consumers should skip them, so an Unknown* variant would change its contract.
• Unions exposed as raw JsonElement (FeedViewPost.Reason, PostView.Threadgate, RecordView.Record → viewRecord.embeds, …) are not retyped here; that belongs to the lexicon-fidelity issue.
• atproto-lexgen still emits [JsonPolymorphic] unions. docs/lexicon-codegen.md now says how to make one open. Switching the emitter to [AtProtoUnion] plus an unknown variant is a follow-up.
• The registry is process-wide (LexiconTypeRegistry.Instance). A per-client registry or options (research WP3.3) would need AtProtoClientOptions plumbing through XrpcClient and RecordCollection.
• DagCborEncoder.PadBase64 is not unified with LexBase64; a later PR does that.

Edits outside this issue's core files

• src/ATProtoNet/Streaming/JetstreamEventParser.cs, JetstreamSegmentReader.cs: the base64 padding fix only, as permitted.
• src/ATProtoNet/Streaming/JetstreamModels.cs: GetRecord<T>() called the removed LexiconTypeRegistry.CreateOptions() and now uses AtProtoJsonDefaults.Options.
• src/ATProtoNet/RecordCollection.cs: AtProtoRecord : LexObject plus a remark.
• tests/ATProtoNet.Tests/Streaming/JetstreamV2EventParserTests.cs: one regression test.

🤖 Generated with Claude Code

Closes #115. Lexicon unions are open by spec and Lexicons grow optional fields, but the SDK's unions were closed and its models dropped unknown fields. One unrecognized `$type` failed the whole response, and read-modify-write lost data. This PR puts the fix in one shared layer that later PRs can build on. "Unions and unknown fields" in `docs/custom-records.md` documents the pattern. ## Open unions - **`[AtProtoUnion]`** (`Serialization/AtProtoUnionAttribute.cs`) marks an abstract union base. Variants are still declared with `[JsonDerivedType]`, so the discriminators stay next to the base. - An open union names an `Unknown{Base}` variant, a sealed subclass implementing `IUnknownUnionVariant` (`Type` + `Raw`, with a `(string, JsonElement)` constructor). - `[AtProtoUnion(Closed = true)]` keeps strict behavior. - A misdeclared base (open with no unknown variant, duplicate discriminators, and so on) fails with a clear `InvalidOperationException` the first time it is used. - **`AtProtoUnionConverterFactory`** (`Serialization/AtProtoUnionConverter.cs`) reads and writes these bases. - **Read:** it scans a copy of the reader for the top-level `$type`, wherever it sits (the appview often puts it last). It then resolves in order: the declared variants, then the registry (checked on every miss, so late registration works), then `Unknown{Base}` holding `JsonElement.ParseValue`. A closed union throws `JsonException` instead. - **Write:** an unknown variant writes its original bytes via `JsonMarshal.GetRawUtf8Value` + `WriteRawValue`, so number formatting, escapes and whitespace survive byte for byte. A known variant is written through its own contract. - **Why a converter and not `IgnoreUnrecognizedTypeDiscriminators`:** that flag falls back to the abstract base and loses the discriminator. The contract modifier `LexiconTypeRegistry.ApplyUnionContracts` makes the converter approach work: - It removes STJ's `PolymorphismOptions` from `[AtProtoUnion]` bases, because STJ refuses polymorphism metadata on a type with a custom converter. - It gives every variant a leading `$type` property that is written but never read. A variant therefore always writes its discriminator first, and an incoming `$type` is matched and skipped, so it never allocates or lands in `ExtensionData`. - It still adds registered variants to plain `[JsonPolymorphic]` bases. - **`AllowOutOfOrderMetadataProperties = true` stays**, for plain `[JsonPolymorphic]` types. Tests cover a trailing `$type` for both kinds, and a JSON-escaped `$type` property name. - **Converted to open unions:** `EmbedBase`, `EmbedView`, `FacetFeature`, `ThreadNode`, `ReportSubject`, and Ozone's `ModEventType` and `ModerationSubject` (Ozone emits 11 event types the SDK doesn't model). Each gets an `Unknown*` variant. - **Closed:** `ApplyWriteOperation`, the only closed union upstream. - **Gallery:** added `app.bsky.embed.gallery` (`GalleryEmbed`/`GalleryImage` with the open `GalleryItem` union, `GalleryView`/`GalleryViewImage` with the open `GalleryViewItem` union), registered on `EmbedBase` and `EmbedView`. That covers `feed.post#embed`, `feed.defs#postView.embed`, and `recordWithMedia#media`/`#view.media`, since both use those bases. `embed.record#viewRecord.embeds` sits inside `RecordView.Record`, which is still a raw `JsonElement`, so it is covered once the lexicon-fidelity issue types it as `List<EmbedView>`. - **Guard test:** reflection over the assembly checks that every `$type` union base is an `[AtProtoUnion]` with a valid shape, and that only `ApplyWriteOperation` is closed. The exceptions are Spaces and `FirehoseMessage`; see "Deferred". ## Unknown fields - **`LexObject`** (`Models/LexObject.cs`) provides `[JsonExtensionData] IDictionary<string, JsonElement>? ExtensionData`. It stays `null` unless an undeclared field shows up. - **Which types derive from it** (a scripted, mechanical change): `AtProtoRecord` and every SDK model of a Lexicon record, view or object, in App.Bsky, Chat, Com.AtProto, Site.Standard, Tools.Ozone and `Models/CommonModels.cs`. - Excluded: XRPC `*Request`/`*Response` envelopes, the fixed data-model shapes `BlobRef`/`BlobLink`/`CidLink`, `IdentityModels`, the firehose frame models, and Spaces. - **`$type` handling:** - `ApplyRecordTypeDiscriminator` works with extension data unchanged. The record's `$type` is a get-only contract property, so an incoming `$type` is matched and skipped rather than routed to `ExtensionData`. Tested: no duplication, and a mismatched incoming `$type` is dropped while the record's own wins on write. - The SDK's record models (`PostRecord` etc.) behave the same way through their get-only `Type`. ## `LexiconTypeRegistry` / `AtProtoJsonDefaults` - `AtProtoJsonDefaults.Options` now carries the union factory and modifier bound to `LexiconTypeRegistry.Instance`. Registered variants therefore take effect in every client, in `RecordCollection<T>`, and in Jetstream `GetRecord<T>()`, which now uses these options. - The options are frozen with `MakeReadOnly()`. The wrong XML remark is gone, and the new one points to copying: `new JsonSerializerOptions(AtProtoJsonDefaults.Options)` keeps all of the behavior above. - **Removed:** - the write-only record-type registry (`RegisterRecordType`/`GetRecordType`/`RecordTypes`) - `_plugins` - the duplicate `CreateOptions()` - `[LexiconPlugin]` + `LoadPluginsFromAssembly` (which did nothing `LoadPlugin<T>()` doesn't) - the public constructor (a second registry had no way to take effect) - the no-op `IAtProtoUnion` (its test became `CustomUnionTests`, the custom open-union test) - **Kept:** `ILexiconPlugin` + `LoadPlugin<T>()`. That is still a meaningful packaging convention for "variants my package adds to unions it doesn't own". - **Validation:** `RegisterUnionVariant` now rejects a non-union base, and a discriminator already declared or registered for another type. ## Bytes - `Label.Sig` has `[JsonConverter(typeof(LexBytesJsonConverter))]`. The test uses the `mod.bsky.app` `queryLabels` shape with a 64-byte unpadded `$bytes`. - A new `LexBase64.Decode` (padding optional) is used by `LexBytesJsonConverter`, `JetstreamEventParser` and `JetstreamSegmentReader`. The latter two used `Convert.FromBase64String`, which rejects the unpadded base64 the data model specifies. - `DagCborEncoder.PadBase64` was left alone as instructed. ## `UpdateProfileAsync` - **New shape:** `Task<RecordRef> UpdateProfileAsync(Action<ProfileRecord> update, CancellationToken)`. 1. It reads `app.bsky.actor.profile/self` (or starts from an empty record with `createdAt` when there is none). 2. It lets the callback edit the record in place. 3. It writes with `swapRecord: <cid>`. 4. On `InvalidSwap` it re-reads and re-runs the callback, up to 3 attempts in total, then rethrows. - Untouched fields, `ExtensionData` included, are written back unchanged. - `ProfileRecord` gains `Pronouns`, `Website`, `Labels` (`SelfLabels`) and `JoinedViaStarterPack`. - **Deviation from the suggested `Func<ProfileRecord, ProfileRecord>`:** the models are classes, and `LexObject` must be a class because users write `class TodoItem : AtProtoRecord`. So `with` is unavailable, and a `Func` would force callers to copy every field and `ExtensionData` by hand. `ProfileRecord`'s properties are therefore settable, and the callback is an `Action`: `p => p.DisplayName = "x"`. Setting `null` clears a field, which the old API could not do. - Only this method and its private helpers changed in `AtProtoClient.cs`. ## Breaking changes (all in CHANGELOG with migration notes) - **Union bases:** `[JsonPolymorphic]` becomes `[AtProtoUnion]`, the bases derive from `LexObject`, and new `Unknown*` variants appear. Migration: add an `Unknown*` or `_` arm to `switch`es; the compiler won't flag it. - **Models:** they now derive from `LexObject`, a binary break. A consumer record type with its own `[JsonExtensionData]` must drop it. - **`UpdateProfileAsync`:** new signature, and `ProfileRecord` setters change from `init` to `set`. Migration: `UpdateProfileAsync(displayName: "x")` becomes `UpdateProfileAsync(p => p.DisplayName = "x")`. - **`AtProtoJsonDefaults.Options` is read-only.** Migration: copy it, or register variants on the registry. - **`LexiconTypeRegistry` removals** as listed above, plus `IAtProtoUnion` and `[LexiconPlugin]`. - **Wire change (listed under Changed):** a union variant now writes `$type` also outside union position, for example `RecordWithMediaEmbed.Record`. The data model allows it, and a `$type` read there is now written back rather than dropped. ## Verification - **Build:** `dotnet build --no-incremental` for the whole solution gives **0 warnings, 0 errors**. - **Unit tests:** **1775 passed, 0 failed, 5 skipped** (1780 total). New tests: - `OpenUnionTests`: every open union reads an unknown variant as `Unknown*` and re-serializes byte for byte, both through the base and as its own type. Also: an unknown embed inside a timeline page, an unknown thread node among replies, an unknown Ozone event, closed-union rejection, missing or non-object `$type`, `null`, `$type` leading on write and never in `ExtensionData`, read-only and copied options, plain options, a JSON-escaped `$type` name, and the assembly-wide guard. - `GalleryEmbedTests`: a timeline page built from the upstream appview snapshots (`packages/bsky/tests/views/__snapshots__/posts.test.ts.snap`, "embeds gallery" and "embeds gallery with record") deserializes. Also: the gallery record reads and round-trips (`JsonElement.DeepEquals`), and a new gallery embed writes the Lexicon shape. - `UnknownFieldTests`: unknown fields at every nesting level of a `PostRecord`, `AtProtoRecord` subclasses (no `$type` duplication), `ProfileRecord` with all upstream fields plus an unknown one, and view extension data. - `AtProtoClientUpdateProfileTests` (fake PDS): unknown and all other fields preserved with `swapRecord` sent, `null` clears a field, retry after one `InvalidSwap`, give up after 3, create without a guard, and unauthenticated. - `CustomUnionTests` / `LexiconTypeRegistryTests`: the custom open union, registration on custom and SDK unions through `AtProtoJsonDefaults.Options`, late registration, plain `[JsonPolymorphic]` registration, conflicts, plugins, and a misdeclared base. - Signed-label and `Sig` round-trip, `LexBase64` vectors, and a Jetstream unpadded `$bytes` regression. - **Timeline deserialization benchmark:** BenchmarkDotNet 0.14, in-process, 5 warmup + 20 iterations, Ryzen 5 7600X, .NET 10.0.12. It deserializes `FeedResponse` with `AtProtoJsonDefaults.Options`, using the performance auditor's fixture generator. | 50-item timeline | before (`origin/main`) | after | Δ | |---|---|---|---| | auditor fixture, 185 KB, every field modeled | 298.8 µs, 389.5 KB | 309.7 µs, 416.0 KB | +3.6 % time, +6.8 % alloc | | same plus 3 fields the appview sends but the SDK doesn't model (`author.associated`, `post.bookmarkCount`, `viewer.bookmarked`), 199 KB | 322.9 µs, 405.9 KB | 358.1 µs, 494.9 KB | +10.9 % time, +22 % alloc | - **First row:** each union member now deserializes through a nested `JsonSerializer.Deserialize`, which allocates its own read-stack frames (about 0.5 KB per embed). There is no public STJ API that avoids it. - **Second row:** this is the cost of keeping unknown fields: each one is captured as a `JsonElement` in a per-object dictionary. It shrinks as the lexicon-fidelity work models those fields. Before this PR, a page with a single gallery post didn't deserialize at all. ## Deliberately deferred - **Spaces:** `Lexicon/Com/AtProto/Space/*` and `SimpleSpace/*` unions and models are untouched. A parallel PR owns them, and the guard test excludes them. - **`FirehoseMessage`:** stays a strict `[JsonPolymorphic]` union, and its frame models don't derive from `LexObject`. These are event-stream frames transcoded from CBOR by `FirehoseEventParser` (owned by a parallel PR). That parser deliberately returns `null` for unknown frame types, as consumers should skip them, so an `Unknown*` variant would change its contract. - **Unions exposed as raw `JsonElement`** (`FeedViewPost.Reason`, `PostView.Threadgate`, `RecordView.Record` → `viewRecord.embeds`, …) are not retyped here; that belongs to the lexicon-fidelity issue. - **`atproto-lexgen`** still emits `[JsonPolymorphic]` unions. `docs/lexicon-codegen.md` now says how to make one open. Switching the emitter to `[AtProtoUnion]` plus an unknown variant is a follow-up. - **The registry is process-wide** (`LexiconTypeRegistry.Instance`). A per-client registry or options (research WP3.3) would need `AtProtoClientOptions` plumbing through `XrpcClient` and `RecordCollection`. - **`DagCborEncoder.PadBase64`** is not unified with `LexBase64`; a later PR does that. ## Edits outside this issue's core files - `src/ATProtoNet/Streaming/JetstreamEventParser.cs`, `JetstreamSegmentReader.cs`: the base64 padding fix only, as permitted. - `src/ATProtoNet/Streaming/JetstreamModels.cs`: `GetRecord<T>()` called the removed `LexiconTypeRegistry.CreateOptions()` and now uses `AtProtoJsonDefaults.Options`. - `src/ATProtoNet/RecordCollection.cs`: `AtProtoRecord : LexObject` plus a remark. - `tests/ATProtoNet.Tests/Streaming/JetstreamV2EventParserTests.cs`: one regression test. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-09-24 18:11:42 +00:00

feat(serialization)!: open unions and unknown-field round-tripping (closes #115) ... d3a54d82d7

Lexicon unions are open by spec and Lexicons grow optional fields, but the
SDK's unions were closed and its models dropped what they did not declare:
one gallery post failed a whole timeline page, an unknown Ozone event type
failed queryEvents, and UpdateProfileAsync deleted profile fields.

- [AtProtoUnion] bases with an Unknown{Base} variant that keeps the raw
  object and writes it back byte-for-byte; closed unions stay strict. A
  converter factory plus a contract modifier replace STJ polymorphism, which
  cannot fall back without losing the discriminator.
- LexObject.ExtensionData on AtProtoRecord and the SDK's Lexicon models, so
  undeclared fields survive deserialize -> serialize.
- AtProtoJsonDefaults.Options consults LexiconTypeRegistry.Instance and is
  frozen; the registry's dead parts (record types, plugin scanning,
  CreateOptions) and IAtProtoUnion are removed.
- app.bsky.embed.gallery, Label.Sig as Lexicon bytes, one padding-tolerant
  base64 decoder for LexBytesJsonConverter and Jetstream.
- UpdateProfileAsync(Action<ProfileRecord>) edits the stored record in
  place, writes with swapRecord and retries on InvalidSwap.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Grandiras commented 2026-09-24 18:13:03 +00:00

Author

Owner

Copy link

@claude /review

@claude /review

Grandiras force-pushed issue-115-open-unions from d3a54d82d7 to 8d84327e18 2026-09-24 18:13:04 +00:00 Compare

claude-bot commented 2026-09-24 18:13:10 +00:00

Collaborator

Copy link

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/584); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-149/run-584.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-09-24 18:15:40 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

This is a large, carefully constructed PR and it holds up well under review. The core mechanism — AtProtoUnionShape reflecting the [AtProtoUnion] attribute once per base, AtProtoUnionConverter<TBase> scanning a copy of the reader for $type (leaving the original reader untouched for the real deserialize), and LexiconTypeRegistry.ApplyUnionContracts stripping PolymorphismOptions from union bases while injecting a leading get-only $type property on variants — is internally consistent. I traced the read/write paths for EmbedBase, GalleryItem/GalleryViewItem, ThreadNode, FacetFeature, ReportSubject/ModerationSubject, and the closed ApplyWriteOperation, and didn't find a case where a declared variant, a registered variant, and the unknown fallback resolve inconsistently between read and write.

JsonMarshal.GetRawUtf8Value (used in UnknownUnionVariant.WriteRaw) is a real API — confirmed via web search it lives in System.Runtime.InteropServices and was added in .NET 9, so it's available on this repo's net10.0 target; the file correctly has using System.Runtime.InteropServices;.

UpdateProfileAsync's retry loop (AtProtoClient.cs) is correct: three total attempts, re-reading and re-running the callback each time, rethrowing on the third InvalidSwap. The create path (no existing profile) correctly passes a null CID as "no swap guard". The contract-modifier ordering (ApplyRecordTypeDiscriminator then registry.ApplyUnionContracts) doesn't double-add a $type property, since ApplyUnionContracts only adds one when the type is an actual union variant (AtProtoUnionShape.FindOwner returns non-null), which plain AtProtoRecord subclasses never are.

I didn't find correctness bugs worth blocking on. Two minor, non-blocking observations:

1. LexiconTypeRegistry.RegisterUnionVariant doesn't reject registering a variant against a closed [AtProtoUnion(Closed = true)] base (only ApplyWriteOperation today) — the read path in AtProtoUnionConverter<TBase>.Read checks the registry before the shape.Closed check, so a registered variant would silently work even though the Lexicon says the union is closed. This is presumably intentional (a consumer overriding closed-ness deliberately) but it's undocumented and untested; worth a one-line note in the XML docs on RegisterUnionVariant or Closed.
2. The gallery Media property on RecordWithMediaEmbed and the general EmbedBase/GalleryItem unions have no runtime restriction preventing e.g. a RecordEmbed from being assigned as Media — but this mirrors the pre-existing lack of enforcement (previously just a doc comment), so it's not a regression.

Overall the design matches the PR description precisely, the breaking changes are all documented with migration notes in CHANGELOG.md, and the docs (custom-records.md, lexicon-codegen.md) were updated consistently with the code.

---

⏱ 2m 13s · 4 turns · tokens: 92.9k in / 11.8k out (+355.5k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, carefully constructed PR and it holds up well under review. The core mechanism — `AtProtoUnionShape` reflecting the `[AtProtoUnion]` attribute once per base, `AtProtoUnionConverter<TBase>` scanning a *copy* of the reader for `$type` (leaving the original reader untouched for the real deserialize), and `LexiconTypeRegistry.ApplyUnionContracts` stripping `PolymorphismOptions` from union bases while injecting a leading get-only `$type` property on variants — is internally consistent. I traced the read/write paths for `EmbedBase`, `GalleryItem`/`GalleryViewItem`, `ThreadNode`, `FacetFeature`, `ReportSubject`/`ModerationSubject`, and the closed `ApplyWriteOperation`, and didn't find a case where a declared variant, a registered variant, and the unknown fallback resolve inconsistently between read and write. `JsonMarshal.GetRawUtf8Value` (used in `UnknownUnionVariant.WriteRaw`) is a real API — confirmed via web search it lives in `System.Runtime.InteropServices` and was added in .NET 9, so it's available on this repo's net10.0 target; the file correctly has `using System.Runtime.InteropServices;`. `UpdateProfileAsync`'s retry loop (`AtProtoClient.cs`) is correct: three total attempts, re-reading and re-running the callback each time, rethrowing on the third `InvalidSwap`. The create path (no existing profile) correctly passes a null CID as "no swap guard". The contract-modifier ordering (`ApplyRecordTypeDiscriminator` then `registry.ApplyUnionContracts`) doesn't double-add a `$type` property, since `ApplyUnionContracts` only adds one when the type is an actual union variant (`AtProtoUnionShape.FindOwner` returns non-null), which plain `AtProtoRecord` subclasses never are. I didn't find correctness bugs worth blocking on. Two minor, non-blocking observations: 1. `LexiconTypeRegistry.RegisterUnionVariant` doesn't reject registering a variant against a *closed* `[AtProtoUnion(Closed = true)]` base (only `ApplyWriteOperation` today) — the read path in `AtProtoUnionConverter<TBase>.Read` checks the registry before the `shape.Closed` check, so a registered variant would silently work even though the Lexicon says the union is closed. This is presumably intentional (a consumer overriding closed-ness deliberately) but it's undocumented and untested; worth a one-line note in the XML docs on `RegisterUnionVariant` or `Closed`. 2. The gallery `Media` property on `RecordWithMediaEmbed` and the general `EmbedBase`/`GalleryItem` unions have no runtime restriction preventing e.g. a `RecordEmbed` from being assigned as `Media` — but this mirrors the pre-existing lack of enforcement (previously just a doc comment), so it's not a regression. Overall the design matches the PR description precisely, the breaking changes are all documented with migration notes in `CHANGELOG.md`, and the docs (`custom-records.md`, `lexicon-codegen.md`) were updated consistently with the code. --- ⏱ 2m 13s · 4 turns · tokens: 92.9k in / 11.8k out (+355.5k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-149/run-584.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:8d84327e1874548455b5cd3ed3fb9c9330e49bbd -->

Grandiras force-pushed issue-115-open-unions from 8d84327e18 to 8003a20cb4 2026-09-24 18:15:42 +00:00 Compare

Grandiras commented 2026-09-24 18:16:58 +00:00

Author

Owner

Copy link

Rebased onto main to resolve a CHANGELOG-only conflict (#146 merged). verify-rebase: the PR's patch outside CHANGELOG.md is byte-identical between the approved head 8d84327e and the new head 8003a20c. CI is green on the new head (and 2473 unit tests pass locally), so the approval carries over under the maintainer-approved policy for CHANGELOG-only rebases. The reviewer's closed-union registration note is tracked for the follow-up that migrates the Space unions (#133).

Rebased onto main to resolve a CHANGELOG-only conflict (#146 merged). `verify-rebase`: the PR's patch outside `CHANGELOG.md` is byte-identical between the approved head `8d84327e` and the new head `8003a20c`. CI is green on the new head (and 2473 unit tests pass locally), so the approval carries over under the maintainer-approved policy for CHANGELOG-only rebases. The reviewer's closed-union registration note is tracked for the follow-up that migrates the Space unions (#133).

Grandiras merged commit 00c2d931af into main 2026-09-24 18:16:59 +00:00

Grandiras deleted branch issue-115-open-unions 2026-09-24 18:17:00 +00:00

Grandiras referenced this pull request from a commit 2026-09-24 18:17:01 +00:00

feat(serialization)!: forward-compatible models — open unions and unknown-field round-tripping (#149)

Grandiras referenced this pull request 2026-09-24 18:17:04 +00:00

fix(spaces): protocol hardening, performance and a smaller server surface #133

Grandiras referenced this pull request 2026-09-24 18:19:36 +00:00

perf(crypto): vectorized BLAKE3 / LtHash and cached did:key parsing #150

Grandiras referenced this pull request 2026-09-24 19:05:04 +00:00

refactor(http)!: one error model and a robust XRPC transport #152

Sign in to join this conversation.

Reviewers

No reviewers

claude-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!149

No description provided.