Improved notification on item archive/restore

Addressed code review comments, added unit-tests.

Change-Id: I2a74e9969540cb636658aa012f82a81e0fd2eac2
Issue-ID: SDC-1667
Signed-off-by: vempo <vitaliy.emporopulo@amdocs.com>

6 files changed, 186 insertions, 37 deletions

diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/ItemsImpl.java b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/ItemsImpl.java
index a93c063220..265570af4e 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/ItemsImpl.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/ItemsImpl.java
@@ -67,6 +67,8 @@ import static org.openecomp.sdc.versioning.VersioningNotificationConstansts.ITEM
 @Validated
 public class ItemsImpl implements Items {
 
+    private static final String ONBOARDING_METHOD = "onboardingMethod";
+
     private ItemManager itemManager = ItemManagerFactory.getInstance().createInterface();
 
     private static ActivityLogManager activityLogManager = ActivityLogManagerFactory.getInstance().createInterface();
@@ -92,8 +94,6 @@ public class ItemsImpl implements Items {
                 .put(ItemAction.RESTORE, new ActionSideAffects(ActivityType.Restore, NotificationEventTypes.RESTORE));
     }
 
-    private static final String ONBOARDING_METHOD = "onboardingMethod";
-
 
     @Override
     public Response actOn(ItemActionRequestDto request, String itemId, String user) {
@@ -115,10 +115,10 @@ public class ItemsImpl implements Items {
 
         actionSideAffectsMap.get(request.getAction()).execute(item, user);
         try {
-            Notifier notifier = NotifierFactory.getInstance();
-            notifier.execute(Collections.singleton(itemId), request.getAction());
+            Notifier catalogNotifier = NotifierFactory.getInstance();
+            catalogNotifier.execute(Collections.singleton(itemId), request.getAction());
         } catch (Exception e) {
-            LOGGER.error("Failed to send catalog notification on item " + itemId + " Error: " + e.getMessage());
+            LOGGER.error("Failed to send catalog notification on item {}", itemId, e);
         }
 
         return Response.ok().build();
@@ -222,7 +222,7 @@ public class ItemsImpl implements Items {
             try {
                 notifier.notifySubscribers(syncEvent, userName);
             } catch (Exception e) {
-                LOGGER.error("Failed to send sync notification to users subscribed to item '" + itemId);
+                LOGGER.error("Failed to send sync notification to users subscribed to item '{}'", itemId, e);
             }
         }
     }
diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifier.java b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifier.java
index 82880106d7..872c61e480 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifier.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifier.java
@@ -17,10 +17,12 @@
 package org.openecomp.sdcrests.item.rest.services.catalog.notification;
 
 import java.util.Collection;
+import java.util.Objects;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.BiFunction;
 import org.openecomp.sdc.logging.api.Logger;
 import org.openecomp.sdc.logging.api.LoggerFactory;
@@ -41,9 +43,19 @@ public class AsyncNotifier implements Notifier {
     private static final long DEFAULT_INTERVAL = 5000;
 
     private final BiFunction<Collection<String>, ItemAction, Callable<NextAction>> taskProducer;
+    private final int numberOfRetries;
+    private final long retryInterval;
+
 
     AsyncNotifier(BiFunction<Collection<String>, ItemAction, Callable<NextAction>> taskProducer) {
+        this(taskProducer, DEFAULT_NUM_OF_RETRIES, DEFAULT_INTERVAL);
+    }
+
+    AsyncNotifier(BiFunction<Collection<String>, ItemAction, Callable<NextAction>> taskProducer, int numOfRetries,
+            long retryInterval) {
         this.taskProducer = taskProducer;
+        this.numberOfRetries = numOfRetries;
+        this.retryInterval = retryInterval;
     }
 
     @Override
@@ -51,8 +63,7 @@ public class AsyncNotifier implements Notifier {
 
         Callable<AsyncNotifier.NextAction> worker = taskProducer.apply(itemIds, action);
 
-        RetryingTask retryingTask =
-                new RetryingTask(worker, DEFAULT_NUM_OF_RETRIES, DEFAULT_INTERVAL, EXECUTOR_SERVICE);
+        RetryingTask retryingTask = new RetryingTask(worker, numberOfRetries, retryInterval, EXECUTOR_SERVICE);
 
         EXECUTOR_SERVICE.submit(LoggingContext.copyToCallable(retryingTask));
     }
@@ -68,19 +79,37 @@ public class AsyncNotifier implements Notifier {
         private final Callable<AsyncNotifier.NextAction> worker;
         private final long delay;
         private final ScheduledExecutorService scheduler;
-        private volatile int retries;
+        private final AtomicInteger retries;
 
         RetryingTask(Callable<AsyncNotifier.NextAction> worker, int numOfRetries, long delay,
                 ScheduledExecutorService scheduler) {
 
-            this.worker = worker;
-            this.retries = numOfRetries;
-            this.delay = delay;
-            this.scheduler = scheduler;
+            this.worker = Objects.requireNonNull(worker);
+            this.retries = new AtomicInteger(requirePositiveRetries(numOfRetries));
+            this.delay = requirePositiveDelay(delay);
+            this.scheduler = Objects.requireNonNull(scheduler);
+        }
+
+        private int requirePositiveRetries(int number) {
+
+            if (number < 1) {
+                throw new IllegalArgumentException("Number of retries must be positive");
+            }
+
+            return number;
+        }
+
+        private long requirePositiveDelay(long number) {
+
+            if (number < 1) {
+                throw new IllegalArgumentException("Delay must be positive");
+            }
+
+            return number;
         }
 
         @Override
-        public synchronized Void call() throws Exception {
+        public Void call() throws Exception {
 
             NextAction next = worker.call();
             if (next == NextAction.DONE) {
@@ -88,8 +117,8 @@ public class AsyncNotifier implements Notifier {
                 return null;
             }
 
-            retries--;
-            if (retries == 0) {
+            int attempts = retries.decrementAndGet();
+            if (attempts < 1) {
                 LOGGER.warn("Exhausted number of retries for task {}, exiting", worker);
                 return null;
             }
diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/NotifierFactory.java b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/NotifierFactory.java
index a7f1e9c7fb..462ffdf00c 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/NotifierFactory.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/NotifierFactory.java
@@ -43,6 +43,10 @@ public class NotifierFactory {
 
     private static final Notifier INSTANCE;
 
+    private NotifierFactory() {
+        // prevent instantiation
+    }
+
     static {
         INSTANCE = createInstance();
     }
diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducer.java b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducer.java
index c6abd346ff..d210dc21af 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducer.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/main/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducer.java
@@ -17,6 +17,8 @@
 package org.openecomp.sdcrests.item.rest.services.catalog.notification.http;
 
 import java.util.Collection;
+import java.util.EnumMap;
+import java.util.Map;
 import java.util.concurrent.Callable;
 import java.util.function.BiFunction;
 import org.openecomp.sdc.common.session.SessionContextProviderFactory;
@@ -40,6 +42,13 @@ public class HttpTaskProducer
 
     private static final String CATALOG_HTTP_PROTOCOL = "HTTP";
     private static final String CATALOG_HTTPS_PROTOCOL = "HTTPS";
+    private static final Map<ItemAction, String> ACTION_PATHS;
+
+    static {
+        ACTION_PATHS = new EnumMap<>(ItemAction.class);
+        ACTION_PATHS.put(ItemAction.ARCHIVE, "archived");
+        ACTION_PATHS.put(ItemAction.RESTORE, "restored");
+    }
 
     private final String notifyCatalogUrl;
 
@@ -81,15 +90,21 @@ public class HttpTaskProducer
         return createNotificationTask(itemIds, action);
     }
 
-    private static String getEndpoint(ItemAction action) {
+    private HttpNotificationTask createNotificationTask(Collection<String> itemIds, ItemAction action) {
+        String userId = SessionContextProviderFactory.getInstance().createInterface().get().getUser().getUserId();
+        String notificationEndpoint = notifyCatalogUrl + getApiPath(action);
+        LOGGER.debug("Catalog notification URL: {}", notificationEndpoint);
+        return new HttpNotificationTask(notificationEndpoint, userId, itemIds);
+    }
 
-        if (action == ItemAction.ARCHIVE) {
-            return "archived";
-        } else if (action == ItemAction.RESTORE) {
-            return "restored";
-        } else {
+    static String getApiPath(ItemAction action) {
+
+        String path = ACTION_PATHS.get(action);
+        if (path == null) {
             throw new IllegalArgumentException("Unsupported action: " + action.name());
         }
+
+        return path;
     }
 
     @Override
@@ -98,10 +113,4 @@ public class HttpTaskProducer
         task.call();
     }
 
-    private HttpNotificationTask createNotificationTask(Collection<String> itemIds, ItemAction action) {
-        String userId = SessionContextProviderFactory.getInstance().createInterface().get().getUser().getUserId();
-        String notificationEndpoint = notifyCatalogUrl + getEndpoint(action);
-        LOGGER.debug("Catalog notification URL: " + notificationEndpoint);
-        return new HttpNotificationTask(notificationEndpoint, userId, itemIds);
-    }
 }
diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifierTest.java b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifierTest.java
index 900fc940ee..6bfa8b24b5 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifierTest.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/AsyncNotifierTest.java
@@ -17,18 +17,29 @@
 package org.openecomp.sdcrests.item.rest.services.catalog.notification;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.openecomp.sdcrests.item.rest.services.catalog.notification.AsyncNotifier.NextAction.DONE;
 import static org.openecomp.sdcrests.item.rest.services.catalog.notification.AsyncNotifier.NextAction.RETRY;
 
+import java.util.Collection;
+import java.util.Collections;
+import java.util.UUID;
 import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.function.BiFunction;
 import org.apache.commons.lang.mutable.MutableInt;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.mockito.Mockito;
 import org.mockito.stubbing.Answer;
+import org.openecomp.sdcrests.item.types.ItemAction;
 
 /**
  * @author evitaliy
@@ -36,6 +47,50 @@ import org.mockito.stubbing.Answer;
  */
 public class AsyncNotifierTest {
 
+    private static final String NUMBER_OF_RETRIES_MESSAGE = "Number of retries must be positive";
+    private static final String DELAY_MESSAGE = "Delay must be positive";
+
+    @Rule
+    public ExpectedException exception = ExpectedException.none();
+
+    @Test(expected = NullPointerException.class)
+    public void errorWhenWorkerNull() {
+        new AsyncNotifier.RetryingTask(null, 10, 10, Mockito.mock(ScheduledExecutorService.class));
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void errorWhenSchedulerServiceNull() {
+        new AsyncNotifier.RetryingTask(() -> DONE, 10, 10, null);
+    }
+
+    @Test
+    public void errorWhenNumberOfRetriesNegative() {
+        exception.expect(IllegalArgumentException.class);
+        exception.expectMessage(NUMBER_OF_RETRIES_MESSAGE);
+        new AsyncNotifier.RetryingTask(() -> DONE, -12, 10, Mockito.mock(ScheduledExecutorService.class));
+    }
+
+    @Test
+    public void errorWhenNumberOfRetriesZero() {
+        exception.expect(IllegalArgumentException.class);
+        exception.expectMessage(NUMBER_OF_RETRIES_MESSAGE);
+        new AsyncNotifier.RetryingTask(() -> DONE, 0, 10, Mockito.mock(ScheduledExecutorService.class));
+    }
+
+    @Test
+    public void errorWhenDelayNegative() {
+        exception.expect(IllegalArgumentException.class);
+        exception.expectMessage(DELAY_MESSAGE);
+        new AsyncNotifier.RetryingTask(() -> DONE, 1, -77, Mockito.mock(ScheduledExecutorService.class));
+    }
+
+    @Test
+    public void errorWhenDelayZero() {
+        exception.expect(IllegalArgumentException.class);
+        exception.expectMessage(DELAY_MESSAGE);
+        new AsyncNotifier.RetryingTask(() -> DONE, 1, 0, Mockito.mock(ScheduledExecutorService.class));
+    }
+
     @Test
     public void taskRunsOnceWhenSuccessful() throws Exception {
 
@@ -53,6 +108,20 @@ public class AsyncNotifierTest {
         assertEquals(1, counter.intValue());
     }
 
+    private ScheduledExecutorService createMockScheduledExecutor() {
+
+        ScheduledExecutorService executorServiceMock = Mockito.mock(ScheduledExecutorService.class);
+        Answer passThrough = invocation -> {
+            ((Callable<?>) invocation.getArgument(0)).call();
+            return null;
+        };
+
+        Mockito.doAnswer(passThrough).when(executorServiceMock).submit(any(Callable.class));
+        Mockito.doAnswer(passThrough).when(executorServiceMock)
+                .schedule(any(Callable.class), anyLong(), any(TimeUnit.class));
+        return executorServiceMock;
+    }
+
     @Test
     public void taskRunsTwiceWhenFailedFirstTime() throws Exception {
 
@@ -88,17 +157,26 @@ public class AsyncNotifierTest {
         assertEquals(numOfRetries, counter.intValue());
     }
 
-    private ScheduledExecutorService createMockScheduledExecutor() {
+    @Test
+    public void workerExecutedWithGivenItemIdsAndAction()
+            throws InterruptedException, ExecutionException, TimeoutException {
 
-        ScheduledExecutorService executorServiceMock = Mockito.mock(ScheduledExecutorService.class);
-        Answer passThrough = invocation -> {
-            ((Callable<?>) invocation.getArgument(0)).call();
-            return null;
+        CompletableFuture<Boolean> completed = new CompletableFuture<>();
+        Callable<AsyncNotifier.NextAction> mockTask = () -> {
+            completed.complete(true);
+            return DONE;
         };
 
-        Mockito.doAnswer(passThrough).when(executorServiceMock).submit(any(Callable.class));
-        Mockito.doAnswer(passThrough).when(executorServiceMock)
-                .schedule(any(Callable.class), anyLong(), any(TimeUnit.class));
-        return executorServiceMock;
+        final Collection<String> itemIds = Collections.singleton(UUID.randomUUID().toString());
+        final ItemAction action = ItemAction.RESTORE;
+
+        BiFunction<Collection<String>, ItemAction, Callable<AsyncNotifier.NextAction>> mockProducer = (i, a) -> {
+            assertEquals(itemIds, i);
+            assertEquals(action, a);
+            return mockTask;
+        };
+
+        new AsyncNotifier(mockProducer, 1, 1).execute(itemIds, action);
+        assertTrue(completed.get(5, TimeUnit.SECONDS));
     }
 }
\ No newline at end of file
diff --git a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducerTest.java b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducerTest.java
index 3c12b37e6d..4d8a1103b8 100644
--- a/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducerTest.java
+++ b/openecomp-be/api/openecomp-sdc-rest-webapp/item-rest/item-rest-services/src/test/java/org/openecomp/sdcrests/item/rest/services/catalog/notification/http/HttpTaskProducerTest.java
@@ -17,11 +17,16 @@
 package org.openecomp.sdcrests.item.rest.services.catalog.notification.http;
 
 import static org.hamcrest.CoreMatchers.containsString;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 
+import java.util.HashSet;
+import java.util.Set;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import org.openecomp.sdcrests.item.rest.services.catalog.notification.EntryNotConfiguredException;
+import org.openecomp.sdcrests.item.types.ItemAction;
 
 /**
  * @author evitaliy
@@ -33,6 +38,30 @@ public class HttpTaskProducerTest {
     public ExpectedException exception = ExpectedException.none();
 
     @Test
+    public void uniquePathExistsForEveryAction() {
+
+        ItemAction[] availableActions = ItemAction.values();
+        Set<String> collectedPaths = new HashSet<>(availableActions.length);
+        for (ItemAction action : availableActions) {
+            String path = HttpTaskProducer.getApiPath(action);
+            assertFalse("Path empty for action '" + action.name() + "'", path == null || path.isEmpty());
+            collectedPaths.add(path);
+        }
+
+        assertEquals("Paths not unique for some actions", availableActions.length, collectedPaths.size());
+    }
+
+    @Test
+    public void restorePathEqualsRestored() {
+        assertEquals("restored", HttpTaskProducer.getApiPath(ItemAction.RESTORE));
+    }
+
+    @Test
+    public void archivePathEqualsArchived() {
+        assertEquals("archived", HttpTaskProducer.getApiPath(ItemAction.ARCHIVE));
+    }
+
+    @Test
     public void errorWhenProtocolNotDefined() {
         HttpConfiguration config = mockConfiguration();
         config.setCatalogBeProtocol(null);