From e9913d2055ec450236701ac48161c38d801412ae Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 17 Dec 2012 23:39:06 -0800 Subject: [PATCH] Fix to skip/limit observe: update would never produce 'changed' calls. Also remove dead upsert code. --- packages/minimongo/minimongo.js | 86 +++++++++++++-------------- packages/minimongo/minimongo_tests.js | 2 + 2 files changed, 45 insertions(+), 43 deletions(-) diff --git a/packages/minimongo/minimongo.js b/packages/minimongo/minimongo.js index 0e328253c07..35dddf1a1f5 100644 --- a/packages/minimongo/minimongo.js +++ b/packages/minimongo/minimongo.js @@ -204,8 +204,7 @@ _.extend(LocalCollection.Cursor.prototype, { sort_f: ordered && self.sort_f, results_snapshot: null, ordered: ordered, - cursor: this, - needsRecompute: false + cursor: this }; query.results = self._getRawObjects(ordered); if (self.collection.paused) @@ -351,24 +350,30 @@ LocalCollection.prototype.insert = function (doc) { self._saveOriginal(doc._id, undefined); self.docs[doc._id] = doc; + var queriesToRecompute = []; + // trigger live queries that match for (var qid in self.queries) { var query = self.queries[qid]; if (query.selector_f(doc)) { if (query.cursor.skip || query.cursor.limit) - query.needsRecompute = true; + queriesToRecompute.push(query); else LocalCollection._insertInResults(query, doc); } } - self._recomputeNecessaryQueries(); + _.each(queriesToRecompute, function (query) { + LocalCollection._recomputeResults(query); + }); }; LocalCollection.prototype.remove = function (selector) { var self = this; var remove = []; + var queriesToRecompute = []; + // Avoid O(n) for "remove a single doc by ID". if (LocalCollection._selectorIsId(selector)) { if (_.has(self.docs, selector)) @@ -390,7 +395,7 @@ LocalCollection.prototype.remove = function (selector) { _.each(self.queries, function (query) { if (query.selector_f(removeDoc)) { if (query.cursor.skip || query.cursor.limit) - query.needsRecompute = true; + queriesToRecompute.push(query); else queryRemove.push([query, removeDoc]); } @@ -403,8 +408,9 @@ LocalCollection.prototype.remove = function (selector) { for (var i = 0; i < queryRemove.length; i++) { LocalCollection._removeFromResults(queryRemove[i][0], queryRemove[i][1]); } - - self._recomputeNecessaryQueries(); + _.each(queriesToRecompute, function (query) { + LocalCollection._recomputeResults(query); + }); }; // XXX atomicity: if multi is true, and one modification fails, do @@ -415,33 +421,34 @@ LocalCollection.prototype.update = function (selector, mod, options) { var self = this; var any = false; var selector_f = LocalCollection._compileSelector(selector); + + var qidToOriginalResults = {}; + _.each(self.queries, function (query, qid) { + if ((query.cursor.skip || query.cursor.limit) && !query.paused) + qidToOriginalResults[qid] = LocalCollection._deepcopy(query.results); + }); + var recomputeQids = {}; + for (var id in self.docs) { var doc = self.docs[id]; if (selector_f(doc)) { // XXX Should we save the original even if mod ends up being a no-op? self._saveOriginal(id, doc); - self._modifyAndNotify(doc, mod); + self._modifyAndNotify(doc, mod, recomputeQids); if (!options.multi) break; any = true; } } - if (options.upsert) { - throw Error("upsert not yet implemented"); - if (!any) { - // XXX is this actually right? don't we have to resolve/delete $-ops or - // something like that? - var insert = LocalCollection._deepcopy(selector); - LocalCollection._modify(insert, mod); - self.insert(insert); - } - } - - self._recomputeNecessaryQueries(); + _.each(recomputeQids, function (dummy, qid) { + LocalCollection._recomputeResults(self.queries[qid], + qidToOriginalResults[qid]); + }); }; -LocalCollection.prototype._modifyAndNotify = function (doc, mod) { +LocalCollection.prototype._modifyAndNotify = function ( + doc, mod, recomputeQids) { var self = this; var matched_before = {}; @@ -450,6 +457,8 @@ LocalCollection.prototype._modifyAndNotify = function (doc, mod) { if (query.ordered) { matched_before[qid] = query.selector_f(doc); } else { + // Because we don't support skip or limit (yet) in unordered queries, we + // can just do a direct lookup. matched_before[qid] = _.has(query.results, doc._id); } } @@ -464,15 +473,7 @@ LocalCollection.prototype._modifyAndNotify = function (doc, mod) { var after = query.selector_f(doc); if (query.cursor.skip || query.cursor.limit) { - // We need to recompute any query where the doc may have been in the - // cursor's window either before or after the update. (Note that if skip - // or limit is set, "before" and "after" being true do not necessarily - // mean that the document is in the cursor's output after skip/limit is - // applied... but if they are false, then the document definitely is NOT - // in the output. So it's safe to skip recompute if neither before or - // after are true.) - if (before || after) - query.needsRecompute = true; + recomputeQids[qid] = true; } else if (before && !after) { LocalCollection._removeFromResults(query, doc); } else if (!before && after) { @@ -563,23 +564,22 @@ LocalCollection._updateInResults = function (query, doc, old_doc) { query.moved(LocalCollection._deepcopy(doc), orig_idx, new_idx); }; -LocalCollection.prototype._recomputeNecessaryQueries = function () { - var self = this; - _.each(self.queries, function (query) { - if (query.needsRecompute) { - LocalCollection._recomputeResults(query); - query.needsRecompute = false; - } - }); -}; - -LocalCollection._recomputeResults = function (query) { - var old_results = query.results; +// Recomputes the results of a query and runs observe callbacks for the +// difference between the previous results and the current results (unless +// paused). Used for skip/limit queries. +// +// When this is used by insert or remove, it can just use query.results for the +// old results (and there's no need to pass in oldResults), because these +// operations don't mutate the documents in the collection. Update needs to pass +// in an oldResults which was deep-copied before the modifier was applied. +LocalCollection._recomputeResults = function (query, oldResults) { + if (!oldResults) + oldResults = query.results; query.results = query.cursor._getRawObjects(query.ordered); if (!query.paused) LocalCollection._diffQuery( - query.ordered, old_results, query.results, query, true); + query.ordered, oldResults, query.results, query, true); }; diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index 92ce8e69f4a..706907c6e99 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1219,6 +1219,8 @@ Tinytest.add("minimongo - observe ordered", function (test) { c.update({a:0}, {a:5}); test.equal(operations.shift(), ['removed', id, 0, {a:2}]); test.equal(operations.shift(), ['added', {a:4}, 1]); + c.update({a:3}, {a:3.5}); + test.equal(operations.shift(), ['changed', {a:3.5}, 0, {a:3}]); handle.stop(); });