Bug 1243585 - Add JSONFilePingStore with tests. r=sebastian
Note: not expected to compile.
MozReview-Commit-ID: 5RTQk5m3zTx
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);
+ }
+ }
+}