Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get(). r=mak

MozReview-Commit-ID: 8OQxAus4rXY

--HG--
extra : rebase_source : e0ef0bba5873485562a2e85c0df68c4015dc3bd7

toolkit/components/places/PlacesSyncUtils.jsm

@@ -82,10 +82,9 @@ const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({
   /**
    * Returns the current history sync ID, or `""` if one isn't set.
    */
-  async getSyncId() {
-    let syncId = await PlacesUtils.metadata.get(
-      HistorySyncUtils.SYNC_ID_META_KEY);
-    return syncId || "";
+  getSyncId() {
+    return PlacesUtils.metadata.get(
+      HistorySyncUtils.SYNC_ID_META_KEY, "");
   },
 
   /**
@@ -124,7 +123,7 @@ const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({
       "HistorySyncUtils: ensureCurrentSyncId",
       async function(db) {
         let existingSyncId = await PlacesUtils.metadata.getWithConnection(
-          db, HistorySyncUtils.SYNC_ID_META_KEY);
+          db, HistorySyncUtils.SYNC_ID_META_KEY, "");
 
         if (existingSyncId == newSyncId) {
           HistorySyncLog.trace("History sync ID up-to-date",
@@ -147,8 +146,8 @@ const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({
    */
   async getLastSync() {
     let lastSync = await PlacesUtils.metadata.get(
-      HistorySyncUtils.LAST_SYNC_META_KEY);
-    return lastSync ? lastSync / 1000 : 0;
+      HistorySyncUtils.LAST_SYNC_META_KEY, 0);
+    return lastSync / 1000;
   },
 
   /**
@@ -407,10 +406,9 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
   /**
    * Returns the current bookmarks sync ID, or `""` if one isn't set.
    */
-  async getSyncId() {
-    let syncId = await PlacesUtils.metadata.get(
-      BookmarkSyncUtils.SYNC_ID_META_KEY);
-    return syncId || "";
+  getSyncId() {
+    return PlacesUtils.metadata.get(
+      BookmarkSyncUtils.SYNC_ID_META_KEY, "");
   },
 
   /**
@@ -420,7 +418,7 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
    */
   async shouldWipeRemote() {
     let shouldWipeRemote = await PlacesUtils.metadata.get(
-      BookmarkSyncUtils.WIPE_REMOTE_META_KEY);
+      BookmarkSyncUtils.WIPE_REMOTE_META_KEY, false);
     return !!shouldWipeRemote;
   },
 
@@ -470,7 +468,7 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
       "BookmarkSyncUtils: ensureCurrentSyncId",
       async function(db) {
         let existingSyncId = await PlacesUtils.metadata.getWithConnection(
-          db, BookmarkSyncUtils.SYNC_ID_META_KEY);
+          db, BookmarkSyncUtils.SYNC_ID_META_KEY, "");
 
         // If we don't have a sync ID, take the server's without resetting
         // sync statuses.
@@ -507,8 +505,8 @@ const BookmarkSyncUtils = PlacesSyncUtils.bookmarks = Object.freeze({
    */
   async getLastSync() {
     let lastSync = await PlacesUtils.metadata.get(
-      BookmarkSyncUtils.LAST_SYNC_META_KEY);
-    return lastSync ? lastSync / 1000 : 0;
+      BookmarkSyncUtils.LAST_SYNC_META_KEY, 0);
+    return lastSync / 1000;
   },
 
   /**

toolkit/components/places/PlacesUtils.jsm

@@ -1988,18 +1988,25 @@ XPCOMUtils.defineLazyGetter(this, "gAsyncDBWrapperPromised",
  */
 PlacesUtils.metadata = {
   cache: new Map(),
+  jsonPrefix: "data:application/json;base64,",
 
   /**
    * Returns the value associated with a metadata key.
    *
    * @param  {String} key
    *         The metadata key to look up.
-   * @return {*}
-   *         The value associated with the key, or `null` if not set.
+   * @param  {String|Object|Array} defaultValue
+   *         Optional. The default value to return if the value is not present,
+   *         or cannot be parsed.
+   * @resolves {*}
+   *         The value associated with the key, or the defaultValue if there is one.
+   * @rejects
+   *         Rejected if the value is not found or it cannot be parsed
+   *         and there is no defaultValue.
    */
-  get(key) {
+  get(key, defaultValue) {
     return PlacesUtils.withConnectionWrapper("PlacesUtils.metadata.get",
-      db => this.getWithConnection(db, key));
+      db => this.getWithConnection(db, key, defaultValue));
   },
 
   /**
@@ -2026,7 +2033,7 @@ PlacesUtils.metadata = {
       db => this.deleteWithConnection(db, ...keys));
   },
 
-  async getWithConnection(db, key) {
+  async getWithConnection(db, key, defaultValue) {
     key = this.canonicalizeKey(key);
     if (this.cache.has(key)) {
       return this.cache.get(key);
@@ -2039,8 +2046,26 @@ PlacesUtils.metadata = {
       let row = rows[0];
       let rawValue = row.getResultByName("value");
       // Convert blobs back to `Uint8Array`s.
-      value = row.getTypeOfIndex(0) == row.VALUE_TYPE_BLOB ?
-              new Uint8Array(rawValue) : rawValue;
+      if (row.getTypeOfIndex(0) == row.VALUE_TYPE_BLOB) {
+        value = new Uint8Array(rawValue);
+      } else if (typeof rawValue == "string" &&
+                 rawValue.startsWith(this.jsonPrefix)) {
+        try {
+          value = JSON.parse(this._base64Decode(rawValue.substr(this.jsonPrefix.length)));
+        } catch (ex) {
+          if (defaultValue !== undefined) {
+            value = defaultValue;
+          } else {
+            throw ex;
+          }
+        }
+      } else {
+        value = rawValue;
+      }
+    } else if (defaultValue !== undefined) {
+      value = defaultValue;
+    } else {
+      throw new Error(`No data stored for key ${key}`);
     }
     this.cache.set(key, value);
     return value;
@@ -2051,12 +2076,18 @@ PlacesUtils.metadata = {
       await this.deleteWithConnection(db, key);
       return;
     }
+
+    let cacheValue = value;
+    if (typeof value == "object" && ChromeUtils.getClassName(value) != "Uint8Array") {
+      value = this.jsonPrefix + this._base64Encode(JSON.stringify(value));
+    }
+
     key = this.canonicalizeKey(key);
     await db.executeCached(`
       REPLACE INTO moz_meta (key, value)
       VALUES (:key, :value)`,
       { key, value });
-    this.cache.set(key, value);
+    this.cache.set(key, cacheValue);
   },
 
   async deleteWithConnection(db, ...keys) {
@@ -2079,6 +2110,17 @@ PlacesUtils.metadata = {
     }
     return key.toLowerCase();
   },
+
+  _base64Encode(str) {
+    return ChromeUtils.base64URLEncode(
+      new TextEncoder("utf-8").encode(str),
+      {pad: true});
+  },
+
+  _base64Decode(str) {
+    return new TextDecoder("utf-8").decode(
+      ChromeUtils.base64URLDecode(str, {padding: "require"}));
+  },
 };
 
 /**

toolkit/components/places/tests/unit/test_metadata.js

@@ -27,27 +27,38 @@ add_task(async function test_metadata() {
   Assert.equal(await PlacesUtils.metadata.get("test/string"), "hi",
     "Should return string value after clearing cache");
 
+  await Assert.rejects(PlacesUtils.metadata.get("test/nonexistent"),
+    /No data stored for key test\/nonexistent/,
+    "Should reject for a non-existent key and no default value.");
+  Assert.equal(await PlacesUtils.metadata.get("test/nonexistent", "defaultValue"), "defaultValue",
+    "Should return the default value for a non-existent key.");
+
   // Values are untyped; it's OK to store a value of a different type for the
   // same key.
   await PlacesUtils.metadata.set("test/string", 111);
   Assert.strictEqual(await PlacesUtils.metadata.get("test/string"), 111,
     "Should replace string with integer");
   await PlacesUtils.metadata.set("test/string", null);
-  Assert.strictEqual(await PlacesUtils.metadata.get("test/string"), null,
+  await Assert.rejects(PlacesUtils.metadata.get("test/string"),
+    /No data stored for key test\/string/,
     "Should clear value when setting to NULL");
 
   await PlacesUtils.metadata.delete("test/string", "test/boolean");
-  Assert.strictEqual(await PlacesUtils.metadata.get("test/string"), null,
+  await Assert.rejects(PlacesUtils.metadata.get("test/string"),
+    /No data stored for key test\/string/,
     "Should delete string value");
-  Assert.strictEqual(await PlacesUtils.metadata.get("test/boolean"), null,
+  await Assert.rejects(PlacesUtils.metadata.get("test/boolean"),
+    /No data stored for key test\/boolean/,
     "Should delete Boolean value");
   Assert.strictEqual(await PlacesUtils.metadata.get("test/integer"), 123,
     "Should keep undeleted integer value");
 
   await PlacesTestUtils.clearMetadata();
-  Assert.strictEqual(await PlacesUtils.metadata.get("test/integer"), null,
+  await Assert.rejects(PlacesUtils.metadata.get("test/integer"),
+    /No data stored for key test\/integer/,
     "Should clear integer value");
-  Assert.strictEqual(await PlacesUtils.metadata.get("test/double"), null,
+  await Assert.rejects(PlacesUtils.metadata.get("test/double"),
+    /No data stored for key test\/double/,
     "Should clear double value");
 });
 
@@ -80,7 +91,7 @@ add_task(async function test_metadata_blobs() {
   let sameBlob = await PlacesUtils.metadata.get("test/blob");
   Assert.equal(ChromeUtils.getClassName(sameBlob), "Uint8Array",
     "Should cache typed array for blob value");
-  deepEqual(sameBlob, blob,
+  Assert.deepEqual(sameBlob, blob,
     "Should store new blob value");
 
   info("Remove blob from cache");
@@ -89,8 +100,67 @@ add_task(async function test_metadata_blobs() {
   let newBlob = await PlacesUtils.metadata.get("test/blob");
   Assert.equal(ChromeUtils.getClassName(newBlob), "Uint8Array",
     "Should inflate blob into typed array");
-  deepEqual(newBlob, blob,
+  Assert.deepEqual(newBlob, blob,
     "Should return same blob after clearing cache");
 
   await PlacesTestUtils.clearMetadata();
 });
+
+add_task(async function test_metadata_arrays() {
+  let array = [1, 2, 3, "\u2713 \u00E0 la mode"];
+  await PlacesUtils.metadata.set("test/array", array);
+
+  let sameArray = await PlacesUtils.metadata.get("test/array");
+  Assert.ok(Array.isArray(sameArray), "Should cache array for array value");
+  Assert.deepEqual(sameArray, array,
+    "Should store new array value");
+
+  info("Remove array from cache");
+  await PlacesUtils.metadata.cache.clear();
+
+  let newArray = await PlacesUtils.metadata.get("test/array");
+  Assert.ok(Array.isArray(newArray), "Should inflate into array");
+  Assert.deepEqual(newArray, array,
+    "Should return same array after clearing cache");
+
+  await PlacesTestUtils.clearMetadata();
+});
+
+add_task(async function test_metadata_objects() {
+  let object = {foo: 123, bar: "test", meow: "\u2713 \u00E0 la mode"};
+  await PlacesUtils.metadata.set("test/object", object);
+
+  let sameObject = await PlacesUtils.metadata.get("test/object");
+  Assert.equal(typeof sameObject, "object", "Should cache object for object value");
+  Assert.deepEqual(sameObject, object,
+    "Should store new object value");
+
+  info("Remove object from cache");
+  await PlacesUtils.metadata.cache.clear();
+
+  let newObject = await PlacesUtils.metadata.get("test/object");
+  Assert.equal(typeof newObject, "object", "Should inflate into object");
+  Assert.deepEqual(newObject, object,
+    "Should return same object after clearing cache");
+
+  await PlacesTestUtils.clearMetadata();
+});
+
+add_task(async function test_metadata_unparsable() {
+  await PlacesUtils.withConnectionWrapper("test_medata", db => {
+    let data = PlacesUtils.metadata._base64Encode("{hjjkhj}");
+
+    return db.execute(`
+      INSERT INTO moz_meta (key, value)
+      VALUES ("test/unparsable", "data:application/json;base64,${data}")
+    `);
+  });
+
+  await Assert.rejects(PlacesUtils.metadata.get("test/unparsable"),
+    /SyntaxError: JSON.parse/,
+    "Should reject for an unparsable value with no default");
+  Assert.deepEqual(await PlacesUtils.metadata.get("test/unparsable", {foo: 1}),
+    {foo: 1}, "Should return the default when encountering an unparsable value.");
+
+  await PlacesTestUtils.clearMetadata();
+});