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

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

merged 4 commits into from Oct 18, 2017

Conversation

carlevans719
Copy link
Contributor

This PR changes the default behaviour of cursor.count() on the client. Previously, .count() on the client would return the number of matching documents taking into account skip and limit. Now, it will ignore those options unless it is passed true when invoked. (i.e. cursor.count(true)). This is consistent with the way it works on the server.

Issue: #1201
Introduction of feature on server-side: 258b14a & bce4df9
Previous PR (incomplete): #5023
Test app: meteor-issue-1201

Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @carlevans719 - thanks for submitting this PR! Everything here looks great! I made one small nitpick suggestion about variable naming, but functionality wise we're really close here. The approach you've implemented that sets applySkipLimit to false by default on the client side (to line up with what's happening on the server side) seems like the way to go, but this will break backwards compatibility for some people (especially those using Minimongo as a client side only cache who are expecting the current applySkipLimit true functionality). We'll review this further in our issue triage meeting today, and get back to you shortly with an update. Thanks!

return this._getRawObjects({ordered: true}).length;
return this._getRawObjects({
ordered: true,
ignoreSkipLimit: !applySkipLimit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might make sense to just carry the applySkipLimit variable naming throughout, instead of switching (and inverting) it to ignoreSkipLimit. This would help simplify the code a bit and makes the logic easier to follow. Also, since applySkipLimit is a Mongo count() supported option, most people reviewing the code will already be familiar with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! I'll get it updated

By default cursor._getRawObjects() will set options.applySkipLimit to true, thereby honoring any skip / limit.
cursor.count() on the client now calls _getRawObjects with applySkipLimit set to false by default. See #1202
@carlevans719
Copy link
Contributor Author

Hey @hwillson. I've updated ignoreSkipLimit to applySkipLimit in _getRawObjects.
I've left the default behaviour of _getRawObjects as it was (applying skip / limit) because .fetch(), .forEach() etc... expect it to do this.

Let me know if you have any other suggestions & feel free to point me towards changelogs / documentation if it needs updating to highlight this breaking change

@hwillson
Copy link
Contributor

Thanks for making these changes @carlevans719! We talked about this a bit more, and have decided that maintaining backwards compatibility in this case is (unfortunately) more important than correctness. By giving people an option to change things we're at least providing a workaround to the broken behavior, and we can re-visit this in the future (potentially alongside other related breaking changes). Would you be able to flip things around so that applySkipLimit is true by default on the client side? This won't line up with the server side, but we'll make sure this is documented in the History.md file. Speaking of which, if you're able to add a small entry to the History.md explaining this change, that would be great!

@carlevans719
Copy link
Contributor Author

Makes sense @hwillson! Is the entry in History.md okay to go under v1.5.2.2 or a different/new heading?

@hwillson
Copy link
Contributor

If you could add a new heading called v.NEXT and add it under that, that would be great!

@carlevans719
Copy link
Contributor Author

@hwillson all sorted. How does that look?

@hwillson
Copy link
Contributor

Looks great @carlevans719 - thanks! As soon as the tests finish running, we should be all set here. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants