Bug 1363997 - Add support for tombstones to profileStorage. r?lchang,MattN
MozReview-Commit-ID: ladknQNOMb
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -188,34 +188,48 @@ 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;
+ recordToSave.version = this.version;
+
+ // Metadata
+ let now = Date.now();
+ recordToSave.timeCreated = now;
+ recordToSave.timeLastModified = now;
+ recordToSave.timeLastUsed = 0;
+ recordToSave.timesUsed = 0;
}
- recordToSave.guid = guid;
- recordToSave.version = this.version;
-
- // 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;
}
@@ -278,18 +292,27 @@ class AutofillRecords {
* Removes the specified record. No error occurs if the record isn't found.
*
* @param {string} guid
* Indicates which record to remove.
*/
remove(guid) {
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);
+ if (index == -1) {
+ this.log.warn("attempting to remove non-existing entry", guid);
+ return;
+ }
+ // replace the record with a tombstone.
+ this._store.data[this._collectionName][index] = {
+ guid,
+ timeLastModified: Date.now(),
+ deleted: true,
+ };
this._store.saveSoon();
Services.obs.notifyObservers(null, "formautofill-storage-changed", "remove");
}
/**
* Returns the record with the specified GUID.
*
@@ -310,29 +333,30 @@ class AutofillRecords {
let clonedRecord = this._clone(recordFound);
this._recordReadProcessor(clonedRecord);
return clonedRecord;
}
/**
* Returns all records.
*
- * @param {Object} config
- * Specifies how data will be retrieved.
- * @param {boolean} config.noComputedFields
+ * @param {boolean} [options.noComputedFields = false]
* Returns raw record without those computed fields.
+ * @param {boolean} [options.includeDeleted = false]
+ * Also return any tombstone records.
* @returns {Array.<Object>}
* An array containing clones of all records.
*/
- getAll(config = {}) {
- this.log.debug("getAll", config);
+ getAll({noComputedFields = false, includeDeleted = false} = {}) {
+ this.log.debug("getAll", noComputedFields, includeDeleted);
+ let records = this._store.data[this._collectionName].filter(r => !r.deleted || includeDeleted);
// Records are cloned to avoid accidental modifications from outside.
- let clonedRecords = this._store.data[this._collectionName].map(this._clone);
- clonedRecords.forEach(record => this._recordReadProcessor(record, config));
+ let clonedRecords = records.map(this._clone);
+ clonedRecords.forEach(record => this._recordReadProcessor(record, {noComputedFields}));
return clonedRecords;
}
/**
* Returns the filtered records based on input's information and searchString.
*
* @returns {Array.<Object>}
* An array containing clones of matched record.
@@ -357,23 +381,25 @@ 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, {includeDeleted = false} = {}) {
+ let found = this._findIndexByGUID(guid, {includeDeleted});
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, {includeDeleted = false} = {}) {
+ return this._store.data[this._collectionName].findIndex(record => {
+ return record.guid == guid && (!record.deleted || includeDeleted);
+ });
}
_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.`);
@@ -381,17 +407,17 @@ class AutofillRecords {
if (typeof record[key] !== "string" &&
typeof record[key] !== "number") {
throw new Error(`"${key}" contains invalid data type.`);
}
}
}
// An interface to be inherited.
- _recordReadProcessor(record, config) {}
+ _recordReadProcessor(record, {noComputedFields = false} = {}) {}
// An interface to be inherited.
_recordWriteProcessor(record) {}
// An interface to be inherited.
mergeIfPossible(guid, record) {}
// An interface to be inherited.
@@ -571,17 +597,17 @@ class Addresses extends AutofillRecords
* @param {Object} targetAddress
* The address for merge.
* @returns {Array.<string>}
* Return an array of the merged GUID string.
*/
mergeToStorage(targetAddress) {
let mergedGUIDs = [];
for (let address of this._store.data[this._collectionName]) {
- if (this.mergeIfPossible(address.guid, targetAddress)) {
+ if (!address.deleted && this.mergeIfPossible(address.guid, targetAddress)) {
mergedGUIDs.push(address.guid);
}
}
this.log.debug("Existing records matching and merging count is", mergedGUIDs.length);
return mergedGUIDs;
}
}
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/unit/test_storage_tombstones.js
@@ -0,0 +1,120 @@
+/**
+ * Tests tombstones in address/creditcard records.
+ */
+
+"use strict";
+
+const {ProfileStorage} = Cu.import("resource://formautofill/ProfileStorage.jsm", {});
+
+const TEST_STORE_FILE_NAME = "test-tombstones.json";
+
+const TEST_ADDRESS_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",
+};
+
+const TEST_CC_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());
+};
+
+// Like add_task, but actually adds 2 - one for addresses and one for cards.
+function add_storage_task(test_function) {
+ add_task(async function() {
+ let path = getTempFile(TEST_STORE_FILE_NAME).path;
+ let profileStorage = new ProfileStorage(path);
+ await profileStorage.initialize();
+
+ for (let [storage, record] of [[profileStorage.addresses, TEST_ADDRESS_1],
+ [profileStorage.creditCards, TEST_CC_1]]) {
+ await test_function(storage, record);
+ }
+ });
+}
+
+add_storage_task(async function test_simple_tombstone(storage, record) {
+ do_print("check simple tombstone semantics");
+
+ let guid = storage.add(record);
+ do_check_eq(storage.getAll().length, 1);
+
+ storage.remove(guid);
+
+ // should be unable to get it normally.
+ Assert.equal(storage.get(guid), null);
+ // and getAll should also not return it.
+ Assert.equal(storage.getAll().length, 0);
+
+ // but getAll allows us to access deleted items.
+ let all = storage.getAll({includeDeleted: true});
+ Assert.equal(all.length, 1);
+
+ do_check_tombstone_record(all[0]);
+});
+
+add_storage_task(async function test_add_tombstone(storage, record) {
+ do_print("Should be able to add a new tombstone");
+ let guid = storage.add({guid: "test-guid-1", deleted: true});
+
+ // should be unable to get it normally.
+ Assert.equal(storage.get(guid), null);
+ // and getAll should also not return it.
+ Assert.equal(storage.getAll().length, 0);
+
+ // but getAll allows us to access deleted items.
+ let all = storage.getAll({includeDeleted: true});
+ Assert.equal(all.length, 1);
+
+ do_check_tombstone_record(all[0]);
+});
+
+add_storage_task(async function test_add_tombstone_without_guid(storage, record) {
+ do_print("Should not be able to add a new tombstone without specifying the guid");
+ Assert.throws(() => { storage.add({deleted: true}); });
+ Assert.equal(storage.getAll({includeDeleted: true}).length, 0);
+});
+
+add_storage_task(async function test_add_tombstone_existing_guid(storage, record) {
+ do_print("Should not be able to add a new tombstone when a record with that ID exists");
+ let guid = storage.add(record);
+ Assert.throws(() => { storage.add({guid, deleted: true}); });
+
+ // same if the existing item is already a tombstone.
+ storage.add({guid: "test-guid-1", deleted: true});
+ Assert.throws(() => { storage.add({guid: "test-guid-1", deleted: true}); });
+});
+
+add_storage_task(async function test_update_tombstone(storage, record) {
+ do_print("Updating a tombstone should fail");
+ let guid = storage.add({guid: "test-guid-1", deleted: true});
+ Assert.throws(() => storage.update(guid, {}), /No matching record./);
+});
+
+add_storage_task(async function test_remove_existing_tombstone(storage, record) {
+ do_print("Removing a record that's already a tombstone should be a no-op");
+ let guid = storage.add({guid: "test-guid-1", deleted: true, timeLastModified: 1234});
+
+ storage.remove(guid);
+ let all = storage.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().
+});
--- a/browser/extensions/formautofill/test/unit/xpcshell.ini
+++ b/browser/extensions/formautofill/test/unit/xpcshell.ini
@@ -30,9 +30,10 @@ support-files =
[test_isCJKName.js]
[test_isFieldEligibleForAutofill.js]
[test_markAsAutofillField.js]
[test_nameUtils.js]
[test_onFormSubmitted.js]
[test_profileAutocompleteResult.js]
[test_savedFieldNames.js]
[test_toOneLineAddress.js]
+[test_storage_tombstones.js]
[test_transformFields.js]