From a05c4ce9ebd86828cbd4c4eafe78da8272d777c0 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 19 Feb 2020 17:46:54 -0500 Subject: [PATCH] Don't thrash delta links (#1721) * Don't thrash delta links * Add changelog * Remove commented out code --- .../4_3_0/1721-dont-thrash-delta-links.yaml | 6 + .../term/TermCodeSystemStorageSvcImpl.java | 48 +++----- .../jpa/term/TerminologySvcDeltaR4Test.java | 116 ++++++++++-------- 3 files changed, 94 insertions(+), 76 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1721-dont-thrash-delta-links.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1721-dont-thrash-delta-links.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1721-dont-thrash-delta-links.yaml new file mode 100644 index 00000000000..7099817d526 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1721-dont-thrash-delta-links.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 1721 +title: When performing a terminology delta ADD operation, existing parent-child links were often + deleted and recrreated needlessly during operations, which could result in a deadlock. This has + been resolved. diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcImpl.java index 43648e3e99c..3b36c1a97ca 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/term/TermCodeSystemStorageSvcImpl.java @@ -451,48 +451,40 @@ public class TermCodeSystemStorageSvcImpl implements ITermCodeSystemStorageSvc { String nextCodeToAdd = conceptToAdd.getCode(); String parentDescription = "(root concept)"; - Set parentConcepts = new HashSet<>(); - - if (!theParentCodes.isEmpty()) { - parentDescription = "[" + String.join(", ", theParentCodes) + "]"; - for (String nextParentCode : theParentCodes) { - Optional nextParentOpt = myConceptDao.findByCodeSystemAndCode(theCsv, nextParentCode); - if (nextParentOpt.isPresent() == false) { - throw new InvalidRequestException("Unable to add code \"" + nextCodeToAdd + "\" to unknown parent: " + nextParentCode); - } - parentConcepts.add(nextParentOpt.get()); - } - } ourLog.info("Saving concept {} with parent {}", theStatisticsTracker.getUpdatedConceptCount(), parentDescription); Optional existingCodeOpt = myConceptDao.findByCodeSystemAndCode(theCsv, nextCodeToAdd); + List existingParentLinks; if (existingCodeOpt.isPresent()) { TermConcept existingCode = existingCodeOpt.get(); existingCode.setIndexStatus(null); existingCode.setDisplay(conceptToAdd.getDisplay()); conceptToAdd = existingCode; + existingParentLinks = conceptToAdd.getParents(); + } else { + existingParentLinks = Collections.emptyList(); + } + + Set parentConceptsWeShouldLinkTo = new HashSet<>(); + for (String nextParentCode : theParentCodes) { + + // Don't add parent links that already exist for the code + if (existingParentLinks.stream().anyMatch(t->t.getParent().getCode().equals(nextParentCode))) { + continue; + } + + Optional nextParentOpt = myConceptDao.findByCodeSystemAndCode(theCsv, nextParentCode); + if (nextParentOpt.isPresent() == false) { + throw new InvalidRequestException("Unable to add code \"" + nextCodeToAdd + "\" to unknown parent: " + nextParentCode); + } + parentConceptsWeShouldLinkTo.add(nextParentOpt.get()); } if (conceptToAdd.getSequence() == null) { conceptToAdd.setSequence(theSequence); } - // Drop any old parent-child links if they aren't explicitly specified in the - // hierarchy being added - if (!theRootConcept) { - for (Iterator iter = conceptToAdd.getParents().iterator(); iter.hasNext(); ) { - TermConceptParentChildLink nextLink = iter.next(); - String parentCode = nextLink.getParent().getCode(); - ourLog.info("Dropping existing parent/child link from {} -> {}", parentCode, nextCodeToAdd); - myConceptParentChildLinkDao.delete(nextLink); - iter.remove(); - - List parentChildrenList = nextLink.getParent().getChildren(); - parentChildrenList.remove(nextLink); - } - } - // Null out the hierarchy PIDs for this concept always. We do this because we're going to // force a reindex, and it'll be regenerated then conceptToAdd.setParentPids(null); @@ -504,7 +496,7 @@ public class TermCodeSystemStorageSvcImpl implements ITermCodeSystemStorageSvc { theStatisticsTracker.incrementUpdatedConceptCount(); // Add link to new child to the parent - for (TermConcept nextParentConcept : parentConcepts) { + for (TermConcept nextParentConcept : parentConceptsWeShouldLinkTo) { TermConceptParentChildLink parentLink = new TermConceptParentChildLink(); parentLink.setParent(nextParentConcept); parentLink.setChild(conceptToAdd); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java index 496296a12ab..0d0dd62fe54 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologySvcDeltaR4Test.java @@ -156,6 +156,8 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { outcome = myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); assertHierarchyContains( "RootA seq=0", + " ChildAA seq=0", + " ChildAAA seq=0", "RootB seq=0", " ChildAA seq=0", " ChildAAA seq=0" @@ -164,6 +166,72 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { } + @Test + public void testReAddingConceptsDoesntRecreateExistingLinks() { + createNotPresentCodeSystem(); + assertHierarchyContains(); + + UploadStatistics outcome; + CustomTerminologySet delta; + + myCaptureQueriesListener.clear(); + + delta = new CustomTerminologySet(); + delta.addRootConcept("RootA", "Root A") + .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAA").setDisplay("Child AA"); + myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); + assertHierarchyContains( + "RootA seq=0", + " ChildAA seq=0" + ); + + myCaptureQueriesListener.logDeleteQueries(); + assertEquals(0, myCaptureQueriesListener.countDeleteQueries()); + myCaptureQueriesListener.logInsertQueries(); + // 2 concepts, 1 link + assertEquals(3, myCaptureQueriesListener.countInsertQueries()); + myCaptureQueriesListener.clear(); + + delta = new CustomTerminologySet(); + delta.addRootConcept("RootA", "Root A") + .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( + "RootA seq=0", + " ChildAA seq=0", + " ChildAAA seq=0" + ); + + myCaptureQueriesListener.logDeleteQueries(); + assertEquals(0, myCaptureQueriesListener.countDeleteQueries()); + myCaptureQueriesListener.logInsertQueries(); + // 1 concept, 1 link + assertEquals(2, myCaptureQueriesListener.countInsertQueries()); + myCaptureQueriesListener.clear(); + + delta = new CustomTerminologySet(); + delta.addRootConcept("RootA", "Root A") + .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("ChildAA").setDisplay("Child AA") + .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( + "RootA seq=0", + " ChildAA seq=0", + " ChildAAA seq=0", + " ChildAAAA seq=0" + ); + + myCaptureQueriesListener.logDeleteQueries(); + assertEquals(0, myCaptureQueriesListener.countDeleteQueries()); + myCaptureQueriesListener.logInsertQueries(); + // 1 concept, 1 link + assertEquals(2, myCaptureQueriesListener.countInsertQueries()); + myCaptureQueriesListener.clear(); + + } + @Test public void testAddNotPermittedForNonExternalCodeSystem() { @@ -307,54 +375,6 @@ public class TerminologySvcDeltaR4Test extends BaseJpaR4Test { assertEquals("CODEA1", myTermSvc.lookupCode(myFhirCtx, "http://foo", "codea").getCodeDisplay()); } - @Test - public void testAddRelocateHierarchy() { - createNotPresentCodeSystem(); - - // Add code hierarchy - CustomTerminologySet delta = new CustomTerminologySet(); - TermConcept codeA = delta.addRootConcept("CodeA", "Code A"); - TermConcept codeAA = codeA.addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeAA").setDisplay("Code AA"); - codeAA.addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeAAA").setDisplay("Code AAA"); - codeAA.addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeAAB").setDisplay("Code AAB"); - TermConcept codeB = delta.addRootConcept("CodeB", "Code B"); - TermConcept codeBA = codeB.addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeBA").setDisplay("Code BA"); - codeBA.addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeBAA").setDisplay("Code BAA"); - codeBA.addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeBAB").setDisplay("Code BAB"); - UploadStatistics outcome = myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertEquals(8, outcome.getUpdatedConceptCount()); - assertHierarchyContains( - "CodeA seq=0", - " CodeAA seq=0", - " CodeAAA seq=0", - " CodeAAB seq=1", - "CodeB seq=0", - " CodeBA seq=0", - " CodeBAA seq=0", - " CodeBAB seq=1" - ); - - // Move a single child code to a new spot and make sure the hierarchy comes along - // for the ride.. - delta = new CustomTerminologySet(); - delta - .addRootConcept("CodeB", "Code B") - .addChild(TermConceptParentChildLink.RelationshipTypeEnum.ISA).setCode("CodeAA").setDisplay("Code AA"); - outcome = myTermCodeSystemStorageSvc.applyDeltaCodeSystemsAdd("http://foo/cs", delta); - assertEquals(2, outcome.getUpdatedConceptCount()); - assertHierarchyContains( - "CodeA seq=0", - "CodeB seq=0", - " CodeBA seq=0", - " CodeBAA seq=0", - " CodeBAB seq=1", - " CodeAA seq=0", // <-- CodeAA got added here so it comes second - " CodeAAA seq=0", - " CodeAAB seq=1" - ); - - } - @Test @Ignore public void testAddWithPropertiesAndDesignations() {