Bug 956443 - Include version in Android Sync client records. r=nalexander, a=bajaj
authorRichard Newman <rnewman@mozilla.com>
Mon, 06 Jan 2014 15:07:55 -0800
changeset 175687 0c0281672c110afb4a9cb17b17df6abcfa037f45
parent 175686 7c5ec27e38940fdf92fcade74d6f879fedb41f5d
child 175688 1877df1a3dc4530b794948d92ea5fbcc688dd013
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander, bajaj
bugs956443
milestone28.0a2
Bug 956443 - Include version in Android Sync client records. r=nalexander, a=bajaj
mobile/android/base/sync/repositories/domain/ClientRecord.java
mobile/android/base/sync/stage/SyncClientsEngineStage.java
--- a/mobile/android/base/sync/repositories/domain/ClientRecord.java
+++ b/mobile/android/base/sync/repositories/domain/ClientRecord.java
@@ -14,18 +14,32 @@ import org.mozilla.gecko.sync.repositori
 public class ClientRecord extends Record {
   private static final String LOG_TAG = "ClientRecord";
 
   public static final String CLIENT_TYPE         = "mobile";
   public static final String COLLECTION_NAME     = "clients";
   public static final long CLIENTS_TTL = 21 * 24 * 60 * 60; // 21 days in seconds.
   public static final String DEFAULT_CLIENT_NAME = "Default Name";
 
+  /**
+   * Each of these fields is 'owned' by the client it represents. For example,
+   * the "version" field is the Firefox version of that client; some time after
+   * that client upgrades, it'll upload a new record with its new version.
+   *
+   * The only exception is for commands. When a command is sent to a client, the
+   * sender will download its current record, append the command to the
+   * "commands" array, and reupload the record. After processing, the recipient
+   * will reupload its record with an empty commands array.
+   *
+   * Note that the version, then, will remain the version of the recipient, as
+   * with the other descriptive fields.
+   */
   public String name = ClientRecord.DEFAULT_CLIENT_NAME;
   public String type = ClientRecord.CLIENT_TYPE;
+  public String version = null;                      // Free-form string, optional.
   public JSONArray commands;
 
   public ClientRecord(String guid, String collection, long lastModified, boolean deleted) {
     super(guid, collection, lastModified, deleted);
     this.ttl = CLIENTS_TTL;
   }
 
   public ClientRecord(String guid, String collection, long lastModified) {
@@ -43,30 +57,37 @@ public class ClientRecord extends Record
   public ClientRecord() {
     this(Utils.generateGuid(), COLLECTION_NAME, 0, false);
   }
 
   @Override
   protected void initFromPayload(ExtendedJSONObject payload) {
     this.name = (String) payload.get("name");
     this.type = (String) payload.get("type");
+    try {
+      this.version = (String) payload.get("version");
+    } catch (Exception e) {
+      // Oh well.
+    }
 
     try {
       commands = payload.getArray("commands");
     } catch (NonArrayJSONException e) {
       Logger.debug(LOG_TAG, "Got non-array commands in client record " + guid, e);
       commands = null;
     }
   }
 
   @Override
   protected void populatePayload(ExtendedJSONObject payload) {
     putPayload(payload, "id",   this.guid);
     putPayload(payload, "name", this.name);
     putPayload(payload, "type", this.type);
+    putPayload(payload, "version", this.version);
+
     if (this.commands != null) {
       payload.put("commands",  this.commands);
     }
   }
 
   @Override
   public boolean equals(Object o) {
     if (!(o instanceof ClientRecord) || !super.equals(o)) {
@@ -77,16 +98,17 @@ public class ClientRecord extends Record
   }
 
   @Override
   public boolean equalPayloads(Object o) {
     if (!(o instanceof ClientRecord) || !super.equalPayloads(o)) {
       return false;
     }
 
+    // Don't compare versions.
     ClientRecord other = (ClientRecord) o;
     if (!RepoUtils.stringsEqual(other.name, this.name) ||
         !RepoUtils.stringsEqual(other.type, this.type)) {
       return false;
     }
     return true;
   }
 
@@ -94,16 +116,17 @@ public class ClientRecord extends Record
   public Record copyWithIDs(String guid, long androidID) {
     ClientRecord out = new ClientRecord(guid, this.collection, this.lastModified, this.deleted);
     out.androidID = androidID;
     out.sortIndex = this.sortIndex;
     out.ttl       = this.ttl;
 
     out.name = this.name;
     out.type = this.type;
+    out.version = this.version;
     return out;
   }
 
 /*
 Example record:
 
 {id:"relf31w7B4F1",
  name:"marina_mac",
--- a/mobile/android/base/sync/stage/SyncClientsEngineStage.java
+++ b/mobile/android/base/sync/stage/SyncClientsEngineStage.java
@@ -8,16 +8,17 @@ import java.io.UnsupportedEncodingExcept
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import org.json.simple.JSONArray;
 import org.json.simple.JSONObject;
+import org.mozilla.gecko.background.common.GlobalConstants;
 import org.mozilla.gecko.background.common.log.Logger;
 import org.mozilla.gecko.sync.CommandProcessor;
 import org.mozilla.gecko.sync.CommandProcessor.Command;
 import org.mozilla.gecko.sync.CryptoRecord;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 import org.mozilla.gecko.sync.HTTPFailureException;
 import org.mozilla.gecko.sync.NoCollectionKeysSetException;
 import org.mozilla.gecko.sync.Utils;
@@ -51,17 +52,17 @@ public class SyncClientsEngineStage exte
   protected final ClientRecordFactory factory = new ClientRecordFactory();
   protected ClientUploadDelegate clientUploadDelegate;
   protected ClientDownloadDelegate clientDownloadDelegate;
 
   // Be sure to use this safely via getClientsDatabaseAccessor/closeDataAccessor.
   protected ClientsDatabaseAccessor db;
 
   protected volatile boolean shouldWipe;
-  protected volatile boolean commandsProcessedShouldUpload;
+  protected volatile boolean shouldUploadLocalRecord;     // Set if, e.g., we received commands or need to refresh our version.
   protected final AtomicInteger uploadAttemptsCount = new AtomicInteger();
   protected final List<ClientRecord> toUpload = new ArrayList<ClientRecord>();
 
   protected int getClientsCount() {
     return getClientsDatabaseAccessor().clientsCount();
   }
 
   protected synchronized ClientsDatabaseAccessor getClientsDatabaseAccessor() {
@@ -178,21 +179,19 @@ public class SyncClientsEngineStage exte
     }
 
     @Override
     public void handleWBO(CryptoRecord record) {
       ClientRecord r;
       try {
         r = (ClientRecord) factory.createRecord(record.decrypt());
         if (clientsDelegate.isLocalGUID(r.guid)) {
-          Logger.info(LOG_TAG, "Local client GUID exists on server and was downloaded");
-
+          Logger.info(LOG_TAG, "Local client GUID exists on server and was downloaded.");
           localAccountGUIDDownloaded = true;
-          session.config.persistServerClientRecordTimestamp(r.lastModified);
-          processCommands(r.commands);
+          handleDownloadedLocalRecord(r);
         } else {
           // Only need to store record if it isn't our local one.
           wipeAndStore(r);
           addCommands(r);
         }
         RepoUtils.logClient(r);
       } catch (Exception e) {
         session.abort(e, "Exception handling client WBO.");
@@ -263,42 +262,42 @@ public class SyncClientsEngineStage exte
         // TODO: check failed uploads in body.
         clearRecordsToUpload();
         checkAndUpload();
         return;
       }
 
       // If we're processing our record, we have a little more cleanup
       // to do.
-      commandsProcessedShouldUpload = false;
+      shouldUploadLocalRecord = false;
       session.config.persistServerClientRecordTimestamp(responseTimestamp);
       session.advance();
     }
 
     @Override
     public void handleRequestFailure(SyncStorageResponse response) {
       int statusCode = response.getStatusCode();
 
       // If upload failed because of `ifUnmodifiedSince` then there are new
       // commands uploaded to our record. We must download and process them first.
-      if (!commandsProcessedShouldUpload ||
+      if (!shouldUploadLocalRecord ||
           statusCode == HttpStatus.SC_PRECONDITION_FAILED ||
           uploadAttemptsCount.incrementAndGet() > MAX_UPLOAD_FAILURE_COUNT) {
 
         Logger.debug(LOG_TAG, "Client upload failed. Aborting sync.");
         if (!currentlyUploadingLocalRecord) {
           toUpload.clear(); // These will be redownloaded.
         }
         BaseResource.consumeEntity(response); // The exception thrown should need the response body.
         session.abort(new HTTPFailureException(response), "Client upload failed.");
         return;
       }
       Logger.trace(LOG_TAG, "Retrying upload…");
       // Preconditions:
-      // commandsProcessedShouldUpload == true &&
+      // shouldUploadLocalRecord == true &&
       // statusCode != 412 &&
       // uploadAttemptCount < MAX_UPLOAD_FAILURE_COUNT
       checkAndUpload();
     }
 
     @Override
     public void handleRequestError(Exception ex) {
       Logger.info(LOG_TAG, "Client upload error. Aborting sync.");
@@ -352,56 +351,70 @@ public class SyncClientsEngineStage exte
     // Nothing more to do.
     this.resetLocal();
   }
 
   public Integer getStorageVersion() {
     return VersionConstants.CLIENTS_ENGINE_VERSION;
   }
 
+  protected String getLocalClientVersion() {
+    return GlobalConstants.MOZ_APP_VERSION;
+  }
+
   protected ClientRecord newLocalClientRecord(ClientsDataDelegate delegate) {
     final String ourGUID = delegate.getAccountGUID();
     final String ourName = delegate.getClientName();
 
     ClientRecord r = new ClientRecord(ourGUID);
     r.name = ourName;
+    r.version = getLocalClientVersion();
     return r;
   }
 
   // TODO: Bug 726055 - More considered handling of when to sync.
   protected boolean shouldDownload() {
     // Ask info/collections whether a download is needed.
     return true;
   }
 
   protected boolean shouldUpload() {
-    if (commandsProcessedShouldUpload) {
+    if (shouldUploadLocalRecord) {
       return true;
     }
 
     long lastUpload = session.config.getPersistedServerClientRecordTimestamp();   // Defaults to 0.
     if (lastUpload == 0) {
       return true;
     }
 
     // Note the opportunity for clock drift problems here.
     // TODO: if we track download times, we can use the timestamp of most
     // recent download response instead of the current time.
     long now = System.currentTimeMillis();
     long age = now - lastUpload;
     return age >= CLIENTS_TTL_REFRESH;
   }
 
+  protected void handleDownloadedLocalRecord(ClientRecord r) {
+    session.config.persistServerClientRecordTimestamp(r.lastModified);
+
+    if (!getLocalClientVersion().equals(r.version)) {
+      shouldUploadLocalRecord = true;
+    }
+    processCommands(r.commands);
+  }
+
   protected void processCommands(JSONArray commands) {
     if (commands == null ||
         commands.size() == 0) {
       return;
     }
 
-    commandsProcessedShouldUpload = true;
+    shouldUploadLocalRecord = true;
     CommandProcessor processor = CommandProcessor.getProcessor();
 
     for (Object o : commands) {
       processor.processCommand(session, new ExtendedJSONObject((JSONObject) o));
     }
   }
 
   @SuppressWarnings("unchecked")