Add IsoDateTimeConverter and StringEnumConverter#109
Conversation
WalkthroughUpdated Newtonsoft.Json defaults: added IsoDateTimeConverter and StringEnumConverter, changed CamelCaseNamingStrategy constructor arguments, and set serializer settings to ignore null values; a unit test JSON payload injection was adjusted and two unused usings were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client code
participant RestClient as RestClient
participant JsonNet as Newtonsoft.Json
rect rgba(200,230,255,0.4)
Caller->>RestClient: Post object (e.g., request DTO)
RestClient->>JsonNet: Serialize(object, settings)
note right of JsonNet #eef7ff: Settings include\n- StringEnumConverter\n- IsoDateTimeConverter\n- SimpleValueObjectConverter\n- CamelCaseNamingStrategy(false,false)\n- NullValueHandling.Ignore
JsonNet-->>RestClient: JSON payload
RestClient->>Caller: send HTTP request with JSON
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs
🧬 Code graph analysis (1)
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs (1)
Activout.RestClient.Json/SimpleValueObjectConverter.cs (1)
JsonConverter(40-50)
🔇 Additional comments (4)
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs (4)
3-3: LGTM! Necessary import for the new converters.
25-25: LGTM! More explicit and conservative naming strategy.The explicit parameters
(false, false)make the behavior clearer and prevent unintended dictionary key transformations and JsonProperty attribute overrides.
37-37: Verify this potentially breaking change.Adding
NullValueHandling.Ignorechanges serialization behavior by omitting null values from the JSON output. This is common practice but could break consumers that explicitly check for null properties versus missing properties.Ensure this change is:
- Documented in release notes as a breaking change
- Tested with existing API consumers
- Intentional for this settings object
28-31: Clarify the intentional difference in NullValueHandling.
DefaultJsonSerializerSettingsdoes not haveNullValueHandling.Ignore, whileCamelCaseSerializerSettingsdoes (line 37). This creates inconsistent serialization behavior between the two settings objects.Is this intentional? If so, the distinction should be documented. If not, consider adding it here as well:
public static readonly JsonSerializerSettings DefaultJsonSerializerSettings = new() { - Converters = DefaultJsonConverters.ToList() + Converters = DefaultJsonConverters.ToList(), + NullValueHandling = NullValueHandling.Ignore };Note: The AI summary mentions adding
NullValueHandling.IgnoretoDefaultJsonSerializerSettings, but this change is not present in the code.
twogood
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (1)
200-200: Consider a more robust JSON manipulation approach.The string replacement of the first
{character adapts correctly to the newNullValueHandling.Ignorebehavior but is fragile—it assumes the JSON structure and would break if the request contains nested objects or arrays. Consider parsing, modifying, and re-serializing the JSON instead:- content = content.Replace("{", "{\"ReviewId\":\"*REVIEW_ID*\", "); + var json = JsonConvert.DeserializeObject<dynamic>(content); + json.ReviewId = "*REVIEW_ID*"; + content = JsonConvert.SerializeObject(json);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs(2 hunks)Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs(0 hunks)
💤 Files with no reviewable changes (1)
- Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.csActivout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs
🧬 Code graph analysis (1)
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs (1)
Activout.RestClient.Json/SimpleValueObjectConverter.cs (1)
JsonConverter(40-50)
🔇 Additional comments (5)
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDefaults.cs (5)
3-3: LGTM!The import is necessary for
IsoDateTimeConverterandStringEnumConverter.
24-24: LGTM!Making the
CamelCaseNamingStrategyparameters explicit (processDictionaryKeys: false,overrideSpecifiedNames: false) improves clarity without changing behavior.
35-41: LGTM! Past review comment addressed.The
StringEnumConverternow correctly usesCamelCasePropertyNamesContractResolver.NamingStrategy, ensuring enum values are serialized in camelCase to match property naming. This addresses the previous review concern. The use of spread syntax (..DefaultJsonConverters) follows the coding guidelines for collection syntax.Note:
NullValueHandling.Ignoreis a breaking change (see comment on lines 29-30).Based on past review comment.
29-30: The NullValueHandling.Ignore default is confirmed intentional—no external breaking impact found.Git history (commit 0b17940) explicitly shows this as a deliberate design decision. The test suite comprehensively validates both
IncludeandIgnorebehaviors across both JSON implementations with conditional handling at line 263–265, indicating maintainers are aware of the behavioral difference.No CHANGELOG, deprecation warnings, or migration guidance exists in the repository. The codebase shows no documented external API consumers or version-controlled breaking change protocols. While the change is technically a semantic shift in null serialization, evidence within the repository suggests this is an intentional API evolution rather than an oversight.
If external consumers exist outside this repository, they would require migration guidance for null value expectations—verify whether such consumers are documented elsewhere.
16-20: Both concerns verified as non-issues; code change is correct.The
SimpleValueObjectConverteris the correct Newtonsoft.Json-compatible version from theActivout.RestClient.Newtonsoft.Jsonnamespace (confirmed by file path andJsonConverterbase class inheritance).The
IsoDateTimeConverteraddition is not a breaking change. The existing testTestQueryParamAsyncalready expects DateTime values to be serialized in ISO 8601 format (2017-01-01T00:00:00.0000000Z), which is exactly whatIsoDateTimeConverterproduces. This indicates the change aligns with the library's intended behavior.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests