Don't thrash delta links (#1721)

* Don't thrash delta links

* Add changelog

* Remove commented out code

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.

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<TermConcept> parentConcepts = new HashSet<>();
-
-		if (!theParentCodes.isEmpty()) {
-			parentDescription = "[" + String.join(", ", theParentCodes) + "]";
-			for (String nextParentCode : theParentCodes) {
-				Optional<TermConcept> 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<TermConcept> existingCodeOpt = myConceptDao.findByCodeSystemAndCode(theCsv, nextCodeToAdd);
+		List<TermConceptParentChildLink> 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<TermConcept> 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<TermConcept> 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<TermConceptParentChildLink> 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<TermConceptParentChildLink> 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);

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() {