diff --git a/.gitignore b/.gitignore index bacbde254..aefc53cb6 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,7 @@ local.properties **/build +bin out classes diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 02be1399a..e4070e681 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -29,7 +29,6 @@ import com.optimizely.ab.config.ProjectConfig; import com.optimizely.ab.config.Variation; import com.optimizely.ab.config.parser.ConfigParseException; -import com.optimizely.ab.config.parser.DefaultConfigParser; import com.optimizely.ab.error.ErrorHandler; import com.optimizely.ab.error.NoOpErrorHandler; import com.optimizely.ab.event.EventHandler; @@ -80,22 +79,21 @@ public class Optimizely { private static final Logger logger = LoggerFactory.getLogger(Optimizely.class); - @VisibleForTesting final DecisionService decisionService; + @VisibleForTesting DecisionService decisionService; @VisibleForTesting final EventFactory eventFactory; - @VisibleForTesting final ProjectConfig projectConfig; + @VisibleForTesting ProjectConfig projectConfig; @VisibleForTesting final EventHandler eventHandler; @VisibleForTesting final ErrorHandler errorHandler; + private boolean isValid; public final NotificationCenter notificationCenter = new NotificationCenter(); @Nullable private final UserProfileService userProfileService; - private Optimizely(@Nonnull ProjectConfig projectConfig, - @Nonnull DecisionService decisionService, - @Nonnull EventHandler eventHandler, + private Optimizely(@Nonnull EventHandler eventHandler, @Nonnull EventFactory eventFactory, @Nonnull ErrorHandler errorHandler, + @Nullable DecisionService decisionService, @Nullable UserProfileService userProfileService) { - this.projectConfig = projectConfig; this.decisionService = decisionService; this.eventHandler = eventHandler; this.eventFactory = eventFactory; @@ -103,10 +101,43 @@ private Optimizely(@Nonnull ProjectConfig projectConfig, this.userProfileService = userProfileService; } - // Do work here that should be done once per Optimizely lifecycle + /** + * Initializes the SDK state. Can conceivably re-use this in the future with datafile sync where + * we can re-initialize the SDK instead of re-instantiating. + */ @VisibleForTesting - void initialize() { + void initialize(@Nonnull String datafile, @Nullable ProjectConfig projectConfig) { + if (projectConfig == null) { + try { + projectConfig = new ProjectConfig.Builder() + .withDatafile(datafile) + .build(); + isValid = true; + logger.info("Datafile is valid"); + } catch (ConfigParseException ex) { + logger.error("Unable to parse the datafile", ex); + logger.info("Datafile is invalid"); + errorHandler.handleError(new OptimizelyRuntimeException(ex)); + } + } else { + isValid = true; + } + + this.projectConfig = projectConfig; + if (decisionService == null) { + Bucketer bucketer = new Bucketer(projectConfig); + decisionService = new DecisionService(bucketer, errorHandler, projectConfig, userProfileService); + } + } + /** + * Determine if the instance of the Optimizely client is valid. An instance can be deemed invalid if it was not + * initialized properly due to an invalid datafile being passed in. + * @return True if the Optimizely instance is valid. + * False if the Optimizely instance is not valid. + */ + public boolean isValid() { + return isValid; } //======== activate calls ========// @@ -121,6 +152,10 @@ Variation activate(@Nonnull String experimentKey, Variation activate(@Nonnull String experimentKey, @Nonnull String userId, @Nonnull Map attributes) throws UnknownExperimentException { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing activate call."); + return null; + } if (experimentKey == null) { logger.error("The experimentKey parameter must be nonnull."); @@ -165,6 +200,10 @@ Variation activate(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing activate call."); + return null; + } if (!validateUserId(userId)){ logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey()); @@ -236,6 +275,10 @@ public void track(@Nonnull String eventName, @Nonnull String userId, @Nonnull Map attributes, @Nonnull Map eventTags) throws UnknownEventTypeException { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing track call."); + return; + } if (!validateUserId(userId)) { logger.info("Not tracking event \"{}\".", eventName); @@ -345,6 +388,11 @@ public void track(@Nonnull String eventName, public @Nonnull Boolean isFeatureEnabled(@Nonnull String featureKey, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing isFeatureEnabled call."); + return false; + } + if (featureKey == null) { logger.warn("The featureKey parameter must be nonnull."); return false; @@ -411,6 +459,11 @@ else if (userId == null) { @Nonnull String variableKey, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getFeatureVariableBoolean call."); + return null; + } + String variableValue = getFeatureVariableValueForType( featureKey, variableKey, @@ -451,6 +504,11 @@ else if (userId == null) { @Nonnull String variableKey, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getFeatureVariableDouble call."); + return null; + } + String variableValue = getFeatureVariableValueForType( featureKey, variableKey, @@ -496,6 +554,11 @@ else if (userId == null) { @Nonnull String variableKey, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getFeatureVariableInteger call."); + return null; + } + String variableValue = getFeatureVariableValueForType( featureKey, variableKey, @@ -541,6 +604,11 @@ else if (userId == null) { @Nonnull String variableKey, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getFeatureVariableString call."); + return null; + } + return getFeatureVariableValueForType( featureKey, variableKey, @@ -617,6 +685,11 @@ else if (userId == null) { public List getEnabledFeatures(@Nonnull String userId, @Nonnull Map attributes) { List enabledFeaturesList = new ArrayList(); + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getEnabledFeatures call."); + return enabledFeaturesList; + } + if (!validateUserId(userId)){ return enabledFeaturesList; } @@ -660,6 +733,11 @@ Variation getVariation(@Nonnull String experimentKey, Variation getVariation(@Nonnull String experimentKey, @Nonnull String userId, @Nonnull Map attributes) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getVariation call."); + return null; + } + if (!validateUserId(userId)) { return null; } @@ -697,7 +775,10 @@ Variation getVariation(@Nonnull String experimentKey, public boolean setForcedVariation(@Nonnull String experimentKey, @Nonnull String userId, @Nullable String variationKey) { - + if (!isValid) { + logger.error("Optimizely instance is not valid, failing setForcedVariation call."); + return false; + } return projectConfig.setForcedVariation(experimentKey, userId, variationKey); } @@ -715,6 +796,11 @@ public boolean setForcedVariation(@Nonnull String experimentKey, */ public @Nullable Variation getForcedVariation(@Nonnull String experimentKey, @Nonnull String userId) { + if (!isValid) { + logger.error("Optimizely instance is not valid, failing getForcedVariation call."); + return null; + } + return projectConfig.getForcedVariation(experimentKey, userId); } @@ -869,17 +955,7 @@ protected Builder withConfig(ProjectConfig projectConfig) { return this; } - public Optimizely build() throws ConfigParseException { - if (projectConfig == null) { - projectConfig = new ProjectConfig.Builder() - .withDatafile(datafile) - .build(); - } - - if (bucketer == null) { - bucketer = new Bucketer(projectConfig); - } - + public Optimizely build() { if (clientEngine == null) { clientEngine = ClientEngine.JAVA_SDK; } @@ -897,12 +973,13 @@ public Optimizely build() throws ConfigParseException { errorHandler = new NoOpErrorHandler(); } - if (decisionService == null) { + // Used for convenience while unit testing to override/mock bucketing. This interface is NOT public and should be refactored out. + if (bucketer != null && decisionService == null) { decisionService = new DecisionService(bucketer, errorHandler, projectConfig, userProfileService); } - Optimizely optimizely = new Optimizely(projectConfig, decisionService, eventHandler, eventFactory, errorHandler, userProfileService); - optimizely.initialize(); + Optimizely optimizely = new Optimizely(eventHandler, eventFactory, errorHandler, decisionService, userProfileService); + optimizely.initialize(datafile, projectConfig); return optimizely; } } diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java index 5083f580b..2e9283e65 100644 --- a/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java +++ b/core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java @@ -62,7 +62,7 @@ public class DecisionService { */ public DecisionService(@Nonnull Bucketer bucketer, @Nonnull ErrorHandler errorHandler, - @Nonnull ProjectConfig projectConfig, + @Nullable ProjectConfig projectConfig, @Nullable UserProfileService userProfileService) { this.bucketer = bucketer; this.errorHandler = errorHandler; diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 086f2d6b5..25a84f823 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -614,7 +614,7 @@ public Builder withDatafile(String datafile) { /** * @return a {@link ProjectConfig} instance given a JSON string datafile */ - public ProjectConfig build() throws ConfigParseException{ + public ProjectConfig build() throws ConfigParseException { if (datafile == null) { throw new ConfigParseException("Unable to parse null datafile."); } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java index e142091ec..6600f3397 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java @@ -33,12 +33,10 @@ import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; -import static com.optimizely.ab.config.ProjectConfigTestUtils.validConfigJsonV2; -import static com.optimizely.ab.config.ProjectConfigTestUtils.validConfigJsonV3; -import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV2; -import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV3; +import static com.optimizely.ab.config.ProjectConfigTestUtils.*; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.mockito.Mockito.mock; @@ -154,20 +152,31 @@ public void withCustomClientVersion() throws Exception { @SuppressFBWarnings(value="NP_NONNULL_PARAM_VIOLATION", justification="Testing nullness contract violation") @Test - public void builderThrowsConfigParseExceptionForNullDatafile() throws Exception { - thrown.expect(ConfigParseException.class); - Optimizely.builder(null, mockEventHandler).build(); + public void nullDatafileResultsInInvalidOptimizelyInstance() throws Exception { + Optimizely optimizelyClient = Optimizely.builder(null, mockEventHandler).build(); + + assertFalse(optimizelyClient.isValid()); } @Test - public void builderThrowsConfigParseExceptionForEmptyDatafile() throws Exception { - thrown.expect(ConfigParseException.class); - Optimizely.builder("", mockEventHandler).build(); + public void emptyDatafileResultsInInvalidOptimizelyInstance() throws Exception { + Optimizely optimizelyClient = Optimizely.builder("", mockEventHandler).build(); + + assertFalse(optimizelyClient.isValid()); } @Test - public void builderThrowsConfigParseExceptionForInvalidDatafile() throws Exception { - thrown.expect(ConfigParseException.class); - Optimizely.builder("{invalidDatafile}", mockEventHandler).build(); + public void invalidDatafileResultsInInvalidOptimizelyInstance() throws Exception { + Optimizely optimizelyClient = Optimizely.builder("{invalidDatafile}", mockEventHandler).build(); + + assertFalse(optimizelyClient.isValid()); + } + + @Test + public void unsupportedDatafileResultsInInvalidOptimizelyInstance() throws Exception { + Optimizely optimizelyClient = Optimizely.builder(invalidProjectConfigV5(), mockEventHandler) + .build(); + + assertFalse(optimizelyClient.isValid()); } } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index a29a1fa12..1add5241e 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -1177,6 +1177,21 @@ public void activateLaunchedExperimentDoesNotDispatchEvent() throws Exception { verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } + /** + * Verify that we don't attempt to activate the user when the Optimizely instance is not valid + */ + @Test + public void activateWithInvalidDatafile() throws Exception { + Optimizely optimizely = Optimizely.builder(invalidProjectConfigV5(), mockEventHandler) + .withBucketing(mockBucketer) + .build(); + Variation expectedVariation = optimizely.activate("etag1", genericUserId); + assertNull(expectedVariation); + + // make sure we didn't even attempt to bucket the user + verify(mockBucketer, never()).bucket(any(Experiment.class), anyString()); + } + //======== track tests ========// /** @@ -2068,6 +2083,21 @@ public void trackDoesNotSendEventWhenUserDoesNotSatisfyAudiences() throws Except verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } + /** + * Verify that we don't attempt to track any events if the Optimizely instance is not valid + */ + @Test + public void trackWithInvalidDatafile() throws Exception { + Optimizely optimizely = Optimizely.builder(invalidProjectConfigV5(), mockEventHandler) + .withBucketing(mockBucketer) + .build(); + optimizely.track("event_with_launched_and_running_experiments", genericUserId); + + // make sure we didn't even attempt to bucket the user or fire any conversion events + verify(mockBucketer, never()).bucket(any(Experiment.class), anyString()); + verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); + } + //======== getVariation tests ========// /** @@ -2356,6 +2386,22 @@ public void getVariationExperimentStatusPrecedesForcedVariation() throws Excepti assertNull(optimizely.getVariation(experiment.getKey(), "testUser3")); } + /** + * Verify that we don't attempt to track any events if the Optimizely instance is not valid + */ + @Test + public void getVariationWithInvalidDatafile() throws Exception { + Optimizely optimizely = Optimizely.builder(invalidProjectConfigV5(), mockEventHandler) + .withBucketing(mockBucketer) + .build(); + Variation variation = optimizely.getVariation("etag1", genericUserId); + + assertNull(variation); + + // make sure we didn't even attempt to bucket the user + verify(mockBucketer, never()).bucket(any(Experiment.class), anyString()); + } + //======== Notification listeners ========// /** @@ -3559,6 +3605,21 @@ public void isFeatureEnabledReturnsTrueAndDispatchesEventWhenUserIsBucketedIntoA verify(mockEventHandler, times(1)).dispatchEvent(any(LogEvent.class)); } + /** + * Verify that we don't attempt to activate the user when the Optimizely instance is not valid + */ + @Test + public void isFeatureEnabledWithInvalidDatafile() throws Exception { + Optimizely optimizely = Optimizely.builder(invalidProjectConfigV5(), mockEventHandler) + .withDecisionService(mockDecisionService) + .build(); + Boolean isEnabled = optimizely.isFeatureEnabled("no_variable_feature", genericUserId); + assertFalse(isEnabled); + + // make sure we didn't even attempt to bucket the user + verify(mockDecisionService, never()).getVariationForFeature(any(FeatureFlag.class), anyString(), anyMap()); + } + /** * Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into * {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag @@ -4546,6 +4607,32 @@ public void getVariationBucketingIdAttribute() throws Exception { assertThat(actualVariation, is(bucketedVariation)); } + //======== isValid calls ========// + + /** + * Verify that {@link Optimizely#isValid()} returns false when the Optimizely instance is not valid + */ + @Test + public void isValidReturnsFalseWhenClientIsInvalid() throws Exception { + Optimizely optimizely = Optimizely.builder(invalidProjectConfigV5(), mockEventHandler) + .withBucketing(mockBucketer) + .build(); + + assertFalse(optimizely.isValid()); + } + + /** + * Verify that {@link Optimizely#isValid()} returns false when the Optimizely instance is not valid + */ + @Test + public void isValidReturnsTrueWhenClientIsValid() throws Exception { + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .build(); + + assertTrue(optimizely.isValid()); + } + //======== Helper methods ========// private Experiment createUnknownExperiment() { diff --git a/core-api/src/test/resources/config/invalid-project-config-v5.json b/core-api/src/test/resources/config/invalid-project-config-v5.json index a3cc668f9..d9c3d1936 100644 --- a/core-api/src/test/resources/config/invalid-project-config-v5.json +++ b/core-api/src/test/resources/config/invalid-project-config-v5.json @@ -77,6 +77,43 @@ "entityId": "281", "endOfRange": 10000 }] + }, + { + "id": "120", + "key": "no_variable_feature_test", + "status": "Running", + "layerId": "3", + "audienceIds": [], + "variations": [ + { + "id": "282", + "key": "no_variable_feature_test_variation_1" + }, + { + "id": "283", + "key": "no_variable_feature_test_variation_2" + } + ], + "forcedVariations": {}, + "trafficAllocation": [ + { + "entityId": "282", + "endOfRange": 5000 + }, + { + "entityId": "283", + "endOfRange": 10000 + } + ] + } + ], + "featureFlags": [ + { + "id": "4195505407", + "key": "no_variable_feature", + "rolloutId": "", + "experimentIds": [120], + "variables": [] } ], "groups": [],