Bug 1465380 - Add handling of objects and arrays to PlacesUtils.metadata, and add optional default values for get(). r=mak
authorMark Banner <standard8@mozilla.com>
Wed, 30 May 2018 11:36:31 +0100
changeset 420681 b489bcf8e20d3e48beb4fa11ffb777814dc84873
parent 420680 e0b837323b48d8e152987dcb1539dd800c815a6b
child 420682 2968c0a2141001407083e05a8bf4609bff91e985
push id64702
push usermbanner@mozilla.com
push dateThu, 31 May 2018 16:12:49 +0000
treeherderautoland@b489bcf8e20d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1465380
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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 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));
   },
 
   /**
    * 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();
+});