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

fix populate called with lean (gh-14794) #14799

Merged
merged 2 commits into from
Aug 11, 2024

Conversation

MohOraby
Copy link
Contributor

@MohOraby MohOraby commented Aug 9, 2024

Summary

in #14794 due to being called with lean() there's no _doc object so that throws an error

another issue here is documents with duplicate foreignField so the rawDocs[key] already exists and its value is being overwritten.

this change shouldn't affect (gh-14759) and its test case still runs successfully.

another solution would be to check for rawDocs[key]._id and val._doc first using the following statement instead

(rawDocs[key]._doc && String(rawDocs[key]._doc._id)) !== (val._doc && String(val._doc._id))) 

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

A couple of quick changes, #14759 made us very wary of cases where we may accidentally be checking for quality on _id after getters applied, so we want to avoid undoing the val._doc._id changes in cases where we know val instanceof Document.

Thanks for finding the issue

lib/model.js Outdated
@@ -4459,7 +4459,7 @@ function _assign(model, vals, mod, assignmentOpts) {
}
} else {
if (_val instanceof Document) {
_val = _val._doc._id;
_val = _val._id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change isn't necessary. L4462 will never run if lean.

lib/model.js Outdated
@@ -4468,7 +4468,7 @@ function _assign(model, vals, mod, assignmentOpts) {
rawOrder[key].push(i);
} else if (isVirtual ||
rawDocs[key].constructor !== val.constructor ||
String(rawDocs[key]._doc._id) !== String(val._doc._id)) {
String(rawDocs[key]._id) !== String(val._id)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we expand this change to check if rawDocs[key] and val are instanceof Document, and, if so, use val._doc._id, otherwise val._id?

Copy link
Contributor Author

@MohOraby MohOraby Aug 10, 2024

Choose a reason for hiding this comment

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

Ok, for this would you prefer it be a ternary operator inside the else if check like this

(rawDocs[key] instanceof Document ? String(rawDocs[key]._doc._id) : String(rawDocs[key]._id)) !== (val instanceof Document ? String(val._doc._id) : String(val._id))

which looks kind of long and hard to read or move them to constant above the if check like this

const rawDocId = rawDocs[key] instanceof Document ? rawDocs[key]._doc._id : rawDocs[key]._id;
const valDocId = val instanceof Document ? val._doc._id : val._id;
// rest of the code
String(rawDocId) !== String(valDocId)

both are fine and would do the same thing, I just don't wanna keep pushing commits since the CI would rerun

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for the the first approach currently, just to move things forward, if it needs refactoring please let me know

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

See previous comments, I accidentally submitted review with "Comment" selected rather than "Request changes"

@vkarpov15 vkarpov15 added this to the 8.5.3 milestone Aug 11, 2024
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@vkarpov15 vkarpov15 merged commit f98b463 into Automattic:master Aug 11, 2024
24 checks passed
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.

2 participants