Improve usability of the HttpProjectConfigManager. (#301)

* Prevent NPE in HttpProjectConfigManager.Builder.
* Move addNotificationHandler into NotificationCenter

Author: Mike Davis

core-api/src/main/java/com/optimizely/ab/Optimizely.java

@@ -971,20 +971,12 @@ public int addUpdateConfigNotificationHandler(NotificationHandler<UpdateConfigNo
     }
 
     /**
-     * Convenience method for adding UpdateConfigNotification Handlers
+     * Convenience method for adding NotificationHandlers
      */
-    private <T> int addNotificationHandler(Class<T> clazz, NotificationHandler<T> handler) {
-        NotificationManager<T> notificationManager = notificationCenter.getNotificationManager(clazz);
-
-        if (notificationManager == null) {
-            logger.warn("{} not supported by the NotificationCenter.", clazz);
-            return -1;
-        }
-
-        return notificationManager.addHandler(handler);
+    public <T> int addNotificationHandler(Class<T> clazz, NotificationHandler<T> handler) {
+        return notificationCenter.addNotificationHandler(clazz, handler);
     }
 
-
     //======== Builder ========//
     @Deprecated
     public static Builder builder(@Nonnull String datafile,
@@ -1062,6 +1054,11 @@ public Builder withConfigManager(ProjectConfigManager projectConfigManager) {
             return this;
         }
 
+        public Builder withNotificationCenter(NotificationCenter notificationCenter) {
+            this.notificationCenter = notificationCenter;
+            return this;
+        }
+
         // Helper function for making testing easier
         protected Builder withBucketing(Bucketer bucketer) {
             this.bucketer = bucketer;
@@ -1083,11 +1080,6 @@ protected Builder withEventBuilder(EventFactory eventFactory) {
             return this;
         }
 
-        protected Builder withNotificationCenter(NotificationCenter notificationCenter) {
-            this.notificationCenter = notificationCenter;
-            return this;
-        }
-
         public Optimizely build() {
 
             if (clientEngine == null) {

core-api/src/main/java/com/optimizely/ab/notification/NotificationCenter.java

@@ -104,6 +104,17 @@ public <T> NotificationManager<T> getNotificationManager(Class clazz) {
         return notifierMap.get(clazz);
     }
 
+    public <T> int addNotificationHandler(Class<T> clazz, NotificationHandler<T> handler) {
+        NotificationManager<T> notificationManager = getNotificationManager(clazz);
+
+        if (notificationManager == null) {
+            logger.warn("{} not supported by the NotificationCenter.", clazz);
+            return -1;
+        }
+
+        return notificationManager.addHandler(handler);
+    }
+
     /**
      * Convenience method to support lambdas as callbacks in later version of Java (8+).
      *

core-api/src/test/java/com/optimizely/ab/notification/NotificationCenterTest.java

@@ -28,7 +28,6 @@
 import org.junit.Test;
 
 import javax.annotation.Nonnull;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
@@ -64,7 +63,6 @@ public void testAddWrongTrackNotificationListener() {
         logbackVerifier.expectMessage(Level.WARN, "Notification listener was the wrong type. It was not added to the notification center.");
         assertEquals(-1, notificationId);
         assertFalse(notificationCenter.removeNotificationListener(notificationId));
-
     }
 
     @Test
@@ -120,7 +118,7 @@ public void testAddDecisionNotification() {
         NotificationManager<DecisionNotification> manager = notificationCenter.getNotificationManager(DecisionNotification.class);
         int notificationId = manager.addHandler(decisionNotification -> { });
         assertNotSame(-1, notificationId);
-        assertTrue(notificationCenter.removeNotificationListener(notificationId));
+        assertTrue(manager.remove(notificationId));
     }
 
     @Test
@@ -174,6 +172,20 @@ public void onActivate(@Nonnull Experiment experiment, @Nonnull String userId, @
         assertTrue(notificationCenter.removeNotificationListener(notificationId));
     }
 
+    @Test
+    public void testAddValidNotificationHandler() {
+        assertEquals(1, notificationCenter.addNotificationHandler(ActivateNotification.class, x -> {}));
+        assertEquals(2, notificationCenter.addNotificationHandler(DecisionNotification.class, x -> {}));
+        assertEquals(3, notificationCenter.addNotificationHandler(TrackNotification.class, x -> {}));
+        assertEquals(4, notificationCenter.addNotificationHandler(UpdateConfigNotification.class, x -> {}));
+    }
+
+    @Test
+    public void testAddInvalidNotificationHandler() {
+        int actual = notificationCenter.addNotificationHandler(Integer.class, i -> {});
+        assertEquals(-1, actual);
+    }
+
     @Test
     @Deprecated
     public void testClearNotificationByActivateType() {
@@ -232,6 +244,5 @@ private void testSendWithNotification(Object notification) {
         List messages = handler.getMessages();
         assertEquals(1, messages.size());
         assertEquals(notification, messages.get(0));
-
     }
 }

core-httpclient-impl/src/main/java/com/optimizely/ab/OptimizelyFactory.java

@@ -66,7 +66,7 @@ public static void setEventQueueParams(int queueCapacity, int numberWorkers) {
 
     /**
      * Convenience method for setting the blocking timeout.
-     * {@link HttpProjectConfigManager.Builder#withBlockingTimeout(long, TimeUnit)}
+     * {@link HttpProjectConfigManager.Builder#withBlockingTimeout(Long, TimeUnit)}
      */
     public static void setBlockingTimeout(long blockingDuration, TimeUnit blockingTimeout) {
         if (blockingTimeout == null) {
@@ -85,7 +85,7 @@ public static void setBlockingTimeout(long blockingDuration, TimeUnit blockingTi
 
     /**
      * Convenience method for setting the polling interval on System properties.
-     * {@link HttpProjectConfigManager.Builder#withPollingInterval(long, TimeUnit)}
+     * {@link HttpProjectConfigManager.Builder#withPollingInterval(Long, TimeUnit)}
      */
     public static void setPollingInterval(long pollingDuration, TimeUnit pollingTimeout) {
         if (pollingTimeout == null) {

core-httpclient-impl/src/main/java/com/optimizely/ab/config/HttpProjectConfigManager.java

@@ -174,12 +174,17 @@ public Builder withOptimizelyHttpClient(OptimizelyHttpClient httpClient) {
          * PollingProjectConfigManager will begin returning null immediately until the call to Poll
          * succeeds.
          */
-        public Builder withBlockingTimeout(long period, TimeUnit timeUnit) {
+        public Builder withBlockingTimeout(Long period, TimeUnit timeUnit) {
             if (timeUnit == null) {
                 logger.warn("TimeUnit cannot be null. Keeping default period: {} and time unit: {}", this.blockingTimeoutPeriod, this.blockingTimeoutUnit);
                 return this;
             }
 
+            if (period == null) {
+                logger.warn("Timeout cannot be null. Keeping default period: {} and time unit: {}", this.blockingTimeoutPeriod, this.blockingTimeoutUnit);
+                return this;
+            }
+
             if (period <= 0) {
                 logger.warn("Timeout cannot be <= 0. Keeping default period: {} and time unit: {}", this.blockingTimeoutPeriod, this.blockingTimeoutUnit);
                 return this;
@@ -191,12 +196,17 @@ public Builder withBlockingTimeout(long period, TimeUnit timeUnit) {
             return this;
         }
 
-        public Builder withPollingInterval(long period, TimeUnit timeUnit) {
+        public Builder withPollingInterval(Long period, TimeUnit timeUnit) {
             if (timeUnit == null) {
                 logger.warn("TimeUnit cannot be null. Keeping default period: {} and time unit: {}", this.period, this.timeUnit);
                 return this;
             }
 
+            if (period == null) {
+                logger.warn("Interval cannot be null. Keeping default period: {} and time unit: {}", this.period, this.timeUnit);
+                return this;
+            }
+
             if (period <= 0) {
                 logger.warn("Interval cannot be <= 0. Keeping default period: {} and time unit: {}", this.period, this.timeUnit);
                 return this;

core-httpclient-impl/src/test/java/com/optimizely/ab/config/HttpProjectConfigManagerTest.java

@@ -147,15 +147,19 @@ public void testInvalidPollingInterval() {
         long expectedPeriod = builder.period;
         TimeUnit expectedTimeUnit = builder.timeUnit;
 
-        builder.withPollingInterval(-1, SECONDS);
+        builder.withPollingInterval(null, SECONDS);
         assertEquals(expectedPeriod, builder.period);
         assertEquals(expectedTimeUnit, builder.timeUnit);
 
-        builder.withPollingInterval(10, null);
+        builder.withPollingInterval(-1L, SECONDS);
         assertEquals(expectedPeriod, builder.period);
         assertEquals(expectedTimeUnit, builder.timeUnit);
 
-        builder.withPollingInterval(10, SECONDS);
+        builder.withPollingInterval(10L, null);
+        assertEquals(expectedPeriod, builder.period);
+        assertEquals(expectedTimeUnit, builder.timeUnit);
+
+        builder.withPollingInterval(10L, SECONDS);
         assertEquals(10, builder.period);
         assertEquals(SECONDS, builder.timeUnit);
     }
@@ -166,15 +170,19 @@ public void testInvalidBlockingTimeout() {
         long expectedPeriod = builder.blockingTimeoutPeriod;
         TimeUnit expectedTimeUnit = builder.blockingTimeoutUnit;
 
-        builder.withBlockingTimeout(-1, SECONDS);
+        builder.withBlockingTimeout(null, SECONDS);
+        assertEquals(expectedPeriod, builder.blockingTimeoutPeriod);
+        assertEquals(expectedTimeUnit, builder.blockingTimeoutUnit);
+
+        builder.withBlockingTimeout(-1L, SECONDS);
         assertEquals(expectedPeriod, builder.blockingTimeoutPeriod);
         assertEquals(expectedTimeUnit, builder.blockingTimeoutUnit);
 
-        builder.withBlockingTimeout(10, null);
+        builder.withBlockingTimeout(10L, null);
         assertEquals(expectedPeriod, builder.blockingTimeoutPeriod);
         assertEquals(expectedTimeUnit, builder.blockingTimeoutUnit);
 
-        builder.withBlockingTimeout(10, SECONDS);
+        builder.withBlockingTimeout(10L, SECONDS);
         assertEquals(10, builder.blockingTimeoutPeriod);
         assertEquals(SECONDS, builder.blockingTimeoutUnit);
     }
@@ -243,7 +251,7 @@ public void testInvalidPayload() throws Exception {
         projectConfigManager = builder()
             .withOptimizelyHttpClient(mockHttpClient)
             .withSdkKey("sdk-key")
-            .withBlockingTimeout(10, TimeUnit.MILLISECONDS)
+            .withBlockingTimeout(10L, TimeUnit.MILLISECONDS)
             .build();
 
         assertNull(projectConfigManager.getConfig());