Uploaded image for project: 'Node.js Driver'
  1. Node.js Driver
  2. NODE-5932

BulkWriteResult show incorrect insertedIds on certain errors when using bulk insert (insertMany)

    • 2
    • Hide

      1. What would you like to communicate to the user about this feature?
      2. Would you like the user to see examples of the syntax and/or executable code and its output?
      3. Which versions of the driver/connector does this apply to?

      Show
      1. What would you like to communicate to the user about this feature? 2. Would you like the user to see examples of the syntax and/or executable code and its output? 3. Which versions of the driver/connector does this apply to?

      I am the one who opened NODE-5421 - which was fixed in 6.2.

      I encountered a different (but similar) scenario where the bug still occur.

      Use Case

      As a developer using the driver
      I want BulkWriteResult.insertedIds to always reflect ids of actually inserted documents to the DB,
      So that I could confidently rely on the information in this property.

      How to reproduce
      I will describe one way I found to reproduce it - it is VERY niche - but I believe it reflects the fact that this bug probably occurs upon other kinds of errors - and that fix for NODE-5421 might not be wholesome. So this repro scenario is, I believe, an example that exposes a more general problem (there are probably simpler ways to reproduce):

      Assuming you have a `collection` variable which is a db.collection.

      1. Define a document to insert copies of:

      const documentToInsert = { date: new Proxy(new Date(), {}) };
      

      2. We will need to duplicate/clone this document. `structuredClone` doesn't work (can't clone certain objects), so you need to use another deep cloning function such as lodash deep clone, I use the npm package: https://www.npmjs.com/package/lodash.clonedeep

      A function to duplicate the document:

      function duplicateDocuments(amount) {
          let count = amount;
          const documents = [];
          while (count-- > 0) {
              // 'clone' is the deep cloning function you use
              documents.push(clone(documentToInsert));
         }
         return documents;
      }
      
      async function run() {
          const manyDocuments = duplicateDocuments(10);
          await collection.insertMany(manyDocuments);
      }
      run();
      

      Result
      The bulk insert operation will fail with the error: 
      `MongoBulkWriteError: this is not a Date object`
      because the date object is wrapped in Proxy
      this is acceptable, but `BulkWriteResult.insertedIds` - shows all the ids of the documents, even though zero documents actually inserted to the DB.

      Expected result`BulkWriteResult.insertedIds` should reflect actual ids of documents which are inserted to the DB.

      I'm attaching screenshots of the error logs.


      Acceptance Criteria

      Implementation Requirements

      • Make the insertedIds map only include actually successfully inserted Ids
        • Should be accomplished by moving insertedIds map construction to after an operation is executed
      • Do not impact the performance of map construction.
        • Keep an eye on iterating the array of documents, should not need to repeatedly iterate

      Testing Requirements

      • Make a local client side error occur when running a bulk operation. Check that insertedIds does not include the failed batch ids
      • Check (at least locally) that insertMany performance is not impacted
        • Check an error-ed batch case

      Documentation Requirements

      • API docs should clarify what insertedIds is set to after an operation succeeds (and after it fails).

      Follow Up Requirements

      • None

            Assignee:
            Unassigned Unassigned
            Reporter:
            yuval.invoke@gmail.com Yuval Aloni
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Created:
              Updated: