SRU2024 v10.2 #115
SRU2024 v10.2 #115
Conversation
… SWIFT and ISO 20022
|
@coderabbitai review |
…ized from XMLGregorianCalendar objects
…cluding new messages such as trck.001.001.03 (#121)
Issue 118: Fix backward compatibility issue on DateTime fields.
…essageapplyStrategy
#131) * CU-86b15d33f_GH-118-⁃-Retro-compatibility-with-dates-between-v9-and-v10_SRU2024 Co-Authored-By: zubri <[email protected]> * rabbit code suggestions --------- Co-authored-by: zubri <[email protected]>
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files, primarily focusing on updating the Java version from 8 to 11, transitioning from the Changes
Possibly related PRs
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (124)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/OffsetTimeJsonAdapter.java (5)
18-18: 🧹 Nitpick (assertive)
Avoid using wildcard imports.
Replace the wildcard import with explicit class imports to improve code readability and prevent the inclusion of unnecessary classes.
68-71: 🛠️ Refactor suggestion
Avoid using system default time zone when offset is missing.
Using
ZoneOffset.systemDefault()may lead to inconsistent behavior across different environments due to time zone differences and daylight saving time adjustments. Consider defining a default offset or explicitly handling cases where the offset is absent.Apply this diff to set a default offset (e.g., UTC):
- offsetTime = - localTime.atOffset(ZoneOffset.systemDefault().getRules().getStandardOffset(Instant.now())); + offsetTime = localTime.atOffset(ZoneOffset.UTC);📝 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.LocalTime localTime = LocalTime.of(timeDTO.time.hour, timeDTO.time.minute, timeDTO.time.second, nano); offsetTime = localTime.atOffset(ZoneOffset.UTC); }
74-77: 🧹 Nitpick (assertive)
Catch specific exceptions and log at an appropriate level.
Catching general
Exceptioncan obscure the root cause of issues. Catch more specific exceptions likeJsonSyntaxException. Also, logging atLevel.FINESTmay cause errors to be missed; consider usingLevel.WARNINGorLevel.SEVEREfor better visibility.Apply this diff to improve error handling:
-} catch (Exception e) { - log.log(Level.FINEST, "Cannot parse JSON into OffsetTime: " + e.getMessage(), e); +} catch (JsonSyntaxException e) { + log.log(Level.SEVERE, "Cannot parse JSON into OffsetTime: " + e.getMessage(), e);Committable suggestion skipped: line range outside the PR's diff.
35-35: 🧹 Nitpick (assertive)
Make the 'gson' instance static final.
Declaring the
gsoninstance asstatic finalavoids creating multiple instances and promotes reusability.Apply this diff to make the change:
-public class OffsetTimeJsonAdapter implements JsonSerializer<OffsetTime>, JsonDeserializer<OffsetTime> { - private static final Logger log = Logger.getLogger(OffsetTimeJsonAdapter.class.getName()); - private final Gson gson = new Gson(); +public class OffsetTimeJsonAdapter implements JsonSerializer<OffsetTime>, JsonDeserializer<OffsetTime> { + private static final Logger log = Logger.getLogger(OffsetTimeJsonAdapter.class.getName()); + + private static final Gson gson = new Gson();📝 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.public class OffsetTimeJsonAdapter implements JsonSerializer<OffsetTime>, JsonDeserializer<OffsetTime> { private static final Logger log = Logger.getLogger(OffsetTimeJsonAdapter.class.getName()); private static final Gson gson = new Gson();
44-44: 🧹 Nitpick (assertive)
Remove unnecessary null check on 'offsetTime.getOffset()'.
The
getOffset()method ofOffsetTimealways returns a non-null value. The null check is redundant and can be safely removed.Apply this diff to simplify the code:
-if (offsetTime.getOffset() != null) { timeOffsetDTO.offset = new OffsetDTO(); timeOffsetDTO.offset.totalSeconds = offsetTime.getOffset().getTotalSeconds(); -}Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/ZuluOffsetDateTimeAdapter.java (3)
98-115:
⚠️ Potential issueAvoid using system default time zone in date-time parsing
Using
ZoneId.systemDefault()in theparseOffsetDateTimemethod can lead to inconsistent behavior across different environments due to varying system time zones. It's advisable to use a consistent time zone, such as UTC, when convertingLocalDateTimetoOffsetDateTime.Apply this diff to fix the issue:
- return localDateTime.atZone(ZoneId.systemDefault()).toOffsetDateTime(); + return localDateTime.atOffset(ZoneOffset.UTC);This ensures that the parsed
LocalDateTimeis consistently interpreted in UTC, preventing unexpected time shifts based on the system's default time zone.📝 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 OffsetDateTime parseOffsetDateTime(String value) { if (value == null) { return null; } try { return OffsetDateTime.parse(value); } catch (DateTimeParseException e) { log.log(Level.FINEST, "Error parsing OffsetDateTime: " + e.getMessage()); try { // Attempt to parse as LocalDateTime and assume system default time zone LocalDateTime localDateTime = LocalDateTime.parse(value); return localDateTime.atOffset(ZoneOffset.UTC); } catch (DateTimeParseException e2) { log.log(Level.FINEST, "Error parsing LocalDateTime: " + e2.getMessage()); return null; } } }
43-43: 🧹 Nitpick (assertive)
Correct the
@sincetag formatting in JavaDocThe
@sincetag has a formatting issue. It should reflect a valid version number. Replace the comma with a period to match standard versioning conventions.Apply this diff to fix the issue:
- * @since 9.4,5 + * @since 9.4.5📝 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.* @since 9.4.5
83-96: 🛠️ Refactor suggestion
Specify exceptions instead of throwing generic
ExceptionDeclaring
throws Exceptionin themarshalmethod signature is too broad. It can make exception handling cumbersome for callers. It's better to handle exceptions within the method or specify the exact exceptions that can be thrown.Apply this diff to refine the exception handling:
- public String marshal(OffsetDateTime offsetDateTime) throws Exception { + public String marshal(OffsetDateTime offsetDateTime) {Since the code uses
DateTimeFormatter.format(), which can throw anUnsupportedTemporalTypeExceptionorDateTimeException, consider handling these exceptions within the method if necessary.Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/v9/V9DateTimeJsonAdapter.java (5)
62-64: 🧹 Nitpick (assertive)
Remove unnecessary null check for
OffsetDateTime.getOffset()The method
OffsetDateTime.getOffset()never returnsnull, so the null check at line 62 is unnecessary. You can simplify the code by removing theifcondition.Apply this diff to simplify the code:
- if (offsetDateTime.getOffset() != null) { xMLGregorianCalendarDTO.timezone = offsetDateTime.getOffset().getTotalSeconds() / 60; - }📝 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.xMLGregorianCalendarDTO.timezone = offsetDateTime.getOffset().getTotalSeconds() / 60;
59-60: 🧹 Nitpick (assertive)
Preserve nanosecond precision in
fractionalSecondduring serializationCurrently, the fractional seconds are rounded to three decimal places (milliseconds) using
RoundingMode.DOWN, which may lead to loss of precision for timestamps with higher precision. Consider preserving the full nanosecond precision by adjusting the scale and rounding mode.Apply this diff to retain full nanosecond precision:
- xMLGregorianCalendarDTO.fractionalSecond = BigDecimal.valueOf(offsetDateTime.getNano()) - .divide(BigDecimal.valueOf(1_000_000_000), 3, RoundingMode.DOWN); + xMLGregorianCalendarDTO.fractionalSecond = BigDecimal.valueOf(offsetDateTime.getNano()) + .divide(BigDecimal.valueOf(1_000_000_000), 9, RoundingMode.DOWN);📝 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.xMLGregorianCalendarDTO.fractionalSecond = BigDecimal.valueOf(offsetDateTime.getNano()) .divide(BigDecimal.valueOf(1_000_000_000), 9, RoundingMode.DOWN);
99-100: 🧹 Nitpick (assertive)
Adjust logging level for exception handling in
deserializemethodThe catch block logs exceptions at
Level.FINEST, which may not be visible in production logs. To ensure that deserialization issues are noticed, consider logging atLevel.WARNINGor higher.Apply this diff to adjust the logging level:
- log.log(Level.FINEST, "Cannot parse JSON into OffsetDateTime: " + e.getMessage(), e); + log.log(Level.WARNING, "Failed to deserialize JSON into OffsetDateTime: " + e.getMessage(), e);Alternatively, you might consider throwing a custom exception to allow the caller to handle deserialization errors appropriately.
📝 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.log.log(Level.WARNING, "Failed to deserialize JSON into OffsetDateTime: " + e.getMessage(), e); return null;
88-96:
⚠️ Potential issueEnsure non-null values in
XMLGregorianCalendarDTOfields during deserializationWhile default values are assigned in
XMLGregorianCalendarDTO, if any fields arenulldue to malformed JSON input, theOffsetDateTime.ofmethod could throw aNullPointerException. Consider adding null checks or default assignments before constructingOffsetDateTime.Apply this diff to handle potential
nullvalues:+ int year = xMLGregorianCalendarDTO.year != null ? xMLGregorianCalendarDTO.year : 0; + int month = xMLGregorianCalendarDTO.month != null ? xMLGregorianCalendarDTO.month : 1; + int day = xMLGregorianCalendarDTO.day != null ? xMLGregorianCalendarDTO.day : 1; + int hour = xMLGregorianCalendarDTO.hour != null ? xMLGregorianCalendarDTO.hour : 0; + int minute = xMLGregorianCalendarDTO.minute != null ? xMLGregorianCalendarDTO.minute : 0; + int second = xMLGregorianCalendarDTO.second != null ? xMLGregorianCalendarDTO.second : 0; + BigDecimal fractionalSecond = xMLGregorianCalendarDTO.fractionalSecond != null ? xMLGregorianCalendarDTO.fractionalSecond : BigDecimal.ZERO; + int timezoneMinutes = xMLGregorianCalendarDTO.timezone != null ? xMLGregorianCalendarDTO.timezone : 0; + ZoneOffset zoneoffset = ZoneOffset.ofTotalSeconds(timezoneMinutes * 60); - 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); + return OffsetDateTime.of( + year, + month, + day, + hour, + minute, + second, + fractionalSecond.multiply(BigDecimal.valueOf(1_000_000_000)).intValue(), + zoneoffset);📝 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.int year = xMLGregorianCalendarDTO.year != null ? xMLGregorianCalendarDTO.year : 0; int month = xMLGregorianCalendarDTO.month != null ? xMLGregorianCalendarDTO.month : 1; int day = xMLGregorianCalendarDTO.day != null ? xMLGregorianCalendarDTO.day : 1; int hour = xMLGregorianCalendarDTO.hour != null ? xMLGregorianCalendarDTO.hour : 0; int minute = xMLGregorianCalendarDTO.minute != null ? xMLGregorianCalendarDTO.minute : 0; int second = xMLGregorianCalendarDTO.second != null ? xMLGregorianCalendarDTO.second : 0; BigDecimal fractionalSecond = xMLGregorianCalendarDTO.fractionalSecond != null ? xMLGregorianCalendarDTO.fractionalSecond : BigDecimal.ZERO; int timezoneMinutes = xMLGregorianCalendarDTO.timezone != null ? xMLGregorianCalendarDTO.timezone : 0; ZoneOffset zoneoffset = ZoneOffset.ofTotalSeconds(timezoneMinutes * 60); return OffsetDateTime.of( year, month, day, hour, minute, second, fractionalSecond.multiply(BigDecimal.valueOf(1_000_000_000)).intValue(), zoneoffset);
96-96: 🧹 Nitpick (assertive)
Avoid precision loss when converting
fractionalSecondback to nanosecondsIn the
deserializemethod, usingdoubleValue()to convertfractionalSecondmay cause precision loss due to floating-point representation. Consider usingBigDecimalmethods to maintain exact precision.Apply this diff to improve precision:
- (int) (xMLGregorianCalendarDTO.fractionalSecond.doubleValue() * 1000_000_000), + xMLGregorianCalendarDTO.fractionalSecond + .multiply(BigDecimal.valueOf(1_000_000_000)) + .intValue(),📝 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.xMLGregorianCalendarDTO.fractionalSecond .multiply(BigDecimal.valueOf(1_000_000_000)) .intValue(),iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/OffsetDateTimeJsonAdapter.java (8)
39-39: 🧹 Nitpick (assertive)
Consider reusing a configured
GsoninstanceThe adapter creates a new
Gsoninstance with default settings. For consistency and to apply any custom configurations (e.g., date formats, null handling), consider injecting a pre-configuredGsoninstance or using a sharedGsonBuilder.
34-34: 🧹 Nitpick (assertive)
Update the
@sincetag to reflect the correct versionThe Javadoc
@sincetag currently states10.0.1. Given that this is for version10.2, update the tag to accurately reflect the introduction version.Apply this diff to update the version:
* @since 10.0.1 + * @since 10.2Committable suggestion skipped: line range outside the PR's diff.
64-81: 🛠️ Refactor suggestion
Catch specific exceptions during deserialization
Catching the generic
Exceptioncan obscure the root cause of failures and make debugging harder. It's better to catch more specific exceptions likeJsonSyntaxExceptionorJsonParseException.Apply this diff to catch specific exceptions:
try { // Parse DTO from current OffsetDateTime model return deserializeFromOffsetDateTimeDTO(gson.fromJson(jsonElement, DateTimeOffsetDTO.class)); - } catch (final Exception e) { + } catch (final JsonSyntaxException | JsonParseException e) { log.log( Level.FINEST, "Cannot parse JSON into OffsetDateTime from current DTO format: " + e.getMessage(), e); } log.log(Level.FINEST, "Attempting parsing from legacy DTO format"); try { // Parse DTO from legacy XMLGregorianCalendar model return deserializeFromXMLGregorianCalendarDTO(gson.fromJson(jsonElement, XMLGregorianCalendarDTO.class)); - } catch (final Exception e) { + } catch (final JsonSyntaxException | JsonParseException e) { log.log(Level.FINEST, "Cannot parse JSON into OffsetDateTime from legacy DTO format: " + e.getMessage(), e); }Committable suggestion skipped: line range outside the PR's diff.
80-81:
⚠️ Potential issueAvoid returning
nullon deserialization failureReturning
nullmay lead toNullPointerExceptionin calling code. Consider throwing aJsonParseExceptionto explicitly indicate deserialization failure.Apply this diff to throw an exception:
} - return null; + throw new JsonParseException("Unable to parse JSON into OffsetDateTime"); }Committable suggestion skipped: line range outside the PR's diff.
150-154:
⚠️ Potential issueHandle potential
ArithmeticExceptionduring nanosecond conversionConverting the scaled
fractionalSecondto anintmay throw anArithmeticExceptionif the value exceeds the integer range. Ensure to handle this exception or validate the value before conversion.Apply this diff to safely convert nanoseconds:
nanoseconds = xmlGregorianCalendarDTO .fractionalSecond .scaleByPowerOfTen(9) - .toBigInteger() - .intValueExact(); + .intValueExact();Or, handle the exception:
+ try { nanoseconds = xmlGregorianCalendarDTO .fractionalSecond .scaleByPowerOfTen(9) .toBigInteger() .intValueExact(); + } catch (ArithmeticException e) { + throw new JsonParseException("Fractional second value out of int range", e); + }Committable suggestion skipped: line range outside the PR's diff.
68-72: 🧹 Nitpick (assertive)
Adjust logging level for deserialization failures
Currently, deserialization exceptions are logged at
Level.FINEST, which may not be visible in production logs. Consider increasing the logging level toLevel.WARNINGorLevel.INFOto make these issues more apparent during runtime.Apply this diff to adjust the logging level:
- log.log( - Level.FINEST, + log.log( + Level.WARNING, "Cannot parse JSON into OffsetDateTime from current DTO format: " + e.getMessage(), e);📝 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.log.log( Level.WARNING, "Cannot parse JSON into OffsetDateTime from current DTO format: " + e.getMessage(), e); }
162-198: 🧹 Nitpick (assertive)
Restrict access to inner DTO classes by making them
privateThe DTO classes are only used within
OffsetDateTimeJsonAdapter. Consider declaring them asprivate staticto encapsulate implementation details and prevent unintended usage outside the adapter.Apply this diff to adjust the access modifiers:
-static class DateTimeOffsetDTO { +private static class DateTimeOffsetDTO { -static class DateTimeDTO { +private static class DateTimeDTO { -static class DateDTO { +private static class DateDTO { -static class TimeDTO { +private static class TimeDTO { -static class OffsetDTO { +private static class OffsetDTO { -static class XMLGregorianCalendarDTO { +private static class XMLGregorianCalendarDTO {📝 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 class DateTimeOffsetDTO { DateTimeDTO dateTime = new DateTimeDTO(); OffsetDTO offset; } private static class DateTimeDTO { DateDTO date = new DateDTO(); TimeDTO time = new TimeDTO(); } private static class DateDTO { Integer year; Integer month; Integer day; } private static class TimeDTO { Integer hour = 0; Integer minute = 0; Integer second = 0; Integer nano = 0; } private static class OffsetDTO { Integer totalSeconds = 0; } private static class XMLGregorianCalendarDTO { Integer year; Integer month; Integer day; Integer hour = 0; Integer minute = 0; Integer second = 0; BigDecimal fractionalSecond = BigDecimal.ZERO; Integer timezone; }
53-56: 🛠️ Refactor suggestion
Remove unnecessary null check for
getOffset()The
OffsetDateTime#getOffset()method always returns a non-nullZoneOffset. The null check is unnecessary and can be safely removed to simplify the code.Apply this diff to remove the unnecessary null check:
- if (offsetDateTime.getOffset() != null) { dateTimeOffsetDTO.offset = new OffsetDTO(); dateTimeOffsetDTO.offset.totalSeconds = offsetDateTime.getOffset().getTotalSeconds(); - }Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/BusinessAppHdrV02.java (1)
24-31: 💡 Codebase verification
Migration from
javax.xml.bindtojakarta.xml.bindRequires Attention in Generated CodeThe verification shows that
javax.xml.bindimports are only present in generated Java files under themodel-seev-typesandmodel-auth-typesdirectories. While the manual changes inBusinessAppHdrV02.javacorrectly usejakarta.xml.bind, the generated code still uses the oldjavax.xml.bindpackage.
model-seev-types/src/generated/java/com/prowidesoftware/swift/model/mx/dic/RejectedStatusReason56.javamodel-auth-types/src/generated/java/com/prowidesoftware/swift/model/mx/dic/FloatingRateIdentification8Choice.javaYou'll need to update the code generation templates or configuration to use
jakarta.xml.bindinstead ofjavax.xml.bindto ensure consistency across the codebase.🔗 Analysis chain
Ensure Complete Migration from
javax.xml.bindtojakarta.xml.bindThe import statements have been updated from
javax.xml.bindtojakarta.xml.bind. Please verify that all references tojavax.xml.bindhave been updated throughout the codebase to prevent compatibility issues.Run the following script to verify that no references to
javax.xml.bindremain in the codebase:
Let me gather more information to understand if these are generated files and if there are any non-generated files still using javax.xml.bind.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any occurrences of 'javax.xml.bind' in Java files. # Expected result: No occurrences of 'javax.xml.bind' should be found. rg 'javax\.xml\.bind' -t javaLength of output: 1269
Script:
#!/bin/bash # Search for javax.xml.bind in non-generated Java files rg 'javax\.xml\.bind' -t java --glob '!**/generated/**' # Also check if there are any pom.xml files that might have javax.xml.bind dependencies rg 'javax\.xml\.bind' -t xmlLength of output: 88
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/AbstractMX.java (1)
621-634: 🧹 Nitpick (assertive)
Refactor
getGsonBuildermethods to reduce code duplicationThe methods
getGsonBuilderWithV9Adapters()andgetGsonBuilderWithV10Adapters()are nearly identical, differing only in the adapter registered forOffsetDateTime. Consider refactoring to a single method that accepts the adapter as a parameter to reduce code duplication and improve maintainability.Apply this diff to implement the suggested refactor:
@@ -611,29 +611,25 @@ } - private static Gson getGsonBuilderWithV9Adapters() { - final Gson gson = getGsonBuilderWithCustomAdapters() - .registerTypeAdapter(OffsetDateTime.class, new V9DateTimeJsonAdapter()) - .setPrettyPrinting() - .create(); - return gson; - } - - private static Gson getGsonBuilderWithV10Adapters() { - final Gson gson = getGsonBuilderWithCustomAdapters() - .registerTypeAdapter(OffsetDateTime.class, new OffsetDateTimeJsonAdapter()) - .setPrettyPrinting() - .create(); - return gson; + private static Gson getGsonBuilderWithAdapters(TypeAdapter<OffsetDateTime> adapter) { + return getGsonBuilderWithCustomAdapters() + .registerTypeAdapter(OffsetDateTime.class, adapter) + .setPrettyPrinting() + .create(); } + private static Gson getGsonBuilderWithV9Adapters() { + return getGsonBuilderWithAdapters(new V9DateTimeJsonAdapter()); + } + + private static Gson getGsonBuilderWithV10Adapters() { + return getGsonBuilderWithAdapters(new OffsetDateTimeJsonAdapter()); + } + /** * @return same as {@link #getNamespace()} * @since 9.1.2 */ public String targetNamespace() { return getNamespace(); } }This refactoring centralizes the creation of
Gsoninstances and makes it easier to manage adapter configurations.📝 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 Gson getGsonBuilderWithAdapters(TypeAdapter<OffsetDateTime> adapter) { return getGsonBuilderWithCustomAdapters() .registerTypeAdapter(OffsetDateTime.class, adapter) .setPrettyPrinting() .create(); } private static Gson getGsonBuilderWithV9Adapters() { return getGsonBuilderWithAdapters(new V9DateTimeJsonAdapter()); } private static Gson getGsonBuilderWithV10Adapters() { return getGsonBuilderWithAdapters(new OffsetDateTimeJsonAdapter()); }iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/AbstractMxJsonTestAdapters.java (9)
60-60: 🧹 Nitpick (assertive)
Remove unnecessary commented-out code
There are commented-out
System.out.printlnstatements on lines 60 and 74. If these are no longer needed, consider removing them to clean up the code.Apply this diff to remove the commented lines:
- // System.out.println(mx.message()); - // System.out.println(mx2.message());Also applies to: 74-74
30-30:
⚠️ Potential issueTypographical error in Javadoc
@sincetagThe Javadoc comment on line 30 has an extra
@symbol at the end. It should be corrected to maintain proper documentation standards.Apply this diff to fix the typo:
- * @since 7.10.2@ + * @since 7.10.2📝 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.* @since 7.10.2
343-343: 🧹 Nitpick (assertive)
Add assertion to verify parsed MX message
In
parseMxWithAppHdr_LocalDate(), consider asserting that the parsedmxobject is not null to strengthen the test.Apply this change:
assertDoesNotThrow(() -> AbstractMX.fromJson(json)); + AbstractMX mx = AbstractMX.fromJson(json); + assertNotNull(mx);Committable suggestion skipped: line range outside the PR's diff.
547-549:
⚠️ Potential issueEnsure safe casting of
mx.getAppHdr()to preventClassCastExceptionIn the method
parseSerializedMxWithAppHdrBAH_V2_OffsetDateTime(), castingmx.getAppHdr()toBusinessAppHdrV02without checking its type risks aClassCastException.Apply this diff to verify the instance before casting:
- BusinessAppHdrV02 BAH_V2 = (BusinessAppHdrV02) mx.getAppHdr(); + assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); + BusinessAppHdrV02 BAH_V2 = (BusinessAppHdrV02) mx.getAppHdr();📝 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.assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); BusinessAppHdrV02 BAH_V2 = (BusinessAppHdrV02) mx.getAppHdr(); assertNotNull(BAH_V2); }
1156-1158:
⚠️ Potential issueEnsure safe casting of
mx.getAppHdr()to preventClassCastExceptionIn
testJSONOffsetDateTimeWithNoNano_TimeWithNoNanoNoOffset(), ensure that the app header is of the expected type before casting.Apply this diff to verify the instance before casting:
- BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr(); + assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); + BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr();📝 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.assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr(); assertNotNull(businessAppHdrV02); }
617-619:
⚠️ Potential issueEnsure safe casting of
mx.getAppHdr()to preventClassCastExceptionIn
parseSerializedMxWithAppHdrNoNameSpace_OffsetDateTime(), castingmx.getAppHdr()toLegacyAppHdrwithout type checking may cause aClassCastException.Apply this diff to verify the instance before casting:
- LegacyAppHdr legacyAppHdr = (LegacyAppHdr) mx.getAppHdr(); + assertTrue(mx.getAppHdr() instanceof LegacyAppHdr); + LegacyAppHdr legacyAppHdr = (LegacyAppHdr) mx.getAppHdr();📝 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.assertTrue(mx.getAppHdr() instanceof LegacyAppHdr); LegacyAppHdr legacyAppHdr = (LegacyAppHdr) mx.getAppHdr(); assertNotNull(legacyAppHdr); }
477-479:
⚠️ Potential issueEnsure safe casting of
mx.getAppHdr()to preventClassCastExceptionIn the method
parseSerializedMxWithAppHdrBAH_V1_OffsetDateTime(), you castmx.getAppHdr()toBusinessAppHdrV01without checking its actual type. This may lead to aClassCastExceptionif the header is not of the expected type.Apply this diff to verify the instance before casting:
- BusinessAppHdrV01 BAH_V1 = (BusinessAppHdrV01) mx.getAppHdr(); + assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV01); + BusinessAppHdrV01 BAH_V1 = (BusinessAppHdrV01) mx.getAppHdr();📝 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.assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV01); BusinessAppHdrV01 BAH_V1 = (BusinessAppHdrV01) mx.getAppHdr(); assertNotNull(BAH_V1); }
771-772:
⚠️ Potential issueEnsure safe casting of
mx.getAppHdr()and handle invalid namespacesIn the method
parseSerializedMxWithAppHdrInvalidNamespace_OffsetDateTime(), casting without checking the header type may lead to runtime exceptions, especially given the invalid namespace in the test data.Consider adding instance checks and handling for unexpected namespaces:
+ assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr(); assertNotNull(businessAppHdrV02);Also, handle the case where the namespace does not match any known headers.
Committable suggestion skipped: line range outside the PR's diff.
1272-1274:
⚠️ Potential issueEnsure safe casting of
mx.getAppHdr()to preventClassCastExceptionIn
testJSONYearMonth(), verify the type ofmx.getAppHdr()before casting to prevent potential runtime exceptions.Apply this diff:
- BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr(); + assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); + BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr();📝 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.assertTrue(mx.getAppHdr() instanceof BusinessAppHdrV02); BusinessAppHdrV02 businessAppHdrV02 = (BusinessAppHdrV02) mx.getAppHdr(); assertNotNull(businessAppHdrV02); }.idea/compiler.xml (1)
4-7: 🧹 Nitpick (assertive)
Document the version compatibility strategy.
Consider adding a comment in the project's README or developer documentation explaining:
- Why core modules maintain Java 8 compatibility
- Which modules can use Java 11 features
- Guidelines for developers to maintain version compatibility
Would you like me to help draft this documentation section?
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/sys/dic/package-info.java (1)
27-28: 🧹 Nitpick (assertive)
Consider documenting Jakarta EE version requirements
Since this change is part of the migration from Java 8 to 11 and javax to Jakarta namespaces, it would be helpful to:
- Document the minimum required Jakarta EE version in the project's README or documentation
- Consider adding version constraints in the build configuration to prevent accidental use of incompatible dependencies
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/YearJsonAdapter.java (3)
43-45: 🧹 Nitpick (assertive)
Improve DTO encapsulation and Gson integration
The DTO class could be improved with better encapsulation and explicit Gson annotations.
Suggested improvements:
- static class YearJsonDTO { - int year; + private static class YearJsonDTO { + @Expose + private int year; + + public int getYear() { + return year; + } + + public void setYear(int year) { + this.year = year; + } }📝 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 class YearJsonDTO { @Expose private int year; public int getYear() { return year; } public void setYear(int year) { this.year = year; } }
22-26: 🧹 Nitpick (assertive)
Enhance class documentation with more details
The current documentation could be more comprehensive. Consider adding:
- The expected JSON format (structure and example)
- Usage examples
- Relationship with other date/time adapters in the system
Example improvement:
/** * This adapter enables accepting Year time Json format. + * + * Example JSON format: + * { + * "year": 2024 + * } + * + * Usage: + * Gson gson = new GsonBuilder() + * .registerTypeAdapter(Year.class, new YearJsonAdapter()) + * .create(); + * + * @see com.prowidesoftware.swift.model.mx.adapters.LocalDateJsonAdapter + * @see com.prowidesoftware.swift.model.mx.adapters.OffsetDateTimeJsonAdapter * * @since 10.1.0 */Committable suggestion skipped: line range outside the PR's diff.
27-41:
⚠️ Potential issueFix potential issues in JSON serialization/deserialization
Several critical issues need to be addressed:
- The
gsonfield creates a new Gson instance which could lead to infinite recursion when serializing/deserializing nested objects.- Missing null checks in both methods.
- No validation for year value range in deserialization.
Apply these improvements:
public class YearJsonAdapter implements JsonSerializer<Year>, JsonDeserializer<Year> { - private final Gson gson = new Gson(); + private static final Gson gson = new GsonBuilder() + .excludeFieldsWithoutExposeAnnotation() + .create(); @Override public JsonElement serialize(Year year, Type type, JsonSerializationContext jsonSerializationContext) { + if (year == null) { + return JsonNull.INSTANCE; + } YearJsonDTO yearJsonDTO = new YearJsonDTO(); yearJsonDTO.year = year.getValue(); - return gson.toJsonTree(yearJsonDTO, YearJsonDTO.class); + return jsonSerializationContext.serialize(yearJsonDTO); } @Override public Year deserialize(JsonElement jsonElement, Type type, JsonDeserializationContext jsonDeserializationContext) { - YearJsonDTO yearJsonDTO = gson.fromJson(jsonElement, YearJsonDTO.class); + if (jsonElement.isJsonNull()) { + return null; + } + YearJsonDTO yearJsonDTO = jsonDeserializationContext.deserialize(jsonElement, YearJsonDTO.class); + if (yearJsonDTO.year < Year.MIN_VALUE || yearJsonDTO.year > Year.MAX_VALUE) { + throw new JsonParseException("Year value " + yearJsonDTO.year + " is outside valid range"); + } return Year.of(yearJsonDTO.year); }Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/YearJsonAdapterTest.java (4)
37-42:
⚠️ Potential issueUpdate implementation to use non-deprecated API and add validation
The current implementation uses a deprecated JsonParser constructor and lacks proper validation.
private void testSerializationAndDeserializationImpl(String json) { - JsonElement jsonElement = new JsonParser().parse(json); + JsonElement jsonElement = JsonParser.parseString(json); + + // Validate JSON structure + if (!jsonElement.isJsonObject() || !jsonElement.getAsJsonObject().has("year")) { + throw new IllegalArgumentException("Invalid JSON structure. Expected {\"year\": <value>}"); + } + Year year = adapter.deserialize(jsonElement, null, null); + if (year == null) { + throw new AssertionError("Deserialized Year should not be null"); + } + JsonElement valueDateResult = adapter.serialize(year, null, null); + if (valueDateResult == null) { + throw new AssertionError("Serialized JsonElement should not be null"); + } + assertEquals(json, valueDateResult.toString()); }📝 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 testSerializationAndDeserializationImpl(String json) { JsonElement jsonElement = JsonParser.parseString(json); // Validate JSON structure if (!jsonElement.isJsonObject() || !jsonElement.getAsJsonObject().has("year")) { throw new IllegalArgumentException("Invalid JSON structure. Expected {\"year\": <value>}"); } Year year = adapter.deserialize(jsonElement, null, null); if (year == null) { throw new AssertionError("Deserialized Year should not be null"); } JsonElement valueDateResult = adapter.serialize(year, null, null); if (valueDateResult == null) { throw new AssertionError("Serialized JsonElement should not be null"); } assertEquals(json, valueDateResult.toString()); }
29-35: 🛠️ Refactor suggestion
Enhance test coverage with more test cases
The current test implementation has duplicate test cases and limited coverage. Consider:
- Removing the duplicate cases for year 2019
- Adding edge cases (e.g., Year.MIN_VALUE, Year.MAX_VALUE)
- Adding invalid year format tests
- Testing different year representations
@Test void testSerializationAndDeserialization() { - testSerializationAndDeserializationImpl("{\"year\":2019}"); - testSerializationAndDeserializationImpl("{\"year\":2019}"); - testSerializationAndDeserializationImpl("{\"year\":2019}"); - testSerializationAndDeserializationImpl("{\"year\":1983}"); + // Valid cases + testSerializationAndDeserializationImpl("{\"year\":2019}"); + testSerializationAndDeserializationImpl("{\"year\":1983}"); + + // Edge cases + testSerializationAndDeserializationImpl("{\"year\":-999999999}"); // Year.MIN_VALUE + testSerializationAndDeserializationImpl("{\"year\":999999999}"); // Year.MAX_VALUE + testSerializationAndDeserializationImpl("{\"year\":0}"); // Year zero } + +@Test +void testInvalidYearDeserialization() { + assertThrows(JsonParseException.class, () -> + testSerializationAndDeserializationImpl("{\"year\":\"invalid\"}")); + assertThrows(DateTimeException.class, () -> + testSerializationAndDeserializationImpl("{\"year\":1000000000}")); +}Committable suggestion skipped: line range outside the PR's diff.
25-28: 🧹 Nitpick (assertive)
Improve test class organization and documentation
Consider enhancing the test class with better organization and documentation:
- Add class-level documentation explaining the test coverage
- Group tests by categories (valid cases, edge cases, error cases)
- Add tests for null handling
+/** + * Test class for {@link YearJsonAdapter} that validates JSON serialization and deserialization + * of {@link Year} objects. + * + * Test categories: + * - Valid year serialization/deserialization + * - Edge cases (MIN/MAX values) + * - Error cases (invalid format, out of range) + * - Null handling + */ public class YearJsonAdapterTest { private final YearJsonAdapter adapter = new YearJsonAdapter(); + + @Test + void testNullHandling() { + assertNull(adapter.deserialize(null, null, null)); + assertNull(adapter.serialize(null, null, null)); + }📝 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./** * Test class for {@link YearJsonAdapter} that validates JSON serialization and deserialization * of {@link Year} objects. * * Test categories: * - Valid year serialization/deserialization * - Edge cases (MIN/MAX values) * - Error cases (invalid format, out of range) * - Null handling */ public class YearJsonAdapterTest { private final YearJsonAdapter adapter = new YearJsonAdapter(); @Test void testNullHandling() { assertNull(adapter.deserialize(null, null, null)); assertNull(adapter.serialize(null, null, null)); }
18-23: 🛠️ Refactor suggestion
Add missing imports for enhanced test coverage
Add the following imports to support the suggested improvements:
import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.gson.JsonElement; import com.google.gson.JsonParser; +import com.google.gson.JsonParseException; import java.time.Year; +import java.time.DateTimeException; import org.junit.jupiter.api.Test;Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/test/java/com/prowidesoftware/issues/Jira1567.java (2)
41-46:
⚠️ Potential issueImprove CustomDateTimeAdapter robustness
The current implementation has several potential issues:
- It assumes the input is always in UTC ("+00:00")
- Direct string replacement is fragile
- Missing unmarshal implementation for symmetry
Consider this more robust implementation:
public class CustomDateTimeAdapter extends OffsetDateTimeAdapter { @Override public String marshal(OffsetDateTime offsetDateTime) throws Exception { - return StringUtils.replace(super.marshal(offsetDateTime), "+00:00", "Z"); + if (offsetDateTime == null) { + return null; + } + // First normalize to UTC + OffsetDateTime utc = offsetDateTime.withOffsetSameInstant(ZoneOffset.UTC); + // Then format with 'Z' + return utc.format(DateTimeFormatter.ISO_INSTANT); + } + + @Override + public OffsetDateTime unmarshal(String value) throws Exception { + if (value == null) { + return null; + } + return OffsetDateTime.parse(value, DateTimeFormatter.ISO_INSTANT); + } }📝 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.public class CustomDateTimeAdapter extends OffsetDateTimeAdapter { @Override public String marshal(OffsetDateTime offsetDateTime) throws Exception { if (offsetDateTime == null) { return null; } // First normalize to UTC OffsetDateTime utc = offsetDateTime.withOffsetSameInstant(ZoneOffset.UTC); // Then format with 'Z' return utc.format(DateTimeFormatter.ISO_INSTANT); } @Override public OffsetDateTime unmarshal(String value) throws Exception { if (value == null) { return null; } return OffsetDateTime.parse(value, DateTimeFormatter.ISO_INSTANT); } }
17-39: 🧹 Nitpick (assertive)
Enhance test coverage and naming
While the test logic is correct, consider these improvements for better maintainability and reliability:
- Rename the test method to describe its purpose (e.g.,
testDateTimeSerializationFormats)- Add more specific assertions beyond simple string contains
- Validate the complete XML structure
- Include negative test cases (e.g., non-UTC timezones)
Here's a suggested improvement:
- @Test - public void test() { + @Test + public void testDateTimeSerializationFormats() { MxXsys01500101 mx = new MxXsys01500101(); // create an OffsetDateTime with the current time in UTC OffsetDateTime now = OffsetDateTime.now(ZoneOffset.UTC); LegacyAppHdr legacy = new LegacyAppHdr(); legacy.setCrDate(now); mx.setAppHdr(legacy); // default date time adapter contains the offset String xml = mx.message(); - assertTrue(xml.contains("+00:00</head:CrDate>")); + assertTrue(xml.matches(".*<head:CrDate>\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\+00:00</head:CrDate>.*")); // custom serialization using a custom date time adapter MxWriteConfiguration config = new MxWriteConfiguration(); config.adapters.dateTimeAdapter = new IsoDateTimeAdapter(new CustomDateTimeAdapter()); xml = mx.message(config); - assertTrue(xml.contains("Z</head:CrDate>")); + assertTrue(xml.matches(".*<head:CrDate>\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z</head:CrDate>.*")); + + // Add negative test case + OffsetDateTime nonUtc = OffsetDateTime.now(ZoneOffset.ofHours(2)); + legacy.setCrDate(nonUtc); + xml = mx.message(config); + assertTrue(xml.matches(".*<head:CrDate>\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z</head:CrDate>.*"), + "Non-UTC time should be converted to UTC with Z suffix"); }📝 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.@Test public void testDateTimeSerializationFormats() { MxXsys01500101 mx = new MxXsys01500101(); // create an OffsetDateTime with the current time in UTC OffsetDateTime now = OffsetDateTime.now(ZoneOffset.UTC); LegacyAppHdr legacy = new LegacyAppHdr(); legacy.setCrDate(now); mx.setAppHdr(legacy); // default date time adapter contains the offset String xml = mx.message(); assertTrue(xml.matches(".*<head:CrDate>\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\+00:00</head:CrDate>.*")); // custom serialization using a custom date time adapter MxWriteConfiguration config = new MxWriteConfiguration(); config.adapters.dateTimeAdapter = new IsoDateTimeAdapter(new CustomDateTimeAdapter()); xml = mx.message(config); assertTrue(xml.matches(".*<head:CrDate>\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z</head:CrDate>.*")); // Add negative test case OffsetDateTime nonUtc = OffsetDateTime.now(ZoneOffset.ofHours(2)); legacy.setCrDate(nonUtc); xml = mx.message(config); assertTrue(xml.matches(".*<head:CrDate>\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}Z</head:CrDate>.*"), "Non-UTC time should be converted to UTC with Z suffix"); }iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/LocalDateJsonAdapterTest.java (3)
29-35: 🧹 Nitpick (assertive)
Enhance test coverage with additional test cases
While the current test cases cover valid scenarios, consider adding:
- Edge cases (min/max dates)
- Invalid JSON input (error handling)
- Null handling
- Malformed date components (e.g., invalid month/day values)
Example additional test cases:
// Edge cases testSerializationAndDeserializationImpl("{\"year\":1,\"month\":1,\"day\":1}"); testSerializationAndDeserializationImpl("{\"year\":9999,\"month\":12,\"day\":31}"); // Invalid cases (add appropriate exception handling) @Test void testInvalidInput() { assertThrows(JsonParseException.class, () -> testSerializationAndDeserializationImpl("{\"year\":2019,\"month\":13,\"day\":32}")); assertThrows(JsonParseException.class, () -> testSerializationAndDeserializationImpl("{\"year\":2019}")); assertThrows(JsonParseException.class, () -> testSerializationAndDeserializationImpl("invalid json")); }
21-21: 🛠️ Refactor suggestion
Replace deprecated JsonParser usage
The
JsonParserclass is deprecated since Gson 2.8.9. Consider usingJsonParser.parseString()static method instead.-import com.google.gson.JsonParser; +import com.google.gson.JsonParser; // For static parseString methodAnd in the implementation:
- JsonElement jsonElement = new JsonParser().parse(json); + JsonElement jsonElement = JsonParser.parseString(json);Committable suggestion skipped: line range outside the PR's diff.
37-42: 🧹 Nitpick (assertive)
Improve implementation method robustness
The current implementation could be enhanced with:
- Input validation
- Explicit error handling
- More descriptive assertion message
- private void testSerializationAndDeserializationImpl(String json) { + private void testSerializationAndDeserializationImpl(String json) { + if (json == null || json.trim().isEmpty()) { + throw new IllegalArgumentException("JSON input cannot be null or empty"); + } + JsonElement jsonElement = new JsonParser().parse(json); LocalDate date = adapter.deserialize(jsonElement, null, null); JsonElement valueDateResult = adapter.serialize(date, null, null); - assertEquals(json, valueDateResult.toString()); + assertEquals(json, valueDateResult.toString(), + String.format("Serialization/deserialization failed for input: %s", json)); }📝 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 testSerializationAndDeserializationImpl(String json) { if (json == null || json.trim().isEmpty()) { throw new IllegalArgumentException("JSON input cannot be null or empty"); } JsonElement jsonElement = new JsonParser().parse(json); LocalDate date = adapter.deserialize(jsonElement, null, null); JsonElement valueDateResult = adapter.serialize(date, null, null); assertEquals(json, valueDateResult.toString(), String.format("Serialization/deserialization failed for input: %s", json)); }iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/YearMonthJsonAdapter.java (4)
31-36: 🛠️ Refactor suggestion
Add null check in serialize method.
The method should handle null YearMonth objects gracefully.
@Override public JsonElement serialize(YearMonth yearMonth, Type type, JsonSerializationContext jsonSerializationContext) { + if (yearMonth == null) { + return JsonNull.INSTANCE; + } YearMonthJsonDTO yearJsonDTO = new YearMonthJsonDTO(); yearJsonDTO.year = yearMonth.getYear(); yearJsonDTO.month = yearMonth.getMonth().getValue(); return gson.toJsonTree(yearJsonDTO, YearMonthJsonDTO.class); }📝 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.public JsonElement serialize(YearMonth yearMonth, Type type, JsonSerializationContext jsonSerializationContext) { if (yearMonth == null) { return JsonNull.INSTANCE; } YearMonthJsonDTO yearJsonDTO = new YearMonthJsonDTO(); yearJsonDTO.year = yearMonth.getYear(); yearJsonDTO.month = yearMonth.getMonth().getValue(); return gson.toJsonTree(yearJsonDTO, YearMonthJsonDTO.class); }
27-29: 🧹 Nitpick (assertive)
Consider making Gson instance static and final.
The Gson instance could be shared across all instances of the adapter since it's stateless.
- private final Gson gson = new Gson(); + private static final Gson gson = new Gson();📝 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.public class YearMonthJsonAdapter implements JsonSerializer<YearMonth>, JsonDeserializer<YearMonth> { private static final Gson gson = new Gson();
45-48: 🧹 Nitpick (assertive)
Consider improving DTO encapsulation.
The DTO could benefit from better encapsulation:
- Make the class private
- Make fields private with getters/setters
- static class YearMonthJsonDTO { - int year; - int month; + private static class YearMonthJsonDTO { + private int year; + private int month; + + public int getYear() { + return year; + } + + public void setYear(int year) { + this.year = year; + } + + public int getMonth() { + return month; + } + + public void setMonth(int month) { + this.month = month; + } }📝 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 class YearMonthJsonDTO { private int year; private int month; public int getYear() { return year; } public void setYear(int year) { this.year = year; } public int getMonth() { return month; } public void setMonth(int month) { this.month = month; } }
38-43: 🛠️ Refactor suggestion
Add validation in deserialize method.
The method should validate the input and handle edge cases:
- Null checks
- Month value range validation (1-12)
@Override public YearMonth deserialize( JsonElement jsonElement, Type type, JsonDeserializationContext jsonDeserializationContext) { + if (jsonElement == null || jsonElement.isJsonNull()) { + return null; + } YearMonthJsonDTO yearMonthJsonDTO = gson.fromJson(jsonElement, YearMonthJsonDTO.class); + if (yearMonthJsonDTO.month < 1 || yearMonthJsonDTO.month > 12) { + throw new JsonParseException("Month value must be between 1 and 12, got: " + yearMonthJsonDTO.month); + } return YearMonth.of(yearMonthJsonDTO.year, yearMonthJsonDTO.month); }📝 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.@Override public YearMonth deserialize( JsonElement jsonElement, Type type, JsonDeserializationContext jsonDeserializationContext) { if (jsonElement == null || jsonElement.isJsonNull()) { return null; } YearMonthJsonDTO yearMonthJsonDTO = gson.fromJson(jsonElement, YearMonthJsonDTO.class); if (yearMonthJsonDTO.month < 1 || yearMonthJsonDTO.month > 12) { throw new JsonParseException("Month value must be between 1 and 12, got: " + yearMonthJsonDTO.month); } return YearMonth.of(yearMonthJsonDTO.year, yearMonthJsonDTO.month); }iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/LocalDateJsonAdapter.java (5)
29-29: 🧹 Nitpick (assertive)
Consider making Gson instance static and thread-safe.
The current implementation creates a new Gson instance per adapter instance. Consider making it static and configuring it with appropriate settings.
- private final Gson gson = new Gson(); + private static final Gson gson = new GsonBuilder() + .serializeNulls() + .create();📝 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 final Gson gson = new GsonBuilder() .serializeNulls() .create();
31-38: 🛠️ Refactor suggestion
Add null check in serialize method.
The method should handle null input gracefully.
@Override public JsonElement serialize(LocalDate localDate, Type typeOfSrc, JsonSerializationContext context) { + if (localDate == null) { + return JsonNull.INSTANCE; + } DateDTO date = new DateDTO(); date.year = localDate.getYear(); date.month = localDate.getMonthValue(); date.day = localDate.getDayOfMonth(); return gson.toJsonTree(date, DateDTO.class); }📝 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.@Override public JsonElement serialize(LocalDate localDate, Type typeOfSrc, JsonSerializationContext context) { if (localDate == null) { return JsonNull.INSTANCE; } DateDTO date = new DateDTO(); date.year = localDate.getYear(); date.month = localDate.getMonthValue(); date.day = localDate.getDayOfMonth(); return gson.toJsonTree(date, DateDTO.class); }
22-26: 🧹 Nitpick (assertive)
Enhance class documentation.
The current documentation is minimal. Consider adding:
- Example JSON format
- Thread-safety considerations
- Null handling behavior
/** * This adapter enables accepting LocalDate time Json format. + * + * Example JSON format: + * { + * "year": 2024, + * "month": 11, + * "day": 15 + * } + * + * Thread-safety: This adapter maintains a shared Gson instance. + * Null handling: null input results in null output. * * @since 10.1.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./** * This adapter enables accepting LocalDate time Json format. * * Example JSON format: * { * "year": 2024, * "month": 11, * "day": 15 * } * * Thread-safety: This adapter maintains a shared Gson instance. * Null handling: null input results in null output. * * @since 10.1.0 */
40-46: 🛠️ Refactor suggestion
Add validation in deserialize method.
The method should:
- Handle null/invalid input
- Validate date values before construction
@Override public LocalDate deserialize( final JsonElement jsonElement, final Type typeOfT, final JsonDeserializationContext context) throws JsonParseException { + if (jsonElement == null || jsonElement.isJsonNull()) { + return null; + } DateDTO date = gson.fromJson(jsonElement, DateDTO.class); + if (date == null || date.year == null || date.month == null || date.day == null) { + throw new JsonParseException("Invalid date format: missing required fields"); + } + try { return LocalDate.of(date.year, date.month, date.day); + } catch (DateTimeException e) { + throw new JsonParseException("Invalid date values", e); + } }📝 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.@Override public LocalDate deserialize( final JsonElement jsonElement, final Type typeOfT, final JsonDeserializationContext context) throws JsonParseException { if (jsonElement == null || jsonElement.isJsonNull()) { return null; } DateDTO date = gson.fromJson(jsonElement, DateDTO.class); if (date == null || date.year == null || date.month == null || date.day == null) { throw new JsonParseException("Invalid date format: missing required fields"); } try { return LocalDate.of(date.year, date.month, date.day); } catch (DateTimeException e) { throw new JsonParseException("Invalid date values", e); } }
48-52: 🧹 Nitpick (assertive)
Consider enhancing the DTO implementation.
The current implementation could benefit from:
- Making fields final for immutability
- Adding a constructor with validation
- Adding toString() for debugging
static class DateDTO { - Integer year; - Integer month; - Integer day; + final Integer year; + final Integer month; + final Integer day; + + DateDTO() { + this.year = null; + this.month = null; + this.day = null; + } + + @Override + public String toString() { + return "DateDTO{" + + "year=" + year + + ", month=" + month + + ", day=" + day + + '}'; + } }Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/JaxbContextCache.java (1)
18-19: 💡 Codebase verification
Migration to jakarta.xml.bind requires attention in generated code
The verification reveals a mixed state of JAXB dependencies:
- All non-generated code correctly uses
jakarta.xml.bindimports (as seen in core and common-types modules)- Generated code in
model-seev-typesandmodel-auth-typesstill usesjavax.xml.bind- Statistics show 2 files with
javax.xml.bindvs 28,766 occurrences ofjakarta.xml.bindThis indicates that while the core codebase has been properly migrated, the code generation templates or tools need to be updated to use the Jakarta namespace instead of javax.
🔗 Analysis chain
Verify JAXB dependency consistency across the project
The migration from
javax.xml.bindtojakarta.xml.bindis correct for Jakarta EE 9+ compatibility. However, we should ensure all JAXB-related dependencies are consistently using the jakarta namespace to prevent runtime conflicts.
The initial scan shows that there are still some files using
javax.xml.bindimports in the generated code directories. Let's verify if these are isolated cases or if there's a broader pattern, and also check the JAXB dependencies more thoroughly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining javax.xml.bind imports and dependencies # that might cause conflicts with jakarta.xml.bind echo "Checking for any remaining javax.xml.bind imports..." rg "javax\.xml\.bind" -t java echo "Checking pom.xml files for JAXB dependencies..." find . -name "pom.xml" -type f -exec grep -l "xml.bind" {} \; | xargs catLength of output: 1564
Script:
#!/bin/bash # Check for all JAXB-related imports (both javax and jakarta) in non-generated code echo "=== Checking JAXB imports in non-generated code ===" rg "import (javax|jakarta)\.xml\.bind" --type java | grep -v "/generated/" # Check specifically for JAXB dependencies in pom files echo -e "\n=== Checking JAXB dependencies in pom.xml files ===" find . -name "pom.xml" -type f -exec sh -c 'echo "=== $1 ==="; grep -A 2 -B 2 "xml.bind\|jaxb" "$1"' sh {} \; # Count occurrences of javax vs jakarta in all Java files echo -e "\n=== Statistics of JAXB usage ===" echo "javax.xml.bind occurrences:" rg "javax\.xml\.bind" -t java --count-matches | wc -l echo "jakarta.xml.bind occurrences:" rg "jakarta\.xml\.bind" -t java --count-matches | wc -lLength of output: 11557
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/v9/V9DateTimeJsonAdapterTest.java (5)
49-49:
⚠️ Potential issueReplace deprecated JsonParser.parse() with JsonParser.parseString()
The
JsonParser.parse()method is deprecated. UseJsonParser.parseString()instead, or better yet, useGson.fromJson()directly.Apply this change:
- JsonElement sourceJsonElement = new JsonParser().parse(sourceJsonV9); + JsonElement sourceJsonElement = JsonParser.parseString(sourceJsonV9); - JsonElement expectedJsonElement = new JsonParser().parse(expectedJsonV9); + JsonElement expectedJsonElement = JsonParser.parseString(expectedJsonV9);Also applies to: 55-55
10-10: 🧹 Nitpick (assertive)
Consider adding tests for error cases and edge cases
The test coverage could be improved by adding tests for:
- Invalid JSON input
- Malformed datetime strings
- Edge cases like maximum/minimum dates
- Null input handling
- Different timezone offsets (positive, negative, large offsets)
Would you like me to provide example test cases for these scenarios?
14-29: 🧹 Nitpick (assertive)
Consider splitting test cases and adding @DisplayName
While the test cases are well structured, consider splitting them into separate methods for better clarity and test reporting.
Here's a suggested refactor:
- @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); - } + @Test + @DisplayName("Should deserialize datetime with timezone offset") + void testDeserializationWithOffset() { + String sourceJsonV9 = "{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}"; + OffsetDateTime expectedOffsetDateTime = OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00"); + testDeserialization(sourceJsonV9, expectedOffsetDateTime); + } + + @Test + @DisplayName("Should deserialize datetime without timezone offset") + void testDeserializationWithoutOffset() { + String sourceJsonV9 = "{\"year\":2019,\"month\":4,\"day\":28,\"timezone\":0,\"hour\":12,\"minute\":13,\"second\":14,\"fractionalSecond\":0}"; + OffsetDateTime expectedOffsetDateTime = OffsetDateTime.parse("2019-04-28T12:13:14.0-00:00"); + testDeserialization(sourceJsonV9, expectedOffsetDateTime); + }📝 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.@Test @DisplayName("Should deserialize datetime with timezone offset") void testDeserializationWithOffset() { String sourceJsonV9 = "{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}"; OffsetDateTime expectedOffsetDateTime = OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00"); testDeserialization(sourceJsonV9, expectedOffsetDateTime); } @Test @DisplayName("Should deserialize datetime without timezone offset") void testDeserializationWithoutOffset() { String sourceJsonV9 = "{\"year\":2019,\"month\":4,\"day\":28,\"timezone\":0,\"hour\":12,\"minute\":13,\"second\":14,\"fractionalSecond\":0}"; OffsetDateTime expectedOffsetDateTime = OffsetDateTime.parse("2019-04-28T12:13:14.0-00:00"); testDeserialization(sourceJsonV9, expectedOffsetDateTime); }
31-46: 🧹 Nitpick (assertive)
Split test cases and fix inconsistent parameter naming
Similar to the deserialization tests, consider splitting these into separate methods. Also, there's an inconsistent parameter name.
Here's a suggested refactor:
- @Test - void testSerialization() { - OffsetDateTime sourceOffsetDateTime; - String expectedJsonV9; - - sourceOffsetDateTime = OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00"); - expectedJsonV9 = - "{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}"; - testSerialization(sourceOffsetDateTime, expectedJsonV9); - - // without offset, nano 0 - sourceOffsetDateTime = OffsetDateTime.parse("2019-04-28T12:13:14.0-00:00"); - expectedJsonV9 = - "{\"year\":2019,\"month\":4,\"day\":28,\"timezone\":0,\"hour\":12,\"minute\":13,\"second\":14,\"fractionalSecond\":0}"; - testSerialization(sourceOffsetDateTime, expectedJsonV9); - } + @Test + @DisplayName("Should serialize datetime with timezone offset") + void testSerializationWithOffset() { + OffsetDateTime sourceDateTime = OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00"); + String expectedJsonV9 = "{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}"; + testSerialization(sourceDateTime, expectedJsonV9); + } + + @Test + @DisplayName("Should serialize datetime without timezone offset") + void testSerializationWithoutOffset() { + OffsetDateTime sourceDateTime = OffsetDateTime.parse("2019-04-28T12:13:14.0-00:00"); + String expectedJsonV9 = "{\"year\":2019,\"month\":4,\"day\":28,\"timezone\":0,\"hour\":12,\"minute\":13,\"second\":14,\"fractionalSecond\":0}"; + testSerialization(sourceDateTime, 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.@Test @DisplayName("Should serialize datetime with timezone offset") void testSerializationWithOffset() { OffsetDateTime sourceDateTime = OffsetDateTime.parse("2022-11-30T10:43:57.412-03:00"); String expectedJsonV9 = "{\"year\":2022,\"month\":11,\"day\":30,\"timezone\":-180,\"hour\":10,\"minute\":43,\"second\":57,\"fractionalSecond\":0.412}"; testSerialization(sourceDateTime, expectedJsonV9); } @Test @DisplayName("Should serialize datetime without timezone offset") void testSerializationWithoutOffset() { OffsetDateTime sourceDateTime = OffsetDateTime.parse("2019-04-28T12:13:14.0-00:00"); String expectedJsonV9 = "{\"year\":2019,\"month\":4,\"day\":28,\"timezone\":0,\"hour\":12,\"minute\":13,\"second\":14,\"fractionalSecond\":0}"; testSerialization(sourceDateTime, expectedJsonV9); }
48-58: 🧹 Nitpick (assertive)
Add parameter validation and fix inconsistent naming in helper methods
The helper methods could be improved with null parameter validation and consistent naming.
Here's a suggested refactor:
private void testDeserialization(String sourceJsonV9, OffsetDateTime expected) { + if (sourceJsonV9 == null || expected == null) { + throw new IllegalArgumentException("Parameters cannot be null"); + } JsonElement sourceJsonElement = JsonParser.parseString(sourceJsonV9); OffsetDateTime deserializeResult = adapter.deserialize(sourceJsonElement, null, null); assertEquals(expected, deserializeResult); } - private void testSerialization(OffsetDateTime sourceOffsetDateTimeV10, String expectedJsonV9) { + private void testSerialization(OffsetDateTime sourceDateTime, String expectedJsonV9) { + if (sourceDateTime == null || expectedJsonV9 == null) { + throw new IllegalArgumentException("Parameters cannot be null"); + } JsonElement expectedJsonElement = JsonParser.parseString(expectedJsonV9); - JsonElement serializeResult = adapter.serialize(sourceOffsetDateTimeV10, null, null); + JsonElement serializeResult = adapter.serialize(sourceDateTime, null, null); assertEquals(expectedJsonElement, serializeResult); }📝 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) { if (sourceJsonV9 == null || expected == null) { throw new IllegalArgumentException("Parameters cannot be null"); } JsonElement sourceJsonElement = JsonParser.parseString(sourceJsonV9); OffsetDateTime deserializeResult = adapter.deserialize(sourceJsonElement, null, null); assertEquals(expected, deserializeResult); } private void testSerialization(OffsetDateTime sourceDateTime, String expectedJsonV9) { if (sourceDateTime == null || expectedJsonV9 == null) { throw new IllegalArgumentException("Parameters cannot be null"); } JsonElement expectedJsonElement = JsonParser.parseString(expectedJsonV9); JsonElement serializeResult = adapter.serialize(sourceDateTime, null, null); assertEquals(expectedJsonElement, serializeResult); }iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/YearMonthJsonAdapterTest.java (4)
46-46: 🧹 Nitpick (assertive)
Improve test method name for clarity
The current name
testYear_CustomPatterncould be more descriptive. Consider renaming to better reflect what's being tested, e.g.,testYearSerializationInNestedStructure.
47-65: 🧹 Nitpick (assertive)
Extract test JSON to a resource file
The large JSON string literal makes the test harder to read and maintain. Consider moving it to a separate test resource file.
Create a new file
src/test/resources/samples/acmt.034.001.04.jsonand load it in the test:String jsonOrigin = Files.readString(Path.of("src/test/resources/samples/acmt.034.001.04.json"));
39-39:
⚠️ Potential issueUpdate deprecated JsonParser.parse() method
The
JsonParser.parse()method is deprecated. UseJsonParser.parseString()instead for better compatibility with newer versions.Apply this change:
- JsonElement jsonElement = new JsonParser().parse(json); + JsonElement jsonElement = JsonParser.parseString(json);📝 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.JsonElement jsonElement = JsonParser.parseString(json);
67-69: 🛠️ Refactor suggestion
Strengthen the assertion for better test reliability
The current assertion only checks for a substring, which could pass even if the JSON structure is incorrect. Consider:
- Parsing the output JSON and asserting on the specific field
- Using JSONAssert or similar library for structured JSON comparison
Example improvement:
- assertTrue(jsonParsed.contains("\"year\": 2022")); + JsonElement parsed = JsonParser.parseString(jsonParsed); + int year = parsed.getAsJsonObject() + .getAsJsonObject("acctSwtchReqPmt") + .getAsJsonObject("cdtInstr") + .getAsJsonObject("tax") + .getAsJsonArray("rcrd") + .get(0) + .getAsJsonObject() + .getAsJsonObject("prd") + .getAsJsonObject("yr") + .get("year") + .getAsInt(); + assertEquals(2022, year);Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/IssueJira1566Test.java (5)
46-51: 🧹 Nitpick (assertive)
Consider splitting into separate test methods
The Zulu time test case contains multiple assertions testing different aspects. Consider splitting into separate test methods for better clarity and failure isolation.
Example refactor:
@Test public void testZuluTimeConversionToOffset() { MxPacs00800108 mx = new MxPacs00800108(); OffsetDateTime offsetDateTime = OffsetDateTime.parse("2021-09-19T12:13:14.000Z"); mx.getFIToFICstmrCdtTrf().getGrpHdr().setCreDtTm(offsetDateTime); String message = mx.message(); assertTrue(message.contains("2021-09-19T12:13:14+00:00")); } @Test public void testZuluTimeFormatNotPresent() { MxPacs00800108 mx = new MxPacs00800108(); OffsetDateTime offsetDateTime = OffsetDateTime.parse("2021-09-19T12:13:14.000Z"); mx.getFIToFICstmrCdtTrf().getGrpHdr().setCreDtTm(offsetDateTime); String message = mx.message(); assertFalse(message.contains("2021-09-19T12:13:14.000Z")); assertFalse(message.contains("2021-09-19T12:13:14.Z")); }
53-68: 🧹 Nitpick (assertive)
Enhance test organization and documentation
The zero nanoseconds test cases could be improved in several ways:
- Use consistent date-time pattern across test cases
- Add more descriptive comments explaining the purpose of each test case
- Consider separating into individual test methods
Example refactor:
@Test public void testZeroNanosecondsWithPattern() { MxPacs00800108 mx = new MxPacs00800108(); // Test explicit zero nanoseconds using pattern OffsetDateTime offsetDateTime = OffsetDateTime.parse( "2021-09-19T12:13:14.00000+00:00", DateTimeFormatter.ISO_OFFSET_DATE_TIME); mx.getFIToFICstmrCdtTrf().getGrpHdr().setCreDtTm(offsetDateTime); assertTrue(mx.message().contains("2021-09-19T12:13:14+00:00")); } @Test public void testZeroNanosecondsWithZulu() { MxPacs00800108 mx = new MxPacs00800108(); // Test zero nanoseconds with Zulu time OffsetDateTime offsetDateTime = OffsetDateTime.parse("2021-09-19T12:13:14.000000000Z"); mx.getFIToFICstmrCdtTrf().getGrpHdr().setCreDtTm(offsetDateTime); assertFalse(mx.message().contains("2021-09-19T12:13:14.Z")); } @Test public void testZeroNanosecondsWithOffset() { MxPacs00800108 mx = new MxPacs00800108(); // Test zero nanoseconds with specific offset OffsetDateTime offsetDateTime = OffsetDateTime.parse("2021-09-19T12:13:14.000000000-03:00"); mx.getFIToFICstmrCdtTrf().getGrpHdr().setCreDtTm(offsetDateTime); assertTrue(mx.message().contains("2021-09-19T12:13:14-03:00")); }
1-69: 🧹 Nitpick (assertive)
Add documentation about Jira issue 1566
Please add a class-level Javadoc comment explaining the purpose of this test class and referencing the specific issue it addresses (Jira 1566). This will help maintain context for future maintenance.
Example:
/** * Test cases for Jira issue 1566: [Brief description of the issue] * * Validates the handling of date-time values with nanosecond precision * in MxPacs00800108 messages, ensuring proper serialization and * deserialization of various time formats. */
14-37: 🧹 Nitpick (assertive)
Fix typo in test data
The message ID "TETS" appears to be a typo.
- "msgId": "TETS", + "msgId": "TEST",📝 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.String json = "{\n" + " \"fiToFICstmrCdtTrf\": {\n" + " \"grpHdr\": {\n" + " \"msgId\": \"TEST\",\n" + " \"creDtTm\": {\n" + " \"dateTime\": {\n" + " \"date\": {\n" + " \"year\": 2023,\n" + " \"month\": 8,\n" + " \"day\": 10\n" + " },\n" + " \"time\": {\n" + " \"hour\": 23,\n" + " \"minute\": 35,\n" + " \"second\": 53,\n" + " \"nano\": 2312\n" + " }\n" + " },\n" + " \"offset\": {\n" + " \"totalSeconds\": -10800\n" + " }\n" + " }\n" + " }\n" + " }\n" + "}";
39-44: 🧹 Nitpick (assertive)
Remove unnecessary method call and improve test clarity
- The
message()call on line 41 is unused and can be removed- Consider adding a descriptive comment explaining what aspect of the date-time format is being verified
- mx.message(); assertEquals( + // Verify correct deserialization of date-time with nanosecond precision "2023-08-10T23:35:53.000002312-03:00", mx.getFIToFICstmrCdtTrf().getGrpHdr().getCreDtTm().toString());📝 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.MxPacs00800108 mx = MxPacs00800108.fromJson(json); assertEquals( // Verify correct deserialization of date-time with nanosecond precision "2023-08-10T23:35:53.000002312-03:00", mx.getFIToFICstmrCdtTrf().getGrpHdr().getCreDtTm().toString());iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/ZuluOffsetDateTimeAdapterUnmarshallingTest.java (3)
28-90: 🧹 Nitpick (assertive)
Enhance test coverage with additional scenarios.
While the current tests cover basic scenarios well, consider adding the following test cases for more robust validation:
- Edge cases:
@Test public void testUnmarshallNull() { assertThrows(IllegalArgumentException.class, () -> adapter.unmarshal(null)); } @Test public void testUnmarshallInvalidFormat() { assertThrows(DateTimeParseException.class, () -> adapter.unmarshal("invalid-date")); }
- Maximum precision handling:
@Test public void testUnmarshallMaxPrecision() { OffsetDateTime datetime = adapter.unmarshal("2022-03-04T12:50:08.123456789Z"); assertEquals(123456789, datetime.getNano()); }
- Positive UTC offset:
@Test public void testUnmarshallPositiveOffset() { OffsetDateTime datetime = adapter.unmarshal("2022-03-04T12:50:08+05:30"); assertEquals(19800, datetime.getOffset().getTotalSeconds()); // +5:30 hours }
28-90: 🧹 Nitpick (assertive)
Consider extracting test data to constants.
The test methods use the same date-time components repeatedly. Consider extracting these to constants for better maintainability:
public class ZuluOffsetDateTimeAdapterUnmarshallingTest { + private static final int EXPECTED_YEAR = 2022; + private static final Month EXPECTED_MONTH = Month.MARCH; + private static final int EXPECTED_DAY = 4; + private static final int EXPECTED_HOUR = 12; + private static final int EXPECTED_MINUTE = 50; + private static final int EXPECTED_SECOND = 8; + private static final String BASE_DATE_TIME = "2022-03-04T12:50:08"; private final ZuluOffsetDateTimeAdapter adapter = new ZuluOffsetDateTimeAdapter();Committable suggestion skipped: line range outside the PR's diff.
24-27: 🧹 Nitpick (assertive)
Add class-level documentation.
Consider adding Javadoc to describe the purpose of this test class and its role in validating the date-time format handling, especially given the transition from Java 8 to 11 and JAXB changes.
+/** + * Unit tests for {@link ZuluOffsetDateTimeAdapter} focusing on unmarshalling various + * date-time string formats into {@link OffsetDateTime} objects. + * <p> + * These tests ensure correct handling of: + * <ul> + * <li>Fractional seconds</li> + * <li>UTC and custom timezone offsets</li> + * <li>Date-time strings without offsets</li> + * </ul> + */ public class ZuluOffsetDateTimeAdapterUnmarshallingTest {📝 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./** * Unit tests for {@link ZuluOffsetDateTimeAdapter} focusing on unmarshalling various * date-time string formats into {@link OffsetDateTime} objects. * <p> * These tests ensure correct handling of: * <ul> * <li>Fractional seconds</li> * <li>UTC and custom timezone offsets</li> * <li>Date-time strings without offsets</li> * </ul> */ public class ZuluOffsetDateTimeAdapterUnmarshallingTest { private final ZuluOffsetDateTimeAdapter adapter = new ZuluOffsetDateTimeAdapter();iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/OffsetTimeJsonAdapterTest.java (2)
78-83:
⚠️ Potential issueUpdate deprecated JsonParser usage and improve JSON comparison
The implementation has several issues that should be addressed:
- Uses deprecated JsonParser constructor
- Missing null checks for adapter results
- Uses direct string comparison for JSON which might be fragile
Here's the suggested fix:
private void testSerializationAndDeserializationImpl(String jsonInput, String jsonResult) { - JsonElement jsonElement = new JsonParser().parse(jsonInput); + JsonElement jsonElement = JsonParser.parseString(jsonInput); OffsetTime offsetTime = adapter.deserialize(jsonElement, null, null); + // Add null check + if (offsetTime == null) { + throw new AssertionError("Failed to deserialize: " + jsonInput); + } JsonElement valueDateResult = adapter.serialize(offsetTime, null, null); - assertEquals(jsonResult, valueDateResult.toString()); + // Compare JSON structures instead of strings + assertEquals( + JsonParser.parseString(jsonResult), + valueDateResult, + "JSON serialization mismatch" + ); }📝 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 testSerializationAndDeserializationImpl(String jsonInput, String jsonResult) { JsonElement jsonElement = JsonParser.parseString(jsonInput); OffsetTime offsetTime = adapter.deserialize(jsonElement, null, null); // Add null check if (offsetTime == null) { throw new AssertionError("Failed to deserialize: " + jsonInput); } JsonElement valueDateResult = adapter.serialize(offsetTime, null, null); // Compare JSON structures instead of strings assertEquals( JsonParser.parseString(jsonResult), valueDateResult, "JSON serialization mismatch" ); }
31-76: 🧹 Nitpick (assertive)
Consider using @ParameterizedTest for better test structure
While the test coverage is comprehensive, the test method could benefit from using JUnit's parameterized tests to make it more maintainable and readable.
Here's how you could refactor it:
- @Test - void testSerializationAndDeserialization() { - int systemOffsetSeconds = ZoneId.systemDefault() - .getRules() - .getStandardOffset(Instant.now()) - .getTotalSeconds(); - - // without offset, nano 0 - String jsonActual = "{\"time\":{\"hour\":12,\"minute\":13,\"second\":14,\"nano\":0}}"; - String jsonExpected = - "{\"time\":{\"hour\":12,\"minute\":13,\"second\":14,\"nano\":0},\"offset\":{\"totalSeconds\":" - + systemOffsetSeconds + "}}"; - testSerializationAndDeserializationImpl(jsonActual, jsonExpected); + private static Stream<Arguments> timeTestCases() { + int systemOffsetSeconds = ZoneId.systemDefault() + .getRules() + .getStandardOffset(Instant.now()) + .getTotalSeconds(); + + return Stream.of( + // without offset, nano 0 + Arguments.of( + "{\"time\":{\"hour\":12,\"minute\":13,\"second\":14,\"nano\":0}}", + "{\"time\":{\"hour\":12,\"minute\":13,\"second\":14,\"nano\":0},\"offset\":{\"totalSeconds\":" + systemOffsetSeconds + "}}" + ), + // Add other test cases here... + ); + } + + @ParameterizedTest + @MethodSource("timeTestCases") + void testSerializationAndDeserialization(String input, String expected) { + testSerializationAndDeserializationImpl(input, expected); + }Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/adapters/TypeAdaptersConfiguration.java (1)
112-120: 🧹 Nitpick (assertive)
Consider documenting adapter order significance
The implementation correctly adds new adapters to the list, but consider documenting whether the order of adapters in the returned list is significant for the marshalling/unmarshalling process.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/OffsetDateTimeJsonAdapterTest.java (2)
85-91:
⚠️ Potential issueUpdate deprecated JsonParser usage.
The
JsonParser.parse()method is deprecated. Replace it withJsonParser.parseString()for better forward compatibility.Here's the fix:
private void testSerializationAndDeserializationImpl(String jsonInput, String jsonResult) { - JsonElement jsonElement = new JsonParser().parse(jsonInput); + JsonElement jsonElement = JsonParser.parseString(jsonInput); OffsetDateTime offsetDateTime = adapter.deserialize(jsonElement, null, null); JsonElement valueDateResult = adapter.serialize(offsetDateTime, null, null); assertEquals(jsonResult, valueDateResult.toString()); }📝 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 testSerializationAndDeserializationImpl(String jsonInput, String jsonResult) { JsonElement jsonElement = JsonParser.parseString(jsonInput); OffsetDateTime offsetDateTime = adapter.deserialize(jsonElement, null, null); JsonElement valueDateResult = adapter.serialize(offsetDateTime, null, null); assertEquals(jsonResult, valueDateResult.toString()); } }
30-83: 🧹 Nitpick (assertive)
Consider refactoring test cases for better maintainability and reliability.
While the test coverage is comprehensive, consider these improvements:
- Use
@ParameterizedTestto organize test cases more efficiently- Extract test data as constants for better maintainability
- Add edge cases for date-time boundaries
- Remove system timezone dependency to ensure consistent test behavior across different environments
Here's an example refactor for the first test case:
+ private static final String EXPECTED_FORMAT = + "{\"dateTime\":{\"date\":{\"year\":%d,\"month\":%d,\"day\":%d}," + + "\"time\":{\"hour\":%d,\"minute\":%d,\"second\":%d,\"nano\":%d}," + + "\"offset\":{\"totalSeconds\":%d}}}"; + @ParameterizedTest + @CsvSource({ + "2019,4,28,12,13,14,0", + "2019,4,28,12,13,14,123456789" + }) + void testSerializationWithDifferentPrecisions(int year, int month, int day, + int hour, int minute, int second, int nano) { + String jsonActual = String.format(EXPECTED_FORMAT, + year, month, day, hour, minute, second, nano, 0); + String jsonExpected = String.format(EXPECTED_FORMAT, + year, month, day, hour, minute, second, nano, + ZoneOffset.systemDefault().getTotalSeconds()); + testSerializationAndDeserializationImpl(jsonActual, jsonExpected); + }Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/ZuluOffsetDateTimeAdapterMarshallingTest.java (4)
50-50: 🧹 Nitpick (assertive)
Follow Java naming conventions for test methods
The test method names use underscores instead of camelCase, which is not consistent with Java naming conventions.
- public void test_marshall_message_with_BAH1_and_CreDt_Zulu() + public void testMarshallMessageWithBAH1AndCreDtZulu() - public void test_marshall_message_with_BAH1_and_CreDt_OffsetDateTime() + public void testMarshallMessageWithBAH1AndCreDtOffsetDateTime()Also applies to: 85-85
81-82: 🧹 Nitpick (assertive)
Enhance test comments with specific scenarios
The current comments could be more descriptive about the specific scenarios being tested.
- // After parse and marshalling, CreDtTm should be preserved as Zulu date time + // Verify that when input is already in Zulu format (Z), it remains unchanged after marshalling - // After parse and marshalling, the original CreDtTm should be converted to 00:00 offset Zulu date time + // Verify that when input has -03:00 offset, it's correctly converted to equivalent Zulu time (+3 hours)Also applies to: 116-117
26-26: 🧹 Nitpick (assertive)
Consider adding tests for edge cases
While the current test coverage is good, consider adding tests for these scenarios:
- Leap year dates
- Daylight Saving Time transitions
- Extreme time zones (e.g., +14:00, -12:00)
- Invalid input handling
Would you like me to help generate these additional test cases?
28-28: 🧹 Nitpick (assertive)
Consider making the adapter field final
Since the adapter instance is not modified after initialization, consider making it final for better immutability.
- private ZuluOffsetDateTimeAdapter adapter = new ZuluOffsetDateTimeAdapter(); + private final ZuluOffsetDateTimeAdapter adapter = new ZuluOffsetDateTimeAdapter();📝 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 final ZuluOffsetDateTimeAdapter adapter = new ZuluOffsetDateTimeAdapter();iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/OffsetDateTimeAdapterTest.java (4)
25-49: 🧹 Nitpick (assertive)
Consider using parameterized tests for better maintainability.
The multiple test cases could be more maintainable using JUnit's
@ParameterizedTest. This would make it easier to add new test cases and improve readability.Example refactor:
@ParameterizedTest @CsvSource({ "2021-09-19T12:13:14, 2021-09-19T12:13:14+01:00", "2021-09-19T12:13:14.1, 2021-09-19T12:13:14.1+01:00", // ... other test cases }) void testOffsetDateTime(String input, String expected) throws Exception { testDateTimeImpl(input, expected); }
70-71: 🧹 Nitpick (assertive)
Consider adding a proper test for the nano precision limit.
Instead of commenting out the test case, consider adding a test that verifies the parser throws an appropriate exception when given more than 9 digits for nanoseconds.
Example:
@Test void testNanoPrecisionLimit() { assertThrows(JAXBException.class, () -> testDateTimeImpl("2018-01-15T17:30:33.0000000000+02:00", "")); }
51-56: 🧹 Nitpick (assertive)
Consider optimizing the helper method.
A few suggestions to improve the helper method:
- The
OffsetDateTimeAdaptercould be a class field to avoid creating new instances.- The generic
Exceptioncould be replaced with more specific exceptions.Example refactor:
+ private final OffsetDateTimeAdapter offsetDateTimeAdapter = new OffsetDateTimeAdapter(); + - private void testDateTimeImpl(String value, String valueResult) throws Exception { - OffsetDateTimeAdapter offsetDateTimeAdapter = new OffsetDateTimeAdapter(); + private void testDateTimeImpl(String value, String valueResult) throws JAXBException { OffsetDateTime offsetDateTime = offsetDateTimeAdapter.unmarshal(value); String valueDateResult = offsetDateTimeAdapter.marshal(offsetDateTime); assertEquals(valueResult, valueDateResult); }📝 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 final OffsetDateTimeAdapter offsetDateTimeAdapter = new OffsetDateTimeAdapter(); private void testDateTimeImpl(String value, String valueResult) throws JAXBException { OffsetDateTime offsetDateTime = offsetDateTimeAdapter.unmarshal(value); String valueDateResult = offsetDateTimeAdapter.marshal(offsetDateTime); assertEquals(valueResult, valueDateResult); }
15-21: 🛠️ Refactor suggestion
Consider using a fixed timezone instead of system default.
Using
ZoneOffset.systemDefault()makes the test dependent on the system's timezone settings. This could lead to non-deterministic behavior when running tests in different environments.Consider using a fixed timezone:
- ZoneId zoneId = ZoneOffset.systemDefault(); + ZoneId zoneId = ZoneOffset.of("+01:00"); // Use a fixed offset for deterministic testing📝 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.ZoneId zoneId = ZoneOffset.of("+01:00"); // Use a fixed offset for deterministic testing OffsetDateTime dateTime = LocalDateTime.parse("2021-09-19T12:13:14") .atZone(zoneId) .withEarlierOffsetAtOverlap() .withFixedOffsetZone() .withEarlierOffsetAtOverlap() .toOffsetDateTime();iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/OffsetTimeAdapterTest.java (5)
28-56: 🧹 Nitpick (assertive)
Consider adding edge case tests for unmarshalling
While the current tests cover the basic scenarios well, consider adding tests for:
- Invalid time formats
- Out-of-range values (e.g., hours > 23)
- Extreme timezone offsets
@Test public void testUnmarshallInvalidFormat() { assertThrows(Exception.class, () -> adapter.unmarshal("25:00:00+01:00")); assertThrows(Exception.class, () -> adapter.unmarshal("12:61:00+01:00")); assertThrows(Exception.class, () -> adapter.unmarshal("12:00:61+01:00")); assertThrows(Exception.class, () -> adapter.unmarshal("12:00:00+99:00")); }
26-26: 🧹 Nitpick (assertive)
Consider making the adapter field final
Since the adapter instance is never reassigned, it should be marked as final for better immutability.
- private OffsetTimeAdapter adapter = new OffsetTimeAdapter(); + private final OffsetTimeAdapter adapter = new OffsetTimeAdapter();📝 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 final OffsetTimeAdapter adapter = new OffsetTimeAdapter();
108-113: 🧹 Nitpick (assertive)
Fix inconsistent variable naming
The variable
OffsetTimeshould follow Java naming conventions (camelCase for variables).- OffsetTime OffsetTime = offsetTimeAdapter.unmarshal(value); + OffsetTime offsetTime = offsetTimeAdapter.unmarshal(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.private void testTimeImpl(String value, String valueResult) throws Exception { OffsetTimeAdapter offsetTimeAdapter = new OffsetTimeAdapter(); OffsetTime offsetTime = offsetTimeAdapter.unmarshal(value); String valueDateResult = offsetTimeAdapter.marshal(offsetTime); assertEquals(valueResult, valueDateResult); }
127-129: 🧹 Nitpick (assertive)
Document nano precision limitation more explicitly
Consider adding a more detailed comment explaining the 9-character limitation for nanoseconds and its origin (Java's OffsetTime implementation).
- // testTimeImpl("17:30:33.0000000000+02:00", "17:30:33+02:00"); //Cannot parse more - // than 9 characters in nano value + // Note: Java's OffsetTime implementation limits nanosecond precision to 9 digits (0-999999999). + // Attempting to parse more than 9 digits will result in a DateTimeParseException. + // testTimeImpl("17:30:33.0000000000+02:00", "17:30:33+02:00");📝 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.// Note: Java's OffsetTime implementation limits nanosecond precision to 9 digits (0-999999999). // Attempting to parse more than 9 digits will result in a DateTimeParseException. // testTimeImpl("17:30:33.0000000000+02:00", "17:30:33+02:00");
58-76: 🧹 Nitpick (assertive)
Improve test readability with descriptive comments
The test methods would benefit from more detailed comments explaining the expected behavior, especially for the system default timezone case.
@Test public void testMarshallNoOffset() throws Exception { + // When no offset is provided, the system default offset should be used + // This test verifies that the marshalled output includes the system offset DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss[.SSS][XXX]"); ZoneOffset offset = ZoneId.systemDefault().getRules().getOffset(Instant.now());📝 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.@Test public void testMarshallNoFractionOfSecondsAndWithFractionOfSeconds() throws Exception { ZoneOffset zoneOffSet = ZoneOffset.of("-03:00"); DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss[.SSS][XXX]"); OffsetTime offsetTime = LocalTime.parse("12:50:08", dateTimeFormatter).atOffset(zoneOffSet); assertEquals("12:50:08-03:00", adapter.marshal(offsetTime)); offsetTime = LocalTime.parse("12:50:08.123", dateTimeFormatter).atOffset(zoneOffSet); assertEquals("12:50:08.123-03:00", adapter.marshal(offsetTime)); } @Test public void testMarshallNoOffset() throws Exception { // When no offset is provided, the system default offset should be used // This test verifies that the marshalled output includes the system offset DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss[.SSS][XXX]"); ZoneOffset offset = ZoneId.systemDefault().getRules().getOffset(Instant.now()); OffsetTime offsetTime = LocalTime.parse("12:50:08", dateTimeFormatter).atOffset(offset); assertEquals("12:50:08" + offset, adapter.marshal(offsetTime)); }CHANGELOG.md (2)
3-5: 🧹 Nitpick (assertive)
Consider enhancing migration documentation for JSON serialization changes
While the changelog mentions the new
toJsonV9andfromJsonV9methods for backward compatibility, it would be helpful to provide more detailed migration guidance for users upgrading from Java 8.Consider adding:
- Example usage of the new methods
- Common migration scenarios
- Any gotchas or edge cases to watch out for
Would you like me to help draft detailed migration documentation?
🧰 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)
54-58: 🧹 Nitpick (assertive)
Consider restructuring migration-related changes for better visibility
The migration changes (Java 11, Jakarta EE 10, DateTime handling) are significant breaking changes. Consider restructuring this section to:
- Group all migration-related changes under a dedicated "Migration Guide" section
- Highlight potential breaking changes more prominently
- Provide step-by-step migration instructions
Would you like me to propose a restructured format for the migration-related changes?
🧰 Tools
🪛 Markdownlint
54-54: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
55-55: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
56-56: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
57-57: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
58-58: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
54-54: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/LocalDateAdapterTest.java (4)
15-20: 🧹 Nitpick (assertive)
Consider adding edge cases to basic date conversion test.
While the basic conversion test is good, consider adding test cases for:
- Null input handling
- Invalid date format handling
- Boundary dates (e.g., leap years, year transitions)
43-155: 🛠️ Refactor suggestion
Refactor test to improve maintainability and security.
The test method is too long and tests multiple aspects. Consider splitting it into smaller, focused test methods:
- One for basic XML parsing
- One for each custom format pattern
The test data contains potentially sensitive information (BIC codes). Consider using dummy/mock data instead.
The XML test data should be moved to a separate test resource file.
Example refactor:
@Test public void testLocalDate_BasicXmlParsing() { String xml = loadTestResource("basic_payment.xml"); final MxPacs00900110 mx = MxPacs00900110.parse(xml); assertTrue(mx.message().contains("IntrBkSttlmDt>2023-01-24</")); } @Test public void testLocalDate_CustomSlashFormat() { MxPacs00900110 mx = createTestMessage(); MxWriteConfiguration conf = new MxWriteConfiguration(); conf.adapters.dateAdapter = new IsoDateAdapter( new LocalDateAdapter((DateTimeFormatter.ofPattern("yyyy/MM/dd"))) ); assertTrue(mx.message(conf).contains("IntrBkSttlmDt>2023/01/24</")); }
157-174: 🧹 Nitpick (assertive)
LGTM! Consider adding more offset scenarios.
The test effectively verifies offset handling, but could be enhanced by testing:
- Different offset values (positive and negative)
- UTC offset
- Daylight saving time transitions
37-41:
⚠️ Potential issueFix invalid test data in
testLocalDateDropTime.The test contains an invalid seconds value "65" in the timestamp "2021-09-19T23:34:65.233Z". Seconds should be in the range 0-59.
- LocalDate localDate = localDateAdapter.unmarshal("2021-09-19T23:34:65.233Z"); + LocalDate localDate = localDateAdapter.unmarshal("2021-09-19T23:34:45.233Z");📝 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.void testLocalDateDropTime() throws Exception { LocalDateAdapter localDateAdapter = new LocalDateAdapter(); LocalDate localDate = localDateAdapter.unmarshal("2021-09-19T23:34:45.233Z"); assertEquals("2021-09-19", localDateAdapter.marshal(localDate)); }iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/YearMonthAdapterTest.java (4)
29-93: 🧹 Nitpick (assertive)
Consider improving test data and readability
- Consider using clearly fake BIC codes (e.g., 'TESTBICXXXXX') instead of what appears to be real or sensitive BIC codes.
- Consider moving the test XML to a separate resource file for better maintainability.
- The test could benefit from additional edge cases:
- Invalid date formats
- Boundary dates (e.g., 9999-12)
- Null/empty date values
95-160: 🛠️ Refactor suggestion
Reduce duplication and enhance test coverage
- Consider extracting common XML test data to a shared method or resource file to reduce duplication.
- The adapter configuration could be moved to a
@BeforeEachsetup method:private MxWriteConfiguration conf; @BeforeEach void setup() { conf = new MxWriteConfiguration(); conf.documentPrefix = null; conf.headerPrefix = null; }
- Consider adding tests for:
- Invalid date format patterns
- Pattern with unsupported format specifiers
162-192: 🧹 Nitpick (assertive)
Enhance year format testing
Consider adding test cases for:
- Year boundary conditions:
- Minimum valid year (e.g., 1900)
- Maximum valid year (e.g., 9999)
- Invalid scenarios:
- Years before allowed range
- Non-numeric years
Example test case:
@Test void testYear_BoundaryConditions() { // Test minimum year assertDoesNotThrow(() -> { conf.adapters.yearAdapter = new IsoYearAdapter( new YearAdapter(DateTimeFormatter.ofPattern("yyyy"))); // Test parsing and formatting }); // Test maximum year // Test invalid years }
27-192: 🧹 Nitpick (assertive)
Consider architectural improvements for better test structure
The test class would benefit from:
- A more structured approach using JUnit 5 features:
@Nestedclasses to group related tests@ParameterizedTestfor testing multiple date formats- Test lifecycle methods (
@BeforeEach) for common setup- Consider creating a test utilities class for common XML operations
- Add explicit test cases for error handling scenarios
This would improve maintainability and make the test coverage more comprehensive.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/BusinessAppHdrV01.java (2)
226-226: 🧹 Nitpick (assertive)
Consider using try-with-resources for adapter management
While the adapter switching logic works, it could be improved using a try-with-resources pattern to ensure the original adapter is always restored, even in case of exceptions.
- IsoDateTimeAdapter currentAdapter = params.adapters.dateTimeAdapter; - params.adapters.dateTimeAdapter = new IsoDateTimeAdapter(new ZuluOffsetDateTimeAdapter()); + try (AdapterContext ac = new AdapterContext(params.adapters)) { + params.adapters.dateTimeAdapter = new IsoDateTimeAdapter(new ZuluOffsetDateTimeAdapter());You'll need to add this helper class:
private static class AdapterContext implements AutoCloseable { private final IsoDateTimeAdapter original; private final MxAdapters adapters; AdapterContext(MxAdapters adapters) { this.adapters = adapters; this.original = adapters.dateTimeAdapter; } @Override public void close() { adapters.dateTimeAdapter = original; } }
174-187: 🧹 Nitpick (assertive)
Consider adding null validation in creationDate()
While the changes to use
OffsetDateTimeare good, consider adding null validation in thecreationDate()method to prevent potential NPEs.@Override public OffsetDateTime creationDate() { - return this.getCreDt(); + return Objects.requireNonNull(this.getCreDt(), "Creation date must not be null"); }📝 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.public OffsetDateTime creationDate() { return Objects.requireNonNull(this.getCreDt(), "Creation date must not be null"); } /** * Sets the creation date. * * @param overwrite if true, the creation date will always be set overwriting any previous value; * @see #setCreDt(OffsetDateTime) */ @Override public void setCreationDate(boolean overwrite) { if (this.getCreDt() == null || overwrite) { this.setCreDt(OffsetDateTime.now(ZoneOffset.UTC));iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/AppHdrFactory.java (3)
115-115:
⚠️ Potential issueMaintain consistency in timezone handling across header versions.
V02 and V03 headers use
OffsetDateTime.now()without an explicit timezone, while V01 uses UTC. This inconsistency could lead to timestamps in different timezones across header versions.Apply this change to maintain consistency:
- h.setCreDt(OffsetDateTime.now()); + h.setCreDt(OffsetDateTime.now(ZoneOffset.UTC));Also applies to: 162-162
69-69: 🧹 Nitpick (assertive)
Consider implementing a centralized timezone strategy.
The current implementation shows inconsistent timezone handling across different header versions. Consider:
- Creating a centralized utility method for timestamp creation
- Documenting the timezone requirements for each header version
- Adding timezone validation in the header classes
This will ensure consistent and maintainable timezone handling across the codebase.
Example implementation:
private static OffsetDateTime createTimestamp() { return OffsetDateTime.now(ZoneOffset.UTC); }Also applies to: 115-115, 162-162, 209-209, 250-250
209-209:
⚠️ Potential issueMaintain consistency in timezone handling for V04 headers.
Similar to V02 and V03, V04 uses
OffsetDateTime.now()without an explicit timezone, which should be consistent with V01's UTC usage.Apply this change:
- h.setCreDt(OffsetDateTime.now()); + h.setCreDt(OffsetDateTime.now(ZoneOffset.UTC));📝 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.h.setCreDt(OffsetDateTime.now(ZoneOffset.UTC));build.gradle (2)
487-496: 🧹 Nitpick (assertive)
LGTM: Multi-version testing configuration
Good practice to test against multiple Java versions. Consider adding a comment explaining the testing strategy and requirements for each Java version.
+// Test against multiple Java versions to ensure forward compatibility +// Java 17 (LTS): Primary target for future migration +// Java 19: Latest version testing for early detection of issues task('testOn17', type: Test) {📝 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.// Test against multiple Java versions to ensure forward compatibility // Java 17 (LTS): Primary target for future migration // Java 19: Latest version testing for early detection of issues task('testOn17', type: Test) { javaLauncher = javaToolchains.launcherFor { languageVersion = JavaLanguageVersion.of(17) } } task('testOn19', type: Test) { javaLauncher = javaToolchains.launcherFor { languageVersion = JavaLanguageVersion.of(19) }
210-214: 🧹 Nitpick (assertive)
LGTM: Improved jar filtering with logging
Good addition of jar filtering and logging. Consider using a constant for the '.jar' extension for maintainability.
- if (it.name.endsWith('.jar')) { + static final String JAR_EXTENSION = '.jar' + if (it.name.endsWith(JAR_EXTENSION)) {Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/MxWriteAdaptersTest.java (5)
32-267: 🧹 Nitpick (assertive)
Consider adding test cases for error scenarios and timezone edge cases
The test suite would benefit from additional test cases:
- Error scenarios:
- Invalid date/time formats
- Null values
- Invalid timezone offsets
- Timezone edge cases:
- DST transitions
- UTC±14 boundaries
- Leap seconds
61-61:
⚠️ Potential issueFix incorrect ChronoField comparison
The test is using
DAY_OF_WEEKinstead ofDAY_OF_YEARfor date comparison, which could lead to false positives.- assertEquals(intrBkSttlmDt1.get(ChronoField.DAY_OF_WEEK), intrBkSttlmDt2.get(ChronoField.DAY_OF_WEEK)); + assertEquals(intrBkSttlmDt1.get(ChronoField.DAY_OF_YEAR), intrBkSttlmDt2.get(ChronoField.DAY_OF_YEAR));📝 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.assertEquals(intrBkSttlmDt1.get(ChronoField.DAY_OF_YEAR), intrBkSttlmDt2.get(ChronoField.DAY_OF_YEAR));
171-180: 🛠️ Refactor suggestion
Remove redundant time retrieval and comparison
The code retrieves the same time value twice (
clsTm1andclsTm2) from the same object, making the comparison redundant.- OffsetTime clsTm1 = mx2.getFIToFICstmrCdtTrf() - .getCdtTrfTxInf() - .get(0) - .getSttlmTmReq() - .getCLSTm(); - OffsetTime clsTm2 = mx2.getFIToFICstmrCdtTrf() - .getCdtTrfTxInf() - .get(0) - .getSttlmTmReq() - .getCLSTm(); + OffsetTime clsTm = mx2.getFIToFICstmrCdtTrf() + .getCdtTrfTxInf() + .get(0) + .getSttlmTmReq() + .getCLSTm(); - assertNotNull(clsTm2); - assertEquals(clsTm1.get(ChronoField.HOUR_OF_DAY), clsTm2.get(ChronoField.HOUR_OF_DAY)); - assertEquals(clsTm1.get(ChronoField.MINUTE_OF_HOUR), clsTm2.get(ChronoField.MINUTE_OF_HOUR)); - assertEquals(clsTm1.get(ChronoField.SECOND_OF_MINUTE), clsTm2.get(ChronoField.SECOND_OF_MINUTE)); - assertEquals(clsTm1.get(ChronoField.NANO_OF_SECOND), clsTm2.get(ChronoField.NANO_OF_SECOND)); - assertEquals(clsTm1.getOffset(), clsTm2.getOffset()); + assertNotNull(clsTm); + OffsetTime expectedTime = OffsetTime.parse("12:13:14.123" + systemOffset); + assertEquals(expectedTime.get(ChronoField.HOUR_OF_DAY), clsTm.get(ChronoField.HOUR_OF_DAY)); + assertEquals(expectedTime.get(ChronoField.MINUTE_OF_HOUR), clsTm.get(ChronoField.MINUTE_OF_HOUR)); + assertEquals(expectedTime.get(ChronoField.SECOND_OF_MINUTE), clsTm.get(ChronoField.SECOND_OF_MINUTE)); + assertEquals(expectedTime.get(ChronoField.NANO_OF_SECOND), clsTm.get(ChronoField.NANO_OF_SECOND)); + assertEquals(expectedTime.getOffset(), clsTm.getOffset());Committable suggestion skipped: line range outside the PR's diff.
286-297: 🧹 Nitpick (assertive)
Add input validation to helper methods
The
setDatesIntoMessagemethod should validate its input parameters to ensure they're not null and maintain consistent state.private MxPacs00800102 setDatesIntoMessage( final LocalDate localDate, OffsetTime offsetTime, OffsetDateTime offsetDateTime) { + Objects.requireNonNull(localDate, "localDate must not be null"); + Objects.requireNonNull(offsetTime, "offsetTime must not be null"); + Objects.requireNonNull(offsetDateTime, "offsetDateTime must not be null"); + final MxPacs00800102 mx = new MxPacs00800102(); mx.setFIToFICstmrCdtTrf(new FIToFICustomerCreditTransferV02());📝 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 MxPacs00800102 setDatesIntoMessage( final LocalDate localDate, OffsetTime offsetTime, OffsetDateTime offsetDateTime) { Objects.requireNonNull(localDate, "localDate must not be null"); Objects.requireNonNull(offsetTime, "offsetTime must not be null"); Objects.requireNonNull(offsetDateTime, "offsetDateTime must not be null"); final MxPacs00800102 mx = new MxPacs00800102(); mx.setFIToFICstmrCdtTrf(new FIToFICustomerCreditTransferV02()); mx.getFIToFICstmrCdtTrf().setGrpHdr(new GroupHeader33()); mx.getFIToFICstmrCdtTrf().getGrpHdr().setCreDtTm(offsetDateTime); // date time mx.getFIToFICstmrCdtTrf().getGrpHdr().setIntrBkSttlmDt(localDate); // date mx.getFIToFICstmrCdtTrf().addCdtTrfTxInf(new CreditTransferTransactionInformation11()); mx.getFIToFICstmrCdtTrf().getCdtTrfTxInf().get(0).setSttlmTmReq(new SettlementTimeRequest2()); mx.getFIToFICstmrCdtTrf().getCdtTrfTxInf().get(0).getSttlmTmReq().setCLSTm(offsetTime); // time return mx; }
299-321: 🛠️ Refactor suggestion
Improve custom adapters for consistency and reliability
- Make both adapter classes consistently package-private or private inner classes
- Use fixed timestamps instead of
now()to prevent flaky tests- class TestCustomDateAdapter extends XmlAdapter<String, LocalDate> { + private static class TestCustomDateAdapter extends XmlAdapter<String, LocalDate> { @Override public LocalDate unmarshal(String v) { - return LocalDate.now(); + return LocalDate.of(2021, 10, 19); } } - public class TestCustomDateTimeAdapter extends XmlAdapter<String, OffsetDateTime> { + private static class TestCustomDateTimeAdapter extends XmlAdapter<String, OffsetDateTime> { @Override public OffsetDateTime unmarshal(String v) { - return OffsetDateTime.now(); + return OffsetDateTime.parse("2021-10-19T12:13:14+00:00"); } }📝 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 class TestCustomDateAdapter extends XmlAdapter<String, LocalDate> { @Override public LocalDate unmarshal(String v) { return LocalDate.of(2021, 10, 19); } @Override public String marshal(LocalDate cal) { return "foobar"; } } private static class TestCustomDateTimeAdapter extends XmlAdapter<String, OffsetDateTime> { @Override public OffsetDateTime unmarshal(String v) { return OffsetDateTime.parse("2021-10-19T12:13:14+00:00"); } @Override public String marshal(OffsetDateTime offsetDateTime) { return "foobar"; } }iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/AppHdrParserTest.java (2)
82-83: 🧹 Nitpick (assertive)
Consider using Month enum for better readability
While the transition to
ChronoFieldis good, the month assertion could be more readable using theMonthenum.- assertEquals(9, h.getCrDate().get(ChronoField.MONTH_OF_YEAR)); + assertEquals(Month.SEPTEMBER.getValue(), h.getCrDate().get(ChronoField.MONTH_OF_YEAR));📝 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.assertEquals(2006, h.getCrDate().get(ChronoField.YEAR)); assertEquals(Month.SEPTEMBER.getValue(), h.getCrDate().get(ChronoField.MONTH_OF_YEAR));
188-189: 🧹 Nitpick (assertive)
Apply consistent date testing improvements
Similar to the previous test method, consider using the
Monthenum for better readability. Additionally, since the test XML contains a complete timestamp, consider testing more fields (day, hour, etc.) for better coverage.assertEquals(2015, bah.getCreDt().get(ChronoField.YEAR)); - assertEquals(8, bah.getCreDt().get(ChronoField.MONTH_OF_YEAR)); + assertEquals(Month.AUGUST.getValue(), bah.getCreDt().get(ChronoField.MONTH_OF_YEAR)); + assertEquals(27, bah.getCreDt().get(ChronoField.DAY_OF_MONTH)); + assertEquals(8, bah.getCreDt().get(ChronoField.HOUR_OF_DAY)); + assertEquals(59, bah.getCreDt().get(ChronoField.MINUTE_OF_HOUR));📝 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.assertEquals(2015, bah.getCreDt().get(ChronoField.YEAR)); assertEquals(Month.AUGUST.getValue(), bah.getCreDt().get(ChronoField.MONTH_OF_YEAR)); assertEquals(27, bah.getCreDt().get(ChronoField.DAY_OF_MONTH)); assertEquals(8, bah.getCreDt().get(ChronoField.HOUR_OF_DAY)); assertEquals(59, bah.getCreDt().get(ChronoField.MINUTE_OF_HOUR));iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/adapters/DateTimeJsonAdapterBackwardCompatibilityTest.java (3)
399-434: 🧹 Nitpick (assertive)
Improve readability by extracting helper methods.
The test contains multiple chained method calls that make it harder to read and maintain. Consider extracting helper methods for accessing nested properties.
+ private LocalDate getTradeDt(MxSese02500109 mx) { + return mx.getSctiesSttlmTxConf() + .getTradDtls() + .getTradDt() + .getDt() + .getDt(); + } + @Test void allParsedDateTimeObjectsShouldMatch() { final MxSese02500109 fromV9 = (MxSese02500109) AbstractMX.fromJson(JSON_MX_V9); final MxSese02500109 fromV10 = (MxSese02500109) AbstractMX.fromJson(JSON_MX_V10); - LocalDate tradDtV9 = fromV9.getSctiesSttlmTxConf().getTradDtls().getTradDt().getDt().getDt(); - LocalDate tradDtV10 = fromV10.getSctiesSttlmTxConf().getTradDtls().getTradDt().getDt().getDt(); + LocalDate tradDtV9 = getTradeDt(fromV9); + LocalDate tradDtV10 = getTradeDt(fromV10);Committable suggestion skipped: line range outside the PR's diff.
380-397: 🧹 Nitpick (assertive)
Add Javadoc to clarify test purpose and expectations.
While the test name is descriptive, adding Javadoc would help clarify:
- The purpose of testing legacy format parsing
- The expected behavior when handling sentinel values
- The significance of the specific date-time values being tested
+ /** + * Verifies that the legacy XMLGregorianCalendar JSON format (V9) can be correctly parsed + * into the new date-time model, properly handling undefined time components and timezone information. + */ @Test void shouldBeAbleToParseJsonFromLegacyXMLGregorianCalendar() {📝 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./** * Verifies that the legacy XMLGregorianCalendar JSON format (V9) can be correctly parsed * into the new date-time model, properly handling undefined time components and timezone information. */ @Test void shouldBeAbleToParseJsonFromLegacyXMLGregorianCalendar() { final MxSese02500109 mx = (MxSese02500109) AbstractMX.fromJson(JSON_MX_V9); assertThat(mx.getSctiesSttlmTxConf().getTradDtls().getTradDt().getDt().getDt()) .isNotNull(); OffsetDateTime dateTime = mx.getSctiesSttlmTxConf().getTradDtls().getFctvSttlmDt().getDt().getDtTm(); assertThat(dateTime).isNotNull(); assertThat(dateTime.getYear()).isEqualTo(2024); assertThat(dateTime.getMonthValue()).isEqualTo(7); assertThat(dateTime.getDayOfMonth()).isEqualTo(17); assertThat(dateTime.getHour()).isEqualTo(7); assertThat(dateTime.getMinute()).isEqualTo(26); assertThat(dateTime.getSecond()).isEqualTo(4); assertThat(dateTime.getNano()).isEqualTo(207459000); }
17-192: 🧹 Nitpick (assertive)
Consider using named constants for sentinel values.
The JSON_MX_V9 constant uses
-2147483648as a sentinel value for undefined time components. This magic number could be confusing and should be replaced with a named constant for better readability.+ private static final int UNDEFINED_TIME_COMPONENT = -2147483648; private static final String JSON_MX_V9 = "{\n" + // ... (existing JSON with -2147483648 replaced with UNDEFINED_TIME_COMPONENT)Committable suggestion skipped: line range outside the PR's diff.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/AbstractMxJsonV9Test.java (5)
31-142: 🧹 Nitpick (assertive)
Consider using anonymized test data.
The XML test data contains what appears to be real-looking BIC codes (e.g., "FOORUS33") and addresses. Consider using clearly fictional data (e.g., "TESTUS33") to prevent any confusion with real financial institutions.
144-296: 🧹 Nitpick (assertive)
Improve test data maintainability.
The current approach of constructing JSON test data through string concatenation is error-prone and difficult to maintain. Consider:
- Using JSON files in the test resources directory
- Using a JSON library to construct test data programmatically
Example implementation:
private String loadJsonFromResource(String filename) { return new String(getClass().getResourceAsStream(filename).readAllBytes()); } // Usage @Test public void testMxToJson() { String jsonV10 = loadJsonFromResource("/testdata/mx_v10.json"); // ... rest of the test }
298-434: 🧹 Nitpick (assertive)
Reduce test data duplication.
The V9 and V10 JSON test data contain significant duplication. Consider creating a base JSON structure and only override the datetime-specific parts for each version.
Example approach:
private JsonObject createBaseJson() { // Create common JSON structure } private String createV9Json() { JsonObject json = createBaseJson(); // Override only V9-specific datetime fields return json.toString(); } private String createV10Json() { JsonObject json = createBaseJson(); // Override only V10-specific datetime fields return json.toString(); }
436-461: 🛠️ Refactor suggestion
Enhance test coverage with edge cases.
The current tests only cover the happy path. Consider adding tests for:
- Error handling (invalid JSON format, missing required fields)
- Edge cases for datetime values (timezone boundaries, maximum/minimum values)
- Null/empty value handling
Example additional tests:
@Test public void testMxFromInvalidJson() { assertThrows(JsonParseException.class, () -> MxPacs00900108.fromJson("{invalid}")); } @Test public void testMxWithExtremeDateTimeValues() { // Test with maximum timezone offset // Test with minimum timezone offset // Test with maximum nano value } @Test public void testMxWithNullValues() { // Test with null datetime fields // Test with empty strings }
24-28: 🧹 Nitpick (assertive)
Enhance class documentation with format differences.
Consider adding documentation that explains the key differences between V9 and V10 JSON formats, particularly regarding the datetime representation changes.
/** * Test for JSON conversion in the MX model (AbstractMX and subclasses). + * + * V9 format uses legacy datetime representation with: + * - timezone (in minutes) + * - fractionalSecond (as decimal) + * + * V10 format uses modern datetime representation with: + * - offset.totalSeconds (in seconds) + * - time.nano (in nanoseconds) * * @since 7.10.2 */📝 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./** * Test for JSON conversion in the MX model (AbstractMX and subclasses). * * V9 format uses legacy datetime representation with: * - timezone (in minutes) * - fractionalSecond (as decimal) * * V10 format uses modern datetime representation with: * - offset.totalSeconds (in seconds) * - time.nano (in nanoseconds) * * @since 7.10.2 */iso20022-core/src/main/java/com/prowidesoftware/swift/model/MxSwiftMessage.java (1)
21-21: 🧹 Nitpick (assertive)
Consider runtime environment compatibility
The migration to Jakarta persistence APIs requires ensuring that:
- The runtime environment supports Jakarta EE
- All dependent libraries are compatible with Jakarta EE
- The persistence provider (e.g., Hibernate) version supports Jakarta persistence
Summary by CodeRabbit
New Features
LocalDate,OffsetDateTime,OffsetTime,Year, andYearMonthobjects.OffsetDateTimeandLocalDate.Bug Fixes
Tests
Documentation