Refactor ncmp request handlers (fix async issue)

- simplified request handlers (sub)classes (no more need for default interface)
- fix issue with async execution of data operation requests (wasn't really async)
- removed redundant (unreachable) check in production code
- Improved code coverage (branches) ncmp request handlers added UNIT test
- removed MVC test scenarios now covered by appropriate unit level tests
- applied Spock Polling Condition for verifying async call

Issue-ID: CPS-475
Signed-off-by: ToineSiebelink <toine.siebelink@est.tech>
Change-Id: Ibe601c709de65080fa8898f2419fdbd92c5ba27d

5 files changed, 131 insertions, 177 deletions

diff --git a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpCachedResourceRequestHandler.java b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpCachedResourceRequestHandler.java
index 76946d3af2..85a1eae234 100644
--- a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpCachedResourceRequestHandler.java
+++ b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpCachedResourceRequestHandler.java
@@ -25,6 +25,7 @@ import org.onap.cps.ncmp.api.NetworkCmProxyDataService;
 import org.onap.cps.ncmp.api.NetworkCmProxyQueryService;
 import org.onap.cps.ncmp.rest.executor.CpsNcmpTaskExecutor;
 import org.onap.cps.spi.FetchDescendantsOption;
+import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Component;
 
 @Component
@@ -48,37 +49,53 @@ public class NcmpCachedResourceRequestHandler extends NcmpDatastoreRequestHandle
         this.networkCmProxyQueryService = networkCmProxyQueryService;
     }
 
+    /**
+     * Executes a synchronous query request for given cm handle.
+     * Note. Currently only ncmp-datastore:operational supports query operations.
+     *
+     * @param cmHandleId         the cm handle
+     * @param resourceIdentifier the resource identifier
+     * @param includeDescendants whether include descendants
+     * @return the response entity
+     */
+    public ResponseEntity<Object> executeRequest(final String cmHandleId,
+                                                 final String resourceIdentifier,
+                                                 final boolean includeDescendants) {
+
+        final Supplier<Object> taskSupplier = getTaskSupplierForQueryRequest(cmHandleId, resourceIdentifier,
+            includeDescendants);
+        return executeTaskSync(taskSupplier);
+    }
+
     @Override
-    public Supplier<Object> getTaskSupplierForGetRequest(final String datastoreName,
-                                                         final String cmHandleId,
-                                                         final String resourceIdentifier,
-                                                         final String optionsParamInQuery,
-                                                         final String topicParamInQuery,
-                                                         final String requestId,
-                                                         final boolean includeDescendants) {
+    protected Supplier<Object> getTaskSupplierForGetRequest(final String datastoreName,
+                                                  final String cmHandleId,
+                                                  final String resourceIdentifier,
+                                                  final String optionsParamInQuery,
+                                                  final String topicParamInQuery,
+                                                  final String requestId,
+                                                  final boolean includeDescendants) {
 
-        final FetchDescendantsOption fetchDescendantsOption =
-                TaskManagementDefaultHandler.getFetchDescendantsOption(includeDescendants);
+        final FetchDescendantsOption fetchDescendantsOption = getFetchDescendantsOption(includeDescendants);
 
         return () -> networkCmProxyDataService.getResourceDataForCmHandle(datastoreName, cmHandleId, resourceIdentifier,
-                fetchDescendantsOption);
+            fetchDescendantsOption);
     }
 
-    /**
-     * Gets ncmp datastore query handler.
-     * Note. Currently only ncmp-datastore:operational supports query operations
-     * @return a ncmp datastore query handler.
-     */
-    @Override
-    public Supplier<Object> getTaskSupplierForQueryRequest(final String cmHandleId,
-                                                           final String resourceIdentifier,
-                                                           final boolean includeDescendants) {
+    private Supplier<Object> getTaskSupplierForQueryRequest(final String cmHandleId,
+                                                            final String resourceIdentifier,
+                                                            final boolean includeDescendants) {
 
-        final FetchDescendantsOption fetchDescendantsOption =
-                TaskManagementDefaultHandler.getFetchDescendantsOption(includeDescendants);
+        final FetchDescendantsOption fetchDescendantsOption = getFetchDescendantsOption(includeDescendants);
 
         return () -> networkCmProxyQueryService.queryResourceDataOperational(cmHandleId, resourceIdentifier,
-                fetchDescendantsOption);
+            fetchDescendantsOption);
     }
 
+    private static FetchDescendantsOption getFetchDescendantsOption(final boolean includeDescendants) {
+        return includeDescendants ? FetchDescendantsOption.INCLUDE_ALL_DESCENDANTS
+            : FetchDescendantsOption.OMIT_DESCENDANTS;
+    }
+
+
 }
diff --git a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpDatastoreRequestHandler.java b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpDatastoreRequestHandler.java
index d7aeab6b0f..d40ab9b390 100644
--- a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpDatastoreRequestHandler.java
+++ b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpDatastoreRequestHandler.java
@@ -20,30 +20,24 @@
 
 package org.onap.cps.ncmp.rest.controller.handlers;
 
-import static org.onap.cps.ncmp.api.impl.operations.DatastoreType.OPERATIONAL;
-import static org.onap.cps.ncmp.api.impl.operations.OperationType.READ;
-
 import java.util.Map;
 import java.util.UUID;
 import java.util.function.Supplier;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
-import org.onap.cps.ncmp.api.impl.exception.InvalidDatastoreException;
-import org.onap.cps.ncmp.api.impl.operations.DatastoreType;
-import org.onap.cps.ncmp.api.impl.operations.OperationType;
-import org.onap.cps.ncmp.api.models.DataOperationRequest;
-import org.onap.cps.ncmp.rest.exceptions.OperationNotSupportedException;
 import org.onap.cps.ncmp.rest.executor.CpsNcmpTaskExecutor;
 import org.onap.cps.ncmp.rest.util.TopicValidator;
 import org.springframework.beans.factory.annotation.Value;
-import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Service;
 
 @Slf4j
 @Service
 @RequiredArgsConstructor
-public class NcmpDatastoreRequestHandler implements TaskManagementDefaultHandler {
+public abstract class NcmpDatastoreRequestHandler {
+
+    private static final String NO_REQUEST_ID = null;
+    private static final String NO_TOPIC = null;
 
     @Value("${notification.async.executor.time-out-value-in-ms:2000}")
     protected int timeOutInMilliSeconds;
@@ -51,10 +45,10 @@ public class NcmpDatastoreRequestHandler implements TaskManagementDefaultHandler
     @Value("${notification.enabled:true}")
     protected boolean notificationFeatureEnabled;
 
-    private final CpsNcmpTaskExecutor cpsNcmpTaskExecutor;
+    protected final CpsNcmpTaskExecutor cpsNcmpTaskExecutor;
 
     /**
-     * Executes synchronous/asynchronous request for given cm handle.
+     * Executes synchronous/asynchronous get request for given cm handle.
      *
      * @param datastoreName       the name of the datastore
      * @param cmHandleId          the cm handle
@@ -86,46 +80,10 @@ public class NcmpDatastoreRequestHandler implements TaskManagementDefaultHandler
         return executeTaskSync(taskSupplier);
     }
 
-    /**
-     * Executes a synchronous request for given cm handle.
-     * Note. Currently only ncmp-datastore:operational supports query operations.
-     *
-     * @param cmHandleId         the cm handle
-     * @param resourceIdentifier the resource identifier
-     * @param includeDescendants whether include descendants
-     * @return the response entity
-     */
-    public ResponseEntity<Object> executeRequest(final String cmHandleId,
-                                                 final String resourceIdentifier,
-                                                 final boolean includeDescendants) {
-
-        final Supplier<Object> taskSupplier = getTaskSupplierForQueryRequest(cmHandleId, resourceIdentifier,
-                includeDescendants);
-        return executeTaskSync(taskSupplier);
-    }
 
-    /**
-     * Executes asynchronous request for group of cm handles to resource data.
-     *
-     * @param topicParamInQuery        the topic param in query
-     * @param dataOperationRequest     data operation request details for resource data
-     * @return the response entity
-     */
-    public ResponseEntity<Object> executeRequest(final String topicParamInQuery,
-                                                 final DataOperationRequest
-                                                         dataOperationRequest) {
-        validateDataOperationRequest(topicParamInQuery, dataOperationRequest);
-        if (!notificationFeatureEnabled) {
-            return ResponseEntity.ok(Map.of("status",
-                    "Asynchronous request is unavailable as notification feature is currently disabled."));
-        }
-        return getRequestIdAndSendDataOperationRequestToDmiService(topicParamInQuery, dataOperationRequest);
-    }
-
-    protected ResponseEntity<Object> executeTaskAsync(final String topicParamInQuery,
+    private ResponseEntity<Object> executeTaskAsync(final String topicParamInQuery,
                                                       final String requestId,
                                                       final Supplier<Object> taskSupplier) {
-
         TopicValidator.validateTopicName(topicParamInQuery);
         log.debug("Received Async request with id {}", requestId);
         cpsNcmpTaskExecutor.executeTask(taskSupplier, timeOutInMilliSeconds);
@@ -145,33 +103,15 @@ public class NcmpDatastoreRequestHandler implements TaskManagementDefaultHandler
         final String requestId = UUID.randomUUID().toString();
         final Supplier<Object> taskSupplier = getTaskSupplierForGetRequest(datastoreName, cmHandleId,
                 resourceIdentifier, optionsParamInQuery, topicParamInQuery, requestId, includeDescendants);
-        if (taskSupplier == NO_OBJECT_SUPPLIER) {
-            return new ResponseEntity<>(Map.of("status", "Unable to execute request as "
-                    + "datastore is not implemented."), HttpStatus.NOT_IMPLEMENTED);
-        }
         return executeTaskAsync(topicParamInQuery, requestId, taskSupplier);
     }
 
-    private ResponseEntity<Object> getRequestIdAndSendDataOperationRequestToDmiService(final String topicParamInQuery,
-                                                                                       final DataOperationRequest
-                                                                                       dataOperationRequest) {
-        final String requestId = UUID.randomUUID().toString();
-        sendDataOperationRequestAsynchronously(topicParamInQuery, dataOperationRequest, requestId);
-        return ResponseEntity.ok(Map.of("requestId", requestId));
-    }
+    protected abstract Supplier<Object> getTaskSupplierForGetRequest(final String datastoreName,
+                                                  final String cmHandleId,
+                                                  final String resourceIdentifier,
+                                                  final String optionsParamInQuery,
+                                                  final String topicParamInQuery,
+                                                  final String requestId,
+                                                  final boolean includeDescendant);
 
-    private void validateDataOperationRequest(final String topicParamInQuery,
-                                              final DataOperationRequest
-                                              dataOperationRequest) {
-        TopicValidator.validateTopicName(topicParamInQuery);
-        dataOperationRequest.getDataOperationDefinitions().forEach(dataOperationDetail -> {
-            if (OperationType.fromOperationName(dataOperationDetail.getOperation()) != READ) {
-                throw new OperationNotSupportedException(
-                        dataOperationDetail.getOperation() + " operation not yet supported");
-            } else if (DatastoreType.fromDatastoreName(dataOperationDetail.getDatastore()) == OPERATIONAL) {
-                throw new InvalidDatastoreException(dataOperationDetail.getDatastore()
-                        + " datastore is not supported");
-            }
-        });
-    }
 }
diff --git a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpPassthroughResourceRequestHandler.java b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpPassthroughResourceRequestHandler.java
index 0e49c6df13..8a3257576d 100644
--- a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpPassthroughResourceRequestHandler.java
+++ b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/NcmpPassthroughResourceRequestHandler.java
@@ -20,11 +20,21 @@
 
 package org.onap.cps.ncmp.rest.controller.handlers;
 
+import static org.onap.cps.ncmp.api.impl.operations.DatastoreType.OPERATIONAL;
+import static org.onap.cps.ncmp.api.impl.operations.OperationType.READ;
+
+import java.util.Map;
+import java.util.UUID;
 import java.util.function.Supplier;
 import org.onap.cps.ncmp.api.NetworkCmProxyDataService;
+import org.onap.cps.ncmp.api.impl.exception.InvalidDatastoreException;
+import org.onap.cps.ncmp.api.impl.operations.DatastoreType;
+import org.onap.cps.ncmp.api.impl.operations.OperationType;
 import org.onap.cps.ncmp.api.models.DataOperationRequest;
+import org.onap.cps.ncmp.rest.exceptions.OperationNotSupportedException;
 import org.onap.cps.ncmp.rest.executor.CpsNcmpTaskExecutor;
-import org.springframework.scheduling.annotation.Async;
+import org.onap.cps.ncmp.rest.util.TopicValidator;
+import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Component;
 
 @Component
@@ -32,6 +42,8 @@ public class NcmpPassthroughResourceRequestHandler extends NcmpDatastoreRequestH
 
     private final NetworkCmProxyDataService networkCmProxyDataService;
 
+    private static final Object noReturn = null;
+
     /**
      * Constructor.
      *
@@ -44,27 +56,71 @@ public class NcmpPassthroughResourceRequestHandler extends NcmpDatastoreRequestH
         this.networkCmProxyDataService = networkCmProxyDataService;
     }
 
+    /**
+     * Executes asynchronous request for group of cm handles to resource data.
+     *
+     * @param topicParamInQuery        the topic param in query
+     * @param dataOperationRequest     data operation request details for resource data
+     * @return the response entity
+     */
+    public ResponseEntity<Object> executeRequest(final String topicParamInQuery,
+                                                 final DataOperationRequest
+                                                     dataOperationRequest) {
+        validateDataOperationRequest(topicParamInQuery, dataOperationRequest);
+        if (!notificationFeatureEnabled) {
+            return ResponseEntity.ok(Map.of("status",
+                "Asynchronous request is unavailable as notification feature is currently disabled."));
+        }
+        return getRequestIdAndSendDataOperationRequestToDmiService(topicParamInQuery, dataOperationRequest);
+    }
+
     @Override
-    public Supplier<Object> getTaskSupplierForGetRequest(final String datastoreName,
-                                                         final String cmHandleId,
-                                                         final String resourceIdentifier,
-                                                         final String optionsParamInQuery,
-                                                         final String topicParamInQuery,
-                                                         final String requestId,
-                                                         final boolean includeDescendants) {
+    protected Supplier<Object> getTaskSupplierForGetRequest(final String datastoreName,
+                                                            final String cmHandleId,
+                                                            final String resourceIdentifier,
+                                                            final String optionsParamInQuery,
+                                                            final String topicParamInQuery,
+                                                            final String requestId,
+                                                            final boolean includeDescendants) {
 
         return () -> networkCmProxyDataService.getResourceDataForCmHandle(
-                datastoreName, cmHandleId, resourceIdentifier, optionsParamInQuery, topicParamInQuery, requestId);
+            datastoreName, cmHandleId, resourceIdentifier, optionsParamInQuery, topicParamInQuery, requestId);
     }
 
-    @Async
-    @Override
-    public void sendDataOperationRequestAsynchronously(final String topicParamInQuery,
-                                                       final DataOperationRequest
-                                                                   dataOperationRequest,
-                                                       final String requestId) {
-        networkCmProxyDataService.executeDataOperationForCmHandles(topicParamInQuery, dataOperationRequest,
-                requestId);
+    private ResponseEntity<Object> getRequestIdAndSendDataOperationRequestToDmiService(final String topicParamInQuery,
+                                                                                       final DataOperationRequest
+                                                                                           dataOperationRequest) {
+        final String requestId = UUID.randomUUID().toString();
+        cpsNcmpTaskExecutor.executeTask(
+            getTaskSupplierForDataOperationRequest(topicParamInQuery, dataOperationRequest, requestId),
+            timeOutInMilliSeconds);
+        return ResponseEntity.ok(Map.of("requestId", requestId));
+    }
 
+    private void validateDataOperationRequest(final String topicParamInQuery,
+                                              final DataOperationRequest
+                                                  dataOperationRequest) {
+        TopicValidator.validateTopicName(topicParamInQuery);
+        dataOperationRequest.getDataOperationDefinitions().forEach(dataOperationDetail -> {
+            if (OperationType.fromOperationName(dataOperationDetail.getOperation()) != READ) {
+                throw new OperationNotSupportedException(
+                    dataOperationDetail.getOperation() + " operation not yet supported");
+            } else if (DatastoreType.fromDatastoreName(dataOperationDetail.getDatastore()) == OPERATIONAL) {
+                throw new InvalidDatastoreException(dataOperationDetail.getDatastore()
+                    + " datastore is not supported");
+            }
+        });
     }
+
+    private Supplier<Object> getTaskSupplierForDataOperationRequest(final String topicParamInQuery,
+                                                                    final DataOperationRequest dataOperationRequest,
+                                                                    final String requestId) {
+        return () -> {
+            networkCmProxyDataService.executeDataOperationForCmHandles(topicParamInQuery,
+                dataOperationRequest,
+                requestId);
+            return noReturn;
+        };
+    }
+
 }
diff --git a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/TaskManagementDefaultHandler.java b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/TaskManagementDefaultHandler.java
deleted file mode 100644
index b2520b1609..0000000000
--- a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/controller/handlers/TaskManagementDefaultHandler.java
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- *  ============LICENSE_START=======================================================
- *  Copyright (C) 2023 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.ncmp.rest.controller.handlers;
-
-import java.util.function.Supplier;
-import org.onap.cps.ncmp.api.models.DataOperationRequest;
-import org.onap.cps.spi.FetchDescendantsOption;
-
-public interface TaskManagementDefaultHandler {
-
-    String NO_REQUEST_ID = null;
-    String NO_TOPIC = null;
-    Supplier<Object> NO_OBJECT_SUPPLIER = null;
-
-    default Supplier<Object> getTaskSupplierForGetRequest(final String datastoreName,
-                                                          final String cmHandleId,
-                                                          final String resourceIdentifier,
-                                                          final String optionsParamInQuery,
-                                                          final String topicParamInQuery,
-                                                          final String requestId,
-                                                          final boolean includeDescendant) {
-        return NO_OBJECT_SUPPLIER;
-    }
-
-    default Supplier<Object> getTaskSupplierForQueryRequest(final String cmHandleId,
-                                                            final String resourceIdentifier,
-                                                            final boolean includeDescendant) {
-        return NO_OBJECT_SUPPLIER;
-    }
-
-    default void sendDataOperationRequestAsynchronously(final String topicParamInQuery,
-                                                        final DataOperationRequest
-                                                                    dataOperationRequest,
-                                                        final String requestId) {
-    }
-
-    static FetchDescendantsOption getFetchDescendantsOption(final boolean includeDescendants) {
-        return includeDescendants ? FetchDescendantsOption.INCLUDE_ALL_DESCENDANTS
-                : FetchDescendantsOption.OMIT_DESCENDANTS;
-    }
-}
diff --git a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/executor/CpsNcmpTaskExecutor.java b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/executor/CpsNcmpTaskExecutor.java
index 0543c4fba3..ba68d5b757 100644
--- a/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/executor/CpsNcmpTaskExecutor.java
+++ b/cps-ncmp-rest/src/main/java/org/onap/cps/ncmp/rest/executor/CpsNcmpTaskExecutor.java
@@ -1,6 +1,6 @@
 /*
  *  ============LICENSE_START=======================================================
- *  Copyright (C) 2022 Nordix Foundation
+ *  Copyright (C) 2022-2023 Nordix Foundation
  *  ================================================================================
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -35,19 +35,19 @@ public class CpsNcmpTaskExecutor {
      * Execute a task asynchronously.
      *
      * @param taskSupplier functional method is get() task need to executed asynchronously
-     * @param timeOutInMillis the time out value in milliseconds
+     * @param timeOutInMillis the time-out value in milliseconds
      */
-    public void executeTask(final Supplier<Object> taskSupplier, final int timeOutInMillis) {
+    public void executeTask(final Supplier<Object> taskSupplier, final long timeOutInMillis) {
         CompletableFuture.supplyAsync(taskSupplier::get)
             .orTimeout(timeOutInMillis, MILLISECONDS)
-            .whenCompleteAsync((responseAsJson, throwable) -> handleTaskCompletion(throwable));
+            .whenCompleteAsync((taskResult, throwable) -> handleTaskCompletion(throwable));
     }
 
     private void handleTaskCompletion(final Throwable throwable) {
         if (throwable == null) {
             log.info("Async task completed successfully.");
         } else {
-            log.error("Async task failed. caused by : {}", throwable.getMessage());
+            log.error("Async task failed. caused by : {}", throwable.toString());
         }
     }
 }