CU-86b15d33f_GH-118-⁃-Retro-compatibility-with-dates-between-v9-and-v… #131
Conversation
…10_SRU2024 Co-Authored-By: zubri <[email protected]>
📝 WalkthroughWalkthroughThis pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)build.gradle(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/AbstractMX.java(5 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/v9/V9DateTimeJsonAdapter.java(1 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/AbstractMxJsonV9Test.java(1 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/v9/V9DateTimeJsonAdapterTest.java(1 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
3-3: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
3-3: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
4-4: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🔇 Additional comments (17)
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/v9/V9DateTimeJsonAdapterTest.java (3)
1-13: LGTM! Clean test class setup.
The class structure follows testing best practices with appropriate visibility and dependencies.
31-46: 🧹 Nitpick (assertive)
Apply similar improvements to serialization tests.
The same improvements suggested for the deserialization tests should be applied here for consistency and better test coverage.
1-59: Verify exception handling and adapter implementation.
The tests should verify:
- Exception handling for invalid inputs
- Integration with the actual V9DateTimeJsonAdapter implementation
✅ Verification successful
Exception handling and implementation verification is sufficient
After examining the implementation and related tests, I can conclude:
-
The V9DateTimeJsonAdapter has proper exception handling in the deserialize method:
- It catches all exceptions and logs them at FINEST level
- Returns null for invalid inputs, which is documented in the JavaDoc
-
The test coverage could be improved but the current tests verify:
- Core serialization/deserialization functionality
- Edge cases like zero nanoseconds
- Different timezone offsets
- The actual adapter implementation is properly integrated
The review comment's concerns have been addressed by the existing implementation, though additional test cases for invalid inputs could be beneficial but aren't strictly necessary given the simple error handling strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for V9DateTimeJsonAdapter implementation to verify exception handling
ast-grep --pattern 'class V9DateTimeJsonAdapter {
$$$
deserialize($$$) {
$$$
}
$$$
}'
# Search for any existing exception handling tests
rg -l "Test.*Exception" --type java
Length of output: 137
Script:
#!/bin/bash
# Let's try a broader search for the adapter implementation
fd "V9DateTimeJsonAdapter.java" --type f
# Search for the adapter class with a simpler pattern
ast-grep --pattern 'class V9DateTimeJsonAdapter'
# Let's also check for any test files related to exception handling in the same directory
fd ".*Test.java" --type f | grep -i "adapter"
Length of output: 1806
Script:
#!/bin/bash
# Let's examine the actual adapter implementation
cat iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/v9/V9DateTimeJsonAdapter.java
# Let's also check other similar adapter tests for exception handling patterns
cat iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/OffsetDateTimeJsonAdapterTest.java
Length of output: 10210
CHANGELOG.md (1)
3-3: LGTM: Version number follows semantic versioning.
The version increment from 10.2.3 to 10.2.4 is appropriate for backward-compatible changes, and the SNAPSHOT suffix correctly indicates work in progress.
🧰 Tools
🪛 Markdownlint
3-3: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
3-3: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
build.gradle (2)
341-341: LGTM! Clean dependency management.
The explicit declaration of jaxb-impl:4.0.5 while relying on its transitive dependencies is a good practice. This helps maintain a clean dependency tree while ensuring all required components are available.
Let's verify there are no version conflicts with the transitive dependencies:
341-341: Verify JAXB compatibility across Java versions.
Since the project supports multiple Java versions (11, 17, 19) and includes date-time serialization changes, it's important to verify that the JAXB implementation works consistently across all supported versions.
Run the following commands to verify:
✅ Verification successful
JAXB 4.0.5 upgrade is compatible with configured Java versions
The upgrade from JAXB 4.0.2 to 4.0.5 is safe because:
- The project correctly configures Java toolchains for versions 11, 17, and 19
- JAXB implementation is properly declared as a runtime dependency
- The codebase uses JAXB extensively for XML serialization in core functionality
- The upgrade only includes bugfixes as noted in the changelog
Key findings:
- Java 11 is set as the base toolchain version
- Test tasks are properly configured for Java 11, 17, and 19 compatibility checks
- JAXB is used in critical components like
MxReadImpl,JaxbContextCache, and various adapters - The README acknowledges JAXB as a core runtime dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the build and tests pass across all supported Java versions
# List all available Java toolchains
./gradlew -q javaToolchains
# Run tests on all supported Java versions
./gradlew testOn11 testOn17 testOn19
Length of output: 438
Script:
#!/bin/bash
# Let's verify the Java version configurations and JAXB usage in the build files
# Check build.gradle files for Java version configurations
rg "sourceCompatibility|targetCompatibility|javaVersion" -A 3 -B 3
# Check for JAXB-related configurations and dependencies
rg "jaxb|javax.xml.bind" -A 2 -B 2
# Look for Java toolchain configurations
rg "toolchain|javaToolchain" -A 3 -B 3
# Check for test tasks configuration
rg "task.*test.*Java|testOn" -A 5 -B 2
Length of output: 13899
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/AbstractMX.java (11)
29-29: Import of V9DateTimeJsonAdapter is appropriate
The addition of the import statement for V9DateTimeJsonAdapter ensures that the class is available for the new JSON serialization methods targeting version 9 compatibility.
41-41: Verify the necessity of importing XMLGregorianCalendar
The import of javax.xml.datatype.XMLGregorianCalendar is added, possibly for use in JavaDoc references. Ensure that this import is necessary and that the class is utilized in the code or documentation to prevent unused imports.
206-206: Update fromJson method to use V10 adapters
Changing the fromJson method to use getGsonBuilderWithV10Adapters() ensures that deserialization uses the latest adapters compatible with version 10, aligning with the updates made for JSON handling.
218-220: Update fromJson method for AbstractMX to use V10 adapters
The method now utilizes getGsonBuilderWithV10Adapters(), which is consistent with the shift to version 10 adapters for JSON deserialization.
235-238: Addition of fromJsonV9 method for version 9 compatibility
The new fromJsonV9 method correctly uses getGsonBuilderWithV9Adapters() to deserialize JSON strings using version 9 adapters, ensuring backward compatibility with Java 8 date-time formats.
251-254: Addition of fromJsonV9 method for AbstractMX
The method fromJsonV9(String json) is appropriately added to handle deserialization of AbstractMX objects using version 9 adapters, facilitating compatibility with legacy JSON formats.
593-594: Update toJson method to use V10 adapters
The toJson method now calls getGsonBuilderWithV10Adapters(), ensuring that serialization aligns with version 10 JSON formats.
606-609: Addition of toJsonV9 method for version 9 compatibility
The new toJsonV9 method correctly serializes AbstractMX objects using version 9 adapters by invoking getGsonBuilderWithV9Adapters(), ensuring backward compatibility with older systems expecting Java 8 date-time formats.
611-619: Definition of getGsonBuilderWithCustomAdapters method
The method getGsonBuilderWithCustomAdapters() effectively registers the necessary type adapters for custom serialization and deserialization, providing a base for both V9 and V10 adapter configurations.
629-634: Consistency in Gson builder methods
The getGsonBuilderWithV10Adapters() method is correctly updated to use the OffsetDateTimeJsonAdapter, ensuring that serialization and deserialization adhere to version 10 formats.
235-238: Ensure unit tests cover new serialization methods
While the AI-generated summary mentions the addition of unit tests, please verify that comprehensive unit tests are included for the new fromJsonV9, toJsonV9, and related methods to ensure they function correctly under various scenarios and maintain backward compatibility.
You can run the following script to list test classes related to JSON serialization:
Also applies to: 251-254, 606-609
✅ Verification successful
Unit tests are properly implemented for JSON V9 serialization methods
The examination of AbstractMxJsonV9Test.java shows comprehensive test coverage for the new JSON V9 serialization methods:
testMxFromJsonV9()verifies thefromJsonV9method with a complex test casetestMxToJson()includes verification oftoJsonV9output- Tests validate both serialization and deserialization with proper assertions
- Tests cover complex data structures including nested objects, arrays, and various data types
- Backward compatibility is verified by comparing parsed results with the XML representation
The test implementation demonstrates thorough coverage of the new V9 JSON serialization functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find unit test classes related to JSON serialization methods.
# Test: Search for test classes containing 'Json' in their names.
# Expect: List of test classes verifying JSON serialization methods.
fd --type f --extension java --exec grep -l 'public class .*Json.*Test'
Length of output: 554
Script:
#!/bin/bash
# Let's examine the content of AbstractMxJsonV9Test.java to verify test coverage
cat ./iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/AbstractMxJsonV9Test.java
Length of output: 19245
Script:
#!/bin/bash
# Let's also check if there are any test methods containing 'fromJsonV9' or 'toJsonV9'
rg -l "test.*(?:from|to)JsonV9" --type java
Length of output: 45
Script:
#!/bin/bash
# Search for actual test method implementations related to JsonV9
ast-grep --pattern 'public void test$_JsonV9$_() {
$$$
}'
Length of output: 61
| @Test | ||
| void testDeserialization() { | ||
| String sourceJsonV9; | ||
| OffsetDateTime expectedOffsetDateTime; | ||
|
|
||
| sourceJsonV9 = | ||
| "{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}"; | ||
| expectedOffsetDateTime = OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00"); | ||
| testDeserialization(sourceJsonV9, expectedOffsetDateTime); | ||
|
|
||
| // without offset, nano 0 | ||
| sourceJsonV9 = | ||
| "{\"year\":2019,\"month\":4,\"day\":28,\"timezone\":0,\"hour\":12,\"minute\":13,\"second\":14,\"fractionalSecond\":0}"; | ||
| expectedOffsetDateTime = OffsetDateTime.parse("2019-04-28T12:13:14.0-00:00"); | ||
| testDeserialization(sourceJsonV9, expectedOffsetDateTime); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing test coverage and maintainability.
While the current test cases are good, consider these improvements:
- Add edge cases (null values, invalid formats)
- Convert to parameterized tests for better maintainability
- Add more diverse test cases (different timezones, boundary dates)
Example parameterized test structure:
@ParameterizedTest
@MethodSource("deserializationTestCases")
void testDeserialization(String sourceJsonV9, OffsetDateTime expected) {
testDeserialization(sourceJsonV9, expected);
}
static Stream<Arguments> deserializationTestCases() {
return Stream.of(
// Current cases
Arguments.of(
"{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}",
OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00")
),
// Additional cases
Arguments.of(
"{\"year\":2024,\"month\":12,\"day\":31,\"timezone\":330,\"hour\":23,\"minute\":59,\"second\":59,\"fractionalSecond\":0.999}",
OffsetDateTime.parse("2024-12-31T23:59:59.999+05:30")
)
// Add more test cases...
);
}| private void testDeserialization(String sourceJsonV9, OffsetDateTime expected) { | ||
| JsonElement sourceJsonElement = new JsonParser().parse(sourceJsonV9); | ||
| OffsetDateTime deserializeResult = adapter.deserialize(sourceJsonElement, null, null); | ||
| assertEquals(expected, deserializeResult); | ||
| } | ||
|
|
||
| private void testSerialization(OffsetDateTime sourceOffsetDateTimeV10, String expectedJsonV9) { | ||
| JsonElement expectedJsonElement = new JsonParser().parse(expectedJsonV9); | ||
| JsonElement serializeResult = adapter.serialize(sourceOffsetDateTimeV10, null, null); | ||
| assertEquals(expectedJsonElement, serializeResult); | ||
| } |
There was a problem hiding this comment.
Update deprecated JsonParser usage.
The new JsonParser() constructor is deprecated. Use the static method instead:
- JsonElement sourceJsonElement = new JsonParser().parse(sourceJsonV9);
+ JsonElement sourceJsonElement = JsonParser.parseString(sourceJsonV9);
- JsonElement expectedJsonElement = new JsonParser().parse(expectedJsonV9);
+ JsonElement expectedJsonElement = JsonParser.parseString(expectedJsonV9);📝 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 void testDeserialization(String sourceJsonV9, OffsetDateTime expected) { | |
| JsonElement sourceJsonElement = new JsonParser().parse(sourceJsonV9); | |
| OffsetDateTime deserializeResult = adapter.deserialize(sourceJsonElement, null, null); | |
| assertEquals(expected, deserializeResult); | |
| } | |
| private void testSerialization(OffsetDateTime sourceOffsetDateTimeV10, String expectedJsonV9) { | |
| JsonElement expectedJsonElement = new JsonParser().parse(expectedJsonV9); | |
| JsonElement serializeResult = adapter.serialize(sourceOffsetDateTimeV10, null, null); | |
| assertEquals(expectedJsonElement, serializeResult); | |
| } | |
| private void testDeserialization(String sourceJsonV9, OffsetDateTime expected) { | |
| JsonElement sourceJsonElement = JsonParser.parseString(sourceJsonV9); | |
| OffsetDateTime deserializeResult = adapter.deserialize(sourceJsonElement, null, null); | |
| assertEquals(expected, deserializeResult); | |
| } | |
| private void testSerialization(OffsetDateTime sourceOffsetDateTimeV10, String expectedJsonV9) { | |
| JsonElement expectedJsonElement = JsonParser.parseString(expectedJsonV9); | |
| JsonElement serializeResult = adapter.serialize(sourceOffsetDateTimeV10, null, null); | |
| assertEquals(expectedJsonElement, serializeResult); | |
| } |
| #### 10.2.4 - SNAPSHOT | ||
| * (GH-118) Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix markdown formatting issues.
The markdown formatting needs improvement for better readability and compliance:
+## Version History
+
-#### 10.2.4 - SNAPSHOT
- * (GH-118) Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization
+### 10.2.4 - SNAPSHOT
+
+* GH-118 Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization
+📝 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.
| #### 10.2.4 - SNAPSHOT | |
| * (GH-118) Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization | |
| ## Version History | |
| ### 10.2.4 - SNAPSHOT | |
| * GH-118 Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization | |
🧰 Tools
🪛 Markdownlint
3-3: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
3-3: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
4-4: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
| #### 10.2.4 - SNAPSHOT | ||
| * (GH-118) Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance the changelog entry with more details.
Consider expanding the changelog entry to provide more context and maintain consistency with other entries:
-#### 10.2.4 - SNAPSHOT
- * (GH-118) Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization
+#### 10.2.4 - SNAPSHOT
+ * GH-118 Added `toJsonV9` and `fromJsonV9` methods in `AbstractMX` class for backward compatibility with Java 8 DateTime elements in JSON serialization
+ * Added `V9DateTimeJsonAdapter` to handle serialization and deserialization of `OffsetDateTime` objects in legacy format📝 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.
| #### 10.2.4 - SNAPSHOT | |
| * (GH-118) Added `toJsonV9` and `fromJsonV9` in the `AbstractMX` to handle retro-compatibility with Java 8 DateTime elements in JSON serialization | |
| #### 10.2.4 - SNAPSHOT | |
| * GH-118 Added `toJsonV9` and `fromJsonV9` methods in `AbstractMX` class for backward compatibility with Java 8 DateTime elements in JSON serialization | |
| * Added `V9DateTimeJsonAdapter` to handle serialization and deserialization of `OffsetDateTime` objects in legacy format |
🧰 Tools
🪛 Markdownlint
3-3: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
3-3: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
4-4: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
| + " }" | ||
| + " }," | ||
| + " 'instdAgt': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOORGB2L'" | ||
| + " }" | ||
| + " }," | ||
| + " 'dbtr': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOORUS33'" | ||
| + " }" | ||
| + " }," | ||
| + " 'cdtr': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOOYGB2L'" | ||
| + " }" | ||
| + " }," | ||
| + " 'undrlygCstmrCdtTrf': {" | ||
| + " 'ultmtDbtr': {" | ||
| + " 'nm': 'GOLDMANS'," | ||
| + " 'pstlAdr': {" | ||
| + " 'strtNm': '5TH AVENUE'," | ||
| + " 'twnNm': 'New York'," | ||
| + " 'ctry': 'US'" | ||
| + " }" | ||
| + " }," | ||
| + " 'initgPty': {" | ||
| + " 'nm': 'Virgay'," | ||
| + " 'pstlAdr': {" | ||
| + " 'strtNm': 'Virginia Lane'," | ||
| + " 'bldgNb': '36'," | ||
| + " 'pstCd': 'NJ 07311'," | ||
| + " 'twnNm': 'Jersey City'," | ||
| + " 'ctry': 'US'" | ||
| + " }" | ||
| + " }," | ||
| + " 'dbtr': {" | ||
| + " 'nm': 'GOLDMANS'," | ||
| + " 'pstlAdr': {" | ||
| + " 'strtNm': '5TH AVENUE'," | ||
| + " 'twnNm': 'New York'," | ||
| + " 'ctry': 'US'" | ||
| + " }" | ||
| + " }," | ||
| + " 'dbtrAgt': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOORUS33'" | ||
| + " }" | ||
| + " }," | ||
| + " 'cdtrAgt': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOOYGB2L'" | ||
| + " }" | ||
| + " }," | ||
| + " 'cdtr': {" | ||
| + " 'nm': 'SMITHSON'," | ||
| + " 'pstlAdr': {" | ||
| + " 'strtNm': 'LONDON WALL'," | ||
| + " 'twnNm': 'LONDON'," | ||
| + " 'ctry': 'GB'" | ||
| + " }" | ||
| + " }" | ||
| + " }" | ||
| + " }" | ||
| + " ]" | ||
| + " }," | ||
| + " 'appHdr': {" | ||
| + " 'fr': {" | ||
| + " 'fiId': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOORUS33'" | ||
| + " }" | ||
| + " }" | ||
| + " }," | ||
| + " 'to': {" | ||
| + " 'fiId': {" | ||
| + " 'finInstnId': {" | ||
| + " 'bicfi': 'FOORGB2L'" | ||
| + " }" | ||
| + " }" | ||
| + " }," | ||
| + " 'bizMsgIdr': 'pacs009bizmsgidr-001'," | ||
| + " 'msgDefIdr': 'pacs.009.001.08CORE'," | ||
| + " 'bizSvc': 'swift.cbprplus.cov.02'," | ||
| + " 'creDt': {" | ||
| + " 'year': 2020," | ||
| + " 'month': 12," | ||
| + " 'day': 14," | ||
| + " 'timezone': -240," | ||
| + " 'hour': 9," | ||
| + " 'minute': 0," | ||
| + " 'second': 0," | ||
| + " 'fractionalSecond': 0" | ||
| + " }," | ||
| + " 'namespace': 'urn:iso:std:iso:20022:tech:xsd:head.001.001.02'" | ||
| + " }," | ||
| + " 'type': 'MX'," | ||
| + " '@xmlns': 'urn:iso:std:iso:20022:tech:xsd:pacs.009.001.08'," | ||
| + " 'identifier': 'pacs.009.001.08'" | ||
| + "}"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace sensitive test data with anonymized values.
The test data contains what appears to be real BIC codes and transaction details. While these might be fictional, it's best practice to use clearly fake data in tests.
Replace sensitive data with clearly fake values:
- BIC codes: Use 'TESTBIC1', 'TESTBIC2', etc.
- Company names: Use 'COMPANY1', 'COMPANY2', etc.
- Addresses: Use 'TEST STREET', 'TEST CITY', etc.
🛠️ Refactor suggestion
Move test data to separate resource files for better maintainability.
The test data (XML and JSON strings) should be moved to separate files under src/test/resources for several reasons:
- Improved readability of test class
- Easier maintenance of test data
- Better IDE support for XML/JSON formatting
Create the following files:
src/test/resources/pacs.009.001.08/sample.xmlsrc/test/resources/pacs.009.001.08/sample_v10.jsonsrc/test/resources/pacs.009.001.08/sample_v9.json
Then refactor the test class to load these resources:
- String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" + ...
- String jsonV10 = "{" + " 'fiCdtTrf': {" + ...
- String jsonV9 = "{" + " 'fiCdtTrf': {" + ...
+ private String xml;
+ private String jsonV10;
+ private String jsonV9;
+
+ @BeforeEach
+ void setUp() throws IOException {
+ xml = Resources.toString(getClass().getResource("/pacs.009.001.08/sample.xml"), StandardCharsets.UTF_8);
+ jsonV10 = Resources.toString(getClass().getResource("/pacs.009.001.08/sample_v10.json"), StandardCharsets.UTF_8);
+ jsonV9 = Resources.toString(getClass().getResource("/pacs.009.001.08/sample_v9.json"), StandardCharsets.UTF_8);
+ }Committable suggestion skipped: line range outside the PR's diff.
| Integer year = 0; | ||
| Integer month = 0; | ||
| Integer day = 0; | ||
| Integer timezone = 0; | ||
| Integer hour = 0; | ||
| Integer minute = 0; | ||
| Integer second = 0; | ||
| BigDecimal fractionalSecond = BigDecimal.ZERO; | ||
| } |
There was a problem hiding this comment.
Avoid Initializing DTO Fields with Default Zero Values
Initializing the XMLGregorianCalendarDTO fields to zero can result in invalid date-time values (e.g., month or day zero) during deserialization if the JSON does not provide these fields. It's better to leave these fields as null and perform null checks during deserialization to handle missing values appropriately.
Apply this diff to remove default initializations:
- Integer year = 0;
- Integer month = 0;
- Integer day = 0;
- Integer timezone = 0;
- Integer hour = 0;
- Integer minute = 0;
- Integer second = 0;
- BigDecimal fractionalSecond = BigDecimal.ZERO;
+ Integer year;
+ Integer month;
+ Integer day;
+ Integer timezone;
+ Integer hour;
+ Integer minute;
+ Integer second;
+ BigDecimal fractionalSecond;Then, add null checks in the deserialize method to validate the presence of these fields before using them.
Committable suggestion skipped: line range outside the PR's diff.
| * @since 10.1.8 | ||
| */ | ||
| public class V9DateTimeJsonAdapter implements JsonSerializer<OffsetDateTime>, JsonDeserializer<OffsetDateTime> { | ||
| private static final Logger log = Logger.getLogger(V9DateTimeJsonAdapter.class.getName()); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Rename Logger Variable to Follow Naming Conventions
Java naming conventions suggest that constants should be in uppercase letters with words separated by underscores. Since log is a static final variable, consider renaming it to LOGGER to clearly indicate its constant nature.
Apply this diff:
- private static final Logger log = Logger.getLogger(V9DateTimeJsonAdapter.class.getName());
+ private static final Logger LOGGER = Logger.getLogger(V9DateTimeJsonAdapter.class.getName());Update references to log within the class:
- log.log(Level.FINEST, "Cannot parse JSON into OffsetDateTime: " + e.getMessage(), e);
+ LOGGER.log(Level.FINEST, "Cannot parse JSON into OffsetDateTime: " + e.getMessage(), e);Committable suggestion skipped: line range outside the PR's diff.
| public class V9DateTimeJsonAdapter implements JsonSerializer<OffsetDateTime>, JsonDeserializer<OffsetDateTime> { | ||
| private static final Logger log = Logger.getLogger(V9DateTimeJsonAdapter.class.getName()); | ||
|
|
||
| private final Gson gson = new Gson(); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make the Gson Instance Static Final
Since Gson instances are thread-safe and there's no need for multiple instances, consider making the gson instance static final to avoid unnecessary instantiation and to indicate that it doesn't change state.
Apply this diff:
- private final Gson gson = new Gson();
+ private static final Gson GSON = new Gson();Update usages in the class accordingly:
- return gson.toJsonTree(xMLGregorianCalendarDTO, XMLGregorianCalendarDTO.class);
+ return GSON.toJsonTree(xMLGregorianCalendarDTO, XMLGregorianCalendarDTO.class);- gson.fromJson(xMLGregorianCalendarJsonElement, XMLGregorianCalendarDTO.class);
+ GSON.fromJson(xMLGregorianCalendarJsonElement, XMLGregorianCalendarDTO.class);Committable suggestion skipped: line range outside the PR's diff.
| ZoneOffset zoneoffset = ZoneOffset.ofTotalSeconds(xMLGregorianCalendarDTO.timezone * 60); | ||
| return OffsetDateTime.of( | ||
| xMLGregorianCalendarDTO.year, | ||
| xMLGregorianCalendarDTO.month, | ||
| xMLGregorianCalendarDTO.day, | ||
| xMLGregorianCalendarDTO.hour, | ||
| xMLGregorianCalendarDTO.minute, | ||
| xMLGregorianCalendarDTO.second, | ||
| (int) (xMLGregorianCalendarDTO.fractionalSecond.doubleValue() * 1000_000_000), | ||
| zoneoffset); |
There was a problem hiding this comment.
Handle Potential Null or Invalid Values in Deserialization
In the deserialize method, constructing an OffsetDateTime directly from fields that may be null or invalid can lead to runtime exceptions such as NullPointerException or DateTimeException. This can occur if the JSON does not contain all the expected fields or contains invalid values.
Consider adding validation checks for null values and ensuring that all required fields are present and valid before constructing the OffsetDateTime object.
Apply this diff to enhance validation:
+ // Validate required fields
+ if (xMLGregorianCalendarDTO == null ||
+ xMLGregorianCalendarDTO.year == null ||
+ xMLGregorianCalendarDTO.month == null ||
+ xMLGregorianCalendarDTO.day == null ||
+ xMLGregorianCalendarDTO.hour == null ||
+ xMLGregorianCalendarDTO.minute == null ||
+ xMLGregorianCalendarDTO.second == null ||
+ xMLGregorianCalendarDTO.timezone == null ||
+ xMLGregorianCalendarDTO.fractionalSecond == null) {
+ log.log(Level.FINEST, "Missing required date-time fields in JSON");
+ return null;
+ }
// Proceed with constructing OffsetDateTime
ZoneOffset zoneOffset = ZoneOffset.ofTotalSeconds(xMLGregorianCalendarDTO.timezone * 60);Committable suggestion skipped: line range outside the PR's diff.
| private static Gson getGsonBuilderWithV9Adapters() { | ||
| final Gson gson = getGsonBuilderWithCustomAdapters() | ||
| .registerTypeAdapter(OffsetDateTime.class, new V9DateTimeJsonAdapter()) | ||
| .setPrettyPrinting() | ||
| .create(); | ||
| return gson; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider refactoring to reduce duplication in getGsonBuilderWithV9Adapters and getGsonBuilderWithV10Adapters
Both methods share similar code structures, differing mainly in the OffsetDateTime type adapter registration. To enhance maintainability and reduce code duplication, consider refactoring by introducing a parameterized method that accepts the specific OffsetDateTime adapter as an argument.
Apply this refactor to combine the methods:
private static Gson getGsonBuilderWithAdapters(TypeAdapter<OffsetDateTime> offsetDateTimeAdapter) {
return getGsonBuilderWithCustomAdapters()
.registerTypeAdapter(OffsetDateTime.class, offsetDateTimeAdapter)
.setPrettyPrinting()
.create();
}
private static Gson getGsonBuilderWithV9Adapters() {
return getGsonBuilderWithAdapters(new V9DateTimeJsonAdapter());
}
private static Gson getGsonBuilderWithV10Adapters() {
return getGsonBuilderWithAdapters(new OffsetDateTimeJsonAdapter());
}
…10_SRU2024
Summary by CodeRabbit
New Features
OffsetDateTimeto maintain backward compatibility withXMLGregorianCalendar.Bug Fixes
Tests