Handle missing flags in a valid JSON configuration request #38
Conversation
| if (!lock.await(10000, TimeUnit.MILLISECONDS)) { | ||
| throw new RuntimeException("Request for RAC did not complete within timeout"); | ||
| } | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
By handling InterruptedException here and removing the throws from the signature, it cleaned up our test code a decent amount. 🧹
| @@ -360,13 +356,76 @@ private List<JsonElement> getJSONAssignments(AssignmentTestCase testCase) { | |||
| return (List<JsonElement>) this.getAssignments(testCase, AssignmentValueType.JSON); | |||
| } | |||
|
|
|||
| private static String getMockRandomizedAssignmentResponse() { | |||
| @Test | |||
| public void testInvalidConfigJSON() { | |||
There was a problem hiding this comment.
Test that reproduced the issue prior to the fix
| httpClientOverrideField = EppoClient.class.getDeclaredField("httpClientOverride"); | ||
| httpClientOverrideField.setAccessible(true); | ||
| httpClientOverrideField.set(null, mockHttpClient); |
There was a problem hiding this comment.
Mockito (and related packages) was not playing nicely with Android so I bailed to this less clean (but works!) solution of private static variables and reflection to inject a mock HTTP client without exposing the ability to in the "public" API.
| } | ||
|
|
||
| @Test | ||
| public void testCachedBadResponseAllowsLaterFetching() { |
There was a problem hiding this comment.
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.
| ; | ||
| 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.
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, onFailure() will be called, which I don't think fully represents what really happened.
Fixing that is outside the scope of this PR, but if we're interested in changing this we can file another ticket.
| @@ -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.
Make a more helpful error message just in case this ever does happen for whatever reason
| if (flags == null) { | ||
| Log.w(TAG, "Flags missing configuration response"); | ||
| flags = new ConcurrentHashMap<>(); | ||
| } |
| // Useful for testing in situations where we want to mock the http client | ||
| private static EppoHttpClient httpClientOverride = null; |
There was a problem hiding this comment.
My workaround for injecting an EppoHttpClient without Mockito magic
| if (flags == null) { | ||
| Log.w(TAG, "Flags missing in configuration response"); | ||
| flags = new ConcurrentHashMap<>(); | ||
| } |
There was a problem hiding this comment.
These four lines are the actual fix 😅
| System.out.println("Sleeping for a bit to wait for cache population to complete"); | ||
| Thread.sleep(10000); | ||
| // wait for a bit since cache file is loaded asynchronously | ||
| waitForNonNullAssignment(); |
There was a problem hiding this comment.
This test seemed flakey for me so I changed the hard-coded wait to use a checking one another test uses too
| } | ||
| try { | ||
| initClient(TEST_HOST, false, false, false); | ||
| } catch (InterruptedException e) { |
There was a problem hiding this comment.
Side note, I tried refactoring throws InterruptedException out of initClient() signature (example commit), but it seemed to (inexplicably) be what was breaking testCachedAssignments() the most 🤷
|
bump versions if you want to publish ASAP. |
aarsilv
force-pushed
the
aaron/ff-1869/handle-null-flags
branch
from
094e451
to
83433fd
Compare
Eppo Internal: 🎟️ Ticket: FF-1869 - crash in android app Attempt to invoke virtual method 'java.util.Set j$.util.concurrent.ConcurrentHashMap.keySet()' on a null object reference
If, for whatever reason, the request for a configuration is technically "successful," but the resulting JSON object is missing the
flagsproperty, it will result in a downstream error.To resolve this--and allow future polls to set the flags--we treat it as having received an empty list of flags.