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

Cursor.count doesn't work on a cursor with a limit specified #654

Closed
timhaines opened this Issue Jan 30, 2013 · 11 comments

Comments

Projects
None yet
6 participants
@timhaines
Contributor

timhaines commented Jan 30, 2013

Minimongo assumes a count is unordered, even if an order is specified.

When I try and do a count with a limit specified, an error occurs "must use ordered observe with skip or limit". This is thrown in _observeInternal.

My use case is that I want to only display 1 page of results (limit of 10 documents). Then I want to run a count with limit + 1 to determine if there's more records to show and a more button should be displayed.

To work around around this for now, I'm running another find with limit set to the original limit + 1, and then looking at the length of the results.

@gschmidt

This comment has been minimized.

Member

gschmidt commented Feb 5, 2013

Thank Tim, nice catch!

I think this is as simple as modifying LocalCollection.Cursor.prototype.count so that the ordered flag to _markAsReactive is true if the cursor has an order specified, or otherwise false. This would be a great starter project for someone that would like to get some experience with Meteor internals and get a bugfix into Meteor -- I'd love to see a pull request that made this fix and also included a test. Accordingly I've tagged this good-starter-project.

Otherwise, I've added this to the core team's bugfix queue and we will get to it when we can. If it is important to you, let me know and I'll bump up the priority. Your workaround (calling find instead of count) will pretty much do the same thing, though.

@belisarius222

This comment has been minimized.

belisarius222 commented Feb 5, 2013

I submitted a pull request in response to this: #665

@glasser

This comment has been minimized.

Member

glasser commented Feb 5, 2013

As a note though: this code is somewhat changed on the ddp-pre1 branch.
On Feb 4, 2013 8:29 PM, "Ted Blackman" notifications@github.com wrote:

I submitted a pull request in response to this: #665#665


Reply to this email directly or view it on GitHubhttps://github.com//issues/654#issuecomment-13114177.

@glasser

This comment has been minimized.

Member

glasser commented Feb 5, 2013

As a note, this code has somewhat changed on the ddp-pre1 branch.
On Feb 4, 2013 4:14 PM, "gschmidt" notifications@github.com wrote:

Thank Tim, nice catch!

I think this is as simple as modifying
LocalCollection.Cursor.prototype.count so that the ordered flag to
_markAsReactive is true if the cursor has an order specified, or otherwise
false. This would be a great starter project for someone that would like to
get some experience with Meteor internals and get a bugfix into Meteor --
I'd love to see a pull request that made this fix and also included a test.
Accordingly I've tagged this good-starter-project.

Otherwise, I've added this to the core team's bugfix queue and we will get
to it when we can. If it is important to you, let me know and I'll bump up
the priority. Your workaround (calling find instead of count) will pretty
much do the same thing, though.


Reply to this email directly or view it on GitHubhttps://github.com//issues/654#issuecomment-13107961.

@belisarius222

This comment has been minimized.

belisarius222 commented Feb 5, 2013

The ddp-pre1 branch seems not to have this bug, as far as I can tell. I copied over the tests I used for the devel branch into ddp-pre1, and they worked right off the bat.

https://github.com/belisarius222/meteor-cursor-count-fix/commit/ee49938136a454ed199cd880b03c99f917c2110f

My understanding of this is that it makes sense that ddp-pre1 doesn't have the bug because it's running the _observeChanges function, which checks whether it should run observe or observeUnordered:
https://github.com/meteor/meteor/blob/ddp-pre1/packages/minimongo/minimongo.js#L783

@timhaines

This comment has been minimized.

Contributor

timhaines commented Mar 18, 2013

This has been fixed (probably in 0.5.7 when ddp-pre1 was merged). Closing.

@timhaines timhaines closed this Mar 18, 2013

@timhaines

This comment has been minimized.

Contributor

timhaines commented Mar 18, 2013

Actually, this isn't resolved. Count raises: Exception from Deps recompute: Error: must use ordered observe with skip or limit

With selector: {field1: false, field2: null} , and options: {limit: 11, sort: {createdAt: -1}}

MyCollection.find(selector, options).fetch().length() works.

MyCollection.find(selector, options).count() doesn't work when in the app source.

However, running MyCollection.find(selector, options).count() works in the console for some reason. It makes me wonder if I'm messing something up.

@timhaines timhaines reopened this Mar 18, 2013

@ghost

This comment has been minimized.

ghost commented Mar 25, 2013

I have the same issue. @timhaines have you solved your problem?

@owenrh

This comment has been minimized.

owenrh commented Jul 17, 2013

+1

@glasser

This comment has been minimized.

Member

glasser commented Sep 12, 2013

Fixed on devel: 49105c3

@glasser glasser closed this Oct 15, 2013

@mitar

This comment has been minimized.

Collaborator

mitar commented Dec 15, 2013

See #1201 as well. Lack of applySkipLimit option to count() I think is the main culprit here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment