Close file streams in Cache to release resources and prevent crashes; Refactor exists(); Other minor cleanup (#130)

Author: elliotykim

datafile-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileCache.java

@@ -18,6 +18,7 @@
 
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
+import android.support.annotation.VisibleForTesting;
 
 import com.optimizely.ab.android.shared.Cache;
 
@@ -26,51 +27,51 @@
 import org.slf4j.Logger;
 
 /**
- * Abstracts the actual data "file" {@link java.io.File}
+ * Abstracts the actual data "file" {@link java.io.File}.
  */
 public class DatafileCache {
 
-    private static final String OPTLY_DATA_FILE_NAME = "optly-data-file-%s.json";
+    private static final String FILENAME = "optly-data-file-%s.json";
 
     @NonNull private final Cache cache;
-    @NonNull private final String projectId;
+    @NonNull private final String filename;
     @NonNull private final Logger logger;
 
     public DatafileCache(@NonNull String projectId, @NonNull Cache cache, @NonNull Logger logger) {
         this.cache = cache;
-        this.projectId = projectId;
+        this.filename = String.format(FILENAME, projectId);
         this.logger = logger;
     }
 
+    public boolean delete() {
+        return cache.delete(filename);
+    }
+
+    public boolean exists() {
+        return cache.exists(filename);
+    }
+
+    @VisibleForTesting
+    public String getFileName() {
+        return filename;
+    }
+
     @Nullable
     public JSONObject load() {
-        String optlyDatafile = cache.load(getFileName());
+        String datafile = cache.load(filename);
 
-        if (optlyDatafile == null) {
+        if (datafile == null) {
             return null;
         }
         try {
-            return new JSONObject(optlyDatafile);
+            return new JSONObject(datafile);
         } catch (JSONException e) {
             logger.error("Unable to parse data file", e);
             return null;
         }
-
-    }
-
-    public boolean delete() {
-        return cache.delete(getFileName());
-    }
-
-    public boolean exists() {
-        return cache.exists(getFileName());
     }
 
     public boolean save(String dataFile) {
-        return cache.save(getFileName(), dataFile);
-    }
-
-    public String getFileName() {
-        return String.format(OPTLY_DATA_FILE_NAME, projectId);
+        return cache.save(filename, dataFile);
     }
 }

datafile-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileLoader.java

@@ -33,11 +33,11 @@
  */
 public class DatafileLoader {
 
+    @NonNull private final DatafileCache datafileCache;
+    @NonNull private final DatafileClient datafileClient;
     @NonNull private final DatafileService datafileService;
     @NonNull private final Executor executor;
     @NonNull private final Logger logger;
-    @NonNull private final DatafileCache datafileCache;
-    @NonNull private final DatafileClient datafileClient;
 
     private boolean hasNotifiedListener = false;
 
@@ -54,9 +54,7 @@ public DatafileLoader(@NonNull DatafileService datafileService,
 
         new DatafileServiceConnection("projectId", datafileService.getApplicationContext(), new DatafileLoadedListener() {
             public void onDatafileLoaded(@Nullable String dataFile) {}
-
             public void onStop(Context context) {}
-
         });
     }
 

shared/src/androidTest/java/com/optimizely/ab/android/shared/CacheTest.java

@@ -1,5 +1,5 @@
 /****************************************************************************
- * Copyright 2016, Optimizely, Inc. and contributors                        *
+ * Copyright 2016-2017, Optimizely, Inc. and contributors                   *
  *                                                                          *
  * Licensed under the Apache License, Version 2.0 (the "License");          *
  * you may not use this file except in compliance with the License.         *
@@ -23,16 +23,19 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mockito;
 import org.slf4j.Logger;
 
 import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
 import java.io.IOException;
 
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertNull;
 import static junit.framework.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -41,45 +44,56 @@
 @RunWith(AndroidJUnit4.class)
 public class CacheTest {
 
-    private static final String FILE_NAME = "foo.txt";
+    private static final String FILENAME = "foo.txt";
 
     private Cache cache;
+    private Logger logger;
 
     @Before
     public void setup() {
         Context context = InstrumentationRegistry.getTargetContext();
-        Logger logger = mock(Logger.class);
+        logger = mock(Logger.class);
         cache = new Cache(context, logger);
     }
 
     @Test
-    public void saveLoadAndDelete() throws IOException {
-        assertTrue(cache.save(FILE_NAME, "bar"));
-        String data = cache.load(FILE_NAME);
+    public void testSaveExistsLoadAndDelete() throws IOException {
+        assertTrue(cache.save(FILENAME, "bar"));
+        assertTrue(cache.exists(FILENAME));
+        String data = cache.load(FILENAME);
         assertEquals("bar", data);
-        assertTrue(cache.delete(FILE_NAME));
+        assertTrue(cache.delete(FILENAME));
     }
 
     @Test
-    public void deleteFileFail() {
-        assertFalse(cache.delete(FILE_NAME));
+    public void testDeleteFail() {
+        assertFalse(cache.delete(FILENAME));
+    }
+
+    @Test
+    public void testExistsFalse() {
+        assertFalse(cache.exists(FILENAME));
     }
 
     @Test
     public void testLoadFileNotFoundExceptionReturnsNull() throws FileNotFoundException {
         Context context = mock(Context.class);
-        Logger logger = mock(Logger.class);
         Cache cache = new Cache(context, logger);
-        when(context.openFileInput(FILE_NAME)).thenThrow(new FileNotFoundException());
-        assertNull(cache.load(FILE_NAME));
+        when(context.openFileInput(FILENAME)).thenThrow(new FileNotFoundException());
+        assertNull(cache.load(FILENAME));
+        verify(logger).warn("Unable to load file {}.", FILENAME);
     }
 
     @Test
-    public void testSaveFileNotFoundExceptionReturnsFalse() throws FileNotFoundException{
+    public void testSaveFail() throws IOException {
         Context context = mock(Context.class);
-        Logger logger = mock(Logger.class);
         Cache cache = new Cache(context, logger);
-        when(context.openFileInput(FILE_NAME)).thenThrow(new FileNotFoundException());
-        assertFalse(cache.save(FILE_NAME, "{}"));
+        FileOutputStream fileOutputStream = mock(FileOutputStream.class);
+
+        String data = "{}";
+        Mockito.doThrow(new IOException()).when(fileOutputStream).write(data.getBytes());
+        when(context.openFileOutput(FILENAME, Context.MODE_PRIVATE)).thenReturn(fileOutputStream);
+        assertFalse(cache.save(FILENAME, data));
+        verify(logger).error("Error saving file {}.", FILENAME);
     }
 }

shared/src/main/java/com/optimizely/ab/android/shared/Cache.java

@@ -1,5 +1,5 @@
 /****************************************************************************
- * Copyright 2016, Optimizely, Inc. and contributors                        *
+ * Copyright 2016-2017, Optimizely, Inc. and contributors                   *
  *                                                                          *
  * Licensed under the Apache License, Version 2.0 (the "License");          *
  * you may not use this file except in compliance with the License.         *
@@ -25,10 +25,13 @@
 import java.io.BufferedReader;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
+import java.io.IOException;
 import java.io.InputStreamReader;
+import java.util.Arrays;
 
 /**
- * Functionality common to all caches
+ * Functionality common to all caches.
+ *
  * @hide
  */
 public class Cache {
@@ -37,7 +40,7 @@ public class Cache {
     @NonNull private final Logger logger;
 
     /**
-     * Create new instances of {@link Cache}
+     * Create new instance of {@link Cache}.
      *
      * @param context any {@link Context instance}
      * @param logger  a {@link Logger} instances
@@ -49,17 +52,44 @@ public Cache(@NonNull Context context, @NonNull Logger logger) {
     }
 
     /**
-     * Load the cache file
+     * Delete the cache file.
      *
-     * @param fileName the path to the file
+     * @param filename the path to the file
+     * @return true if the file was deleted or false otherwise
+     * @hide
+     */
+    public boolean delete(String filename) {
+        return context.deleteFile(filename);
+    }
+
+    /**
+     * Check if the cache file exists.
+     *
+     * @param filename the path to the file
+     * @return true if the file exists or false otherwise
+     * @hide
+     */
+    public boolean exists(String filename) {
+        String[] files = context.fileList();
+        if (files == null) {
+            return false;
+        }
+        return Arrays.asList(files).contains(filename);
+    }
+
+    /**
+     * Load data from the cache file.
+     *
+     * @param filename the path to the file
      * @return the loaded cache file as String or null if the file cannot be loaded
      * @hide
      */
     @Nullable
-    public String load(String fileName) {
+    public String load(String filename) {
+        FileInputStream fileInputStream = null;
         try {
-            FileInputStream fis = context.openFileInput(fileName);
-            InputStreamReader inputStreamReader = new InputStreamReader(fis);
+            fileInputStream = context.openFileInput(filename);
+            InputStreamReader inputStreamReader = new InputStreamReader(fileInputStream);
             BufferedReader bufferedReader = new BufferedReader(inputStreamReader);
             StringBuilder sb = new StringBuilder();
             String line;
@@ -68,49 +98,44 @@ public String load(String fileName) {
             }
             return sb.toString();
         } catch (Exception e) {
-            logger.warn("Unable to load file {}.", fileName);
+            logger.warn("Unable to load file {}.", filename);
             return null;
+        } finally {
+            try {
+                if (fileInputStream != null) {
+                    fileInputStream.close();
+                }
+            } catch (IOException e) {
+                logger.warn("Unable to close file {}.", filename);
+            }
         }
     }
 
     /**
-     * Delete the cache file
-     * @param fileName the path to the file
-     * @return true if the file was deleted or false otherwise
-     * @hide
-     */
-    public boolean delete(String fileName) {
-        return context.deleteFile(fileName);
-    }
-
-    /**
-     * Check if the cache file exists
-     * @param fileName the path to the file
-     * @return true if the file exists or false otherwise
-     * @hide
-     */
-    public boolean exists(String fileName) {
-        String file = load(fileName);
-        return file != null;
-    }
-
-    /**
-     * Save the existing cache file with new data
+     * Save data to the cache file and overwrite any existing data.
      *
-     * The original file will be overwritten.
-     * @param fileName the path to the file
-     * @param data the String data to write to the file
+     * @param filename the path to the file
+     * @param data     the String data to write to the file
      * @return true if the file was saved
      * @hide
      */
-    public boolean save(String fileName, String data) {
+    public boolean save(String filename, String data) {
+        FileOutputStream fileOutputStream = null;
         try {
-            FileOutputStream fos = context.openFileOutput(fileName, Context.MODE_PRIVATE);
-            fos.write(data.getBytes());
+            fileOutputStream = context.openFileOutput(filename, Context.MODE_PRIVATE);
+            fileOutputStream.write(data.getBytes());
             return true;
         } catch (Exception e) {
-            logger.error("Error saving file {}.", fileName);
+            logger.error("Error saving file {}.", filename);
+            return false;
+        } finally {
+            if (fileOutputStream != null) {
+                try {
+                    fileOutputStream.close();
+                } catch (IOException e) {
+                    logger.warn("Unable to close file {}.", filename);
+                }
+            }
         }
-        return false;
     }
 }