Improve performance for update data nodes

- Extract method getFragmentEntities from getDataNodes
- Remove unused code from getFragmentEntity
- Use bulk getFragmentEntities in updateDataNodesAndDescendants
- Update performance test timings

Issue-ID: CPS-1504
Signed-off-by: danielhanrahan <daniel.hanrahan@est.tech>
Change-Id: Icd2c6a0e6009e152de43090cbc23a21349703da2

3 files changed, 72 insertions, 52 deletions

diff --git a/cps-ri/src/main/java/org/onap/cps/spi/impl/CpsDataPersistenceServiceImpl.java b/cps-ri/src/main/java/org/onap/cps/spi/impl/CpsDataPersistenceServiceImpl.java
index 2fba30ee2..dd2d3652e 100644
--- a/cps-ri/src/main/java/org/onap/cps/spi/impl/CpsDataPersistenceServiceImpl.java
+++ b/cps-ri/src/main/java/org/onap/cps/spi/impl/CpsDataPersistenceServiceImpl.java
@@ -120,7 +120,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     private void addNewChildDataNode(final String dataspaceName, final String anchorName,
                                      final String parentNodeXpath, final DataNode newChild) {
         final FragmentEntity parentFragmentEntity =
-            getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, parentNodeXpath);
+            getFragmentEntity(dataspaceName, anchorName, parentNodeXpath);
         final FragmentEntity newChildAsFragmentEntity =
                 convertToFragmentWithAllDescendants(parentFragmentEntity.getDataspace(),
                         parentFragmentEntity.getAnchor(), newChild);
@@ -136,7 +136,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     private void addChildrenDataNodes(final String dataspaceName, final String anchorName, final String parentNodeXpath,
                                       final Collection<DataNode> newChildren) {
         final FragmentEntity parentFragmentEntity =
-            getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, parentNodeXpath);
+            getFragmentEntity(dataspaceName, anchorName, parentNodeXpath);
         final List<FragmentEntity> fragmentEntities = new ArrayList<>(newChildren.size());
         try {
             newChildren.forEach(newChildAsDataNode -> {
@@ -265,6 +265,12 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     public Collection<DataNode> getDataNodesForMultipleXpaths(final String dataspaceName, final String anchorName,
                                                               final Collection<String> xpaths,
                                                               final FetchDescendantsOption fetchDescendantsOption) {
+        final Collection<FragmentEntity> fragmentEntities = getFragmentEntities(dataspaceName, anchorName, xpaths);
+        return toDataNodes(fragmentEntities, fetchDescendantsOption);
+    }
+
+    private Collection<FragmentEntity> getFragmentEntities(final String dataspaceName, final String anchorName,
+                                                           final Collection<String> xpaths) {
         final DataspaceEntity dataspaceEntity = dataspaceRepository.getByName(dataspaceName);
         final AnchorEntity anchorEntity = anchorRepository.getByDataspaceAndName(dataspaceEntity, anchorName);
 
@@ -276,7 +282,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
             try {
                 normalizedXpaths.add(CpsPathUtil.getNormalizedXpath(xpath));
             } catch (final PathParsingException e) {
-                log.warn("Error parsing xpath \"{}\" in getDataNodesForMultipleXpaths: {}", xpath, e.getMessage());
+                log.warn("Error parsing xpath \"{}\": {}", xpath, e.getMessage());
             }
         }
         final Collection<FragmentEntity> fragmentEntities =
@@ -288,17 +294,10 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
             fragmentEntities.addAll(FragmentEntityArranger.toFragmentEntityTrees(anchorEntity, fragmentExtracts));
         }
 
-        return toDataNodes(fragmentEntities, fetchDescendantsOption);
+        return fragmentEntities;
     }
 
-    private FragmentEntity getFragmentWithoutDescendantsByXpath(final String dataspaceName,
-                                                                final String anchorName,
-                                                                final String xpath) {
-        return getFragmentByXpath(dataspaceName, anchorName, xpath, FetchDescendantsOption.OMIT_DESCENDANTS);
-    }
-
-    private FragmentEntity getFragmentByXpath(final String dataspaceName, final String anchorName,
-                                              final String xpath, final FetchDescendantsOption fetchDescendantsOption) {
+    private FragmentEntity getFragmentEntity(final String dataspaceName, final String anchorName, final String xpath) {
         final DataspaceEntity dataspaceEntity = dataspaceRepository.getByName(dataspaceName);
         final AnchorEntity anchorEntity = anchorRepository.getByDataspaceAndName(dataspaceEntity, anchorName);
         final FragmentEntity fragmentEntity;
@@ -309,13 +308,8 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
                 .stream().findFirst().orElse(null);
         } else {
             final String normalizedXpath = getNormalizedXpath(xpath);
-            if (FetchDescendantsOption.OMIT_DESCENDANTS.equals(fetchDescendantsOption)) {
-                fragmentEntity =
-                    fragmentRepository.getByDataspaceAndAnchorAndXpath(dataspaceEntity, anchorEntity, normalizedXpath);
-            } else {
-                fragmentEntity = buildFragmentEntitiesFromFragmentExtracts(anchorEntity, normalizedXpath)
-                    .stream().findFirst().orElse(null);
-            }
+            fragmentEntity =
+                fragmentRepository.getByDataspaceAndAnchorAndXpath(dataspaceEntity, anchorEntity, normalizedXpath);
         }
         if (fragmentEntity == null) {
             throw new DataNodeNotFoundException(dataspaceEntity.getName(), anchorEntity.getName(), xpath);
@@ -491,7 +485,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     @Override
     public void updateDataLeaves(final String dataspaceName, final String anchorName, final String xpath,
                                  final Map<String, Serializable> updateLeaves) {
-        final FragmentEntity fragmentEntity = getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, xpath);
+        final FragmentEntity fragmentEntity = getFragmentEntity(dataspaceName, anchorName, xpath);
         final String currentLeavesAsString = fragmentEntity.getAttributes();
         final String mergedLeaves = mergeLeaves(updateLeaves, currentLeavesAsString);
         fragmentEntity.setAttributes(mergedLeaves);
@@ -501,8 +495,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     @Override
     public void updateDataNodeAndDescendants(final String dataspaceName, final String anchorName,
                                              final DataNode dataNode) {
-        final FragmentEntity fragmentEntity =
-            getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, dataNode.getXpath());
+        final FragmentEntity fragmentEntity = getFragmentEntity(dataspaceName, anchorName, dataNode.getXpath());
         updateFragmentEntityAndDescendantsWithDataNode(fragmentEntity, dataNode);
         try {
             fragmentRepository.save(fragmentEntity);
@@ -514,21 +507,24 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     }
 
     @Override
-    public void updateDataNodesAndDescendants(final String dataspaceName,
-                                              final String anchorName,
-                                              final List<DataNode> dataNodes) {
-
-        final Map<DataNode, FragmentEntity> dataNodeFragmentEntityMap = dataNodes.stream()
-                .collect(Collectors.toMap(
-                        dataNode -> dataNode,
-                        dataNode ->
-                            getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, dataNode.getXpath())));
-        dataNodeFragmentEntityMap.forEach(
-                (dataNode, fragmentEntity) -> updateFragmentEntityAndDescendantsWithDataNode(fragmentEntity, dataNode));
+    public void updateDataNodesAndDescendants(final String dataspaceName, final String anchorName,
+                                              final List<DataNode> updatedDataNodes) {
+        final Map<String, DataNode> xpathToUpdatedDataNode = updatedDataNodes.stream()
+            .collect(Collectors.toMap(DataNode::getXpath, dataNode -> dataNode));
+
+        final Collection<String> xpaths = xpathToUpdatedDataNode.keySet();
+        final Collection<FragmentEntity> existingFragmentEntities =
+            getFragmentEntities(dataspaceName, anchorName, xpaths);
+
+        for (final FragmentEntity existingFragmentEntity : existingFragmentEntities) {
+            final DataNode updatedDataNode = xpathToUpdatedDataNode.get(existingFragmentEntity.getXpath());
+            updateFragmentEntityAndDescendantsWithDataNode(existingFragmentEntity, updatedDataNode);
+        }
+
         try {
-            fragmentRepository.saveAll(dataNodeFragmentEntityMap.values());
+            fragmentRepository.saveAll(existingFragmentEntities);
         } catch (final StaleStateException staleStateException) {
-            retryUpdateDataNodesIndividually(dataspaceName, anchorName, dataNodeFragmentEntityMap.values());
+            retryUpdateDataNodesIndividually(dataspaceName, anchorName, existingFragmentEntities);
         }
     }
 
@@ -582,8 +578,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
     @Transactional
     public void replaceListContent(final String dataspaceName, final String anchorName, final String parentNodeXpath,
                                    final Collection<DataNode> newListElements) {
-        final FragmentEntity parentEntity =
-            getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, parentNodeXpath);
+        final FragmentEntity parentEntity = getFragmentEntity(dataspaceName, anchorName, parentNodeXpath);
         final String listElementXpathPrefix = getListElementXpathPrefix(newListElements);
         final Map<String, FragmentEntity> existingListElementFragmentEntitiesByXPath =
                 extractListElementFragmentEntitiesByXPath(parentEntity.getChildFragments(), listElementXpathPrefix);
@@ -631,7 +626,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
             try {
                 normalizedXpaths.add(CpsPathUtil.getNormalizedXpath(xpath));
             } catch (final PathParsingException e) {
-                log.debug("Error parsing xpath \"{}\" in deleteDataNodes: {}", xpath, e.getMessage());
+                log.debug("Error parsing xpath \"{}\": {}", xpath, e.getMessage());
             }
         }
 
@@ -666,7 +661,7 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
             } else {
                 parentNodeXpath = CpsPathUtil.getNormalizedParentXpath(targetXpath);
             }
-            parentFragmentEntity = getFragmentWithoutDescendantsByXpath(dataspaceName, anchorName, parentNodeXpath);
+            parentFragmentEntity = getFragmentEntity(dataspaceName, anchorName, parentNodeXpath);
             if (CpsPathUtil.isPathToListElement(targetXpath)) {
                 targetDeleted = deleteDataNode(parentFragmentEntity, targetXpath);
             } else {
diff --git a/cps-ri/src/test/groovy/org/onap/cps/spi/impl/CpsDataPersistenceServiceSpec.groovy b/cps-ri/src/test/groovy/org/onap/cps/spi/impl/CpsDataPersistenceServiceSpec.groovy
index ac66e8c02..ba42a083e 100644
--- a/cps-ri/src/test/groovy/org/onap/cps/spi/impl/CpsDataPersistenceServiceSpec.groovy
+++ b/cps-ri/src/test/groovy/org/onap/cps/spi/impl/CpsDataPersistenceServiceSpec.groovy
@@ -88,15 +88,17 @@ class CpsDataPersistenceServiceSpec extends Specification {
     }
 
     def 'Handling of StaleStateException (caused by concurrent updates) during update data nodes and descendants.'() {
-        given: 'the system contains and can update one datanode'
-            def dataNode1 = createDataNodeAndMockRepositoryMethodSupportingIt('/node1', 'OK')
-        and: 'the system contains two more datanodes that throw an exception while updating'
-            def dataNode2 = createDataNodeAndMockRepositoryMethodSupportingIt('/node2', 'EXCEPTION')
-            def dataNode3 = createDataNodeAndMockRepositoryMethodSupportingIt('/node3', 'EXCEPTION')
+        given: 'the system can update one datanode and has two more datanodes that throw an exception while updating'
+            def dataNodes = createDataNodesAndMockRepositoryMethodSupportingThem([
+                '/node1': 'OK',
+                '/node2': 'EXCEPTION',
+                '/node3': 'EXCEPTION'])
+        and: 'db contains an anchor'
+            mockAnchorRepository.getByDataspaceAndName(*_) >> new AnchorEntity(id:123)
         and: 'the batch update will therefore also fail'
             mockFragmentRepository.saveAll(*_) >> { throw new StaleStateException("concurrent updates") }
         when: 'attempt batch update data nodes'
-            objectUnderTest.updateDataNodesAndDescendants('some-dataspace', 'some-anchor', [dataNode1, dataNode2, dataNode3])
+            objectUnderTest.updateDataNodesAndDescendants('some-dataspace', 'some-anchor', dataNodes)
         then: 'concurrency exception is thrown'
             def thrown = thrown(ConcurrencyException)
             assert thrown.message == 'Concurrent Transactions'
@@ -199,7 +201,9 @@ class CpsDataPersistenceServiceSpec extends Specification {
 
     def 'update data node and descendants: #scenario'(){
         given: 'mocked responses'
-            mockFragmentRepository.getByDataspaceAndAnchorAndXpath(_, _, '/test/xpath') >> new FragmentEntity(xpath: '/test/xpath', childFragments: [])
+            mockAnchorRepository.getByDataspaceAndName(_, _) >> new AnchorEntity(id:123)
+            mockFragmentRepository.findByAnchorAndMultipleCpsPaths(_, [] as Set) >> []
+            mockFragmentRepository.findByAnchorAndMultipleCpsPaths(_, ['/test/xpath'] as Set) >> [new FragmentEntity(xpath: '/test/xpath', childFragments: [])]
         when: 'replace data node tree'
             objectUnderTest.updateDataNodesAndDescendants('dataspaceName', 'anchorName', dataNodes)
         then: 'call fragment repository save all method'
@@ -211,9 +215,12 @@ class CpsDataPersistenceServiceSpec extends Specification {
     }
 
     def 'update data nodes and descendants'() {
-        given: 'the fragment repository returns a fragment entity related to the xpath input'
-            mockFragmentRepository.getByDataspaceAndAnchorAndXpath(_, _, '/test/xpath1') >> new FragmentEntity(xpath: '/test/xpath1', childFragments: [])
-            mockFragmentRepository.getByDataspaceAndAnchorAndXpath(_, _, '/test/xpath2') >> new FragmentEntity(xpath: '/test/xpath2', childFragments: [])
+        given: 'the fragment repository returns fragment entities related to the xpath inputs'
+            mockFragmentRepository.findByAnchorAndMultipleCpsPaths(_, ['/test/xpath1', '/test/xpath2'] as Set) >> [
+                new FragmentEntity(xpath: '/test/xpath1', childFragments: []),
+                new FragmentEntity(xpath: '/test/xpath2', childFragments: [])]
+        and: 'db contains an anchor'
+            mockAnchorRepository.getByDataspaceAndName(*_) >> new AnchorEntity(id:123)
         and: 'some data nodes with descendants'
             def dataNode1 = new DataNode(xpath: '/test/xpath1', leaves: ['id': 'testId1'], childDataNodes: [new DataNode(xpath: '/test/xpath1/child', leaves: ['id': 'childTestId1'])])
             def dataNode2 = new DataNode(xpath: '/test/xpath2', leaves: ['id': 'testId2'], childDataNodes: [new DataNode(xpath: '/test/xpath2/child', leaves: ['id': 'childTestId2'])])
@@ -239,6 +246,25 @@ class CpsDataPersistenceServiceSpec extends Specification {
         return dataNode
     }
 
+    def createDataNodesAndMockRepositoryMethodSupportingThem(Map<String, String> xpathToScenarioMap) {
+        def dataNodes = []
+        def fragmentEntities = []
+        xpathToScenarioMap.each {
+            def xpath = it.key
+            def scenario = it.value
+            def dataNode = new DataNodeBuilder().withXpath(xpath).build()
+            dataNodes.add(dataNode)
+            def fragmentEntity = new FragmentEntity(xpath: xpath, childFragments: [])
+            fragmentEntities.add(fragmentEntity)
+            mockFragmentRepository.getByDataspaceAndAnchorAndXpath(_, _, xpath) >> fragmentEntity
+            if ('EXCEPTION' == scenario) {
+                mockFragmentRepository.save(fragmentEntity) >> { throw new StaleStateException("concurrent updates") }
+            }
+        }
+        mockFragmentRepository.findByAnchorAndMultipleCpsPaths(_, xpathToScenarioMap.keySet()) >> fragmentEntities
+        return dataNodes
+    }
+
     def mockFragmentWithJson(json) {
         def anchorEntity = new AnchorEntity(id:123)
         mockAnchorRepository.getByDataspaceAndName(*_) >> anchorEntity
diff --git a/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServicePerfTest.groovy b/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServicePerfTest.groovy
index cfd1093a9..3562419c0 100644
--- a/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServicePerfTest.groovy
+++ b/cps-ri/src/test/groovy/org/onap/cps/spi/performance/CpsDataPersistenceServicePerfTest.groovy
@@ -108,7 +108,6 @@ class CpsDataPersistenceServicePerfTest extends CpsPersistencePerfSpecBase {
             stopWatch.stop()
             def readDurationInMillis = stopWatch.getTotalTimeMillis()
         then: 'read duration is under #allowedDuration milliseconds'
-            assert readDurationInMillis < allowedDuration
             recordAndAssertPerformance("Query many descendants by cpspath (${scenario})", allowedDuration, readDurationInMillis)
         and: 'data node is returned with all the descendants populated'
             assert result.size() == NUMBER_OF_CHILDREN
@@ -153,7 +152,7 @@ class CpsDataPersistenceServicePerfTest extends CpsPersistencePerfSpecBase {
             objectUnderTest.updateDataNodesAndDescendants(PERF_DATASPACE, PERF_ANCHOR, dataNodes)
             stopWatch.stop()
             def updateDurationInMillis = stopWatch.getTotalTimeMillis()
-        then: 'update duration is under 1400 milliseconds'
-            recordAndAssertPerformance('Update data nodes without descendants', 1400, updateDurationInMillis)
+        then: 'update duration is under 600 milliseconds'
+            recordAndAssertPerformance('Update data nodes without descendants', 600, updateDurationInMillis)
     }
 }