From a93567eed11da072f72d454fffa7249a6e7c97e5 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 18 Dec 2012 12:19:25 -0800 Subject: [PATCH] Change removed messages to just take a single ID. --- packages/livedata/livedata_connection.js | 26 +++---- packages/livedata/livedata_server.js | 81 ++++++++++----------- packages/livedata/livedata_tests.js | 2 +- packages/livedata/session_view_tests.js | 91 ++++++++++++------------ packages/mongo-livedata/collection.js | 15 ++-- packages/mongo-livedata/mongo_driver.js | 2 +- 6 files changed, 102 insertions(+), 115 deletions(-) diff --git a/packages/livedata/livedata_connection.js b/packages/livedata/livedata_connection.js index 09556b21b2..697a5181dd 100644 --- a/packages/livedata/livedata_connection.js +++ b/packages/livedata/livedata_connection.js @@ -1002,26 +1002,22 @@ _.extend(Meteor._LivedataConnection.prototype, { _process_removed: function (msg, updates) { var self = this; - var idsRemovedNow = []; - _.each(msg.ids, function (removedId) { - var serverDoc = Meteor._get(self._serverDocuments, msg.collection, removedId); - if (serverDoc) { - // Some outstanding stub wrote here. - if (serverDoc.document === undefined) { - throw new Error("It doesn't make sense to be deleting something we don't know exists: " - + removedId); - } - serverDoc.document = undefined; - } else { - idsRemovedNow.push(removedId); + var serverDoc = Meteor._get( + self._serverDocuments, msg.collection, msg.id); + if (serverDoc) { + // Some outstanding stub wrote here. + if (serverDoc.document === undefined) { + throw new Error("It doesn't make sense to be deleting something we don't know exists: " + + msg.id); } - }); - if (!_.isEmpty(idsRemovedNow)) + serverDoc.document = undefined; + } else { self._pushUpdate(updates, msg.collection, { msg: 'removed', collection: msg.collection, - ids: idsRemovedNow + id: msg.id }); + } }, _process_updated: function (msg, updates) { diff --git a/packages/livedata/livedata_server.js b/packages/livedata/livedata_server.js index b1689281c9..007125de57 100644 --- a/packages/livedata/livedata_server.js +++ b/packages/livedata/livedata_server.js @@ -124,7 +124,6 @@ _.extend(Meteor._SessionCollectionView.prototype, { diff: function (previous) { var self = this; - var removedIds = []; diffObjects(previous.documents, self.documents, { both: _.bind(self.diffDocument, self), @@ -133,11 +132,9 @@ _.extend(Meteor._SessionCollectionView.prototype, { }, leftOnly: function (id, prevDV) { - removedIds.push(id); + self.callbacks.removed(self.collectionName, id); } }); - if (!_.isEmpty(removedIds)) - self.callbacks.removed(self.collectionName, removedIds); }, diffDocument: function (id, prevDV, nowDV) { @@ -195,33 +192,28 @@ _.extend(Meteor._SessionCollectionView.prototype, { self.callbacks.changed(self.collectionName, id, changedResult, clearedResult); }, - removed: function (subscriptionId, ids) { + removed: function (subscriptionId, id) { var self = this; - var removedIds = []; - _.each(ids, function (id) { - var docView = self.documents[id]; - if (!docView) { - throw new Error("Removed nonexistent document " + id); - } - delete docView.existsIn[subscriptionId]; - if (_.isEmpty(docView.existsIn)) { - // it is gone from everyone - removedIds.push(id); - delete self.documents[id]; - } else { - var changed = {}; - var cleared = []; - // remove this subscription from every precedence list - // and record the changes - _.each(docView.dataByKey, function (precedenceList, key) { - docView.clearField(subscriptionId, key, changed, cleared); - }); + var docView = self.documents[id]; + if (!docView) { + throw new Error("Removed nonexistent document " + id); + } + delete docView.existsIn[subscriptionId]; + if (_.isEmpty(docView.existsIn)) { + // it is gone from everyone + self.callbacks.removed(self.collectionName, id); + delete self.documents[id]; + } else { + var changed = {}; + var cleared = []; + // remove this subscription from every precedence list + // and record the changes + _.each(docView.dataByKey, function (precedenceList, key) { + docView.clearField(subscriptionId, key, changed, cleared); + }); - self.callbacks.changed(self.collectionName, id, changed, cleared); - } - }); - if (!_.isEmpty(removedIds)) - self.callbacks.removed(self.collectionName, removedIds); + self.callbacks.changed(self.collectionName, id, changed, cleared); + } } }); /******************************************************************************/ @@ -301,10 +293,10 @@ _.extend(Meteor._LivedataSession.prototype, { } }, - sendRemoved: function (collectionName, ids) { + sendRemoved: function (collectionName, id) { var self = this; if (self._isSending) - self.send({msg: "removed", collection: collectionName, ids: ids}); + self.send({msg: "removed", collection: collectionName, id: id}); }, getSendCallbacks: function () { @@ -333,10 +325,10 @@ _.extend(Meteor._LivedataSession.prototype, { view.added(subscriptionId, id, fields); }, - removed: function (subscriptionId, collectionName, ids) { + removed: function (subscriptionId, collectionName, id) { var self = this; var view = self.getCollectionView(collectionName); - view.removed(subscriptionId, ids); + view.removed(subscriptionId, id); if (view.isEmpty()) { delete self.collectionViews[collectionName]; } @@ -653,7 +645,9 @@ _.extend(Meteor._LivedataSession.prototype, { }); }, leftOnly: function (collectionName, leftValue) { - self.sendRemoved(collectionName, _.keys(leftValue.documents)); + _.each(leftValue.documents, function (doc, id) { + self.sendRemoved(collectionName, id); + }); } }); }, @@ -829,15 +823,12 @@ _.extend(Meteor._LivedataSubscription.prototype, { self._session.changed(self._subscriptionId, collectionName, id, fields); }, - removed: function (collectionName, ids) { + removed: function (collectionName, id) { var self = this; - _.each(ids, function(id) { - // we don't bother to delete sets of things in a collection if the - // collection is empty. It could break below, where we iterate over - // it removing items. - delete self._documents[collectionName][id]; - }); - self._session.removed(self._subscriptionId, collectionName, ids); + // We don't bother to delete sets of things in a collection if the + // collection is empty. It could break _removeAllDocuments. + delete self._documents[collectionName][id]; + self._session.removed(self._subscriptionId, collectionName, id); }, complete: function () { @@ -861,7 +852,11 @@ _.extend(Meteor._LivedataSubscription.prototype, { _removeAllDocuments: function () { var self = this; _.each(self._documents, function(collectionDocs, collectionName) { - self.removed(collectionName, _.keys(collectionDocs)); + // Iterate over _.keys instead of the dictionary itself, since we'll be + // mutating it. + _.each(_.keys(collectionDocs), function (id) { + self.removed(collectionName, id); + }); }); } diff --git a/packages/livedata/livedata_tests.js b/packages/livedata/livedata_tests.js index 662abd8ab5..817a54d719 100644 --- a/packages/livedata/livedata_tests.js +++ b/packages/livedata/livedata_tests.js @@ -317,7 +317,7 @@ if (Meteor.isClient) { if (msg.msg === 'added') ++actualAddedMessageCount; else if (msg.msg === 'removed') - actualRemovedMessageCount += msg.ids.length; + ++actualRemovedMessageCount; else test.fail({unexpected: JSON.stringify(msg)}); }); diff --git a/packages/livedata/session_view_tests.js b/packages/livedata/session_view_tests.js index bbee21c81f..e46fef0dc0 100644 --- a/packages/livedata/session_view_tests.js +++ b/packages/livedata/session_view_tests.js @@ -9,8 +9,8 @@ var newView = function(test) { return; results.push({fun: 'changed', id: id, changed: changed, cleared: cleared}); }, - removed: function (collection, ids) { - results.push({fun: 'removed', ids: ids}); + removed: function (collection, id) { + results.push({fun: 'removed', id: id}); } }); var v = { @@ -44,11 +44,11 @@ Tinytest.add('livedata - sessionview - exists reveal', function (test) { v.added("B", "A1", {}); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectNoResult(); - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -60,14 +60,14 @@ Tinytest.add('livedata - sessionview - field reveal', function (test) { v.expectNoResult(); v.added("B", "A1", {foo: "baz"}); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectResult({fun: 'changed', id: "A1", changed: {foo: "baz"}, cleared: []}); v.expectNoResult(); // Somewhere in here we must have changed foo to baz. Legal either on the // added or on the removed, but only once. - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -82,8 +82,8 @@ Tinytest.add('livedata - sessionview - field change', function (test) { v.expectResult({fun: 'changed', id: "A1", changed: {foo: "baz"}, cleared: []}); v.expectNoResult(); - v.removed("A", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("A", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -98,8 +98,8 @@ Tinytest.add('livedata - sessionview - field clear', function (test) { v.expectResult({fun: 'changed', id: "A1", changed: {}, cleared: ["foo"]}); v.expectNoResult(); - v.removed("A", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("A", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -122,8 +122,8 @@ Tinytest.add('livedata - sessionview - add, remove, add', function (test) { v.expectResult({fun: 'added', id: "A1", fields: {foo: "bar"}}); v.expectNoResult(); - v.removed("A", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("A", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); v.added("A", "A1", {foo: "bar"}); @@ -145,10 +145,10 @@ Tinytest.add('livedata - sessionview - field clear reveal', function (test) { v.expectResult({fun: 'changed', id: "A1", changed: {foo: "baz"}, cleared: []}); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectNoResult(); - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -174,10 +174,10 @@ Tinytest.add('livedata - sessionview - change to canonical value produces no cha v.changed("B", "A1", {foo: canon}, []); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectNoResult(); - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -194,10 +194,10 @@ Tinytest.add('livedata - sessionview - new field of canonical value produces no v.changed("B", "A1", {foo: "bar"}, []); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectNoResult(); - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -216,9 +216,9 @@ Tinytest.add('livedata - sessionview - clear all clears only once', function (te v.expectResult({fun: 'changed', id: "A1", changed: {}, cleared: ["foo"]}); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectNoResult(); - v.removed("B", ["A1"]); + v.removed("B", "A1"); v.expectNoResult(); }); @@ -237,9 +237,9 @@ Tinytest.add('livedata - sessionview - change all changes only once', function ( v.expectResult({fun: 'changed', id: "A1", changed: {foo: "baz"}, cleared: []}); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectNoResult(); - v.removed("B", ["A1"]); + v.removed("B", "A1"); v.expectNoResult(); }); @@ -256,11 +256,11 @@ Tinytest.add('livedata - sessionview - multiple operations at once in a change', v.expectResult({fun: 'changed', id: "A1", changed: {foo: "baz", thing: "stuff"}, cleared: ["baz"]}); v.expectNoResult(); - v.removed("A", ["A1"]); + v.removed("A", "A1"); v.expectResult({fun: 'changed', id: "A1", changed: {}, cleared: ["thing"]}); v.expectNoResult(); - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -278,16 +278,16 @@ Tinytest.add('livedata - sessionview - more than one document', function (test) v.expectResult({fun: 'changed', id: "A1", changed: {thing: "stuff"}, cleared: ["foo", "baz"]}); v.expectNoResult(); - v.removed("A", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("A", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); - v.removed("A", ["A2"]); - v.expectResult({fun: 'removed', ids: ["A2"]}); + v.removed("A", "A2"); + v.expectResult({fun: 'removed', id: "A2"}); v.expectNoResult(); }); -Tinytest.add('livedata - sessionview - multiple docs removed at once', function (test) { +Tinytest.add('livedata - sessionview - multiple docs removed', function (test) { var v = newView(test); v.added("A", "A1", {foo: "bar", baz: "quux"}); @@ -299,8 +299,10 @@ Tinytest.add('livedata - sessionview - multiple docs removed at once', function v.expectResult({fun: 'added', id: "A2", fields: {foo: "baz"}}); v.expectNoResult(); - v.removed("A", ["A1", "A2"]); - v.expectResult({fun: 'removed', ids: ["A1", "A2"]}); + v.removed("A", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); + v.removed("A", "A2"); + v.expectResult({fun: 'removed', id: "A2"}); v.expectNoResult(); }); @@ -320,12 +322,13 @@ Tinytest.add('livedata - sessionview - complicated sequence', function (test) { v.expectResult({fun: 'changed', id: "A1", changed: {foo: "baz", thing: "stuff"}, cleared: ["baz"]}); v.expectNoResult(); - v.removed("A", ["A1", "A2"]); + v.removed("A", "A1"); + v.removed("A", "A2"); v.expectResult({fun: 'changed', id: "A1", changed: {}, cleared: ["thing"]}); - v.expectResult({fun: 'removed', ids: ["A2"]}); + v.expectResult({fun: 'removed', id: "A2"}); v.expectNoResult(); - v.removed("B", ["A1"]); - v.expectResult({fun: 'removed', ids: ["A1"]}); + v.removed("B", "A1"); + v.expectResult({fun: 'removed', id: "A1"}); v.expectNoResult(); }); @@ -339,11 +342,11 @@ Tinytest.add('livedata - sessionview - added becomes changed', function (test) { v.expectResult({fun: 'changed', id: 'A1', changed: {hi: 'there'}, cleared: []}); - v.removed('A', ['A1']); + v.removed('A', 'A1'); v.expectResult({fun: 'changed', id: 'A1', changed: {}, cleared: ['foo']}); - v.removed('B', ['A1']); - v.expectResult({fun: 'removed', ids: ['A1']}); + v.removed('B', 'A1'); + v.expectResult({fun: 'removed', id: 'A1'}); }); Tinytest.add('livedata - sessionview - weird key names', function (test) { diff --git a/packages/mongo-livedata/collection.js b/packages/mongo-livedata/collection.js index 4968445794..44bf2b5a40 100644 --- a/packages/mongo-livedata/collection.js +++ b/packages/mongo-livedata/collection.js @@ -68,17 +68,6 @@ Meteor.Collection = function (name, options) { // Apply an update. // XXX better specify this interface (not in terms of a wire message)? update: function (msg) { - // Handle removed separately, since it's the only one without an 'id' - // field. - if (msg.msg === 'removed') { - _.each(msg.ids, function (removedId) { - if (!self._collection.findOne(removedId)) - throw new Error("Expected to find a document to remove"); - self._collection.remove(removedId); - }); - return; - } - var doc = self._collection.findOne(msg.id); // Is this a "replace the whole doc" message coming from the quiescence @@ -100,6 +89,10 @@ Meteor.Collection = function (name, options) { if (doc) throw new Error("Expected not to find a document already present for an add"); self._collection.insert(_.extend({_id: msg.id}, msg.fields)); + } else if (msg.msg === 'removed') { + if (!doc) + throw new Error("Expected to find a document already present for removed"); + self._collection.remove(msg.id); } else if (msg.msg === 'changed') { if (!doc) throw new Error("Expected to find a document to change"); diff --git a/packages/mongo-livedata/mongo_driver.js b/packages/mongo-livedata/mongo_driver.js index 9b749914ec..67a13a98c1 100644 --- a/packages/mongo-livedata/mongo_driver.js +++ b/packages/mongo-livedata/mongo_driver.js @@ -311,7 +311,7 @@ Cursor.prototype._publishCursor = function (sub) { sub.changed(collection, obj._id, fields); }, removed: function (oldObj) { - sub.removed(collection, [oldObj._id]); + sub.removed(collection, oldObj._id); } });