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

C.find(q, {limit: N}).count() inconsistent between server and client #1201

Closed
mitar opened this Issue Jul 3, 2013 · 26 comments

Comments

Projects
None yet
10 participants
@mitar
Collaborator

mitar commented Jul 3, 2013

When comparing how .count() works in Meteor and MongoDB I found a difference. In MongoDB default is that .count() returns the count of all elements in the cursor, no mater if skip or limit was used. But in Meteor this is not true. As a consequence, it seems impossible to get number of all results in find in Meteor.

See MongoDB documentation.

The issue is if I want to provide pagination. I would like to know how many were all results, but then send over the wire only a page of the results.

@timhaines

This comment has been minimized.

Contributor

timhaines commented Jul 3, 2013

Are you talking about Meteor's count on the client only? Or does it apply the skip and limit on the server count method too?

As your work around you can easily strip the skip and limit before executing the count can't you?

@mitar

This comment has been minimized.

Collaborator

mitar commented Jul 3, 2013

I tried it on the client. So without limit there were more results counted and with limit there were less. It should in both cases return the same value, in my opinion.

But on the server side there is another issue. If we see the count room example, or any other publication where I want reactive number/count of results in the query to be send to the client. How is this possible to achieve? If I use limit and observeChanges, added is triggered only limit number of times. Is should then create two queries, one with limit and one without? But just to count (a possibly huge number of collections), I don't really want to processes all added events individually. The room count example probably fails for queries with huge number of results.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jul 3, 2013

So for the server side there should be a way to call something like .countObserve() and get an easy way to be notified when the number of results for the query changes.

@glasser

This comment has been minimized.

Member

glasser commented Oct 15, 2013

I updated the issue title to be clear that this differs between server (where limit is ignored by count) and minimongo.

Apparently Mongo shell and Node Mongo driver added a applySkipLimit option to count at some point... we could do that too.

@mitar

This comment has been minimized.

Collaborator

mitar commented Oct 15, 2013

Yes, please allow for the option. I imagine cases when either would be useful. But in any case it has to be in sync between client and server.

@davidworkman9

This comment has been minimized.

Contributor

davidworkman9 commented Jun 2, 2014

It would be nice to have the applySkipLimit property, but I think it's more important that this at least be the same on the client and the server.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jan 22, 2015

The main issue is not that the option is not implemented, but that behavior between server and client is different. Minimongo behaves like applySkipLimit is set to true, while server side queries like applySkipLimit is set to false. So if you run the same query both on the client and server side you get differences.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jan 22, 2015

To see what I am talking about, run minimongo - basics tests on the server. Basic tests will fail.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jan 30, 2015

I made this test package, which runs Minimongo tests on the server. It exposes this issue as one of the failed tests.

@glasser

This comment has been minimized.

Member

glasser commented Mar 6, 2015

Unfortunately, it doesn't look like I got to this as part of the 1.0.4 maintenance work.

@RobertLowe

This comment has been minimized.

Contributor

RobertLowe commented Mar 12, 2015

@glasser D'aw, I wanted to see this! applySkipLimit would be great for publish-counts I'm using. Maybe for 1.0.5?

I feel keeping things 1-to-1 with Mongo is pretty important priority.

I'll just make another workaround in the meantime. 😞

@jamesgibson14

This comment has been minimized.

jamesgibson14 commented Jul 23, 2015

I would love for my minimongo code Collection.find({},{limit:10}).count() to have the same out put on both client and server.

okland added a commit to okland/meteor that referenced this issue Oct 14, 2015

Fix issue meteor#1201
Allow count apply skip limit in mongo driver
@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 21, 2016

Fixed in bce4df9

@laosb laosb closed this Jun 21, 2016

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 21, 2016

What about on the client side? There is no way to control it on the client side, no?

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 21, 2016

See #5023 pull request and discussion there.

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 21, 2016

I didn't look at all the comments carefully and closed this seeing @okland added a commit to okland/meteor that referenced this issue on 15 Oct 2015, sorry!

Should I reopen this or track it in #5023?

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 21, 2016

I think that #5023 is a pull request which does not even have source repository anymore. That one should probably be closed, and this one kept around until we address this also on the client side.

BTW, are there any tests added for bce4df9?

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 21, 2016

Yeah, I'd reopen this. As #5023 lost its source repo so no further updates can be made, we should close it.

@laosb laosb reopened this Jun 21, 2016

tmeasday added a commit that referenced this issue Jul 20, 2016

Allow passing `applySkipLimit` to count and default it to false.
Not currently supported in minimongo, so I'm not sure we want to
document it yet. Supporting it (and making it consistent) in MM
will take a little bit of work:
#1201
@titovanton

This comment has been minimized.

titovanton commented Apr 19, 2017

Hmmm. First of all, for a user, who is familiar with MongoDB, the behavior of your counting - is not obviose. And it makes me fill strange: what did you think about when prefer: Blabla.find().count(), instead of original db.collection.count(query, options)? And applySkipLimit - looks like ugly crutch, not less! :/

@hwillson

This comment has been minimized.

Member

hwillson commented Apr 26, 2017

Hi all - to help move this along, I've marked this issue as pull-requests-encouraged. PR #5023 was started to address this issue, but was closed due to inactivity. If anyone in the community is interested in blowing the dust off things and submitting a new PR, it will definitely be reviewed in a timely manner, and be considered for merging. Thanks!

@hwillson hwillson removed this from the Winter 2015 Mongo Maintenance milestone Sep 25, 2017

@carlevans719

This comment has been minimized.

Contributor

carlevans719 commented Oct 10, 2017

I'm happy to pick this up. Is the consensus still that we will be changing the default behaviour of .count() on the client so that it is consistent with the way it works on the server?

Edit: created #9205

@hwillson

This comment has been minimized.

Member

hwillson commented Oct 19, 2017

#9205 was merged, so this functionality will be coming soon. Thanks @carlevans719 et. al!

@hwillson hwillson closed this Oct 19, 2017

@mitar

This comment has been minimized.

Collaborator

mitar commented Oct 19, 2017

I do not think this should be closed. By default, client and server are still inconsistent, as this issue is claiming. #9205 is just adding an option to change this. This makes isomorphic code hard to write.

@hwillson

This comment has been minimized.

Member

hwillson commented Oct 19, 2017

Thanks @mitar - yes, we went back and forth on this one. For now this approach is the most flexible while still maintaining backwards compatibility (in particular in the case where people are using minimongo without mongo, for client side caching, etc.). This new option will show up in the docs, so people who want to ultimately enable the correct behavior can. At some point this will be switched around (so we're passing false into count() by default), but the consensus was not yet (given the impending release of 1.6). That being said, we don't want to lose sight of this, so keeping this open sounds like the way to go.

Just to re-itereate though to anyone coming to this issue. PR #9205 introduces the ability to fix this manually by passing false into your client side cursor count() - e.g. someCollection.find().count(false).

@hwillson hwillson reopened this Oct 19, 2017

@mitar

This comment has been minimized.

Collaborator

mitar commented Oct 19, 2017

Yes, this is great progress and the major step! Thanks!

@hwillson

This comment has been minimized.

Member

hwillson commented Mar 6, 2018

While we think resolving this issue would be a great addition to the Meteor project, we're going to close it for now due to inactivity (see the bug issue lifespan section of Meteor's traige doc for more information). If anyone comes across this issue in the future, and is interested in working on resolving it, please let us know by posting here and we'll consider re-opening this issue. Thanks!

@hwillson hwillson closed this Mar 6, 2018

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