Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(init): build invalid instance if datafile cannot be parsed. #209

Merged
merged 2 commits into from
Sep 5, 2018
Merged

refactor(init): build invalid instance if datafile cannot be parsed. #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b25f747
refactor(init): build invalid instance if datafile cannot be parsed.
Aug 31, 2018
c7e8d4c
Make isValid property private and provide a getter instead.
Sep 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
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
Open in desktop
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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step de-couples the "building" of the Optimizely instance from the "initialization", which is where we start populating state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should probably discuss this. As far as I know for threading purposes, you would not reuse these components but instead, initialize a new instance. This actually brings up an interesting point of Android datafile handler: Should the datafile handler reinitialize or have a shutdown mechanism?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, will take this offline.

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
Open in desktop
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
Open in desktop
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
Open in desktop
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