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

Introduce populate ordered option for populating in series rather than in parallel #15231

Merged
merged 3 commits into from
Feb 6, 2025

Conversation

vkarpov15
Copy link
Collaborator

Summary

Currently Mongoose executes all populate queries in parallel, which is problematic for transactions. Re: #15091, #15100, etc., MongoDB server does not support multiple operations in parallel within a single transaction. With this PR, you can populate with ordered: true as follows

Fix #15210

// Document.prototype.populate() with multiple paths
await docD
  .populate({
          path: ['refA', 'refB', 'refC'], 
          ordered: true
  })

// query.populate() with options
await Model.find().populate([{ path: 'refA', ordered: true }, { path: 'refB', ordered: true }]);

A couple of noteworthy caveats:

  1. The options passed to populate() is always an array, because ordered is the only option we have that configures the top-level populate() call, as opposed to the individual _populatePaths() calls. That makes it somewhat tricky to pass ordered as a top-level option: assigning ordered as a property on the paths array is a bit error prone because it is easy to copy the array and lose ordered. So we made it so that we treat top-level populate as ordered if any of the populate paths are set to ordered, which seems like a reasonable workaround.
  2. Before this PR, you couldn't pass an array of strings as populate path option. This PR allows passing an array of strings as path.
// This is fine in Mongoose 8.9, will populate `refA`, `refB`, `refC` as expected
await docD
  .populate({
          path: 'refA refB refC'
  })

// In Mongoose 8.9, the following would throw an error. With this PR, the following will be equivalent to the above
await docD
  .populate({
          path: ['refA', 'refB', 'refC'],
  })

This PR may be helpful for #13985, but I need to double check.

Examples

@vkarpov15 vkarpov15 added this to the 8.11 milestone Feb 4, 2025
Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Code looks good, but there should be tests for this like other ordered operations, right?

@vkarpov15
Copy link
Collaborator Author

I would like to have tests for the populate ordered option, but testing is very tricky because we don't have a good way to ensure that the populate queries run in series rather than in parallel. Running queries in parallel in a transaction does not fail consistently. The only option I can think of right now is adding a $where to each populate to make each query slow, and assert that the full populate() is sufficiently slow, but that is slow and brittle.

@hasezoey
Copy link
Collaborator

hasezoey commented Feb 6, 2025

I would like to have tests for the populate ordered option, but testing is very tricky because we don't have a good way to ensure that the populate queries run in series rather than in parallel

I can understand that it might be tricky to correctly test the behavior that this code path was made for, but with the way it currently is, it adds a diverging code path which, to my knowledge, is currently completely untested.
Personally i would just at least add some test that tests that this code path gives some correct result (and does not like run into a TypeError or something).

@vkarpov15
Copy link
Collaborator Author

That's fair, I added a test case in 5d2a5a4

@vkarpov15 vkarpov15 merged commit 9d68b0a into master Feb 6, 2025
78 checks passed
@hasezoey hasezoey deleted the vkarpov15/gh-15210 branch February 6, 2025 21:26
@hasezoey hasezoey restored the vkarpov15/gh-15210 branch February 6, 2025 21:26
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.

Adding a session to an existing doc sometimes crashes population
2 participants