From 1a34f32fccfb49d5e9300a4290cc4ebc6154aea0 Mon Sep 17 00:00:00 2001 From: Basilio Bogado <541149+basiliskus@users.noreply.github.com> Date: Tue, 3 Dec 2024 13:42:35 -0800 Subject: [PATCH] Refactor `HapiHL7Message.getName` to return more useful identifier (#1636) * WIP: Refactored to avoid leaking external dependency + some cleanup * Renamed HapiHL7Message.getName => getIdentifier * Fixed failing tests * Fixed flipped parameters * Missed updating Message => HapiHL7Message in HapiHL7FileMatcherTest * Cleanup --- .../external/hapi/HapiHL7FileMatcher.java | 17 ++++---- .../rse2e/external/hapi/HapiHL7Message.java | 11 +++++- .../rse2e/ruleengine/AssertionRule.java | 2 +- .../rse2e/ruleengine/AssertionRuleEngine.java | 12 ++---- .../rse2e/AutomatedTest.groovy | 5 +-- .../hapi/HapiHL7FileMatcherTest.groovy | 39 ++++++++++--------- .../external/hapi/HapiHL7MessageTest.groovy | 19 +++++---- .../ruleengine/AssertionRuleEngineTest.groovy | 10 ++--- .../wrappers/HealthData.java | 2 +- .../wrappers/HealthDataTest.groovy | 2 +- 10 files changed, 62 insertions(+), 57 deletions(-) diff --git a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcher.java b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcher.java index 4ab050ef2..ca9ca419f 100644 --- a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcher.java +++ b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcher.java @@ -4,7 +4,6 @@ import ca.uhn.hl7v2.HL7Exception; import ca.uhn.hl7v2.HapiContext; import ca.uhn.hl7v2.model.Message; -import ca.uhn.hl7v2.model.v251.segment.MSH; import ca.uhn.hl7v2.parser.Parser; import com.google.common.collect.Sets; import gov.hhs.cdc.trustedintermediary.rse2e.HL7FileStream; @@ -32,13 +31,13 @@ public static HapiHL7FileMatcher getInstance() { return INSTANCE; } - public Map matchFiles( + public Map matchFiles( List outputFiles, List inputFiles) throws HapiHL7FileMatcherException { // We pair up output and input files based on the control ID, which is in MSH-10 // Any files (either input or output) that don't have a match are logged - Map inputMap = mapMessageByControlId(inputFiles); - Map outputMap = mapMessageByControlId(outputFiles); + Map inputMap = parseAndMapMessageByControlId(inputFiles); + Map outputMap = parseAndMapMessageByControlId(outputFiles); Set inputKeys = inputMap.keySet(); Set outputKeys = outputMap.keySet(); @@ -55,10 +54,10 @@ public Map matchFiles( return inputKeys.stream().collect(Collectors.toMap(inputMap::get, outputMap::get)); } - public Map mapMessageByControlId(List files) + public Map parseAndMapMessageByControlId(List files) throws HapiHL7FileMatcherException { - Map messageMap = new HashMap<>(); + Map messageMap = new HashMap<>(); try (HapiContext context = new DefaultHapiContext()) { Parser parser = context.getPipeParser(); @@ -68,13 +67,13 @@ public Map mapMessageByControlId(List files) try (InputStream inputStream = hl7FileStream.inputStream()) { String content = new String(inputStream.readAllBytes(), StandardCharsets.UTF_8); Message message = parser.parse(content); - MSH mshSegment = (MSH) message.get("MSH"); - String msh10 = mshSegment.getMessageControlID().getValue(); + HapiHL7Message hapiHL7Message = new HapiHL7Message(message); + String msh10 = hapiHL7Message.getIdentifier(); if (msh10 == null || msh10.isEmpty()) { throw new HapiHL7FileMatcherException( String.format("MSH-10 is empty for file: %s", fileName)); } - messageMap.put(msh10, message); + messageMap.put(msh10, hapiHL7Message); } catch (HL7Exception e) { throw new HapiHL7FileMatcherException( String.format("Failed to parse HL7 message from file: %s", fileName), diff --git a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7Message.java b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7Message.java index 81da8afff..2ff732add 100644 --- a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7Message.java +++ b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7Message.java @@ -1,6 +1,8 @@ package gov.hhs.cdc.trustedintermediary.rse2e.external.hapi; +import ca.uhn.hl7v2.HL7Exception; import ca.uhn.hl7v2.model.Message; +import ca.uhn.hl7v2.model.v251.segment.MSH; import gov.hhs.cdc.trustedintermediary.wrappers.HealthData; /** @@ -21,7 +23,12 @@ public Message getUnderlyingData() { } @Override - public String getName() { - return underlyingData.getName(); + public String getIdentifier() { + try { + MSH mshSegment = (MSH) underlyingData.get("MSH"); + return mshSegment.getMessageControlID().getValue(); + } catch (HL7Exception e) { + return null; + } } } diff --git a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRule.java b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRule.java index c53b0729a..c26828ac4 100644 --- a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRule.java +++ b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRule.java @@ -46,7 +46,7 @@ public final void runRule(HealthData... data) { + "': " + assertion + " (" - + outputData.getName() + + outputData.getIdentifier() + ")"); } } catch (Exception e) { diff --git a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java index cf5a41178..76774bc39 100644 --- a/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java +++ b/rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java @@ -1,9 +1,8 @@ package gov.hhs.cdc.trustedintermediary.rse2e.ruleengine; -import ca.uhn.hl7v2.model.Message; -import gov.hhs.cdc.trustedintermediary.rse2e.external.hapi.HapiHL7Message; import gov.hhs.cdc.trustedintermediary.ruleengine.RuleLoader; import gov.hhs.cdc.trustedintermediary.ruleengine.RuleLoaderException; +import gov.hhs.cdc.trustedintermediary.wrappers.HealthData; import gov.hhs.cdc.trustedintermediary.wrappers.Logger; import gov.hhs.cdc.trustedintermediary.wrappers.formatter.TypeReference; import java.io.IOException; @@ -64,7 +63,7 @@ public List getRules() { return assertionRules; } - public Set runRules(Message outputMessage, Message inputMessage) { + public Set runRules(HealthData outputMessage, HealthData inputMessage) { try { ensureRulesLoaded(); } catch (RuleLoaderException e) { @@ -72,13 +71,10 @@ public Set runRules(Message outputMessage, Message inputMessage) return Set.of(); } - HapiHL7Message outputHapiMessage = new HapiHL7Message(outputMessage); - HapiHL7Message inputHapiMessage = new HapiHL7Message(inputMessage); - Set runRules = new HashSet<>(); for (AssertionRule rule : assertionRules) { - if (rule.shouldRun(outputHapiMessage)) { - rule.runRule(outputHapiMessage, inputHapiMessage); + if (rule.shouldRun(outputMessage)) { + rule.runRule(outputMessage, inputMessage); runRules.add(rule); } } diff --git a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/AutomatedTest.groovy b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/AutomatedTest.groovy index ea7257832..c1a4d45ab 100644 --- a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/AutomatedTest.groovy +++ b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/AutomatedTest.groovy @@ -1,6 +1,5 @@ package gov.hhs.cdc.trustedintermediary.rse2e -import ca.uhn.hl7v2.model.Message import gov.hhs.cdc.trustedintermediary.context.TestApplicationContext import gov.hhs.cdc.trustedintermediary.rse2e.external.hapi.HapiHL7FileMatcher import gov.hhs.cdc.trustedintermediary.rse2e.external.hapi.HapiHL7ExpressionEvaluator @@ -65,8 +64,8 @@ class AutomatedTest extends Specification { when: for (messagePair in matchedFiles) { - Message inputMessage = messagePair.getKey() as Message - Message outputMessage = messagePair.getValue() as Message + def inputMessage = messagePair.getKey() + def outputMessage = messagePair.getValue() def evaluatedRules = engine.runRules(outputMessage, inputMessage) rulesToEvaluate.removeAll(evaluatedRules) } diff --git a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcherTest.groovy b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcherTest.groovy index 8e0f98c20..c00789a4d 100644 --- a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcherTest.groovy +++ b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7FileMatcherTest.groovy @@ -1,6 +1,5 @@ package gov.hhs.cdc.trustedintermediary.rse2e.external.hapi -import ca.uhn.hl7v2.model.Message import gov.hhs.cdc.trustedintermediary.context.TestApplicationContext import gov.hhs.cdc.trustedintermediary.rse2e.HL7FileStream import gov.hhs.cdc.trustedintermediary.wrappers.Logger @@ -31,15 +30,15 @@ class HapiHL7FileMatcherTest extends Specification { new HL7FileStream("outputFileStream1", Mock(InputStream)), new HL7FileStream("outputFileStream2", Mock(InputStream)) ] - def mockInputMessage1 = Mock(Message) - def mockInputMessage2 = Mock(Message) - def mockOutputMessage1 = Mock(Message) - def mockOutputMessage2 = Mock(Message) - spyFileMatcher.mapMessageByControlId(mockInputFiles) >> [ "001": mockInputMessage1, "002": mockInputMessage2 ] - spyFileMatcher.mapMessageByControlId(mockOutputFiles) >> [ "001": mockOutputMessage1, "002": mockOutputMessage2 ] + def mockInputMessage1 = Mock(HapiHL7Message) + def mockInputMessage2 = Mock(HapiHL7Message) + def mockOutputMessage1 = Mock(HapiHL7Message) + def mockOutputMessage2 = Mock(HapiHL7Message) + spyFileMatcher.parseAndMapMessageByControlId(mockInputFiles) >> ["001": mockInputMessage1, "002": mockInputMessage2 ] + spyFileMatcher.parseAndMapMessageByControlId(mockOutputFiles) >> ["001": mockOutputMessage1, "002": mockOutputMessage2 ] when: - def result =spyFileMatcher.matchFiles(mockOutputFiles, mockInputFiles) + def result = spyFileMatcher.matchFiles(mockOutputFiles, mockInputFiles) then: result.size() == 2 @@ -61,8 +60,8 @@ class HapiHL7FileMatcherTest extends Specification { mockOutputFiles = [ new HL7FileStream("matchingOutputFileStream", Mock(InputStream)) ] - spyFileMatcher.mapMessageByControlId(mockInputFiles) >> [ "001": Mock(Message), "002": Mock(Message) ] - spyFileMatcher.mapMessageByControlId(mockOutputFiles) >> [ "001": Mock(Message) ] + spyFileMatcher.parseAndMapMessageByControlId(mockInputFiles) >> ["001": Mock(HapiHL7Message), "002": Mock(HapiHL7Message) ] + spyFileMatcher.parseAndMapMessageByControlId(mockOutputFiles) >> ["001": Mock(HapiHL7Message) ] spyFileMatcher.matchFiles(mockOutputFiles, mockInputFiles) then: @@ -80,8 +79,8 @@ class HapiHL7FileMatcherTest extends Specification { new HL7FileStream("matchingOutputFileStream", Mock(InputStream)), new HL7FileStream("nonMatchingOutputFileStream", Mock(InputStream)) ] - spyFileMatcher.mapMessageByControlId(mockInputFiles) >> [ "001": Mock(Message) ] - spyFileMatcher.mapMessageByControlId(mockOutputFiles) >> [ "001": Mock(Message), "003": Mock(Message) ] + spyFileMatcher.parseAndMapMessageByControlId(mockInputFiles) >> ["001": Mock(HapiHL7Message) ] + spyFileMatcher.parseAndMapMessageByControlId(mockOutputFiles) >> ["001": Mock(HapiHL7Message), "003": Mock(HapiHL7Message) ] spyFileMatcher.matchFiles(mockOutputFiles, mockInputFiles) then: @@ -110,14 +109,16 @@ class HapiHL7FileMatcherTest extends Specification { ] when: - def result = fileMatcher.mapMessageByControlId(mockFiles) + def result = fileMatcher.parseAndMapMessageByControlId(mockFiles) then: result.size() == 2 - result[file1Msh10] != null - file1MshSegment == result[file1Msh10].encode().trim() - result[file2Msh10] != null - file2MshSegment == result[file2Msh10].encode().trim() + def message1 = result[file1Msh10] + def message2 = result[file2Msh10] + message1 != null + message2 != null + file1MshSegment == message1.getUnderlyingData().encode().trim() + file2MshSegment == message2.getUnderlyingData().encode().trim() } def "should throw HapiHL7FileMatcherException when MSH-10 is empty"() { @@ -130,7 +131,7 @@ class HapiHL7FileMatcherTest extends Specification { def hl7FileStream = new HL7FileStream("file1", inputStream) when: - fileMatcher.mapMessageByControlId([hl7FileStream]) + fileMatcher.parseAndMapMessageByControlId([hl7FileStream]) then: thrown(HapiHL7FileMatcherException) @@ -142,7 +143,7 @@ class HapiHL7FileMatcherTest extends Specification { def hl7FileStream = new HL7FileStream("badFile", inputStream) when: - fileMatcher.mapMessageByControlId([hl7FileStream]) + fileMatcher.parseAndMapMessageByControlId([hl7FileStream]) then: thrown(HapiHL7FileMatcherException) diff --git a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7MessageTest.groovy b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7MessageTest.groovy index 18fb3cd1a..c7df2dd72 100644 --- a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7MessageTest.groovy +++ b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/external/hapi/HapiHL7MessageTest.groovy @@ -1,11 +1,13 @@ package gov.hhs.cdc.trustedintermediary.rse2e.external.hapi import ca.uhn.hl7v2.model.Message +import ca.uhn.hl7v2.model.v251.message.ORU_R01 +import ca.uhn.hl7v2.model.v251.segment.MSH import spock.lang.Specification class HapiHL7MessageTest extends Specification { - def "should correctly initialize and return underlying data"() { + def "getUnderlyingData should correctly initialize and return underlying data"() { given: def mockMessage = Mock(Message) def hl7Message = new HapiHL7Message(mockMessage) @@ -14,19 +16,20 @@ class HapiHL7MessageTest extends Specification { hl7Message.getUnderlyingData() == mockMessage } - def "should return the name of the underlying message"() { + def "getIdentifier should return the MSH-10 identifier of the underlying message"() { given: - def expectedName = "TestMessage" - def message = Mock(Message) - message.getName() >> expectedName + def expectedIdentifier = "0001" + def oruMessage = new ORU_R01() + MSH mshSegment = oruMessage.getMSH() + mshSegment.getMessageControlID().setValue(expectedIdentifier) and: - def hl7Message = new HapiHL7Message(message) + def hl7Message = new HapiHL7Message(oruMessage) when: - def name = hl7Message.getName() + def name = hl7Message.getIdentifier() then: - name == expectedName + name == expectedIdentifier } } diff --git a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngineTest.groovy b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngineTest.groovy index c7582c80e..9869d7634 100644 --- a/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngineTest.groovy +++ b/rs-e2e/src/test/groovy/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngineTest.groovy @@ -85,7 +85,7 @@ class AssertionRuleEngineTest extends Specification { } when: - ruleEngine.runRules(Mock(Message), Mock(Message)) + ruleEngine.runRules(Mock(HapiHL7Message), Mock(HapiHL7Message)) then: 1 * mockLogger.logError(_ as String, exception) @@ -97,7 +97,7 @@ class AssertionRuleEngineTest extends Specification { mockRuleLoader.loadRules(_ as InputStream, _ as TypeReference) >> { throw exception } when: - ruleEngine.runRules(Mock(Message), Mock(Message)) + ruleEngine.runRules(Mock(HapiHL7Message), Mock(HapiHL7Message)) then: 1 * mockLogger.logError(_ as String, exception) @@ -105,7 +105,7 @@ class AssertionRuleEngineTest extends Specification { def "runRules returns nothing when there are no rules"() { when: - def result = ruleEngine.runRules(Mock(Message), Mock(Message)) + def result = ruleEngine.runRules(Mock(HapiHL7Message), Mock(HapiHL7Message)) then: result.isEmpty() @@ -117,7 +117,7 @@ class AssertionRuleEngineTest extends Specification { mockRuleLoader.loadRules(_ as InputStream, _ as TypeReference) >> [rule] when: - def result = ruleEngine.runRules(Mock(Message), Mock(Message)) + def result = ruleEngine.runRules(Mock(HapiHL7Message), Mock(HapiHL7Message)) then: result.size() == 1 @@ -130,7 +130,7 @@ class AssertionRuleEngineTest extends Specification { mockRuleLoader.loadRules(_ as InputStream, _ as TypeReference) >> [rule] when: - def result = ruleEngine.runRules(Mock(Message), Mock(Message)) + def result = ruleEngine.runRules(Mock(HapiHL7Message), Mock(HapiHL7Message)) then: result.isEmpty() diff --git a/shared/src/main/java/gov/hhs/cdc/trustedintermediary/wrappers/HealthData.java b/shared/src/main/java/gov/hhs/cdc/trustedintermediary/wrappers/HealthData.java index 5273da407..70a0bf7ea 100644 --- a/shared/src/main/java/gov/hhs/cdc/trustedintermediary/wrappers/HealthData.java +++ b/shared/src/main/java/gov/hhs/cdc/trustedintermediary/wrappers/HealthData.java @@ -6,7 +6,7 @@ public interface HealthData { T getUnderlyingData(); - default String getName() { + default String getIdentifier() { return ""; } } diff --git a/shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/wrappers/HealthDataTest.groovy b/shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/wrappers/HealthDataTest.groovy index 7e8da084d..49eb54759 100644 --- a/shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/wrappers/HealthDataTest.groovy +++ b/shared/src/test/groovy/gov/hhs/cdc/trustedintermediary/wrappers/HealthDataTest.groovy @@ -8,7 +8,7 @@ class HealthDataTest extends Specification { def healthData = new IntegerHealthData(1) when: - def actual = healthData.getName() + def actual = healthData.getIdentifier() then: actual == ""