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

Eliminate warnings for missing fields (fix unsigned commits) #1664

Merged
merged 18 commits into from
Dec 19, 2024

Conversation

jbiskie
Copy link
Contributor

@jbiskie jbiskie commented Dec 18, 2024

Description

This PR updates and/or removes unnecessary fields in our FHIR mappings that were causing warnings when sending messages.

Note: this is a replacement of PR #1656, which had unsigned commits in the branch.

Issue

#940

When an extension contains a URL but no value of any type, the message "Resource is missing required element: value" is printed in the log.

There were 3 "value-less" extensions remaining that were producing these warnings.

  1. Patient identifier type code CX-5 (PID-3.5 in the v2 message) - transform: RemovePatientIdentifiers
  2. Patient name type code XPN-7 (PID-5.7 in the v2 message) - transform: RemovePatientNameTypeCode
  3. Observation request OBR-16 (occurs only when ORC-12 is blank - transform: CopyOrcOrderProviderToObrOrderProvider

This PR ensures that the "value-less" extensions do not get added during the TI transforms and does some additional cleanup of the transform output to ensure that the values being cleared by the transforms no longer exist in other places in the affected resource.

See the Areas of Concern comment in the issue for more details.

Testing instructions

  • Run a sample CA message with empty ORC-12 and OBR-16 through RS and TI end to end (such as CA examples 7, 19, or 20). Review the transformed fhir and final HL7.
  • The transformed fhir should have no extensions without a value (specifically CX.5, XPN.7, and OBR.16)
  • There should be no change in the final HL7

Checklist

  • I have added tests to cover my changes

jbiskie and others added 17 commits December 18, 2024 12:51
…meter refactor, fix logic for existence of practitioner
Co-Authored-By: Joel Biskie <84460447+jbiskie@users.noreply.github.com>
Co-Authored-By: Jeremy Rosenfeld <10262289+JeremyIR@users.noreply.github.com>
Co-authored-by: tjohnson@flexion.us
Co-authored-by: jrosenfeld@flexion.us
Co-authored-by: tjohnson7021
Co-authored-by: jeremyir
…lue, refactor PID-3.4 testing into discrete unit tests
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Test Coverage
The PR introduces multiple new methods and logic paths, but it seems that not all new functionalities are covered by unit tests. It's crucial to ensure that all new code paths are tested to prevent future regressions.

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Prevent potential NullPointerException by checking if obrExtension is null before usage

Check if obrExtension is null before attempting to use it in the
setOBR16WithPractitioner method to avoid potential NullPointerException.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelper.java [561-564]

+if (obrExtension == null) {
+    return;
+}
 Extension obr16Extension =
         ensureSubExtensionExists(obrExtension, EXTENSION_OBR16_DATA_TYPE.toString());
 obr16Extension.setValue(practitionerRole.getPractitioner());
Suggestion importance[1-10]: 8

Why: This suggestion correctly identifies a potential NullPointerException risk in the method setOBR16WithPractitioner. Adding a null check before using obrExtension is crucial for robust error handling and preventing runtime exceptions.

8
Prevent NullPointerException by checking for null before calling toString() on an object

Add a null check for extensionValue in hasMatchingCodingExtension to prevent
NullPointerException when calling toString() on a potentially null object.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelper.java [814-815]

-var extensionValue = getCodingExtensionByUrl(coding, extensionUrl).getValue().toString();
+var extension = getCodingExtensionByUrl(coding, extensionUrl);
+var extensionValue = (extension != null && extension.getValue() != null) ? extension.getValue().toString() : null;
 return Objects.equals(valueToMatch, extensionValue);
Suggestion importance[1-10]: 8

Why: This suggestion addresses a potential NullPointerException by adding a null check before accessing the value of an extension and calling toString(). This is a critical fix as it ensures the code does not crash when the extension or its value is null.

8
Add null checks for objects returned by helper methods to prevent runtime exceptions

Ensure that the method HapiHelper.getObr16ExtensionPractitioner(serviceRequest)
returns a valid or expected Practitioner object before accessing its properties to
avoid potential NullPointerException.

etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/CopyOrcOrderProviderToObrOrderProviderTest.groovy [229-230]

 def practitioner = HapiHelper.getObr16ExtensionPractitioner(serviceRequest)
-def xcnExtension = practitioner.getExtensionByUrl(PRACTITIONER_EXTENSION_URL)
+if (practitioner) {
+    def xcnExtension = practitioner.getExtensionByUrl(PRACTITIONER_EXTENSION_URL)
+}
Suggestion importance[1-10]: 7

Why: Adding null checks before accessing properties of objects returned by helper methods is crucial to prevent runtime exceptions such as NullPointerException. This suggestion enhances the robustness of the code.

7
Implement null checks on objects derived from helper methods to ensure stability

Verify that the pid5Extension and patientName are not null before accessing their
properties to prevent NullPointerException.

etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/ruleengine/transformation/custom/RemovePatientNameTypeCodeTest.groovy [27-29]

 def pid5Extension = HapiHelper.getPID5Extension(bundle)
-def patientName = HapiHelper.getPIDPatient(bundle).getNameFirstRep()
-def patientNameUse = patientName.getUse()
+if (pid5Extension && HapiHelper.getPIDPatient(bundle)?.getNameFirstRep()) {
+    def patientName = HapiHelper.getPIDPatient(bundle).getNameFirstRep()
+    def patientNameUse = patientName.getUse()
+}
Suggestion importance[1-10]: 7

Why: The suggestion to add null checks before accessing properties of objects is valid and improves the code's stability by preventing potential NullPointerExceptions. This is a good practice, especially when dealing with external method calls that may return null.

7
Safeguard against ClassCastException by verifying the instance type before casting

Ensure that reference.getResource() returns an instance of Practitioner before
casting it in the getObr16ExtensionPractitioner method to avoid ClassCastException.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelper.java [581-584]

-if (obr16Extension.getValue() instanceof Reference reference
-        && reference.getResource() instanceof Practitioner practitioner) {
-    return practitioner;
+if (obr16Extension.getValue() instanceof Reference reference) {
+    Resource resource = reference.getResource();
+    if (resource instanceof Practitioner) {
+        return (Practitioner) resource;
+    }
 }
Suggestion importance[1-10]: 7

Why: The suggestion improves the type safety of the code by ensuring that the getResource() method returns a Practitioner instance before casting. This change prevents a possible ClassCastException, enhancing the method's reliability.

7
Add null checking for the bundle before setting PID3_4Value to prevent errors

Ensure that the HapiHelper.setPID3_4Value method handles the case where the bundle
is null to prevent potential NullPointerExceptions.

shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelperTest.groovy [448]

-HapiHelper.setPID3_4Value(bundle, pid3_4Value)
+if (bundle != null) {
+    HapiHelper.setPID3_4Value(bundle, pid3_4Value)
+}
Suggestion importance[1-10]: 6

Why: Adding null checks before setting values can prevent runtime exceptions such as NullPointerException, which enhances the robustness of the code.

6
Ensure the bundle is not null before getting PID3_4Value to avoid runtime exceptions

Verify that HapiFhirHelper.getPID3_4Value(bundle) is called on a non-null bundle to
avoid NullPointerException.

shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelperTest.groovy [451]

-HapiFhirHelper.getPID3_4Value(bundle) == null
+if (bundle != null) {
+    HapiFhirHelper.getPID3_4Value(bundle) == null
+}
Suggestion importance[1-10]: 6

Why: This suggestion correctly identifies a potential issue where a null bundle could lead to a NullPointerException, thus improving the code's safety.

6
Implement null checking for the bundle in the removePID3_5Value method to ensure stability

Add validation to ensure that bundle is not null in HapiHelper.removePID3_5Value to
prevent potential NullPointerExceptions.

shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelperTest.groovy [515]

-HapiHelper.removePID3_5Value(bundle)
+if (bundle != null) {
+    HapiHelper.removePID3_5Value(bundle)
+}
Suggestion importance[1-10]: 6

Why: Adding a null check before attempting to modify the bundle can prevent runtime errors, which is crucial for maintaining the stability of the application.

6
Ensure patientName is not null before modifying its properties to prevent errors

Validate that patientName is not null before setting its properties in
removePID5_7Value to avoid NullPointerException.

shared/src/main/java/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelper.java [349-350]

 HumanName patientName = patient.getNameFirstRep();
-patientName.setUse(null);
+if (patientName != null) {
+    patientName.setUse(null);
+}
Suggestion importance[1-10]: 6

Why: Adding a null check before setting properties on patientName in the removePID5_7Value method is a good practice to avoid NullPointerException. This suggestion enhances the safety and robustness of the method by ensuring that operations are only performed on non-null objects.

6
Add handling for null pid3_4Value to prevent unintended behavior or errors

Ensure that HapiHelper.setPID3_4Value correctly handles the scenario where
pid3_4Value is null, potentially by adding a check or handling within the method.

shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/external/hapi/HapiHelperTest.groovy [448]

-HapiHelper.setPID3_4Value(bundle, pid3_4Value)
+if (pid3_4Value != null) {
+    HapiHelper.setPID3_4Value(bundle, pid3_4Value)
+}
Suggestion importance[1-10]: 5

Why: Handling null values for pid3_4Value before setting it can prevent issues in scenarios where null is not expected, thus improving the method's reliability.

5

@jbiskie jbiskie requested a review from basiliskus December 18, 2024 21:17
@jbiskie jbiskie changed the title Bug/940 commit fix eliminate warnings for missing fields Eliminate warnings for missing fields (fix unsigned commits) Dec 19, 2024
@jbiskie jbiskie merged commit d257103 into main Dec 19, 2024
17 checks passed
@jbiskie jbiskie deleted the bug/940-commit-fix-eliminate-warnings-for-missing-fields branch December 19, 2024 15:01
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.

3 participants