From 9968f19f8aa47ecb1eec1bc90fe7a96164281126 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Fri, 28 Feb 2014 19:07:35 -0800 Subject: [PATCH 01/11] Failing test for sort/selector issue --- packages/minimongo/minimongo_tests.js | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index 6e95993962..0d365c72da 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1781,19 +1781,27 @@ Tinytest.add("minimongo - array sort", function (test) { // in the document, and when sorting descending, you use the maximum value you // can find. So [1, 4] shows up in the 1 slot when sorting ascending and the 4 // slot when sorting descending. - c.insert({up: 1, down: 1, a: {x: [1, 4]}}); - c.insert({up: 2, down: 2, a: [{x: [2]}, {x: 3}]}); - c.insert({up: 0, down: 4, a: {x: 0}}); - c.insert({up: 3, down: 3, a: {x: 2.5}}); - c.insert({up: 4, down: 0, a: {x: 5}}); + // + // Similarly, "selected" is the index that the doc should have in the query + // that sorts ascending on "a.x" and selects {'a.x': {$gt: 1}}. In this case, + // the 1 in [1, 4] may not be used as a sort key. + c.insert({up: 1, down: 1, selected: 2, a: {x: [1, 4]}}); + c.insert({up: 2, down: 2, selected: 0, a: [{x: [2]}, {x: 3}]}); + c.insert({up: 0, down: 4, a: {x: 0}}); + c.insert({up: 3, down: 3, selected: 1, a: {x: 2.5}}); + c.insert({up: 4, down: 0, selected: 3, a: {x: 5}}); test.equal( _.pluck(c.find({}, {sort: {'a.x': 1}}).fetch(), 'up'), - _.range(c.find().count())); + _.range(5)); test.equal( _.pluck(c.find({}, {sort: {'a.x': -1}}).fetch(), 'down'), - _.range(c.find().count())); + _.range(5)); + + test.equal( + _.pluck(c.find({'a.x': {$gt: 1}}, {sort: {'a.x': 1}}).fetch(), 'selected'), + _.range(4)); }); Tinytest.add("minimongo - sort keys", function (test) { From 8822386d7b32f5a9397d7dc23b3063dc423307af Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 16 Mar 2014 17:03:18 -0700 Subject: [PATCH 02/11] test for affectedByModifier for $elemMatch --- packages/minimongo/minimongo_server_tests.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/minimongo/minimongo_server_tests.js b/packages/minimongo/minimongo_server_tests.js index 127e30a8e6..5443659567 100644 --- a/packages/minimongo/minimongo_server_tests.js +++ b/packages/minimongo/minimongo_server_tests.js @@ -92,6 +92,8 @@ Tinytest.add("minimongo - modifier affects selector", function (test) { affected({ 'foo.bar.baz': 0 }, { $unset: { 'foo.3.bar': 1 } }, "delicate work with numeric fields in selector"); affected({ 'foo.0.bar': 0 }, { $set: { 'foo.0.0.bar': 1 } }, "delicate work with nested arrays and selectors by indecies"); + + affected({foo: {$elemMatch: {bar: 5}}}, {$set: {'foo.4.bar': 5}}, "$elemMatch"); }); Tinytest.add("minimongo - selector and projection combination", function (test) { From 17ec29603cad54c41298df2178c895428cd9f568 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 16 Mar 2014 17:22:00 -0700 Subject: [PATCH 03/11] Initial stab at fixing the sort/selector issue --- packages/minimongo/minimongo.js | 1 + packages/minimongo/selector.js | 19 ++-- packages/minimongo/sort.js | 116 ++++++++++++++++++++---- packages/mongo-livedata/mongo_driver.js | 4 + 4 files changed, 117 insertions(+), 23 deletions(-) diff --git a/packages/minimongo/minimongo.js b/packages/minimongo/minimongo.js index cc6f58d13f..c9a2c3f0e1 100644 --- a/packages/minimongo/minimongo.js +++ b/packages/minimongo/minimongo.js @@ -101,6 +101,7 @@ LocalCollection.Cursor = function (collection, selector, options) { self.matcher = new Minimongo.Matcher(selector, self); self.sorter = (self.matcher.hasGeoQuery() || options.sort) ? new Minimongo.Sorter(options.sort || []) : null; + self.sorter && self.sorter.useWithMatcher(self.matcher); } self.skip = options.skip; self.limit = options.limit; diff --git a/packages/minimongo/selector.js b/packages/minimongo/selector.js index c07e9b8664..88c7fba25a 100644 --- a/packages/minimongo/selector.js +++ b/packages/minimongo/selector.js @@ -35,7 +35,10 @@ Minimongo.Matcher = function (selector) { // Set to a dummy document which always matches this Matcher. Or set to null // if such document is too hard to find. self._matchingDocument = undefined; - // A clone of the original selector. Used by canBecomeTrueByModifier. + // A clone of the original selector. It may just be a function if the user + // passed in a function; otherwise is definitely an object (eg, IDs are + // translated into {_id: ID} first. Used by canBecomeTrueByModifier and + // Sorter.useWithMatcher. self._selector = null; self._docMatcher = self._compileSelector(selector); }; @@ -217,7 +220,7 @@ var regexpElementMatcher = function (regexp) { // Takes something that is not an operator object and returns an element matcher // for equality with that thing. -var equalityElementMatcher = function (elementSelector) { +equalityElementMatcher = function (elementSelector) { if (isOperatorObject(elementSelector)) throw Error("Can't create equalityValueSelector for operator object"); @@ -261,8 +264,6 @@ var operatorBranchedMatcher = function (valueSelector, matcher, isRoot) { VALUE_OPERATORS[operator](operand, valueSelector, matcher, isRoot)); } else if (_.has(ELEMENT_OPERATORS, operator)) { var options = ELEMENT_OPERATORS[operator]; - if (typeof options === 'function') - options = {compileElementSelector: options}; operatorMatchers.push( convertElementMatcherToBranchedMatcher( options.compileElementSelector( @@ -380,7 +381,7 @@ var VALUE_OPERATORS = { }, $nin: function (operand) { return invertBranchedMatcher(convertElementMatcherToBranchedMatcher( - ELEMENT_OPERATORS.$in(operand))); + ELEMENT_OPERATORS.$in.compileElementSelector(operand))); }, $exists: function (operand) { var exists = convertElementMatcherToBranchedMatcher(function (value) { @@ -548,7 +549,7 @@ var makeInequality = function (cmpValueComparator) { // // An element selector compiler returns a function mapping a single value to // bool. -var ELEMENT_OPERATORS = { +ELEMENT_OPERATORS = { $lt: makeInequality(function (cmpValue) { return cmpValue < 0; }), @@ -702,6 +703,12 @@ var ELEMENT_OPERATORS = { } } }; +_.each(ELEMENT_OPERATORS, function (options, operator) { + if (typeof options === 'function') { + // XXX safe to modify while iterating? + ELEMENT_OPERATORS[operator] = {compileElementSelector: options}; + } +}); // makeLookupFunction(key) returns a lookup function. // diff --git a/packages/minimongo/sort.js b/packages/minimongo/sort.js index 49f039ae21..2dafb5f7c1 100644 --- a/packages/minimongo/sort.js +++ b/packages/minimongo/sort.js @@ -59,6 +59,11 @@ Minimongo.Sorter = function (spec) { _.map(self._sortSpecParts, function (spec, i) { return self._keyFieldComparator(i); })); + + // If you call useWithMatcher on this Sorter, this may be set to a function + // which selects whether or not a given "sort key" (tuple of values for the + // different sort spec fields) is compatible with the selector. + self._keyFilter = null; }; // In addition to these methods, sorter_project.js defines combineIntoProjection @@ -106,23 +111,8 @@ _.extend(Minimongo.Sorter.prototype, { var minKey = null; self._generateKeysFromDoc(doc, function (key) { - // XXX This is actually wrong! In fact, the whole attempt to compile sort - // functions independently of selectors is wrong. In MongoDB, if you - // have documents {_id: 'x', a: [1, 10]} and {_id: 'y', a: [5, 15]}, - // then C.find({}, {sort: {a: 1}}) puts x before y (1 comes before 5). - // But C.find({a: {$gt: 3}}, {sort: {a: 1}}) puts y before x (1 does - // not match the selector, and 5 comes before 10). - // - // The way this works is pretty subtle! For example, if the documents - // are instead {_id: 'x', a: [{x: 1}, {x: 10}]}) and - // {_id: 'y', a: [{x: 5}, {x: 15}]}), - // then C.find({'a.x': {$gt: 3}}, {sort: {'a.x': 1}}) and - // C.find({a: {$elemMatch: {x: {$gt: 3}}}}, {sort: {'a.x': 1}}) - // both follow this rule (y before x). (ie, you do have to apply this - // through $elemMatch.) - // - // So we ought to skip keys here that don't work for the selector, but - // we don't do that yet. + if (self._keyFilter && !self._keyFilter(key)) + return; if (minKey === null) { minKey = key; @@ -133,6 +123,8 @@ _.extend(Minimongo.Sorter.prototype, { } }); + // This could happen if our key filter somehow filters out all the keys even + // though somehow the selector matches. if (minKey === null) throw Error("sort selector found no keys in doc?"); return minKey; @@ -281,6 +273,96 @@ _.extend(Minimongo.Sorter.prototype, { var key2 = self._getMinKeyFromDoc(doc2); return self._compareKeys(key1, key2); }; + }, + + // In MongoDB, if you have documents + // {_id: 'x', a: [1, 10]} and + // {_id: 'y', a: [5, 15]}, + // then C.find({}, {sort: {a: 1}}) puts x before y (1 comes before 5). + // But C.find({a: {$gt: 3}}, {sort: {a: 1}}) puts y before x (1 does not + // match the selector, and 5 comes before 10). + // + // The way this works is pretty subtle! For example, if the documents + // are instead {_id: 'x', a: [{x: 1}, {x: 10}]}) and + // {_id: 'y', a: [{x: 5}, {x: 15}]}), + // then C.find({'a.x': {$gt: 3}}, {sort: {'a.x': 1}}) and + // C.find({a: {$elemMatch: {x: {$gt: 3}}}}, {sort: {'a.x': 1}}) + // both follow this rule (y before x). (ie, you do have to apply this + // through $elemMatch.) + // + // So if you call `useWithMatcher(matcher)` with the Matcher that goes with + // this Sorter, we will attempt to skip sort keys that don't match the + // selector. The logic here is pretty subtle and undocumented; we've gotten as + // close as we can figure out based on our understanding of Mongo's behavior. + useWithMatcher: function (matcher) { + var self = this; + + if (self._keyFilter) + throw Error("called useWithMatcher twice?"); + + // If we are only sorting by distance, then we're not going to bother to + // build a key filter. + // XXX figure out how geoqueries interact with this stuff + if (_.isEmpty(self._sortSpecParts)) + return; + + var selector = matcher._selector; + + // If the user just passed a literal function to find(), then we can't get a + // key filter from it. + if (selector instanceof Function) + return; + + // It appears that the first sort field is treated differently from the + // others; we shouldn't create a key filter unless the first sort field is + // restricted, though after that point we can restrict the other sort fields + // or not as we wish. + var firstSortField = self._sortSpecParts[0].path; + var otherSortFields = {}; + _.each(self._sortSpecParts, function (path, i) { + if (i !== 0) + otherSortFields[path] = true; + }); + + var constraints = []; + _.each(self._sortSpecParts, function () { + constraints.push([]); + }); + + _.each(selector, function (subSelector, key) { + // XXX support constraints on non-first sort fields, $and, $or, etc. + if (key !== firstSortField) + return; + + // XXX support RegExp constraints + if (subSelector instanceof RegExp) + return; + + if (isOperatorObject(subSelector)) { + _.each(subSelector, function (operand, operator) { + if (_.contains(['$lt', '$lte', '$gt', '$gte'], operator)) { + // XXX this depends on us knowing that these operators don't use any + // of the arguments to compileElementSelector other than operand. + constraints[0].push( + ELEMENT_OPERATORS[operator].compileElementSelector(operand)); + } + // XXX support $regex/RegExp, {$exists: true}, $mod, $type, $in + }); + } + + // OK, it's an equality thing. + if (subSelector == null) { + constraints[0].push(equalityElementMatcher(subSelector)); + } + }); + + if (!_.isEmpty(constraints[0])) { + self._keyFilter = function (key) { + return _.all(constraints[0], function (f) { + return f(key[0]); + }); + }; + } } }); diff --git a/packages/mongo-livedata/mongo_driver.js b/packages/mongo-livedata/mongo_driver.js index d39653ff0f..a770d0176e 100644 --- a/packages/mongo-livedata/mongo_driver.js +++ b/packages/mongo-livedata/mongo_driver.js @@ -1032,6 +1032,10 @@ MongoConnection.prototype._observeChanges = function ( } }], function (f) { return f(); }); // invoke each function + if (matcher && sorter) { + sorter.useWithMatcher(matcher); + } + var driverClass = canUseOplog ? OplogObserveDriver : PollingObserveDriver; observeDriver = new driverClass({ cursorDescription: cursorDescription, From 125051f2f8df931badb94c004d8c54e9b0cf7f21 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 16 Mar 2014 18:39:03 -0700 Subject: [PATCH 04/11] more tests for sort/select, and fix a silly bug --- packages/minimongo/minimongo_tests.js | 52 +++++++++++++++++++++++++++ packages/minimongo/sort.js | 12 ++++--- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index 0d365c72da..311c885dad 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1867,6 +1867,58 @@ Tinytest.add("minimongo - sort keys", function (test) { {x: 2, y: [4, 5]}]}); }); +Tinytest.add("minimongo - sort key filter", function (test) { + var testOrder = function (sortSpec, selector, doc1, doc2) { + var matcher = new Minimongo.Matcher(selector); + var sorter = new Minimongo.Sorter(sortSpec); + sorter.useWithMatcher(matcher); + var comparator = sorter.getComparator(); + var comparison = comparator(doc1, doc2); + test.isTrue(comparison < 0); + }; + + testOrder({'a.x': 1}, {'a.x': {$gt: 1}}, + {a: {x: 3}}, + {a: {x: [1, 4]}}); + testOrder({'a.x': 1}, {'a.x': {$gt: 0}}, + {a: {x: [1, 4]}}, + {a: {x: 3}}); + + var keyCompatible = function (sortSpec, selector, key, compatible) { + var matcher = new Minimongo.Matcher(selector); + var sorter = new Minimongo.Sorter(sortSpec); + sorter.useWithMatcher(matcher); + var actual = sorter._keyCompatibleWithSelector(key); + test.equal(actual, compatible); + }; + + keyCompatible({a: 1}, {a: 5}, [5], true); + keyCompatible({a: 1}, {a: 5}, [8], false); + keyCompatible({a: 1}, {a: {x: 5}}, [{x: 5}], true); + keyCompatible({a: 1}, {a: {x: 5}}, [{x: 5, y: 9}], false); + keyCompatible({'a.x': 1}, {a: {x: 5}}, [5], true); + // To confirm this: + // > db.x.insert({_id: "q", a: [{x:1}, {x:5}], b: 2}) + // > db.x.insert({_id: "w", a: [{x:5}, {x:10}], b: 1}) + // > db.x.find({}).sort({'a.x': 1, b: 1}) + // { "_id" : "q", "a" : [ { "x" : 1 }, { "x" : 5 } ], "b" : 2 } + // { "_id" : "w", "a" : [ { "x" : 5 }, { "x" : 10 } ], "b" : 1 } + // > db.x.find({a: {x:5}}).sort({'a.x': 1, b: 1}) + // { "_id" : "q", "a" : [ { "x" : 1 }, { "x" : 5 } ], "b" : 2 } + // { "_id" : "w", "a" : [ { "x" : 5 }, { "x" : 10 } ], "b" : 1 } + // > db.x.find({'a.x': 5}).sort({'a.x': 1, b: 1}) + // { "_id" : "w", "a" : [ { "x" : 5 }, { "x" : 10 } ], "b" : 1 } + // { "_id" : "q", "a" : [ { "x" : 1 }, { "x" : 5 } ], "b" : 2 } + // ie, only the last one manages to trigger the key compatibility code, + // not the previous one. (The "b" sort is necessary because when the key + // compatibility code *does* kick in, both documents only end up with "5" + // for the first field as their only sort key, and we need to differentiate + // somehow...) + keyCompatible({'a.x': 1}, {a: {x: 5}}, [1], true); + keyCompatible({'a.x': 1}, {'a.x': 5}, [5], true); + keyCompatible({'a.x': 1}, {'a.x': 5}, [1], false); +}); + Tinytest.add("minimongo - binary search", function (test) { var forwardCmp = function (a, b) { return a - b; diff --git a/packages/minimongo/sort.js b/packages/minimongo/sort.js index 2dafb5f7c1..fa50b9c0af 100644 --- a/packages/minimongo/sort.js +++ b/packages/minimongo/sort.js @@ -111,7 +111,7 @@ _.extend(Minimongo.Sorter.prototype, { var minKey = null; self._generateKeysFromDoc(doc, function (key) { - if (self._keyFilter && !self._keyFilter(key)) + if (!self._keyCompatibleWithSelector(key)) return; if (minKey === null) { @@ -130,6 +130,11 @@ _.extend(Minimongo.Sorter.prototype, { return minKey; }, + _keyCompatibleWithSelector: function (key) { + var self = this; + return !self._keyFilter || self._keyFilter(key); + }, + // Iterates over each possible "key" from doc (ie, over each branch), calling // 'cb' with the key. _generateKeysFromDoc: function (doc, cb) { @@ -348,12 +353,11 @@ _.extend(Minimongo.Sorter.prototype, { } // XXX support $regex/RegExp, {$exists: true}, $mod, $type, $in }); + return; } // OK, it's an equality thing. - if (subSelector == null) { - constraints[0].push(equalityElementMatcher(subSelector)); - } + constraints[0].push(equalityElementMatcher(subSelector)); }); if (!_.isEmpty(constraints[0])) { From 29cfa27ef3c1496170c251053955cf5bcd5439fb Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 16 Mar 2014 19:09:51 -0700 Subject: [PATCH 05/11] support regexps in sort key filter also in EJSON.clone --- packages/ejson/ejson.js | 4 ++++ packages/minimongo/minimongo_tests.js | 19 +++++++++++++++++++ packages/minimongo/selector.js | 2 +- packages/minimongo/sort.js | 27 ++++++++++++++++++++++++--- 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/packages/ejson/ejson.js b/packages/ejson/ejson.js index 48888b1003..1b8f7b704b 100644 --- a/packages/ejson/ejson.js +++ b/packages/ejson/ejson.js @@ -356,6 +356,10 @@ EJSON.clone = function (v) { return null; // null has typeof "object" if (v instanceof Date) return new Date(v.getTime()); + // RegExps are not really EJSON elements (eg we don't define a serialization + // for them), but they're immutable anyway, so we can support them in clone. + if (v instanceof RegExp) + return v; if (EJSON.isBinary(v)) { ret = EJSON.newBinary(v.length); for (var i = 0; i < v.length; i++) { diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index 311c885dad..2912ff7421 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1917,6 +1917,25 @@ Tinytest.add("minimongo - sort key filter", function (test) { keyCompatible({'a.x': 1}, {a: {x: 5}}, [1], true); keyCompatible({'a.x': 1}, {'a.x': 5}, [5], true); keyCompatible({'a.x': 1}, {'a.x': 5}, [1], false); + + // Regex key check. + keyCompatible({a: 1}, {a: /^foo+/}, ['foo'], true); + keyCompatible({a: 1}, {a: /^foo+/}, ['foooo'], true); + keyCompatible({a: 1}, {a: /^foo+/}, ['foooobar'], true); + keyCompatible({a: 1}, {a: /^foo+/}, ['afoooo'], false); + keyCompatible({a: 1}, {a: /^foo+/}, [''], false); + keyCompatible({a: 1}, {a: {$regex: "^foo+"}}, ['foo'], true); + keyCompatible({a: 1}, {a: {$regex: "^foo+"}}, ['foooo'], true); + keyCompatible({a: 1}, {a: {$regex: "^foo+"}}, ['foooobar'], true); + keyCompatible({a: 1}, {a: {$regex: "^foo+"}}, ['afoooo'], false); + keyCompatible({a: 1}, {a: {$regex: "^foo+"}}, [''], false); + + keyCompatible({a: 1}, {a: /^foo+/i}, ['foo'], true); + // Key compatibility check appears to be turned off for regexps with flags. + keyCompatible({a: 1}, {a: /^foo+/i}, ['bar'], true); + keyCompatible({a: 1}, {a: /^foo+/m}, ['bar'], true); + keyCompatible({a: 1}, {a: {$regex: "^foo+", $options: "i"}}, ['bar'], true); + keyCompatible({a: 1}, {a: {$regex: "^foo+", $options: "m"}}, ['bar'], true); }); Tinytest.add("minimongo - binary search", function (test) { diff --git a/packages/minimongo/selector.js b/packages/minimongo/selector.js index 88c7fba25a..2e8516786c 100644 --- a/packages/minimongo/selector.js +++ b/packages/minimongo/selector.js @@ -204,7 +204,7 @@ var convertElementMatcherToBranchedMatcher = function ( }; // Takes a RegExp object and returns an element matcher. -var regexpElementMatcher = function (regexp) { +regexpElementMatcher = function (regexp) { return function (value) { if (value instanceof RegExp) { // Comparing two regexps means seeing if the regexps are identical diff --git a/packages/minimongo/sort.js b/packages/minimongo/sort.js index fa50b9c0af..3747e4bf78 100644 --- a/packages/minimongo/sort.js +++ b/packages/minimongo/sort.js @@ -339,9 +339,22 @@ _.extend(Minimongo.Sorter.prototype, { if (key !== firstSortField) return; - // XXX support RegExp constraints - if (subSelector instanceof RegExp) + // XXX it looks like the real MongoDB implementation isn't "does the + // regexp match" but "does the value fall into a range named by the + // literal prefix of the regexp", ie "foo" in /^foo(bar|baz)+/ But + // "does the regexp match" is a good approximation. + if (subSelector instanceof RegExp) { + // As far as we can tell, using either of the options that both we and + // MongoDB support ('i' and 'm') disables use of the key filter. This + // makes sense: MongoDB mostly appears to be calculating ranges of an + // index to use, which means it only cares about regexps that match + // one range (with a literal prefix), and both 'i' and 'm' prevent the + // literal prefix of the regexp from actually meaning one range. + if (subSelector.ignoreCase || subSelector.multiline) + return; + constraints[0].push(regexpElementMatcher(subSelector)); return; + } if (isOperatorObject(subSelector)) { _.each(subSelector, function (operand, operator) { @@ -351,7 +364,15 @@ _.extend(Minimongo.Sorter.prototype, { constraints[0].push( ELEMENT_OPERATORS[operator].compileElementSelector(operand)); } - // XXX support $regex/RegExp, {$exists: true}, $mod, $type, $in + + // See comments in the RegExp block above. + if (operator === '$regex' && !subSelector.$options) { + constraints[0].push( + ELEMENT_OPERATORS.$regex.compileElementSelector( + operand, subSelector)); + } + + // XXX support {$exists: true}, $mod, $type, $in }); return; } From 8488047c13047a22867cd737f3ddcf75bbee59f4 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 16 Mar 2014 19:18:45 -0700 Subject: [PATCH 06/11] support non-initial keys in sort selector filter --- packages/minimongo/minimongo_tests.js | 12 +++++++ packages/minimongo/sort.js | 52 +++++++++++++-------------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index 2912ff7421..f159cb37e8 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1936,6 +1936,18 @@ Tinytest.add("minimongo - sort key filter", function (test) { keyCompatible({a: 1}, {a: /^foo+/m}, ['bar'], true); keyCompatible({a: 1}, {a: {$regex: "^foo+", $options: "i"}}, ['bar'], true); keyCompatible({a: 1}, {a: {$regex: "^foo+", $options: "m"}}, ['bar'], true); + + // Multiple keys! + keyCompatible({a: 1, b: 1, c: 1}, + {a: {$gt: 5}, c: {$lt: 3}}, [6, "bla", 2], true); + keyCompatible({a: 1, b: 1, c: 1}, + {a: {$gt: 5}, c: {$lt: 3}}, [6, "bla", 4], false); + keyCompatible({a: 1, b: 1, c: 1}, + {a: {$gt: 5}, c: {$lt: 3}}, [3, "bla", 1], false); + // No filtering is done (ie, all keys are compatible) if the first key isn't + // constrained. + keyCompatible({a: 1, b: 1, c: 1}, + {c: {$lt: 3}}, [3, "bla", 4], true); }); Tinytest.add("minimongo - binary search", function (test) { diff --git a/packages/minimongo/sort.js b/packages/minimongo/sort.js index 3747e4bf78..b36a7b0669 100644 --- a/packages/minimongo/sort.js +++ b/packages/minimongo/sort.js @@ -318,25 +318,16 @@ _.extend(Minimongo.Sorter.prototype, { if (selector instanceof Function) return; - // It appears that the first sort field is treated differently from the - // others; we shouldn't create a key filter unless the first sort field is - // restricted, though after that point we can restrict the other sort fields - // or not as we wish. - var firstSortField = self._sortSpecParts[0].path; - var otherSortFields = {}; - _.each(self._sortSpecParts, function (path, i) { - if (i !== 0) - otherSortFields[path] = true; - }); - - var constraints = []; - _.each(self._sortSpecParts, function () { - constraints.push([]); + var constraintsByPath = {}; + _.each(self._sortSpecParts, function (spec, i) { + constraintsByPath[spec.path] = []; }); _.each(selector, function (subSelector, key) { - // XXX support constraints on non-first sort fields, $and, $or, etc. - if (key !== firstSortField) + // XXX support $and and $or + + var constraints = constraintsByPath[key]; + if (!constraints) return; // XXX it looks like the real MongoDB implementation isn't "does the @@ -352,7 +343,7 @@ _.extend(Minimongo.Sorter.prototype, { // literal prefix of the regexp from actually meaning one range. if (subSelector.ignoreCase || subSelector.multiline) return; - constraints[0].push(regexpElementMatcher(subSelector)); + constraints.push(regexpElementMatcher(subSelector)); return; } @@ -361,33 +352,40 @@ _.extend(Minimongo.Sorter.prototype, { if (_.contains(['$lt', '$lte', '$gt', '$gte'], operator)) { // XXX this depends on us knowing that these operators don't use any // of the arguments to compileElementSelector other than operand. - constraints[0].push( + constraints.push( ELEMENT_OPERATORS[operator].compileElementSelector(operand)); } // See comments in the RegExp block above. if (operator === '$regex' && !subSelector.$options) { - constraints[0].push( + constraints.push( ELEMENT_OPERATORS.$regex.compileElementSelector( operand, subSelector)); } - // XXX support {$exists: true}, $mod, $type, $in + // XXX support {$exists: true}, $mod, $type, $in, $elemMatch }); return; } // OK, it's an equality thing. - constraints[0].push(equalityElementMatcher(subSelector)); + constraints.push(equalityElementMatcher(subSelector)); }); - if (!_.isEmpty(constraints[0])) { - self._keyFilter = function (key) { - return _.all(constraints[0], function (f) { - return f(key[0]); + // It appears that the first sort field is treated differently from the + // others; we shouldn't create a key filter unless the first sort field is + // restricted, though after that point we can restrict the other sort fields + // or not as we wish. + if (_.isEmpty(constraintsByPath[self._sortSpecParts[0].path])) + return; + + self._keyFilter = function (key) { + return _.all(self._sortSpecParts, function (specPart, index) { + return _.all(constraintsByPath[specPart.path], function (f) { + return f(key[index]); }); - }; - } + }); + }; } }); From d4241aa998a11c2cf9ebeb9fa9e4c6c3a7bcbc40 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 17 Mar 2014 01:08:29 -0700 Subject: [PATCH 07/11] Improve ELEMENT_OPERATORS comment. Make there be one consistent syntax for element operators. --- packages/minimongo/selector.js | 188 +++++++++++++++++---------------- 1 file changed, 96 insertions(+), 92 deletions(-) diff --git a/packages/minimongo/selector.js b/packages/minimongo/selector.js index 2e8516786c..a573c0f90c 100644 --- a/packages/minimongo/selector.js +++ b/packages/minimongo/selector.js @@ -509,46 +509,50 @@ var pointToArray = function (point) { // Helper for $lt/$gt/$lte/$gte. var makeInequality = function (cmpValueComparator) { - return function (operand) { - // Arrays never compare false with non-arrays for any inequality. - // XXX This was behavior we observed in pre-release MongoDB 2.5, but - // it seems to have been reverted. - // See https://jira.mongodb.org/browse/SERVER-11444 - if (isArray(operand)) { - return function () { - return false; + return { + compileElementSelector: function (operand) { + // Arrays never compare false with non-arrays for any inequality. + // XXX This was behavior we observed in pre-release MongoDB 2.5, but + // it seems to have been reverted. + // See https://jira.mongodb.org/browse/SERVER-11444 + if (isArray(operand)) { + return function () { + return false; + }; + } + + // Special case: consider undefined and null the same (so true with + // $gte/$lte). + if (operand === undefined) + operand = null; + + var operandType = LocalCollection._f._type(operand); + + return function (value) { + if (value === undefined) + value = null; + // Comparisons are never true among things of different type (except + // null vs undefined). + if (LocalCollection._f._type(value) !== operandType) + return false; + return cmpValueComparator(LocalCollection._f._cmp(value, operand)); }; } - - // Special case: consider undefined and null the same (so true with - // $gte/$lte). - if (operand === undefined) - operand = null; - - var operandType = LocalCollection._f._type(operand); - - return function (value) { - if (value === undefined) - value = null; - // Comparisons are never true among things of different type (except null - // vs undefined). - if (LocalCollection._f._type(value) !== operandType) - return false; - return cmpValueComparator(LocalCollection._f._cmp(value, operand)); - }; }; }; -// Each element selector is a function with args: -// - operand - the "right hand side" of the operator -// - valueSelector - the "context" for the operator (so that $regex can find -// $options) -// Or is an object with an compileElementSelector field (the above) and optional -// flags dontExpandLeafArrays and dontIncludeLeafArrays which control if -// expandArraysInBranches is called and if it takes an optional argument. -// -// An element selector compiler returns a function mapping a single value to -// bool. +// Each element selector contains: +// - compileElementSelector, a function with args: +// - operand - the "right hand side" of the operator +// - valueSelector - the "context" for the operator (so that $regex can find +// $options) +// - matcher - the Matcher this is going into (so that $elemMatch can compile +// more things) +// returning a function mapping a single value to bool. +// - dontExpandLeafArrays, a bool which prevents expandArraysInBranches from +// being called +// - dontIncludeLeafArrays, a bool which causes an argument to be passed to +// expandArraysInBranches if it is called ELEMENT_OPERATORS = { $lt: makeInequality(function (cmpValue) { return cmpValue < 0; @@ -562,41 +566,45 @@ ELEMENT_OPERATORS = { $gte: makeInequality(function (cmpValue) { return cmpValue >= 0; }), - $mod: function (operand) { - if (!(isArray(operand) && operand.length === 2 - && typeof(operand[0]) === 'number' - && typeof(operand[1]) === 'number')) { - throw Error("argument to $mod must be an array of two numbers"); + $mod: { + compileElementSelector: function (operand) { + if (!(isArray(operand) && operand.length === 2 + && typeof(operand[0]) === 'number' + && typeof(operand[1]) === 'number')) { + throw Error("argument to $mod must be an array of two numbers"); + } + // XXX could require to be ints or round or something + var divisor = operand[0]; + var remainder = operand[1]; + return function (value) { + return typeof value === 'number' && value % divisor === remainder; + }; } - // XXX could require to be ints or round or something - var divisor = operand[0]; - var remainder = operand[1]; - return function (value) { - return typeof value === 'number' && value % divisor === remainder; - }; }, - $in: function (operand) { - if (!isArray(operand)) - throw Error("$in needs an array"); + $in: { + compileElementSelector: function (operand) { + if (!isArray(operand)) + throw Error("$in needs an array"); - var elementMatchers = []; - _.each(operand, function (option) { - if (option instanceof RegExp) - elementMatchers.push(regexpElementMatcher(option)); - else if (isOperatorObject(option)) - throw Error("cannot nest $ under $in"); - else - elementMatchers.push(equalityElementMatcher(option)); - }); - - return function (value) { - // Allow {a: {$in: [null]}} to match when 'a' does not exist. - if (value === undefined) - value = null; - return _.any(elementMatchers, function (e) { - return e(value); + var elementMatchers = []; + _.each(operand, function (option) { + if (option instanceof RegExp) + elementMatchers.push(regexpElementMatcher(option)); + else if (isOperatorObject(option)) + throw Error("cannot nest $ under $in"); + else + elementMatchers.push(equalityElementMatcher(option)); }); - }; + + return function (value) { + // Allow {a: {$in: [null]}} to match when 'a' does not exist. + if (value === undefined) + value = null; + return _.any(elementMatchers, function (e) { + return e(value); + }); + }; + } }, $size: { // {a: [[5, 5]]} must match {a: {$size: 1}} but not {a: {$size: 2}}, so we @@ -631,30 +639,32 @@ ELEMENT_OPERATORS = { }; } }, - $regex: function (operand, valueSelector) { - if (!(typeof operand === 'string' || operand instanceof RegExp)) - throw Error("$regex has to be a string or RegExp"); + $regex: { + compileElementSelector: function (operand, valueSelector) { + if (!(typeof operand === 'string' || operand instanceof RegExp)) + throw Error("$regex has to be a string or RegExp"); - var regexp; - if (valueSelector.$options !== undefined) { - // Options passed in $options (even the empty string) always overrides - // options in the RegExp object itself. (See also - // Meteor.Collection._rewriteSelector.) + var regexp; + if (valueSelector.$options !== undefined) { + // Options passed in $options (even the empty string) always overrides + // options in the RegExp object itself. (See also + // Meteor.Collection._rewriteSelector.) - // Be clear that we only support the JS-supported options, not extended - // ones (eg, Mongo supports x and s). Ideally we would implement x and s - // by transforming the regexp, but not today... - if (/[^gim]/.test(valueSelector.$options)) - throw new Error("Only the i, m, and g regexp options are supported"); + // Be clear that we only support the JS-supported options, not extended + // ones (eg, Mongo supports x and s). Ideally we would implement x and s + // by transforming the regexp, but not today... + if (/[^gim]/.test(valueSelector.$options)) + throw new Error("Only the i, m, and g regexp options are supported"); - var regexSource = operand instanceof RegExp ? operand.source : operand; - regexp = new RegExp(regexSource, valueSelector.$options); - } else if (operand instanceof RegExp) { - regexp = operand; - } else { - regexp = new RegExp(operand); + var regexSource = operand instanceof RegExp ? operand.source : operand; + regexp = new RegExp(regexSource, valueSelector.$options); + } else if (operand instanceof RegExp) { + regexp = operand; + } else { + regexp = new RegExp(operand); + } + return regexpElementMatcher(regexp); } - return regexpElementMatcher(regexp); }, $elemMatch: { dontExpandLeafArrays: true, @@ -703,12 +713,6 @@ ELEMENT_OPERATORS = { } } }; -_.each(ELEMENT_OPERATORS, function (options, operator) { - if (typeof options === 'function') { - // XXX safe to modify while iterating? - ELEMENT_OPERATORS[operator] = {compileElementSelector: options}; - } -}); // makeLookupFunction(key) returns a lookup function. // From cda515249186e3cc05079ebf86473fff4556ba23 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 17 Mar 2014 01:14:14 -0700 Subject: [PATCH 08/11] Refactor a test to remove magic numbers --- packages/minimongo/minimongo_tests.js | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index f159cb37e8..e5f1280212 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1791,17 +1791,22 @@ Tinytest.add("minimongo - array sort", function (test) { c.insert({up: 3, down: 3, selected: 1, a: {x: 2.5}}); c.insert({up: 4, down: 0, selected: 3, a: {x: 5}}); - test.equal( - _.pluck(c.find({}, {sort: {'a.x': 1}}).fetch(), 'up'), - _.range(5)); + // Test that the the documents in "cursor" contain values with the name + // "field" running from 0 to the max value of that name in the collection. + var testCursorMatchesField = function (cursor, field) { + var fieldValues = []; + c.find().forEach(function (doc) { + if (_.has(doc, field)) + fieldValues.push(doc[field]); + }); + test.equal(_.pluck(cursor.fetch(), field), + _.range(_.max(fieldValues) + 1)); + }; - test.equal( - _.pluck(c.find({}, {sort: {'a.x': -1}}).fetch(), 'down'), - _.range(5)); - - test.equal( - _.pluck(c.find({'a.x': {$gt: 1}}, {sort: {'a.x': 1}}).fetch(), 'selected'), - _.range(4)); + testCursorMatchesField(c.find({}, {sort: {'a.x': 1}}), 'up'); + testCursorMatchesField(c.find({}, {sort: {'a.x': -1}}), 'down'); + testCursorMatchesField(c.find({'a.x': {$gt: 1}}, {sort: {'a.x': 1}}), + 'selected'); }); Tinytest.add("minimongo - sort keys", function (test) { From 15fa6b2ab7b8820dd61448aa35ff295f9ccd834c Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 17 Mar 2014 01:24:23 -0700 Subject: [PATCH 09/11] Pass Matcher to Sorter constructor --- packages/minimongo/minimongo.js | 3 +-- packages/minimongo/minimongo_tests.js | 6 ++---- packages/minimongo/selector.js | 2 +- packages/minimongo/sort.js | 22 ++++++++++++---------- packages/mongo-livedata/mongo_driver.js | 7 ++----- 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/packages/minimongo/minimongo.js b/packages/minimongo/minimongo.js index c9a2c3f0e1..e9ecdf1fc5 100644 --- a/packages/minimongo/minimongo.js +++ b/packages/minimongo/minimongo.js @@ -100,8 +100,7 @@ LocalCollection.Cursor = function (collection, selector, options) { self._selectorId = undefined; self.matcher = new Minimongo.Matcher(selector, self); self.sorter = (self.matcher.hasGeoQuery() || options.sort) ? - new Minimongo.Sorter(options.sort || []) : null; - self.sorter && self.sorter.useWithMatcher(self.matcher); + new Minimongo.Sorter(options.sort || [], {matcher: self.matcher}) : null; } self.skip = options.skip; self.limit = options.limit; diff --git a/packages/minimongo/minimongo_tests.js b/packages/minimongo/minimongo_tests.js index e5f1280212..933ee9365f 100644 --- a/packages/minimongo/minimongo_tests.js +++ b/packages/minimongo/minimongo_tests.js @@ -1875,8 +1875,7 @@ Tinytest.add("minimongo - sort keys", function (test) { Tinytest.add("minimongo - sort key filter", function (test) { var testOrder = function (sortSpec, selector, doc1, doc2) { var matcher = new Minimongo.Matcher(selector); - var sorter = new Minimongo.Sorter(sortSpec); - sorter.useWithMatcher(matcher); + var sorter = new Minimongo.Sorter(sortSpec, {matcher: matcher}); var comparator = sorter.getComparator(); var comparison = comparator(doc1, doc2); test.isTrue(comparison < 0); @@ -1891,8 +1890,7 @@ Tinytest.add("minimongo - sort key filter", function (test) { var keyCompatible = function (sortSpec, selector, key, compatible) { var matcher = new Minimongo.Matcher(selector); - var sorter = new Minimongo.Sorter(sortSpec); - sorter.useWithMatcher(matcher); + var sorter = new Minimongo.Sorter(sortSpec, {matcher: matcher}); var actual = sorter._keyCompatibleWithSelector(key); test.equal(actual, compatible); }; diff --git a/packages/minimongo/selector.js b/packages/minimongo/selector.js index a573c0f90c..437d0c366d 100644 --- a/packages/minimongo/selector.js +++ b/packages/minimongo/selector.js @@ -38,7 +38,7 @@ Minimongo.Matcher = function (selector) { // A clone of the original selector. It may just be a function if the user // passed in a function; otherwise is definitely an object (eg, IDs are // translated into {_id: ID} first. Used by canBecomeTrueByModifier and - // Sorter.useWithMatcher. + // Sorter._useWithMatcher. self._selector = null; self._docMatcher = self._compileSelector(selector); }; diff --git a/packages/minimongo/sort.js b/packages/minimongo/sort.js index b36a7b0669..81dd548f78 100644 --- a/packages/minimongo/sort.js +++ b/packages/minimongo/sort.js @@ -11,8 +11,9 @@ // first object comes first in order, 1 if the second object comes // first, or 0 if neither object comes before the other. -Minimongo.Sorter = function (spec) { +Minimongo.Sorter = function (spec, options) { var self = this; + options = options || {}; self._sortSpecParts = []; @@ -60,10 +61,11 @@ Minimongo.Sorter = function (spec) { return self._keyFieldComparator(i); })); - // If you call useWithMatcher on this Sorter, this may be set to a function - // which selects whether or not a given "sort key" (tuple of values for the - // different sort spec fields) is compatible with the selector. + // If you specify a matcher for this Sorter, _keyFilter may be set to a + // function which selects whether or not a given "sort key" (tuple of values + // for the different sort spec fields) is compatible with the selector. self._keyFilter = null; + options.matcher && self._useWithMatcher(options.matcher); }; // In addition to these methods, sorter_project.js defines combineIntoProjection @@ -295,15 +297,15 @@ _.extend(Minimongo.Sorter.prototype, { // both follow this rule (y before x). (ie, you do have to apply this // through $elemMatch.) // - // So if you call `useWithMatcher(matcher)` with the Matcher that goes with - // this Sorter, we will attempt to skip sort keys that don't match the - // selector. The logic here is pretty subtle and undocumented; we've gotten as - // close as we can figure out based on our understanding of Mongo's behavior. - useWithMatcher: function (matcher) { + // So if you pass a matcher to this sorter's constructor, we will attempt to + // skip sort keys that don't match the selector. The logic here is pretty + // subtle and undocumented; we've gotten as close as we can figure out based + // on our understanding of Mongo's behavior. + _useWithMatcher: function (matcher) { var self = this; if (self._keyFilter) - throw Error("called useWithMatcher twice?"); + throw Error("called _useWithMatcher twice?"); // If we are only sorting by distance, then we're not going to bother to // build a key filter. diff --git a/packages/mongo-livedata/mongo_driver.js b/packages/mongo-livedata/mongo_driver.js index a770d0176e..ab9c9ea3a5 100644 --- a/packages/mongo-livedata/mongo_driver.js +++ b/packages/mongo-livedata/mongo_driver.js @@ -1023,7 +1023,8 @@ MongoConnection.prototype._observeChanges = function ( if (!cursorDescription.options.sort) return true; try { - sorter = new Minimongo.Sorter(cursorDescription.options.sort); + sorter = new Minimongo.Sorter(cursorDescription.options.sort, + {matcher: matcher}); return true; } catch (e) { // XXX make all compilation errors MinimongoError or something @@ -1032,10 +1033,6 @@ MongoConnection.prototype._observeChanges = function ( } }], function (f) { return f(); }); // invoke each function - if (matcher && sorter) { - sorter.useWithMatcher(matcher); - } - var driverClass = canUseOplog ? OplogObserveDriver : PollingObserveDriver; observeDriver = new driverClass({ cursorDescription: cursorDescription, From 17297d98d7c5757f5168c0cb22a6b2e2017c2d02 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 17 Mar 2014 02:16:16 -0700 Subject: [PATCH 10/11] make LocalCollection.Cursor easier to read and whitespace cleanup --- packages/minimongo/minimongo.js | 10 ++++++---- packages/mongo-livedata/mongo_driver.js | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/minimongo/minimongo.js b/packages/minimongo/minimongo.js index e9ecdf1fc5..f9fc5debf9 100644 --- a/packages/minimongo/minimongo.js +++ b/packages/minimongo/minimongo.js @@ -89,18 +89,20 @@ LocalCollection.Cursor = function (collection, selector, options) { var self = this; if (!options) options = {}; - this.collection = collection; + self.collection = collection; + self.sorter = null; if (LocalCollection._selectorIsId(selector)) { // stash for fast path self._selectorId = selector; self.matcher = new Minimongo.Matcher(selector, self); - self.sorter = undefined; } else { self._selectorId = undefined; self.matcher = new Minimongo.Matcher(selector, self); - self.sorter = (self.matcher.hasGeoQuery() || options.sort) ? - new Minimongo.Sorter(options.sort || [], {matcher: self.matcher}) : null; + if (self.matcher.hasGeoQuery() || options.sort) { + self.sorter = new Minimongo.Sorter(options.sort || [], + { matcher: self.matcher }); + } } self.skip = options.skip; self.limit = options.limit; diff --git a/packages/mongo-livedata/mongo_driver.js b/packages/mongo-livedata/mongo_driver.js index ab9c9ea3a5..e43e00d68c 100644 --- a/packages/mongo-livedata/mongo_driver.js +++ b/packages/mongo-livedata/mongo_driver.js @@ -1024,7 +1024,7 @@ MongoConnection.prototype._observeChanges = function ( return true; try { sorter = new Minimongo.Sorter(cursorDescription.options.sort, - {matcher: matcher}); + { matcher: matcher }); return true; } catch (e) { // XXX make all compilation errors MinimongoError or something From 93e1969560f8bcdfcfffbf125507320acaced049 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 17 Mar 2014 02:56:07 -0700 Subject: [PATCH 11/11] history update --- History.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/History.md b/History.md index d5a1ba16cd..457e2cdfaf 100644 --- a/History.md +++ b/History.md @@ -27,6 +27,14 @@ `{a: [{x: 0, y: 5}, {x: 1, y: 3}]}`, because the 3 should not be used as a tie-breaker because it is not "next to" the tied 0s. +* minimongo: Fix sort implementation when selector and sort key share a field, + that field matches an array in the document, and only some values of the array + match the selector. eg, ensure that with sort key `{a: 1}` and selector + `{a: {$gt: 3}}`, the document `{a: [4, 6]}` sorts before `{a: [1, 5]}`, + because the 1 should not be used as a sort key because it does not match the + selector. (We only approximate the MongoDB behavior here by only supporting + relatively selectors.) + * Use `faye-websocket` (0.7.2) npm module instead of `websocket` (1.0.8) for server-to-server DDP.