Bug 1363997 - Add support for tombstones to profileStorage. r?lchang,MattN draft
authorMark Hammond <mhammond@skippinet.com.au>
Tue, 09 May 2017 14:18:40 +1000
changeset 578506 e80ca77b2c96271524d62a4377a9c2ba1f1ed367
parent 578504 75f7f17ade163e9ce815b557de755eb7e5a2a5cf
child 628735 9080385ed37e17d0074abb48da133d4aa7a859a1
push id58942
push userbmo:markh@mozilla.com
push dateTue, 16 May 2017 04:30:43 +0000
reviewerslchang, MattN
bugs1363997
milestone55.0a1
Bug 1363997 - Add support for tombstones to profileStorage. r?lchang,MattN MozReview-Commit-ID: ladknQNOMb
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/unit/test_addressRecords_tombstones.js
browser/extensions/formautofill/test/unit/test_creditCardRecords_tombstones.js
browser/extensions/formautofill/test/unit/xpcshell.ini
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -169,33 +169,47 @@ class AutofillRecords {
    *
    * @param {Object} record
    *        The new record for saving.
    * @returns {string}
    *          The GUID of the newly added item..
    */
   add(record) {
     this.log.debug("add:", record);
-
-    let recordToSave = this._clone(record);
-    this._normalizeRecord(recordToSave);
+    let recordToSave;
+    if (record.deleted) {
+      if (!record.guid) {
+        throw new Error("you must specify the GUID when creating a tombstone");
+      }
+      if (this._findByGUID(record.guid, {includeDeleted: true})) {
+        throw new Error("a record with this GUID already exists");
+      }
+      recordToSave = {
+        guid: record.guid,
+        timeLastModified: record.timeLastModified || Date.now(),
+        deleted: true,
+      }
+    } else {
+      recordToSave = this._clone(record);
+      this._normalizeRecord(recordToSave);
 
-    let guid;
-    while (!guid || this._findByGUID(guid)) {
-      guid = gUUIDGenerator.generateUUID().toString()
-                           .replace(/[{}-]/g, "").substring(0, 12);
+      let guid;
+      while (!guid || this._findByGUID(guid)) {
+        guid = gUUIDGenerator.generateUUID().toString()
+                             .replace(/[{}-]/g, "").substring(0, 12);
+      }
+      recordToSave.guid = guid;
+
+      // Metadata
+      let now = Date.now();
+      recordToSave.timeCreated = now;
+      recordToSave.timeLastModified = now;
+      recordToSave.timeLastUsed = 0;
+      recordToSave.timesUsed = 0;
     }
-    recordToSave.guid = guid;
-
-    // Metadata
-    let now = Date.now();
-    recordToSave.timeCreated = now;
-    recordToSave.timeLastModified = now;
-    recordToSave.timeLastUsed = 0;
-    recordToSave.timesUsed = 0;
 
     this._store.data[this._collectionName].push(recordToSave);
     this._store.saveSoon();
 
     Services.obs.notifyObservers(null, "formautofill-storage-changed", "add");
     return recordToSave.guid;
   }
 
@@ -254,22 +268,40 @@ class AutofillRecords {
     Services.obs.notifyObservers(null, "formautofill-storage-changed", "notifyUsed");
   }
 
   /**
    * Removes the specified record. No error occurs if the record isn't found.
    *
    * @param  {string} guid
    *         Indicates which record to remove.
+   * @param  {?boolean} [options.keepTombstone = true]
+   *         Should we keep a tombstone for this removal?
    */
-  remove(guid) {
+  remove(guid, {keepTombstone = true} = {}) {
     this.log.debug("remove:", guid);
 
-    this._store.data[this._collectionName] =
-      this._store.data[this._collectionName].filter(record => record.guid != guid);
+    let index = this._findIndexByGUID(guid, {includeDeleted: !keepTombstone});
+    if (index == -1) {
+      this.log.warn("attempting to remove non-existing entry", guid);
+      return;
+    }
+    if (keepTombstone) {
+      let existing = this._store.data[this._collectionName][index];
+      if (existing.deleted) {
+        return; // already a tombstone - don't touch it.
+      }
+      this._store.data[this._collectionName][index] = {
+        guid,
+        timeLastModified: Date.now(),
+        deleted: true,
+      }
+    } else {
+      this._store.data[this._collectionName].splice(index, 1);
+    }
     this._store.saveSoon();
 
     Services.obs.notifyObservers(null, "formautofill-storage-changed", "remove");
   }
 
   /**
    * Returns the record with the specified GUID.
    *
@@ -294,24 +326,27 @@ class AutofillRecords {
 
   /**
    * Returns all records.
    *
    * @param   {Object} config
    *          Specifies how data will be retrieved.
    * @param   {boolean} config.noComputedFields
    *          Returns raw record without those computed fields.
+   * @param   {boolean} config.includeDeleted = false
+   *          Returns raw record without those computed fields.
    * @returns {Array.<Object>}
    *          An array containing clones of all records.
    */
   getAll(config = {}) {
     this.log.debug("getAll", config);
 
+    let records = this._store.data[this._collectionName].filter(r => !r.deleted || config.includeDeleted);
     // Records are cloned to avoid accidental modifications from outside.
-    let clonedRecords = this._store.data[this._collectionName].map(this._clone);
+    let clonedRecords = records.map(this._clone);
     clonedRecords.forEach(record => this._recordReadProcessor(record, config));
     return clonedRecords;
   }
 
   /**
    * Returns the filtered records based on input's information and searchString.
    *
    * @returns {Array.<Object>}
@@ -337,23 +372,26 @@ class AutofillRecords {
     this.log.debug("getByFilter:", "Returning", result.length, "result(s)");
     return result;
   }
 
   _clone(record) {
     return Object.assign({}, record);
   }
 
-  _findByGUID(guid) {
-    let found = this._findIndexByGUID(guid);
+  _findByGUID(guid, config) {
+    let found = this._findIndexByGUID(guid, config);
     return found < 0 ? undefined : this._store.data[this._collectionName][found];
   }
 
-  _findIndexByGUID(guid) {
-    return this._store.data[this._collectionName].findIndex(record => record.guid == guid);
+  _findIndexByGUID(guid, config = {}) {
+    let filter = record => {
+      return record.guid == guid && (config.includeDeleted || !record.deleted)
+    }
+    return this._store.data[this._collectionName].findIndex(filter);
   }
 
   _normalizeRecord(record) {
     this._recordWriteProcessor(record);
 
     for (let key in record) {
       if (!this.VALID_FIELDS.includes(key)) {
         throw new Error(`"${key}" is not a valid field.`);
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords_tombstones.js
@@ -0,0 +1,142 @@
+/**
+ * Tests tombstones in address records.
+ */
+
+"use strict";
+
+Services.prefs.setCharPref("extensions.formautofill.loglevel", "Trace");
+
+const {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
+
+const TEST_STORE_FILE_NAME = "test-profile.json";
+
+const TEST_PROFILE_1 = {
+  "given-name": "Timothy",
+  "additional-name": "John",
+  "family-name": "Berners-Lee",
+  organization: "World Wide Web Consortium",
+  "street-address": "32 Vassar Street\nMIT Room 32-G524",
+  "address-level2": "Cambridge",
+  "address-level1": "MA",
+  "postal-code": "02139",
+  country: "US",
+  tel: "+1 617 253 5702",
+  email: "timbl@w3.org",
+};
+
+let do_check_tombstone_record = (profile) => {
+  Assert.ok(profile.deleted);
+  Assert.deepEqual(Object.keys(profile).sort(),
+               ["guid", "timeLastModified", "deleted"].sort());
+};
+
+add_task(async function test_simple_tombstone() {
+  do_print("check simple tombstone semantics");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.addresses.add(TEST_PROFILE_1);
+
+  do_check_eq(profileStorage._store.data.version, 1);
+  do_check_eq(profileStorage._store.data.addresses.length, 1);
+
+  profileStorage.addresses.remove(guid);
+
+  // should be unable to get it normally.
+  Assert.equal(profileStorage.addresses.get(guid), null);
+  // and getAll should also not return it.
+  Assert.equal(profileStorage.addresses.getAll().length, 0);
+
+  // but getAll allows us to access deleted items.
+  let all = profileStorage.addresses.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  do_check_tombstone_record(all[0]);
+});
+
+add_task(async function test_add_tombstone() {
+  do_print("Should be able to add a new tombstone");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.addresses.add({guid: "test-guid-1", deleted: true});
+
+  // should be unable to get it normally.
+  Assert.equal(profileStorage.addresses.get(guid), null);
+  // and getAll should also not return it.
+  Assert.equal(profileStorage.addresses.getAll().length, 0);
+
+  // but getAll allows us to access deleted items.
+  let all = profileStorage.addresses.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  do_check_tombstone_record(all[0]);
+});
+
+add_task(async function test_add_tombstone_without_guid() {
+  do_print("Should not be able to add a new tombstone without specifying the guid");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  Assert.throws(() => { profileStorage.addresses.add({deleted: true}); });
+  Assert.equal(profileStorage.addresses.getAll({includeDeleted: true}).length, 0);
+});
+
+add_task(async function test_add_tombstone_existing_guid() {
+  do_print("Should not be able to add a new tombstone when a record with that ID exists");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.addresses.add(TEST_PROFILE_1);
+  Assert.throws(() => { profileStorage.addresses.add({guid, deleted: true}); });
+
+  // same if the existing item is already a tombstone.
+  profileStorage.addresses.add({guid: "test-guid-1", deleted: true});
+  Assert.throws(() => { profileStorage.addresses.add({guid: "test-guid-1", deleted: true}); });
+});
+
+add_task(async function test_update_tombstone() {
+  do_print("Updating a tombstone should fail");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.addresses.add({guid: "test-guid-1", deleted: true});
+
+  Assert.throws(() => profileStorage.addresses.update(guid, {}), "No matching profile.");
+});
+
+add_task(async function test_update_tombstone() {
+  do_print("Removing a record that's already a tombstone should be a no-op");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.addresses.add({guid: "test-guid-1", deleted: true, timeLastModified: 1234});
+
+  profileStorage.addresses.remove(guid);
+  let all = profileStorage.addresses.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  do_check_tombstone_record(all[0]);
+  equal(all[0].timeLastModified, 1234); // should not be updated to now().
+});
+
+add_task(async function test_remove_notombstone() {
+  do_print("Should be able to remove a record without creating a tombstone.");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.addresses.add({guid: "test-guid-1", deleted: true});
+
+  let all = profileStorage.addresses.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  profileStorage.addresses.remove(guid, {keepTombstone: false});
+  Assert.equal(profileStorage.addresses.getAll({includeDeleted: true}).length, 0);
+});
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/unit/test_creditCardRecords_tombstones.js
@@ -0,0 +1,135 @@
+/**
+ * Tests tombstones in credit card records.
+ */
+
+"use strict";
+
+Services.prefs.setCharPref("extensions.formautofill.loglevel", "Trace");
+
+const {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
+
+const TEST_STORE_FILE_NAME = "test-credit-card.json";
+
+const TEST_CREDIT_CARD_1 = {
+  "cc-name": "John Doe",
+  "cc-number": "1234567812345678",
+  "cc-exp-month": 4,
+  "cc-exp-year": 2017,
+};
+
+let do_check_tombstone_record = (profile) => {
+  Assert.ok(profile.deleted);
+  Assert.deepEqual(Object.keys(profile).sort(),
+               ["guid", "timeLastModified", "deleted"].sort());
+};
+
+add_task(async function test_simple_tombstone() {
+  do_print("check simple tombstone semantics");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.creditCards.add(TEST_CREDIT_CARD_1);
+
+  do_check_eq(profileStorage._store.data.version, 1);
+  do_check_eq(profileStorage._store.data.creditCards.length, 1);
+
+  profileStorage.creditCards.remove(guid);
+
+  // should be unable to get it normally.
+  Assert.equal(profileStorage.creditCards.get(guid), null);
+  // and getAll should also not return it.
+  Assert.equal(profileStorage.creditCards.getAll().length, 0);
+
+  // but getAll allows us to access deleted items.
+  let all = profileStorage.creditCards.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  do_check_tombstone_record(all[0]);
+});
+
+add_task(async function test_add_tombstone() {
+  do_print("Should be able to add a new tombstone");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.creditCards.add({guid: "test-guid-1", deleted: true});
+
+  // should be unable to get it normally.
+  Assert.equal(profileStorage.creditCards.get(guid), null);
+  // and getAll should also not return it.
+  Assert.equal(profileStorage.creditCards.getAll().length, 0);
+
+  // but getAll allows us to access deleted items.
+  let all = profileStorage.creditCards.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  do_check_tombstone_record(all[0]);
+});
+
+add_task(async function test_add_tombstone_without_guid() {
+  do_print("Should not be able to add a new tombstone without specifying the guid");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  Assert.throws(() => { profileStorage.creditCards.add({deleted: true}); });
+  Assert.equal(profileStorage.creditCards.getAll({includeDeleted: true}).length, 0);
+});
+
+add_task(async function test_add_tombstone_existing_guid() {
+  do_print("Should not be able to add a new tombstone when a record with that ID exists");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.creditCards.add(TEST_CREDIT_CARD_1);
+  Assert.throws(() => { profileStorage.creditCards.add({guid, deleted: true}); });
+
+  // same if the existing item is already a tombstone.
+  profileStorage.creditCards.add({guid: "test-guid-1", deleted: true});
+  Assert.throws(() => { profileStorage.creditCards.add({guid: "test-guid-1", deleted: true}); });
+});
+
+add_task(async function test_update_tombstone() {
+  do_print("Updating a tombstone should fail");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.creditCards.add({guid: "test-guid-1", deleted: true});
+
+  Assert.throws(() => profileStorage.creditCards.update(guid, {}), "No matching profile.");
+});
+
+add_task(async function test_update_tombstone() {
+  do_print("Removing a record that's already a tombstone should be a no-op");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.creditCards.add({guid: "test-guid-1", deleted: true, timeLastModified: 1234});
+
+  profileStorage.creditCards.remove(guid);
+  let all = profileStorage.creditCards.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  do_check_tombstone_record(all[0]);
+  equal(all[0].timeLastModified, 1234); // should not be updated to now().
+});
+
+add_task(async function test_remove_notombstone() {
+  do_print("Should be able to remove a record without creating a tombstone.");
+  let path = getTempFile(TEST_STORE_FILE_NAME).path;
+  let profileStorage = new ProfileStorage(path);
+  await profileStorage.initialize();
+
+  let guid = profileStorage.creditCards.add({guid: "test-guid-1", deleted: true});
+
+  let all = profileStorage.creditCards.getAll({includeDeleted: true});
+  Assert.equal(all.length, 1);
+
+  profileStorage.creditCards.remove(guid, {keepTombstone: false});
+  Assert.equal(profileStorage.creditCards.getAll({includeDeleted: true}).length, 0);
+});
--- a/browser/extensions/formautofill/test/unit/xpcshell.ini
+++ b/browser/extensions/formautofill/test/unit/xpcshell.ini
@@ -12,19 +12,21 @@ support-files =
 [heuristics/third_party/test_Macys.js]
 [heuristics/third_party/test_NewEgg.js]
 [heuristics/third_party/test_OfficeDepot.js]
 [heuristics/third_party/test_QVC.js]
 [heuristics/third_party/test_Sears.js]
 [heuristics/third_party/test_Staples.js]
 [heuristics/third_party/test_Walmart.js]
 [test_addressRecords.js]
+[test_addressRecords_tombstones.js]
 [test_autofillFormFields.js]
 [test_collectFormFields.js]
 [test_creditCardRecords.js]
+[test_creditCardRecords_tombstones.js]
 [test_enabledStatus.js]
 [test_findLabelElements.js]
 [test_getFormInputDetails.js]
 [test_isCJKName.js]
 [test_markAsAutofillField.js]
 [test_nameUtils.js]
 [test_onFormSubmitted.js]
 [test_profileAutocompleteResult.js]