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

Include merge options in chain find count #386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Include merge options in chain find count #386

wants to merge 1 commit into from

Conversation

Tape
Copy link
Contributor

@Tape Tape commented Nov 13, 2013

This change will not affect any existing results, however the use of such a feature looks like it's only compatible with SQL databases. I included a test case but I could not get it to pass on MongoDB (as such I have commented it out, but these tests will pass on MySQL, Postgres, and SQLite).

I was doing a chain find filter to do 2 separate queries: 1 would fetch a count of all results, one would use the exact same chain find data and do a count on it (for pagination reasons). While the regular .find() was returning the correct results (8 results), the count would return incorrect results since the merge information was absent (24 results), throwing off the pagination count data. This simple change will take that into account.

@dxg
Copy link
Collaborator

dxg commented Jan 16, 2014

I took a look at this today.
The test passes, but there is a problem; the test also passes if I remove your changes from ChainFind.js.
Can you please tweak the test so that it fails once your changes are removed from ChainFind? Thanks.

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

2 participants