CU-86b2tfgf6: MxParseUtils enhancement API to retrieve content by paths #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 | |
| */ |
|
|
||
| private SettlementMethod settlementMethod; | ||
| private String clrSysCd; // Clearing System Code | ||
| private String clrSysPrtry; // Clearing System Proprietary Code |
There was a problem hiding this comment.
rename it to clearingSystemCode and as a single field, we do not care if it comes from code or proprietary
fernando-prowide
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
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(3 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java(4 hunks)
🔇 Additional comments (10)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/NamespaceReader.java (3)
18-18: Document the dependency on MxParseUtils
The class now depends on MxParseUtils through the static import of findElementByTags. This dependency should be documented in the class-level documentation.
61-62: LGTM! Good refactoring
The change improves code reusability by leveraging MxParseUtils#findElementByTags while maintaining the same functionality and backward compatibility.
92-92: Verify error handling behavior
Please ensure that MxParseUtils#findElementByTags handles XML parsing errors consistently with the previous implementation.
✅ Verification successful
Let me generate additional scripts to check the error handling behavior:
Based on the search results, I can now provide a final response since we have clear evidence of the error handling behavior in MxParseUtils:
Error handling is consistent with previous implementation
The findElementByTags method in MxParseUtils maintains proper error handling by:
- Catching XMLStreamException and wrapping it in a RuntimeException, consistent with standard XML parsing error handling
- Returning Optional.empty() when the element is not found, providing the same null-safe behavior
- Logging errors appropriately at warning level for debugging purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for error handling in MxParseUtils#findElementByTags
# Search for error handling in findElementByTags implementation
ast-grep --pattern 'findElementByTags($$$) {
try {
$$$
} catch ($$$) {
$$$
}
}'
# Search for tests covering error cases
rg -A 5 "findElementByTags.*malformed|invalid|error"
Length of output: 124531
Script:
#!/bin/bash
# Search for findElementByTags implementation and error handling
ast-grep --pattern 'findElementByTags($$$) {
$$$
}'
# Search for tests related to findElementByTags error cases
rg -A 5 "test.*findElementByTags.*error|test.*findElementByTags.*invalid|test.*findElementByTags.*malformed"
# Search for any exception handling related to XML parsing
rg -A 5 "catch.*SAXException|catch.*XMLStreamException|catch.*ParserConfigurationException"
Length of output: 3591
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java (4)
321-321: Correct null check and exception message in findElementByTags method
At line 321, the code incorrectly checks xml 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, "tags 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");323-343: Ensure XMLStreamReader is properly closed to prevent resource leaks
The XMLStreamReader should be closed after usage to prevent resource leaks. Consider using try-with-resources to automatically close the reader.
367-367:
Correct null check and exception message in findElementByPath method
At line 367, the code incorrectly checks xml and uses an inappropriate exception message. It should check targetPath 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, "targetPath to find must not be null");
+Objects.requireNonNull(targetPath, "targetPath to find must not be null");Likely invalid or redundant comment.
371-408:
Ensure XMLStreamReader is properly closed to prevent resource leaks in findElementByPath
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:
- final XMLStreamReader reader =
- factory.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)));
- // existing code...
- while (reader.hasNext()) {
+ try (final XMLStreamReader reader =
+ factory.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)))) {
+ // existing code...
+ while (reader.hasNext()) {
// parsing logic
}
}Likely invalid or redundant comment.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java (3)
31-98: Consider moving large XML strings to external resources for better readability
Including large XML strings directly in the code reduces test readability and maintainability. Consider moving the XML payloads to external resource files and loading them during the tests.
237-370: Consider moving large XML payloads to external resource files
The test method identifySettlementMethodCLRG includes a large XML string embedded within the code. Moving the XML to an external file would improve readability and make it easier to manage test data.
385-420: 🧹 Nitpick (assertive)
Parameterize tests to improve coverage and reduce redundancy
The test methods testFindFieldValueByTags and testFindFieldValueByAbsolutePath have similar structures but test different inputs. Consider using parameterized tests to streamline the code and enhance test coverage.
Example refactor using JUnit 5 parameterized tests:
- @Test
- void testFindFieldValueByTags() throws XMLStreamException {
- // Test logic
- }
-
- @Test
- void testFindFieldValueByAbsolutePath() throws XMLStreamException {
- // Test logic
- }
+ @ParameterizedTest
+ @MethodSource("provideTestCases")
+ void testFindFieldValue(
+ String methodName, String[] tagsOrPath, String expectedValue) throws XMLStreamException {
+ Optional<XMLStreamReader> result;
+ if ("findElementByTags".equals(methodName)) {
+ result = MxParseUtils.findElementByTags(xml_pacs_008_001_01, tagsOrPath);
+ } else {
+ result = MxParseUtils.findElementByPath(xml_pacs_008_001_01, tagsOrPath[0]);
+ }
+ assertTrue(result.isPresent());
+ assertEquals(expectedValue, result.get().getElementText());
+ }
+
+ static Stream<Arguments> provideTestCases() {
+ return Stream.of(
+ Arguments.of("findElementByTags", new String[]{"FIToFIPmtStsReq", "GrpHdr", "MsgId"}, "MSGID-pacs028-20190529-1"),
+ Arguments.of("findElementByPath", new String[]{"/Document/FIToFIPmtStsReq/GrpHdr/MsgId"}, "MSGID-pacs028-20190529-1")
+ // Add more test cases as needed
+ );
+ }Likely invalid or redundant comment.
| * \ | ||
| * @param pathStack | ||
| * @return the current path | ||
| * Join the stack elements with "/" to form the current path | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix incomplete and malformed Javadoc comment in buildCurrentPath method
At lines 411-415, the Javadoc comment is incomplete and contains an unnecessary backslash, which may cause documentation generation issues.
Apply this diff to correct the Javadoc:
- /**
- * \
- * @param pathStack
- * @return the current path
- * Join the stack elements with "/" to form the current path
- */
+ /**
+ * Joins the stack elements with "/" to form the current XML path.
+ *
+ * @param pathStack the stack containing XML element names
+ * @return the current path as a String
+ */📝 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.
| * \ | |
| * @param pathStack | |
| * @return the current path | |
| * Join the stack elements with "/" to form the current path | |
| */ | |
| /** | |
| * Joins the stack elements with "/" to form the current XML path. | |
| * | |
| * @param pathStack the stack containing XML element names | |
| * @return the current path as a String | |
| */ |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java(5 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java(4 hunks)
🔇 Additional comments (8)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java (6)
271-271:
Add null check for xml parameter in getSettlementInfo method
The method getSettlementInfo should explicitly check if xml is null to prevent a 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 = findElementByTags(xml, "SttlmMtd");321-321:
Correct null check for tags parameter in findElementByTags method
At line 321, the code incorrectly checks xml instead of tags and provides an incorrect exception message. It should check tags for nullity.
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, "tags 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");326-339: 🛠️ Refactor suggestion
Ensure XMLStreamReader is properly closed in findElementByTags method
The XMLStreamReader should be closed after usage to prevent resource leaks. Consider using a try-with-resources statement.
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);
}411-415: 🧹 Nitpick (assertive)
Fix incomplete and malformed Javadoc comment in buildCurrentPath method
At lines 411-415, the Javadoc comment is incomplete and contains an unnecessary backslash, which may cause documentation generation issues.
Apply this diff to correct the Javadoc:
- /**
- * \
- * @param pathStack
- * @return the current path
- * Join the stack elements with "/" to form the current path
- */
+ /**
+ * Joins the stack elements with "/" to form the current XML path.
+ *
+ * @param pathStack the stack containing XML element names
+ * @return the current path as a String
+ */366-366:
Correct null check for targetPath parameter in findElementByPath method
At line 366, the code incorrectly checks xml instead of targetPath and provides an incorrect exception message. It should check targetPath for nullity.
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, "targetPath to find must not be null");
+ Objects.requireNonNull(targetPath, "targetPath to find must not be null");Likely invalid or redundant comment.
371-407: 🛠️ Refactor suggestion
Ensure XMLStreamReader is properly closed in findElementByPath method
The XMLStreamReader in findElementByPath should be closed after usage to prevent resource leaks. Use a try-with-resources statement to ensure it is closed automatically.
Apply this diff:
try {
- final XMLStreamReader reader =
- factory.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)));
+ try (final XMLStreamReader reader =
+ factory.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)))) {
Stack<String> pathStack = new Stack<>();
while (reader.hasNext()) {
int event = reader.next();
switch (event) {
case XMLStreamConstants.START_ELEMENT:
if (!reader.getLocalName().equals("RequestPayload")) {
pathStack.push(reader.getLocalName());
String currentPath = buildCurrentPath(pathStack);
if (currentPath.equals(targetPath)) {
return Optional.of(reader);
}
}
break;
case XMLStreamConstants.END_ELEMENT:
if (!pathStack.isEmpty()) {
pathStack.pop();
}
break;
default:
break;
}
}
+ }
} catch (XMLStreamException e) {
throw new RuntimeException(e);
}Likely invalid or redundant comment.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java (2)
236-382: 🧹 Nitpick (assertive)
Consider parameterizing test cases for better coverage
The test methods identifySettlementMethodCLRG and identifySettlementMethodINDA provide good coverage for specific settlement methods. Consider using parameterized tests to cover additional settlement methods and clearing system combinations more efficiently.
Example refactor:
@ParameterizedTest
@CsvSource({
"CLRG,TGT,",
"INDA,,ABE"
})
void identifySettlementMethod(String method, String code, String proprietary) {
String xml = loadTestXml(method, code, proprietary);
// common test logic
}238-382: 🧹 Nitpick (assertive)
Consider masking sensitive data in test XML
The test data includes realistic institution names and addresses. To prevent potential exposure of sensitive information, consider using fictional data or anonymizing the existing data.
| String xml_pacs_008_001_01 = "<?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>"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider moving large XML test data to external resource files
The large XML string xml_pacs_008_001_01 reduces test readability and maintainability. Consider moving this test data to a separate resource file and loading it during the test.
Example refactor:
String xml_pacs_008_001_01 = TestUtils.readResource("/testdata/pacs_008_001_01.xml");
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java(5 hunks)iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java(4 hunks)
🔇 Additional comments (8)
iso20022-core/src/main/java/com/prowidesoftware/swift/model/mx/MxParseUtils.java (5)
319-322:
Correct null check for 'tags' parameter in findElementByTags method
At lines 319-322, 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, "tags 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");411-416: 🧹 Nitpick (assertive)
Fix incomplete and malformed Javadoc comment in buildCurrentPath method
At lines 411-416, the Javadoc comment is incomplete and contains an unnecessary backslash, which may cause documentation generation issues.
Apply this diff to correct the Javadoc:
- /**
- * \
- * @param pathStack
- * @return the current path
- * Join the stack elements with "/" to form the current path
- */
+ /**
+ * Joins the stack elements with "/" to form the current XML path.
+ *
+ * @param pathStack the stack containing XML element names
+ * @return the current path as a String
+ */271-272:
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 = findElementByTags(xml, "SttlmMtd");Likely invalid or redundant comment.
326-343: 🛠️ 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);
}Likely invalid or redundant comment.
371-409: 🛠️ 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 =
- factory.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)));
+ try (final XMLStreamReader reader =
+ factory.createXMLStreamReader(new StringReader(MxParseUtils.makeXmlLenient(xml)))) {
Stack<String> pathStack = new Stack<>();
while (reader.hasNext()) {
int event = reader.next();
switch (event) {
case XMLStreamConstants.START_ELEMENT:
// Push the current element onto the path stack
if (!reader.getLocalName().equals("RequestPayload")) {
pathStack.push(reader.getLocalName());
// Build the current path
String currentPath = buildCurrentPath(pathStack);
// Check if the current path matches the target path
if (currentPath.equals(targetPath)) {
return Optional.of(reader);
}
break;
}
case XMLStreamConstants.END_ELEMENT:
// Pop the element from the path stack
if (!pathStack.isEmpty()) {
pathStack.pop();
}
break;
default:
break;
}
}
- }
} catch (XMLStreamException e) {
throw new RuntimeException(e);
}Likely invalid or redundant comment.
iso20022-core/src/test/java/com/prowidesoftware/swift/model/mx/MxParseUtilsTest.java (3)
31-99: 🧹 Nitpick (assertive)
Consider moving large XML test data to external resource files
Including large XML strings directly in test code reduces readability and maintainability. Consider moving this test data to separate resource files and loading it during the test.
Example refactor:
String xml_pacs_008_001_01 = TestUtils.readResource("/testdata/pacs_008_001_01.xml");238-370: 🧹 Nitpick (assertive)
Consider moving large XML test data to external resource files
Including large XML strings directly in test code reduces readability and maintainability. Consider moving this test data to separate resource files and loading it during the test.
Example refactor:
String xml = TestUtils.readResource("/testdata/pacs_009_001_08.xml");236-382: 🧹 Nitpick (assertive)
Consider parameterizing test cases for better coverage and maintainability
The test methods identifySettlementMethodCLRG and identifySettlementMethodINDA provide good coverage for different settlement methods. Consider using parameterized tests to cover additional settlement methods and clearing system combinations more efficiently.
Example refactor:
@ParameterizedTest
@CsvSource({
"'pacs.009.001.08', 'cbn.rtgs.01', 'CLRG', 'TGT', null",
"'pacs.028.001.01', '', 'INDA', null, 'ABE'"
})
void identifySettlementMethod(
String messageId, String bizService, String settlementMethod, String clrSysCd, String clrSysPrtry) {
String xml = loadTestXml(messageId, settlementMethod, clrSysCd, clrSysPrtry);
// common test logic
}| public static Optional<XMLStreamReader> findElementByAbsolutePath(String xml, String targetPath) { | ||
| 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, "targetPath to find must not be null"); |
There was a problem hiding this comment.
Correct null check for 'targetPath' parameter in findElementByAbsolutePath method
At lines 363-366, the code incorrectly checks xml instead of targetPath, and provides an inappropriate exception message. It should check targetPath 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, "targetPath to find must not be null");
+Objects.requireNonNull(targetPath, "targetPath to find 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 static Optional<XMLStreamReader> findElementByAbsolutePath(String xml, String targetPath) { | |
| 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, "targetPath to find must not be null"); | |
| public static Optional<XMLStreamReader> findElementByAbsolutePath(String xml, String targetPath) { | |
| Objects.requireNonNull(xml, "XML to parse must not be null"); | |
| Validate.notBlank(xml, "XML to parse must not be a blank string"); | |
| Objects.requireNonNull(targetPath, "targetPath to find must not be null"); |
| * @throws IllegalArgumentException if the {@code xml} is a blank string. | ||
| * @since 9.5.5 | ||
| */ | ||
| public static Optional<XMLStreamReader> findElementByAbsolutePath(String xml, String targetPath) { |
There was a problem hiding this comment.
should be byPath and the relative path indicated with double slash at the beginning
/A/B/C -> Absolute
//C/D -> relative
C/D -> same as absolute because you search C from the root
| MxParseUtils.findElementByAbsolutePath(xml_camt_053_001_12, "/AppHdr/Fr/FIId/FinInstnId/BICFI"); | ||
| Optional<XMLStreamReader> bizPrcgDt = MxParseUtils.findElementByAbsolutePath(xml_camt_053_001_12, "/AppHdr/BizPrcgDt"); | ||
| Optional<XMLStreamReader> pgNb = | ||
| MxParseUtils.findElementByAbsolutePath(xml_camt_053_001_12, "/Document/BkToCstmrStmt/GrpHdr/MsgPgntn/PgNb"); |
There was a problem hiding this comment.
for example relative
//MsgPgntn/PgNb
zubri
changed the title
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.findElementByTagsmethod.SettlementMethodenum to represent various settlement methods for financial transactions.Bug Fixes
Tests
Chores
MxWriteTestfor Windows operating systems.