-
Notifications
You must be signed in to change notification settings - Fork 2
Fix Parameters not being deserialized with System.Text.Json #48
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
… init accessors 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 enables System.Text.Json to populate Parameters collections by adding init accessors and adds tests to verify correct deserialization of method and constructor parameters.
- Switched
Parametersproperties inMethodDescriptionandConstructorDescriptionfrom getter-only toget; init;. - Added
TextJsonDeserializationTeststo cover method and constructor parameter deserialization.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/DendroDocs.Shared/Descriptions/MethodDescription.cs | Changed Parameters to use get; init; |
| src/DendroDocs.Shared/Descriptions/ConstructorDescription.cs | Changed Parameters to use get; init; |
| tests/DendroDocs.Shared.Tests/Serialization/TextJsonDeserializationTests.cs | Added tests for method & constructor parameters |
Comments suppressed due to low confidence (1)
tests/DendroDocs.Shared.Tests/Serialization/TextJsonDeserializationTests.cs:484
- Consider adding equivalent tests to verify Newtonsoft.Json deserialization for method and constructor parameters to guard against regressions in JSON.NET scenarios.
[TestMethod]
|
Adding equivalent tests to verify Newtonsoft.Json deserialization for method and constructor parameters to guard against regressions in JSON.NET scenarios. |
…regressions Co-authored-by: eNeRGy164 <10671831+eNeRGy164@users.noreply.github.com>
Added equivalent Newtonsoft.Json deserialization tests for both method and constructor parameters with attributes. The tests use the same JSON structure and assertions as the System.Text.Json tests to ensure consistent behavior across both serialization libraries. (66ede2d) |
|
I see 2 approaches emerging in the code base. Using (Json)Constructors and To keep consistency for now, I would opt to use constructors for now. |
Co-authored-by: eNeRGy164 <10671831+eNeRGy164@users.noreply.github.com>
Changed to use the constructor approach for consistency. Added JSON constructors that accept |
|
The JsonConstructors should only be used by the Json deserialization, there should be no other usage, so there should be no breaking change to just modify them. Maybe they should be internal instead of public? This might be out of scope at the moment. Check if the constructors where used anywhere in the code base, and if not, don't introduce new ones, just modify them. |
…istency Co-authored-by: eNeRGy164 <10671831+eNeRGy164@users.noreply.github.com>
Modified the existing JSON constructors to accept both parameters and statements instead of adding new ones. Removed the separate constructors that only took statements since they weren't used elsewhere in the codebase. This maintains consistency with the established pattern while keeping the JSON constructors solely for deserialization purposes. (3a6dcd6) |
Pull Request Test Coverage Report for Build 16018266506Details
💛 - Coveralls |
This PR fixes an issue where
Parameterscollections on methods and constructors were not being properly deserialized when using System.Text.Json, whileAttributescollections worked correctly.Problem
When deserializing JSON with System.Text.Json, the
Parametersproperty onMethodDescriptionandConstructorDescriptionwould remain empty, even when the JSON contained parameter data:{ "FullName": "Test.Controller", "Methods": [{ "Name": "RegisterAsync", "Parameters": [{ "Type": "Commands.RegisterCustomer", "Name": "command", "Attributes": [{ "Type": "Microsoft.AspNetCore.Mvc.FromBodyAttribute", "Name": "FromBody" }] }] }] }After deserialization:
method.Attributeswould be populated correctlymethod.Parameterswould be empty (Count = 0)Root Cause
The issue was that
Parametersproperties were defined with{ get; }accessors only:While
Attributesproperties had{ get; init; }accessors:System.Text.Json with
JsonObjectCreationHandling.Populatecannot deserialize into properties that only havegetaccessors - they need eithersetorinitaccessors to be populated during deserialization.Solution
Changed the
Parametersproperties to use{ get; init; }accessors:MethodDescription.ParametersConstructorDescription.ParametersTesting
Added comprehensive tests to verify both method and constructor parameter deserialization:
MethodParameters_Should_BeDeserializedCorrectly- Tests method parameters with attributesConstructorParameters_Should_BeDeserializedCorrectly- Tests constructor parameters with attributesAll existing tests continue to pass, confirming that:
Fixes #47.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.