-
Notifications
You must be signed in to change notification settings - Fork 2
Fix InvocationDescription Arguments deserialization for both JSON serializers #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ation Co-authored-by: eNeRGy164 <10671831+eNeRGy164@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the deserialization of the Arguments property on InvocationDescription for both System.Text.Json and Newtonsoft.Json by introducing a dedicated constructor and converter attributes, and adds regression tests to verify behavior.
- Added a
JsonConstructoroverload onInvocationDescriptionthat accepts an argument list and populates theArgumentscollection. - Applied Newtonsoft converter attributes to the
Argumentsproperty for proper type handling. - Introduced comprehensive TextJson and Newtonsoft.Json tests to cover simple and complex invocation scenarios.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/DendroDocs.Shared/Descriptions/InvocationDescription.cs | Added a JsonConstructor overload, converter attributes, and import |
| tests/.../TextJsonDeserializationTests.cs | Added TextJson tests for invocation argument deserialization |
| tests/.../NewtonsoftDeserializationTests.cs | Added Newtonsoft.Json tests for invocation argument deserialization |
Comments suppressed due to low confidence (1)
src/DendroDocs.Shared/Descriptions/InvocationDescription.cs:40
- The
[]initializer is valid only for arrays. ForList<ArgumentDescription>, use= new()or an explicit collection initializer to avoid a compilation error.
public List<ArgumentDescription> Arguments { get; } = [];
Pull Request Test Coverage Report for Build 16025606938Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 16025607078Details
💛 - Coveralls |
The
Argumentsproperty onInvocationDescriptionwas not being deserialized correctly when using either System.Text.Json or Newtonsoft.Json. This occurred because the class only had a primary constructor for basic parameters but lacked aJsonConstructorto handle theArgumentscollection during deserialization.Problem
Given JSON like this:
{ "$type": "DendroDocs.InvocationDescription, DendroDocs.Shared", "ContainingType": "Pitstop.Infrastructure.Messaging.IMessagePublisher", "Name": "PublishMessageAsync", "Arguments": [ { "Type": "string", "Text": "e.MessageType" }, { "Type": "Pitstop.CustomerManagementAPI.Events.CustomerRegistered", "Text": "e" } ] }The
Argumentscollection would remain empty after deserialization, resulting in loss of important method call information.Solution
Added a
JsonConstructortoInvocationDescriptionfollowing the established pattern used byConstructorDescriptionandMethodDescription:argumentsparameter to the constructor that populates theArgumentscollectionConcreteTypeConverterattribute for Newtonsoft.Json compatibilityChanges Made
The fix is minimal, follows existing codebase patterns, and maintains full backward compatibility while enabling proper deserialization of method invocations with arguments.
Fixes #50.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.