Bug 676404 - Migrate command APIs from Service to Clients engine. r=rnewman

services/sync/modules/engines/bookmarks.js

@@ -1294,7 +1294,7 @@ BookmarksTracker.prototype = {
         this._log.debug("Restore succeeded: wiping server and other clients.");
         Weave.Service.resetClient([this.name]);
         Weave.Service.wipeServer([this.name]);
-        Weave.Service.prepCommand("wipeEngine", [this.name]);
+        Clients.sendCommand("wipeEngine", [this.name]);
         break;
       case "bookmarks-restore-failed":
         this._log.debug("Tracking all items on failed import.");

services/sync/modules/engines/clients.js

@@ -107,41 +107,6 @@ ClientEngine.prototype = {
     return stats;
   },
 
-  // Remove any commands for the local client and mark it for upload
-  clearCommands: function clearCommands() {
-    delete this.localCommands;
-    this._tracker.addChangedID(this.localID);
-  },
-
-  // Send a command+args pair to each remote client
-  sendCommand: function sendCommand(command, args) {
-    // Helper to determine if the client already has this command
-    let notDupe = function(other) other.command != command ||
-      JSON.stringify(other.args) != JSON.stringify(args);
-
-    // Package the command/args pair into an object
-    let action = {
-      command: command,
-      args: args,
-    };
-
-    // Send the command to each remote client
-    for (let [id, client] in Iterator(this._store._remoteClients)) {
-      // Set the action to be a new commands array if none exists
-      if (client.commands == null)
-        client.commands = [action];
-      // Add the new action if there are no duplicates
-      else if (client.commands.every(notDupe))
-        client.commands.push(action);
-      // Must have been a dupe.. skip!
-      else
-        continue;
-
-      this._log.trace("Client " + id + " got a new action: " + [command, args]);
-      this._tracker.addChangedID(id);
-    }
-  },
-
   get localID() {
     // Generate a random GUID id we don't have one
     let localID = Svc.Prefs.get("client.GUID", "");
@@ -221,6 +186,147 @@ ClientEngine.prototype = {
 
     // Neither try again nor error; we're going to delete it.
     return SyncEngine.kRecoveryStrategy.ignore;
+  },
+
+  /**
+   * A hash of valid commands that the client knows about. The key is a command
+   * and the value is a hash containing information about the command such as
+   * number of arguments and description.
+   */
+  _commands: {
+    resetAll:    { args: 0, desc: "Clear temporary local data for all engines" },
+    resetEngine: { args: 1, desc: "Clear temporary local data for engine" },
+    wipeAll:     { args: 0, desc: "Delete all client data for all engines" },
+    wipeEngine:  { args: 1, desc: "Delete all client data for engine" },
+    logout:      { args: 0, desc: "Log out client" }
+  },
+
+  /**
+   * Remove any commands for the local client and mark it for upload.
+   */
+  clearCommands: function clearCommands() {
+    delete this.localCommands;
+    this._tracker.addChangedID(this.localID);
+  },
+
+  /**
+   * Sends a command+args pair to a specific client.
+   *
+   * @param command Command string
+   * @param args Array of arguments/data for command
+   * @param clientId Client to send command to
+   */
+  _sendCommandToClient: function sendCommandToClient(command, args, clientId) {
+    this._log.trace("Sending " + command + " to " + clientId);
+
+    let client = this._store._remoteClients[clientId];
+    if (!client) {
+      throw new Error("Unknown remote client ID: '" + clientId + "'.");
+    }
+
+    // notDupe compares two commands and returns if they are not equal.
+    let notDupe = function(other) {
+      return other.command != command || !Utils.deepEquals(other.args, args);
+    };
+
+    let action = {
+      command: command,
+      args: args,
+    };
+
+    if (!client.commands) {
+      client.commands = [action];
+    }
+    // Add the new action if there are no duplicates.
+    else if (client.commands.every(notDupe)) {
+      client.commands.push(action);
+    }
+    // It must be a dupe. Skip.
+    else {
+      return;
+    }
+
+    this._log.trace("Client " + clientId + " got a new action: " + [command, args]);
+    this._tracker.addChangedID(clientId);
+  },
+
+  /**
+   * Check if the local client has any remote commands and perform them.
+   *
+   * @return false to abort sync
+   */
+  processIncomingCommands: function processIncomingCommands() {
+    this._notify("clients:process-commands", "", function() {
+      // Immediately clear out the commands as we've got them locally.
+      this.clearCommands();
+
+      // Process each command in order.
+      for each ({command: command, args: args} in this.localCommands) {
+        this._log.debug("Processing command: " + command + "(" + args + ")");
+
+        let engines = [args[0]];
+        switch (command) {
+          case "resetAll":
+            engines = null;
+            // Fallthrough
+          case "resetEngine":
+            Weave.Service.resetClient(engines);
+            break;
+          case "wipeAll":
+            engines = null;
+            // Fallthrough
+          case "wipeEngine":
+            Weave.Service.wipeClient(engines);
+            break;
+          case "logout":
+            Weave.Service.logout();
+            return false;
+          default:
+            this._log.debug("Received an unknown command: " + command);
+            break;
+        }
+      }
+
+      return true;
+    })();
+  },
+
+  /**
+   * Validates and sends a command to a client or all clients.
+   *
+   * Calling this does not actually sync the command data to the server. If the
+   * client already has the command/args pair, it won't receive a duplicate
+   * command.
+   *
+   * @param command
+   *        Command to invoke on remote clients
+   * @param args
+   *        Array of arguments to give to the command
+   * @param clientId
+   *        Client ID to send command to. If undefined, send to all remote
+   *        clients.
+   */
+  sendCommand: function sendCommand(command, args, clientId) {
+    let commandData = this._commands[command];
+    // Don't send commands that we don't know about.
+    if (!commandData) {
+      this._log.error("Unknown command to send: " + command);
+      return;
+    }
+    // Don't send a command with the wrong number of arguments.
+    else if (!args || args.length != commandData.args) {
+      this._log.error("Expected " + commandData.args + " args for '" +
+                      command + "', but got " + args);
+      return;
+    }
+
+    if (clientId) {
+      this._sendCommandToClient(command, args, clientId);
+    } else {
+      for (let id in this._store._remoteClients) {
+        this._sendCommandToClient(command, args, id);
+      }
+    }
   }
 };
 

services/sync/modules/service.js

@@ -1461,10 +1461,9 @@ WeaveSvc.prototype = {
         break;
     }
 
-    // Process the incoming commands if we have any
     if (Clients.localCommands) {
       try {
-        if (!(this.processCommands())) {
+        if (!(Clients.processIncomingCommands())) {
           Status.sync = ABORT_SYNC_COMMAND;
           throw "aborting sync, process commands said so";
         }
@@ -1817,20 +1816,22 @@ WeaveSvc.prototype = {
    */
   wipeRemote: function WeaveSvc_wipeRemote(engines)
     this._catch(this._notify("wipe-remote", "", function() {
-      // Make sure stuff gets uploaded
+      // Make sure stuff gets uploaded.
       this.resetClient(engines);
 
-      // Clear out any server data
+      // Clear out any server data.
       this.wipeServer(engines);
 
-      // Only wipe the engines provided
+      // Only wipe the engines provided.
-      if (engines)
+      if (engines) {
-        engines.forEach(function(e) this.prepCommand("wipeEngine", [e]), this);
+        engines.forEach(function(e) Clients.sendCommand("wipeEngine", [e]), this);
-      // Tell the remote machines to wipe themselves
+      }
-      else
+      // Tell the remote machines to wipe themselves.
-        this.prepCommand("wipeAll", []);
+      else {
+        Clients.sendCommand("wipeAll", []);
+      }
 
-      // Make sure the changed clients get updated
+      // Make sure the changed clients get updated.
       Clients.sync();
     }))(),
 
@@ -1871,104 +1872,16 @@ WeaveSvc.prototype = {
         engine.resetClient();
     }))(),
 
-  /**
-   * A hash of valid commands that the client knows about. The key is a command
-   * and the value is a hash containing information about the command such as
-   * number of arguments and description.
-   */
-  _commands: [
-    ["resetAll", 0, "Clear temporary local data for all engines"],
-    ["resetEngine", 1, "Clear temporary local data for engine"],
-    ["wipeAll", 0, "Delete all client data for all engines"],
-    ["wipeEngine", 1, "Delete all client data for engine"],
-    ["logout", 0, "Log out client"],
-  ].reduce(function WeaveSvc__commands(commands, entry) {
-    commands[entry[0]] = {};
-    for (let [i, attr] in Iterator(["args", "desc"]))
-      commands[entry[0]][attr] = entry[i + 1];
-    return commands;
-  }, {}),
-
-  /**
-   * Check if the local client has any remote commands and perform them.
-   *
-   * @return False to abort sync
-   */
-  processCommands: function WeaveSvc_processCommands()
-    this._notify("process-commands", "", function() {
-      // Immediately clear out the commands as we've got them locally
-      let commands = Clients.localCommands;
-      Clients.clearCommands();
-
-      // Process each command in order
-      for each ({command: command, args: args} in commands) {
-        this._log.debug("Processing command: " + command + "(" + args + ")");
-
-        let engines = [args[0]];
-        switch (command) {
-          case "resetAll":
-            engines = null;
-            // Fallthrough
-          case "resetEngine":
-            this.resetClient(engines);
-            break;
-          case "wipeAll":
-            engines = null;
-            // Fallthrough
-          case "wipeEngine":
-            this.wipeClient(engines);
-            break;
-          case "logout":
-            this.logout();
-            return false;
-          default:
-            this._log.debug("Received an unknown command: " + command);
-            break;
-        }
-      }
-
-      return true;
-    })(),
-
-  /**
-   * Prepare to send a command to each remote client. Calling this doesn't
-   * actually sync the command data to the server. If the client already has
-   * the command/args pair, it won't get a duplicate action.
-   *
-   * @param command
-   *        Command to invoke on remote clients
-   * @param args
-   *        Array of arguments to give to the command
-   */
-  prepCommand: function WeaveSvc_prepCommand(command, args) {
-    let commandData = this._commands[command];
-    // Don't send commands that we don't know about
-    if (commandData == null) {
-      this._log.error("Unknown command to send: " + command);
-      return;
-    }
-    // Don't send a command with the wrong number of arguments
-    else if (args == null || args.length != commandData.args) {
-      this._log.error("Expected " + commandData.args + " args for '" +
-                      command + "', but got " + args);
-      return;
-    }
-
-    // Send the command to all remote clients
-    this._log.debug("Sending clients: " + [command, args, commandData.desc]);
-    Clients.sendCommand(command, args);
-  },
-
   /**
    * Fetch storage info from the server.
-   * 
+   *
    * @param type
    *        String specifying what info to fetch from the server. Must be one
    *        of the INFO_* values. See Sync Storage Server API spec for details.
    * @param callback
    *        Callback function with signature (error, data) where `data' is
    *        the return value from the server already parsed as JSON.
-   * 
+   *
    * @return RESTRequest instance representing the request, allowing callers
    *         to cancel the request.
    */

services/sync/tests/unit/test_clients_engine.js

@@ -239,6 +239,198 @@ add_test(function test_client_name_change() {
   run_next_test();
 });
 
+add_test(function test_send_command() {
+  _("Verifies _sendCommandToClient puts commands in the outbound queue.");
+
+  let store = Clients._store;
+  let tracker = Clients._tracker;
+  let remoteId = Utils.makeGUID();
+  let rec = new ClientsRec("clients", remoteId);
+
+  store.create(rec);
+  let remoteRecord = store.createRecord(remoteId, "clients");
+
+  let action = "testCommand";
+  let args = ["foo", "bar"];
+
+  Clients._sendCommandToClient(action, args, remoteId);
+
+  let newRecord = store._remoteClients[remoteId];
+  do_check_neq(newRecord, undefined);
+  do_check_eq(newRecord.commands.length, 1);
+
+  let command = newRecord.commands[0];
+  do_check_eq(command.command, action);
+  do_check_eq(command.args.length, 2);
+  do_check_eq(command.args, args);
+
+  do_check_neq(tracker.changedIDs[remoteId], undefined);
+
+  run_next_test();
+});
+
+add_test(function test_command_validation() {
+  _("Verifies that command validation works properly.");
+
+  let store = Clients._store;
+
+  let testCommands = [
+    ["resetAll",    [],       true ],
+    ["resetAll",    ["foo"],  false],
+    ["resetEngine", ["tabs"], true ],
+    ["resetEngine", [],       false],
+    ["wipeAll",     [],       true ],
+    ["wipeAll",     ["foo"],  false],
+    ["wipeEngine",  ["tabs"], true ],
+    ["wipeEngine",  [],       false],
+    ["logout",      [],       true ],
+    ["logout",      ["foo"],  false],
+    ["__UNKNOWN__", [],       false]
+  ];
+
+  for each (let [action, args, expectedResult] in testCommands) {
+    let remoteId = Utils.makeGUID();
+    let rec = new ClientsRec("clients", remoteId);
+
+    store.create(rec);
+    store.createRecord(remoteId, "clients");
+
+    Clients.sendCommand(action, args, remoteId);
+
+    let newRecord = store._remoteClients[remoteId];
+    do_check_neq(newRecord, undefined);
+
+    if (expectedResult) {
+      _("Ensuring command is sent: " + action);
+      do_check_eq(newRecord.commands.length, 1);
+
+      let command = newRecord.commands[0];
+      do_check_eq(command.command, action);
+      do_check_eq(command.args, args);
+
+      do_check_neq(Clients._tracker, undefined);
+      do_check_neq(Clients._tracker.changedIDs[remoteId], undefined);
+    } else {
+      _("Ensuring command is scrubbed: " + action);
+      do_check_eq(newRecord.commands, undefined);
+
+      if (store._tracker) {
+        do_check_eq(Clients._tracker[remoteId], undefined);
+      }
+    }
+
+  }
+  run_next_test();
+});
+
+add_test(function test_command_duplication() {
+  _("Ensures duplicate commands are detected and not added");
+
+  let store = Clients._store;
+  let remoteId = Utils.makeGUID();
+  let rec = new ClientsRec("clients", remoteId);
+  store.create(rec);
+  store.createRecord(remoteId, "clients");
+
+  let action = "resetAll";
+  let args = [];
+
+  Clients.sendCommand(action, args, remoteId);
+  Clients.sendCommand(action, args, remoteId);
+
+  let newRecord = store._remoteClients[remoteId];
+  do_check_eq(newRecord.commands.length, 1);
+
+  _("Check variant args length");
+  newRecord.commands = [];
+
+  action = "resetEngine";
+  Clients.sendCommand(action, [{ x: "foo" }], remoteId);
+  Clients.sendCommand(action, [{ x: "bar" }], remoteId);
+
+  _("Make sure we spot a real dupe argument.");
+  Clients.sendCommand(action, [{ x: "bar" }], remoteId);
+
+  do_check_eq(newRecord.commands.length, 2);
+
+  run_next_test();
+});
+
+add_test(function test_command_invalid_client() {
+  _("Ensures invalid client IDs are caught");
+
+  let id = Utils.makeGUID();
+  let error;
+
+  try {
+    Clients.sendCommand("wipeAll", [], id);
+  } catch (ex) {
+    error = ex;
+  }
+
+  do_check_eq(error.message.indexOf("Unknown remote client ID: "), 0);
+
+  run_next_test();
+});
+
+add_test(function test_command_sync() {
+  _("Ensure that commands are synced across clients.");
+  Svc.Prefs.set("clusterURL", "http://localhost:8080/");
+  Svc.Prefs.set("username", "foo");
+
+  generateNewKeys();
+
+  let global = new ServerWBO('global',
+                             {engines: {clients: {version: Clients.version,
+                                                  syncID: Clients.syncID}}});
+  let coll = new ServerCollection();
+  let clientwbo = coll.wbos[Clients.localID] = new ServerWBO(Clients.localID);
+  let server = httpd_setup({
+      "/1.1/foo/storage/meta/global": global.handler(),
+      "/1.1/foo/storage/clients": coll.handler()
+  });
+  let remoteId = Utils.makeGUID();
+  let remotewbo = coll.wbos[remoteId] = new ServerWBO(remoteId);
+  server.registerPathHandler(
+    "/1.1/foo/storage/clients/" + Clients.localID, clientwbo.handler());
+  server.registerPathHandler(
+    "/1.1/foo/storage/clients/" + remoteId, remotewbo.handler());
+
+  _("Create remote client record");
+  let rec = new ClientsRec("clients", remoteId);
+  Clients._store.create(rec);
+  let remoteRecord = Clients._store.createRecord(remoteId, "clients");
+  Clients.sendCommand("wipeAll", []);
+
+  let clientRecord = Clients._store._remoteClients[remoteId];
+  do_check_neq(clientRecord, undefined);
+  do_check_eq(clientRecord.commands.length, 1);
+
+  try {
+    Clients.sync();
+    do_check_neq(clientwbo.payload, undefined);
+    do_check_true(Clients.lastRecordUpload > 0);
+
+    do_check_neq(remotewbo.payload, undefined);
+
+    Svc.Prefs.set("client.GUID", remoteId);
+    Clients._resetClient();
+    do_check_eq(Clients.localID, remoteId);
+    Clients.sync();
+    do_check_neq(Clients.localCommands, undefined);
+    do_check_eq(Clients.localCommands.length, 1);
+
+    let command = Clients.localCommands[0];
+    do_check_eq(command.command, "wipeAll");
+    do_check_eq(command.args.length, 0);
+
+  } finally {
+    Svc.Prefs.resetBranch("");
+    Records.clearCache();
+    server.stop(run_next_test);
+  }
+});
+
 function run_test() {
   initTestLogging("Trace");
   Log4Moz.repository.getLogger("Sync.Engine.Clients").level = Log4Moz.Level.Trace;