diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6159-code-system-delta-add-or-remove-fail-without-concept-display.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6159-code-system-delta-add-or-remove-fail-without-concept-display.yaml new file mode 100644 index 00000000000..6356d7a86f7 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6159-code-system-delta-add-or-remove-fail-without-concept-display.yaml @@ -0,0 +1,8 @@ +--- +type: fix +issue: 6159 +jira: SMILE-8604 +title: "Previously, `$apply-codesystem-delta-add` and `$apply-codesystem-delta-remove` operations were failing +with a 500 Server Error when invoked with a CodeSystem Resource payload that had a concept without a +`display` element. This has now been fixed so that concepts without display field is accepted, as `display` +element is not required." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/TerminologyUploaderProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/TerminologyUploaderProvider.java index eda484f7cbe..1365917162b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/TerminologyUploaderProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/TerminologyUploaderProvider.java @@ -475,6 +475,9 @@ public class TerminologyUploaderProvider extends BaseJpaProvider { } private static String csvEscape(String theValue) { + if (theValue == null) { + return ""; + } return '"' + theValue.replace("\"", "\"\"").replace("\n", "\\n").replace("\r", "") + '"'; } } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/TerminologyUploaderProviderR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/TerminologyUploaderProviderR4Test.java index 3deec41ad94..99180105bde 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/TerminologyUploaderProviderR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/TerminologyUploaderProviderR4Test.java @@ -347,7 +347,7 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes "\"reference\": \"CodeSystem/" ); - assertHierarchyContains( + assertHierarchyContainsExactly( "CHEM seq=0", " HB seq=0", " NEUT seq=1", @@ -387,7 +387,7 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes "\"reference\": \"CodeSystem/" ); - assertHierarchyContains( + assertHierarchyContainsExactly( "CHEM seq=0", " HB seq=0", " NEUT seq=1", @@ -457,7 +457,7 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes "\"reference\": \"CodeSystem/" ); - assertHierarchyContains( + assertHierarchyContainsExactly( "1111222233 seq=0", " 1111222234 seq=0" ); @@ -527,13 +527,45 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes "\"reference\": \"CodeSystem/" ); - assertHierarchyContains( + assertHierarchyContainsExactly( "CHEM seq=0", " HB seq=0", " HBA seq=0" ); } + @Test + public void testApplyDeltaAdd_UsingCodeSystem_NoDisplaySetOnConcepts() throws IOException { + + CodeSystem codeSystem = new CodeSystem(); + codeSystem.setUrl("http://foo/cs"); + // setting codes are enough, no need to call setDisplay etc + codeSystem.addConcept().setCode("Code1"); + codeSystem.addConcept().setCode("Code2"); + + LoggingInterceptor interceptor = new LoggingInterceptor(true); + myClient.registerInterceptor(interceptor); + Parameters outcome = myClient + .operation() + .onType(CodeSystem.class) + .named(JpaConstants.OPERATION_APPLY_CODESYSTEM_DELTA_ADD) + .withParameter(Parameters.class, TerminologyUploaderProvider.PARAM_SYSTEM, new UriType("http://foo/cs")) + .andParameter(TerminologyUploaderProvider.PARAM_CODESYSTEM, codeSystem) + .prettyPrint() + .execute(); + myClient.unregisterInterceptor(interceptor); + + String encoded = myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome); + ourLog.info(encoded); + assertThat(encoded).contains("\"valueInteger\": 2"); + + // assert other codes remain, and HB and NEUT is removed + assertHierarchyContainsExactly( + "Code1 seq=0", + "Code2 seq=0" + ); + } + @Test public void testApplyDeltaAdd_MissingSystem() throws IOException { String conceptsCsv = loadResource("/custom_term/concepts.csv"); @@ -582,30 +614,12 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes } @Test - public void testApplyDeltaRemove() throws IOException { - String conceptsCsv = loadResource("/custom_term/concepts.csv"); - Attachment conceptsAttachment = new Attachment() - .setData(conceptsCsv.getBytes(Charsets.UTF_8)) - .setContentType("text/csv") - .setUrl("file:/foo/concepts.csv"); - String hierarchyCsv = loadResource("/custom_term/hierarchy.csv"); - Attachment hierarchyAttachment = new Attachment() - .setData(hierarchyCsv.getBytes(Charsets.UTF_8)) - .setContentType("text/csv") - .setUrl("file:/foo/hierarchy.csv"); + public void testApplyDeltaRemove_UsingCsvFiles_RemoveAllCodes() throws IOException { // Add the codes - myClient - .operation() - .onType(CodeSystem.class) - .named(JpaConstants.OPERATION_APPLY_CODESYSTEM_DELTA_ADD) - .withParameter(Parameters.class, TerminologyUploaderProvider.PARAM_SYSTEM, new UriType("http://foo/cs")) - .andParameter(TerminologyUploaderProvider.PARAM_FILE, conceptsAttachment) - .andParameter(TerminologyUploaderProvider.PARAM_FILE, hierarchyAttachment) - .prettyPrint() - .execute(); + applyDeltaAddCustomTermCodes(); - // And remove them + // And remove all of them using the same set of csv files LoggingInterceptor interceptor = new LoggingInterceptor(true); myClient.registerInterceptor(interceptor); Parameters outcome = myClient @@ -613,8 +627,8 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes .onType(CodeSystem.class) .named(JpaConstants.OPERATION_APPLY_CODESYSTEM_DELTA_REMOVE) .withParameter(Parameters.class, TerminologyUploaderProvider.PARAM_SYSTEM, new UriType("http://foo/cs")) - .andParameter(TerminologyUploaderProvider.PARAM_FILE, conceptsAttachment) - .andParameter(TerminologyUploaderProvider.PARAM_FILE, hierarchyAttachment) + .andParameter(TerminologyUploaderProvider.PARAM_FILE, getCustomTermConceptsAttachment()) + .andParameter(TerminologyUploaderProvider.PARAM_FILE, getCustomTermHierarchyAttachment()) .prettyPrint() .execute(); myClient.unregisterInterceptor(interceptor); @@ -622,8 +636,129 @@ public class TerminologyUploaderProviderR4Test extends BaseResourceProviderR4Tes String encoded = myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome); ourLog.info(encoded); assertThat(encoded).contains("\"valueInteger\": 5"); + + + // providing no arguments, since there should be no code left + assertHierarchyContainsExactly(); } + @Test + public void testApplyDeltaRemove_UsingConceptsCsvFileOnly() throws IOException { + + //add some concepts + applyDeltaAddCustomTermCodes(); + + // And remove 2 of them, providing values for DISPLAY is not necessary + String conceptsToRemoveCsvData = """ + CODE,DISPLAY + HB, + NEUT, + """; + + Attachment conceptsAttachment = createCsvAttachment(conceptsToRemoveCsvData, "file:/concepts.csv"); + + LoggingInterceptor interceptor = new LoggingInterceptor(true); + myClient.registerInterceptor(interceptor); + Parameters outcome = myClient + .operation() + .onType(CodeSystem.class) + .named(JpaConstants.OPERATION_APPLY_CODESYSTEM_DELTA_REMOVE) + .withParameter(Parameters.class, TerminologyUploaderProvider.PARAM_SYSTEM, new UriType("http://foo/cs")) + // submitting concepts is enough (no need to submit hierarchy) + .andParameter(TerminologyUploaderProvider.PARAM_FILE, conceptsAttachment) + .prettyPrint() + .execute(); + myClient.unregisterInterceptor(interceptor); + + String encoded = myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome); + ourLog.info(encoded); + assertThat(encoded).contains("\"valueInteger\": 2"); + + // assert other codes remain, and HB and NEUT is removed + assertHierarchyContainsExactly( + "CHEM seq=0", + "MICRO seq=0", + " C&S seq=0" + ); + } + + @Test + public void testApplyDeltaRemove_UsingCodeSystemPayload() throws IOException { + + // add some custom codes + applyDeltaAddCustomTermCodes(); + + + // remove 2 of them using CodeSystemPayload + CodeSystem codeSystem = new CodeSystem(); + codeSystem.setUrl("http://foo/cs"); + // setting codes are enough for remove, no need to call setDisplay etc + codeSystem.addConcept().setCode("HB"); + codeSystem.addConcept().setCode("NEUT"); + + LoggingInterceptor interceptor = new LoggingInterceptor(true); + myClient.registerInterceptor(interceptor); + Parameters outcome = myClient + .operation() + .onType(CodeSystem.class) + .named(JpaConstants.OPERATION_APPLY_CODESYSTEM_DELTA_REMOVE) + .withParameter(Parameters.class, TerminologyUploaderProvider.PARAM_SYSTEM, new UriType("http://foo/cs")) + .andParameter(TerminologyUploaderProvider.PARAM_CODESYSTEM, codeSystem) + .prettyPrint() + .execute(); + myClient.unregisterInterceptor(interceptor); + + String encoded = myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome); + ourLog.info(encoded); + assertThat(encoded).contains("\"valueInteger\": 2"); + + // assert other codes remain, and HB and NEUT is removed + assertHierarchyContainsExactly( + "CHEM seq=0", + "MICRO seq=0", + " C&S seq=0" + ); + } + + + private Attachment createCsvAttachment(String theData, String theUrl) { + return new Attachment() + .setData(theData.getBytes(Charsets.UTF_8)) + .setContentType("text/csv") + .setUrl(theUrl); + } + + private Attachment getCustomTermConceptsAttachment() throws IOException { + String conceptsCsv = loadResource("/custom_term/concepts.csv"); + return createCsvAttachment(conceptsCsv, "file:/foo/concepts.csv"); + } + + private Attachment getCustomTermHierarchyAttachment() throws IOException { + String hierarchyCsv = loadResource("/custom_term/hierarchy.csv"); + return createCsvAttachment(hierarchyCsv, "file:/foo/hierarchy.csv"); + } + + private void applyDeltaAddCustomTermCodes() throws IOException { + myClient + .operation() + .onType(CodeSystem.class) + .named(JpaConstants.OPERATION_APPLY_CODESYSTEM_DELTA_ADD) + .withParameter(Parameters.class, TerminologyUploaderProvider.PARAM_SYSTEM, new UriType("http://foo/cs")) + .andParameter(TerminologyUploaderProvider.PARAM_FILE, getCustomTermConceptsAttachment()) + .andParameter(TerminologyUploaderProvider.PARAM_FILE, getCustomTermHierarchyAttachment()) + .prettyPrint() + .execute(); + + assertHierarchyContainsExactly( + "CHEM seq=0", + " HB seq=0", + " NEUT seq=1", + "MICRO seq=0", + " C&S seq=0" + ); + + + } private static void addFile(ZipOutputStream theZos, String theFileName) throws IOException { theZos.putNextEntry(new ZipEntry(theFileName)); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java index 09eeadb6796..ccb9ce4b962 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java @@ -61,7 +61,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { delta.addRootConcept("RootA", "Root A"); delta.addRootConcept("RootB", "Root B"); myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", "RootB seq=0" ); @@ -70,7 +70,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { delta.addRootConcept("RootC", "Root C"); delta.addRootConcept("RootD", "Root D"); myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", "RootB seq=0", "RootC seq=0", @@ -104,7 +104,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { ourLog.info("Starting testAddHierarchyConcepts"); createNotPresentCodeSystem(); - assertHierarchyContains(); + assertHierarchyContainsExactly(); ourLog.info("Have created code system"); runInTransaction(() -> { @@ -117,7 +117,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { delta.addRootConcept("RootA", "Root A"); delta.addRootConcept("RootB", "Root B"); myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", "RootB seq=0" ); @@ -139,7 +139,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { myCaptureQueriesListener.logAllQueriesForCurrentThread(); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", " ChildAA seq=0", " ChildAB seq=1", @@ -151,7 +151,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { @Test public void testAddMoveConceptFromOneParentToAnother() { createNotPresentCodeSystem(); - assertHierarchyContains(); + assertHierarchyContainsExactly(); UploadStatistics outcome; CustomTerminologySet delta; @@ -162,7 +162,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAAA").setDisplay("Child AAA"); delta.addRootConcept("RootB", "Root B"); outcome = myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", " ChildAA seq=0", " ChildAAA seq=0", @@ -174,7 +174,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { delta.addRootConcept("RootB", "Root B") .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAA").setDisplay("Child AA"); outcome = myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", " ChildAA seq=0", " ChildAAA seq=0", @@ -195,7 +195,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { @Test public void testReAddingConceptsDoesntRecreateExistingLinks() { createNotPresentCodeSystem(); - assertHierarchyContains(); + assertHierarchyContainsExactly(); UploadStatistics outcome; CustomTerminologySet delta; @@ -206,7 +206,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { delta.addRootConcept("RootA", "Root A") .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAA").setDisplay("Child AA"); myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", " ChildAA seq=0" ); @@ -223,7 +223,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAA").setDisplay("Child AA") .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAAA").setDisplay("Child AAA"); myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", " ChildAA seq=0", " ChildAAA seq=0" @@ -242,7 +242,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAAA").setDisplay("Child AAA") .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAAAA").setDisplay("Child AAAA"); myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertHierarchyContains( + assertHierarchyContainsExactly( "RootA seq=0", " ChildAA seq=0", " ChildAAA seq=0", @@ -293,7 +293,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo", set); // Check so far - assertHierarchyContains( + assertHierarchyContainsExactly( "ParentA seq=0", " ChildA seq=0" ); @@ -306,7 +306,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo", set); // Check so far - assertHierarchyContains( + assertHierarchyContainsExactly( "ParentA seq=0", " ChildA seq=0", " ChildAA seq=0" @@ -331,7 +331,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo", set); // Check so far - assertHierarchyContains( + assertHierarchyContainsExactly( "ParentA seq=0", " ChildA seq=0" ); @@ -344,7 +344,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo", set); // Check so far - assertHierarchyContains( + assertHierarchyContainsExactly( "ParentA seq=0", " ChildA seq=0", " ChildAA seq=0" @@ -416,7 +416,7 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { expectedHierarchy.add(expected); } - assertHierarchyContains(expectedHierarchy.toArray(new String[0])); + assertHierarchyContainsExactly(expectedHierarchy.toArray(new String[0])); } diff --git a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaR4Test.java b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaR4Test.java index 9d535aa67c5..2fb5a9b87f6 100644 --- a/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaR4Test.java +++ b/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/BaseJpaR4Test.java @@ -740,7 +740,7 @@ public abstract class BaseJpaR4Test extends BaseJpaTest implements ITestDataBuil dao.update(resourceParsed); } - protected void assertHierarchyContains(String... theStrings) { + protected void assertHierarchyContainsExactly(String... theStrings) { List hierarchy = runInTransaction(() -> { List hierarchyHolder = new ArrayList<>(); TermCodeSystem codeSystem = myTermCodeSystemDao.findAll().iterator().next();