Bug 1140810 - Upload material (non-status) Reading List modifications. r=rnewman
authorNick Alexander <nalexander@mozilla.com>
Thu, 26 Mar 2015 15:47:20 -0700
changeset 264782 2918aa7b51ca7cb59ccf51fdc382c32228b19d94
parent 264781 47d3cee563e594e1a4ba818f43c0362e30330d8f
child 264783 08cef2ba28a035bddc5af21cfe2b8fc146642aa6
push id4718
push userraliiev@mozilla.com
push dateMon, 11 May 2015 18:39:53 +0000
treeherdermozilla-beta@c20c4ef55f08 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1140810
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 1140810 - Upload material (non-status) Reading List modifications. r=rnewman ======== https://github.com/mozilla-services/android-sync/commit/575d80fddb1afc10123b928a07800bda15c8c087 Author: Nick Alexander <nalexander@mozilla.com> Date: Wed Mar 25 16:35:26 2015 -0700 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);
   }
 }