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

Warn when using skip without sort #7579

Open
199911 opened this issue Mar 4, 2019 · 3 comments
Open

Warn when using skip without sort #7579

199911 opened this issue Mar 4, 2019 · 3 comments
Labels
developer-experience This issue improves error messages, debugging, or reporting
Milestone

Comments

@199911
Copy link

199911 commented Mar 4, 2019

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Populate with options, skip does not work when sort not defined
.populate({ options: {skip, limit} })

If the current behavior is a bug, please provide the steps to reproduce.
https://github.com/199911/mongoose-populate-bug/blob/master/index.js#L98

What is the expected behavior?
Story list have stories [A, B, C]
expect .populate({ options: {skip: 0, limit: 1} }) return A
expect .populate({ options: {skip: 1, limit: 1} }) return B
but .populate({ options: {skip: 1, limit: 1} }) still return A

Please mention your node.js, mongoose and MongoDB version.
Node,js 8, mongoose 5, MongoDB 4

@vkarpov15 vkarpov15 added this to the 5.4.20 milestone Mar 5, 2019
@vkarpov15 vkarpov15 added the has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue label Mar 5, 2019
@vkarpov15
Copy link
Collaborator

I took a closer look at this and you need to specify sort here, because MongoDB doesn't guarantee order of query results unless you specify sort, so skip without sort doesn't really make sense. Mongoose does send the correct query and handle the result correctly, it's just that the MongoDB server is returning an incorrect doc unless you explicitly sort.

    before('Get first story', async () => {
      const list = await StoryList
        .findById('333333333333333333333333')
        .populate({
          path: 'stories',
          options: {
            skip: 1,
            limit: 1,
            sort: { $natural: -1 }
          },
        }); 
      story = list.stories[0];
    });

@vkarpov15 vkarpov15 removed this from the 5.4.20 milestone Mar 23, 2019
@vkarpov15 vkarpov15 added won't fix and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Mar 23, 2019
@199911
Copy link
Author

199911 commented Apr 11, 2019

@vkarpov15
Understand this is not a flaw in Mongoose.
Can find some way to notify Mongoose users?

Maybe jotting down this problem in the API documents
or throw an error when calling populate() with skip but without sort?

@vkarpov15
Copy link
Collaborator

Worth considering for a future release

@vkarpov15 vkarpov15 reopened this Apr 13, 2019
@vkarpov15 vkarpov15 changed the title Populate with options, skip does not work when sort not defined Warn when using skip without sort Apr 13, 2019
@vkarpov15 vkarpov15 added developer-experience This issue improves error messages, debugging, or reporting and removed won't fix labels Apr 13, 2019
@vkarpov15 vkarpov15 added this to the Parking Lot milestone Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience This issue improves error messages, debugging, or reporting
Projects
None yet
Development

No branches or pull requests

2 participants