Bug 1314879 - Add a unique flowID GUID to each command sent via the clients collection. r?rnewman draft
authorMark Hammond <mhammond@skippinet.com.au>
Fri, 04 Nov 2016 12:46:57 +1100
changeset 434702 8ecb302f9fc5c01dd34be0a59f3f172df9f57695
parent 431863 1aa20bcbb80e1014e4d01057f7d52269b0c2d908
child 536086 0db02e9c85a44107f5f23ac548f3869cc885a96c
push id34794
push userbmo:markh@mozilla.com
push dateMon, 07 Nov 2016 07:04:23 +0000
reviewersrnewman
bugs1314879
milestone52.0a1
Bug 1314879 - Add a unique flowID GUID to each command sent via the clients collection. r?rnewman MozReview-Commit-ID: 4eTFDq4Yr7S
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_clients_engine.js
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -485,16 +485,17 @@ ClientEngine.prototype = {
     }
     if (client.stale) {
       throw new Error("Stale remote client ID: '" + clientId + "'.");
     }
 
     let action = {
       command: command,
       args: args,
+      flowID: Utils.makeGUID(), // used for telemetry.
     };
 
     this._log.trace("Client " + clientId + " got a new action: " + [command, args]);
     this._addClientCommand(clientId, action);
     this._tracker.addChangedID(clientId);
   },
 
   /**
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -32,16 +32,28 @@ function check_record_version(user, id) 
 
     _("Payload is " + JSON.stringify(cleartext));
     equal(Services.appinfo.version, cleartext.version);
     equal(2, cleartext.protocols.length);
     equal("1.1", cleartext.protocols[0]);
     equal("1.5", cleartext.protocols[1]);
 }
 
+// compare 2 different command arrays, taking into account that a flowID
+// attribute must exist, be unique in the commands, but isn't specified in
+// "expected" as the value isn't known.
+function compareCommands(actual, expected, description) {
+  let tweakedActual = JSON.parse(JSON.stringify(actual));
+  tweakedActual.map(elt => delete elt.flowID);
+  deepEqual(tweakedActual, expected, description);
+  // each item must have a unique flowID.
+  let allIDs = new Set(actual.map(elt => elt.flowID).filter(fid => !!fid));
+  equal(allIDs.size, actual.length, "all items have unique IDs");
+}
+
 add_test(function test_bad_hmac() {
   _("Ensure that Clients engine deletes corrupt records.");
   let contents = {
     meta: {global: {engines: {clients: {version: engine.version,
                                         syncID: engine.syncID}}}},
     clients: {},
     crypto: {}
   };
@@ -372,16 +384,17 @@ add_test(function test_send_command() {
   let clientCommands = engine._readCommands()[remoteId];
   notEqual(newRecord, undefined);
   equal(clientCommands.length, 1);
 
   let command = clientCommands[0];
   equal(command.command, action);
   equal(command.args.length, 2);
   deepEqual(command.args, args);
+  ok(command.flowID);
 
   notEqual(tracker.changedIDs[remoteId], undefined);
 
   run_next_test();
 });
 
 add_test(function test_command_validation() {
   _("Verifies that command validation works properly.");
@@ -610,22 +623,22 @@ add_test(function test_filter_duplicate_
     equal(counts.newFailed, 0);
 
     _("Broadcast logout to all clients");
     engine.sendCommand("logout", []);
     engine._sync();
 
     let collection = server.getCollection("foo", "clients");
     let recentPayload = JSON.parse(JSON.parse(collection.payload(recentID)).ciphertext);
-    deepEqual(recentPayload.commands, [{ command: "logout", args: [] }],
-              "Should send commands to the recent client");
+    compareCommands(recentPayload.commands, [{ command: "logout", args: [] }],
+                    "Should send commands to the recent client");
 
     let oldPayload = JSON.parse(JSON.parse(collection.payload(oldID)).ciphertext);
-    deepEqual(oldPayload.commands, [{ command: "logout", args: [] }],
-              "Should send commands to the week-old client");
+    compareCommands(oldPayload.commands, [{ command: "logout", args: [] }],
+                    "Should send commands to the week-old client");
 
     let dupePayload = JSON.parse(JSON.parse(collection.payload(dupeID)).ciphertext);
     deepEqual(dupePayload.commands, [],
               "Should not send commands to the dupe client");
 
     _("Update the dupe client's modified time");
     server.insertWBO("foo", "clients", new ServerWBO(dupeID, encryptPayload({
       id: dupeID,
@@ -889,29 +902,31 @@ add_test(function test_merge_commands() 
   let desktopID = Utils.makeGUID();
   server.insertWBO("foo", "clients", new ServerWBO(desktopID, encryptPayload({
     id: desktopID,
     name: "Desktop client",
     type: "desktop",
     commands: [{
       command: "displayURI",
       args: ["https://example.com", engine.localID, "Yak Herders Anonymous"],
+      flowID: Utils.makeGUID(),
     }],
     version: "48",
     protocols: ["1.5"],
   }), now - 10));
 
   let mobileID = Utils.makeGUID();
   server.insertWBO("foo", "clients", new ServerWBO(mobileID, encryptPayload({
     id: mobileID,
     name: "Mobile client",
     type: "mobile",
     commands: [{
       command: "logout",
       args: [],
+      flowID: Utils.makeGUID(),
     }],
     version: "48",
     protocols: ["1.5"],
   }), now - 10));
 
   try {
     let store = engine._store;
 
@@ -920,27 +935,27 @@ add_test(function test_merge_commands() 
     engine._sync();
 
     _("Broadcast logout to all clients");
     engine.sendCommand("logout", []);
     engine._sync();
 
     let collection = server.getCollection("foo", "clients");
     let desktopPayload = JSON.parse(JSON.parse(collection.payload(desktopID)).ciphertext);
-    deepEqual(desktopPayload.commands, [{
+    compareCommands(desktopPayload.commands, [{
       command: "displayURI",
       args: ["https://example.com", engine.localID, "Yak Herders Anonymous"],
     }, {
       command: "logout",
       args: [],
     }], "Should send the logout command to the desktop client");
 
     let mobilePayload = JSON.parse(JSON.parse(collection.payload(mobileID)).ciphertext);
-    deepEqual(mobilePayload.commands, [{ command: "logout", args: [] }],
-      "Should not send a duplicate logout to the mobile client");
+    compareCommands(mobilePayload.commands, [{ command: "logout", args: [] }],
+                    "Should not send a duplicate logout to the mobile client");
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     engine._resetClient();
 
     try {
       server.deleteCollections("foo");
     } finally {
@@ -997,17 +1012,17 @@ add_test(function test_duplicate_remote_
     }), now - 10));
 
     _("Send another tab to the desktop client");
     engine.sendCommand("displayURI", ["https://foobar.com", engine.localID, "Foo bar!"], desktopID);
     engine._sync();
 
     let collection = server.getCollection("foo", "clients");
     let desktopPayload = JSON.parse(JSON.parse(collection.payload(desktopID)).ciphertext);
-    deepEqual(desktopPayload.commands, [{
+    compareCommands(desktopPayload.commands, [{
       command: "displayURI",
       args: ["https://foobar.com", engine.localID, "Foo bar!"],
     }], "Should only send the second command to the desktop client");
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     engine._resetClient();
 
@@ -1037,17 +1052,19 @@ add_test(function test_upload_after_rebo
 
   let deviceBID = Utils.makeGUID();
   let deviceCID = Utils.makeGUID();
   server.insertWBO("foo", "clients", new ServerWBO(deviceBID, encryptPayload({
     id: deviceBID,
     name: "Device B",
     type: "desktop",
     commands: [{
-      command: "displayURI", args: ["https://deviceclink.com", deviceCID, "Device C link"]
+      command: "displayURI",
+      args: ["https://deviceclink.com", deviceCID, "Device C link"],
+      flowID: Utils.makeGUID(),
     }],
     version: "48",
     protocols: ["1.5"],
   }), now - 10));
   server.insertWBO("foo", "clients", new ServerWBO(deviceCID, encryptPayload({
     id: deviceCID,
     name: "Device C",
     type: "desktop",
@@ -1067,17 +1084,17 @@ add_test(function test_upload_after_rebo
     engine.sendCommand("displayURI", ["https://example.com", engine.localID, "Yak Herders Anonymous"], deviceBID);
 
     const oldUploadOutgoing = SyncEngine.prototype._uploadOutgoing;
     SyncEngine.prototype._uploadOutgoing = () => engine._onRecordsWritten.call(engine, [], [deviceBID]);
     engine._sync();
 
     let collection = server.getCollection("foo", "clients");
     let deviceBPayload = JSON.parse(JSON.parse(collection.payload(deviceBID)).ciphertext);
-    deepEqual(deviceBPayload.commands, [{
+    compareCommands(deviceBPayload.commands, [{
       command: "displayURI", args: ["https://deviceclink.com", deviceCID, "Device C link"]
     }], "Should be the same because the upload failed");
 
     _("Simulate the client B consuming the command and syncing to the server");
     server.insertWBO("foo", "clients", new ServerWBO(deviceBID, encryptPayload({
       id: deviceBID,
       name: "Device B",
       type: "desktop",
@@ -1088,17 +1105,17 @@ add_test(function test_upload_after_rebo
 
     // Simulate reboot
     SyncEngine.prototype._uploadOutgoing = oldUploadOutgoing;
     engine = Service.clientsEngine = new ClientEngine(Service);
 
     engine._sync();
 
     deviceBPayload = JSON.parse(JSON.parse(collection.payload(deviceBID)).ciphertext);
-    deepEqual(deviceBPayload.commands, [{
+    compareCommands(deviceBPayload.commands, [{
       command: "displayURI",
       args: ["https://example.com", engine.localID, "Yak Herders Anonymous"],
     }], "Should only had written our outgoing command");
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     engine._resetClient();
 
@@ -1128,20 +1145,24 @@ add_test(function test_keep_cleared_comm
 
   let deviceBID = Utils.makeGUID();
   let deviceCID = Utils.makeGUID();
   server.insertWBO("foo", "clients", new ServerWBO(engine.localID, encryptPayload({
     id: engine.localID,
     name: "Device A",
     type: "desktop",
     commands: [{
-      command: "displayURI", args: ["https://deviceblink.com", deviceBID, "Device B link"]
+      command: "displayURI",
+      args: ["https://deviceblink.com", deviceBID, "Device B link"],
+      flowID: Utils.makeGUID(),
     },
     {
-      command: "displayURI", args: ["https://deviceclink.com", deviceCID, "Device C link"]
+      command: "displayURI",
+      args: ["https://deviceclink.com", deviceCID, "Device C link"],
+      flowID: Utils.makeGUID(),
     }],
     version: "48",
     protocols: ["1.5"],
   }), now - 10));
   server.insertWBO("foo", "clients", new ServerWBO(deviceBID, encryptPayload({
     id: deviceBID,
     name: "Device B",
     type: "desktop",
@@ -1170,36 +1191,42 @@ add_test(function test_keep_cleared_comm
     let commandsProcessed = 0;
     engine._handleDisplayURIs = (uris) => { commandsProcessed = uris.length };
 
     engine._sync();
     engine.processIncomingCommands(); // Not called by the engine.sync(), gotta call it ourselves
     equal(commandsProcessed, 2, "We processed 2 commands");
 
     let localRemoteRecord = JSON.parse(JSON.parse(collection.payload(engine.localID)).ciphertext);
-    deepEqual(localRemoteRecord.commands, [{
+    compareCommands(localRemoteRecord.commands, [{
       command: "displayURI", args: ["https://deviceblink.com", deviceBID, "Device B link"]
     },
     {
       command: "displayURI", args: ["https://deviceclink.com", deviceCID, "Device C link"]
     }], "Should be the same because the upload failed");
 
     // Another client sends another link
     server.insertWBO("foo", "clients", new ServerWBO(engine.localID, encryptPayload({
       id: engine.localID,
       name: "Device A",
       type: "desktop",
       commands: [{
-        command: "displayURI", args: ["https://deviceblink.com", deviceBID, "Device B link"]
+        command: "displayURI",
+        args: ["https://deviceblink.com", deviceBID, "Device B link"],
+        flowID: Utils.makeGUID(),
       },
       {
-        command: "displayURI", args: ["https://deviceclink.com", deviceCID, "Device C link"]
+        command: "displayURI",
+        args: ["https://deviceclink.com", deviceCID, "Device C link"],
+        flowID: Utils.makeGUID(),
       },
       {
-        command: "displayURI", args: ["https://deviceclink2.com", deviceCID, "Device C link 2"]
+        command: "displayURI",
+        args: ["https://deviceclink2.com", deviceCID, "Device C link 2"],
+        flowID: Utils.makeGUID(),
       }],
       version: "48",
       protocols: ["1.5"],
     }), now - 10));
 
     // Simulate reboot
     SyncEngine.prototype._uploadOutgoing = oldUploadOutgoing;
     engine = Service.clientsEngine = new ClientEngine(Service);
@@ -1277,17 +1304,17 @@ add_test(function test_deleted_commands(
     _("Broadcast a command to all clients");
     engine.sendCommand("logout", []);
     engine._sync();
 
     deepEqual(collection.keys().sort(), [activeID, engine.localID].sort(),
       "Should not reupload deleted clients");
 
     let activePayload = JSON.parse(JSON.parse(collection.payload(activeID)).ciphertext);
-    deepEqual(activePayload.commands, [{ command: "logout", args: [] }],
+    compareCommands(activePayload.commands, [{ command: "logout", args: [] }],
       "Should send the command to the active client");
   } finally {
     Svc.Prefs.resetBranch("");
     Service.recordManager.clearCache();
     engine._resetClient();
 
     try {
       server.deleteCollections("foo");
@@ -1321,28 +1348,29 @@ add_test(function test_send_uri_ack() {
     let collection = server.getCollection("foo", "clients");
     let ourPayload = JSON.parse(JSON.parse(collection.payload(engine.localID)).ciphertext);
     ok(ourPayload, "Should upload our client record");
 
     _("Send a URL to the device on the server");
     ourPayload.commands = [{
       command: "displayURI",
       args: ["https://example.com", fakeSenderID, "Yak Herders Anonymous"],
+      flowID: Utils.makeGUID(),
     }];
     server.insertWBO("foo", "clients", new ServerWBO(engine.localID, encryptPayload(ourPayload), now));
 
     _("Sync again");
     engine._sync();
-    deepEqual(engine.localCommands, [{
+    compareCommands(engine.localCommands, [{
       command: "displayURI",
       args: ["https://example.com", fakeSenderID, "Yak Herders Anonymous"],
     }], "Should receive incoming URI");
     ok(engine.processIncomingCommands(), "Should process incoming commands");
     const clearedCommands = engine._readCommands()[engine.localID];
-    deepEqual(clearedCommands, [{
+    compareCommands(clearedCommands, [{
       command: "displayURI",
       args: ["https://example.com", fakeSenderID, "Yak Herders Anonymous"],
     }], "Should mark the commands as cleared after processing");
 
     _("Check that the command was removed on the server");
     engine._sync();
     ourPayload = JSON.parse(JSON.parse(collection.payload(engine.localID)).ciphertext);
     ok(ourPayload, "Should upload the synced client record");