Handle partial failure 52/130852/3
authormpriyank <priyank.maheshwari@est.tech>
Tue, 6 Sep 2022 17:29:34 +0000 (18:29 +0100)
committermpriyank <priyank.maheshwari@est.tech>
Mon, 12 Sep 2022 17:09:43 +0000 (18:09 +0100)
- Removing the transaction boundaries as it was getting rollbacked on
  partial failures
- Handled adding the elements in batch and if it fails try them
  individually
- Refactored code a bit and when there is partial failure we try one
  more time in sequence and even if there are failures we collect the
  failures

Issue-ID: CPS-1232
Issue-ID: CPS-1126
Change-Id: I7824c9f37f80cbaeedd5dc06d598ca0e3a69c59b
Signed-off-by: mpriyank <priyank.maheshwari@est.tech>
cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/impl/NetworkCmProxyDataServiceImpl.java
cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/models/CmHandleRegistrationResponse.java
cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/api/impl/NetworkCmProxyDataServiceImplRegistrationSpec.groovy
cps-ri/src/main/java/org/onap/cps/spi/impl/CpsDataPersistenceServiceImpl.java
cps-ri/src/test/groovy/org/onap/cps/spi/impl/CpsDataPersistenceServiceIntegrationSpec.groovy
cps-service/src/main/java/org/onap/cps/api/impl/CpsDataServiceImpl.java
cps-service/src/main/java/org/onap/cps/spi/CpsDataPersistenceService.java
cps-service/src/main/java/org/onap/cps/spi/exceptions/AlreadyDefinedExceptionBatch.java [new file with mode: 0644]
cps-service/src/test/groovy/org/onap/cps/api/impl/CpsDataServiceImplSpec.groovy

index 209ade9..0eb275c 100755 (executable)
@@ -59,7 +59,7 @@ import org.onap.cps.ncmp.api.models.DmiPluginRegistration;
 import org.onap.cps.ncmp.api.models.DmiPluginRegistrationResponse;
 import org.onap.cps.ncmp.api.models.NcmpServiceCmHandle;
 import org.onap.cps.spi.FetchDescendantsOption;
-import org.onap.cps.spi.exceptions.AlreadyDefinedException;
+import org.onap.cps.spi.exceptions.AlreadyDefinedExceptionBatch;
 import org.onap.cps.spi.exceptions.CpsException;
 import org.onap.cps.spi.exceptions.DataNodeNotFoundException;
 import org.onap.cps.spi.exceptions.DataValidationException;
@@ -365,12 +365,12 @@ public class NetworkCmProxyDataServiceImpl implements NetworkCmProxyDataService
         try {
             lcmEventsCmHandleStateHandler.updateCmHandleStateBatch(cmHandleStatePerCmHandle);
             return CmHandleRegistrationResponse.createSuccessResponses(cmHandleIds);
-        } catch (final AlreadyDefinedException alreadyDefinedException) {
-            return List.of(CmHandleRegistrationResponse.createFailureResponse(
-                    String.join(",", cmHandleIds), RegistrationError.CM_HANDLE_ALREADY_EXIST));
+        } catch (final AlreadyDefinedExceptionBatch alreadyDefinedExceptionBatch) {
+            return CmHandleRegistrationResponse.createFailureResponses(
+                    alreadyDefinedExceptionBatch.getAlreadyDefinedCmHandleIds(),
+                    RegistrationError.CM_HANDLE_ALREADY_EXIST);
         } catch (final Exception exception) {
-            return List.of(CmHandleRegistrationResponse.createFailureResponse(String.join(",", cmHandleIds),
-                    exception));
+            return CmHandleRegistrationResponse.createFailureResponses(cmHandleIds, exception);
         }
     }
 }
index b7faf09..9f80218 100644 (file)
@@ -21,6 +21,7 @@
 
 package org.onap.cps.ncmp.api.models;
 
+import java.util.Collection;
 import java.util.List;
 import java.util.stream.Collectors;
 import lombok.Builder;
@@ -67,6 +68,34 @@ public class CmHandleRegistrationResponse {
             .build();
     }
 
+    /**
+     * Creates a failure response based on registration error.
+     *
+     * @param cmHandleIds       list of failed cmHandleIds
+     * @param registrationError enum describing the type of registration error
+     * @return CmHandleRegistrationResponse
+     */
+    public static List<CmHandleRegistrationResponse> createFailureResponses(final Collection<String> cmHandleIds,
+            final RegistrationError registrationError) {
+        return cmHandleIds.stream()
+                .map(cmHandleId -> CmHandleRegistrationResponse.createFailureResponse(cmHandleId, registrationError))
+                .collect(Collectors.toList());
+    }
+
+    /**
+     * Creates a failure response based on other exception.
+     *
+     * @param cmHandleIds list of failed cmHandleIds
+     * @param exception   exception caught during the registration
+     * @return CmHandleRegistrationResponse
+     */
+    public static List<CmHandleRegistrationResponse> createFailureResponses(final Collection<String> cmHandleIds,
+            final Exception exception) {
+        return cmHandleIds.stream()
+                .map(cmHandleId -> CmHandleRegistrationResponse.createFailureResponse(cmHandleId, exception))
+                .collect(Collectors.toList());
+    }
+
     public static CmHandleRegistrationResponse createSuccessResponse(final String cmHandle) {
         return CmHandleRegistrationResponse.builder().cmHandle(cmHandle)
             .status(Status.SUCCESS).build();
index 86a32a1..2fe521c 100644 (file)
@@ -28,14 +28,13 @@ import org.onap.cps.ncmp.api.NetworkCmProxyCmHandlerQueryService
 import org.onap.cps.ncmp.api.impl.event.lcm.LcmEventsCmHandleStateHandler
 import org.onap.cps.ncmp.api.impl.exception.DmiRequestException
 import org.onap.cps.ncmp.api.impl.operations.DmiDataOperations
-import org.onap.cps.ncmp.api.impl.yangmodels.YangModelCmHandle
 import org.onap.cps.ncmp.api.inventory.CmHandleQueries
 import org.onap.cps.ncmp.api.inventory.CmHandleState
 import org.onap.cps.ncmp.api.inventory.InventoryPersistence
 import org.onap.cps.ncmp.api.models.CmHandleRegistrationResponse
 import org.onap.cps.ncmp.api.models.DmiPluginRegistration
 import org.onap.cps.ncmp.api.models.NcmpServiceCmHandle
-import org.onap.cps.spi.exceptions.AlreadyDefinedException
+import org.onap.cps.spi.exceptions.AlreadyDefinedExceptionBatch
 import org.onap.cps.spi.exceptions.DataNodeNotFoundException
 import org.onap.cps.spi.exceptions.DataValidationException
 import org.onap.cps.spi.exceptions.SchemaSetNotFoundException
@@ -185,20 +184,17 @@ class NetworkCmProxyDataServiceImplRegistrationSpec extends Specification {
                                        new NcmpServiceCmHandle(cmHandleId: 'cmhandle2'),
                                        new NcmpServiceCmHandle(cmHandleId: 'cmhandle3')])
         and: 'cm-handle creation is successful for 1st and 3rd; failed for 2nd'
-            mockLcmEventsCmHandleStateHandler.updateCmHandleStateBatch(*_) >> { throw new RuntimeException("Failed") }
+            mockLcmEventsCmHandleStateHandler.updateCmHandleStateBatch(*_) >> { throw new AlreadyDefinedExceptionBatch(['cmhandle2']) }
         when: 'registration is updated to create cm-handles'
             def response = objectUnderTest.updateDmiRegistrationAndSyncModule(dmiPluginRegistration)
         then: 'a response is received for all cm-handles'
             response.getCreatedCmHandles().size() == 1
         and: 'all cm-handles creation fails'
-            with(response.getCreatedCmHandles().get(0)) {
+            response.getCreatedCmHandles().each {
+                assert it.cmHandle == 'cmhandle2'
                 assert it.status == Status.FAILURE
-                assert it.registrationError == UNKNOWN_ERROR
-                assert it.errorText == 'Failed'
-                def sortedCmHandles = it.cmHandle.split(',').sort()
-                assert sortedCmHandles[0] == 'cmhandle1'
-                assert sortedCmHandles[1] == 'cmhandle2'
-                assert sortedCmHandles[2] == 'cmhandle3'
+                assert it.registrationError == CM_HANDLE_ALREADY_EXIST
+                assert it.errorText == 'cm-handle already exists'
             }
     }
 
@@ -219,10 +215,10 @@ class NetworkCmProxyDataServiceImplRegistrationSpec extends Specification {
                 assert it.errorText == expectedErrorText
             }
         where:
-            scenario                                        | cmHandleId             | exception                                               || expectedError           | expectedErrorText
-            'cm-handle already exist'                       | 'cmhandle'             | new AlreadyDefinedException('', new RuntimeException()) || CM_HANDLE_ALREADY_EXIST | 'cm-handle already exists'
-            'cm-handle has invalid name'                    | 'cm handle with space' | new DataValidationException("", "")                     || CM_HANDLE_INVALID_ID    | 'cm-handle has an invalid character(s) in id'
-            'unknown exception while registering cm-handle' | 'cmhandle'             | new RuntimeException('Failed')                          || UNKNOWN_ERROR           | 'Failed'
+            scenario                                        | cmHandleId             | exception                                      || expectedError           | expectedErrorText
+            'cm-handle already exist'                       | 'cmhandle'             | new AlreadyDefinedExceptionBatch([cmHandleId]) || CM_HANDLE_ALREADY_EXIST | 'cm-handle already exists'
+            'cm-handle has invalid name'                    | 'cm handle with space' | new DataValidationException("", "")            || CM_HANDLE_INVALID_ID    | 'cm-handle has an invalid character(s) in id'
+            'unknown exception while registering cm-handle' | 'cmhandle'             | new RuntimeException('Failed')                 || UNKNOWN_ERROR           | 'Failed'
     }
 
     def 'Update CM-Handle: Update Operation Response is added to the response'() {
index 61e1d5b..e02fb73 100644 (file)
@@ -52,6 +52,7 @@ import org.onap.cps.spi.entities.FragmentEntity;
 import org.onap.cps.spi.entities.SchemaSetEntity;
 import org.onap.cps.spi.entities.YangResourceEntity;
 import org.onap.cps.spi.exceptions.AlreadyDefinedException;
+import org.onap.cps.spi.exceptions.AlreadyDefinedExceptionBatch;
 import org.onap.cps.spi.exceptions.ConcurrencyException;
 import org.onap.cps.spi.exceptions.CpsAdminException;
 import org.onap.cps.spi.exceptions.CpsPathException;
@@ -88,51 +89,75 @@ public class CpsDataPersistenceServiceImpl implements CpsDataPersistenceService
             Pattern.compile("\\[(\\@([^\\/]{0,9999}))\\]$");
 
     @Override
-    @Transactional
     public void addChildDataNode(final String dataspaceName, final String anchorName, final String parentNodeXpath,
                                  final DataNode newChildDataNode) {
-        addChildDataNodes(dataspaceName, anchorName, parentNodeXpath, Collections.singleton(newChildDataNode));
+        addNewChildDataNode(dataspaceName, anchorName, parentNodeXpath, newChildDataNode);
     }
 
     @Override
-    @Transactional
     public void addListElements(final String dataspaceName, final String anchorName, final String parentNodeXpath,
                                 final Collection<DataNode> newListElements) {
-        addChildDataNodes(dataspaceName, anchorName, parentNodeXpath, newListElements);
+        addChildrenDataNodes(dataspaceName, anchorName, parentNodeXpath, newListElements);
     }
 
     @Override
-    @Transactional
-    public void addListElementsBatch(final String dataspaceName, final String anchorName, final String parentNodeXpath,
-            final Collection<Collection<DataNode>> newListsElements) {
+    public void addMultipleLists(final String dataspaceName, final String anchorName, final String parentNodeXpath,
+            final Collection<Collection<DataNode>> newLists) {
+        final Collection<String> failedCmHandleIds = new HashSet<>();
+        newLists.forEach(newList -> {
+            try {
+                addChildrenDataNodes(dataspaceName, anchorName, parentNodeXpath, newList);
+            } catch (final AlreadyDefinedException e) {
+                newList.forEach(listElement -> failedCmHandleIds.add((String) listElement.getLeaves().get("id")));
+            }
+        });
 
-        newListsElements.forEach(
-                newListElement -> addListElements(dataspaceName, anchorName, parentNodeXpath, newListElement));
+        if (!failedCmHandleIds.isEmpty()) {
+            throw new AlreadyDefinedExceptionBatch(failedCmHandleIds);
+        }
 
     }
 
-    private void addChildDataNodes(final String dataspaceName, final String anchorName, final String parentNodeXpath,
-                                   final Collection<DataNode> newChildren) {
+    private void addNewChildDataNode(final String dataspaceName, final String anchorName,
+            final String parentNodeXpath, final DataNode newChild) {
         final FragmentEntity parentFragmentEntity = getFragmentByXpath(dataspaceName, anchorName, parentNodeXpath);
+        final FragmentEntity newChildAsFragmentEntity =
+                convertToFragmentWithAllDescendants(parentFragmentEntity.getDataspace(),
+                        parentFragmentEntity.getAnchor(), newChild);
+        newChildAsFragmentEntity.setParentId(parentFragmentEntity.getId());
+        try {
+            fragmentRepository.save(newChildAsFragmentEntity);
+        } catch (final DataIntegrityViolationException e) {
+            throw AlreadyDefinedException.forDataNode(newChild.getXpath(), anchorName, e);
+        }
+
+    }
+
+    private void addChildrenDataNodes(final String dataspaceName, final String anchorName, final String parentNodeXpath,
+            final Collection<DataNode> newChildren) {
+        final FragmentEntity parentFragmentEntity = getFragmentByXpath(dataspaceName, anchorName, parentNodeXpath);
+        final List<FragmentEntity> fragmentEntities = new ArrayList<>(newChildren.size());
         try {
-            final List<FragmentEntity> fragmentEntities = new ArrayList<>();
             newChildren.forEach(newChildAsDataNode -> {
-                final FragmentEntity newChildAsFragmentEntity = convertToFragmentWithAllDescendants(
-                    parentFragmentEntity.getDataspace(),
-                    parentFragmentEntity.getAnchor(),
-                    newChildAsDataNode);
+                final FragmentEntity newChildAsFragmentEntity =
+                        convertToFragmentWithAllDescendants(parentFragmentEntity.getDataspace(),
+                                parentFragmentEntity.getAnchor(), newChildAsDataNode);
                 newChildAsFragmentEntity.setParentId(parentFragmentEntity.getId());
                 fragmentEntities.add(newChildAsFragmentEntity);
             });
             fragmentRepository.saveAll(fragmentEntities);
-        } catch (final DataIntegrityViolationException exception) {
-            final List<String> conflictXpaths = newChildren.stream()
-                    .map(DataNode::getXpath)
-                    .collect(Collectors.toList());
-            throw AlreadyDefinedException.forDataNodes(conflictXpaths, anchorName, exception);
+        } catch (final DataIntegrityViolationException e) {
+            log.warn("Exception occurred : {} , Batch with size : {} will be retried using individual save operations",
+                    e, fragmentEntities.size());
+            retrySavingEachChildIndividually(dataspaceName, anchorName, parentNodeXpath, newChildren);
         }
     }
 
+    private void retrySavingEachChildIndividually(final String dataspaceName, final String anchorName,
+            final String parentNodeXpath, final Collection<DataNode> newChildren) {
+        newChildren.forEach(newChild -> addNewChildDataNode(dataspaceName, anchorName, parentNodeXpath, newChild));
+    }
+
     @Override
     public void storeDataNode(final String dataspaceName, final String anchorName, final DataNode dataNode) {
         final DataspaceEntity dataspaceEntity = dataspaceRepository.getByName(dataspaceName);
index acc243b..ba9bd6f 100755 (executable)
@@ -27,6 +27,7 @@ import org.onap.cps.cpspath.parser.PathParsingException
 import org.onap.cps.spi.CpsDataPersistenceService
 import org.onap.cps.spi.entities.FragmentEntity
 import org.onap.cps.spi.exceptions.AlreadyDefinedException
+import org.onap.cps.spi.exceptions.AlreadyDefinedExceptionBatch
 import org.onap.cps.spi.exceptions.AnchorNotFoundException
 import org.onap.cps.spi.exceptions.CpsAdminException
 import org.onap.cps.spi.exceptions.CpsPathException
@@ -165,7 +166,7 @@ class CpsDataPersistenceServiceIntegrationSpec extends CpsPersistenceSpecBase {
             def grandChild = buildDataNode('/parent-201/child-204[@key="NEW1"]/grand-child-204[@key2="NEW1-CHILD"]', [leave:'value'], [])
             listElements[0].childDataNodes = [grandChild]
         when: 'the new data node (list elements) are added to an existing parent node'
-            objectUnderTest.addListElementsBatch(DATASPACE_NAME, ANCHOR_NAME3, '/parent-201', [listElements])
+            objectUnderTest.addMultipleLists(DATASPACE_NAME, ANCHOR_NAME3, '/parent-201', [listElements])
         then: 'new entries are successfully persisted, parent node now contains 5 children (2 new + 3 existing before)'
             def parentFragment = fragmentRepository.getById(LIST_DATA_NODE_PARENT201_FRAGMENT_ID)
             def allChildXpaths = parentFragment.childFragments.collect { it.xpath }
@@ -178,6 +179,22 @@ class CpsDataPersistenceServiceIntegrationSpec extends CpsPersistenceSpecBase {
             assert grandChildFragmentEntity.isPresent()
     }
 
+    @Sql([CLEAR_DATA, SET_DATA])
+    def 'Add already existing list elements'() {
+        given: 'two new child list elements for an existing parent'
+            def listElementXpaths1 = ['/parent-100', '/parent-100/child-001']
+            def listElementXpaths2 = ['/parent-200', '/parent-200/child-201']
+            def listElements1 = toDataNodesWithId(listElementXpaths1, 'cmhandle1')
+            def listElements2 = toDataNodesWithId(listElementXpaths2, 'cmhandle2')
+        when: 'duplicate data node is requested to be added'
+            objectUnderTest.addMultipleLists(DATASPACE_NAME, ANCHOR_NAME3, '/', [listElements1,listElements2])
+        then: 'already defined batch exception is thrown'
+            def e = thrown(AlreadyDefinedExceptionBatch)
+        and: 'it contains both cmhandle ids'
+            assert e.alreadyDefinedCmHandleIds.size() == 2
+            assert e.alreadyDefinedCmHandleIds.containsAll(['cmhandle1', 'cmhandle2'])
+    }
+
     @Sql([CLEAR_DATA, SET_DATA])
     def 'Add list element error scenario: #scenario.'() {
         given: 'list element as a collection of data nodes'
@@ -559,6 +576,10 @@ class CpsDataPersistenceServiceIntegrationSpec extends CpsPersistenceSpecBase {
         return xpaths.collect { new DataNodeBuilder().withXpath(it).build() }
     }
 
+    static Collection<DataNode> toDataNodesWithId(xpaths, id) {
+        return xpaths.collect { new DataNodeBuilder().withXpath(it).withLeaves(['id': id]).build() }
+    }
+
     static DataNode buildDataNode(xpath, leaves, childDataNodes) {
         return new DataNodeBuilder().withXpath(xpath).withLeaves(leaves).withChildDataNodes(childDataNodes).build()
     }
index 6bf4935..b6aa04b 100755 (executable)
@@ -97,7 +97,7 @@ public class CpsDataServiceImpl implements CpsDataService {
         CpsValidator.validateNameCharacters(dataspaceName, anchorName);
         final Collection<Collection<DataNode>> listElementDataNodeCollections =
                 buildDataNodes(dataspaceName, anchorName, parentNodeXpath, jsonDataList);
-        cpsDataPersistenceService.addListElementsBatch(dataspaceName, anchorName, parentNodeXpath,
+        cpsDataPersistenceService.addMultipleLists(dataspaceName, anchorName, parentNodeXpath,
                 listElementDataNodeCollections);
         processDataUpdatedEventAsync(dataspaceName, anchorName, parentNodeXpath, UPDATE, observedTimestamp);
     }
index 8b45ae7..cd0cefc 100644 (file)
@@ -66,15 +66,15 @@ public interface CpsDataPersistenceService {
         Collection<DataNode> listElementsCollection);
 
     /**
-     * Adds list child elements to a Fragment.
+     * Add multiple lists of data nodes to a parent node at the same time.
      *
      * @param dataspaceName           dataspace name
      * @param anchorName              anchor name
      * @param parentNodeXpath         parent node xpath
-     * @param listElementsCollections collections of data nodes representing list elements
+     * @param newLists collections of lists of data nodes representing list elements
      */
-    void addListElementsBatch(String dataspaceName, String anchorName, String parentNodeXpath,
-            Collection<Collection<DataNode>> listElementsCollections);
+    void addMultipleLists(String dataspaceName, String anchorName, String parentNodeXpath,
+            Collection<Collection<DataNode>> newLists);
 
     /**
      * Retrieves datanode by XPath for given dataspace and anchor.
diff --git a/cps-service/src/main/java/org/onap/cps/spi/exceptions/AlreadyDefinedExceptionBatch.java b/cps-service/src/main/java/org/onap/cps/spi/exceptions/AlreadyDefinedExceptionBatch.java
new file mode 100644 (file)
index 0000000..a5ce6fd
--- /dev/null
@@ -0,0 +1,34 @@
+/*
+ *  ============LICENSE_START=======================================================
+ *  Copyright (C) 2022 Nordix Foundation
+ *  ================================================================================
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *
+ *  SPDX-License-Identifier: Apache-2.0
+ *  ============LICENSE_END=========================================================
+ */
+
+package org.onap.cps.spi.exceptions;
+
+import java.util.Collection;
+import lombok.Getter;
+
+public class AlreadyDefinedExceptionBatch extends RuntimeException {
+
+    @Getter
+    private final Collection<String> alreadyDefinedCmHandleIds;
+
+    public AlreadyDefinedExceptionBatch(final Collection<String> alreadyDefinedCmHandleIds) {
+        this.alreadyDefinedCmHandleIds = alreadyDefinedCmHandleIds;
+    }
+}
index ab960df..3f28f0a 100644 (file)
@@ -143,7 +143,7 @@ class CpsDataServiceImplSpec extends Specification {
             def jsonData = '{"branch": [{"name": "A"}, {"name": "B"}]}'
             objectUnderTest.saveListElementsBatch(dataspaceName, anchorName, '/test-tree', [jsonData], observedTimestamp)
         then: 'the persistence service method is invoked with correct parameters'
-            1 * mockCpsDataPersistenceService.addListElementsBatch(dataspaceName, anchorName, '/test-tree',_) >> {
+            1 * mockCpsDataPersistenceService.addMultipleLists(dataspaceName, anchorName, '/test-tree',_) >> {
                 args -> {
                     def listElementsCollection = args[3] as Collection<Collection<DataNode>>
                     assert listElementsCollection.size() == 1