Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor rs-e2e project to remove Hapi HL7 library #1654

Open
wants to merge 69 commits into
base: main
Choose a base branch
from

Conversation

basiliskus
Copy link
Contributor

@basiliskus basiliskus commented Dec 12, 2024

Description

  • Removed the ca.uhn.hapi dependencies
  • Added HL7Message, HL7Segment, HL7Encoding and HL7Path classes to replace Hapi's Message class
  • Improved and refactored HL7 parser and expression evaluator
  • Improved handling of edge cases
  • Reorganized file structure
  • Added full test coverage

Issue

#1532

Checklist

  • I have added tests to cover my changes
  • I have added JavaDocs where required
  • I have updated the documentation accordingly

Co-authored-by: luis-pabon-tf <luis-pabon-tf@users.noreply.github.com>
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1532 - Partially compliant

Fully compliant requirements:

  • Remove dependency to HAPI HL7 library

Not compliant requirements:

  • Refactoring parser
  • Expanding the functionality of the parser to include what we're currently using the HAPI HL7 library for
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling
The error handling in the file matching process might not cover all potential issues, such as parsing errors.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add exception handling for parsing HL7 messages to manage potential parsing errors

Ensure that the HL7Parser.parse method handles exceptions internally or declare that
it throws an appropriate exception, and handle this in the calling method.

rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcher.java [63]

-HL7Message message = HL7Parser.parse(content);
+HL7Message message;
+try {
+  message = HL7Parser.parse(content);
+} catch (HL7ParseException e) {
+  throw new HapiHL7FileMatcherException(
+    String.format("Failed to parse HL7 message from file: %s", fileName), e);
+}
Suggestion importance[1-10]: 8

Why: The suggestion to add exception handling around the HL7Parser.parse method is crucial because it ensures robust error management and prevents the application from crashing due to unhandled exceptions during the parsing of HL7 messages. This enhances the reliability and maintainability of the code.

8

basiliskus and others added 24 commits December 12, 2024 09:10
Remove library usage on test class, one test not finished yet.
Corrected structure on test object
Replacing Message with HL7Message, some incompatible methods remain
Add method for getting segment counts
Adding replacement method for evaluation collection counts
Adding some deprecation flags and replacement methods
Updated test data and a mock
…e hapi dependency

Co-authored-by: luis-pabon-tf <luis-pabon-tf@users.noreply.github.com>
Most tests are passing, and HAPI Message is gone now.
@jbiskie
Copy link
Contributor

jbiskie commented Dec 20, 2024

Tested and looks good.

Sonar flagged a circular dependency between HL7Message and HL7Parser, and there are actually a lot of them. Should we consider tagging this a a separate task to clean up?

@basiliskus
Copy link
Contributor Author

Sonar flagged a circular dependency between HL7Message and HL7Parser, and there are actually a lot of them. Should we consider tagging this a a separate task to clean up?

I'll take a look. If it's straightforward to fix, we can include it in this PR


String segmentName = matcher.group(1);
int[] indices =
Arrays.stream(matcher.group(2).split("\\.")).mapToInt(Integer::parseInt).toArray();

Check notice

Code scanning / CodeQL

Missing catch of NumberFormatException Note

Potential uncaught 'java.lang.NumberFormatException'.
@jorg3lopez
Copy link
Contributor

Test task fails when I run ./gradlew :rs-e2e:test --info

image

@jorg3lopez
Copy link
Contributor

Test task fails when I run ./gradlew :rs-e2e:test --info

image

I think we need to see the mocking for the rulesloader or rule. This is what is probably causing the null pointer:

image

image

@basiliskus
Copy link
Contributor Author

@jorg3lopez I'm not able to replicate your error. We didn't make any changes to the assertion engine or its test in this PR. It's probably related to your local configuration. Can you try running the other the rule engine tests (ValidationRuleEngineTest and TransformationRuleEngineTest) and see if you get the same error?

@jorg3lopez
Copy link
Contributor

@jorg3lopez I'm not able to replicate your error. We didn't make any changes to the assertion engine or its test in this PR. It's probably related to your local configuration. Can you try running the other the rule engine tests (ValidationRuleEngineTest and TransformationRuleEngineTest) and see if you get the same error?

Per our conversation, this is a pre-existing bug that we will need to look into. This bug is present in the main repo, so ignore it for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants