Fixing how project config is updated. Also, fixing issue in Optimizely.close() (#309)

aliabbasrizvi · Mike Davis · commit 464370f828ec · 2019-06-26T13:40:36.000-07:00
diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java
@@ -120,15 +120,15 @@ public boolean isValid() {
     }
 
     /**
-     * Helper method which checks if Object is an instance of Closeable and calls close() on it.
+     * Helper method which checks if Object is an instance of AutoCloseable and calls close() on it.
      */
     private void tryClose(Object obj) {
-        if (!(obj instanceof Closeable)) {
+        if (!(obj instanceof AutoCloseable)) {
             return;
         }
 
         try {
-            ((Closeable) obj).close();
+            ((AutoCloseable) obj).close();
         } catch (Exception e) {
             logger.warn("Unexpected exception on trying to close {}.", obj);
         }
@@ -214,7 +214,7 @@ private Variation activate(@Nullable ProjectConfig projectConfig,
         }
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
         // bucket the user to the given experiment and dispatch an impression event
-        Variation variation = getVariation(experiment, userId, copiedAttributes, projectConfig);
+        Variation variation = getVariation(projectConfig, experiment, userId, copiedAttributes);
         if (variation == null) {
             logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
             return null;
@@ -370,6 +370,20 @@ public Boolean isFeatureEnabled(@Nonnull String featureKey,
     public Boolean isFeatureEnabled(@Nonnull String featureKey,
                                     @Nonnull String userId,
                                     @Nonnull Map<String, ?> attributes) {
+        ProjectConfig projectConfig = getProjectConfig();
+        if (projectConfig == null) {
+            logger.error("Optimizely instance is not valid, failing isFeatureEnabled call.");
+            return false;
+        }
+
+        return isFeatureEnabled(projectConfig, featureKey, userId, attributes);
+    }
+
+    @Nonnull
+    private Boolean isFeatureEnabled(@Nonnull ProjectConfig projectConfig,
+                                     @Nonnull String featureKey,
+                                     @Nonnull String userId,
+                                     @Nonnull Map<String, ?> attributes) {
         if (featureKey == null) {
             logger.warn("The featureKey parameter must be nonnull.");
             return false;
@@ -378,12 +392,6 @@ public Boolean isFeatureEnabled(@Nonnull String featureKey,
             return false;
         }
 
-        ProjectConfig projectConfig = getProjectConfig();
-        if (projectConfig == null) {
-            logger.error("Optimizely instance is not valid, failing isFeatureEnabled call.");
-            return false;
-        }
-
         FeatureFlag featureFlag = projectConfig.getFeatureKeyMapping().get(featureKey);
         if (featureFlag == null) {
             logger.info("No feature flag was found for key \"{}\".", featureKey);
@@ -753,7 +761,7 @@ public List<String> getEnabledFeatures(@Nonnull String userId, @Nonnull Map<Stri
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
         for (FeatureFlag featureFlag : projectConfig.getFeatureFlags()) {
             String featureKey = featureFlag.getKey();
-            if (isFeatureEnabled(featureKey, userId, copiedAttributes))
+            if (isFeatureEnabled(projectConfig, featureKey, userId, copiedAttributes))
                 enabledFeaturesList.add(featureKey);
         }
 
@@ -773,20 +781,20 @@ public Variation getVariation(@Nonnull Experiment experiment,
     public Variation getVariation(@Nonnull Experiment experiment,
                                   @Nonnull String userId,
                                   @Nonnull Map<String, ?> attributes) throws UnknownExperimentException {
-        return getVariation(experiment, userId, attributes, getProjectConfig());
+        return getVariation(getProjectConfig(), experiment, userId, attributes);
     }
 
     @Nullable
-    public Variation getVariation(@Nonnull Experiment experiment,
-                                  @Nonnull String userId,
-                                  @Nonnull Map<String, ?> attributes,
-                                  @Nonnull ProjectConfig projectConfig) throws UnknownExperimentException {
+    private Variation getVariation(@Nonnull ProjectConfig projectConfig,
+                                   @Nonnull Experiment experiment,
+                                   @Nonnull String userId,
+                                   @Nonnull Map<String, ?> attributes) throws UnknownExperimentException {
         Map<String, ?> copiedAttributes = copyAttributes(attributes);
         Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes, projectConfig);
 
         String notificationType = NotificationCenter.DecisionNotificationType.AB_TEST.toString();
 
-        if (getProjectConfig().getExperimentFeatureKeyMapping().get(experiment.getId()) != null) {
+        if (projectConfig.getExperimentFeatureKeyMapping().get(experiment.getId()) != null) {
             notificationType = NotificationCenter.DecisionNotificationType.FEATURE_TEST.toString();
         }
 
@@ -835,7 +843,7 @@ public Variation getVariation(@Nonnull String experimentKey,
             return null;
         }
 
-        return getVariation(experiment, userId, attributes, projectConfig);
+        return getVariation(projectConfig, experiment, userId, attributes);
     }
 
     /**
diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
@@ -145,7 +145,28 @@ public OptimizelyTest(String validDatafile, String noAudienceDatafile) throws Co
     }
 
     @Test
-    public void testClose() throws IOException {
+    public void testClose() throws Exception {
+        // Check for AutoCloseable
+        EventHandler mockAutoCloseableEventHandler = mock(
+            EventHandler.class,
+            withSettings().extraInterfaces(AutoCloseable.class)
+        );
+        ProjectConfigManager mockAutoCloseableProjectConfigManager = mock(
+            ProjectConfigManager.class,
+            withSettings().extraInterfaces(AutoCloseable.class)
+        );
+
+        Optimizely optimizely = Optimizely.builder()
+            .withEventHandler(mockAutoCloseableEventHandler)
+            .withConfigManager(mockAutoCloseableProjectConfigManager)
+            .build();
+
+        optimizely.close();
+
+        verify((AutoCloseable) mockAutoCloseableEventHandler).close();
+        verify((AutoCloseable) mockAutoCloseableProjectConfigManager).close();
+
+        // Check for Closeable
         EventHandler mockCloseableEventHandler = mock(
             EventHandler.class,
             withSettings().extraInterfaces(Closeable.class)
@@ -155,7 +176,7 @@ public void testClose() throws IOException {
             withSettings().extraInterfaces(Closeable.class)
         );
 
-        Optimizely optimizely = Optimizely.builder()
+        optimizely = Optimizely.builder()
             .withEventHandler(mockCloseableEventHandler)
             .withConfigManager(mockCloseableProjectConfigManager)
             .build();
@@ -167,44 +188,44 @@ public void testClose() throws IOException {
     }
 
     @Test
-    public void testCloseConfigManagerThrowsException() throws IOException {
-        EventHandler mockCloseableEventHandler = mock(
+    public void testCloseConfigManagerThrowsException() throws Exception {
+        EventHandler mockAutoCloseableEventHandler = mock(
             EventHandler.class,
-            withSettings().extraInterfaces(Closeable.class)
+            withSettings().extraInterfaces(AutoCloseable.class)
         );
-        ProjectConfigManager mockCloseableProjectConfigManager = mock(
+        ProjectConfigManager mockAutoCloseableProjectConfigManager = mock(
             ProjectConfigManager.class,
-            withSettings().extraInterfaces(Closeable.class)
+            withSettings().extraInterfaces(AutoCloseable.class)
         );
 
         Optimizely optimizely = Optimizely.builder()
-            .withEventHandler(mockCloseableEventHandler)
-            .withConfigManager(mockCloseableProjectConfigManager)
+            .withEventHandler(mockAutoCloseableEventHandler)
+            .withConfigManager(mockAutoCloseableProjectConfigManager)
             .build();
 
-        doThrow(new IOException()).when((Closeable) mockCloseableProjectConfigManager).close();
-        logbackVerifier.expectMessage(Level.WARN, "Unexpected exception on trying to close " + mockCloseableProjectConfigManager + ".");
+        doThrow(new IOException()).when((AutoCloseable) mockAutoCloseableProjectConfigManager).close();
+        logbackVerifier.expectMessage(Level.WARN, "Unexpected exception on trying to close " + mockAutoCloseableProjectConfigManager + ".");
         optimizely.close();
     }
 
     @Test
-    public void testCloseEventHandlerThrowsException() throws IOException {
-        EventHandler mockCloseableEventHandler = mock(
+    public void testCloseEventHandlerThrowsException() throws Exception {
+        EventHandler mockAutoCloseableEventHandler = mock(
             EventHandler.class,
-            withSettings().extraInterfaces(Closeable.class)
+            withSettings().extraInterfaces(AutoCloseable.class)
         );
-        ProjectConfigManager mockCloseableProjectConfigManager = mock(
+        ProjectConfigManager mockAutoCloseableProjectConfigManager = mock(
             ProjectConfigManager.class,
-            withSettings().extraInterfaces(Closeable.class)
+            withSettings().extraInterfaces(AutoCloseable.class)
         );
 
         Optimizely optimizely = Optimizely.builder()
-            .withEventHandler(mockCloseableEventHandler)
-            .withConfigManager(mockCloseableProjectConfigManager)
+            .withEventHandler(mockAutoCloseableEventHandler)
+            .withConfigManager(mockAutoCloseableProjectConfigManager)
             .build();
 
-        doThrow(new IOException()).when((Closeable) mockCloseableEventHandler).close();
-        logbackVerifier.expectMessage(Level.WARN, "Unexpected exception on trying to close " + mockCloseableEventHandler + ".");
+        doThrow(new IOException()).when((AutoCloseable) mockAutoCloseableEventHandler).close();
+        logbackVerifier.expectMessage(Level.WARN, "Unexpected exception on trying to close " + mockAutoCloseableEventHandler + ".");
         optimizely.close();
     }
 
@@ -2861,9 +2882,9 @@ public void getEnabledFeaturesWithListenerMultipleFeatureEnabled() throws Except
 
     /**
      * Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into
-     * {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag sending
+     * {@link DecisionService#getVariationForFeature} for each featureFlag sending
      * userId and emptyMap and Mocked {@link Optimizely#isFeatureEnabled(String, String, Map)}
-     * to return false so {@link Optimizely#getEnabledFeatures(String, Map)} will
+     * to return feature disabled so {@link Optimizely#getEnabledFeatures(String, Map)} will
      * return empty List of FeatureFlags and no notification listener will get called.
      */
     @Test
@@ -2873,13 +2894,16 @@ public void getEnabledFeaturesWithNoFeatureEnabled() throws Exception {
         isListenerCalled = false;
         Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler)
             .withConfig(validProjectConfig)
+            .withDecisionService(mockDecisionService)
             .build());
-        doReturn(false).when(spyOptimizely).isFeatureEnabled(
-            any(String.class),
-            eq(genericUserId),
-            eq(Collections.<String, String>emptyMap())
-        );
 
+        FeatureDecision featureDecision = new FeatureDecision(null, null, FeatureDecision.DecisionSource.ROLLOUT);
+        doReturn(featureDecision).when(mockDecisionService).getVariationForFeature(
+            any(FeatureFlag.class),
+            anyString(),
+            anyMapOf(String.class, String.class),
+            any(ProjectConfig.class)
+        );
         int notificationId = spyOptimizely.addDecisionNotificationHandler( decisionNotification -> { });
 
         ArrayList<String> featureFlags = (ArrayList<String>) spyOptimizely.getEnabledFeatures(genericUserId,
@@ -4839,23 +4863,27 @@ public void getEnabledFeatureWithNullUserID() throws ConfigParseException {
 
     /**
      * Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into
-     * {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag sending
-     * userId and emptyMap and Mocked {@link Optimizely#isFeatureEnabled(String, String, Map)}
-     * to return false so {@link Optimizely#getEnabledFeatures(String, Map)} will
+     * {@link DecisionService#getVariationForFeature} to return feature
+     * disabled so {@link Optimizely#getEnabledFeatures(String, Map)} will
      * return empty List of FeatureFlags.
      */
     @Test
-    public void getEnabledFeatureWithMockIsFeatureEnabledToReturnFalse() throws ConfigParseException {
+    public void getEnabledFeatureWithMockDecisionService() throws ConfigParseException {
         assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString()));
 
         Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler)
             .withConfig(validProjectConfig)
+            .withDecisionService(mockDecisionService)
             .build());
-        doReturn(false).when(spyOptimizely).isFeatureEnabled(
-            any(String.class),
-            eq(genericUserId),
-            eq(Collections.<String, String>emptyMap())
+
+        FeatureDecision featureDecision = new FeatureDecision(null, null, FeatureDecision.DecisionSource.ROLLOUT);
+        doReturn(featureDecision).when(mockDecisionService).getVariationForFeature(
+            any(FeatureFlag.class),
+            anyString(),
+            anyMapOf(String.class, String.class),
+            any(ProjectConfig.class)
         );
+
         ArrayList<String> featureFlags = (ArrayList<String>) spyOptimizely.getEnabledFeatures(genericUserId,
             Collections.<String, String>emptyMap());
         assertTrue(featureFlags.isEmpty());
