CU-86b2tfgf6_MxParseUtils---Method-to-resolve-MX-Message-Clearing-Type #134
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several significant changes, including the addition of a new class Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/SettlementInfo.java(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/SettlementMethod.java(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java(4 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/NamespaceReader.java(1 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java(4 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxWriteTest.java(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGELOG.md
3-3: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
6-6: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
3-3: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
4-4: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🔇 Additional comments (3)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/NamespaceReader.java (1)
18-19: LGTM!
The refactoring to use findElement from MxParseUtils is appropriate and enhances code reusability.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxWriteTest.java (1)
30-30: Document the rationale for disabling tests on Windows.
The @DisabledOnOs(OS.WINDOWS) annotation disables all tests in this class when running on Windows. This could potentially hide platform-specific issues and reduce test coverage. Please:
- Add a comment explaining why these tests need to be skipped on Windows
- Consider if there's a way to fix the underlying issue instead of disabling the tests
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java (1)
165-299: LGTM! Comprehensive test coverage for CLRG settlement method.
The test case thoroughly validates both message identification and settlement information extraction, including business service and clearing system code verification.
| Objects.requireNonNull(xml, "XML to parse must not be null"); | ||
| Validate.notBlank(xml, "XML to parse must not be a blank string"); | ||
| Objects.requireNonNull(xml, "localName to find must not be null"); |
There was a problem hiding this comment.
Correct null check and exception message in findElement method
At lines 313-315, the code incorrectly checks xml twice and uses an inappropriate exception message. It should check tags and provide the correct message.
Apply this diff to fix the issue:
Objects.requireNonNull(xml, "XML to parse must not be null");
Validate.notBlank(xml, "XML to parse must not be a blank string");
- Objects.requireNonNull(xml, "localName to find must not be null");
+ Objects.requireNonNull(tags, "Tags to find must not be null");
+ Validate.notEmpty(tags, "Tags to find must not be empty");📝 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.
| Objects.requireNonNull(xml, "XML to parse must not be null"); | |
| Validate.notBlank(xml, "XML to parse must not be a blank string"); | |
| Objects.requireNonNull(xml, "localName to find must not be null"); | |
| Objects.requireNonNull(xml, "XML to parse must not be null"); | |
| Validate.notBlank(xml, "XML to parse must not be a blank string"); | |
| Objects.requireNonNull(tags, "Tags to find must not be null"); | |
| Validate.notEmpty(tags, "Tags to find must not be empty"); |
| * of the required elements is found; otherwise, an empty {@link Optional}. | ||
| * @throws NullPointerException if the {@code xml} is null. | ||
| */ | ||
| public static Optional<SettlementInfo> getSettlementInfo(final String xml) { |
There was a problem hiding this comment.
Add null check for xml parameter in getSettlementInfo method
The method getSettlementInfo should explicitly check if xml is null to prevent potential NullPointerException, as indicated in the method documentation.
Apply this diff to add the null check:
+ Objects.requireNonNull(xml, "XML to parse must not be null");
Optional<XMLStreamReader> sttlmMtdMaybe = findElement(xml, "SttlmMtd");Committable suggestion skipped: line range outside the PR's diff.
| public class SettlementInfo { | ||
|
|
||
| private SettlementMethod settlementMethod; | ||
| private String clrSysCd; // Clearing System Code | ||
| private String clrSysPrtry; // Clearing System Proprietary Code | ||
|
|
||
| public SettlementInfo() {} | ||
|
|
||
| public SettlementMethod getSettlementMethod() { | ||
| return settlementMethod; | ||
| } | ||
|
|
||
| public void setSettlementMethod(SettlementMethod sttlmMtd) { | ||
| this.settlementMethod = sttlmMtd; | ||
| } | ||
|
|
||
| public String getClrSysCd() { | ||
| return clrSysCd; | ||
| } | ||
|
|
||
| public void setClrSysCd(String clrSysCd) { | ||
| this.clrSysCd = clrSysCd; | ||
| } | ||
|
|
||
| public String getClrSysPrtry() { | ||
| return clrSysPrtry; | ||
| } | ||
|
|
||
| public void setClrSysPrtry(String clrSysPrtry) { | ||
| this.clrSysPrtry = clrSysPrtry; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider overriding equals, hashCode, and toString methods
For better usability of the SettlementInfo class, especially when using in collections or logging, consider overriding the equals, hashCode, and toString methods.
| public static Optional<SettlementMethod> findByLabel(String label) { | ||
| return Arrays.stream(values()) | ||
| .filter(method -> method.label.equalsIgnoreCase(label)) | ||
| .findFirst(); | ||
| } | ||
|
|
||
| public static Optional<SettlementMethod> findByName(String name) { | ||
| return Arrays.stream(values()) | ||
| .filter(method -> method.name.equalsIgnoreCase(name)) | ||
| .findFirst(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle null inputs in findByLabel and findByName methods
The methods findByLabel and findByName may throw a NullPointerException if the input label or name is null. Consider adding null checks to handle null inputs gracefully.
Apply this diff to add null checks:
public static Optional<SettlementMethod> findByLabel(String label) {
+ if (label == null) {
+ return Optional.empty();
+ }
return Arrays.stream(values())
.filter(method -> method.label.equalsIgnoreCase(label))
.findFirst();
}
public static Optional<SettlementMethod> findByName(String name) {
+ if (name == null) {
+ return Optional.empty();
+ }
return Arrays.stream(values())
.filter(method -> method.name.equalsIgnoreCase(name))
.findFirst();
}📝 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 static Optional<SettlementMethod> findByLabel(String label) { | |
| return Arrays.stream(values()) | |
| .filter(method -> method.label.equalsIgnoreCase(label)) | |
| .findFirst(); | |
| } | |
| public static Optional<SettlementMethod> findByName(String name) { | |
| return Arrays.stream(values()) | |
| .filter(method -> method.name.equalsIgnoreCase(name)) | |
| .findFirst(); | |
| } | |
| public static Optional<SettlementMethod> findByLabel(String label) { | |
| if (label == null) { | |
| return Optional.empty(); | |
| } | |
| return Arrays.stream(values()) | |
| .filter(method -> method.label.equalsIgnoreCase(label)) | |
| .findFirst(); | |
| } | |
| public static Optional<SettlementMethod> findByName(String name) { | |
| if (name == null) { | |
| return Optional.empty(); | |
| } | |
| return Arrays.stream(values()) | |
| .filter(method -> method.name.equalsIgnoreCase(name)) | |
| .findFirst(); | |
| } |
CHANGELOG.md
Outdated
| #### 9.4.8 - SNAPSHOT | ||
| * Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message. | ||
| * Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix markdown heading levels and list formatting
The CHANGELOG.md has formatting issues that affect readability:
- At line 3, change the heading from h4 (
####) to h2 (##) to maintain proper heading hierarchy. - Adjust the indentation of the unordered list items at lines 4 and 5 to align correctly.
Apply this diff to fix the issues:
-#### 9.4.8 - SNAPSHOT
+## 9.4.8 - SNAPSHOT
- * Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message.
- * Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path
+* Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message.
+* Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path 📝 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.
| #### 9.4.8 - SNAPSHOT | |
| * Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message. | |
| * Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path | |
| ## 9.4.8 - SNAPSHOT | |
| * Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message. | |
| * Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path |
🧰 Tools
🪛 Markdownlint (0.35.0)
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)
5-5: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
6-6: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
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)
| + " <FinInstnId>" | ||
| + " <BICFI>FOOWDK22XXX</BICFI>" | ||
| + " <Nm>ARBEJDSMARKEDETS TILLAEGSPENSION</Nm>" | ||
| + " <PstlAdr>" | ||
| + " <PstCd>3400</PstCd>" | ||
| + " <TwnNm>REGION HOVEDSTADEN</TwnNm>" | ||
| + " <Ctry>DK</Ctry>" | ||
| + " </PstlAdr>" | ||
| + " </FinInstnId>" | ||
| + " </Dbtr>" | ||
| + " <DbtrAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOGBEBBXXX</BICFI>" | ||
| + " <Nm>THE BANK OF NEW YORK MELLON SA/NV</Nm>" | ||
| + " <PstlAdr>" | ||
| + " <PstCd>1000</PstCd>" | ||
| + " <TwnNm>BRUXELLES-CAPITALE</TwnNm>" | ||
| + " <Ctry>BE</Ctry>" | ||
| + " </PstlAdr>" | ||
| + " </FinInstnId>" | ||
| + " </DbtrAgt>" | ||
| + " <CdtrAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOVGB2LLON</BICFI>" | ||
| + " <Nm>SOCIETE GENERALE</Nm>" | ||
| + " <PstlAdr>" | ||
| + " <PstCd>E14 4SG</PstCd>" | ||
| + " <TwnNm>GREATER LONDON</TwnNm>" | ||
| + " <Ctry>GB</Ctry>" | ||
| + " </PstlAdr>" | ||
| + " </FinInstnId>" | ||
| + " </CdtrAgt>" | ||
| + " <Cdtr>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOVNL2AXXX</BICFI>" | ||
| + " <Nm>SOCIETE GENERALE</Nm>" | ||
| + " <PstlAdr>" | ||
| + " <PstCd>1096 HA</PstCd>" | ||
| + " <TwnNm>NOORD-HOLLAND</TwnNm>" | ||
| + " <Ctry>NL</Ctry>" | ||
| + " </PstlAdr>" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider masking sensitive data in test XML.
The test data contains what appears to be real-world postal addresses and institution names. While this provides realistic test scenarios, consider using fictional data to avoid any potential sensitive information exposure.
| @Test | ||
| void identifySettlementMethodINDA() { | ||
| String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" | ||
| + "<Document xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xmlns=\"http://www.six-interbank-clearing.com/de/pacs.028.001.01.chsepa.02\" xsi:schemaLocation=\"http://www.six-interbank-clearing.com/de/pacs.028.001.01.chsepa.02 pacs.028.001.01.chsepa.02.xsd\">" | ||
| + " <FIToFIPmtStsReq>" | ||
| + " <GrpHdr>" | ||
| + " <MsgId>MSGID-pacs028-20190529-1</MsgId>" | ||
| + " <CreDtTm>2019-05-29T09:30:47Z</CreDtTm>" | ||
| + " <InstgAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOICHBBXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </InstgAgt>" | ||
| + " <InstdAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOTDEFFXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </InstdAgt>" | ||
| + " </GrpHdr>" | ||
| + " <OrgnlGrpInf>" | ||
| + " <OrgnlMsgId>MSGID-camt056-20190522-2-SEPA</OrgnlMsgId>" | ||
| + " <OrgnlMsgNmId>camt.056.001.01</OrgnlMsgNmId>" | ||
| + " </OrgnlGrpInf>" | ||
| + " <TxInf>" | ||
| + " <StsReqId>STSREQID-pacs028StatusReqId4713</StsReqId>" | ||
| + " <OrgnlInstrId>XCORW-xzng943XzmxvoRwIvu5287</OrgnlInstrId>" | ||
| + " <OrgnlEndToEndId>1234567891</OrgnlEndToEndId>" | ||
| + " <OrgnlTxId>79809477-7-9998</OrgnlTxId>" | ||
| + " <OrgnlTxRef>" | ||
| + " <SttlmInf>" | ||
| + " <SttlmMtd>INDA</SttlmMtd>" | ||
| + " <ClrSys>" | ||
| + " <Prtry>ABE</Prtry>" | ||
| + " </ClrSys>" | ||
| + " </SttlmInf>" | ||
| + " <PmtTpInf>" | ||
| + " <SvcLvl>" | ||
| + " <Cd>SEPA</Cd>" | ||
| + " </SvcLvl>" | ||
| + " </PmtTpInf>" | ||
| + " <Dbtr>" | ||
| + " <Nm>Horlogerie du Joux, Mueller et Cie.</Nm>" | ||
| + " </Dbtr>" | ||
| + " <DbtrAcct>" | ||
| + " <Id>" | ||
| + " <IBAN>CH5598064001234567890</IBAN>" | ||
| + " </Id>" | ||
| + " </DbtrAcct>" | ||
| + " <DbtrAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOICHBBXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </DbtrAgt>" | ||
| + " <CdtrAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOYDEFFXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </CdtrAgt>" | ||
| + " <Cdtr>" | ||
| + " <Nm>Uhrengrosshandel Buxtehude, Peter Maier und Co.</Nm>" | ||
| + " </Cdtr>" | ||
| + " <CdtrAcct>" | ||
| + " <Id>" | ||
| + " <IBAN>DE47100100001234567890</IBAN>" | ||
| + " </Id>" | ||
| + " </CdtrAcct>" | ||
| + " </OrgnlTxRef>" | ||
| + " </TxInf>" | ||
| + " </FIToFIPmtStsReq>" | ||
| + "</Document>"; | ||
| MxId id = MxParseUtils.identifyMessage(xml).orElse(null); | ||
| assertNotNull(id); | ||
| assertEquals("pacs.028.001.01", id.id()); | ||
| assertFalse(id.getBusinessService().isPresent()); | ||
| Optional<SettlementInfo> settlementInfo = MxParseUtils.getSettlementInfo(xml); | ||
| assertTrue(settlementInfo.isPresent()); | ||
| assertEquals(SettlementMethod.INDA, settlementInfo.get().getSettlementMethod()); | ||
| assertEquals("ABE", settlementInfo.get().getClrSysPrtry()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Comprehensive test coverage for INDA settlement method.
The test case provides good coverage for an alternative settlement method scenario with proprietary clearing system code.
Consider moving the large XML test payloads to separate resource files to improve test readability. Example refactor:
- String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" +
- "<Document xmlns:xsi=..."; // long XML string
+ String xml = TestUtils.readResource("/testdata/pacs.028.001.01.xml");Committable suggestion skipped: line range outside the PR's diff.
ptorres-prowide
force-pushed
the
CU-86b2tfgf6_MxParseUtils---Method-to-resolve-MX-Message-Clearing-Type
branch
from
67d2358
to
8c334cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/SettlementInfo.java(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/SettlementMethod.java(1 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java(4 hunks)iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/NamespaceReader.java(1 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java(4 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxWriteTest.java(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
CHANGELOG.md
3-3: Expected: h2; Actual: h4
Heading levels should only increment by one level at a time
(MD001, heading-increment)
4-4: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
6-6: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
3-3: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines
(MD022, blanks-around-headings)
4-4: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🔇 Additional comments (10)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java (2)
268-268: Add null check for xml parameter in getSettlementInfo method
The method getSettlementInfo should explicitly check if xml is null to prevent potential NullPointerException, as indicated in the method documentation.
Apply this diff to add the null check:
+ Objects.requireNonNull(xml, "XML to parse must not be null");
Optional<XMLStreamReader> sttlmMtdMaybe = findElement(xml, "SttlmMtd");317-319: Correct null check and exception message in findElement method
At lines 317-319, the code incorrectly checks xml twice and uses an inappropriate exception message. It should check tags and provide the correct message.
Apply this diff to fix the issue:
Objects.requireNonNull(xml, "XML to parse must not be null");
Validate.notBlank(xml, "XML to parse must not be a blank string");
- Objects.requireNonNull(xml, "localName to find must not be null");
+ Objects.requireNonNull(tags, "Tags to find must not be null");
+ Validate.notEmpty(tags, "Tags to find must not be empty");iso20022-core/src/main/java/com/prowidesoftware/swift/model/SettlementInfo.java (1)
11-42: Consider overriding equals, hashCode, and toString methods
For better usability of the SettlementInfo class, especially when using in collections or logging, consider overriding the equals, hashCode, and toString methods.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/SettlementMethod.java (1)
34-44: Handle null inputs in findByLabel and findByName methods
The methods findByLabel and findByName may throw a NullPointerException if the input label or name is null. Consider adding null checks to handle null inputs gracefully.
Apply this diff to add null checks:
public static Optional<SettlementMethod> findByLabel(String label) {
+ if (label == null) {
+ return Optional.empty();
+ }
return Arrays.stream(values())
.filter(method -> method.label.equalsIgnoreCase(label))
.findFirst();
}
public static Optional<SettlementMethod> findByName(String name) {
+ if (name == null) {
+ return Optional.empty();
+ }
return Arrays.stream(values())
.filter(method -> method.name.equalsIgnoreCase(name))
.findFirst();
}CHANGELOG.md (1)
3-6: 🛠️ Refactor suggestion
Fix Markdown formatting issues
The CHANGELOG.md file has formatting issues that affect readability:
- At line 3, change the heading from h4 (
####) to h2 (##) to maintain proper heading hierarchy. - Adjust the indentation of the unordered list items at lines 4 and 5 to align correctly.
Apply this diff to fix the issues:
-#### 9.5.5 - SNAPSHOT
+## 9.5.5 - SNAPSHOT
- * Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message.
- * Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path
+* Added new SettlementInfo class, and added MxParseUtils#getSettlementInfo to extract it from a raw MX message.
+* Moved and enhanced the MxSwiftMessage#findElement to support multiple element's path Likely invalid or redundant comment.
🧰 Tools
🪛 Markdownlint (0.35.0)
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)
5-5: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
5-5: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
6-6: Expected: 0 or 2; Actual: 1
Trailing spaces
(MD009, no-trailing-spaces)
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)
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxWriteTest.java (1)
24-25: Verify the rationale for disabling tests on Windows.
The @DisabledOnOs(OS.WINDOWS) annotation disables all tests in this class when running on Windows systems. This could significantly impact test coverage on Windows environments.
Also applies to: 30-30
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java (2)
165-299: Consider masking sensitive data in test XML.
The test data contains what appears to be real-world postal addresses and institution names. While this provides realistic test scenarios, consider using fictional data to avoid any potential sensitive information exposure.
301-379: LGTM! Comprehensive test coverage for INDA settlement method.
The test case provides good coverage for an alternative settlement method scenario with proprietary clearing system code.
Consider moving the large XML test payloads to separate resource files to improve test readability.
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/NamespaceReader.java (2)
Line range hint 1-95: LGTM! The refactoring improves code organization
The changes effectively consolidate XML parsing logic in MxParseUtils while maintaining clean code principles and clear documentation. The class continues to fulfill its single responsibility of namespace handling effectively.
18-19: 🧹 Nitpick (assertive)
Verify behavior consistency with MxParseUtils#findElement
The refactoring to use MxParseUtils#findElement improves code organization by consolidating XML parsing logic. However, we should ensure complete behavioral compatibility.
✅ Verification successful
Behavior consistency with MxParseUtils#findElement is verified
The verification shows that the findElement method in MxParseUtils is a well-tested, stable implementation that:
- Handles various XML namespace scenarios correctly
- Supports both single and multi-tag path searches
- Is used consistently throughout the codebase for XML parsing
- Has extensive test coverage in
MxParseUtilsTestandNamespaceReaderTest - Maintains backward compatibility as evidenced by the test cases
The refactoring to use this implementation through static import is safe and improves code organization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the behavior consistency of findElement implementation
# Check the implementation in MxParseUtils
ast-grep --pattern 'findElement($_, $_) {
$$$
}'
# Look for any test cases that might help verify behavior
rg -l "findElement.*test" --type java
Length of output: 92
Script:
#!/bin/bash
# Let's try different approaches to find the implementation and usage
# 1. Search for findElement method with broader pattern
ast-grep --pattern 'findElement'
# 2. Look for the actual file and its content
fd MxParseUtils.java --type f --exec cat {}
# 3. Search for test files related to namespace or XML parsing
rg -l "NamespaceReader.*Test" --type java
rg -l "MxParseUtils.*Test" --type java
# 4. Look for usage patterns of findElement
rg "findElement\(" --type java -A 2
Length of output: 20190
Script:
#!/bin/bash
# Let's check the test files to verify behavior consistency
# Check NamespaceReaderTest content
cat iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/NamespaceReaderTest.java
# Check MxParseUtilsTest content
cat iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java
Length of output: 28372
| final XMLStreamReader reader = | ||
| xif.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml))); | ||
| while (reader.hasNext()) { | ||
| int event = reader.next(); | ||
| if (XMLStreamConstants.START_ELEMENT == event) { | ||
| if (reader.getLocalName().equals(tags[tagsIndex])) { | ||
| if (tagsIndex == tags.length - 1) { | ||
| return Optional.of(reader); | ||
| } | ||
| tagsIndex++; | ||
| } | ||
| } | ||
| } | ||
| } catch (XMLStreamException e) { | ||
| log.log(Level.WARNING, "Error reading XML", e); | ||
| } | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure XMLStreamReader is properly closed to prevent resource leaks
The XMLStreamReader should be closed after usage to prevent resource leaks. Consider using a try-with-resources statement to automatically close the reader.
Apply this diff to use try-with-resources:
try {
- final XMLStreamReader reader =
- xif.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)));
+ try (final XMLStreamReader reader =
+ xif.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)))) {
while (reader.hasNext()) {
int event = reader.next();
if (XMLStreamConstants.START_ELEMENT == event) {
if (reader.getLocalName().equals(tags[tagsIndex])) {
if (tagsIndex == tags.length - 1) {
return Optional.of(reader);
}
tagsIndex++;
}
}
}
}
} catch (XMLStreamException e) {
log.log(Level.WARNING, "Error reading XML", e);
}Committable suggestion skipped: line range outside the PR's diff.
| + " <PstlAdr>" | ||
| + " <PstCd>1096 HA</PstCd>" | ||
| + " <TwnNm>NOORD-HOLLAND</TwnNm>" | ||
| + " <Ctry>NL</Ctry>" | ||
| + " </PstlAdr>" | ||
| + " </FinInstnId>" | ||
| + " </Cdtr>" | ||
| + " </CdtTrfTxInf>" | ||
| + " </FICdtTrf>" | ||
| + " </Document>" | ||
| + "</RequestPayload>"; | ||
| MxId id = MxParseUtils.identifyMessage(xml).orElse(null); | ||
| assertNotNull(id); | ||
| assertEquals("pacs.009.001.08", id.id()); | ||
| assertEquals("cbn.rtgs.01", id.getBusinessService().orElse(null)); | ||
| Optional<SettlementInfo> settlementInfo = MxParseUtils.getSettlementInfo(xml); | ||
| assertTrue(settlementInfo.isPresent()); | ||
| assertEquals(SettlementMethod.CLRG, settlementInfo.get().getSettlementMethod()); | ||
| assertEquals("TGT", settlementInfo.get().getClrSysCd()); | ||
| } | ||
|
|
||
| @Test | ||
| void identifySettlementMethodINDA() { | ||
| String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>" | ||
| + "<Document xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xmlns=\"http://www.six-interbank-clearing.com/de/pacs.028.001.01.chsepa.02\" xsi:schemaLocation=\"http://www.six-interbank-clearing.com/de/pacs.028.001.01.chsepa.02 pacs.028.001.01.chsepa.02.xsd\">" | ||
| + " <FIToFIPmtStsReq>" | ||
| + " <GrpHdr>" | ||
| + " <MsgId>MSGID-pacs028-20190529-1</MsgId>" | ||
| + " <CreDtTm>2019-05-29T09:30:47Z</CreDtTm>" | ||
| + " <InstgAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOICHBBXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </InstgAgt>" | ||
| + " <InstdAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOTDEFFXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </InstdAgt>" | ||
| + " </GrpHdr>" | ||
| + " <OrgnlGrpInf>" | ||
| + " <OrgnlMsgId>MSGID-camt056-20190522-2-SEPA</OrgnlMsgId>" | ||
| + " <OrgnlMsgNmId>camt.056.001.01</OrgnlMsgNmId>" | ||
| + " </OrgnlGrpInf>" | ||
| + " <TxInf>" | ||
| + " <StsReqId>STSREQID-pacs028StatusReqId4713</StsReqId>" | ||
| + " <OrgnlInstrId>XCORW-xzng943XzmxvoRwIvu5287</OrgnlInstrId>" | ||
| + " <OrgnlEndToEndId>1234567891</OrgnlEndToEndId>" | ||
| + " <OrgnlTxId>79809477-7-9998</OrgnlTxId>" | ||
| + " <OrgnlTxRef>" | ||
| + " <SttlmInf>" | ||
| + " <SttlmMtd>INDA</SttlmMtd>" | ||
| + " <ClrSys>" | ||
| + " <Prtry>ABE</Prtry>" | ||
| + " </ClrSys>" | ||
| + " </SttlmInf>" | ||
| + " <PmtTpInf>" | ||
| + " <SvcLvl>" | ||
| + " <Cd>SEPA</Cd>" | ||
| + " </SvcLvl>" | ||
| + " </PmtTpInf>" | ||
| + " <Dbtr>" | ||
| + " <Nm>Horlogerie du Joux, Mueller et Cie.</Nm>" | ||
| + " </Dbtr>" | ||
| + " <DbtrAcct>" | ||
| + " <Id>" | ||
| + " <IBAN>CH5598064001234567890</IBAN>" | ||
| + " </Id>" | ||
| + " </DbtrAcct>" | ||
| + " <DbtrAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOICHBBXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </DbtrAgt>" | ||
| + " <CdtrAgt>" | ||
| + " <FinInstnId>" | ||
| + " <BICFI>FOOYDEFFXXX</BICFI>" | ||
| + " </FinInstnId>" | ||
| + " </CdtrAgt>" | ||
| + " <Cdtr>" | ||
| + " <Nm>Uhrengrosshandel Buxtehude, Peter Maier und Co.</Nm>" | ||
| + " </Cdtr>" | ||
| + " <CdtrAcct>" | ||
| + " <Id>" | ||
| + " <IBAN>DE47100100001234567890</IBAN>" | ||
| + " </Id>" | ||
| + " </CdtrAcct>" | ||
| + " </OrgnlTxRef>" | ||
| + " </TxInf>" | ||
| + " </FIToFIPmtStsReq>" | ||
| + "</Document>"; | ||
| MxId id = MxParseUtils.identifyMessage(xml).orElse(null); | ||
| assertNotNull(id); | ||
| assertEquals("pacs.028.001.01", id.id()); | ||
| assertFalse(id.getBusinessService().isPresent()); | ||
| Optional<SettlementInfo> settlementInfo = MxParseUtils.getSettlementInfo(xml); | ||
| assertTrue(settlementInfo.isPresent()); | ||
| assertEquals(SettlementMethod.INDA, settlementInfo.get().getSettlementMethod()); | ||
| assertEquals("ABE", settlementInfo.get().getClrSysPrtry()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider parameterizing test cases for better coverage.
The test methods provide good coverage for CLRG and INDA settlement methods. Consider using parameterized tests to cover additional settlement methods and clearing system combinations more efficiently.
Example refactor:
- @Test
- void identifySettlementMethodCLRG() {
- String xml = "...";
- // test logic
- }
-
- @Test
- void identifySettlementMethodINDA() {
- String xml = "...";
- // test logic
- }
+ @ParameterizedTest
+ @CsvSource({
+ "CLRG,TGT,",
+ "INDA,,ABE"
+ })
+ void identifySettlementMethod(String method, String code, String proprietary) {
+ String xml = loadTestXml(method, code, proprietary);
+ // common test logic
+ }Committable suggestion skipped: line range outside the PR's diff.
| import static com.prowidesoftware.swift.model.mx.MxParseUtils.findElement; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the dependency on MxParseUtils
Since this class now depends on MxParseUtils, consider adding a note in the class-level documentation about this dependency for better maintainability.
/**
* Helper API to extract information from an XML using lightweight XML streams API
*
+ * This class relies on {@link MxParseUtils#findElement} for XML parsing operations.
*
* @since 9.2.1
*/📝 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.
| import static com.prowidesoftware.swift.model.mx.MxParseUtils.findElement; | |
| /** | |
| * Helper API to extract information from an XML using lightweight XML streams API | |
| * This class relies on {@link MxParseUtils#findElement} for XML parsing operations. | |
| * | |
| * @since 9.2.1 | |
| */ |
Summary by CodeRabbit
Release Notes for Version 9.5.5
New Features
SettlementInfoclass to encapsulate settlement-related information.getSettlementInfomethod for extracting settlement information from XML documents.findElementmethod to support multiple element paths for improved XML parsing.SettlementMethodenum to represent various settlement methods for financial transactions.Bug Fixes
Tests
Chores