Skip to content

Commit

Permalink
refactor(init): build invalid instance if datafile cannot be parsed. (#…
Browse files Browse the repository at this point in the history 
…209)
  • Loading branch information
@mikeproeng37
mikeproeng37 authored Sep 5, 2018
1 parent 4ecd765 commit 7347f06
Show file tree
Hide file tree
Showing 7 changed files with 250 additions and 39 deletions.
1 change: 1 addition & 0 deletions .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
local.properties

**/build
bin
out
classes

Expand Down
125 changes: 101 additions & 24 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -80,33 +79,65 @@ 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;
this.errorHandler = errorHandler;
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 ========//
Expand All @@ -121,6 +152,10 @@ Variation activate(@Nonnull String experimentKey,
Variation activate(@Nonnull String experimentKey,
@Nonnull String userId,
@Nonnull Map<String, ?> 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.");
Expand Down Expand Up @@ -165,6 +200,10 @@ Variation activate(@Nonnull ProjectConfig projectConfig,
@Nonnull Experiment experiment,
@Nonnull String userId,
@Nonnull Map<String, ?> 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());
Expand Down Expand Up @@ -236,6 +275,10 @@ public void track(@Nonnull String eventName,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes,
@Nonnull Map<String, ?> 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);
Expand Down Expand Up @@ -345,6 +388,11 @@ public void track(@Nonnull String eventName,
public @Nonnull Boolean isFeatureEnabled(@Nonnull String featureKey,
@Nonnull String userId,
@Nonnull Map<String, ?> 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;
Expand Down Expand Up @@ -411,6 +459,11 @@ else if (userId == null) {
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
if (!isValid) {
logger.error("Optimizely instance is not valid, failing getFeatureVariableBoolean call.");
return null;
}

String variableValue = getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -451,6 +504,11 @@ else if (userId == null) {
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
if (!isValid) {
logger.error("Optimizely instance is not valid, failing getFeatureVariableDouble call.");
return null;
}

String variableValue = getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -496,6 +554,11 @@ else if (userId == null) {
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
if (!isValid) {
logger.error("Optimizely instance is not valid, failing getFeatureVariableInteger call.");
return null;
}

String variableValue = getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -541,6 +604,11 @@ else if (userId == null) {
@Nonnull String variableKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
if (!isValid) {
logger.error("Optimizely instance is not valid, failing getFeatureVariableString call.");
return null;
}

return getFeatureVariableValueForType(
featureKey,
variableKey,
Expand Down Expand Up @@ -617,6 +685,11 @@ else if (userId == null) {
public List<String> getEnabledFeatures(@Nonnull String userId, @Nonnull Map<String, ?> attributes) {
List<String> enabledFeaturesList = new ArrayList<String>();

if (!isValid) {
logger.error("Optimizely instance is not valid, failing getEnabledFeatures call.");
return enabledFeaturesList;
}

if (!validateUserId(userId)){
return enabledFeaturesList;
}
Expand Down Expand Up @@ -660,6 +733,11 @@ Variation getVariation(@Nonnull String experimentKey,
Variation getVariation(@Nonnull String experimentKey,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) {
if (!isValid) {
logger.error("Optimizely instance is not valid, failing getVariation call.");
return null;
}

if (!validateUserId(userId)) {
return null;
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
}
Expand Down
35 changes: 22 additions & 13 deletions core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
}
}
Loading

0 comments on commit 7347f06

Please sign in to comment.