Bug 1140810 - Upload material (non-status) Reading List modifications. r=rnewman, a=readinglist
authorNick Alexander <nalexander@mozilla.com>
Wed, 25 Mar 2015 16:35:26 -0700
changeset 258206 f1c7c471c2d8
parent 258205 daf8a9291a9b
child 258207 c27964aaa4c5
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)
reviewersrnewman, readinglist
bugs1140810
milestone38.0
Bug 1140810 - Upload material (non-status) Reading List modifications. r=rnewman, a=readinglist ======== https://github.com/mozilla-services/android-sync/commit/575d80fddb1afc10123b928a07800bda15c8c087 Author: Nick Alexander <nalexander@mozilla.com> Bug 1140810 - Part 2: Upload material (non-status) modifications. ======== https://github.com/mozilla-services/android-sync/commit/a86e734ef12624267e12898140bf72db8e7b56c2 Author: Nick Alexander <nalexander@mozilla.com> Date: Wed Mar 25 10:56:07 2015 -0700 Bug 1140810 - Part 1: Add storage test for material (non-status) modifications. ======== https://github.com/mozilla-services/android-sync/commit/2259378d084df7c203ea29e4dec8c292d4cdf6fc Author: Nick Alexander <nalexander@mozilla.com> Date: Wed Mar 25 13:33:13 2015 -0700 Bug 1140810 - Part 0: Add and use HTTP PATCH. ======== https://github.com/mozilla-services/android-sync/commit/0222d53d980c87a52c1416f4e683af79a9c79983 Author: Nick Alexander <nalexander@mozilla.com> Date: Wed Mar 25 14:49:19 2015 -0700 Bug 1140810 - Pre: Don't fail in status upload when there are no failures. ======== https://github.com/mozilla-services/android-sync/commit/7f2feede3bf7a9d83c59a3184a58f6b125b21bbd Author: Nick Alexander <nalexander@mozilla.com> Date: Wed Mar 25 14:53:45 2015 -0700 Bug 1140810 - Pre: convert 4 spaces to 2 spaces. ======== https://github.com/mozilla-services/android-sync/commit/49e80d271efcbf5554f10e35591027b5a81cefcc Author: Nick Alexander <nalexander@mozilla.com> Date: Wed Mar 25 10:40:26 2015 -0700 Bug 1140810 - Pre: Fix whitespace.
mobile/android/base/reading/LocalReadingListStorage.java
mobile/android/base/reading/ReadingListClient.java
mobile/android/base/reading/ReadingListConstants.java
mobile/android/base/reading/ReadingListSynchronizer.java
mobile/android/base/reading/ServerReadingListRecord.java
mobile/android/base/sync/net/BaseResource.java
mobile/android/base/sync/net/Resource.java
mobile/android/base/sync/net/SyncStorageRequest.java
--- a/mobile/android/base/reading/LocalReadingListStorage.java
+++ b/mobile/android/base/reading/LocalReadingListStorage.java
@@ -261,25 +261,27 @@ public class LocalReadingListStorage imp
 
   public Cursor getModifiedWithSelection(final String selection) {
     final String[] projection = new String[] {
       ReadingListItems.GUID,
       ReadingListItems.IS_FAVORITE,
       ReadingListItems.RESOLVED_TITLE,
       ReadingListItems.RESOLVED_URL,
       ReadingListItems.EXCERPT,
+      // TODO: ReadingListItems.IS_ARTICLE,
+      // TODO: ReadingListItems.WORD_COUNT,
     };
 
-
     try {
       return client.query(URI_WITHOUT_DELETED, projection, selection, null, null);
     } catch (RemoteException e) {
       throw new IllegalStateException(e);
     }
   }
+
   @Override
   public Cursor getModified() {
     final String selection = ReadingListItems.SYNC_STATUS + " = " + ReadingListItems.SYNC_STATUS_MODIFIED;
     return getModifiedWithSelection(selection);
   }
 
   // Return changed items that aren't just status changes.
   // This isn't necessary because we insist on processing status changes before modified items.
--- a/mobile/android/base/reading/ReadingListClient.java
+++ b/mobile/android/base/reading/ReadingListClient.java
@@ -477,17 +477,17 @@ public class ReadingListClient {
     final BaseResource r = getRelativeArticleResource(guid);
     r.delegate = new DelegatingUploadResourceDelegate(r, auth, ReadingListRecordResponse.FACTORY, up,
                                                       uploadDelegate);
 
     final ExtendedJSONObject body = up.toJSON();
     if (ReadingListConstants.DEBUG) {
       Logger.info(LOG_TAG, "Patching record " + guid + ": " + body.toJSONString());
     }
-    r.post(body);
+    r.patch(body);
   }
 
   /**
    * Mutates the provided queue.
    */
   public void add(final Queue<ClientReadingListRecord> queue, final Executor executor, final ReadingListRecordUploadDelegate batchUploadDelegate) {
     if (queue.isEmpty()) {
       batchUploadDelegate.onBatchDone();
--- a/mobile/android/base/reading/ReadingListConstants.java
+++ b/mobile/android/base/reading/ReadingListConstants.java
@@ -5,14 +5,14 @@
 package org.mozilla.gecko.reading;
 
 import org.mozilla.gecko.AppConstants;
 
 public class ReadingListConstants {
   public static final String GLOBAL_LOG_TAG = "FxReadingList";
   public static final String USER_AGENT = "Firefox-Android-FxReader/" + AppConstants.MOZ_APP_VERSION + " (" + AppConstants.MOZ_APP_DISPLAYNAME + ")";
   public static final String DEFAULT_DEV_ENDPOINT = "https://readinglist.dev.mozaws.net/v1/";
-  public static final String DEFAULT_PROD_ENDPOINT = null;     // TODO
+  public static final String DEFAULT_PROD_ENDPOINT = "https://readinglist.services.mozilla.com/v1/";
 
   public static final String OAUTH_ENDPOINT_PROD = "https://oauth.accounts.firefox.com/v1";
 
   public static boolean DEBUG = false;
 }
--- a/mobile/android/base/reading/ReadingListSynchronizer.java
+++ b/mobile/android/base/reading/ReadingListSynchronizer.java
@@ -243,21 +243,20 @@ public class ReadingListSynchronizer {
       try {
         acc.finish();
       } catch (Exception e) {
         next.fail(e);
         return;
       }
 
       if (failures == 0) {
-        try {
-          next.next();
-        } catch (Exception e) {
-        }
+        next.next();
+        return;
       }
+
       next.fail();
     }
   }
 
   private Queue<ClientReadingListRecord> collectStatusChangesFromCursor(final Cursor cursor) {
     try {
       final Queue<ClientReadingListRecord> toUpload = new LinkedList<>();
 
@@ -300,16 +299,159 @@ public class ReadingListSynchronizer {
       }
 
       return toUpload;
     } finally {
       cursor.close();
     }
   }
 
+  private static class ModifiedUploadDelegate implements ReadingListRecordUploadDelegate {
+    private final ReadingListChangeAccumulator acc;
+
+    public volatile int failures = 0;
+    private final StageDelegate next;
+
+    ModifiedUploadDelegate(ReadingListChangeAccumulator acc, StageDelegate next) {
+      this.acc = acc;
+      this.next = next;
+    }
+
+    @Override
+    public void onInvalidUpload(ClientReadingListRecord up,
+                                ReadingListResponse response) {
+      recordFailed(up);
+    }
+
+    @Override
+    public void onConflict(ClientReadingListRecord up,
+                           ReadingListResponse response) {
+      // This can happen for a material change.
+      failures++;
+    }
+
+    @Override
+    public void onSuccess(ClientReadingListRecord up,
+                          ReadingListRecordResponse response,
+                          ServerReadingListRecord down) {
+      if (!TextUtils.equals(up.getGUID(), down.getGUID())) {
+        // Uh oh!
+        // This should never occur. We should get an onConflict instead,
+        // so this would imply a server bug, or something like a truncated
+        // over-long GUID string.
+        //
+        // Should we wish to recover from this case, probably the right approach
+        // is to ensure that the GUID is overwritten locally (given that we know
+        // the numeric ID).
+      }
+
+      // We could upload our material changes but get back additional status
+      // changes from the server.  Apply them.
+      acc.addChangedRecord(up.givenServerRecord(down));
+    }
+
+    @Override
+    public void onBadRequest(ClientReadingListRecord up, MozResponse response) {
+      recordFailed(up);
+    }
+
+    @Override
+    public void onFailure(ClientReadingListRecord up, Exception ex) {
+      recordFailed(up);
+    }
+
+    @Override
+    public void onFailure(ClientReadingListRecord up, MozResponse response) {
+      // Since we download and apply remote changes before uploading local changes, the conflict
+      // window is very small.  We should essentially never see true conflicts here.
+      if (response.getStatusCode() == 404) {
+        // We shouldn't see a 404; we should see a record with deleted=true when
+        // we fetch remote changes.
+        Logger.warn(LOG_TAG, "Ignoring 404 response patching record with guid: " + up.getGUID());
+      } else if (response.getStatusCode() == 409) {
+        // A 409 indicates that resolved_url has collided with an existing
+        // record. Not much to be done here.
+        Logger.info(LOG_TAG, "409 response seen; deleting record with guid: " + up.getGUID());
+        acc.addDeletion(up);
+      } else {
+        // We should never see a 412 since we race to upload our changes (and
+        // accept whatever the server gives us back).
+        recordFailed(up);
+      }
+    }
+
+    private void recordFailed(ClientReadingListRecord up) {
+      ++failures;
+    }
+
+    @Override
+    public void onBatchDone() {
+      try {
+        acc.finish();
+      } catch (Exception e) {
+        next.fail(e);
+        return;
+      }
+
+      if (failures == 0) {
+        next.next();
+        return;
+      }
+
+      next.fail();
+    }
+  }
+
+  private Queue<ClientReadingListRecord> collectModifiedFromCursor(final Cursor cursor) {
+    try {
+      final Queue<ClientReadingListRecord> toUpload = new LinkedList<>();
+
+      final int columnGUID = cursor.getColumnIndexOrThrow(ReadingListItems.GUID);
+      final int columnExcerpt = cursor.getColumnIndexOrThrow(ReadingListItems.EXCERPT);
+      final int columnResolvedURL = cursor.getColumnIndexOrThrow(ReadingListItems.RESOLVED_URL);
+      final int columnResolvedTitle = cursor.getColumnIndexOrThrow(ReadingListItems.RESOLVED_TITLE);
+      // TODO: final int columnIsArticle = cursor.getColumnIndexOrThrow(ReadingListItems.IS_ARTICLE);
+      // TODO: final int columnWordCount = cursor.getColumnIndexOrThrow(ReadingListItems.WORD_COUNT);
+
+      while (cursor.moveToNext()) {
+        final String guid = cursor.getString(columnGUID);
+        if (guid == null) {
+          // Nothing we can do here, but this should never happen: we should
+          // have uploaded this record as new before trying to upload a
+          // material modification!
+          continue;
+        }
+
+        final ExtendedJSONObject o = new ExtendedJSONObject();
+        o.put("id", guid);
+        final String excerpt = cursor.getString(columnExcerpt); // Can be NULL.
+        final String resolvedURL = cursor.getString(columnResolvedURL); // Can be NULL.
+        final String resolvedTitle = cursor.getString(columnResolvedTitle); // Can be NULL.
+        if (excerpt == null && resolvedURL == null && resolvedTitle == null) {
+          // Nothing material to upload, so skip this record.
+          continue;
+        }
+        o.put("excerpt", excerpt);
+        o.put("resolved_url", resolvedURL);
+        o.put("resolved_title", resolvedTitle);
+        // TODO: o.put("is_article", cursor.getInt(columnIsArticle) == 1);
+        // TODO: o.put("word_count", cursor.getInt(columnWordCount));
+
+        final ClientMetadata cm = null;
+        final ServerMetadata sm = new ServerMetadata(guid, -1L);
+        final ClientReadingListRecord record = new ClientReadingListRecord(sm, cm, o);
+        toUpload.add(record);
+      }
+
+      return toUpload;
+    } finally {
+      cursor.close();
+    }
+  }
+
   private Queue<ClientReadingListRecord> accumulateNewItems(Cursor cursor) {
     try {
       final Queue<ClientReadingListRecord> toUpload = new LinkedList<>();
       final ReadingListClientRecordFactory factory = new ReadingListClientRecordFactory(cursor);
 
       ClientReadingListRecord record;
       while ((record = factory.getNext()) != null) {
         toUpload.add(record);
@@ -330,18 +472,21 @@ public class ReadingListSynchronizer {
         delegate.fail(new RuntimeException("Unable to get unread item cursor."));
         return;
       }
 
       final Queue<ClientReadingListRecord> toUpload = collectStatusChangesFromCursor(cursor);
 
       // Nothing to do.
       if (toUpload.isEmpty()) {
+        Logger.debug(LOG_TAG, "No new unread changes to upload. Skipping.");
         delegate.next();
         return;
+      } else {
+        Logger.debug(LOG_TAG, "Uploading " + toUpload.size() + " new unread changes.");
       }
 
       // Upload each record. This looks like batching, but it's really chained serial requests.
       final ReadingListChangeAccumulator acc = this.local.getChangeAccumulator();
       final StatusUploadDelegate uploadDelegate = new StatusUploadDelegate(acc, delegate);
 
       // Don't send I-U-S; in the case of favorites we're
       // happy to overwrite the server value, and in the case of unread status
@@ -363,16 +508,18 @@ public class ReadingListSynchronizer {
 
       Queue<ClientReadingListRecord> toUpload = accumulateNewItems(cursor);
 
       // Nothing to do.
       if (toUpload.isEmpty()) {
         Logger.debug(LOG_TAG, "No new items to upload. Skipping.");
         delegate.next();
         return;
+      } else {
+        Logger.debug(LOG_TAG, "Uploading " + toUpload.size() + " new items.");
       }
 
       final ReadingListChangeAccumulator acc = this.local.getChangeAccumulator();
       final NewItemUploadDelegate uploadDelegate = new NewItemUploadDelegate(acc, new StageDelegate() {
         private boolean tryFlushChanges() {
           Logger.debug(LOG_TAG, "Flushing post-upload changes.");
           try {
             acc.finish();
@@ -415,19 +562,89 @@ public class ReadingListSynchronizer {
       // ... we need to apply it locally.
       this.remote.add(toUpload, executor, uploadDelegate);
     } catch (Exception e) {
       delegate.fail(e);
       return;
     }
   }
 
-  private void uploadModified(final StageDelegate delegate) {
-    // TODO
-    delegate.next();
+  protected void uploadModified(final StageDelegate delegate) {
+    try {
+      // This looks strange because modified includes material changes and
+      // status changes, but this is called after status changes have been
+      // uploaded and removed from local storage. So what's left should be
+      // material changes.  Even so, it should be safe to upload status changes
+      // here.
+      final Cursor cursor = this.local.getModified();
+
+      if (cursor == null) {
+        delegate.fail(new RuntimeException("Unable to get modified item cursor."));
+        return;
+      }
+
+      final Queue<ClientReadingListRecord> toUpload = collectModifiedFromCursor(cursor);
+
+      // Nothing to do.
+      if (toUpload.isEmpty()) {
+        Logger.debug(LOG_TAG, "No modified items to upload. Skipping.");
+        delegate.next();
+        return;
+      } else {
+        Logger.debug(LOG_TAG, "Uploading " + toUpload.size() + " modified items.");
+      }
+
+      final ReadingListChangeAccumulator acc = this.local.getChangeAccumulator();
+      final ModifiedUploadDelegate uploadDelegate = new ModifiedUploadDelegate(acc, new StageDelegate() {
+        private boolean tryFlushChanges() {
+          Logger.debug(LOG_TAG, "Flushing post-upload changes.");
+          try {
+            acc.finish();
+            return true;
+          } catch (Exception e) {
+            Logger.warn(LOG_TAG, "Flushing changes failed! This sync went wrong.", e);
+            delegate.fail(e);
+            return false;
+          }
+        }
+
+        @Override
+        public void next() {
+          Logger.debug(LOG_TAG, "Modified items uploaded successfully.");
+
+          if (tryFlushChanges()) {
+            delegate.next();
+          }
+        }
+
+        @Override
+        public void fail() {
+          Logger.warn(LOG_TAG, "Couldn't upload modified items.");
+          if (tryFlushChanges()) {
+            delegate.fail();
+          }
+        }
+
+        @Override
+        public void fail(Exception e) {
+          Logger.warn(LOG_TAG, "Couldn't upload modified items.", e);
+          if (tryFlushChanges()) {
+            delegate.fail(e);
+          }
+        }
+      });
+
+      // Handle 201 for success, 400 for invalid, 303 for redirect.
+      // TODO: 200 == "was already on the server, we didn't touch it, here it is."
+      // ... we need to apply it locally.
+      this.remote.patch(toUpload, executor, uploadDelegate);
+    } catch (Exception e) {
+      delegate.fail(e);
+      return;
+    }
   }
 
   private void downloadIncoming(final long since, final StageDelegate delegate) {
     final ReadingListChangeAccumulator postDownload = this.local.getChangeAccumulator();
 
     final FetchSpec spec = new FetchSpec.Builder().setSince(since).build();
 
     // TODO: should we flush the accumulator if we get a failure?
--- a/mobile/android/base/reading/ServerReadingListRecord.java
+++ b/mobile/android/base/reading/ServerReadingListRecord.java
@@ -23,9 +23,13 @@ public class ServerReadingListRecord ext
   public String getTitle() {
     return this.fields.getString("title");  // TODO: resolved_title
   }
 
   @Override
   public String getAddedBy() {
     return this.fields.getString("added_by");
   }
-}
\ No newline at end of file
+
+  public String getExcerpt() {
+    return this.fields.getString("excerpt");
+  }
+}
--- a/mobile/android/base/sync/net/BaseResource.java
+++ b/mobile/android/base/sync/net/BaseResource.java
@@ -25,16 +25,17 @@ import org.mozilla.gecko.sync.ExtendedJS
 import ch.boye.httpclientandroidlib.Header;
 import ch.boye.httpclientandroidlib.HttpEntity;
 import ch.boye.httpclientandroidlib.HttpResponse;
 import ch.boye.httpclientandroidlib.HttpVersion;
 import ch.boye.httpclientandroidlib.client.AuthCache;
 import ch.boye.httpclientandroidlib.client.ClientProtocolException;
 import ch.boye.httpclientandroidlib.client.methods.HttpDelete;
 import ch.boye.httpclientandroidlib.client.methods.HttpGet;
+import ch.boye.httpclientandroidlib.client.methods.HttpPatch;
 import ch.boye.httpclientandroidlib.client.methods.HttpPost;
 import ch.boye.httpclientandroidlib.client.methods.HttpPut;
 import ch.boye.httpclientandroidlib.client.methods.HttpRequestBase;
 import ch.boye.httpclientandroidlib.client.methods.HttpUriRequest;
 import ch.boye.httpclientandroidlib.client.protocol.ClientContext;
 import ch.boye.httpclientandroidlib.conn.ClientConnectionManager;
 import ch.boye.httpclientandroidlib.conn.scheme.PlainSocketFactory;
 import ch.boye.httpclientandroidlib.conn.scheme.Scheme;
@@ -337,16 +338,24 @@ public class BaseResource implements Res
   public void post(HttpEntity body) {
     Logger.debug(LOG_TAG, "HTTP POST " + this.uri.toASCIIString());
     HttpPost request = new HttpPost(this.uri);
     request.setEntity(body);
     this.go(request);
   }
 
   @Override
+  public void patch(HttpEntity body) {
+    Logger.debug(LOG_TAG, "HTTP PATCH " + this.uri.toASCIIString());
+    HttpPatch request = new HttpPatch(this.uri);
+    request.setEntity(body);
+    this.go(request);
+  }
+
+  @Override
   public void put(HttpEntity body) {
     Logger.debug(LOG_TAG, "HTTP PUT " + this.uri.toASCIIString());
     HttpPut request = new HttpPut(this.uri);
     request.setEntity(body);
     this.go(request);
   }
 
   protected static StringEntity stringEntityWithContentTypeApplicationJSON(String s) {
@@ -458,9 +467,21 @@ public class BaseResource implements Res
 
   public void post(ExtendedJSONObject o) {
     post(jsonEntity(o));
   }
 
   public void post(JSONObject jsonObject) throws UnsupportedEncodingException {
     post(jsonEntity(jsonObject));
   }
+
+  public void patch(JSONArray jsonArray) throws UnsupportedEncodingException {
+    patch(jsonEntity(jsonArray));
+  }
+
+  public void patch(ExtendedJSONObject o) {
+    patch(jsonEntity(o));
+  }
+
+  public void patch(JSONObject jsonObject) throws UnsupportedEncodingException {
+    patch(jsonEntity(jsonObject));
+  }
 }
--- a/mobile/android/base/sync/net/Resource.java
+++ b/mobile/android/base/sync/net/Resource.java
@@ -10,10 +10,11 @@ import ch.boye.httpclientandroidlib.Http
 
 public interface Resource {
   public abstract URI getURI();
   public abstract String getURIString();
   public abstract String getHostname();
   public abstract void get();
   public abstract void delete();
   public abstract void post(HttpEntity body);
+  public abstract void patch(HttpEntity body);
   public abstract void put(HttpEntity body);
 }
--- a/mobile/android/base/sync/net/SyncStorageRequest.java
+++ b/mobile/android/base/sync/net/SyncStorageRequest.java
@@ -186,12 +186,17 @@ public class SyncStorageRequest implemen
   }
 
   @Override
   public void post(HttpEntity body) {
     this.resource.post(body);
   }
 
   @Override
+  public void patch(HttpEntity body) {
+    this.resource.patch(body);
+  }
+
+  @Override
   public void put(HttpEntity body) {
     this.resource.put(body);
   }
 }