From 26430d117bd05a29aa5848d37950d655596172d5 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 4 Mar 2013 18:30:38 -0800 Subject: [PATCH] Review of PR #716 (multi-cursor publish). - History.md update - tweak docs - refactor the "one per collection" check - make errors into internal errors. Programming errors like returning the wrong type from a function on the server don't need to be reported to the client. --- History.md | 3 ++ docs/client/api.html | 15 ++++-- packages/livedata/livedata_server.js | 61 ++++++++++++---------- packages/livedata/livedata_test_service.js | 12 +++-- packages/livedata/livedata_tests.js | 6 +-- packages/mongo-livedata/mongo_driver.js | 9 ++-- 6 files changed, 65 insertions(+), 41 deletions(-) diff --git a/History.md b/History.md index 7df60f3002..36ee74e686 100644 --- a/History.md +++ b/History.md @@ -1,6 +1,9 @@ ## vNEXT +* Publish functions may now return an array of cursors to publish. Currently, + the cursors must all be from different collections. + * User documents have id's when onCreateUser and validateNewUser hooks run. * Removed all restrictions on EJSON types in MongoDB, even user-defined ones. diff --git a/docs/client/api.html b/docs/client/api.html index 585c988a99..6a261a76a9 100644 --- a/docs/client/api.html +++ b/docs/client/api.html @@ -43,10 +43,17 @@ To publish records to clients, call `Meteor.publish` on the server with two parameters: the name of the record set, and a *publish function* that Meteor will call each time a client subscribes to the name. -Publish functions can return an instance or array of +Publish functions can return a [`Collection.Cursor`](#meteor_collection_cursor), in which case Meteor -will publish that cursor's documents. You can return only one cursor -for each collection when returning an array of cursors. +will publish that cursor's documents. You can also return an array of +`Collection.Cursor`s, in which case Meteor will publish all of the +cursors. + +{{#warning}} +If you return multiple cursors in an array, they currently must all be from +different collections. We hope to lift this restriction in a future release. +cursors. +{{/warning}} // server: publish the rooms collection, minus secret info. Meteor.publish("rooms", function () { @@ -61,7 +68,7 @@ for each collection when returning an array of cursors. }); // publish dependent documents and simulate joins - Meteor.publish("room", function (roomId) { + Meteor.publish("roomAndMessages", function (roomId) { return [ Rooms.find({_id: roomId}, {fields: {secretInfo: 0}}), Messages.find({roomId: roomId}) diff --git a/packages/livedata/livedata_server.js b/packages/livedata/livedata_server.js index dec23874eb..901dac065a 100644 --- a/packages/livedata/livedata_server.js +++ b/packages/livedata/livedata_server.js @@ -819,10 +819,11 @@ _.extend(Meteor._LivedataSubscription.prototype, { return; } - // SPECIAL CASE: Instead of writing their own callbacks that invoke + // SPECIAL CASE: Instead of writing their own callbacks that invoke // this.added/changed/ready/etc, the user can just return a collection - // cursor or array of cursors from the publish function; we call its _publishCursor method which - // starts observing the cursor and publishes the results. + // cursor or array of cursors from the publish function; we call their + // _publishCursor method which starts observing the cursor and publishes the + // results. Note that _publishCursor does NOT call ready(). // // XXX This uses an undocumented interface which only the Mongo cursor // interface publishes. Should we make this interface public and encourage @@ -834,33 +835,39 @@ _.extend(Meteor._LivedataSubscription.prototype, { // reactiveThingy.publishMe(); // }); // }; - if (res) { - if (res._publishCursor) { - res._publishCursor(self); - // _publishCursor only returns after the initial added callbacks have run. - // mark subscription as ready. - self.ready(); - } else if (_.isArray(res)) { - // check all the elements are cursors - if (! _.every(res, function (cur) { return cur && cur._publishCursor; })) { - self.error(new Meteor.Error(500, "Pulish function returned an array of non-Cursors")); + var isCursor = function (c) { + return c && c._publishCursor; + }; + if (isCursor(res)) { + res._publishCursor(self); + // _publishCursor only returns after the initial added callbacks have run. + // mark subscription as ready. + self.ready(); + } else if (_.isArray(res)) { + // check all the elements are cursors + if (! _.all(res, isCursor)) { + self.error(new Error("Publish function returned an array of non-Cursors")); + return; + } + // find duplicate collection names + // XXX we should support overlapping cursors, but that would require the + // merge box to allow overlap within a subscription + var collectionNames = {}; + for (var i = 0; i < res.length; ++i) { + var collectionName = res[i]._getCollectionName(); + if (_.has(collectionNames, collectionName)) { + self.error(new Error( + "Publish function returned multiple cursors for collection " + + collectionName)); return; } - // find duplicate collection names - var dups = []; - _.each(_.countBy(res, function (cur) { return cur._getCollectionName(); }), - function (count, col) { - count > 1 && dups.push(col); - }); + collectionNames[collectionName] = true; + }; - if (dups.length === 0) { // no duplicates - _.each(res, function (cur) { - cur._publishCursor(self); - }); - self.ready(); - } else - self.error(new Meteor.Error(500, "Publish function returned multiple cursors for one collection", dups)); - } + _.each(res, function (cur) { + cur._publishCursor(self); + }); + self.ready(); } }, diff --git a/packages/livedata/livedata_test_service.js b/packages/livedata/livedata_test_service.js index edb1133a49..213282a92e 100644 --- a/packages/livedata/livedata_test_service.js +++ b/packages/livedata/livedata_test_service.js @@ -294,24 +294,28 @@ if (Meteor.isServer) { Two.insert({name: "value5"}); Meteor.publish("multiPublish", function (options) { - if (options.normal) + if (options.normal) { return [ One.find(), Two.find() ]; - else if (options.dup) + } else if (options.dup) { + // Suppress the log of the expected internal error. + Meteor._suppress_log(1); return [ One.find(), One.find({name: "value2"}), // multiple cursors for one collection - error Two.find() ]; - else if (options.notCursor) + } else if (options.notCursor) { + // Suppress the log of the expected internal error. + Meteor._suppress_log(1); return [ One.find(), "not a cursor", Two.find() ]; - else + } else throw "unexpected options"; }); } diff --git a/packages/livedata/livedata_tests.js b/packages/livedata/livedata_tests.js index 38ee3e9f61..02a8f9a6a4 100644 --- a/packages/livedata/livedata_tests.js +++ b/packages/livedata/livedata_tests.js @@ -521,15 +521,15 @@ if (Meteor.isClient) { function (test, expect) { Meteor.subscribe("multiPublish", {dup: 1}, { onReady: failure(), - onError: expect(failure(test, 500, "Publish function returned multiple cursors for one collection")) + onError: expect(failure(test, 500, "Internal server error")) }); }, function (test, expect) { Meteor.subscribe("multiPublish", {notCursor: 1}, { onReady: failure(), - onError: expect(failure(test, 500, "Pulish function returned an array of non-Cursors")) + onError: expect(failure(test, 500, "Internal server error")) }); - }, + } ]); } diff --git a/packages/mongo-livedata/mongo_driver.js b/packages/mongo-livedata/mongo_driver.js index 3aa4f4dd08..dd8c1403a2 100644 --- a/packages/mongo-livedata/mongo_driver.js +++ b/packages/mongo-livedata/mongo_driver.js @@ -398,13 +398,16 @@ Cursor.prototype._publishCursor = function (sub) { } }); + // We don't call sub.ready() here: it gets called in livedata_server, after + // possibly calling _publishCursor on multiple returned cursors. + // register stop callback (expects lambda w/ no args). sub.onStop(function () {observeHandle.stop();}); }; -// When you call Meteor.publish() with a function that returns an array of Cursor, we need -// to check uniqueness of Cursor for each collection in that array. We can accomplish it through -// Cursor collection name comparison. +// Used to guarantee that publish functions return at most one cursor per +// collection. Private, because we might later have cursors that include +// documents from multiple collections somehow. Cursor.prototype._getCollectionName = function () { var self = this; return self._cursorDescription.collectionName;