Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.79k stars 3.8k forks source link

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

Closed blowfishlol closed 6 days ago

blowfishlol commented 1 week ago

Prerequisites

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

vkarpov15 commented 1 week ago

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.