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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Aug 11, 2024
+49 −1
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2918f79
fix populate called with lean when multiple documents with same forei…
MohOraby Aug 9, 2024
0cb96b9
check if rawDocs[key] and val are instance of Document
MohOraby Aug 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}
key = String(_val);
if (rawDocs[key]) {
Expand All @@ -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 •
edited
Loading

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

// May need to store multiple docs with the same id if there's multiple models
// if we have discriminators or a ref function. But avoid converting to an array
// if we have multiple queries on the same model because of `perDocumentLimit` re: gh-9906
Expand Down
48 changes: 48 additions & 0 deletions test/model.populate.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11048,4 +11048,52 @@ describe('model: populate:', function() {
assert.equal(pet1.owner.name, 'Alice');
assert.equal(pet2.owner.name, 'Alice');
});

it('makes sure that populate works correctly with duplicate foreignField with lean(); (gh-14794)', async function() {
const authorSchema = new mongoose.Schema({
group: String,
name: String
});

const postSchema = new mongoose.Schema({
authorGroup: String,
title: String,
content: String
});

const Author = db.model('Author', authorSchema);
const Post = db.model('Post', postSchema);

await Author.create({ group: 'AUTH1', name: 'John Doe' });
await Author.create({ group: 'AUTH2', name: 'Jane Smith' });
await Author.create({ group: 'AUTH2', name: 'Will Jons' });

await Post.create({
authorGroup: 'AUTH1',
title: 'First Post',
content: 'Content 1'
});

await Post.create({
authorGroup: 'AUTH2',
title: 'Second Post',
content: 'Content 2'
});

const posts = await Post.find()
.populate({
path: 'authorGroup',
model: 'Author',
select: { _id: 1, name: 1 },
foreignField: 'group'
})
.select({ _id: 1, authorGroup: 1, title: 1 })
.lean();

for (const post of posts) {
assert.ok(post.authorGroup._id instanceof mongoose.Types.ObjectId);
assert.ok(typeof post.authorGroup.name === 'string');
}
assert.equal(posts.length, 2);
});
});