Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get(). r?mak draft
authorMark Banner <standard8@mozilla.com>
Wed, 30 May 2018 11:36:31 +0100
changeset 802167 f4180743cab6a18b234dc237a8ea2c59a2e9f8ee
parent 802102 763f30c3421233a45ef9e67a695c5c241a2c8a3a
push id111846
push userbmo:standard8@mozilla.com
push dateThu, 31 May 2018 16:07:22 +0000
reviewersmak
bugs1465380
milestone62.0a1
Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get(). r?mak MozReview-Commit-ID: 8OQxAus4rXY
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/unit/test_metadata.js
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -77,20 +77,19 @@ XPCOMUtils.defineLazyGetter(this, "ROOTS
 
 const HistorySyncUtils = PlacesSyncUtils.history = Object.freeze({
   SYNC_ID_META_KEY: "sync/history/syncId",
   LAST_SYNC_META_KEY: "sync/history/lastSync",
 
   /**
    * 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, "");
   },
 
   /**
    * Assigns a new sync ID. This is called when we sync for the first time with
    * a new account, and when we're the first to sync after a node reassignment.
    *
    * @return {Promise} resolved once the ID has been updated.
    * @resolves to the new sync ID.
@@ -119,17 +118,17 @@ const HistorySyncUtils = PlacesSyncUtils
   async ensureCurrentSyncId(newSyncId) {
     if (!newSyncId || typeof newSyncId != "string") {
       throw new TypeError("Invalid new history sync ID");
     }
     await PlacesUtils.withConnectionWrapper(
       "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",
                                { existingSyncId });
           return;
         }
 
         HistorySyncLog.info("History sync ID changed; resetting metadata",
@@ -142,18 +141,18 @@ const HistorySyncUtils = PlacesSyncUtils
   },
 
   /**
    * Returns the last sync time, in seconds, for the history collection, or 0
    * if history has never synced before.
    */
   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;
   },
 
   /**
    * Updates the history collection last sync time.
    *
    * @param lastSyncSeconds
    *        The collection last sync time, in seconds, as a number or string.
    */
@@ -402,30 +401,29 @@ const BookmarkSyncUtils = PlacesSyncUtil
 
   get ROOTS() {
     return ROOTS;
   },
 
   /**
    * 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, "");
   },
 
   /**
    * Indicates if the bookmarks engine should erase all bookmarks on the server
    * and all other clients, because the user manually restored their bookmarks
    * from a backup on this client.
    */
   async shouldWipeRemote() {
     let shouldWipeRemote = await PlacesUtils.metadata.get(
-      BookmarkSyncUtils.WIPE_REMOTE_META_KEY);
+      BookmarkSyncUtils.WIPE_REMOTE_META_KEY, false);
     return !!shouldWipeRemote;
   },
 
   /**
    * Assigns a new sync ID, bumps the change counter, and flags all items as
    * "NEW" for upload. This is called when we sync for the first time with a
    * new account, when we're the first to sync after a node reassignment, and
    * on the first sync after a manual restore.
@@ -465,17 +463,17 @@ const BookmarkSyncUtils = PlacesSyncUtil
   async ensureCurrentSyncId(newSyncId) {
     if (!newSyncId || typeof newSyncId != "string") {
       throw new TypeError("Invalid new bookmarks sync ID");
     }
     await PlacesUtils.withConnectionWrapper(
       "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.
         if (!existingSyncId) {
           BookmarkSyncLog.info("Taking new bookmarks sync ID", { newSyncId });
           await db.executeTransaction(() => setBookmarksSyncId(db, newSyncId));
           return;
         }
@@ -502,18 +500,18 @@ const BookmarkSyncUtils = PlacesSyncUtil
   },
 
   /**
    * Returns the last sync time, in seconds, for the bookmarks collection, or 0
    * if bookmarks have never synced before.
    */
   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;
   },
 
   /**
    * Updates the bookmarks collection last sync time.
    *
    * @param lastSyncSeconds
    *        The collection last sync time, in seconds, as a number or string.
    */
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1983,28 +1983,35 @@ XPCOMUtils.defineLazyGetter(this, "gAsyn
  * cached in memory for faster lookups.
  *
  * Since some consumers set metadata as part of an existing operation or active
  * transaction, the API also exposes a `*withConnection` variant for each
  * method that takes an open database connection.
  */
 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 the 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));
   },
 
   /**
    * Sets the value for a metadata key.
    *
    * @param {String} key
    *        The metadata key to update.
    * @param {*}
@@ -2021,47 +2028,71 @@ PlacesUtils.metadata = {
    * @param {String...}
    *        One or more metadata keys to remove.
    */
   delete(...keys) {
     return PlacesUtils.withConnectionWrapper("PlacesUtils.metadata.delete",
       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);
     }
     let rows = await db.executeCached(`
       SELECT value FROM moz_meta WHERE key = :key`,
       { key });
     let value = null;
     if (rows.length) {
       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;
   },
 
   async setWithConnection(db, key, value) {
     if (value === null) {
       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) {
     keys = keys.map(this.canonicalizeKey);
     if (!keys.length) {
       return;
     }
     await db.execute(`
@@ -2074,16 +2105,27 @@ PlacesUtils.metadata = {
   },
 
   canonicalizeKey(key) {
     if (typeof key != "string" || !/^[a-zA-Z0-9\/]+$/.test(key)) {
       throw new TypeError("Invalid metadata key: " + key);
     }
     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"}));
+  },
 };
 
 /**
  * Keywords management API.
  * Sooner or later these keywords will merge with search aliases, this is an
  * interim API that should then be replaced by a unified one.
  * Keywords are associated with URLs and can have POST data.
  * The relations between URLs and keywords are the following:
--- a/toolkit/components/places/tests/unit/test_metadata.js
+++ b/toolkit/components/places/tests/unit/test_metadata.js
@@ -22,37 +22,48 @@ add_task(async function test_metadata() 
 
   await PlacesUtils.metadata.set("test/string", "hi");
   Assert.equal(await PlacesUtils.metadata.get("test/string"), "hi",
     "Should store new string value");
   await PlacesUtils.metadata.cache.clear();
   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");
 });
 
 add_task(async function test_metadata_canonical_keys() {
   await PlacesUtils.metadata.set("Test/Integer", 123);
   Assert.strictEqual(await PlacesUtils.metadata.get("tEsT/integer"), 123,
     "New keys should be case-insensitive");
   await PlacesUtils.metadata.set("test/integer", 456);
@@ -75,22 +86,81 @@ add_task(async function test_metadata_ca
 
 add_task(async function test_metadata_blobs() {
   let blob = new Uint8Array([1, 2, 3]);
   await PlacesUtils.metadata.set("test/blob", blob);
 
   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");
   await PlacesUtils.metadata.cache.clear();
 
   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();
+});