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.

Author: Josh Deffibaugh

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

@@ -188,6 +188,6 @@ public void handlesNullResponse() throws MalformedURLException {
     public void handlesEmptyStringResponse() throws MalformedURLException {
         URL url = new URL(String.format(DataFileLoader.RequestDataFileFromClientTask.FORMAT_CDN_URL, "1"));
         when(client.execute(any(Client.Request.class), eq(2), eq(3))).thenReturn("");
-        assertNull(dataFileClient.request(url.toString()));
+        assertEquals("", dataFileClient.request(url.toString()));
     }
 }

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

@@ -212,7 +212,5 @@ public void injectOptimizelyDoesNotDuplicateCallback() {
         } catch (InterruptedException e) {
             fail("Timed out");
         }
-
-        verify(logger).info("No listener to send Optimizely to");
     }
 }

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

@@ -17,6 +17,7 @@
 package com.optimizely.ab.android.sdk;
 
 import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 
 import com.optimizely.ab.android.shared.Client;
 
@@ -27,7 +28,7 @@
 import java.net.URL;
 
 /*
- * Makes requests to the Optly CDN to get the data file
+ * Makes requests to the Optly CDN to get the datafile
  */
 class DataFileClient {
 
@@ -39,6 +40,20 @@ class DataFileClient {
         this.logger = logger;
     }
 
+    /*
+     * If the datafile is modified on the CDN since the last time
+     * our local datafile was modified the response body will be a valid
+     * Optimizely datafile and the response code will be 200.
+     *
+     * If the datafile has not been modified since last time our local
+     * datafile was modified there will be no response body and the
+     * response code will be 304.
+     *
+     * @param urlString the CDN url of an Optimizely datafile
+     *
+     * @return a valid datafile, null, or an empty string (on 304 responses)
+     */
+    @Nullable
     String request(final String urlString) {
         Client.Request<String> request = new Client.Request<String>() {
             @Override
@@ -76,13 +91,14 @@ public String execute() {
             }
         };
 
-        // If the response was a 304 Not Modified an empty string is returned
-        // Consumers of this method expect either a valid datafile or null
-        String response = client.execute(request, 2, 3);
-        if (response != null && response.isEmpty()) {
-            response = null;
-        }
+        return client.execute(request, 2, 3);
+    }
+
+    boolean isModifiedResponse(@Nullable String dataFile) {
+        return dataFile != null && !dataFile.isEmpty();
+    }
 
-        return response;
+    boolean isNotModifiedResponse(@Nullable String dataFile) {
+        return dataFile == null || dataFile.isEmpty();
     }
 }

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

@@ -16,18 +16,23 @@
 
 package com.optimizely.ab.android.sdk;
 
+import android.support.annotation.Nullable;
+
 /**
  * Listens for new Optimizely datafiles
  *
+ * Datafiles can come from a local file or the CDN
+ *
  * @hide
  */
 interface DataFileLoadedListener {
 
     /**
      * Called with new datafile
      *
-     * @param dataFile the datafile json
+     * @param dataFile the datafile json, can be null if datafile loading failed.
+     *
      * @hide
      */
-    void onDataFileLoaded(String dataFile);
+    void onDataFileLoaded(@Nullable String dataFile);
 }

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

@@ -47,18 +47,17 @@ class DataFileLoader {
     }
 
     @RequiresApi(api = Build.VERSION_CODES.HONEYCOMB)
-    boolean getDataFile(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileLoadedListener) {
+    void getDataFile(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileLoadedListener) {
         taskChain.start(projectId, dataFileLoadedListener);
 
         logger.info("Refreshing data file");
-
-        return true;
     }
 
     static class TaskChain {
 
         @NonNull private final DataFileService dataFileService;
         @NonNull private final Executor executor;
+        @Nullable DataFileLoadedListener dataFileLoadedListener;
 
         TaskChain(@NonNull DataFileService dataFileService) {
             this.dataFileService = dataFileService;
@@ -67,6 +66,7 @@ static class TaskChain {
 
         @RequiresApi(api = Build.VERSION_CODES.HONEYCOMB)
         void start(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileLoadedListener) {
+            this.dataFileLoadedListener = dataFileLoadedListener;
             DataFileClient dataFileClient = new DataFileClient(
                     new Client(new OptlyStorage(dataFileService), LoggerFactory.getLogger(OptlyStorage.class)),
                     LoggerFactory.getLogger(DataFileClient.class));
@@ -76,27 +76,30 @@ void start(@NonNull String projectId, @Nullable DataFileLoadedListener dataFileL
                     LoggerFactory.getLogger(DataFileCache.class));
             RequestDataFileFromClientTask requestDataFileFromClientTask =
                     new RequestDataFileFromClientTask(projectId,
-                            dataFileService, dataFileCache,
+                            dataFileService,
+                            dataFileCache,
                             dataFileClient,
-                            dataFileLoadedListener,
+                            this,
                             LoggerFactory.getLogger(RequestDataFileFromClientTask.class));
-            LoadDataFileFromCacheTask loadDataFileFromCacheTask = new LoadDataFileFromCacheTask(dataFileCache, dataFileLoadedListener);
+            LoadDataFileFromCacheTask loadDataFileFromCacheTask = new LoadDataFileFromCacheTask(dataFileCache, this);
 
             // Execute tasks in order
             loadDataFileFromCacheTask.executeOnExecutor(executor);
             requestDataFileFromClientTask.executeOnExecutor(executor);
         }
+
+
     }
 
     static class LoadDataFileFromCacheTask extends AsyncTask<Void, Void, JSONObject> {
 
         @NonNull private final DataFileCache dataFileCache;
-        @Nullable private final DataFileLoadedListener dataFileLoadedListener;
+        @NonNull private final TaskChain taskChain;
 
         LoadDataFileFromCacheTask(@NonNull DataFileCache dataFileCache,
-                                  @Nullable DataFileLoadedListener dataFileLoadedListener) {
+                                  @NonNull TaskChain taskChain) {
             this.dataFileCache = dataFileCache;
-            this.dataFileLoadedListener = dataFileLoadedListener;
+            this.taskChain = taskChain;
         }
 
         @Override
@@ -107,8 +110,14 @@ protected JSONObject doInBackground(Void... params) {
         @Override
         protected void onPostExecute(JSONObject dataFile) {
             if (dataFile != null) {
-                if (dataFileLoadedListener != null) {
-                    dataFileLoadedListener.onDataFileLoaded(dataFile.toString());
+                if (taskChain.dataFileLoadedListener != null) {
+                    taskChain.dataFileLoadedListener.onDataFileLoaded(dataFile.toString());
+                    // Prevents the callback from being hit twice
+                    // if the CDN datafile is different from the cached
+                    // datafile.  The service will still get the remote
+                    // datafile and update the cache but Optimizely
+                    // will be started from the cached datafile.
+                    taskChain.dataFileLoadedListener = null;
                 }
             }
         }
@@ -122,51 +131,51 @@ static class RequestDataFileFromClientTask extends AsyncTask<Void, Void, String>
         @NonNull private final DataFileCache dataFileCache;
         @NonNull private final DataFileClient dataFileClient;
         @NonNull private final Logger logger;
-        @Nullable private final DataFileLoadedListener optimizelyStartedListener;
+        @NonNull private final TaskChain taskChain;
 
         RequestDataFileFromClientTask(@NonNull String projectId,
                                       @NonNull DataFileService dataFileService,
                                       @NonNull DataFileCache dataFileCache,
                                       @NonNull DataFileClient dataFileClient,
-                                      @Nullable DataFileLoadedListener dataFileLoadedListener,
+                                      @NonNull TaskChain taskChain,
                                       @NonNull Logger logger) {
             this.projectId = projectId;
             this.dataFileService = dataFileService;
             this.dataFileCache = dataFileCache;
             this.dataFileClient = dataFileClient;
-            this.optimizelyStartedListener = dataFileLoadedListener;
+            this.taskChain = taskChain;
             this.logger = logger;
         }
 
         @Override
         protected String doInBackground(Void... params) {
             String dataFile = dataFileClient.request(String.format(FORMAT_CDN_URL, projectId));
-            if (dataFile != null) {
+            if (dataFileClient.isModifiedResponse(dataFile)) {
                 if (dataFileCache.exists()) {
                     if (!dataFileCache.delete()) {
                         logger.warn("Unable to delete old data file");
-                        return null; // Unable to delete
                     }
                 }
                 if (!dataFileCache.save(dataFile)) {
                     logger.warn("Unable to save new data file");
-                    return null;
                 }
             }
 
             return dataFile;
         }
 
         @Override
-        protected void onPostExecute(String dataFile) {
-            // If dataFile isn't null the dataFile has been modified on the CDN because we are
-            // using last-modified and since-last-modified headers.
-            if (dataFile != null) {
-                if (optimizelyStartedListener != null) {
-                    optimizelyStartedListener.onDataFileLoaded(dataFile);
+        protected void onPostExecute(@Nullable String dataFile) {
+            // If this is null either this is a background sync
+            // Or a locally cached datafile was already loaded.
+            if (taskChain.dataFileLoadedListener != null) {
+                // An empty datafile (304) should not be possible unless someone manually
+                // deleted the cached datafile and not the caching headers
+                // on a rooted device.
+                if (dataFileClient.isNotModifiedResponse(dataFile)) {
+                    taskChain.dataFileLoadedListener.onDataFileLoaded(dataFile);
                 }
             }
-
             // We are running in the background so stop the service
             dataFileService.stop();
         }

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

@@ -272,10 +272,6 @@ protected void onPostExecute(UserExperimentRecord userExperimentRecord) {
 
                     if (optimizelyStartListener != null) {
                         optimizelyStartListener.onStart(optimizelyClient);
-                        // Prevent the onOptimizelyStarted(AndroidOptimizely) callback from being hit twice
-                        // This could happen if the local data file is not null and is different
-                        // from the remote data file.  Setting the listener to null handles this case.
-                        optimizelyStartListener = null;
                     } else {
                         logger.info("No listener to send Optimizely to");
                     }
@@ -411,14 +407,26 @@ public void onServiceConnected(ComponentName className,
                         LoggerFactory.getLogger(DataFileLoader.class));
                 dataFileService.getDataFile(optimizelyManager.getProjectId(), dataFileLoader, new DataFileLoadedListener() {
                     @Override
-                    public void onDataFileLoaded(String dataFile) {
+                    public void onDataFileLoaded(@Nullable String dataFile) {
+                        // App is being used, i.e. in the foreground
+                        AlarmManager alarmManager = (AlarmManager) dataFileService.getApplicationContext().getSystemService(Context.ALARM_SERVICE);
+                        ServiceScheduler.PendingIntentFactory pendingIntentFactory = new ServiceScheduler.PendingIntentFactory(dataFileService.getApplicationContext());
+                        ServiceScheduler serviceScheduler = new ServiceScheduler(alarmManager, pendingIntentFactory, LoggerFactory.getLogger(ServiceScheduler.class));
                         if (bound) {
-                            AlarmManager alarmManager = (AlarmManager) dataFileService.getApplicationContext().getSystemService(Context.ALARM_SERVICE);
-                            ServiceScheduler.PendingIntentFactory pendingIntentFactory = new ServiceScheduler.PendingIntentFactory(dataFileService.getApplicationContext());
-                            ServiceScheduler serviceScheduler = new ServiceScheduler(alarmManager, pendingIntentFactory, LoggerFactory.getLogger(ServiceScheduler.class));
-                            AndroidUserExperimentRecord userExperimentRecord =
-                                    (AndroidUserExperimentRecord) AndroidUserExperimentRecord.newInstance(optimizelyManager.getProjectId(), dataFileService.getApplicationContext());
-                            optimizelyManager.injectOptimizely(dataFileService.getApplicationContext(), userExperimentRecord, serviceScheduler, dataFile);
+                            if (dataFile != null) {
+                                AndroidUserExperimentRecord userExperimentRecord =
+                                        (AndroidUserExperimentRecord) AndroidUserExperimentRecord.newInstance(optimizelyManager.getProjectId(), dataFileService.getApplicationContext());
+                                optimizelyManager.injectOptimizely(dataFileService.getApplicationContext(), userExperimentRecord, serviceScheduler, dataFile);
+                            } else {
+                                // We should always call the callback even with the dummy
+                                // instances.  Devs might gate the rest of their app
+                                // based on the loading of Optimizely
+                                OptimizelyStartListener optimizelyStartListener = optimizelyManager.getOptimizelyStartListener();
+                                if (optimizelyStartListener != null) {
+                                    optimizelyStartListener.onStart(optimizelyManager.getOptimizely());
+                                }
+                            }
+
                         }
                     }
                 });