Bug 1148432 - Sync reading list deletions. r=nalexander
authorRichard Newman <rnewman@mozilla.com>
Fri, 27 Mar 2015 11:54:16 -0700
changeset 266471 f81a156c8a71af5dc7a095034a11759e2d2c4bd6
parent 266470 eab17adb9a35eecd07fb41375b10d58e0e10fa79
child 266472 5bc74f6fa5c8034bfb151efee6a622d472ea0acb
child 266501 24bf86f4b440353e074c3e5c8323b2b218aa4b0e
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs1148432
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1148432 - Sync reading list deletions. r=nalexander
mobile/android/base/reading/LocalReadingListStorage.java
mobile/android/base/reading/ReadingListChangeAccumulator.java
mobile/android/base/reading/ReadingListClient.java
mobile/android/base/reading/ReadingListClientRecordFactory.java
mobile/android/base/reading/ReadingListDeleteDelegate.java
mobile/android/base/reading/ReadingListStorage.java
mobile/android/base/reading/ReadingListSyncAdapter.java
mobile/android/base/reading/ReadingListSynchronizer.java
mobile/android/base/reading/ReadingListSynchronizerDelegate.java
--- a/mobile/android/base/reading/LocalReadingListStorage.java
+++ b/mobile/android/base/reading/LocalReadingListStorage.java
@@ -43,64 +43,70 @@ public class LocalReadingListStorage imp
     private final Queue<ClientReadingListRecord> changes;
 
     /**
      * These are deletions that result from uploading new or changed records to the server.
      * They should always correspond to local records.
      * These are not common: they should only occur if a conflict occurs.
      */
     private final Queue<ClientReadingListRecord> deletions;
+    private final Queue<String> deletedGUIDs;
 
     /**
      * These are additions or changes fetched from the server.
      * At the point of collection we don't know if they're records
      * that exist locally.
      *
      * Batching these here, rather than in the client or the synchronizer,
      * puts the storage implementation in control of when batches are flushed,
      * or if batches are used at all.
      */
     private final Queue<ServerReadingListRecord> additionsOrChanges;
 
     LocalReadingListChangeAccumulator() {
       this.changes = new ConcurrentLinkedQueue<>();
       this.deletions = new ConcurrentLinkedQueue<>();
+      this.deletedGUIDs = new ConcurrentLinkedQueue<>();
       this.additionsOrChanges = new ConcurrentLinkedQueue<>();
     }
 
     public boolean flushDeletions() throws RemoteException {
-      if (deletions.isEmpty()) {
+      if (deletions.isEmpty() && deletedGUIDs.isEmpty()) {
         return true;
       }
 
       long[] ids = new long[deletions.size()];
-      String[] guids = new String[deletions.size()];
+      String[] guids = new String[deletions.size() + deletedGUIDs.size()];
       int iID = 0;
       int iGUID = 0;
       for (ClientReadingListRecord record : deletions) {
         if (record.clientMetadata.id > -1L) {
           ids[iID++] = record.clientMetadata.id;
         } else {
           final String guid = record.getGUID();
           if (guid == null) {
             continue;
           }
           guids[iGUID++] = guid;
         }
       }
+      for (String guid : deletedGUIDs) {
+        guids[iGUID++] = guid;
+      }
 
       if (iID > 0) {
         client.delete(URI_WITH_DELETED, RepoUtils.computeSQLLongInClause(ids, ReadingListItems._ID), null);
       }
 
       if (iGUID > 0) {
         client.delete(URI_WITH_DELETED, RepoUtils.computeSQLInClause(iGUID, ReadingListItems.GUID), guids);
       }
 
       deletions.clear();
+      deletedGUIDs.clear();
       return true;
     }
 
     public boolean flushRecordChanges() throws RemoteException {
       if (changes.isEmpty() && additionsOrChanges.isEmpty()) {
         return true;
       }
 
@@ -207,16 +213,21 @@ public class LocalReadingListStorage imp
     }
 
     @Override
     public void addDeletion(ClientReadingListRecord record) {
       deletions.add(record);
     }
 
     @Override
+    public void addDeletion(String guid) {
+      deletedGUIDs.add(guid);
+    }
+
+    @Override
     public void addChangedRecord(ClientReadingListRecord record) {
       changes.add(record);
     }
 
     @Override
     public void addUploadedRecord(ClientReadingListRecord up,
                                   ServerReadingListRecord down) {
       // TODO
@@ -314,16 +325,30 @@ public class LocalReadingListStorage imp
     try {
       return client.query(URI_WITHOUT_DELETED, projection, selection, null, null);
     } catch (RemoteException e) {
       throw new IllegalStateException(e);
     }
   }
 
   @Override
+  public Cursor getDeletedItems() {
+    final String[] projection = new String[] {
+      ReadingListItems.GUID,
+    };
+
+    final String selection = "(" + ReadingListItems.IS_DELETED + " = 1) AND (" + ReadingListItems.GUID + " IS NOT NULL)";
+    try {
+      return client.query(URI_WITH_DELETED, projection, selection, null, null);
+    } catch (RemoteException e) {
+      throw new IllegalStateException(e);
+    }
+  }
+
+  @Override
   public Cursor getNew() {
     // N.B., query for items that have no GUID, regardless of status.
     // They should all be marked as NEW, but belt and braces.
     final String selection = WHERE_STATUS_NEW + " OR (" + ReadingListItems.GUID + " IS NULL)";
 
     try {
       return client.query(URI_WITHOUT_DELETED, null, selection, null, null);
     } catch (RemoteException e) {
--- a/mobile/android/base/reading/ReadingListChangeAccumulator.java
+++ b/mobile/android/base/reading/ReadingListChangeAccumulator.java
@@ -6,14 +6,15 @@ package org.mozilla.gecko.reading;
 
 
 /**
  * Grab one of these, then you can add records to it by parsing
  * server responses. Finishing it will flush those changes (e.g.,
  * via UPDATE) to the DB.
  */
 public interface ReadingListChangeAccumulator {
+  void addDeletion(String guid);
   void addDeletion(ClientReadingListRecord record);
   void addChangedRecord(ClientReadingListRecord record);
   void addUploadedRecord(ClientReadingListRecord up, ServerReadingListRecord down);
   void addDownloadedRecord(ServerReadingListRecord down);
   void finish() throws Exception;
 }
--- a/mobile/android/base/reading/ReadingListClient.java
+++ b/mobile/android/base/reading/ReadingListClient.java
@@ -426,16 +426,86 @@ public class ReadingListClient {
     }
 
     @Override
     void again(ClientReadingListRecord record) {
       patch(record, PatchBatchingUploadDelegate.this);
     }
   }
 
+  private class DeleteBatchingDelegate implements ReadingListDeleteDelegate {
+    private final Queue<String> queue;
+    private final ReadingListDeleteDelegate batchDeleteDelegate;
+    private final Executor executor;
+
+    DeleteBatchingDelegate(Queue<String> guids,
+                           ReadingListDeleteDelegate batchDeleteDelegate,
+                           Executor executor) {
+      this.queue = guids;
+      this.batchDeleteDelegate = batchDeleteDelegate;
+      this.executor = executor;
+    }
+
+    void next() {
+      final String guid = queue.poll();
+      executor.execute(new Runnable() {
+        @Override
+        public void run() {
+          if (guid == null) {
+            batchDeleteDelegate.onBatchDone();
+            return;
+          }
+
+          again(guid);
+        }
+      });
+    }
+
+    void again(String guid) {
+      delete(guid, DeleteBatchingDelegate.this, -1L);
+    }
+
+    @Override
+    public void onSuccess(ReadingListRecordResponse response,
+                          ReadingListRecord record) {
+      batchDeleteDelegate.onSuccess(response, record);
+      next();
+    }
+
+    @Override
+    public void onPreconditionFailed(String guid, MozResponse response) {
+      batchDeleteDelegate.onPreconditionFailed(guid, response);
+      next();
+    }
+
+    @Override
+    public void onRecordMissingOrDeleted(String guid, MozResponse response) {
+      batchDeleteDelegate.onRecordMissingOrDeleted(guid, response);
+      next();
+    }
+
+    @Override
+    public void onFailure(Exception e) {
+      batchDeleteDelegate.onFailure(e);
+      next();
+    }
+
+    @Override
+    public void onFailure(MozResponse response) {
+      batchDeleteDelegate.onFailure(response);
+      next();
+    }
+
+    @Override
+    public void onBatchDone() {
+      // This should never occur, but if it does, pass through.
+      batchDeleteDelegate.onBatchDone();
+    }
+  }
+
   // Deliberately declare `delegate` non-final so we can't capture it below. We prefer
   // to use `recordDelegate` explicitly.
   public void getOne(final String guid, ReadingListRecordDelegate delegate, final long ifModifiedSince) {
     final BaseResource r = getRelativeArticleResource(guid);
     r.delegate = new SingleRecordResourceDelegate(r, auth, delegate, ReadingListRecordResponse.FACTORY, ifModifiedSince, guid);
     if (ReadingListConstants.DEBUG) {
       Logger.info(LOG_TAG, "Getting record " + guid);
     }
@@ -506,16 +576,27 @@ public class ReadingListClient {
 
     final ExtendedJSONObject body = up.toJSON();
     if (ReadingListConstants.DEBUG) {
       Logger.info(LOG_TAG, "Uploading new record: " + body.toJSONString());
     }
     r.post(body);
   }
 
+  public void delete(final Queue<String> guids, final Executor executor, final ReadingListDeleteDelegate batchDeleteDelegate) {
+    if (guids.isEmpty()) {
+      batchDeleteDelegate.onBatchDone();
+      return;
+    }
+
+    final ReadingListDeleteDelegate deleteDelegate = new DeleteBatchingDelegate(guids, batchDeleteDelegate, executor);
+
+    delete(guids.poll(), deleteDelegate, -1L);
+  }
+
   public void delete(final String guid, final ReadingListDeleteDelegate delegate, final long ifUnmodifiedSince) {
     final BaseResource r = getRelativeArticleResource(guid);
 
     // If If-Unmodified-Since is provided, and the record has been modified,
     // we'll receive a 412 Precondition Failed.
     // If the record is missing or already deleted, a 404 will be returned.
     // Otherwise, the response will be the deleted record.
     r.delegate = new ReadingListResourceDelegate<ReadingListRecordResponse>(r, auth, ReadingListRecordResponse.FACTORY) {
--- a/mobile/android/base/reading/ReadingListClientRecordFactory.java
+++ b/mobile/android/base/reading/ReadingListClientRecordFactory.java
@@ -5,19 +5,19 @@
 package org.mozilla.gecko.reading;
 
 import org.mozilla.gecko.AppConstants.Versions;
 import org.mozilla.gecko.db.BrowserContract.ReadingListItems;
 import org.mozilla.gecko.reading.ReadingListRecord.ServerMetadata;
 import org.mozilla.gecko.sync.ExtendedJSONObject;
 
 import android.annotation.TargetApi;
+import android.database.AbstractWindowedCursor;
 import android.database.Cursor;
 import android.database.CursorWindow;
-import android.database.sqlite.SQLiteCursor;
 import android.os.Build;
 
 /**
  * This class converts database rows into {@link ClientReadingListRecord}s.
  *
  * In doing so it has to:
  *
  *  * Translate column names.
@@ -127,21 +127,21 @@ public class ReadingListClientRecordFact
     default:
       // Do nothing.
       return;
     }
   }
 
   @SuppressWarnings("deprecation")
   private final void fillGingerbread(ExtendedJSONObject o, Cursor c, String f, int i) {
-    if (!(c instanceof SQLiteCursor)) {
+    if (!(c instanceof AbstractWindowedCursor)) {
       throw new IllegalStateException("Unable to handle cursors that don't have a CursorWindow!");
     }
 
-    final SQLiteCursor sqc = (SQLiteCursor) c;
+    final AbstractWindowedCursor sqc = (AbstractWindowedCursor) c;
     final CursorWindow w = sqc.getWindow();
     final int pos = c.getPosition();
     if (w.isNull(pos, i)) {
       putNull(o, f);
     } else if (w.isString(pos, i)) {
       put(o, f, c.getString(i));
     } else if (w.isLong(pos, i)) {
       put(o, f, c.getLong(i));
--- a/mobile/android/base/reading/ReadingListDeleteDelegate.java
+++ b/mobile/android/base/reading/ReadingListDeleteDelegate.java
@@ -3,17 +3,19 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.reading;
 
 import org.mozilla.gecko.sync.net.MozResponse;
 
 /**
  * Response delegate for a server DELETE.
- * Only one of these methods will be called, and it will be called precisely once.
+ * Only one of these methods will be called, and it will be called precisely once,
+ * unless batching is used.
  */
 public interface ReadingListDeleteDelegate {
   void onSuccess(ReadingListRecordResponse response, ReadingListRecord record);
   void onPreconditionFailed(String guid, MozResponse response);
   void onRecordMissingOrDeleted(String guid, MozResponse response);
   void onFailure(Exception e);
   void onFailure(MozResponse response);
+  void onBatchDone();
 }
--- a/mobile/android/base/reading/ReadingListStorage.java
+++ b/mobile/android/base/reading/ReadingListStorage.java
@@ -3,13 +3,14 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.reading;
 
 import android.database.Cursor;
 
 public interface ReadingListStorage {
   Cursor getModified();
+  Cursor getDeletedItems();
   Cursor getStatusChanges();
   Cursor getNew();
   Cursor getAll();
   ReadingListChangeAccumulator getChangeAccumulator();
 }
--- a/mobile/android/base/reading/ReadingListSyncAdapter.java
+++ b/mobile/android/base/reading/ReadingListSyncAdapter.java
@@ -77,16 +77,22 @@ public class ReadingListSyncAdapter exte
     @Override
     public void onUnableToSync(Exception e) {
       Logger.warn(LOG_TAG, "Unable to sync.", e);
       cpc.release();
       syncDelegate.handleError(e);
     }
 
     @Override
+    public void onDeletionsUploadComplete() {
+      Logger.debug(LOG_TAG, "Step: onDeletionsUploadComplete");
+      this.result.stats.numEntries += 1;   // TODO: Bug 1140809.
+    }
+
+    @Override
     public void onStatusUploadComplete(Collection<String> uploaded,
                                        Collection<String> failed) {
       Logger.debug(LOG_TAG, "Step: onStatusUploadComplete");
       this.result.stats.numEntries += 1;   // TODO: Bug 1140809.
     }
 
     @Override
     public void onNewItemUploadComplete(Collection<String> uploaded,
--- a/mobile/android/base/reading/ReadingListSynchronizer.java
+++ b/mobile/android/base/reading/ReadingListSynchronizer.java
@@ -171,16 +171,90 @@ public class ReadingListSynchronizer {
           next.fail(e);
         }
         return;
       }
       next.fail();
     }
   }
 
+  private static class DeletionUploadDelegate implements ReadingListDeleteDelegate {
+    private final ReadingListChangeAccumulator acc;
+    private final StageDelegate next;
+
+    DeletionUploadDelegate(ReadingListChangeAccumulator acc, StageDelegate next) {
+      this.acc = acc;
+      this.next = next;
+    }
+
+    @Override
+    public void onBatchDone() {
+      try {
+        acc.finish();
+      } catch (Exception e) {
+        next.fail(e);
+        return;
+      }
+
+      next.next();
+    }
+
+    @Override
+    public void onSuccess(ReadingListRecordResponse response,
+                          ReadingListRecord record) {
+      Logger.debug(LOG_TAG, "Tracking uploaded deletion " + record.getGUID());
+      acc.addDeletion(record.getGUID());
+    }
+
+    @Override
+    public void onPreconditionFailed(String guid, MozResponse response) {
+      // Should never happen.
+    }
+
+    @Override
+    public void onRecordMissingOrDeleted(String guid, MozResponse response) {
+      // Great!
+      Logger.debug(LOG_TAG, "Tracking redundant deletion " + guid);
+      acc.addDeletion(guid);
+    }
+
+    @Override
+    public void onFailure(Exception e) {
+      // Ignore.
+    }
+
+    @Override
+    public void onFailure(MozResponse response) {
+      // Ignore.
+    }
+  }
+
+
+  private Queue<String> collectDeletedIDsFromCursor(Cursor cursor) {
+    try {
+      final Queue<String> toDelete = new LinkedList<>();
+
+      final int columnGUID = cursor.getColumnIndexOrThrow(ReadingListItems.GUID);
+
+      while (cursor.moveToNext()) {
+        final String guid = cursor.getString(columnGUID);
+        if (guid == null) {
+          // Nothing we can do here.
+          continue;
+        }
+
+        toDelete.add(guid);
+      }
+
+      return toDelete;
+    } finally {
+      cursor.close();
+    }
+  }
+
   private static class StatusUploadDelegate implements ReadingListRecordUploadDelegate {
     private final ReadingListChangeAccumulator acc;
 
     public volatile int failures = 0;
     private final StageDelegate next;
 
     StatusUploadDelegate(ReadingListChangeAccumulator acc, StageDelegate next) {
       this.acc = acc;
@@ -457,16 +531,46 @@ public class ReadingListSynchronizer {
         toUpload.add(record);
       }
       return toUpload;
     } finally {
       cursor.close();
     }
   }
 
+  protected void uploadDeletions(final StageDelegate delegate) {
+    try {
+      final Cursor cursor = local.getDeletedItems();
+
+      if (cursor == null) {
+        delegate.fail(new RuntimeException("Unable to get unread item cursor."));
+        return;
+      }
+
+      final Queue<String> toDelete = collectDeletedIDsFromCursor(cursor);
+
+      // Nothing to do.
+      if (toDelete.isEmpty()) {
+        Logger.debug(LOG_TAG, "No new deletions to upload. Skipping.");
+        delegate.next();
+        return;
+      } else {
+        Logger.debug(LOG_TAG, "Deleting " + toDelete.size() + " records from the server.");
+      }
+
+      final ReadingListChangeAccumulator acc = this.local.getChangeAccumulator();
+      final DeletionUploadDelegate deleteDelegate = new DeletionUploadDelegate(acc, delegate);
+
+      // Don't send I-U-S; we're happy for the client to win, because this is a one-way state change.
+      this.remote.delete(toDelete, executor, deleteDelegate);
+    } catch (Exception e) {
+      delegate.fail(e);
+    }
+  }
+
   // N.B., status changes for items that haven't been uploaded yet are dealt with in
   // uploadNewItems.
   protected void uploadUnreadChanges(final StageDelegate delegate) {
     try {
       final Cursor cursor = local.getStatusChanges();
 
       if (cursor == null) {
         delegate.fail(new RuntimeException("Unable to get unread item cursor."));
@@ -692,53 +796,68 @@ public class ReadingListSynchronizer {
     try {
       remote.getAll(spec, recordDelegate, since);
     } catch (URISyntaxException e) {
       delegate.fail(e);
     }
   }
 
   /**
-   * Upload unread changes, then upload new items, then call `done`.
+   * Upload deletions and unread changes, then upload new items, then call `done`.
    * Substantially modified records are uploaded last.
    *
    * @param syncDelegate only used for status callbacks.
    */
   private void syncUp(final ReadingListSynchronizerDelegate syncDelegate, final StageDelegate done) {
-    // Second.
+    // Third.
     final StageDelegate onNewItemsUploaded = new NextDelegate(executor) {
       @Override
       public void doNext() {
         syncDelegate.onNewItemUploadComplete(null, null);
         done.next();
       }
 
       @Override
       public void doFail(Exception e) {
         done.fail(e);
       }
     };
 
-    // First.
+    // Second.
     final StageDelegate onUnreadChangesUploaded = new NextDelegate(executor) {
       @Override
       public void doNext() {
         syncDelegate.onStatusUploadComplete(null, null);
         uploadNewItems(onNewItemsUploaded);
       }
 
       @Override
       public void doFail(Exception e) {
         Logger.warn(LOG_TAG, "Uploading unread changes failed.", e);
         done.fail(e);
       }
     };
 
+    // First.
+    final StageDelegate onDeletionsUploaded = new NextDelegate(executor) {
+      @Override
+      public void doNext() {
+        syncDelegate.onDeletionsUploadComplete();
+        uploadUnreadChanges(onUnreadChangesUploaded);
+      }
+
+      @Override
+      public void doFail(Exception e) {
+        Logger.warn(LOG_TAG, "Uploading deletions failed.", e);
+        done.fail(e);
+      }
+    };
+
     try {
-      uploadUnreadChanges(onUnreadChangesUploaded);
+      uploadDeletions(onDeletionsUploaded);
     } catch (Exception ee) {
       done.fail(ee);
     }
   }
 
 
   /**
    * Do an upload-only sync.
--- a/mobile/android/base/reading/ReadingListSynchronizerDelegate.java
+++ b/mobile/android/base/reading/ReadingListSynchronizerDelegate.java
@@ -7,16 +7,17 @@ package org.mozilla.gecko.reading;
 import java.util.Collection;
 
 public interface ReadingListSynchronizerDelegate {
   // Called on failure.
   void onUnableToSync(Exception e);
 
   // These are called sequentially, or not at all
   // if a failure occurs.
+  void onDeletionsUploadComplete();
   void onStatusUploadComplete(Collection<String> uploaded, Collection<String> failed);
   void onNewItemUploadComplete(Collection<String> uploaded, Collection<String> failed);
   void onDownloadComplete();
   void onModifiedUploadComplete();
 
   // If no failure occurred, called at the end.
   void onComplete();
 }