Bug 1148432 - Sync reading list deletions. r=nalexander, a=readinglist
authorRichard Newman <rnewman@mozilla.com>
Fri, 27 Mar 2015 11:54:16 -0700
changeset 258207 c27964aaa4c5
parent 258206 f1c7c471c2d8
child 258208 ac9b83aca21f
push id4620
push userrnewman@mozilla.com
push date2015-04-02 16:21 +0000
treeherdermozilla-beta@27f61020a9e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander, readinglist
bugs1148432
milestone38.0
Bug 1148432 - Sync reading list deletions. r=nalexander, a=readinglist
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();
 }