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

Cursor.hasNext should not transform documents

    • 3
    • Not Needed
    • v4.x
    • Not Needed
    • 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?

      Use Case

      As a driver user,
      I want cursor.hasNext not to modify any cursor documents,
      So that I can iterate a cursor using hasNext and next without double transforming documents.

      There are two related bugs here (both reproduced in [this](https://github.com/mongodb/node-mongodb-native/tree/NODE-5372) branch).

      • If an error is thrown in the `transform` , we throw an unhandled process exception. This only occurs when there are not documents in the cursor’s buffer (otherwise, getting a document from the cursor is synchronous and the error can be caught).
      • `hasNext` transforms documents, resulting in documents that are double transformed when used in conjunction with `cursor.next`

      This bug is present at least as far back as 4.0, it was introduced in the abstract cursor refactor.

      User Impact

      Two issues caused by the above bugs:

      • Crashing the user’s process with an uncatchable exception
      • Silent data corruption (client-side). Example:
        // db has [{ x: 1}, {x: 2}] in it
        const cursor = collection.aggregate(..).map((doc) => ({ ...doc, x: doc.x * 2 }))
        while (await cursor.hasNext()) {
          console.log(await cursor.next()); 
        }
        // output: { x: 4 }, { x: 8 } 
        

      Dependencies

      n/a

      Unknowns

      • Should we take this time and make our 5.x branch's `next` method async? We currently wrap it with a `promisify`. – Create a separate ticket for this.

      Acceptance Criteria

      • `hasNext` should not transform documents
      • any code that call the cursor's transform function should happen either in an async context or should be wrapped in a try-catch

      Implementation Requirements

      • move the call to `transform` out of the generic next method and transform at the places where `next` is called instead
        • Cursor.next
        • Cursor.tryNext
        • ReadableCursorStream.read()

      Testing Requirements

      • Integration tests for the following scenarios
        • when Cursor.hasNext is called, we do not call the transform.
        • when the cursor's transform throws an error, we catch the error and pass it to the user in a promise rejection.
      • add tests demonstrating that the transform is correctly called for each API that yields documents to the user
        • Cursor.stream()
        • Cursor.next() && Cursor.tryNext()
        • Symbol.asyncIterator

      We currently have no integration test suite for generic cursor behavior, so we can create one. For the purposes of testing we can either use an existing cursor with generic behavior (AggregationCursor) or create a mock cursor that returns mock documents from a buffer asynchronously.

      Our unhandled error mocha hooks seem to be failing - I was unable to reproduce the process crashing error inside mocha. We can consider investigating this further as a part of this work or filing a follow up ticket.

      • Follow up ticket.

      Documentation Requirements

      n/a

      Follow Up Requirements

      • File a follow up ticket for async refactor of `next<T>`
      • File a follow up ticket for mocha uncaught exception investigation

            Assignee:
            bailey.pearson@mongodb.com Bailey Pearson
            Reporter:
            bailey.pearson@mongodb.com Bailey Pearson
            Durran Jordan
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: