Bug 729248 - Smarter upload of our clients record. r=rnewman

mobile/android/base/sync/SyncConfiguration.java

@@ -190,6 +190,8 @@ public class SyncConfiguration implements CredentialsSource {
   public String          prefsPath;
   public PrefsSource     prefsSource;
 
+  public static final String CLIENT_RECORD_TIMESTAMP = "serverClientRecordTimestamp";
+
   /**
    * Create a new SyncConfiguration instance. Pass in a PrefsSource to
    * provide access to preferences.
@@ -369,4 +371,12 @@ public class SyncConfiguration implements CredentialsSource {
   public Editor getEditor() {
     return this.getPrefs().edit();
   }
+
+  public void persistServerClientRecordTimestamp(long timestamp) {
+    getEditor().putLong(SyncConfiguration.CLIENT_RECORD_TIMESTAMP, timestamp).commit();
+  }
+
+  public long getPersistedServerClientRecordTimestamp() {
+    return getPrefs().getLong(SyncConfiguration.CLIENT_RECORD_TIMESTAMP, 0);
+  }
 }

mobile/android/base/sync/delegates/ClientsDataDelegate.java

@@ -9,4 +9,5 @@ public interface ClientsDataDelegate {
   public String getClientName();
   public void setClientsCount(int clientsCount);
   public int getClientsCount();
+  public boolean isLocalGUID(String guid);
 }

mobile/android/base/sync/repositories/android/ClientsDatabaseAccessor.java

@@ -11,13 +11,13 @@ import java.util.Map;
 
 import org.mozilla.gecko.sync.repositories.NullCursorException;
 import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
-import org.mozilla.gecko.sync.setup.Constants;
 
 import android.content.Context;
 import android.database.Cursor;
 
 public class ClientsDatabaseAccessor {
 
+  public static final String PROFILE_ID = "default";     // Generic profile id for now, until multiple profiles are implemented.
   public static final String LOG_TAG = "ClientsDatabaseAccessor";
 
   private ClientsDatabase db;
@@ -102,7 +102,7 @@ public class ClientsDatabaseAccessor {
   }
 
   private String getProfileId() {
-    return Constants.PROFILE_ID;
+    return ClientsDatabaseAccessor.PROFILE_ID;
   }
 
   public void wipe() {

mobile/android/base/sync/repositories/domain/ClientRecord.java

@@ -4,14 +4,23 @@
 
 package org.mozilla.gecko.sync.repositories.domain;
 
+import org.json.simple.JSONArray;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
+import org.mozilla.gecko.sync.Logger;
+import org.mozilla.gecko.sync.NonArrayJSONException;
 import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.repositories.android.RepoUtils;
-import org.mozilla.gecko.sync.setup.Constants;
 
 public class ClientRecord extends Record {
+  private static final String LOG_TAG = "ClientRecord";
 
-  public static final String COLLECTION_NAME = "clients";
+  public static final String CLIENT_TYPE         = "mobile";
+  public static final String COLLECTION_NAME     = "clients";
+  public static final String DEFAULT_CLIENT_NAME = "Default Name";
+
+  public String name = ClientRecord.DEFAULT_CLIENT_NAME;
+  public String type = ClientRecord.CLIENT_TYPE;
+  public JSONArray commands;
 
   public ClientRecord(String guid, String collection, long lastModified, boolean deleted) {
     super(guid, collection, lastModified, deleted);
@@ -33,13 +42,17 @@ public class ClientRecord extends Record {
     this(Utils.generateGuid(), COLLECTION_NAME, 0, false);
   }
 
-  public String name = Constants.DEFAULT_CLIENT_NAME;
-  public String type = Constants.CLIENT_TYPE;
-
   @Override
   protected void initFromPayload(ExtendedJSONObject payload) {
     this.name = (String) payload.get("name");
     this.type = (String) payload.get("type");
+
+    try {
+      commands = payload.getArray("commands");
+    } catch (NonArrayJSONException e) {
+      Logger.debug(LOG_TAG, "Got non-array commands in client record " + guid, e);
+      commands = null;
+    }
   }
 
   @Override

mobile/android/base/sync/repositories/domain/ClientRecordFactory.java

@@ -8,7 +8,6 @@ import org.mozilla.gecko.sync.CryptoRecord;
 import org.mozilla.gecko.sync.repositories.RecordFactory;
 
 public class ClientRecordFactory extends RecordFactory {
-
   @Override
   public Record createRecord(Record record) {
     ClientRecord r = new ClientRecord();

mobile/android/base/sync/setup/Constants.java

@@ -17,11 +17,6 @@ public class Constants {
   public static final String CLIENT_NAME          = "account.clientName";
   public static final String NUM_CLIENTS          = "account.numClients";
 
-  // Constants for client records.
-  public static final String PROFILE_ID          = "default";     // Generic profile id for now, until multiple profiles are implemented.
-  public static final String CLIENT_TYPE         = "mobile";
-  public static final String DEFAULT_CLIENT_NAME = "Default Name";
-
   // Constants for Activities.
   public static final String INTENT_EXTRA_IS_SETUP = "isSetup";
   public static final String INTENT_EXTRA_IS_PAIR  = "isPair";

mobile/android/base/sync/stage/SyncClientsEngineStage.java

@@ -7,12 +7,15 @@ package org.mozilla.gecko.sync.stage;
 import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.concurrent.atomic.AtomicInteger;
 
+import org.json.simple.JSONArray;
 import org.mozilla.gecko.sync.CryptoRecord;
 import org.mozilla.gecko.sync.GlobalSession;
 import org.mozilla.gecko.sync.HTTPFailureException;
 import org.mozilla.gecko.sync.Logger;
 import org.mozilla.gecko.sync.NoCollectionKeysSetException;
+import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.crypto.CryptoException;
 import org.mozilla.gecko.sync.crypto.KeyBundle;
 import org.mozilla.gecko.sync.delegates.ClientsDataDelegate;
@@ -27,9 +30,13 @@ import org.mozilla.gecko.sync.repositories.android.RepoUtils;
 import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
 import org.mozilla.gecko.sync.repositories.domain.ClientRecordFactory;
 
+import ch.boye.httpclientandroidlib.HttpStatus;
+
 public class SyncClientsEngineStage implements GlobalSyncStage {
-  protected static final String LOG_TAG = "SyncClientsEngineStage";
-  protected static final String COLLECTION_NAME = "clients";
+  public static final String LOG_TAG = "SyncClientsEngineStage";
+  public static final String COLLECTION_NAME = "clients";
+  public static final int CLIENTS_TTL_REFRESH = 604800000; // 7 days
+  public static final int MAX_UPLOAD_FAILURE_COUNT = 5;
 
   protected GlobalSession session;
   protected final ClientRecordFactory factory = new ClientRecordFactory();
@@ -37,8 +44,9 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
   protected ClientDownloadDelegate clientDownloadDelegate;
   protected ClientsDatabaseAccessor db;
 
-  // Account/Profile info
-  protected boolean shouldWipe;
+  protected volatile boolean shouldWipe;
+  protected volatile boolean commandsProcessedShouldUpload;
+  protected final AtomicInteger uploadAttemptsCount = new AtomicInteger();
 
   /**
    * The following two delegates, ClientDownloadDelegate and ClientUploadDelegate
@@ -53,6 +61,9 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
    */
   public class ClientDownloadDelegate extends WBOCollectionRequestDelegate {
 
+    // We use this on each WBO, so lift it out.
+    final ClientsDataDelegate clientsDelegate = session.getClientsDelegate();
+
     @Override
     public String credentials() {
       return session.credentials();
@@ -67,15 +78,22 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
     @Override
     public void handleRequestSuccess(SyncStorageResponse response) {
       BaseResource.consumeEntity(response); // We don't need the response at all.
+      final int clientsCount;
       try {
-        clientUploadDelegate = new ClientUploadDelegate();
-        session.getClientsDelegate().setClientsCount(db.clientsCount());
-        checkAndUpload();
+        clientsCount = db.clientsCount();
       } finally {
         // Close the database to clear cached readableDatabase/writableDatabase
         // after we've completed our last transaction (db.store()).
         db.close();
       }
+
+      Logger.debug(LOG_TAG, "Database contains " + clientsCount + " clients.");
+      Logger.debug(LOG_TAG, "Server response asserts " + response.weaveRecords() + " records.");
+
+      // TODO: persist the response timestamp to know whether to download next time (Bug 726055).
+      clientUploadDelegate = new ClientUploadDelegate();
+      clientsDelegate.setClientsCount(clientsCount);
+      checkAndUpload();
     }
 
     @Override
@@ -106,6 +124,15 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
       ClientRecord r;
       try {
         r = (ClientRecord) factory.createRecord(record.decrypt());
+        if (clientsDelegate.isLocalGUID(r.guid)) {
+          // Oh hey! Our record is on the server. This is the authoritative
+          // server timestamp, so let's hang on to it to decide whether we
+          // need to upload.
+          session.config.persistServerClientRecordTimestamp(r.lastModified);
+
+          // Process commands.
+          processCommands(r.commands);
+        }
         RepoUtils.logClient(r);
       } catch (Exception e) {
         session.abort(e, "Exception handling client WBO.");
@@ -135,21 +162,47 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
 
     @Override
     public String ifUnmodifiedSince() {
-      // TODO last client upload time?
-      return null;
+      Long timestampInMilliseconds = session.config.getPersistedServerClientRecordTimestamp();
+
+      // It's the first upload so we don't care about X-If-Unmodified-Since.
+      if (timestampInMilliseconds == 0) {
+        return null;
+      }
+
+      return Utils.millisecondsToDecimalSecondsString(timestampInMilliseconds);
     }
 
     @Override
     public void handleRequestSuccess(SyncStorageResponse response) {
+      Logger.debug(LOG_TAG, "Upload succeeded.");
+      commandsProcessedShouldUpload = false;
+      uploadAttemptsCount.set(0);
+      session.config.persistServerClientRecordTimestamp(response.normalizedWeaveTimestamp());
+
       BaseResource.consumeEntity(response);
       session.advance();
     }
 
     @Override
     public void handleRequestFailure(SyncStorageResponse response) {
-      Logger.info(LOG_TAG, "Client upload failed. Aborting sync.");
-      BaseResource.consumeEntity(response); // The exception thrown should need the response body.
-      session.abort(new HTTPFailureException(response), "Client upload failed.");
+      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 ||
+          statusCode == HttpStatus.SC_PRECONDITION_FAILED ||
+          uploadAttemptsCount.incrementAndGet() > MAX_UPLOAD_FAILURE_COUNT) {
+        Logger.debug(LOG_TAG, "Client upload failed. Aborting sync.");
+        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 &&
+      // statusCode != 412 &&
+      // uploadAttemptCount < MAX_UPLOAD_FAILURE_COUNT
+      checkAndUpload();
     }
 
     @Override
@@ -200,26 +253,49 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
     return true;
   }
 
-  // TODO: Bug 729248 - Smarter upload of client records.
   protected boolean shouldUpload() {
-    return true;
+    if (commandsProcessedShouldUpload) {
+      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 processCommands(JSONArray commands) {
+    if (commands == null ||
+        commands.size() == 0) {
+      return;
+    }
+
+    commandsProcessedShouldUpload = true;
+
+    // TODO: Bug 715792 - Process commands here.
   }
 
   protected void checkAndUpload() {
     if (!shouldUpload()) {
-      Logger.trace(LOG_TAG, "Not uploading client record.");
+      Logger.debug(LOG_TAG, "Not uploading client record.");
       session.advance();
       return;
     }
 
     // Generate CryptoRecord from ClientRecord to upload.
-    String encryptionFailure = "Couldn't encrypt new client record.";
-    ClientRecord localClient  = newLocalClientRecord(session.getClientsDelegate());
-    CryptoRecord cryptoRecord = localClient.getEnvelope();
+    final String encryptionFailure = "Couldn't encrypt new client record.";
+    final ClientRecord localClient = newLocalClientRecord(session.getClientsDelegate());
     try {
+      CryptoRecord cryptoRecord = localClient.getEnvelope();
       cryptoRecord.keyBundle = clientUploadDelegate.keyBundle();
       cryptoRecord.encrypt();
-      this.wipeAndStore(localClient);
       this.uploadClientRecord(cryptoRecord);
     } catch (UnsupportedEncodingException e) {
       session.abort(e, encryptionFailure + " Unsupported encoding.");
@@ -246,6 +322,7 @@ public class SyncClientsEngineStage implements GlobalSyncStage {
   }
 
   protected void uploadClientRecord(CryptoRecord record) {
+    Logger.debug(LOG_TAG, "Uploading client record " + record.guid);
     try {
       URI putURI = session.config.wboURI(COLLECTION_NAME, record.guid);
 

mobile/android/base/sync/syncadapter/SyncAdapter.java

@@ -422,6 +422,11 @@ public class SyncAdapter extends AbstractThreadedSyncAdapter implements GlobalSe
         Integer.toString(clientsCount));
   }
 
+  @Override
+  public boolean isLocalGUID(String guid) {
+    return getAccountGUID().equals(guid);
+  }
+
   @Override
   public synchronized int getClientsCount() {
     String clientsCount = mAccountManager.getUserData(localAccount, Constants.NUM_CLIENTS);