Bug 1273234 - Add method for summarizing sync bookmark problems generically. r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Fri, 20 May 2016 13:19:54 -0400
changeset 369810 9ff2062c97003b729a5bb1d7a43c3f11d432f709
parent 369809 b0765d563e785ffc85c2d3ed84f8642ee2be7c56
child 521620 24c7a3745461e02724a9da76da93acac98efc819
push id18917
push userbmo:tchiovoloni@mozilla.com
push dateMon, 23 May 2016 18:11:17 +0000
reviewersmarkh
bugs1273234
milestone49.0a1
Bug 1273234 - Add method for summarizing sync bookmark problems generically. r?markh MozReview-Commit-ID: 9u8J3zh4J10
services/sync/modules/bookmark_validator.js
services/sync/tests/unit/test_bookmark_validator.js
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -5,18 +5,115 @@
 "use strict";
 
 const Cu = Components.utils;
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/bookmark_utils.js");
 
-this.EXPORTED_SYMBOLS = ["BookmarkValidator"];
+this.EXPORTED_SYMBOLS = ["BookmarkValidator", "BookmarkProblemData"];
+
+/**
+ * Result of bookmark validation. Contains the following fields which describe
+ * server-side problems unless otherwise specified.
+ *
+ * - missingIDs (number): # of objects with missing ids
+ * - duplicates (array of ids): ids seen more than once
+ * - parentChildMismatches (array of {parent: parentid, child: childid}):
+ *   instances where the child's parentid and the parent's children array
+ *   do not match
+ * - cycles (array of array of ids). List of cycles found in the "tree".
+ * - orphans (array of {id: string, parent: string}): List of nodes with
+ *   either no parentid, or where the parent could not be found.
+ * - missingChildren (array of {parent: id, child: id}):
+ *   List of parent/children where the child id couldn't be found
+ * - multipleParents (array of {child: id, parents: array of ids}):
+ *   List of children that were part of multiple parent arrays
+ * - deletedParents (array of ids) : List of records that aren't deleted but
+ *   had deleted parents
+ * - childrenOnNonFolder (array of ids): list of non-folders that still have
+ *   children arrays
+ * - duplicateChildren (array of ids): list of records who have the same
+ *   child listed multiple times in their children array
+ * - parentNotFolder (array of ids): list of records that have parents that
+ *   aren't folders
+ * - wrongParentName (array of ids): list of records whose parentName does
+ *   not match the parent's actual title
+ * - rootOnServer (boolean): true if the root came from the server
+ *
+ * - clientMissing: Array of ids on the server missing from the client
+ * - serverMissing: Array of ids on the client missing from the server
+ * - serverDeleted: Array of ids on the client that the server had marked as deleted.
+ * - serverUnexpected: Array of ids that appear on the server but shouldn't
+ *   because the client attempts to never upload them.
+ * - differences: Array of {id: string, differences: string array} recording
+ *   the non-structural properties that are differente between the client and server
+ * - structuralDifferences: As above, but contains the items where the differences were
+ *   structural, that is, they contained childGUIDs or parentid
+ */
+class BookmarkProblemData {
+  constructor() {
+    this.rootOnServer = false;
+    this.missingIDs = 0;
 
+    this.duplicates = [];
+    this.parentChildMismatches = [];
+    this.cycles = [];
+    this.orphans = [];
+    this.missingChildren = [];
+    this.multipleParents = [];
+    this.deletedParents = [];
+    this.childrenOnNonFolder = [];
+    this.duplicateChildren = [];
+    this.parentNotFolder = [];
+    this.wrongParentName = [];
+
+    this.clientMissing = [];
+    this.serverMissing = [];
+    this.serverDeleted = [];
+    this.serverUnexpected = [];
+    this.differences = [];
+    this.structuralDifferences = [];
+  }
+
+  /**
+   * Produce a list summarizing problems found. Each entry contains {name, count},
+   * where name is the field name for the problem, and count is the number of times
+   * the problem was encountered.
+   *
+   * Validation has failed if all counts are not 0.
+   */
+  getSummary() {
+    return [
+      { name: "clientMissing", count: this.clientMissing.length },
+      { name: "serverMissing", count: this.serverMissing.length },
+      { name: "serverDeleted", count: this.serverDeleted.length },
+      { name: "serverUnexpected", count: this.serverUnexpected.length },
+
+      { name: "structuralDifferences", count: this.structuralDifferences.length },
+      { name: "differences", count: this.differences.length },
+
+      { name: "missingIDs", count: this.missingIDs },
+      { name: "rootOnServer", count: this.rootOnServer ? 1 : 0 },
+
+      { name: "duplicates", count: this.duplicates.length },
+      { name: "parentChildMismatches", count: this.parentChildMismatches.length },
+      { name: "cycles", count: this.cycles.length },
+      { name: "orphans", count: this.orphans.length },
+      { name: "missingChildren", count: this.missingChildren.length },
+      { name: "multipleParents", count: this.multipleParents.length },
+      { name: "deletedParents", count: this.deletedParents.length },
+      { name: "childrenOnNonFolder", count: this.childrenOnNonFolder.length },
+      { name: "duplicateChildren", count: this.duplicateChildren.length },
+      { name: "parentNotFolder", count: this.parentNotFolder.length },
+      { name: "wrongParentName", count: this.wrongParentName.length },
+    ];
+  }
+}
 
 class BookmarkValidator {
 
   createClientRecordsFromTree(clientTree) {
     // Iterate over the treeNode, converting it to something more similar to what
     // the server stores.
     let records = [];
     function traverse(treeNode) {
@@ -101,64 +198,29 @@ class BookmarkValidator {
    *       contain any records that could not be found.
    *     - parent (record): The parent to this record.
    *     - Unchanged properties send down from the server: id, title, type,
    *       parentName, parentid, bmkURI, keyword, tags, pos, queryId, loadInSidebar
    * - root: Root of the server-side bookmark tree. Has the same properties as
    *   above.
    * - deletedRecords: As above, but only contains items that the server sent
    *   where it also sent indication that the item should be deleted.
-   * - problemData: Object containing info about problems recorded.
-   *     - missingIDs (number): # of objects with missing ids
-   *     - duplicates (array of ids): ids seen more than once
-   *     - parentChildMismatches (array of {parent: parentid, child: childid}):
-   *       instances where the child's parentid and the parent's children array
-   *       do not match
-   *     - cycles (array of array of ids). List of cycles found in the "tree".
-   *     - orphans (array of {id: string, parent: string}): List of nodes with
-   *       either no parentid, or where the parent could not be found.
-   *     - missingChildren (array of {parent: id, child: id}):
-   *       List of parent/children where the child id couldn't be found
-   *     - multipleParents (array of {child: id, parents: array of ids}):
-   *       List of children that were part of multiple parent arrays
-   *     - deletedParents (array of ids) : List of records that aren't deleted but
-   *       had deleted parents
-   *     - childrenOnNonFolder (array of ids): list of non-folders that still have
-   *       children arrays
-   *     - duplicateChildren (array of ids): list of records who have the same
-   *       child listed multiple times in their children array
-   *     - parentNotFolder (array of ids): list of records that have parents that
-   *       aren't folders
-   *     - wrongParentName (array of ids): list of records whose parentName does
-   *       not match the parent's actual title
-   *     - rootOnServer (boolean): true if the root came from the server
+   * - problemData: a BookmarkProblemData object, with the caveat that
+   *   the fields describing client/server relationship will not have been filled
+   *   out yet.
    */
   inspectServerRecords(serverRecords) {
     let deletedItemIds = new Set();
     let idToRecord = new Map();
     let deletedRecords = [];
 
     let folders = [];
     let problems = [];
 
-    let problemData = {
-      missingIDs: 0,
-      duplicates: [],
-      parentChildMismatches: [],
-      cycles: [],
-      orphans: [],
-      missingChildren: [],
-      multipleParents: [],
-      deletedParents: [],
-      childrenOnNonFolder: [],
-      duplicateChildren: [],
-      parentNotFolder: [],
-      wrongParentName: [],
-      rootOnServer: false
-    };
+    let problemData = new BookmarkProblemData();
 
     let resultRecords = [];
 
     for (let record of serverRecords) {
       if (!record.id) {
         ++problemData.missingIDs;
         continue;
       }
@@ -362,42 +424,29 @@ class BookmarkValidator {
 
   /**
    * Compare the list of server records with the client tree.
    *
    * Returns the same data as described in the inspectServerRecords comment,
    * with the following additional fields.
    * - clientRecords: an array of client records in a similar format to
    *   the .records (ie, server records) entry.
-   * - problemData is the same as for inspectServerRecords, but with the
-   *   following additional entries.
-   *   - clientMissing: Array of ids on the server missing from the client
-   *   - serverMissing: Array of ids on the client missing from the server
-   *   - serverUnexpected: Array of ids that appear on the server but shouldn't
-   *     because the client attempts to never upload them.
-   *   - differences: Array of {id: string, differences: string array} recording
-   *     the properties that are differente between the client and server
+   * - problemData is the same as for inspectServerRecords, except all properties
+   *   will be filled out.
    */
   compareServerWithClient(serverRecords, clientTree) {
 
     let clientRecords = this.createClientRecordsFromTree(clientTree);
     let inspectionInfo = this.inspectServerRecords(serverRecords);
     inspectionInfo.clientRecords = clientRecords;
 
     // Mainly do this to remove deleted items and normalize child guids.
     serverRecords = inspectionInfo.records;
     let problemData = inspectionInfo.problemData;
 
-    problemData.clientMissing = [];
-    problemData.serverMissing = [];
-    problemData.serverDeleted = [];
-    problemData.serverUnexpected = [];
-    problemData.differences = [];
-    problemData.good = [];
-
     let matches = [];
 
     let allRecords = new Map();
     let serverDeletedLookup = new Set(inspectionInfo.deletedRecords.map(r => r.id));
 
     for (let sr of serverRecords) {
       allRecords.set(sr.id, {client: null, server: sr});
     }
@@ -424,24 +473,25 @@ class BookmarkValidator {
           problemData.serverMissing.push(id);
         }
         continue;
       }
       if (server && client && client.ignored) {
         problemData.serverUnexpected.push(id);
       }
       let differences = [];
+      let structuralDifferences = [];
       // We want to treat undefined, null and an empty string as identical
       if ((client.title || "") !== (server.title || "")) {
         differences.push('title');
       }
 
       if (client.parentid || server.parentid) {
         if (client.parentid !== server.parentid) {
-          differences.push('parentid');
+          structuralDifferences.push('parentid');
         }
         // Need to special case 'unfiled' due to it's recent name change
         // ("Other Bookmarks" vs "Unsorted Bookmarks"), otherwise this has a lot
         // of false positives.
         if (client.parentName !== server.parentName && server.parentid !== 'unfiled') {
           differences.push('parentName');
         }
       }
@@ -490,25 +540,28 @@ class BookmarkValidator {
               // It's the fabricated places root. It won't have the GUIDs, but
               // it doesn't matter.
               break;
             }
             if (client.childGUIDs || server.childGUIDs) {
               let cl = client.childGUIDs || [];
               let sl = server.childGUIDs || [];
               if (cl.length !== sl.length || !cl.every((id, i) => sl[i] === id)) {
-                differences.push('childGUIDs');
+                structuralDifferences.push('childGUIDs');
               }
             }
             break;
         }
       }
 
       if (differences.length) {
         problemData.differences.push({id, differences});
       }
+      if (structuralDifferences.length) {
+        problemData.structuralDifferences.push({ id, differences: structuralDifferences });
+      }
     }
     return inspectionInfo;
   }
 };
 
 
 
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -196,28 +196,28 @@ add_test(function test_cswc_serverMissin
   let {server, client} = getDummyServerAndClient();
   // remove c
   server.pop();
   server[0].children.pop();
 
   let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
   deepEqual(c.serverMissing, ['cccccccccccc']);
   equal(c.clientMissing.length, 0);
-  deepEqual(c.differences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+  deepEqual(c.structuralDifferences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
   run_next_test();
 });
 
 add_test(function test_cswc_clientMissing() {
   let {server, client} = getDummyServerAndClient();
   client.children[0].children.pop();
 
   let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
   deepEqual(c.clientMissing, ['cccccccccccc']);
   equal(c.serverMissing.length, 0);
-  deepEqual(c.differences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
+  deepEqual(c.structuralDifferences, [{id: 'aaaaaaaaaaaa', differences: ['childGUIDs']}]);
   run_next_test();
 });
 
 add_test(function test_cswc_differences() {
   {
     let {server, client} = getDummyServerAndClient();
     client.children[0].children[0].title = 'asdf';
     let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;