Avoid redownloading our own changes (fixes #144)

src/collection.js

@@ -727,10 +727,20 @@ export default class Collection {
       lastModified: this.lastModified,
       headers: {},
     }, options);
+
+    // Optionally ignore records when pulling for changes.
+    // (avoid redownloading our own changes on last step of #sync())
+    let filters;
+    if (options.exclude) {
+      // Take the first 50 records to limit size of querystring (~2000 chars).
+      const exclude_id = options.exclude.slice(0, 50).map((r) => r.id).join(",");
+      filters = {exclude_id};
+    }
     // First fetch remote changes from the server
     return this._apiCollection.listRecords({
       since: options.lastModified || undefined,
-      headers: options.headers
+      headers: options.headers,
+      filters
     })
     .then(({data, last_modified}) => {
       // last_modified is the ETag header value (string).
@@ -745,7 +755,7 @@ export default class Collection {
       const localSynced = options.lastModified;
       const serverChanged = unquoted > options.lastModified;
       const emptyCollection = data.length === 0;
-      if (localSynced && serverChanged && emptyCollection) {
+      if (!options.exclude && localSynced && serverChanged && emptyCollection) {
         throw Error("Server has been flushed.");
       }
 
@@ -820,25 +830,29 @@ export default class Collection {
         const conflicts = synced.conflicts.map((c) => {
           return {type: c.type, local: c.local.data, remote: c.remote};
         });
+        // Merge outgoing conflicts into sync result object
+        syncResultObject.add("conflicts", conflicts);
+
+        // Reflect publication results locally using the response from
+        // the batch request.
+        // For created and updated records, the last_modified coming from server
+        // will be stored locally.
         const published = synced.published.map((c) => c.data);
         const skipped = synced.skipped.map((c) => c.data);
 
-        // Merge outgoing conflicts into sync result object
-        syncResultObject.add("conflicts", conflicts);
-        // Reflect publication results locally
+        // Records that must be deleted are either deletions that were pushed
+        // to server (published) or deleted records that were never pushed (skipped).
         const missingRemotely = skipped.map(r => Object.assign({}, r, {deleted: true}));
         const toApplyLocally = published.concat(missingRemotely);
-        // Deleted records are distributed accross local and missing records
-        // XXX: When tackling the issue to avoid downloading our own changes
-        // from the server. `toDeleteLocally` should be obtained from local db.
-        // See https://github.com/Kinto/kinto.js/issues/144
+
         const toDeleteLocally = toApplyLocally.filter((r) => r.deleted);
         const toUpdateLocally = toApplyLocally.filter((r) => !r.deleted);
+
         // First, apply the decode transformers, if any
         return Promise.all(toUpdateLocally.map(record => {
           return this._decodeRecord("remote", record);
         }))
-          // Process everything within a single transaction
+          // Process everything within a single transaction.
           .then((results) => {
             return this.db.execute((transaction) => {
               const updated = results.map((record) => {
@@ -965,7 +979,9 @@ export default class Collection {
         if (result.published.length === 0) {
           return result;
         }
-        return this.pullChanges(result, options);
+        // Avoid redownloading our own changes during the last pull.
+        const pullOpts = Object.assign({}, options, {exclude: result.published});
+        return this.pullChanges(result, pullOpts);
       });
     // Ensure API default remote is reverted if a custom one's been used
     return pFinally(syncPromise, () => this.api.remote = previousRemote);

test/collection_test.js

@@ -1190,6 +1190,7 @@ describe("Collection", () => {
             sinon.assert.calledOnce(listRecords);
             sinon.assert.calledWithExactly(listRecords, {
               since: undefined,
+              filters: undefined,
               headers: {}
             });
           });
@@ -1201,6 +1202,20 @@ describe("Collection", () => {
             sinon.assert.calledOnce(listRecords);
             sinon.assert.calledWithExactly(listRecords, {
               since: 42,
+              filters: undefined,
+              headers: {}
+            });
+          });
+      });
+
+      it("should pass provided filters when polling changes from server", () => {
+        const exclude = [{id: 1}, {id: 2}, {id: 3}];
+        return articles.pullChanges(result, {lastModified: 42, exclude})
+          .then(_ => {
+            sinon.assert.calledOnce(listRecords);
+            sinon.assert.calledWithExactly(listRecords, {
+              since: 42,
+              filters: {exclude_id: "1,2,3"},
               headers: {}
             });
           });
@@ -1851,6 +1866,20 @@ describe("Collection", () => {
       });
     });
 
+    it("should not redownload pushed changes", () => {
+      const record1 = {id: uuid4(), title: "blog"};
+      const record2 = {id: uuid4(), title: "post"};
+      const syncResult = new SyncResultObject();
+      syncResult.add("published", record1);
+      syncResult.add("published", record2);
+      sandbox.stub(articles, "pullChanges").returns(Promise.resolve(syncResult));
+      sandbox.stub(articles, "pushChanges").returns(Promise.resolve(syncResult));
+      return articles.sync().then(res => {
+        expect(res.published).to.have.length(2);
+        expect(articles.pullChanges.lastCall.args[1].exclude).eql([record1, record2]);
+      });
+    });
+
     describe("Options", () => {
       let pullChanges;