Bug 1243585 - Add JSONFilePingStore with tests. r=sebastian draft
authorMichael Comella <michael.l.comella@gmail.com>
Thu, 28 Apr 2016 17:05:25 -0700
changeset 357487 6dfb6d55427ae61fe66950e14105be701ce09cd5
parent 357486 9dbbd325a46492d0de34d7086671c558e9453fe6
child 357488 813fc3e8fbe81f6ac44e595f0113ee20fd9a6a0c
push id16806
push usermichael.l.comella@gmail.com
push dateFri, 29 Apr 2016 00:52:31 +0000
reviewerssebastian
bugs1243585
milestone49.0a1
Bug 1243585 - Add JSONFilePingStore with tests. r=sebastian Note: not expected to compile. MozReview-Commit-ID: 5RTQk5m3zTx
mobile/android/base/java/org/mozilla/gecko/telemetry/stores/JSONFilePingStore.java
mobile/android/base/moz.build
mobile/android/tests/background/junit4/src/org/mozilla/gecko/telemetry/stores/TestJSONFilePingStore.java
new file mode 100644
--- /dev/null
+++ b/mobile/android/base/java/org/mozilla/gecko/telemetry/stores/JSONFilePingStore.java
@@ -0,0 +1,279 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+package org.mozilla.gecko.telemetry.stores;
+
+import android.os.Parcel;
+import android.os.Parcelable;
+import android.support.annotation.VisibleForTesting;
+import android.util.Log;
+import org.json.JSONException;
+import org.json.JSONObject;
+import org.mozilla.gecko.sync.ExtendedJSONObject;
+import org.mozilla.gecko.sync.NonObjectJSONException;
+import org.mozilla.gecko.telemetry.TelemetryPing;
+import org.mozilla.gecko.telemetry.TelemetryPingFromStore;
+import org.mozilla.gecko.util.FileUtils;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.channels.FileLock;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.Locale;
+import java.util.Set;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * An implementation of TelemetryPingStore that is backed by JSON files.
+ *
+ * This implementation seeks simplicity. Each ping to upload is stored in its own file with a
+ * name patterned after {@link #FILENAME}. The file name includes the ping's unique id - these are used to
+ * both remove pings and prune pings. During prune, the pings with the smallest ids will be removed first
+ * so these ping ids should be strictly increasing as new pings are stored; consider using data already contained
+ * in the ping, such as a sequence number.
+ *
+ * Using separate files for this store allows for less restrictive concurrency:
+ *   * requires locking: {@link #storePing(long, TelemetryPing)} writes a new file
+ *   * requires locking: {@link #getAllPings()} reads all files, including those potentially being written,
+ * hence locking
+ *   * no locking: {@link #maybePrunePings()} deletes the least recently written pings, none of which should
+ * be currently written
+ *   * no locking: {@link #onUploadAttemptComplete(Set)} deletes the given pings, none of which should be
+ * currently written
+ */
+public class JSONFilePingStore implements TelemetryPingStore {
+    private static final String LOGTAG = "Gecko" + JSONFilePingStore.class.getSimpleName();
+
+    @VisibleForTesting static final int MAX_PING_COUNT = 40; // TODO: value.
+
+    private static final String FILENAME = "ping-%s.json";
+    private static final Pattern FILENAME_PATTERN = Pattern.compile("ping-([0-9]+)\\.json");
+
+    // We keep the key names short to reduce storage size impact.
+    @VisibleForTesting static final String KEY_PAYLOAD = "p";
+    @VisibleForTesting static final String KEY_URL = "u";
+
+    private final File storeDir;
+
+    public JSONFilePingStore(final File storeDir) {
+        this.storeDir = storeDir;
+        this.storeDir.mkdirs();
+    }
+
+    @VisibleForTesting File getPingFile(final long id) {
+        final String filename = String.format(Locale.US, FILENAME, id);
+        return new File(storeDir, filename);
+    }
+
+    /**
+     * @return the ID from the filename or -1 if the id does not exist.
+     */
+    @VisibleForTesting static int getIDFromFilename(final String filename) {
+        final Matcher matcher = FILENAME_PATTERN.matcher(filename);
+        if (!matcher.matches()) { // Matcher.matches has the side effect of starting the match - needed to call Matcher.group
+            return -1;
+        }
+        return Integer.parseInt(matcher.group(1));
+    }
+
+    @Override
+    public void storePing(final long uniqueID, final TelemetryPing ping) throws IOException {
+        final String output;
+        try {
+            output = new JSONObject()
+                    .put(KEY_PAYLOAD, ping.getPayload())
+                    .put(KEY_URL, ping.getURL())
+                    .toString();
+        } catch (final JSONException e) {
+            // Do not log the exception to avoid leaking personal data.
+            throw new IOException("Unable to create JSON to store to disk");
+        }
+
+        final FileOutputStream outputStream = new FileOutputStream(getPingFile(uniqueID), false);
+        blockForLockAndWriteFileAndCloseStream(outputStream, output);
+    }
+
+    @Override
+    public void maybePrunePings() {
+        final File[] files = storeDir.listFiles(new PingFileFilter());
+        if (files.length < MAX_PING_COUNT) {
+            return;
+        }
+
+        final SortedSet<Integer> ids = getIDsFromFileList(files);
+        removeFilesWithSmallestIDs(ids, files.length - MAX_PING_COUNT);
+    }
+
+    private static SortedSet<Integer> getIDsFromFileList(final File[] files) {
+        // Since we get the ids from a file list which is probably sorted, I assume a TreeSet will get unbalanced
+        // and could be inefficient. However, it sounds less complex and more efficient than the alternative: the
+        // ConcurrentSkipListSet. In any case, our data is relatively small.
+        final SortedSet<Integer> out = new TreeSet<>();
+        for (final File file : files) {
+            final int id = getIDFromFilename(file.getName());
+            if (id >= 0) {
+                out.add(id);
+            }
+        }
+        return out;
+    }
+
+    private void removeFilesWithSmallestIDs(final SortedSet<Integer> ids, final int numFilesToRemove) {
+        final Iterator<Integer> it = ids.iterator();
+        int i = 0;
+        while (i < numFilesToRemove) { // Sorted set so these are ascending values.
+            i += 1;
+            final Integer id = it.next(); // file count > files to remove so this should not throw.
+            getPingFile(id).delete();
+        }
+    }
+
+    @Override
+    public ArrayList<TelemetryPingFromStore> getAllPings() {
+        final File[] files = storeDir.listFiles(new PingFileFilter());
+        final ArrayList<TelemetryPingFromStore> out = new ArrayList<>(files.length);
+        for (final File file : files) {
+            final FileInputStream inputStream;
+            try {
+                inputStream = new FileInputStream(file);
+            } catch (final FileNotFoundException e) {
+                throw new IllegalStateException("Expected file to exist");
+            }
+
+            final JSONObject obj;
+            try {
+                // Potential optimization: re-use the same buffer for reading from files.
+                obj = lockAndReadFileAndCloseStream(inputStream, (int) file.length());
+            } catch (final IOException | JSONException e) {
+                // We couldn't read this file so let's just skip it. These potentially
+                // corrupted files should be removed when the data is pruned.
+                Log.w(LOGTAG, "Error when reading file: " + file.getName() + " Likely corrupted. Ignoring");
+                continue;
+            }
+
+            if (obj == null) {
+                Log.d(LOGTAG, "Could not read given file: " + file.getName() + " File is locked. Ignoring");
+                continue;
+            }
+
+            try {
+                final String url = obj.getString(KEY_URL);
+                final ExtendedJSONObject payload = new ExtendedJSONObject(obj.getString(KEY_PAYLOAD));
+                final int id = getIDFromFilename(file.getName());
+                if (id < 0) {
+                    throw new IllegalStateException("These files are already filtered - did not expect to see " +
+                            "an invalid ID in these files");
+                }
+                out.add(new TelemetryPingFromStore(url, payload, id));
+            } catch (final IOException | JSONException | NonObjectJSONException e) {
+                Log.w(LOGTAG, "Bad json in ping. Ignoring.");
+                continue;
+            }
+        }
+        return out;
+    }
+
+    @Override
+    public void onUploadAttemptComplete(final Set<Integer> successfulRemoveIDs) {
+        final File[] files = storeDir.listFiles(new PingFileFilter(successfulRemoveIDs));
+        for (final File file : files) {
+            file.delete();
+        }
+    }
+
+    /**
+     * Locks the given {@link FileOutputStream} and writes the given String. This method will close the given stream.
+     *
+     * Note: this method blocks until a file lock can be acquired.
+     */
+    private static void blockForLockAndWriteFileAndCloseStream(final FileOutputStream outputStream, final String str)
+            throws IOException {
+        try {
+            final FileLock lock = outputStream.getChannel().lock(0, Long.MAX_VALUE, false);
+            if (lock != null) {
+                // The file lock is released when the stream is closed. If we try to redundantly close it, we get
+                // a ClosedChannelException. To be safe, we could catch that every time but there is a performance
+                // hit to exception handling so instead we assume the file lock will be closed.
+                FileUtils.writeStringToOutputStreamAndCloseStream(outputStream, str);
+            }
+        } finally {
+            outputStream.close(); // redundant: closed when the stream is closed, but let's be safe.
+        }
+    }
+
+    /**
+     * Locks the given {@link FileInputStream} and reads the data. This method will close the given stream.
+     *
+     * Note: this method returns null when a lock could not be acquired.
+     */
+    private static JSONObject lockAndReadFileAndCloseStream(final FileInputStream inputStream, final int fileSize)
+            throws IOException, JSONException {
+        try {
+            final FileLock lock = inputStream.getChannel().tryLock(0, Long.MAX_VALUE, true); // null when lock not acquired
+            if (lock == null) {
+                return null;
+            }
+            // The file lock is released when the stream is closed. If we try to redundantly close it, we get
+            // a ClosedChannelException. To be safe, we could catch that every time but there is a performance
+            // hit to exception handling so instead we assume the file lock will be closed.
+            return new JSONObject(FileUtils.readStringFromInputStreamAndCloseStream(inputStream, fileSize));
+        } finally {
+            inputStream.close(); // redundant: closed when the steram is closed, but let's be safe.
+        }
+    }
+
+    private static class PingFileFilter implements FilenameFilter {
+        private final Set<Integer> idsToFilter;
+
+        public PingFileFilter() {
+            this(null);
+        }
+
+        public PingFileFilter(final Set<Integer> idsToFilter) {
+            this.idsToFilter = idsToFilter;
+        }
+
+        @Override
+        public boolean accept(final File dir, final String filename) {
+            if (idsToFilter == null) {
+                return FILENAME_PATTERN.matcher(filename).matches();
+            }
+
+            return idsToFilter.contains(getIDFromFilename(filename));
+        }
+    }
+
+    public static final Parcelable.Creator<JSONFilePingStore> CREATOR = new Parcelable.Creator<JSONFilePingStore>() {
+        @Override
+        public JSONFilePingStore createFromParcel(final Parcel source) {
+            final String storeDirPath = source.readString();
+            return new JSONFilePingStore(new File(storeDirPath));
+        }
+
+        @Override
+        public JSONFilePingStore[] newArray(final int size) {
+            return new JSONFilePingStore[size];
+        }
+    };
+
+    @Override
+    public int describeContents() {
+        return 0;
+    }
+
+    @Override
+    public void writeToParcel(final Parcel dest, final int flags) {
+        dest.writeString(storeDir.getAbsolutePath());
+    }
+}
--- a/mobile/android/base/moz.build
+++ b/mobile/android/base/moz.build
@@ -569,16 +569,17 @@ gbjar.sources += ['java/org/mozilla/geck
     'tabs/TabsLayoutItemView.java',
     'tabs/TabsListLayout.java',
     'tabs/TabsPanel.java',
     'tabs/TabsPanelThumbnailView.java',
     'Telemetry.java',
     'telemetry/core/TelemetryCorePingBuilder.java',
     'telemetry/schedulers/TelemetryUploadAllPingsImmediatelyScheduler.java',
     'telemetry/schedulers/TelemetryUploadScheduler.java',
+    'telemetry/stores/JSONFilePingStore.java',
     'telemetry/stores/TelemetryPingStore.java',
     'telemetry/TelemetryConstants.java',
     'telemetry/TelemetryDispatcher.java',
     'telemetry/TelemetryPing.java',
     'telemetry/TelemetryPingBuilder.java',
     'telemetry/TelemetryPingFromStore.java',
     'telemetry/TelemetryUploadService.java',
     'TelemetryContract.java',
new file mode 100644
--- /dev/null
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/telemetry/stores/TestJSONFilePingStore.java
@@ -0,0 +1,203 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+package org.mozilla.gecko.telemetry.stores;
+
+import org.json.JSONObject;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mozilla.gecko.background.testhelpers.TestRunner;
+import org.mozilla.gecko.sync.ExtendedJSONObject;
+import org.mozilla.gecko.telemetry.TelemetryPing;
+import org.mozilla.gecko.telemetry.TelemetryPingFromStore;
+import org.mozilla.gecko.util.FileUtils;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static org.junit.Assert.*;
+
+/**
+ * Unit test methods of the {@link JSONFilePingStore} class.
+ */
+@RunWith(TestRunner.class)
+public class TestJSONFilePingStore {
+
+    private final Pattern ID_PATTERN = Pattern.compile("[^0-9]*([0-9]+)[^0-9]*");
+
+    @Rule
+    public TemporaryFolder tempDir = new TemporaryFolder();
+    private File testDir;
+    private JSONFilePingStore testStore;
+
+    @Before
+    public void setUp() throws Exception {
+        testDir = tempDir.newFolder();
+        testStore = new JSONFilePingStore(testDir);
+    }
+
+    private ExtendedJSONObject generateTelemetryPayload() {
+        final ExtendedJSONObject out = new ExtendedJSONObject();
+        out.put("str", "a String");
+        out.put("int", 42);
+        out.put("null", (ExtendedJSONObject) null);
+        return out;
+    }
+
+    private void assertIsGeneratedPayload(final ExtendedJSONObject actual) throws Exception {
+        assertNull("Null field is null", actual.getObject("null"));
+        assertEquals("int field is correct", 42, (int) actual.getIntegerSafely("int"));
+        assertEquals("str field is correct", "a String", actual.getString("str"));
+    }
+
+    private void assertStoreFileCount(final int expectedCount) {
+        assertEquals("Store contains " + expectedCount + " item(s)", expectedCount, testDir.list().length);
+    }
+
+    @Test
+    public void testConstructorOnlyWritesToGivenDir() throws Exception {
+        // Constructor is called in @Before method
+        assertTrue("Store dir exists", testDir.exists());
+        assertEquals("Temp dir contains one dir (the store dir)", 1, tempDir.getRoot().list().length);
+    }
+
+    @Test
+    public void testStorePingStoresCorrectData() throws Exception {
+        assertStoreFileCount(0);
+
+        final int expectedID = 48679;
+        final TelemetryPing expectedPing = new TelemetryPing("a/server/url", generateTelemetryPayload());
+        testStore.storePing(expectedID, expectedPing);
+
+        assertStoreFileCount(1);
+        final String filename = testDir.list()[0];
+        assertTrue("Filename contains expected ID", filename.contains(Integer.toString(expectedID)));
+        final JSONObject actual = FileUtils.readJSONObjectFromFile(new File(testDir, filename));
+        assertEquals("Ping urls are equal", expectedPing.getURL(), actual.getString(JSONFilePingStore.KEY_URL));
+        assertIsGeneratedPayload(new ExtendedJSONObject(actual.getString(JSONFilePingStore.KEY_PAYLOAD)));
+    }
+
+    @Test
+    public void testStorePingMultiplePingsStoreSeparateFiles() throws Exception {
+        assertStoreFileCount(0);
+        for (int i = 1; i < 10; ++i) {
+            testStore.storePing(i, new TelemetryPing("server " + i, generateTelemetryPayload()));
+            assertStoreFileCount(i);
+        }
+    }
+
+    @Test
+    public void testStorePingReleasesFileLock() throws Exception {
+        assertStoreFileCount(0);
+        testStore.storePing(0, new TelemetryPing("server", generateTelemetryPayload()));
+        assertStoreFileCount(1);
+        final File file = new File(testDir, testDir.list()[0]);
+        final FileOutputStream stream = new FileOutputStream(file);
+        try {
+            assertNotNull("File lock is released after store write", stream.getChannel().tryLock());
+        } finally {
+            stream.close(); // releases lock
+        }
+    }
+
+    @Test
+    public void testGetAllPings() throws Exception {
+        final String urlPrefix = "url";
+        writeTestPingsToStore(3, urlPrefix);
+
+        final ArrayList<TelemetryPingFromStore> pings = testStore.getAllPings();
+        for (final TelemetryPingFromStore ping : pings) {
+            final int id = ping.getUniqueID(); // we use ID as a key for specific pings and check against the url values.
+            assertEquals("Expected url value received", urlPrefix + id, ping.getURL());
+            assertIsGeneratedPayload(ping.getPayload());
+        }
+    }
+
+    @Test
+    public void testMaybePrunePingsDoesNothingIfAtMax() throws Exception {
+        final int pingCount = JSONFilePingStore.MAX_PING_COUNT;
+        writeTestPingsToStore(pingCount, "whatever");
+        assertStoreFileCount(pingCount);
+        testStore.maybePrunePings();
+        assertStoreFileCount(pingCount);
+    }
+
+    @Test
+    public void testMaybePrunePingsPrunesIfAboveMax() throws Exception {
+        final int pingCount = JSONFilePingStore.MAX_PING_COUNT + 1;
+        writeTestPingsToStore(pingCount, "whatever");
+        assertStoreFileCount(pingCount);
+        testStore.maybePrunePings();
+        assertStoreFileCount(JSONFilePingStore.MAX_PING_COUNT);
+
+        final HashSet<Integer> existingIDs = new HashSet<>(JSONFilePingStore.MAX_PING_COUNT);
+        for (final String filename : testDir.list()) {
+            existingIDs.add(getIDFromFilename(filename));
+        }
+        assertFalse("Smallest ID was removed", existingIDs.contains(1));
+    }
+
+    @Test
+    public void testOnUploadAttemptCompleted() throws Exception {
+        writeTestPingsToStore(10, "url");
+        final HashSet<Integer> unuploadedPingIDs = new HashSet<>(Arrays.asList(1, 3, 5, 7, 9));
+        final HashSet<Integer> removedPingIDs = new HashSet<>(Arrays.asList(2, 4, 6, 8, 10));
+        testStore.onUploadAttemptComplete(removedPingIDs);
+
+        for (final String unuploadedFilePath : testDir.list()) {
+            final int unuploadedID = getIDFromFilename(unuploadedFilePath);
+            assertFalse("Unuploaded ID is not in removed ping IDs", removedPingIDs.contains(unuploadedID));
+            assertTrue("Unuploaded ID is in unuploaded ping IDs", unuploadedPingIDs.contains(unuploadedID));
+            unuploadedPingIDs.remove(unuploadedID);
+        }
+        assertTrue("All non-successful-upload ping IDs were matched", unuploadedPingIDs.isEmpty());
+    }
+
+    @Test
+    public void testGetPingFileContainsID() throws Exception {
+        final int expected = 1234567890;
+        final File file = testStore.getPingFile(expected);
+        assertTrue("Ping filename contains ID", file.getName().contains(Integer.toString(expected)));
+    }
+
+    @Test // assumes {@link JSONFilePingStore.getPingFile(String)} is working.
+    public void testGetIDFromFilename() throws Exception {
+        final int expectedID = 465739201;
+        final File file = testStore.getPingFile(expectedID);
+        assertEquals("Retrieved ID from filename", expectedID, JSONFilePingStore.getIDFromFilename(file.getName()));
+    }
+
+    private int getIDFromFilename(final String filename) {
+        final Matcher matcher = ID_PATTERN.matcher(filename);
+        assertTrue("Filename contains ID", matcher.matches());
+        return Integer.parseInt(matcher.group(1));
+    }
+
+    /**
+     * Writes pings to store without using store API with:
+     *   id = 1 to count (inclusive)
+     *   server = urlPrefix + id
+     *   payload = generated payload
+     *
+     * Note: assumes {@link JSONFilePingStore#getPingFile(long)} works.
+     */
+    private void writeTestPingsToStore(final int count, final String urlPrefix) throws Exception {
+        for (int i = 1; i <= count; ++i) {
+            final JSONObject obj = new JSONObject()
+                    .put(JSONFilePingStore.KEY_URL, urlPrefix + i)
+                    .put(JSONFilePingStore.KEY_PAYLOAD, generateTelemetryPayload());
+            FileUtils.writeJSONObjectToFile(testStore.getPingFile(i), obj);
+        }
+    }
+}