Skip to content

Commit 3011abe

Browse files
author
Josh Deffibaugh
committed
Tweaks datafile loading pipeline to ensure callback is hit
Currently it's possible for OptimizelyStartListener#onStart(OptimizelyClient) method to never get called if datafile loading fails from disk and the CDN. We should still call it with the dummy instance. It's highly likely that developers will gate their app depending on whether or not Optimizely loaded and calls the callback. For example, they could start Optimizely in a splash screen activity and launch the next activity once Optimizely is loaded.
1 parent e94dc2d commit 3011abe

File tree

7 files changed

+115
-67
lines changed

7 files changed

+115
-67
lines changed

‎android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/DataFileClientTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,6 @@ public void handlesNullResponse() throws MalformedURLException {
188188
public void handlesEmptyStringResponse() throws MalformedURLException {
189189
URL url = new URL(String.format(DataFileLoader.RequestDataFileFromClientTask.FORMAT_CDN_URL, "1"));
190190
when(client.execute(any(Client.Request.class), eq(2), eq(3))).thenReturn("");
191-
assertNull(dataFileClient.request(url.toString()));
191+
assertEquals("", dataFileClient.request(url.toString()));
192192
}
193193
}

‎android-sdk/src/androidTest/java/com/optimizely/ab/android/sdk/OptimizelyManagerTest.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,5 @@ public void injectOptimizelyDoesNotDuplicateCallback() {
212212
} catch (InterruptedException e) {
213213
fail("Timed out");
214214
}
215-
216-
verify(logger).info("No listener to send Optimizely to");
217215
}
218216
}

‎android-sdk/src/main/java/com/optimizely/ab/android/sdk/DataFileClient.java‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package com.optimizely.ab.android.sdk;
1818

1919
import android.support.annotation.NonNull;
20+
import android.support.annotation.Nullable;
2021

2122
import com.optimizely.ab.android.shared.Client;
2223

@@ -27,7 +28,7 @@
2728
import java.net.URL;
2829

2930
/*
30-
* Makes requests to the Optly CDN to get the data file
31+
* Makes requests to the Optly CDN to get the datafile
3132
*/
3233
class DataFileClient {
3334

@@ -39,6 +40,20 @@ class DataFileClient {
3940
this.logger = logger;
4041
}
4142

43+
/*
44+
* If the datafile is modified on the CDN since the last time
45+
* our local datafile was modified the response body will be a valid
46+
* Optimizely datafile and the response code will be 200.
47+
*
48+
* If the datafile has not been modified since last time our local
49+
* datafile was modified there will be no response body and the
50+
* response code will be 304.
51+
*
52+
* @param urlString the CDN url of an Optimizely datafile
53+
*
54+
* @return a valid datafile, null, or an empty string (on 304 responses)
55+
*/
56+
@Nullable
4257
String request(final String urlString) {
4358
Client.Request<String> request = new Client.Request<String>() {
4459
@Override
@@ -76,13 +91,14 @@ public String execute() {
7691
}
7792
};
7893

79-
// If the response was a 304 Not Modified an empty string is returned
80-
// Consumers of this method expect either a valid datafile or null
81-
String response = client.execute(request, 2, 3);
82-
if (response != null && response.isEmpty()) {
83-
response = null;
84-
}
94+
return client.execute(request, 2, 3);
95+
}
96+
97+
boolean isModifiedResponse(@Nullable String dataFile) {
98+
return dataFile != null && !dataFile.isEmpty();
99+
}
85100

86-
return response;
101+
boolean isNotModifiedResponse(@Nullable String dataFile) {
102+
return dataFile == null || dataFile.isEmpty();
87103
}
88104
}

‎android-sdk/src/main/java/com/optimizely/ab/android/sdk/DataFileLoadedListener.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,23 @@
1616

1717
package com.optimizely.ab.android.sdk;
1818

19+
import android.support.annotation.Nullable;
20+
1921
/**
2022
* Listens for new Optimizely datafiles
2123
*
24+
* Datafiles can come from a local file or the CDN
25+
*
2226
* @hide
2327
*/
2428
interface DataFileLoadedListener {
2529

2630
/**
2731
* Called with new datafile
2832
*
29-
* @param dataFile the datafile json
33+
* @param dataFile the datafile json, can be null if datafile loading failed.
34+
*
3035
* @hide
3136
*/
32-
void onDataFileLoaded(String dataFile);
37+
void onDataFileLoaded(@Nullable String dataFile);
3338
}

‎android-sdk/src/main/java/com/optimizely/ab/android/sdk/DataFileLoader.java‎

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -47,18 +47,17 @@ class DataFileLoader {
4747
}
4848

4949
@RequiresApi(api = Build.VERSION_CODES.HONEYCOMB)
50-
boolean getDataFile(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileLoadedListener) {
50+
void getDataFile(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileLoadedListener) {
5151
taskChain.start(projectId, dataFileLoadedListener);
5252

5353
logger.info("Refreshing data file");
54-
55-
return true;
5654
}
5755

5856
static class TaskChain {
5957

6058
@NonNull private final DataFileService dataFileService;
6159
@NonNull private final Executor executor;
60+
@Nullable DataFileLoadedListener dataFileLoadedListener;
6261

6362
TaskChain(@NonNull DataFileService dataFileService) {
6463
this.dataFileService = dataFileService;
@@ -67,6 +66,7 @@ static class TaskChain {
6766

6867
@RequiresApi(api = Build.VERSION_CODES.HONEYCOMB)
6968
void start(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileLoadedListener) {
69+
this.dataFileLoadedListener = dataFileLoadedListener;
7070
DataFileClient dataFileClient = new DataFileClient(
7171
new Client(new OptlyStorage(dataFileService), LoggerFactory.getLogger(OptlyStorage.class)),
7272
LoggerFactory.getLogger(DataFileClient.class));
@@ -76,27 +76,30 @@ void start(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileL
7676
LoggerFactory.getLogger(DataFileCache.class));
7777
RequestDataFileFromClientTask requestDataFileFromClientTask =
7878
new RequestDataFileFromClientTask(projectId,
79-
dataFileService, dataFileCache,
79+
dataFileService,
80+
dataFileCache,
8081
dataFileClient,
81-
dataFileLoadedListener,
82+
this,
8283
LoggerFactory.getLogger(RequestDataFileFromClientTask.class));
83-
LoadDataFileFromCacheTask loadDataFileFromCacheTask = new LoadDataFileFromCacheTask(dataFileCache, dataFileLoadedListener);
84+
LoadDataFileFromCacheTask loadDataFileFromCacheTask = new LoadDataFileFromCacheTask(dataFileCache, this);
8485

8586
// Execute tasks in order
8687
loadDataFileFromCacheTask.executeOnExecutor(executor);
8788
requestDataFileFromClientTask.executeOnExecutor(executor);
8889
}
90+
91+
8992
}
9093

9194
static class LoadDataFileFromCacheTask extends AsyncTask<Void, Void, JSONObject> {
9295

9396
@NonNull private final DataFileCache dataFileCache;
94-
@Nullable private final DataFileLoadedListener dataFileLoadedListener;
97+
@NonNull private final TaskChain taskChain;
9598

9699
LoadDataFileFromCacheTask(@NonNull DataFileCache dataFileCache,
97-
@Nullable DataFileLoadedListener dataFileLoadedListener) {
100+
@NonNull TaskChain taskChain) {
98101
this.dataFileCache = dataFileCache;
99-
this.dataFileLoadedListener = dataFileLoadedListener;
102+
this.taskChain = taskChain;
100103
}
101104

102105
@Override
@@ -107,8 +110,14 @@ protected JSONObject doInBackground(Void... params) {
107110
@Override
108111
protected void onPostExecute(JSONObject dataFile) {
109112
if (dataFile != null) {
110-
if (dataFileLoadedListener != null) {
111-
dataFileLoadedListener.onDataFileLoaded(dataFile.toString());
113+
if (taskChain.dataFileLoadedListener != null) {
114+
taskChain.dataFileLoadedListener.onDataFileLoaded(dataFile.toString());
115+
// Prevents the callback from being hit twice
116+
// if the CDN datafile is different from the cached
117+
// datafile. The service will still get the remote
118+
// datafile and update the cache but Optimizely
119+
// will be started from the cached datafile.
120+
taskChain.dataFileLoadedListener = null;
112121
}
113122
}
114123
}
@@ -122,51 +131,51 @@ static class RequestDataFileFromClientTask extends AsyncTask<Void, Void, String>
122131
@NonNull private final DataFileCache dataFileCache;
123132
@NonNull private final DataFileClient dataFileClient;
124133
@NonNull private final Logger logger;
125-
@Nullable private final DataFileLoadedListener optimizelyStartedListener;
134+
@NonNull private final TaskChain taskChain;
126135

127136
RequestDataFileFromClientTask(@NonNull String projectId,
128137
@NonNull DataFileService dataFileService,
129138
@NonNull DataFileCache dataFileCache,
130139
@NonNull DataFileClient dataFileClient,
131-
@Nullable DataFileLoadedListener dataFileLoadedListener,
140+
@NonNull TaskChain taskChain,
132141
@NonNull Logger logger) {
133142
this.projectId = projectId;
134143
this.dataFileService = dataFileService;
135144
this.dataFileCache = dataFileCache;
136145
this.dataFileClient = dataFileClient;
137-
this.optimizelyStartedListener = dataFileLoadedListener;
146+
this.taskChain = taskChain;
138147
this.logger = logger;
139148
}
140149

141150
@Override
142151
protected String doInBackground(Void... params) {
143152
String dataFile = dataFileClient.request(String.format(FORMAT_CDN_URL, projectId));
144-
if (dataFile != null) {
153+
if (dataFileClient.isModifiedResponse(dataFile)) {
145154
if (dataFileCache.exists()) {
146155
if (!dataFileCache.delete()) {
147156
logger.warn("Unable to delete old data file");
148-
return null; // Unable to delete
149157
}
150158
}
151159
if (!dataFileCache.save(dataFile)) {
152160
logger.warn("Unable to save new data file");
153-
return null;
154161
}
155162
}
156163

157164
return dataFile;
158165
}
159166

160167
@Override
161-
protected void onPostExecute(String dataFile) {
162-
// If dataFile isn't null the dataFile has been modified on the CDN because we are
163-
// using last-modified and since-last-modified headers.
164-
if (dataFile != null) {
165-
if (optimizelyStartedListener != null) {
166-
optimizelyStartedListener.onDataFileLoaded(dataFile);
168+
protected void onPostExecute(@Nullable String dataFile) {
169+
// If this is null either this is a background sync
170+
// Or a locally cached datafile was already loaded.
171+
if (taskChain.dataFileLoadedListener != null) {
172+
// An empty datafile (304) should not be possible unless someone manually
173+
// deleted the cached datafile and not the caching headers
174+
// on a rooted device.
175+
if (dataFileClient.isNotModifiedResponse(dataFile)) {
176+
taskChain.dataFileLoadedListener.onDataFileLoaded(dataFile);
167177
}
168178
}
169-
170179
// We are running in the background so stop the service
171180
dataFileService.stop();
172181
}

‎android-sdk/src/main/java/com/optimizely/ab/android/sdk/OptimizelyManager.java‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -272,10 +272,6 @@ protected void onPostExecute(UserExperimentRecord userExperimentRecord) {
272272

273273
if (optimizelyStartListener != null) {
274274
optimizelyStartListener.onStart(optimizelyClient);
275-
// Prevent the onOptimizelyStarted(AndroidOptimizely) callback from being hit twice
276-
// This could happen if the local data file is not null and is different
277-
// from the remote data file. Setting the listener to null handles this case.
278-
optimizelyStartListener = null;
279275
} else {
280276
logger.info("No listener to send Optimizely to");
281277
}
@@ -411,14 +407,26 @@ public void onServiceConnected(ComponentName className,
411407
LoggerFactory.getLogger(DataFileLoader.class));
412408
dataFileService.getDataFile(optimizelyManager.getProjectId(), dataFileLoader, new DataFileLoadedListener() {
413409
@Override
414-
public void onDataFileLoaded(String dataFile) {
410+
public void onDataFileLoaded(@Nullable String dataFile) {
411+
// App is being used, i.e. in the foreground
412+
AlarmManager alarmManager = (AlarmManager) dataFileService.getApplicationContext().getSystemService(Context.ALARM_SERVICE);
413+
ServiceScheduler.PendingIntentFactory pendingIntentFactory = new ServiceScheduler.PendingIntentFactory(dataFileService.getApplicationContext());
414+
ServiceScheduler serviceScheduler = new ServiceScheduler(alarmManager, pendingIntentFactory, LoggerFactory.getLogger(ServiceScheduler.class));
415415
if (bound) {
416-
AlarmManager alarmManager = (AlarmManager) dataFileService.getApplicationContext().getSystemService(Context.ALARM_SERVICE);
417-
ServiceScheduler.PendingIntentFactory pendingIntentFactory = new ServiceScheduler.PendingIntentFactory(dataFileService.getApplicationContext());
418-
ServiceScheduler serviceScheduler = new ServiceScheduler(alarmManager, pendingIntentFactory, LoggerFactory.getLogger(ServiceScheduler.class));
419-
AndroidUserExperimentRecord userExperimentRecord =
420-
(AndroidUserExperimentRecord) AndroidUserExperimentRecord.newInstance(optimizelyManager.getProjectId(), dataFileService.getApplicationContext());
421-
optimizelyManager.injectOptimizely(dataFileService.getApplicationContext(), userExperimentRecord, serviceScheduler, dataFile);
416+
if (dataFile != null) {
417+
AndroidUserExperimentRecord userExperimentRecord =
418+
(AndroidUserExperimentRecord) AndroidUserExperimentRecord.newInstance(optimizelyManager.getProjectId(), dataFileService.getApplicationContext());
419+
optimizelyManager.injectOptimizely(dataFileService.getApplicationContext(), userExperimentRecord, serviceScheduler, dataFile);
420+
} else {
421+
// We should always call the callback even with the dummy
422+
// instances. Devs might gate the rest of their app
423+
// based on the loading of Optimizely
424+
OptimizelyStartListener optimizelyStartListener = optimizelyManager.getOptimizelyStartListener();
425+
if (optimizelyStartListener != null) {
426+
optimizelyStartListener.onStart(optimizelyManager.getOptimizely());
427+
}
428+
}
429+
422430
}
423431
}
424432
});

0 commit comments

Comments
 (0)