-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Handle missing flags in a valid JSON configuration request #38
Changes from all commits
dff97f2
efc3372
d75cc2d
5647f2e
b2cce67
11c3a32
4036419
4a5c146
c3db9e5
99301bf
d134a2a
e4dd058
3b84663
ad39f9b
450e21d
7912a42
3eef794
83433fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ plugins { | |
} | ||
|
||
group = "cloud.eppo" | ||
version = "1.0.3" | ||
version = "1.0.4" | ||
|
||
android { | ||
compileSdk 33 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,9 @@ | |
import com.google.gson.JsonElement; | ||
import com.google.gson.JsonParseException; | ||
|
||
import static org.mockito.Mockito.doAnswer; | ||
import static org.mockito.Mockito.doThrow; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.spy; | ||
import static org.mockito.Matchers.any; | ||
import static org.mockito.Matchers.anyString; | ||
|
@@ -32,7 +34,9 @@ | |
import java.io.File; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.io.StringReader; | ||
import java.lang.reflect.Type; | ||
import java.lang.reflect.Field; | ||
import java.util.List; | ||
import java.util.concurrent.CountDownLatch; | ||
import java.util.concurrent.TimeUnit; | ||
|
@@ -360,13 +364,85 @@ private List<JsonElement> getJSONAssignments(AssignmentTestCase testCase) { | |
return (List<JsonElement>) this.getAssignments(testCase, AssignmentValueType.JSON); | ||
} | ||
|
||
private static String getMockRandomizedAssignmentResponse() { | ||
@Test | ||
public void testInvalidConfigJSON() { | ||
|
||
// Create a mock instance of EppoHttpClient | ||
EppoHttpClient mockHttpClient = mock(EppoHttpClient.class); | ||
|
||
doAnswer(invocation -> { | ||
RequestCallback callback = invocation.getArgument(1); | ||
callback.onSuccess(new StringReader("{}")); | ||
return null; // doAnswer doesn't require a return value | ||
}).when(mockHttpClient).get(anyString(), any(RequestCallback.class)); | ||
|
||
Field httpClientOverrideField = null; | ||
try { | ||
// Use reflection to set the httpClientOverride field | ||
httpClientOverrideField = EppoClient.class.getDeclaredField("httpClientOverride"); | ||
httpClientOverrideField.setAccessible(true); | ||
httpClientOverrideField.set(null, mockHttpClient); | ||
Comment on lines
+382
to
+384
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
|
||
initClient(TEST_HOST, true, true, false); | ||
} catch (InterruptedException | NoSuchFieldException | IllegalAccessException e) { | ||
throw new RuntimeException(e); | ||
} finally { | ||
if (httpClientOverrideField != null) { | ||
try { | ||
httpClientOverrideField.set(null, null); | ||
} catch (IllegalAccessException e) { | ||
throw new RuntimeException(e); | ||
} | ||
httpClientOverrideField.setAccessible(false); | ||
} | ||
} | ||
|
||
String result = EppoClient.getInstance().getStringAssignment("dummy subject", "dummy flag"); | ||
assertNull(result); | ||
} | ||
|
||
@Test | ||
public void testCachedBadResponseAllowsLaterFetching() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made this test to check another possible cause of bad flags: a cached but invalid JSON file. It turns out it was handling things ok but I'll keep the test around to ensure this continues to work. |
||
// Populate the cache with a bad response | ||
ConfigCacheFile cacheFile = new ConfigCacheFile(ApplicationProvider.getApplicationContext()); | ||
cacheFile.delete(); | ||
try { | ||
InputStream in = ApplicationProvider.getApplicationContext().getAssets() | ||
.open("rac-experiments-v3-hashed-keys.json"); | ||
return IOUtils.toString(in, Charsets.toCharset("UTF8")); | ||
cacheFile.getOutputWriter().write("{}"); | ||
cacheFile.getOutputWriter().close(); | ||
} catch (IOException e) { | ||
throw new RuntimeException("Error reading mock RAC data", e); | ||
throw new RuntimeException(e); | ||
} | ||
try { | ||
initClient(TEST_HOST, false, false, false); | ||
} catch (InterruptedException e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side note, I tried refactoring |
||
throw new RuntimeException(e); | ||
}; | ||
|
||
String result = EppoClient.getInstance().getStringAssignment("dummy subject", "dummy flag"); | ||
assertNull(result); | ||
// Failure callback will have fired from cache read error, but configuration request will still be fired off on init | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side note: I think we should consider reworking things so that we can use another callback on loadFromCache before firing off our request. The way things are now, if there is a cache file the initialization callback will be used regardless on whether the cache was used or not. So if the cache failed but we got a valid configuration 500ms later, Fixing that is outside the scope of this PR, but if we're interested in changing this we can file another ticket. |
||
// Wait for the configuration request to load the configuration | ||
waitForNonNullAssignment(); | ||
String assignment = EppoClient.getInstance().getStringAssignment("6255e1a7fc33a9c050ce9508", "randomization_algo"); | ||
assertEquals("control", assignment); | ||
} | ||
|
||
private void waitForNonNullAssignment() { | ||
long waitStart = System.currentTimeMillis(); | ||
long waitEnd = waitStart + 15 * 1000; // allow up to 15 seconds | ||
String assignment = null; | ||
try { | ||
while (assignment == null) { | ||
if (System.currentTimeMillis() > waitEnd) { | ||
throw new InterruptedException("Non-null assignment never received; assuming configuration not loaded"); | ||
} | ||
// Uses third subject in test-case-0 | ||
assignment = EppoClient.getInstance().getStringAssignment("6255e1a7fc33a9c050ce9508", "randomization_algo"); | ||
Thread.sleep(100); | ||
} | ||
} catch (InterruptedException e) { | ||
throw new RuntimeException(e); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
|
||
import com.google.gson.Gson; | ||
import com.google.gson.GsonBuilder; | ||
import com.google.gson.JsonSyntaxException; | ||
|
||
import java.io.InputStreamReader; | ||
import java.io.OutputStreamWriter; | ||
|
@@ -55,6 +56,10 @@ public boolean loadFromCache(InitializationCallback callback) { | |
InputStreamReader reader = cacheFile.getInputReader(); | ||
RandomizationConfigResponse configResponse = gson.fromJson(reader, RandomizationConfigResponse.class); | ||
reader.close(); | ||
if (configResponse == null || configResponse.getFlags() == null) { | ||
// Invalid cached configuration, initialize as an empty map and delete file | ||
throw new JsonSyntaxException("Configuration file missing flags"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make a more helpful error message just in case this ever does happen for whatever reason |
||
} | ||
flags = configResponse.getFlags(); | ||
} | ||
Log.d(TAG, "Cache loaded successfully"); | ||
|
@@ -77,6 +82,10 @@ public boolean loadFromCache(InitializationCallback callback) { | |
public void setFlags(Reader response) { | ||
RandomizationConfigResponse config = gson.fromJson(response, RandomizationConfigResponse.class); | ||
flags = config.getFlags(); | ||
if (flags == null) { | ||
Log.w(TAG, "Flags missing in configuration response"); | ||
flags = new ConcurrentHashMap<>(); | ||
} | ||
Comment on lines
+85
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These four lines are the actual fix π |
||
|
||
// update any existing flags already in shared prefs | ||
updateConfigsInSharedPrefs(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,8 +36,11 @@ public class EppoClient { | |
private boolean isGracefulMode; | ||
private static EppoClient instance; | ||
|
||
// Useful for testing in situations where we want to mock the http client | ||
private static EppoHttpClient httpClientOverride = null; | ||
Comment on lines
+39
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My workaround for injecting an EppoHttpClient without Mockito magic |
||
|
||
private EppoClient(Application application, String apiKey, String host, AssignmentLogger assignmentLogger, boolean isGracefulMode) { | ||
EppoHttpClient httpClient = new EppoHttpClient(host, apiKey); | ||
EppoHttpClient httpClient = httpClientOverride == null ? new EppoHttpClient(host, apiKey) : httpClientOverride; | ||
ConfigurationStore configStore = new ConfigurationStore(application); | ||
requestor = new ConfigurationRequestor(configStore, httpClient); | ||
this.isGracefulMode = isGracefulMode; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test that reproduced the issue prior to the fix