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

perf(model): make insertMany() lean option skip hydrating Mongoose docs #14376

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

vkarpov15
Copy link
Collaborator

Fix #14372

Summary

Initial implementation of lean for insertMany in #8507 made lean just skip validation, not hydrating docs, which makes insertMany with lean still significantly slower. I'm inclined to change it so that insertMany with lean also skips hydrating Mongoose docs, because the name lean implies that we skip all hydration and other Mongoose-specific stuff similar to query lean.

Examples

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.

Looks ok, but i will hold-off on approving until the main 6.x branch does not have failing tests anymore

@vkarpov15
Copy link
Collaborator Author

@hasezoey can you please re-review? This PR has fix for test issues in 6.x.

@vkarpov15 vkarpov15 merged commit 0af28e0 into 6.x Feb 28, 2024
34 checks passed
@vkarpov15 vkarpov15 deleted the vkarpov15/gh-14372 branch February 28, 2024 19:28
@AbdelrahmanHafez
Copy link
Collaborator

@vkarpov15 @hasezoey
If I understand this correctly, would this mean that documents created by insertMany and lean options would bypass the casting step? If that is true, then that feels like a foot-gun, and might be useful to have a big warning for that behavior.

If not, then LGTM 😁

@hasezoey
Copy link
Collaborator

hasezoey commented Mar 2, 2024

If I understand this correctly, would this mean that documents created by insertMany and lean options would bypass the casting step?

i guess that would be true, but isnt lean's whole point that it basically returns what the driver returns (/ like when using mongodb directly) instead of going through mongoose's steps?

also the documentation already mentions it:

The lean option tells Mongoose to skip hydrating the result documents
...
The downside of enabling lean is that lean docs don't have:

Change tracking
Casting and validation
Getters and setters
Virtuals
save()

source

@AbdelrahmanHafez
Copy link
Collaborator

Yes, and that's fine in the case of reads, assuming that it's mostly used in the context of reading and then returning in a JSON response.

But in the case of writes, very often data is not cast to the proper types, and it would be stored in the database with the wrong type. I feel like people would use that option thinking that it's only a lighter way to "return" the data, unaware of what it means under the hood. 🤔

@vkarpov15
Copy link
Collaborator Author

I can see @AbdelrahmanHafez 's point, but I'm not sure I fully agree. Even before this change, lean would bypass validation, so it would still potentially lead to storing invalid data. I can see the justification for adding a warning in the docs that lean with insertMany skips casting and validation entirely, but I don't think adding a casting step by default is beneficial. Users can just use Model.castObject() to cast, like TestModel.insertMany(docs.map(doc => TestModel.castObject(doc)), { lean: true })

@AbdelrahmanHafez
Copy link
Collaborator

Yeah, I am not referring to this change as the concern, it's generally having the lean option with insertMany that skips the casting step.

Now that I think about it, a warning would be enough, especially that we have castObject.

vkarpov15 added a commit that referenced this pull request Mar 5, 2024
vkarpov15 added a commit that referenced this pull request Mar 7, 2024
docs(model): add extra note about `lean` option for `insertMany()` skipping casting
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