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

Model.bulkSave() and setting the query with "save" and "bulkWrite" pre middleware causes path conflict #14722

Closed
1 task done
blowfishlol opened this issue Jul 3, 2024 · 1 comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@blowfishlol
Copy link

blowfishlol commented Jul 3, 2024

Prerequisites

  • I have written a descriptive issue title

Mongoose version

8.4.4

Node.js version

18.19.0

MongoDB version

6.0.15 Community

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.4.1 (23E224)

Issue

I have the following code that have both save and bulkWrite pre-hooks installed to a schema:

const mongoose = require('mongoose');

const getUser = () => ({
    _id: new mongoose.Types.ObjectId("66852317541ef0e22ae5214c"),
    name: "blowfishlol",
    email: "blowfish@test.com"
});

const updateInfoSchema = new mongoose.Schema({
    name: {
        type: String, required: true
    },
    email: {
        type: String,
        required: true
    }
}, {
    versionKey: false,
});
const schema = new mongoose.Schema({ name: String, updater: updateInfoSchema });
schema.pre(["updateOne", "updateMany", "findOneAndUpdate", "save"], function (next) {
    this.set("updater", getUser());
    next();
});
schema.pre("bulkWrite", function (next, ops) {
    for (const op of ops) {
        op.updateOne.update["$set"].updater = getUser();
    }
    next();
});

const main = async () => {
    await mongoose.connect('mongodb://localhost:27017/mongoose_test');

    const TestModel = mongoose.model('Test', schema, "tests");

    const doc1 = new TestModel({ name: 'foobar', updater: getUser() });
    const doc2 = new TestModel({ name: 'bazinga', updater: getUser() });
    await doc1.save();
    await doc2.save();


    const docs = await TestModel.find().exec();
    for (const doc of docs) {
        doc.name = doc.name + "edit";
    }
    await TestModel.bulkSave(docs);

    const result = await TestModel.find().exec();

    console.log('Result', result.map(doc => doc.toObject()));
    await mongoose.connection.collection("tests").drop();
};

main();

In the example above, i will get an error:

Uncaught MongoBulkWriteError MongoBulkWriteError: Updating the path 'updater' would create a conflict at 'updater'
    at handleWriteError (/Users/blowfish/Source/finway/laboratory/mongoose-test/node_modules/mongodb/lib/bulk/common.js:826:22)
    at resultHandler (/Users/blowfish/Source/finway/laboratory/mongoose-test/node_modules/mongodb/lib/bulk/common.js:303:27)
    at <anonymous> (/Users/blowfish/Source/finway/laboratory/mongoose-test/node_modules/mongodb/lib/bulk/common.js:346:116)
    at processTicksAndRejections (internal/process/task_queues:95:5)

This happens because in the bulkWrite pre-hook, im assigning $set.updater as getUser() when the op already contains updatedBy._id , which is set by the "save" hook:

After the save prehook, before the bulkWrite prehook
image

And after the the bulkWrite prehook is done
image

Is there any i can stop the bulkWrite hook from firing if i save using bulkSave()? Im aware bulkSave will fire both save and uses bulkWrite under the hood.

Or i should just do a check in the bulkWrite middleware to see if the ops already contains the updater._id?

Other thing that i realised when testing around is that adding _id explicitly into the updateInfoSchema will not cause this issue, since the ops inside the bulkWrite prehook does not contain the udpate to updater._id

@blowfishlol blowfishlol added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary help wanted labels Jul 3, 2024
@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. and removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary help wanted has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Jul 4, 2024
@vkarpov15
Copy link
Collaborator

The fundamental issue here is that this.set("updater", getUser()); leaves updater._id in default state, even though _id wasn't modified, because of how Mongoose applies defaults to new subdocuments before setting any values. We've found a workaround for that, but that workaround causes some issues with #4145 that we're working through.

vkarpov15 added a commit that referenced this issue Jul 9, 2024
Avoid leaving subdoc defaults on top-level doc when setting subdocument to same value
@vkarpov15 vkarpov15 added this to the 8.4.6 milestone Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Projects
None yet
Development

No branches or pull requests

2 participants