V9.0.0/net9rc1 support #2
Conversation
WalkthroughThe changes introduce new methods and classes for JSON conversion in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
==========================================
- Coverage 73.41% 67.38% -6.03%
==========================================
Files 25 26 +1
Lines 884 1015 +131
Branches 111 132 +21
==========================================
+ Hits 649 684 +35
- Misses 233 329 +96
Partials 2 2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (8)
.nuget/Codebelt.Extensions.Newtonsoft.Json.App/PackageReleaseNotes.txt (1)
8-8: Consider removing the previous version release notes.To improve clarity and readability, consider removing the release notes for the previous version (8.4.0) from this file. Keeping the release notes focused on the current version (9.0.0) will make it easier for users to find the relevant information.
.nuget/Codebelt.Extensions.Newtonsoft.Json/PackageReleaseNotes.txt (3)
4-6: Communicate the breaking change to users.Updating dependencies and removing support for .NET 6 (LTS) aligns with the PR objectives. However, removing .NET 6 support is a breaking change. Please ensure that this is clearly communicated to users in the release notes and documentation.
8-9: Fix the typo in the description.The addition of the
FailureConverterclass aligns with the PR objective of introducing new features. However, there seems to be a typo in the description. It should be "to convertFailureto JSON" instead of "to convertFailureConverterto JSON".
14-15: Communicate the breaking change to users.The changes made to the
ValidatorExtensionsclass to comply with the quality analysis rule are a good practice. However, since these changes result in a breaking change, please ensure that this is clearly communicated to users in the release notes and documentation.Directory.Build.props (1)
52-52: Nitpick: UpdateTargetFrameworksfor test projects on Linux to maintain consistency.The
TargetFrameworksproperty for test projects on Linux has been updated to includenet9.0and removenet6.0. This change is consistent with the update made to theTargetFrameworksfor non-test projects.CHANGELOG.md (2)
22-22: Suggestion: Format the URL as a link.While the current format is correct according to the Markdown specification, it is a good practice to format URLs as links to improve readability and make them clickable.
Consider applying this change:
-ValidatorExtensions class in the Codebelt.Extensions.Newtonsoft.Json namespace to be compliant with https://rules.sonarsource.com/csharp/type/Bug/RSPEC-3343/ (breaking change) +ValidatorExtensions class in the Codebelt.Extensions.Newtonsoft.Json namespace to be compliant with [RSPEC-3343](https://rules.sonarsource.com/csharp/type/Bug/RSPEC-3343/) (breaking change)Tools
Markdownlint
22-22: null
Bare URL used(MD034, no-bare-urls)
16-17: Suggestion: Clarify the description of theFailureConverterclass's purpose.The current description states that the
FailureConverterclass is used to convertFailureConverterto JSON, which seems to be a typo or inconsistency. It is likely that the class is used to convertFailureobjects to JSON, notFailureConverterobjects.Consider applying this change:
-FailureConverter class in the Cuemon.Extensions.Newtonsoft.Json.Converters namespace to convert FailureConverter to JSON +FailureConverter class in the Cuemon.Extensions.Newtonsoft.Json.Converters namespace to convert Failure objects to JSONsrc/Codebelt.Extensions.Newtonsoft.Json/Converters/FailureConverter.cs (1)
122-126: Clarify theCanReadproperty behavior.The
CanReadproperty is overridden to returnfalse, indicating that the converter does not support reading JSON. This is acceptable given thatReadJsonthrows aNotImplementedException, but it would be helpful to add a summary comment explaining this behavior for clarity.Add the following summary comment above the
CanReadproperty:/// <summary> /// Gets a value indicating whether this converter can read JSON. Returns <c>false</c> as reading is not implemented. /// </summary>
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (30)
- .docfx/api/namespaces/Codebelt.Extensions.AspNetCore.Newtonsoft.Json.Converters.md (1 hunks)
- .docfx/api/namespaces/Codebelt.Extensions.Newtonsoft.Json.Converters.md (1 hunks)
- .github/workflows/pipelines.yml (1 hunks)
- .nuget/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json/PackageReleaseNotes.txt (1 hunks)
- .nuget/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/PackageReleaseNotes.txt (1 hunks)
- .nuget/Codebelt.Extensions.Newtonsoft.Json.App/PackageReleaseNotes.txt (1 hunks)
- .nuget/Codebelt.Extensions.Newtonsoft.Json/PackageReleaseNotes.txt (1 hunks)
- CHANGELOG.md (1 hunks)
- Directory.Build.props (3 hunks)
- src/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.csproj (3 hunks)
- src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs (1 hunks)
- src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Codebelt.Extensions.AspNetCore.Newtonsoft.Json.csproj (2 hunks)
- src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Converters/JsonConverterCollectionExtensions.cs (4 hunks)
- src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Formatters/ServiceCollectionExtensions.cs (2 hunks)
- src/Codebelt.Extensions.Newtonsoft.Json.App/Codebelt.Extensions.Newtonsoft.Json.App.csproj (1 hunks)
- src/Codebelt.Extensions.Newtonsoft.Json/Codebelt.Extensions.Newtonsoft.Json.csproj (1 hunks)
- src/Codebelt.Extensions.Newtonsoft.Json/Converters/FailureConverter.cs (1 hunks)
- src/Codebelt.Extensions.Newtonsoft.Json/Converters/JsonConverterCollectionExtensions.cs (1 hunks)
- src/Codebelt.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs (1 hunks)
- src/Codebelt.Extensions.Newtonsoft.Json/ValidatorExtensions.cs (2 hunks)
- test/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests.csproj (1 hunks)
- test/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests/Converters/JsonConverterCollectionExtensionsTest.cs (2 hunks)
- test/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests/JsonSerializationInputFormatterTest.cs (1 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Codebelt.Extensions.Newtonsoft.Json.Tests.csproj (1 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Converters/JsonConverterCollectionExtensionsTest.cs (0 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Formatters/NewtonsoftJsonFormatterOptionsTest.cs (1 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Formatters/NewtonsoftJsonFormatterTest.cs (0 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/JDataTest.cs (0 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Serialization/ContractResolverExtensionsTest.cs (1 hunks)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/ValidatorExtensionsTest.cs (1 hunks)
Files not reviewed due to no reviewable changes (3)
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Converters/JsonConverterCollectionExtensionsTest.cs
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Formatters/NewtonsoftJsonFormatterTest.cs
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/JDataTest.cs
Files skipped from review due to trivial changes (2)
- test/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests/JsonSerializationInputFormatterTest.cs
- test/Codebelt.Extensions.Newtonsoft.Json.Tests/Serialization/ContractResolverExtensionsTest.cs
Additional context used
Markdownlint
CHANGELOG.md
22-22: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (43)
test/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests.csproj (1)
4-4: Target framework update looks good, verify .NET 6.0 removal is intentional.Updating the target frameworks to include .NET 9.0 aligns with the PR objective and ensures the project can leverage the latest features.
However, removing support for .NET 6.0 is a breaking change. Please verify that this removal is intentional and properly documented in the release notes and changelog to inform users about the compatibility implications.
.docfx/api/namespaces/Codebelt.Extensions.AspNetCore.Newtonsoft.Json.Converters.md (1)
15-15: LGTM!The documentation update is consistent with the AI-generated summary and follows the existing documentation structure. The addition of the
AddProblemDetailsConvertermethod to the list of methods for theJsonConvertertype is accurate and unlikely to have any negative impact..docfx/api/namespaces/Codebelt.Extensions.Newtonsoft.Json.Converters.md (1)
15-15: Documentation update looks good!The addition of the
AddFailureConvertermethod to the table of extension methods for theJsonConvertertype accurately reflects the expanded functionality and keeps the documentation up to date.test/Codebelt.Extensions.Newtonsoft.Json.Tests/Codebelt.Extensions.Newtonsoft.Json.Tests.csproj (3)
12-12: Verify compatibility of the updatedCodebelt.Extensions.Xunitpackage.The
Codebelt.Extensions.Xunitpackage has been updated to a new major version (9.x.x) and a preview release (9.0.0-preview.2). This update could potentially introduce breaking changes or compatibility issues.Please ensure that:
- The updated package version is compatible with the other dependencies in the project.
- The updated package version is compatible with the target framework of the project.
- All usages of the
Codebelt.Extensions.Xunitpackage in the project are compatible with the new version.Consider running the tests and performing thorough testing to verify that the update does not introduce any issues.
13-13: Verify compatibility of the updatedCuemon.Extensions.Corepackage.The
Cuemon.Extensions.Corepackage has been updated to a new major version (9.x.x) and a preview release (9.0.0-preview.7). This update could potentially introduce breaking changes or compatibility issues.Please ensure that:
- The updated package version is compatible with the other dependencies in the project.
- The updated package version is compatible with the target framework of the project.
- All usages of the
Cuemon.Extensions.Corepackage in the project are compatible with the new version.Consider running the tests and performing thorough testing to verify that the update does not introduce any issues.
14-14: Verify compatibility of the updatedCuemon.Extensions.IOpackage.The
Cuemon.Extensions.IOpackage has been updated to a new major version (9.x.x) and a preview release (9.0.0-preview.7). This update could potentially introduce breaking changes or compatibility issues.Please ensure that:
- The updated package version is compatible with the other dependencies in the project.
- The updated package version is compatible with the target framework of the project.
- All usages of the
Cuemon.Extensions.IOpackage in the project are compatible with the new version.Consider running the tests and performing thorough testing to verify that the update does not introduce any issues.
src/Codebelt.Extensions.Newtonsoft.Json/Codebelt.Extensions.Newtonsoft.Json.csproj (1)
13-14: Package versions updated to support .NET 9.0The
Cuemon.CoreandCuemon.IOpackage versions have been updated to9.0.0-preview.7, which aligns with the PR objective of supporting .NET 9.0. This ensures compatibility with the latest releases of those dependencies.However, please note that using preview versions may introduce instability or unexpected behavior if those packages contain bugs or breaking changes. Thoroughly test the project with these updated dependencies to ensure compatibility and identify any potential issues.
To verify the compatibility of the updated packages, consider running the following tests:
If any issues are identified during testing, consider opening a new issue to track and address them separately.
src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Bootstrapper.cs (1)
23-23: LGTM!The addition of the
AddProblemDetailsConverterto the list of default converters forNewtonsoftJsonFormatterOptionsis a valuable enhancement. It extends the JSON formatter's functionality to support the serialization ofProblemDetailsobjects.This change is unlikely to introduce any breaking changes as it adds a new converter without modifying existing behavior.
src/Codebelt.Extensions.Newtonsoft.Json.App/Codebelt.Extensions.Newtonsoft.Json.App.csproj (1)
4-4: LGTM! The target framework update aligns with the PR objective.The change in target frameworks from
net8.0;net6.0tonet9.0;net8.0ensures compatibility with the latest .NET versions, specifically .NET 9.0, which is one of the primary objectives of this PR.Please note that removing support for .NET 6.0 might affect users who are still using that version. Consider communicating this breaking change clearly in the release notes and documentation to help users plan their upgrade path.
src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Codebelt.Extensions.AspNetCore.Newtonsoft.Json.csproj (2)
4-4: Breaking change: Ensure clear communication to users.Updating the target frameworks to include
net9.0and removenet6.0aligns with the PR objective. This change enables the project to leverage the latest .NET features and improvements.However, removing
net6.0is a breaking change that will affect users still on .NET 6.0. They will need to upgrade their projects to either .NET 8.0 or .NET 9.0 to continue using this library.Please ensure this breaking change is clearly communicated in the release notes and documentation to help users plan their upgrades accordingly.
14-14: Clarify the stability and compatibility of the preview dependency.The
Cuemon.Extensions.AspNetCorepackage has been updated to version9.0.0-preview.7, which is a preview release. While this aligns with the goal of supporting .NET 9.0, it raises some concerns:
- Preview versions may not be stable or fully compatible, potentially introducing issues in the library.
- Consumers of this library may need to update their own dependencies to maintain compatibility with the preview version.
To address these concerns, please:
- Clarify the stability and compatibility of the preview version. Has it been thoroughly tested with the library?
- Ensure comprehensive testing is performed to identify and resolve any issues introduced by the preview dependency.
- Clearly communicate the use of a preview dependency in the release notes and documentation, so users can make informed decisions about upgrading.
.nuget/Codebelt.Extensions.Newtonsoft.Json.App/PackageReleaseNotes.txt (3)
1-2: Ensure breaking changes are documented.The version update to 9.0.0 and the availability update to support .NET 9 and .NET 8 look good. As this is a major version change, please ensure that any breaking changes are clearly documented in the release notes to help users with the upgrade process.
5-5: LGTM!Updating the dependencies to the latest versions with respect to TFMs is a good practice to ensure compatibility and security.
6-6: Communicate the removal of .NET 6 support to users.Removing support for .NET 6 (LTS) is a significant change that may affect users who are still using this version. Please ensure that this change is clearly communicated in the release notes and consider providing migration guidance to help users update their projects to a supported .NET version if necessary.
.nuget/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/PackageReleaseNotes.txt (3)
1-3: LGTM!The version update to 9.0.0 and the support for .NET 9 and .NET 8 align with the latest framework versions.
9-9: LGTM!The new
AddProblemDetailsConvertermethod in theJsonConverterCollectionExtensionsclass extends the functionality of theCodebelt.Extensions.AspNetCore.Newtonsoft.Json.Convertersnamespace without introducing any breaking changes.
4-6: Verify the impact of removing .NET 6 (LTS) support.Updating the dependencies to the latest versions corresponding to the target framework monikers (TFMs) ensures compatibility with the supported frameworks.
However, removing support for .NET 6 (LTS) is a breaking change that could impact users who are still using this version. Please ensure that this change is communicated clearly in the release notes and documentation.
Run the following script to verify the usage of .NET 6 (LTS) in the codebase:
src/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.csproj (4)
4-4: LGTM!The
TargetFrameworksproperty has been updated to includenet9.0and removenet6.0. This change ensures compatibility with .NET 9.0 and .NET 8.0 applications.Note: Applications using this library and targeting .NET 6.0 will need to upgrade to a supported version.
13-15: Verify the stability of the release candidate version.The new
ItemGroupfor thenet9framework ensures compatibility with .NET 9.0 applications by referencing theMicrosoft.AspNetCore.Mvc.NewtonsoftJsonpackage with version9.0.0-rc.1.24452.1.However, please note that this is a release candidate version and may introduce instability or breaking changes in the future. Ensure that the release candidate version is stable enough for your use case.
26-26: Verify the stability of the preview release version.The
Cuemon.AspNetCore.Mvcpackage reference has been updated to version9.0.0-preview.7. This update ensures compatibility with the latest changes in the package.However, please note that this is a preview release version and may introduce instability or breaking changes in the future. Ensure that the preview release version is stable enough for your use case.
27-27: Verify the need for the new package and its stability.A new package reference for
Cuemon.Extensions.IOhas been added with version9.0.0-preview.7. This package may introduce additional functionality or dependencies to the project.Please verify that this package is necessary for the desired functionality and ensure that the preview release version is stable enough for your use case, as it may introduce instability or breaking changes in the future.
test/Codebelt.Extensions.Newtonsoft.Json.Tests/ValidatorExtensionsTest.cs (1)
20-20: LGTM!The change enhances code clarity by explicitly specifying the parameter name using a named argument. It improves readability without altering the test behavior.
.nuget/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json/PackageReleaseNotes.txt (3)
1-3: LGTM!The version number and availability have been updated correctly.
4-6: LGTM!The dependencies have been updated correctly, and the removal of support for .NET 6 (LTS) is clearly documented.
8-9: LGTM!The breaking change is clearly documented in the release notes. Please ensure that this breaking change is communicated to the users and any necessary migration steps are provided in the documentation.
.nuget/Codebelt.Extensions.Newtonsoft.Json/PackageReleaseNotes.txt (2)
1-2: LGTM!The version number and availability information are accurate and consistent with the PR objectives.
11-12: LGTM!Extending the
JsonConverterCollectionExtensionsclass with theAddFailureConvertermethod aligns with the PR objectives and complements the newFailureConverterclass.test/Codebelt.Extensions.Newtonsoft.Json.Tests/Formatters/NewtonsoftJsonFormatterOptionsTest.cs (1)
76-76: LGTM!The change in the assertion aligns with the test method's purpose and does not introduce any apparent issues. The updated count of default converters from 4 to 5 likely reflects an addition of a new default converter.
Directory.Build.props (3)
18-18: Breaking Change: .NET 6.0 support has been dropped in favor of .NET 9.0.The
TargetFrameworksproperty for non-test projects has been updated to includenet9.0and removenet6.0. This aligns with the PR objective of introducing .NET 9.0 support.Consumers of this library targeting .NET 6.0 will need to upgrade their projects to a supported version.
56-56: LGTM!The
TargetFrameworksproperty for test projects on Windows has been updated consistently with the changes made to non-test projects and test projects on Linux.
84-84: Confirm if using a pre-release version ofCodebelt.Extensions.Xunit.Appis intentional.The version of the
Codebelt.Extensions.Xunit.Apppackage has been updated to9.0.0-preview.2, which is a pre-release version. While this aligns with the PR objective of introducing .NET 9.0 support, using a pre-release version for testing may result in instability or unexpected behavior.Please confirm that using the pre-release version is intentional and necessary. Additionally, ensure thorough testing is performed to verify compatibility and stability with the updated package version.
src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Converters/JsonConverterCollectionExtensions.cs (3)
25-30: LGTM!The
AddProblemDetailsConvertermethod is well-implemented and follows the expected signature. It adds the necessary JSON converters forProblemDetailsandIDecorator<ProblemDetails>types to the provided collection.
32-72: LGTM!The
WriteProblemDetailsmethod correctly handles the serialization ofProblemDetailsobjects. It writes all the necessary properties and extensions to the JSON output using theJsonWriterandJsonSerializercorrectly.
84-88: Verify the impact of the breaking change in the method signature.The new code for writing the
InstanceandTraceIdproperties ofHttpExceptionDescriptorto the JSON output looks good. It follows the existing pattern and checks for null or whitespace before writing the properties.However, the method signature has been updated to take an
Action<ExceptionDescriptorOptions>parameter instead ofAction<ExceptionDescriptorConverterOptions<HttpExceptionDescriptor>>, which is a breaking change. Please ensure that all the existing code that uses this method is updated to match the new signature.Run the following script to verify the usage of
AddHttpExceptionDescriptorConverter:Also applies to: 103-107
Verification successful
Method signature change verified successfully.
All usages of
AddHttpExceptionDescriptorConverterhave been updated to the newAction<ExceptionDescriptorOptions>signature. No legacy usages of the old signature were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `AddHttpExceptionDescriptorConverter` match the new signature. # Test: Search for the method usage. Expect: Only occurrences of the new signature. rg --type csharp -A 5 $'AddHttpExceptionDescriptorConverter'Length of output: 8291
.github/workflows/pipelines.yml (1)
29-29: LGTM! Note the breaking change for .NET 6.0 users.The change aligns the pipeline with the PR objective of introducing support for .NET 9.0. However, removing
net6.0from the framework matrix is a breaking change for users who are still on .NET 6.0. They will not be able to use the new version of the library.Going forward, the library will only support .NET 9.0, .NET 8.0, and .NET Standard 2.0.
src/Codebelt.Extensions.Newtonsoft.Json/Converters/JsonConverterCollectionExtensions.cs (1)
104-113: LGTM!The new
AddFailureConvertermethod follows the existing convention in the class and extends its functionality by providing a way to add aFailureConverterto an existingICollection<JsonConverter>. The method signature, XML documentation, and implementation are all correct.The change is additive and does not modify any existing functionality, making it a safe addition to the class.
src/Codebelt.Extensions.Newtonsoft.Json/Formatters/NewtonsoftJsonFormatterOptions.cs (1)
37-37: LGTM!The addition of the
FailureConverterto the default converters is a valuable enhancement that enables the serialization and deserialization ofFailureobjects. This change aligns with the existing pattern of adding converters to the default list and does not introduce any apparent issues.test/Codebelt.Extensions.AspNetCore.Mvc.Formatters.Newtonsoft.Json.Tests/Converters/JsonConverterCollectionExtensionsTest.cs (1)
Line range hint
101-111: LGTM!The assertions correctly verify the presence or absence of the
"failure"key and related error details in the JSON output based on thesensitivityDetailsflag. This enhances the test coverage and ensures that theHttpExceptionDescriptorConverterbehaves as expected.CHANGELOG.md (4)
14-17: LGTM!The addition of the
FailureConverterclass is correctly documented under the "Added" section. The description of the class's purpose is clear and concise.
18-22: LGTM!The changes made to the existing classes are correctly documented under the "Changed" section. The descriptions of the changes are clear and concise, and the breaking change to the
ValidatorExtensionsclass is correctly noted.Tools
Markdownlint
22-22: null
Bare URL used(MD034, no-bare-urls)
24-27: LGTM!The removal of the
HttpExceptionDescriptorResponseHandlerExtensionsclass is correctly documented under the "Removed" section, and the breaking change is correctly noted.
Line range hint
1-27: LGTM!The changelog follows the "Keep a Changelog" format and adheres to Semantic Versioning. The changes are well-categorized, and the descriptions are clear and concise. The note about breaking changes is helpful for users preparing to update. Overall, the structure and content of the changelog are excellent.
Tools
LanguageTool
[uncategorized] ~12-~12: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... introduced with .NET 9 preview releases so the final release is production ready t...(COMMA_COMPOUND_SENTENCE_2)
Markdownlint
22-22: null
Bare URL used(MD034, no-bare-urls)
src/Codebelt.Extensions.AspNetCore.Newtonsoft.Json/Formatters/ServiceCollectionExtensions.cs (1)
Line range hint
40-46: Configuration logic is correctly implementedThe configuration of
NewtonsoftJsonFormatterOptionsusingservices.Configure(setup ?? (o => { ... }));correctly handles cases wheresetupis null by providing a default configuration. This ensures that the options are properly set even when no setup action is provided.
| /// <paramref name="argument"/> must be a JSON representation that complies with RFC 8259. | ||
| /// </exception> | ||
| public static void InvalidJsonDocument(this Validator _, string argument, [CallerArgumentExpression(nameof(argument))] string paramName = null, string message = "Value must be a JSON representation that complies with RFC 8259.") | ||
| public static void InvalidJsonDocument(this Validator _, string argument, string message = "Value must be a JSON representation that complies with RFC 8259.", [CallerArgumentExpression(nameof(argument))] string paramName = null) |
There was a problem hiding this comment.
Tip
Codebase Verification
Action Required: Update All Calls to InvalidJsonDocument to Match the New Signature.
The method signature change for InvalidJsonDocument is a breaking change. Ensure that all method calls to InvalidJsonDocument have been updated to match the new parameter order to prevent compilation errors and ensure consistent behavior.
- Affected Files:
test/Codebelt.Extensions.Newtonsoft.Json.Tests/ValidatorExtensionsTest.cssrc/Codebelt.Extensions.Newtonsoft.Json/JData.cs- Any other files identified by the search that do not match the new signature.
Analysis chain
Verify the method usage across the codebase.
The method signature change is a breaking change. Ensure that all method calls to InvalidJsonDocument have been updated to match the new parameter order.
Run the following script to verify the method usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all method calls to `InvalidJsonDocument` match the new signature.
# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type cs -A 5 $'InvalidJsonDocument\('
Length of output: 6970
| /// <paramref name="argument"/> must be a JSON representation that complies with RFC 8259. | ||
| /// </exception> | ||
| public static void InvalidJsonDocument(this Validator _, ref JsonReader argument, [CallerArgumentExpression(nameof(argument))] string paramName = null, string message = "Value must be a JSON representation that complies with RFC 8259.") | ||
| public static void InvalidJsonDocument(this Validator _, ref JsonReader argument, string message = "Value must be a JSON representation that complies with RFC 8259.", [CallerArgumentExpression(nameof(argument))] string paramName = null) |
There was a problem hiding this comment.
Tip
Codebase Verification
Update method calls to InvalidJsonDocument to include the ref keyword.
The following method calls still use the old signature without the ref parameter, which can cause compilation errors:
test/Codebelt.Extensions.Newtonsoft.Json.Tests/ValidatorExtensionsTest.cs:20test/Codebelt.Extensions.Newtonsoft.Json.Tests/ValidatorExtensionsTest.cs:26test/Codebelt.Extensions.Newtonsoft.Json.Tests/ValidatorExtensionsTest.cs:40
Please ensure all usages of InvalidJsonDocument are updated to match the new method signature with the ref keyword.
Analysis chain
Verify the method usage across the codebase.
The method signature change is a breaking change. Ensure that all method calls to InvalidJsonDocument have been updated to match the new parameter order.
Run the following script to verify the method usage:
Further verify that no method calls to InvalidJsonDocument are using the old signature without the ref parameter.
Please run the following script to identify any remaining usages that haven't been updated to the new method signature:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all method calls to `InvalidJsonDocument` match the new signature.
# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type cs -A 5 $'InvalidJsonDocument\('
Length of output: 6970
Script:
#
#!/bin/bash
# Description: Find all calls to `InvalidJsonDocument` that do not use the `ref` keyword.
# Search for method calls to `InvalidJsonDocument` without `ref`
rg --type cs 'InvalidJsonDocument\s*\([^ref]' -n
Length of output: 2403
| { | ||
| var options = provider.GetService<IOptions<NewtonsoftJsonFormatterOptions>>().Value; | ||
| var faultDescriptorOptions = provider.GetRequiredService<IOptions<FaultDescriptorOptions>>().Value; |
There was a problem hiding this comment.
Use GetRequiredService to ensure options are not null
Currently, options is retrieved using GetService<IOptions<NewtonsoftJsonFormatterOptions>>().Value;, which could result in a NullReferenceException if the service is not registered. Since AddNewtonsoftJsonFormatterOptions is called prior and should register the options, using GetRequiredService ensures that an exception is thrown if the options are not available, preventing potential runtime errors.
Apply this diff to change GetService to GetRequiredService:
- var options = provider.GetService<IOptions<NewtonsoftJsonFormatterOptions>>().Value;
+ var options = provider.GetRequiredService<IOptions<NewtonsoftJsonFormatterOptions>>().Value;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| var options = provider.GetService<IOptions<NewtonsoftJsonFormatterOptions>>().Value; | |
| var faultDescriptorOptions = provider.GetRequiredService<IOptions<FaultDescriptorOptions>>().Value; | |
| { | |
| var options = provider.GetRequiredService<IOptions<NewtonsoftJsonFormatterOptions>>().Value; | |
| var faultDescriptorOptions = provider.GetRequiredService<IOptions<FaultDescriptorOptions>>().Value; |
| public override Failure ReadJson(JsonReader reader, Type objectType, Failure existingValue, bool hasExistingValue, JsonSerializer serializer) | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Implementation of ReadJson method is incomplete.
The ReadJson method currently throws a NotImplementedException, which means deserialization of Failure objects is not supported. This could lead to runtime exceptions if deserialization is attempted.
Consider implementing the ReadJson method or explicitly documenting that deserialization is not supported.
| if (value.Stack.Any()) | ||
| { | ||
| writer.WritePropertyName(nameof(value.Stack), serializer); | ||
| writer.WriteStartArray(); | ||
| foreach (var line in value.Stack) | ||
| { | ||
| writer.WriteValue(line); | ||
| } | ||
| writer.WriteEndArray(); | ||
| } |
There was a problem hiding this comment.
Optimize stack trace writing by checking for non-empty stack.
Currently, the code checks if (value.Stack.Any()), which could be optimized by checking if (value.Stack != null && value.Stack.Count > 0) to avoid potential NullReferenceException if value.Stack is null.
Apply this diff to ensure safety:
- if (value.Stack.Any())
+ if (value.Stack != null && value.Stack.Count > 0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (value.Stack.Any()) | |
| { | |
| writer.WritePropertyName(nameof(value.Stack), serializer); | |
| writer.WriteStartArray(); | |
| foreach (var line in value.Stack) | |
| { | |
| writer.WriteValue(line); | |
| } | |
| writer.WriteEndArray(); | |
| } | |
| if (value.Stack != null && value.Stack.Count > 0) | |
| { | |
| writer.WritePropertyName(nameof(value.Stack), serializer); | |
| writer.WriteStartArray(); | |
| foreach (var line in value.Stack) | |
| { | |
| writer.WriteValue(line); | |
| } | |
| writer.WriteEndArray(); | |
| } |
| WriteInnerExceptions(writer, value, serializer); | ||
| } | ||
|
|
||
| private static void WriteInnerExceptions(JsonWriter writer, Failure value, JsonSerializer serializer) | ||
| { | ||
| var exception = value.GetUnderlyingException(); | ||
| var innerExceptions = new List<Exception>(); | ||
| if (exception is AggregateException aggregated) | ||
| { | ||
| innerExceptions.AddRange(aggregated.Flatten().InnerExceptions); | ||
| } | ||
| else | ||
| { | ||
| if (exception.InnerException != null) { innerExceptions.Add(exception.InnerException); } | ||
| } | ||
| if (innerExceptions.Count > 0) | ||
| { | ||
| var endElementsToWrite = 0; | ||
| foreach (var inner in innerExceptions) | ||
| { | ||
| var innerValue = new Failure(inner, value.GetUnderlyingSensitivity()); | ||
| writer.WritePropertyName("Inner", serializer); | ||
| writer.WriteStartObject(); | ||
| writer.WritePropertyName(nameof(value.Type), serializer); | ||
| writer.WriteValue(innerValue.Type); | ||
| WriteException(writer, innerValue, serializer); | ||
| endElementsToWrite++; | ||
| } | ||
|
|
||
| for (var i = 0; i < endElementsToWrite; i++) { writer.WriteEndObject(); } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential issues with writing inner exceptions.
The WriteInnerExceptions method writes multiple inner exceptions but uses the same property name "Inner" for each. Additionally, it writes multiple start objects without a corresponding JSON array, and then writes multiple end objects in a loop, which might not correctly structure the JSON output.
Consider wrapping the inner exceptions in a JSON array to maintain proper JSON structure.
Apply this diff to refactor the method:
- if (innerExceptions.Count > 0)
+ if (innerExceptions.Any())
{
+ writer.WritePropertyName("InnerExceptions", serializer);
+ writer.WriteStartArray();
foreach (var inner in innerExceptions)
{
- writer.WritePropertyName("Inner", serializer);
writer.WriteStartObject();
writer.WritePropertyName(nameof(value.Type), serializer);
writer.WriteValue(innerValue.Type);
WriteException(writer, innerValue, serializer);
- endElementsToWrite++;
writer.WriteEndObject();
}
+ writer.WriteEndArray();
}
- for (var i = 0; i < endElementsToWrite; i++) { writer.WriteEndObject(); }
}This modification ensures that all inner exceptions are included in an array named "InnerExceptions", which is more appropriate for multiple items.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WriteInnerExceptions(writer, value, serializer); | |
| } | |
| private static void WriteInnerExceptions(JsonWriter writer, Failure value, JsonSerializer serializer) | |
| { | |
| var exception = value.GetUnderlyingException(); | |
| var innerExceptions = new List<Exception>(); | |
| if (exception is AggregateException aggregated) | |
| { | |
| innerExceptions.AddRange(aggregated.Flatten().InnerExceptions); | |
| } | |
| else | |
| { | |
| if (exception.InnerException != null) { innerExceptions.Add(exception.InnerException); } | |
| } | |
| if (innerExceptions.Count > 0) | |
| { | |
| var endElementsToWrite = 0; | |
| foreach (var inner in innerExceptions) | |
| { | |
| var innerValue = new Failure(inner, value.GetUnderlyingSensitivity()); | |
| writer.WritePropertyName("Inner", serializer); | |
| writer.WriteStartObject(); | |
| writer.WritePropertyName(nameof(value.Type), serializer); | |
| writer.WriteValue(innerValue.Type); | |
| WriteException(writer, innerValue, serializer); | |
| endElementsToWrite++; | |
| } | |
| for (var i = 0; i < endElementsToWrite; i++) { writer.WriteEndObject(); } | |
| } | |
| } | |
| WriteInnerExceptions(writer, value, serializer); | |
| } | |
| private static void WriteInnerExceptions(JsonWriter writer, Failure value, JsonSerializer serializer) | |
| { | |
| var exception = value.GetUnderlyingException(); | |
| var innerExceptions = new List<Exception>(); | |
| if (exception is AggregateException aggregated) | |
| { | |
| innerExceptions.AddRange(aggregated.Flatten().InnerExceptions); | |
| } | |
| else | |
| { | |
| if (exception.InnerException != null) { innerExceptions.Add(exception.InnerException); } | |
| } | |
| if (innerExceptions.Any()) | |
| { | |
| writer.WritePropertyName("InnerExceptions", serializer); | |
| writer.WriteStartArray(); | |
| foreach (var inner in innerExceptions) | |
| { | |
| var innerValue = new Failure(inner, value.GetUnderlyingSensitivity()); | |
| writer.WriteStartObject(); | |
| writer.WritePropertyName(nameof(value.Type), serializer); | |
| writer.WriteValue(innerValue.Type); | |
| WriteException(writer, innerValue, serializer); | |
| writer.WriteEndObject(); | |
| } | |
| writer.WriteEndArray(); | |
| } | |
| } |
| private static void WriteException(JsonWriter writer, Failure value, JsonSerializer serializer) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(value.Source)) | ||
| { | ||
| writer.WritePropertyName(nameof(value.Source), serializer); | ||
| writer.WriteValue(value.Source); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(value.Message)) | ||
| { | ||
| writer.WritePropertyName(nameof(value.Message), serializer); | ||
| writer.WriteValue(value.Message); | ||
| } | ||
|
|
||
| if (value.Stack.Any()) | ||
| { | ||
| writer.WritePropertyName(nameof(value.Stack), serializer); | ||
| writer.WriteStartArray(); | ||
| foreach (var line in value.Stack) | ||
| { | ||
| writer.WriteValue(line); | ||
| } | ||
| writer.WriteEndArray(); | ||
| } | ||
|
|
||
| if (value.Data.Count > 0) | ||
| { | ||
| writer.WritePropertyName(nameof(value.Data), serializer); | ||
| writer.WriteStartObject(); | ||
| foreach (var kvp in value.Data) | ||
| { | ||
| writer.WritePropertyName(nameof(kvp.Key), serializer); | ||
| writer.WriteObject(kvp.Value, serializer); | ||
| } | ||
| writer.WriteEndObject(); | ||
| } | ||
|
|
||
| foreach (var kvp in value) | ||
| { | ||
| writer.WritePropertyName(kvp.Key, serializer); | ||
| writer.WriteObject(kvp.Value, serializer); | ||
| } | ||
|
|
||
| WriteInnerExceptions(writer, value, serializer); | ||
| } |
There was a problem hiding this comment.
Incorrect usage of nameof operator for dictionary keys.
In lines 77 and 85, nameof(kvp.Key) is used, which will return the string "Key" instead of the actual key from the dictionary. This results in all property names being "Key" in the JSON output, causing a loss of the original keys.
Apply this diff to fix the issue:
- writer.WritePropertyName(nameof(kvp.Key), serializer);
+ writer.WritePropertyName(kvp.Key.ToString(), serializer);Repeat this change for both occurrences on lines 77 and 85.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static void WriteException(JsonWriter writer, Failure value, JsonSerializer serializer) | |
| { | |
| if (!string.IsNullOrWhiteSpace(value.Source)) | |
| { | |
| writer.WritePropertyName(nameof(value.Source), serializer); | |
| writer.WriteValue(value.Source); | |
| } | |
| if (!string.IsNullOrWhiteSpace(value.Message)) | |
| { | |
| writer.WritePropertyName(nameof(value.Message), serializer); | |
| writer.WriteValue(value.Message); | |
| } | |
| if (value.Stack.Any()) | |
| { | |
| writer.WritePropertyName(nameof(value.Stack), serializer); | |
| writer.WriteStartArray(); | |
| foreach (var line in value.Stack) | |
| { | |
| writer.WriteValue(line); | |
| } | |
| writer.WriteEndArray(); | |
| } | |
| if (value.Data.Count > 0) | |
| { | |
| writer.WritePropertyName(nameof(value.Data), serializer); | |
| writer.WriteStartObject(); | |
| foreach (var kvp in value.Data) | |
| { | |
| writer.WritePropertyName(nameof(kvp.Key), serializer); | |
| writer.WriteObject(kvp.Value, serializer); | |
| } | |
| writer.WriteEndObject(); | |
| } | |
| foreach (var kvp in value) | |
| { | |
| writer.WritePropertyName(kvp.Key, serializer); | |
| writer.WriteObject(kvp.Value, serializer); | |
| } | |
| WriteInnerExceptions(writer, value, serializer); | |
| } | |
| private static void WriteException(JsonWriter writer, Failure value, JsonSerializer serializer) | |
| { | |
| if (!string.IsNullOrWhiteSpace(value.Source)) | |
| { | |
| writer.WritePropertyName(nameof(value.Source), serializer); | |
| writer.WriteValue(value.Source); | |
| } | |
| if (!string.IsNullOrWhiteSpace(value.Message)) | |
| { | |
| writer.WritePropertyName(nameof(value.Message), serializer); | |
| writer.WriteValue(value.Message); | |
| } | |
| if (value.Stack.Any()) | |
| { | |
| writer.WritePropertyName(nameof(value.Stack), serializer); | |
| writer.WriteStartArray(); | |
| foreach (var line in value.Stack) | |
| { | |
| writer.WriteValue(line); | |
| } | |
| writer.WriteEndArray(); | |
| } | |
| if (value.Data.Count > 0) | |
| { | |
| writer.WritePropertyName(nameof(value.Data), serializer); | |
| writer.WriteStartObject(); | |
| foreach (var kvp in value.Data) | |
| { | |
| writer.WritePropertyName(kvp.Key.ToString(), serializer); | |
| writer.WriteObject(kvp.Value, serializer); | |
| } | |
| writer.WriteEndObject(); | |
| } | |
| foreach (var kvp in value) | |
| { | |
| writer.WritePropertyName(kvp.Key.ToString(), serializer); | |
| writer.WriteObject(kvp.Value, serializer); | |
| } | |
| WriteInnerExceptions(writer, value, serializer); | |
| } |
|
1 participant
PR Classification
New feature and code cleanup to enhance functionality and ensure compatibility with the latest .NET versions.
PR Summary
This PR introduces new features, updates for .NET 9.0 compatibility, and code cleanup across the Codebelt.Extensions library.
JsonConverterCollectionExtensions.cs: AddedAddProblemDetailsConvertermethod forProblemDetailsserialization,FailureConverter.cs: IntroducedFailureConverterclass to convertFailureobjects to JSON,pipelines.yml: Updated to includenet9.0and removenet6.0,PackageReleaseNotes.txtandCHANGELOG.md: Updated for version9.0.0, noting new features and breaking changes,Summary by CodeRabbit
Release Notes
New Features
AddProblemDetailsConverterandAddFailureConverterfor enhanced JSON handling.FailureConverterclass for convertingFailureinstances to JSON format.Changes
Bug Fixes
ValidatorExtensionsmethods for better parameter handling.