readSnapshots rejection - More code review updates

Author: ericyhwang

lib/agent.js

@@ -465,21 +465,18 @@ function getResultsData(results) {
 
 function getMapResult(snapshotMap) {
   var data = {};
-  var errorMap = {};
-  var hasSnapshotError = false;
   for (var id in snapshotMap) {
     var mapValue = snapshotMap[id];
     // fetchBulk / subscribeBulk map data can have either a Snapshot or an object
     // `{error: Error | string}` as a value.
     if (mapValue.error) {
-      hasSnapshotError = true;
       // Transform errors to serialization-friendly objects.
-      errorMap[id] = getReplyErrorObject(mapValue.error);
+      data[id] = {error: getReplyErrorObject(mapValue.error)};
     } else {
       data[id] = getSnapshotData(mapValue);
     }
   }
-  return hasSnapshotError ? {data: data, errorMap: errorMap} : {data: data};
+  return {data: data};
 }
 
 function getSnapshotData(snapshot) {

lib/client/connection.js

@@ -275,13 +275,18 @@ function wrapErrorData(errorData, fullMessage) {
 Connection.prototype._handleBulkMessage = function(err, message, method) {
   if (message.data) {
     for (var id in message.data) {
+      var dataForId = message.data[id];
       var doc = this.getExisting(message.c, id);
-      if (doc) doc[method](err, message.data[id]);
-    }
-    // Handle doc-specific errors.
-    for (var id in message.errorMap) {
-      var doc = this.getExisting(message.c, id);
-      if (doc) doc[method](wrapErrorData(message.errorMap[id]));
+      if (doc) {
+        if (err) {
+          doc[method](err);
+        } else if (dataForId.error) {
+          // Bulk reply snapshot-specific errorr - see agent.js getMapResult
+          doc[method](wrapErrorData(dataForId.error));
+        } else {
+          doc[method](null, dataForId);
+        }
+      }
     }
   } else if (Array.isArray(message.b)) {
     for (var i = 0; i < message.b.length; i++) {

lib/read-snapshots-request.js

@@ -32,10 +32,12 @@ function ReadSnapshotsRequest(collection, snapshots, snapshotType) {
  *
  * If the error has a `code` property of `"ERR_SNAPSHOT_READ_SILENT_REJECTION"`, then the Share
  * client will not pass the error to user code, but will still do things like cancel subscriptions.
+ * The `#rejectSnapshotReadSilent(snapshot, errorMessage)` method can also be used for convenience.
  *
  * @param {Snapshot} snapshot
  * @param {string | Error} error
  *
+ * @see #rejectSnapshotReadSilent
  * @see ShareDBError.CODES.ERR_SNAPSHOT_READ_SILENT_REJECTION
  * @see ShareDBError.CODES.ERR_SNAPSHOT_READS_REJECTED
  */
@@ -46,6 +48,24 @@ ReadSnapshotsRequest.prototype.rejectSnapshotRead = function(snapshot, error) {
   this._idToError[snapshot.id] = error;
 };
 
+/**
+ * Rejects the read of a specific snapshot. A rejected snapshot read will not have that snapshot's
+ * data sent down to the client.
+ *
+ * This method will set a special error code that causes the Share client to not pass the error to
+ * user code, though it will still do things like cancel subscriptions.
+ *
+ * @param {Snapshot} snapshot
+ * @param {string} errorMessage
+ */
+ReadSnapshotsRequest.prototype.rejectSnapshotReadSilent = function(snapshot, errorMessage) {
+  this.rejectSnapshotRead(snapshot, this.silentRejectionError(errorMessage));
+};
+
+ReadSnapshotsRequest.prototype.silentRejectionError = function(errorMessage) {
+  return new ShareDBError(ShareDBError.CODES.ERR_SNAPSHOT_READ_SILENT_REJECTION, errorMessage);
+};
+
 /**
  * Returns whether this trigger of "readSnapshots" has had a snapshot read rejected.
  */

test/client/subscribe.js

@@ -1,6 +1,5 @@
 var expect = require('chai').expect;
 var async = require('async');
-var ERROR_CODES = require('../../lib/error').CODES;
 
 module.exports = function() {
   describe('client subscribe', function() {
@@ -47,7 +46,7 @@ module.exports = function() {
       });
 
       function testSingleSnapshotSpecificError(label, fns) {
-        var createReadSnapshotsError = fns.createReadSnapshotsError;
+        var rejectSnapshot = fns.rejectSnapshot;
         var verifyClientError = fns.verifyClientError;
         it(method + ' single with readSnapshots rejectSnapshotRead ' + label, function(done) {
           var backend = this.backend;
@@ -59,7 +58,7 @@ module.exports = function() {
             backend.use('readSnapshots', function(context, cb) {
               expect(context.snapshots).to.be.an('array').of.length(1);
               expect(context.snapshots[0]).to.have.property('id', 'fido');
-              context.rejectSnapshotRead(context.snapshots[0], createReadSnapshotsError());
+              rejectSnapshot(context, context.snapshots[0]);
               cb();
             });
 
@@ -94,18 +93,16 @@ module.exports = function() {
         });
       }
       testSingleSnapshotSpecificError('normal error', {
-        createReadSnapshotsError: function() {
-          return new Error('Failed to fetch fido');
+        rejectSnapshot: function(context, snapshot) {
+          context.rejectSnapshotRead(snapshot, new Error('Failed to fetch fido'));
         },
         verifyClientError: function(err) {
           expect(err).to.be.an('error').with.property('message', 'Failed to fetch fido');
         }
       });
-      testSingleSnapshotSpecificError('special ignorable error', {
-        createReadSnapshotsError: function() {
-          var err = new Error('Failed to fetch fido');
-          err.code = ERROR_CODES.ERR_SNAPSHOT_READ_SILENT_REJECTION;
-          return err;
+      testSingleSnapshotSpecificError('silent error', {
+        rejectSnapshot: function(context, snapshot) {
+          context.rejectSnapshotReadSilent(snapshot, 'Failed to fetch fido');
         },
         verifyClientError: function(err) {
           expect(err).to.equal(undefined);
@@ -222,7 +219,7 @@ module.exports = function() {
       });
 
       function testBulkSnapshotSpecificError(label, fns) {
-        var createReadSnapshotsError = fns.createReadSnapshotsError;
+        var rejectSnapshot = fns.rejectSnapshot;
         var verifyClientError = fns.verifyClientError;
         it(method + ' bulk with readSnapshots rejectSnapshotRead ' + label, function(done) {
           var backend = this.backend;
@@ -241,7 +238,7 @@ module.exports = function() {
             backend.use('readSnapshots', function(context, cb) {
               expect(context.snapshots).to.be.an('array').of.length(2);
               expect(context.snapshots[0]).to.have.property('id', 'fido');
-              context.rejectSnapshotRead(context.snapshots[0], createReadSnapshotsError());
+              rejectSnapshot(context, context.snapshots[0]);
               cb();
             });
 
@@ -294,18 +291,16 @@ module.exports = function() {
         });
       }
       testBulkSnapshotSpecificError('normal error', {
-        createReadSnapshotsError: function() {
-          return new Error('Failed to fetch fido');
+        rejectSnapshot: function(context, snapshot) {
+          context.rejectSnapshotRead(snapshot, new Error('Failed to fetch fido'));
         },
         verifyClientError: function(err) {
           expect(err).to.be.an('error').with.property('message', 'Failed to fetch fido');
         }
       });
       testBulkSnapshotSpecificError('special ignorable error', {
-        createReadSnapshotsError: function() {
-          var err = new Error('Failed to fetch fido');
-          err.code = ERROR_CODES.ERR_SNAPSHOT_READ_SILENT_REJECTION;
-          return err;
+        rejectSnapshot: function(context, snapshot) {
+          context.rejectSnapshotReadSilent(snapshot, 'Failed to fetch fido');
         },
         verifyClientError: function(err) {
           expect(err).to.equal(undefined);