Bug 1368393 - Handle already synced commands in clients collection duplicate checking r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Fri, 09 Jun 2017 12:07:43 -0400
changeset 591823 13f3edec162ff8ec63acce98942c42fa6a0113bf
parent 591190 fc905fca340af6f85a9ba95cd024572d6708f7ad
child 632631 406a5a6680fe91aa1a93e72e42095fa9f295a20a
push id63182
push userbmo:tchiovoloni@mozilla.com
push dateFri, 09 Jun 2017 16:08:01 +0000
reviewersmarkh
bugs1368393
milestone55.0a1
Bug 1368393 - Handle already synced commands in clients collection duplicate checking r?markh MozReview-Commit-ID: Hs0Jh8mMlOp
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
@@ -273,23 +273,29 @@ ClientEngine.prototype = {
 
   removeLocalCommand(command) {
     // the implementation of this engine is such that adding a command to
     // the local client is how commands are deleted! ¯\_(ツ)_/¯
     this._addClientCommand(this.localID, command);
   },
 
   _addClientCommand(clientId, command) {
-    const allCommands = this._readCommands();
-    const clientCommands = allCommands[clientId] || [];
+    const localCommands = this._readCommands();
+    const localClientCommands = localCommands[clientId] || [];
+    const remoteClient = this._store._remoteClients[clientId];
+    let remoteClientCommands = []
+    if (remoteClient && remoteClient.commands) {
+      remoteClientCommands = remoteClient.commands;
+    }
+    const clientCommands = localClientCommands.concat(remoteClientCommands);
     if (hasDupeCommand(clientCommands, command)) {
       return false;
     }
-    allCommands[clientId] = clientCommands.concat(command);
-    this._saveCommands(allCommands);
+    localCommands[clientId] = localClientCommands.concat(command);
+    this._saveCommands(localCommands);
     return true;
   },
 
   _removeClientCommands(clientId) {
     const allCommands = this._readCommands();
     delete allCommands[clientId];
     this._saveCommands(allCommands);
   },
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1464,20 +1464,20 @@ add_task(async function test_command_syn
 
 add_task(async function ensureSameFlowIDs() {
   let events = []
   let origRecordTelemetryEvent = Service.recordTelemetryEvent;
   Service.recordTelemetryEvent = (object, method, value, extra) => {
     events.push({ object, method, value, extra });
   }
 
+  let server = serverForFoo(engine);
   try {
     // Setup 2 clients, send them a command, and ensure we get to events
     // written, both with the same flowID.
-    let server    = serverForFoo(engine);
     await SyncTestingInfrastructure(server);
 
     let remoteId   = Utils.makeGUID();
     let remoteId2  = Utils.makeGUID();
 
     _("Create remote client record 1");
     server.insertWBO("foo", "clients", new ServerWBO(remoteId, encryptPayload({
       id: remoteId,
@@ -1499,47 +1499,118 @@ add_task(async function ensureSameFlowID
     }), Date.now() / 1000));
 
     engine._sync();
     engine.sendCommand("wipeAll", []);
     engine._sync();
     equal(events.length, 2);
     // we don't know what the flowID is, but do know it should be the same.
     equal(events[0].extra.flowID, events[1].extra.flowID);
-
+    // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+    for (let client of Object.values(engine._store._remoteClients)) {
+      client.commands = [];
+    }
     // check it's correctly used when we specify a flow ID
     events.length = 0;
     let flowID = Utils.makeGUID();
     engine.sendCommand("wipeAll", [], null, { flowID });
     engine._sync();
     equal(events.length, 2);
     equal(events[0].extra.flowID, flowID);
     equal(events[1].extra.flowID, flowID);
 
+    // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+    for (let client of Object.values(engine._store._remoteClients)) {
+      client.commands = [];
+    }
+
     // and that it works when something else is in "extra"
     events.length = 0;
     engine.sendCommand("wipeAll", [], null, { reason: "testing" });
     engine._sync();
     equal(events.length, 2);
     equal(events[0].extra.flowID, events[1].extra.flowID);
     equal(events[0].extra.reason, "testing");
     equal(events[1].extra.reason, "testing");
+    // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+    for (let client of Object.values(engine._store._remoteClients)) {
+      client.commands = [];
+    }
 
     // and when both are specified.
     events.length = 0;
     engine.sendCommand("wipeAll", [], null, { reason: "testing", flowID });
     engine._sync();
     equal(events.length, 2);
     equal(events[0].extra.flowID, flowID);
     equal(events[1].extra.flowID, flowID);
     equal(events[0].extra.reason, "testing");
     equal(events[1].extra.reason, "testing");
+    // Wipe remote clients to ensure deduping doesn't prevent us from adding the command.
+    for (let client of Object.values(engine._store._remoteClients)) {
+      client.commands = [];
+    }
 
   } finally {
     Service.recordTelemetryEvent = origRecordTelemetryEvent;
+    cleanup();
+    await promiseStopServer(server);
+  }
+});
+
+add_task(async function test_duplicate_commands_telemetry() {
+  let events = []
+  let origRecordTelemetryEvent = Service.recordTelemetryEvent;
+  Service.recordTelemetryEvent = (object, method, value, extra) => {
+    events.push({ object, method, value, extra });
+  }
+
+  let server = serverForFoo(engine);
+  try {
+    await SyncTestingInfrastructure(server);
+
+    let remoteId   = Utils.makeGUID();
+    let remoteId2  = Utils.makeGUID();
+
+    _("Create remote client record 1");
+    server.insertWBO("foo", "clients", new ServerWBO(remoteId, encryptPayload({
+      id: remoteId,
+      name: "Remote client",
+      type: "desktop",
+      commands: [],
+      version: "48",
+      protocols: ["1.5"]
+    }), Date.now() / 1000));
+
+    _("Create remote client record 2");
+    server.insertWBO("foo", "clients", new ServerWBO(remoteId2, encryptPayload({
+      id: remoteId2,
+      name: "Remote client 2",
+      type: "mobile",
+      commands: [],
+      version: "48",
+      protocols: ["1.5"]
+    }), Date.now() / 1000));
+
+    engine._sync();
+    // Make sure deduping works before syncing
+    engine.sendURIToClientForDisplay("https://example.com", remoteId, "Example");
+    engine.sendURIToClientForDisplay("https://example.com", remoteId, "Example");
+    equal(events.length, 1);
+    engine._sync();
+    // And after syncing.
+    engine.sendURIToClientForDisplay("https://example.com", remoteId, "Example");
+    equal(events.length, 1);
+    // Ensure we aren't deduping commands to different clients
+    engine.sendURIToClientForDisplay("https://example.com", remoteId2, "Example");
+    equal(events.length, 2);
+  } finally {
+    Service.recordTelemetryEvent = origRecordTelemetryEvent;
+    cleanup();
+    await promiseStopServer(server);
   }
 });
 
 function run_test() {
   initTestLogging("Trace");
   Log.repository.getLogger("Sync.Engine.Clients").level = Log.Level.Trace;
   run_next_test();
 }