Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make behaviour of cursor.count() on client reflect server #9205

Merged
merged 4 commits into from
Oct 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions History.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
## v.NEXT

* Enables passing `false` to `cursor.count()` on the client to prevent skip
and limit from having an effect on the result.
[Issue #1201](https://github.com/meteor/meteor/issues/1201)
[PR #9205](https://github.com/meteor/meteor/pull/9205)

* iOS icons and launch screens have been updated to support iOS 11
[Issue #9196](https://github.com/meteor/meteor/issues/9196)
[PR #9198](https://github.com/meteor/meteor/pull/9198)
Expand Down
37 changes: 29 additions & 8 deletions packages/minimongo/cursor.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,25 @@ export default class Cursor {
* @summary Returns the number of documents that match a query.
* @memberOf Mongo.Cursor
* @method count
* @param {boolean} [applySkipLimit=true] If set to `false`, the value
* returned will reflect the total
* number of matching documents,
* ignoring any value supplied for
* limit
* @instance
* @locus Anywhere
* @returns {Number}
*/
count() {
count(applySkipLimit = true) {
if (this.reactive) {
// allow the observe to be unordered
this._depend({added: true, removed: true}, true);
}

return this._getRawObjects({ordered: true}).length;
return this._getRawObjects({
ordered: true,
applySkipLimit
}).length;
}

/**
Expand Down Expand Up @@ -379,7 +387,8 @@ export default class Cursor {
// Returns a collection of matching objects, but doesn't deep copy them.
//
// If ordered is set, returns a sorted array, respecting sorter, skip, and
// limit properties of the query. if sorter is falsey, no sort -- you get the
// limit properties of the query provided that options.applySkipLimit is
// not set to false (#1201). If sorter is falsey, no sort -- you get the
// natural order.
//
// If ordered is not set, returns an object mapping from ID to doc (sorter,
Expand All @@ -393,16 +402,21 @@ export default class Cursor {
// implementation uses this to remember the distances after this function
// returns.
_getRawObjects(options = {}) {
// By default this method will respect skip and limit because .fetch(),
// .forEach() etc... expect this behaviour. It can be forced to ignore
// skip and limit by setting applySkipLimit to false (.count() does this,
// for example)
const applySkipLimit = options.applySkipLimit !== false;

// XXX use OrderedDict instead of array, and make IdMap and OrderedDict
// compatible
const results = options.ordered ? [] : new LocalCollection._IdMap;

// fast path for single ID value
if (this._selectorId !== undefined) {
// If you have non-zero skip and ask for a single id, you get
// nothing. This is so it matches the behavior of the '{_id: foo}'
// path.
if (this.skip) {
// If you have non-zero skip and ask for a single id, you get nothing.
// This is so it matches the behavior of the '{_id: foo}' path.
if (applySkipLimit && this.skip) {
return results;
}

Expand Down Expand Up @@ -449,6 +463,11 @@ export default class Cursor {
}
}

// Override to ensure all docs are matched if ignoring skip & limit
if (!applySkipLimit) {
return true;
}

// Fast path for limited unsorted queries.
// XXX 'length' check here seems wrong for ordered
return (
Expand All @@ -467,7 +486,9 @@ export default class Cursor {
results.sort(this.sorter.getComparator({distances}));
}

if (!this.limit && !this.skip) {
// Return the full set of results if there is no skip or limit or if we're
// ignoring them
if (!applySkipLimit || (!this.limit && !this.skip)) {
return results;
}

Expand Down
35 changes: 32 additions & 3 deletions packages/minimongo/minimongo_tests_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,30 +137,46 @@ Tinytest.add('minimongo - basics', test => {
test.equal(c.find('abc').count(), 0);
test.equal(c.find(undefined).count(), 0);
test.equal(c.find().count(), 3);
test.equal(c.find(1, {skip: 1}).count(false), 1);
test.equal(c.find(1, {skip: 1}).count(), 0);
test.equal(c.find({_id: 1}, {skip: 1}).count(false), 1);
test.equal(c.find({_id: 1}, {skip: 1}).count(), 0);
test.equal(c.find({_id: undefined}).count(), 0);
test.equal(c.find({_id: false}).count(), 0);
test.equal(c.find({_id: null}).count(), 0);
test.equal(c.find({_id: ''}).count(), 0);
test.equal(c.find({_id: 0}).count(), 0);
test.equal(c.find({}, {skip: 1}).count(false), 3);
test.equal(c.find({}, {skip: 1}).count(), 2);
test.equal(c.find({}, {skip: 2}).count(), 1);
test.equal(c.find({}, {limit: 2}).count(false), 3);
test.equal(c.find({}, {limit: 2}).count(), 2);
test.equal(c.find({}, {limit: 1}).count(), 1);
test.equal(c.find({}, {skip: 1, limit: 1}).count(false), 3);
test.equal(c.find({}, {skip: 1, limit: 1}).count(), 1);
test.equal(c.find({tags: 'fruit'}, {skip: 1}).count(false), 2);
test.equal(c.find({tags: 'fruit'}, {skip: 1}).count(), 1);
test.equal(c.find({tags: 'fruit'}, {limit: 1}).count(false), 2);
test.equal(c.find({tags: 'fruit'}, {limit: 1}).count(), 1);
test.equal(c.find({tags: 'fruit'}, {skip: 1, limit: 1}).count(false), 2);
test.equal(c.find({tags: 'fruit'}, {skip: 1, limit: 1}).count(), 1);
test.equal(c.find(1, {sort: ['_id', 'desc'], skip: 1}).count(false), 1);
test.equal(c.find(1, {sort: ['_id', 'desc'], skip: 1}).count(), 0);
test.equal(c.find({_id: 1}, {sort: ['_id', 'desc'], skip: 1}).count(false), 1);
test.equal(c.find({_id: 1}, {sort: ['_id', 'desc'], skip: 1}).count(), 0);
test.equal(c.find({}, {sort: ['_id', 'desc'], skip: 1}).count(false), 3);
test.equal(c.find({}, {sort: ['_id', 'desc'], skip: 1}).count(), 2);
test.equal(c.find({}, {sort: ['_id', 'desc'], skip: 2}).count(), 1);
test.equal(c.find({}, {sort: ['_id', 'desc'], limit: 2}).count(false), 3);
test.equal(c.find({}, {sort: ['_id', 'desc'], limit: 2}).count(), 2);
test.equal(c.find({}, {sort: ['_id', 'desc'], limit: 1}).count(), 1);
test.equal(c.find({}, {sort: ['_id', 'desc'], skip: 1, limit: 1}).count(false), 3);
test.equal(c.find({}, {sort: ['_id', 'desc'], skip: 1, limit: 1}).count(), 1);
test.equal(c.find({tags: 'fruit'}, {sort: ['_id', 'desc'], skip: 1}).count(false), 2);
test.equal(c.find({tags: 'fruit'}, {sort: ['_id', 'desc'], skip: 1}).count(), 1);
test.equal(c.find({tags: 'fruit'}, {sort: ['_id', 'desc'], limit: 1}).count(false), 2);
test.equal(c.find({tags: 'fruit'}, {sort: ['_id', 'desc'], limit: 1}).count(), 1);
test.equal(c.find({tags: 'fruit'}, {sort: ['_id', 'desc'], skip: 1, limit: 1}).count(false), 2);
test.equal(c.find({tags: 'fruit'}, {sort: ['_id', 'desc'], skip: 1, limit: 1}).count(), 1);

// Regression test for #455.
Expand Down Expand Up @@ -3441,7 +3457,7 @@ Tinytest.add('minimongo - immediate invalidate', test => {

Tinytest.add('minimongo - count on cursor with limit', test => {
const coll = new LocalCollection();
let count;
let count, unlimitedCount;

coll.insert({_id: 'A'});
coll.insert({_id: 'B'});
Expand All @@ -3451,26 +3467,32 @@ Tinytest.add('minimongo - count on cursor with limit', test => {
const c = Tracker.autorun(c => {
const cursor = coll.find({_id: {$exists: true}}, {sort: {_id: 1}, limit: 3});
count = cursor.count();
unlimitedCount = cursor.count(false);
});

test.equal(count, 3);
test.equal(unlimitedCount, 4);

coll.remove('A'); // still 3 in the collection
Tracker.flush();
test.equal(count, 3);
test.equal(unlimitedCount, 3);

coll.remove('B'); // expect count now 2
Tracker.flush();
test.equal(count, 2);
test.equal(unlimitedCount, 2);


coll.insert({_id: 'A'}); // now 3 again
Tracker.flush();
test.equal(count, 3);
test.equal(unlimitedCount, 3);

coll.insert({_id: 'B'}); // now 4 entries, but count should be 3 still
Tracker.flush();
test.equal(count, 3);
test.equal(unlimitedCount, 4); // unlimitedCount should be 4 now

c.stop();
});
Expand Down Expand Up @@ -3749,30 +3771,37 @@ Tinytest.add('minimongo - fine-grained reactivity of query with fields projectio
Tinytest.add('minimongo - reactive skip/limit count while updating', test => {
const X = new LocalCollection;
let count = -1;
let unlimitedCount = -1;

const c = Tracker.autorun(() => {
count = X.find({}, {skip: 1, limit: 1}).count();
unlimitedCount = X.find({}, {skip: 1, limit: 1}).count(false);
});

test.equal(count, 0);
test.equal(unlimitedCount, 0);

X.insert({});
Tracker.flush({_throwFirstError: true});
test.equal(count, 0);
test.equal(unlimitedCount, 1);

X.insert({});
Tracker.flush({_throwFirstError: true});
test.equal(count, 1);
test.equal(unlimitedCount, 2);

X.update({}, {$set: {foo: 1}});
Tracker.flush({_throwFirstError: true});
test.equal(count, 1);

test.equal(unlimitedCount, 2);

// Make sure a second update also works
X.update({}, {$set: {foo: 2}});
Tracker.flush({_throwFirstError: true});
test.equal(count, 1);

test.equal(unlimitedCount, 2);

c.stop();
});

Expand Down