bulkSave doesn't throw an error even if one of the submitted documents was not updated

[ksyd9821]

Prerequisites

• I have written a descriptive issue title

Mongoose version

8.x.x

Node.js version

20.11

MongoDB version

7.0

Operating system

None

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

No response

Issue

Hi all,

While troubleshooting we ran into some unexpected behavior of the bulkSave function.
If one of the submitted documents is outdated (has a lower version than the copy on the DB), this operation will not throw any error, even though the document won't be updated.
We expected it to behave similarly to the save function, which throws a VersionError in these cases.

Repro:

    public async foo() {
        const fooSchema = new mongoose.Schema({
            bar: { type: Number },
        });
        const model = mongoose.model("foo", fooSchema);
        await model.deleteMany();

        const foo = await model.create({
            bar: 0
        });

        // update 1
        foo.bar = 1;
        await foo.save();

        // parallel update
        const fooCopy = await model.findById(foo._id);
        fooCopy.bar = 99;
        await fooCopy.save();

        // update 2 - this should throw a version error
        foo.bar = 2;
        // await foo.save(); // throws a VersionError
        await model.bulkSave([foo]) // no error thrown, but update not applied

        // log result
        console.log(`bar: ${(await model.findById(foo._id)).bar}`); // logs 99
    }

Is this the expected behavior of bulkSave? Is there any possibility of making it fail if one of the documents has been already replaced by a newer version instead of returning silently?

[github-actions]

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

[vkarpov15]

As written, neither await foo.save() nor await model.bulkSave([foo]) throw a VersionError, and the final result is 2 not 99. In general, versioning in Mongoose only applies to array fields by default. However, the following script demonstrates the issue you're seeing (optimistic concurrency enabled)

const mongoose = require('mongoose');

require('util').inspect.defaultOptions.depth = 10;

mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

void async function main() {
        const fooSchema = new mongoose.Schema({
            bar: { type: Number },
        }, { optimisticConcurrency: true });
        const model = mongoose.model("foo", fooSchema);
        await model.deleteMany();

        const foo = await model.create({
            bar: 0
        });

        // update 1
        foo.bar = 1;
        await foo.save();

        // parallel update
        const fooCopy = await model.findById(foo._id);
        fooCopy.bar = 99;
        await fooCopy.save();

        // update 2 - this should throw a version error
        foo.bar = 2;
        // await foo.save(); // throws a VersionError
        await model.bulkSave([foo]) // no error thrown, but update not applied

        // log result
        console.log(`bar: ${(await model.findById(foo._id)).bar}`); // logs 99
}();

[vkarpov15]

So long story short, there's no way for bulkSave() to know which documents failed to save due to concurrency issues. MongoDB bulkWrite() doesn't return how many documents each update operation updated. So if you bulkSave() 3 docs, and MongoDB server tells us that 2 docs matched, there's no way for Mongoose to know which 2 docs matched and which one didn't. All we get back from bulkWrite() is matchedCount, which contains the total number of documents matched by all the update operations.

@AbdelrahmanHafez @hasezoey what do you think about this one? There's a couple of options:

1. Make bulkSave() throw an error if matchedCount is 0: that way we know all documents failed to update
2. Make bulkSave() throw an error if matchedCount isn't equal to the number of documents being saved. In that case, we know at least one document wasn't found.
3. Keep the current behavior: bulkSave() never throws a DocumentNotFoundError or VersionError, but we should make a note in the docs that bulkSave() is unable to report versioning errors

[ksyd9821]

@vkarpov15 yes, you are right. I forgot to mention that we have optimistic concurrency enabled by default on all schemas through a plugin.

Not sure if this can help, but as a workaround we went with option 2 combined with a transaction. So if matchedCount doesn't match the number of submitted documents, we rollback the transaction and throw a VersionError.

[hasezoey]

@AbdelrahmanHafez @hasezoey what do you think about this one? There's a couple of options:

i dont fully know how bulkSave is meant to work, so option 1 sound good to me, as long as update and new document operations are taken into account (are they both taken into account in matchedCount?).
as for option 2, it also sounds good, but i think that would be better to be put as a option to get the old behavior, along the lines of throwOnCountMismatch?

So if you bulkSave() 3 docs, and MongoDB server tells us that 2 docs matched, there's no way for Mongoose to know which 2 docs matched and which one didn't.

does this returns some kind of error in bulkSave (like WriteError via BulkSaveResult.getWriteErrors) or are those mismatches silent in the mongodb driver?

[AbdelrahmanHafez]

@hasezoey
bulkSave is just a good old bulkWrite under the hood, so MongoDB doesn't return this as an error, { filter: { _id: 'A', __v: 1 } } just a filter that, in this scenario, doesn't match anything as far as mongo is concerned.

I'm a bit sad that none of the solutions are robust enough to provide the id(s) of the failing documents, and there's no way we can handle that from mongoose side.

I'm leaning towards a combination of the second and third options. We make a note in the docs about this behavior, and have a global option/bulkSave option throwOnCountMismatch like @hasezoey suggests.

What do you think?

[vkarpov15]

I put in a PR for option (1) since that is the least brittle - in that case, we know all documents failed so we shouldn't update any $isNew or run any normal post('save') hooks (although we should actually execute post save error middleware for bulkSave()).

Right now I'm thinking a combination of option (1) and option (2). In the option (1) case we know all documents failed, so we know we can handle that correctly. In the case where we have a mismatch, we don't know which documents were updated, so we don't know which documents to update or run middleware on, so we should throw an error. Likely a different class of error than DocumentNotFound though.

I'm not necessarily a fan of the throwOnCountMismatch option, I don't like adding too many extra one-off options, but I'm not sure I have a good alternative other than not adding any option at all. None of our existing options line up well.