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

Feature: Add forAll for cursor queries #9861

Closed
khaledosama999 opened this issue Jan 25, 2021 · 5 comments
Closed

Feature: Add forAll for cursor queries #9861

khaledosama999 opened this issue Jan 25, 2021 · 5 comments
Labels
discussion If you have any thoughts or comments on this issue, please share them!

Comments

@khaledosama999
Copy link
Contributor

khaledosama999 commented Jan 25, 2021

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

Feature Description

For cursor queries (that return the data in batches) currently there is a method forEach which is used to loop over each document returned from the query not each batch. it would be nice to loop over each batch so you can perform a bulk action on each batch.

Use Case

You want to send an email to all users currently in your system obviously you will fetch the whole user collection in batches. But it's not very efficient to send each user an email one at a time that wouldn't be very cost effective plus any mail service has a limit on the max number of bulk emails that could be sent (1000 for example). So the ideal situation would be to fetch the users in batches of 1000 and for each batch perform a bulk action of sending an email to all those users.

I can make the PR myself but I will wait for the issue approval

@vkarpov15
Copy link
Collaborator

I have a few issues with this. First, finding a good name: batchSize already means something else. Second, it isn't particularly difficult to do this with eachAsync:

let batch = [];
const batchSize = 1000;

await Users.find().eachAsync(async function(user) {
  batch.push(user);
  if (batch.length >= batchSize) {
    await sendEmailToUsers(batch);
    batch = [];
  }
});

@vkarpov15 vkarpov15 added the discussion If you have any thoughts or comments on this issue, please share them! label Jan 28, 2021
@khaledosama999
Copy link
Contributor Author

khaledosama999 commented Jan 29, 2021

That's exactly what I did to solve it :D, but for me here's why I think it would be nice to have it as part of mongoose:

  • From a functional programming point of view the function passed to eachAsync is impure since it depends on the batch array, so it could introduce all kinds of weird behavior if the user is not careful when it comes to the usage of the batch array outside of the function passed to eachAsync .

  • Although the solution might seem easy I don't think it will occur to everybody especially beginners, but if mongoose has a built in function and it's mentioned in the docs that would make it a lot easier.

  • And lastly this seems more readable and cleaner to me :

await Users
.find()
.batchSize(1000)
.eachBatch(async function (batch) {
await sendEmailToUsers(batch);
});

@khaledosama999
Copy link
Contributor Author

@vkarpov15

@vkarpov15
Copy link
Collaborator

I think a batchSize option to eachAsync() would work if you're willing to put in a PR 👍

@khaledosama999
Copy link
Contributor Author

@vkarpov15 Done I don't know exactly how you manage features but the feature branch was based on branch 6.0 and the PR is on branch 6.0 also I didn't want to come near the master branch as I'm assuming this is the branch used for the latest mongoose version

vkarpov15 added a commit that referenced this issue Feb 15, 2021
feat: Add batch processing for eachAsync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them!
Projects
None yet
Development

No branches or pull requests

2 participants