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

bulkWrite does not throw an error if all documents are invalid (ordered: false) #14572

Closed
2 tasks done
AlexBrohshtut opened this issue May 6, 2024 · 4 comments · Fixed by #14587
Closed
2 tasks done
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.
Milestone

Comments

@AlexBrohshtut
Copy link

AlexBrohshtut commented May 6, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

6.12.2

Node.js version

20.9.0

MongoDB server version

5.0

Typescript version (if applicable)

No response

Description

When using bulkWrite in Mongoose to insert multiple documents, if all documents fail validation (e.g., required fields missing), bulkWrite completes without throwing any errors. Instead, it returns a default result indicating success, which is misleading as no documents are written to the database due to validation errors.
It seems that the change that was expecting to fix the error on empty array insertion may be involved: 17c31b7

Steps to Reproduce

  1. Define a Mongoose model with required field validations.
  2. Use bulkWrite to insert multiple documents where all documents violate the validation rules.
  3. Observe that no error is thrown, and a default success response is returned.
const mongoose = require('mongoose');
const { Schema } = mongoose;

const userSchema = new Schema({
  name: { type: String, required: true }
});

const User = mongoose.model('User', userSchema);

async function testBulkWrite() {
  try {
    const result = await User.bulkWrite([
      {
        insertOne: {
          document: { name: '' } // This is invalid as `name` is required
        }
      },
      {
        insertOne: {
          document: { name: '' } // This is also invalid
        }
      }
    ], { ordered: false } );

    console.log('Bulk write result:', result);
  } catch (error) {
    console.error('Error during bulk write:', error);
  }
}

mongoose.connect('mongodb://localhost:27017/testdb', { useNewUrlParser: true, useUnifiedTopology: true })
  .then(() => testBulkWrite())
  .catch(err => console.error('Error connecting to MongoDB:', err));

Result:

Bulk write result: {
  result: {
    ok: 1,
    writeErrors: [],
    writeConcernErrors: [],
    insertedIds: [],
    nInserted: 0,
    nUpserted: 0,
    nMatched: 0,
    nModified: 0,
    nRemoved: 0,
    upserted: []
  },
  insertedCount: 0,
  matchedCount: 0,
  modifiedCount: 0,
  deletedCount: 0,
  upsertedCount: 0,
  upsertedIds: {},
  insertedIds: {},
  n: 0
}

Expected Behavior

bulkWrite should return an error or a response indicating that no documents were inserted due to validation failures.

@AlexBrohshtut
Copy link
Author

Proposed Solution for Handling All Validation Failures in bulkWrite

The issue occurs because bulkWrite does not properly handle cases where all operations are invalid due to failed validations. Here's a suggested change to the handling code to address this issue:

Code Change

if (validOps.length === 0) {
  if (validationErrors.length > 0 && validationErrors.length === ops.length) {
    let error = new Error("All operations failed validation.");
    error.mongoose = error.mongoose || {};
    error.mongoose.validationErrors = validationErrors;
    return cb(error);
  }

  return cb(null, getDefaultBulkwriteResult());
}

Explanation

This change introduces a check within the handling of bulkWrite results:

  • Check for No Valid Operations: It checks if there are no valid operations (validOps.length === 0).
  • Validate Against Total Operations: It then checks if the number of validation errors matches the total number of operations attempted (validationErrors.length === ops.length). This condition ensures that the error is thrown only when all operations fail due to validation errors.
  • Error Creation and Customization: If all operations fail, it constructs an Error with a specific message indicating the failure reason. It also attaches the validation errors to the error object under error.mongoose.validationErrors, providing detailed insight into what went wrong.
  • Handling Empty Operations: If there are no operations (bulkWrite([])), it continues to return the default result, ensuring that existing functionality for empty operation arrays is preserved without error.

This solution ensures that users of the library are correctly informed about operation failures due to validation issues, improving error handling transparency and aiding in debugging and error tracking in production environments.

@vkarpov15 vkarpov15 added this to the 6.12.9 milestone May 6, 2024
@IslandRhythms IslandRhythms added the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label May 7, 2024
@IslandRhythms
Copy link
Collaborator

const mongoose = require('mongoose');
const { Schema } = mongoose;

const userSchema = new Schema({
  name: { type: String, required: true }
});

const User = mongoose.model('User', userSchema);

async function testBulkWrite() {

  await mongoose.connect('mongodb://localhost:27017');
  await mongoose.connection.dropDatabase();
  try {
    const result = await User.bulkWrite([
      {
        insertOne: {
          document: { name: '' } // This is invalid as `name` is required
        }
      },
      {
        insertOne: {
          document: { name: '' } // This is also invalid
        }
      }
    ], { ordered: false } );

    console.log('Bulk write result:', result);
  } catch (error) {
    console.error('Error during bulk write:', error);
  }
}

testBulkWrite();

@vkarpov15
Copy link
Collaborator

Not throwing an error is expected behavior, you should opt in to throwing an error if any bulkWrite ops fail validation using throwOnValidationError option:

    const result = await User.bulkWrite([
      {
        insertOne: {
          document: { name: '' } // This is invalid as `name` is required
        }
      },
      {
        insertOne: {
          document: { name: '' } // This is also invalid
        }
      }
    ], { ordered: false, throwOnValidationError: true } );

There's a bug where we don't throw an error if all operations fail validation even if throwOnValidationError is set, there's a fix for that in #14587 that we will ship in our next release

@AlexBrohshtut
Copy link
Author

Good, it seems like it does the job 👍🏻

vkarpov15 added a commit that referenced this issue May 16, 2024
…g MongooseBulkWriteError if all valid operations succeed in bulkWrite() and insertMany()

Backport #13410
Backport #14587
Fix #14572
vkarpov15 added a commit that referenced this issue May 17, 2024
fix(model): make `bulkWrite()` and `insertMany()` throw if `throwOnValidationError` set and all ops invalid
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
3 participants