Claude: changes for issue #49 #57
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!57
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-49"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Requested in #49.
AtProtoRecord.Type is abstract and carries [JsonPropertyName("$type")] on the base member. System.Text.Json neither inherits that attribute through an override nor collapses the base member and the override into one contract property, so a record declared the documented way public override string Type => "com.example.todo.item"; serialized as BOTH "$type" (from the base member) and a stray camelCased "type" (from the override) — polluting records that other AT Protocol apps read. Add AtProtoJsonDefaults.ApplyRecordTypeDiscriminator, a JsonTypeInfo contract modifier that collapses every property mapping to the AtProtoRecord.Type virtual slot down to one, named "$type" and emitted first. Wire it into AtProtoJsonDefaults.Options and LexiconTypeRegistry.CreateOptions(), so RecordCollection<T> and RepoClient writes are fixed with no consumer change. The member is public so apps serializing records with hand-built JsonSerializerOptions can add it to DefaultJsonTypeInfoResolver.Modifiers. Matching on the virtual slot (getter GetBaseDefinition) rather than the member name leaves an unrelated `new`-shadowing Type member alone, and re-declaring [JsonPropertyName("$type")] on the override — the workaround consumers adopted — stays safe instead of colliding with the base member. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.
@claude-bot Please fix the changelog conflict.
🤖 On it — follow along in the action run; the reply will link the full session transcript.
Fixed — the branch is merged with
mainand pushed (claude/issue-49, merge commit4f3bf7f).What the conflict was:
mainhad moved ahead with #55 (issue #46, removing the no-opUnionJsonConverterFactory), which added its own bullet at the top ofCHANGELOG.md's### Fixedsection — exactly where this PR added the #49 bullet. A pure adjacent-add conflict, no semantic overlap.Resolution: kept both bullets — the #49 discriminator fix first, then the #46
IAtProtoUnionfix — with the rest of theFixedlist unchanged.The two source files both branches touch (
AtProtoJsonDefaults.cs,LexiconTypeRegistry.cs) auto-merged cleanly and the result is coherent:main's removal of the union converter registration and this PR'sApplyRecordTypeDiscriminatormodifier land side by side in bothCreateOptions()paths.Verified:
dotnet build -p:EnableSourceControlManagerQueries=false— 0 errors (had to install the .NET 10 SDK in this container first; warnings are all pre-existing CS1591/NU1902 noise)dotnet test tests/ATProtoNet.Tests/ -p:EnableSourceControlManagerQueries=false— 1010 passed, 0 failed, covering both this PR'sRecordTypeDiscriminatorTestsandmain'sAtProtoUnionMarkerTestsThe PR diff against
mainis now just the 6 intended files (+276/−3).⏱ 1m 58s · 16 turns · tokens: 25.8k in / 3.6k out (+518.8k cache-read)
📜 Full session transcript
📝 Retroactive review summary (was stuck as an invisible draft by the APPROVE/APPROVED bug, fixed in v12.2 — verdict was: approve):
Solid, well-scoped fix for the stray
type/$typeduplication caused by System.Text.Json not inheriting[JsonPropertyName]through property overrides. TheApplyRecordTypeDiscriminatorcontract modifier correctly scopes itself toAtProtoRecord-derivedJsonTypeInfoKind.Objectcontracts, andIsTypeDiscriminatoridentifies the discriminator by resolving the virtual slot (GetBaseDefinition().DeclaringType == typeof(AtProtoRecord)) rather than by name, which correctly avoids false positives on an unrelatedType/new-shadowed member (covered by theLabelledItemtest) while still collapsing the base member and the override (covered byTodoItem) as well as the pre-existing workaround of re-declaring[JsonPropertyName("$type")]on the override (covered byAnnotatedItem, which asserts no collision with the base member).Good catches:
LexiconTypeRegistry.CreateOptions()was also updated to include the new modifier alongsideApplyUnionVariants, so the fix applies uniformly across both serializer-options construction paths (verified bySerialize_ThroughTypeRegistryOptions_WritesDollarTypeAndNoStrayType).RecordCollectionTests.AtProtoRecord_SerializesWithDollarTypetest was fixed to actually serialize withAtProtoJsonDefaults.Optionsinstead of the process-wide default options — previously it wasn't exercising the SDK's configured serializer at all, so it couldn't have caught this bug.docs/custom-records.mdare all updated in the same commit per repo convention, and clearly explain the STJ quirk plus the escape hatch for hand-builtJsonSerializerOptions.The new test suite (
RecordTypeDiscriminatorTests) is thorough — covers plain overrides, the boxed-objectserialization path actually used byCreateRecordAsync/PutRecordAsync, property ordering ($typefirst), deserialization round-trip, and the standalone-modifier usage pattern. I did not executedotnet testin this sandbox (bash access was restricted), but the diff is internally consistent and the reasoning aboutGetBaseDefinition()correctly identifying the shared virtual slot (so both properties always report the same value, making the "keep first, discard rest" collapse safe) holds up under inspection. No correctness, security, or omission concerns found.⏱ 1m 16s · 5 turns · tokens: 19.9k in / 6.3k out (+218.8k cache-read)
📜 Full session transcript