Use STJ's polymorphism support instead of custom converter#70
Use STJ's polymorphism support instead of custom converter#70SteveSandersonMS merged 3 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request modernizes the JSON serialization for session events by replacing a custom polymorphic JSON converter with System.Text.Json's built-in polymorphism support. The changes improve NativeAOT compatibility, add proper nullable annotations to reference types, and mark properties as required when they are required by the schema.
Changes:
- Replaced custom
SessionEventConverterwith[JsonPolymorphic]and[JsonDerivedType]attributes on the baseSessionEventclass - Added nullable annotations (
?) to optional reference types and array properties - Added
requiredmodifier to properties that are required in the schema - Updated enum declarations with
[JsonConverter(typeof(JsonStringEnumConverter<T>))]and[JsonStringEnumMemberName]attributes for NativeAOT compatibility - Updated usage code to use pattern matching instead of string-based type checking
- Removed CS8618 warning suppression
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nodejs/scripts/generate-csharp-session-types.ts | Updated generator to add nullable annotations to optional types, add required modifier to required properties, generate JsonPolymorphic attributes instead of custom converter, and add NativeAOT-friendly enum attributes |
| dotnet/src/Generated/SessionEvents.cs | Regenerated file reflecting all generator changes - uses JsonPolymorphic attributes, has proper nullable annotations, required modifiers on properties, and NativeAOT-compatible enum declarations |
| dotnet/src/Session.cs | Updated implementation code to use pattern matching (case AssistantMessageEvent:) instead of string comparisons (evt.Type == "assistant.message") and updated documentation examples |
| dotnet/src/Client.cs | Removed unused imports (Newtonsoft.Json.Linq, System.Text.Json.Nodes) and simplified serializer options by removing converter loop |
Comments suppressed due to low confidence (2)
dotnet/src/Session.cs:206
- The code uses pattern matching without capturing variables (
case AssistantMessageEvent:), but then tries to access type-specific properties (evt.Data?.Content). Sinceevtis still of typeSessionEvent, this will not compile. The code should use pattern matching with variable declarations, for example:case AssistantMessageEvent assistantMessage: Console.WriteLine($"Assistant: {assistantMessage.Data?.Content}");
/// switch (evt)
/// {
/// case AssistantMessageEvent:
/// Console.WriteLine($"Assistant: {evt.Data?.Content}");
/// break;
/// case SessionErrorEvent:
/// Console.WriteLine($"Error: {evt.Data?.Message}");
/// break;
dotnet/src/Session.cs:335
- The code checks if
evt is AssistantMessageEventbut then tries to accessevt.Data?.Contentwithout casting. Sinceevtis still of typeSessionEvent, which doesn't have aDataproperty with aContentfield, this will not compile. The code should use pattern matching with a variable declaration, for example:if (evt is AssistantMessageEvent assistantMessage) Console.WriteLine($"Assistant: {assistantMessage.Data?.Content}");
/// if (evt is AssistantMessageEvent)
/// {
/// Console.WriteLine($"Assistant: {evt.Data?.Content}");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Also: - Mark properties as required if they're required in the schema - Mark reference types as nullable if they're optional in the schema - Remove the suppression of CS8618, which is resulting in public types having incorrect nullable annotations - Annotate enums so the NativeAOT-unfriendly non-generic enum converter isn't needed
fa75e5c to
b595685
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Thanks very much @stephentoub!
Also: