Skip to content
This repository was archived by the owner on May 30, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions src/main/java/com/launchdarkly/client/EvaluationException.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.launchdarkly.client;

/**
* An error indicating an abnormal result from evaluating a feature
*/
class EvaluationException extends Exception {
public EvaluationException(String message) {
super(message);
}
}
63 changes: 37 additions & 26 deletions src/main/java/com/launchdarkly/client/FeatureFlag.java
Original file line number Diff line number Diff line change
Expand Up @@ -50,44 +50,33 @@ static Map<String, FeatureFlag> fromJsonMap(String json) {
this.deleted = deleted;
}

Integer getOffVariation() {
return this.offVariation;
}

JsonElement getOffVariationValue() {
if (offVariation != null && offVariation < variations.size()) {
return variations.get(offVariation);
}
return null;
}

EvalResult evaluate(LDUser user, FeatureStore featureStore) {
EvalResult evaluate(LDUser user, FeatureStore featureStore) throws EvaluationException {
if (user == null || user.getKey() == 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.

In this case we should return the default.

return null;
throw new EvaluationException("User or user key is null");
}
List<FeatureRequestEvent> prereqEvents = new ArrayList<>();
Set<String> visited = new HashSet<>();
JsonElement value = evaluate(user, featureStore, prereqEvents, visited);
JsonElement value = evaluate(user, featureStore, prereqEvents);
return new EvalResult(value, prereqEvents);
}

// Returning either a JsonElement or null indicating prereq failure/error.
private JsonElement evaluate(LDUser user, FeatureStore featureStore, List<FeatureRequestEvent> events, Set<String> visited) {
private JsonElement evaluate(LDUser user, FeatureStore featureStore, List<FeatureRequestEvent> events) throws EvaluationException {
boolean prereqOk = true;
visited.add(key);
for (Prerequisite prereq : prerequisites) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the broken cycle detection algorithm.

if (visited.contains(prereq.getKey())) {
logger.error("Prerequisite cycle detected when evaluating feature flag: " + key);
return null;
}
FeatureFlag prereqFeatureFlag = featureStore.get(prereq.getKey());
JsonElement prereqEvalResult = null;
if (prereqFeatureFlag == null) {
logger.error("Could not retrieve prerequisite flag: " + prereq.getKey() + " when evaluating: " + key);
return null;
} else if (prereqFeatureFlag.isOn()) {
prereqEvalResult = prereqFeatureFlag.evaluate(user, featureStore, events, visited);
if (prereqEvalResult == null || !prereqEvalResult.equals(prereqFeatureFlag.getVariation(prereq.getVariation()))) {
prereqEvalResult = prereqFeatureFlag.evaluate(user, featureStore, events);
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If prerequisite evaluation fails for whatever reason, we return the off variation, not an error. So we don't bubble up prereq evaluation exceptions.

JsonElement variation = prereqFeatureFlag.getVariation(prereq.getVariation());
if (prereqEvalResult == null || variation == null || !prereqEvalResult.equals(variation)) {
prereqOk = false;
}
} catch (EvaluationException err) {
logger.warn("Error evaluating prerequisites: " + err.getMessage());
prereqOk = false;
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 we should log the exception here- at debug or warn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

}
} else {
Expand Down Expand Up @@ -123,10 +112,30 @@ private Integer evaluateIndex(LDUser user) {
return fallthrough.variationIndexForUser(user, key, salt);
}

private JsonElement getVariation(Integer index) {
if (index == null || index >= variations.size()) {
JsonElement getOffVariationValue() throws EvaluationException {
if (offVariation == null) {
return null;
} else {
}

if (offVariation >= variations.size()) {
throw new EvaluationException("Invalid off variation index");
}

return variations.get(offVariation);
}

private JsonElement getVariation(Integer index) throws EvaluationException {
// If the supplied index is null, then rules didn't match, and we want to return
// the off variation
if (index == null) {
return null;
}
// If the index doesn't refer to a valid variation, that's an unexpected exception and we will
// return the default variation
else if (index >= variations.size()) {
throw new EvaluationException("Invalid index");
}
else {
return variations.get(index);
}
}
Expand Down Expand Up @@ -171,6 +180,8 @@ List<JsonElement> getVariations() {
return variations;
}

Integer getOffVariation() { return offVariation; }

static class EvalResult {
private final JsonElement value;
private final List<FeatureRequestEvent> prerequisiteEvents;
Expand Down
27 changes: 8 additions & 19 deletions src/main/java/com/launchdarkly/client/LDClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -294,47 +294,36 @@ public JsonElement jsonVariation(String featureKey, LDUser user, JsonElement def
return value;
}


private JsonElement evaluate(String featureKey, LDUser user, JsonElement defaultValue) {
if (!initialized()) {
return defaultValue;
}
try {
FeatureFlag featureFlag = config.featureStore.get(featureKey);
if (featureFlag != null) {
if (config.stream && config.debugStreaming) {
FeatureFlag pollingResult = requestor.makeRequest(featureKey, true);
if (!featureFlag.equals(pollingResult)) {
logger.warn("Mismatch between streaming and polling feature! Streaming: {} Polling: {}", featureFlag, pollingResult);
}
}
} else {
if (featureFlag == 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.

I removed the debugStreaming option; I believe it has outlived its usefulness.

Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

logger.warn("Unknown feature flag " + featureKey + "; returning default value: ");
return defaultValue;
}
if (featureFlag.isOn()) {
FeatureFlag.EvalResult evalResult = featureFlag.evaluate(user, config.featureStore);
if (evalResult != null) {
if (!isOffline()) {
for (FeatureRequestEvent event : evalResult.getPrerequisiteEvents()) {
eventProcessor.sendEvent(event);
}
}
if (evalResult.getValue() == null) {
return defaultValue;
} else {
if (evalResult.getValue() != null) {
return evalResult.getValue();
}
}
} else {
JsonElement offVariation = featureFlag.getOffVariationValue();
if (offVariation != null) {
return offVariation;
}
}
JsonElement offVariation = featureFlag.getOffVariationValue();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return the offVariation unless there is an error.

if (offVariation != null) {
return offVariation;
}
} catch (Exception e) {
logger.error("Encountered exception in LaunchDarkly client", e);
}


return defaultValue;
}

Expand Down
15 changes: 0 additions & 15 deletions src/main/java/com/launchdarkly/client/LDConfig.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ public final class LDConfig {
final int flushInterval;
final HttpHost proxyHost;
final boolean stream;
final boolean debugStreaming;
final FeatureStore featureStore;
final boolean useLdd;
final boolean offline;
Expand All @@ -55,7 +54,6 @@ protected LDConfig(Builder builder) {
this.proxyHost = builder.proxyHost();
this.streamURI = builder.streamURI;
this.stream = builder.stream;
this.debugStreaming = builder.debugStreaming;
this.featureStore = builder.featureStore;
this.useLdd = builder.useLdd;
this.offline = builder.offline;
Expand Down Expand Up @@ -91,7 +89,6 @@ public static class Builder {
private int proxyPort = -1;
private String proxyScheme;
private boolean stream = true;
private boolean debugStreaming = false;
private boolean useLdd = false;
private boolean offline = false;
private long pollingIntervalMillis = DEFAULT_POLLING_INTERVAL_MILLIS;
Expand Down Expand Up @@ -140,18 +137,6 @@ public Builder featureStore(FeatureStore store) {
return this;
}

/**
* Set whether we should debug streaming mode. If set, the client will fetch features via polling and compare the
* retrieved feature with the value in the feature store. There is a performance cost to this, so it is not
* recommended for production use.
* @param debugStreaming whether streaming mode should be debugged
* @return the builder
*/
public Builder debugStreaming(boolean debugStreaming) {
this.debugStreaming = debugStreaming;
return this;
}

/**
* Set whether streaming mode should be enabled. By default, streaming is enabled.
* @param stream whether streaming mode should be enabled
Expand Down
7 changes: 0 additions & 7 deletions src/main/java/com/launchdarkly/client/Operator.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,6 @@ public boolean apply(JsonPrimitive uValue, JsonPrimitive cValue) {
if (uValue.isNumber() && cValue.isNumber()) {
return uValue.getAsDouble() == cValue.getAsDouble();
}
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 cleans up the semantics of the in operator.

DateTime uDateTime = Util.jsonPrimitiveToDateTime(uValue);
if (uDateTime != null) {
DateTime cDateTime = Util.jsonPrimitiveToDateTime(cValue);
if (cDateTime != null) {
return uDateTime.getMillis() == cDateTime.getMillis();
}
}
return false;
}
},
Expand Down
51 changes: 2 additions & 49 deletions src/test/java/com/launchdarkly/client/FeatureFlagTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,54 +21,7 @@ public void before() {
}

@Test
public void testPrereqSelfCycle() {
String keyA = "keyA";
FeatureFlag f = newFlagWithPrereq(keyA, keyA);

featureStore.upsert(keyA, f);
LDUser user = new LDUser.Builder("userKey").build();

FeatureFlag.EvalResult actual = f.evaluate(user, featureStore);

Assert.assertNull(actual.getValue());
Assert.assertNotNull(actual.getPrerequisiteEvents());
Assert.assertEquals(0, actual.getPrerequisiteEvents().size());
}

@Test
public void testPrereqSimpleCycle() {
String keyA = "keyA";
String keyB = "keyB";
FeatureFlag f1 = newFlagWithPrereq(keyA, keyB);
FeatureFlag f2 = newFlagWithPrereq(keyB, keyA);

featureStore.upsert(f1.getKey(), f1);
featureStore.upsert(f2.getKey(), f2);
LDUser user = new LDUser.Builder("userKey").build();
Assert.assertNull(f1.evaluate(user, featureStore).getValue());
Assert.assertNull(f2.evaluate(user, featureStore).getValue());
}

@Test
public void testPrereqCycle() {
String keyA = "keyA";
String keyB = "keyB";
String keyC = "keyC";
FeatureFlag f1 = newFlagWithPrereq(keyA, keyB);
FeatureFlag f2 = newFlagWithPrereq(keyB, keyC);
FeatureFlag f3 = newFlagWithPrereq(keyC, keyA);

featureStore.upsert(f1.getKey(), f1);
featureStore.upsert(f2.getKey(), f2);
featureStore.upsert(f3.getKey(), f3);
LDUser user = new LDUser.Builder("userKey").build();
Assert.assertNull(f1.evaluate(user, featureStore).getValue());
Assert.assertNull(f2.evaluate(user, featureStore).getValue());
Assert.assertNull(f3.evaluate(user, featureStore).getValue());
}

@Test
public void testPrereqDoesNotExist() {
public void testPrereqDoesNotExist() throws EvaluationException {
String keyA = "keyA";
String keyB = "keyB";
FeatureFlag f1 = newFlagWithPrereq(keyA, keyB);
Expand All @@ -83,7 +36,7 @@ public void testPrereqDoesNotExist() {
}

@Test
public void testPrereqCollectsEventsForPrereqs() {
public void testPrereqCollectsEventsForPrereqs() throws EvaluationException {
String keyA = "keyA";
String keyB = "keyB";
String keyC = "keyC";
Expand Down