-
Type: Bug
-
Resolution: Done
-
Priority: Unknown
-
Affects Version/s: None
-
Component/s: Cursors
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
- backported by
-
NODE-5375 [v4.x BACKPORT] Cursor.hasNext should not transform documents
- Closed
- is duplicated by
-
NODE-3232 cursor.map() iterator exception is not handled
- Closed
-
NODE-4879 cursor.hasNext() applies transform from cursor.map() to next document in cursor
- Closed
- is related to
-
NODE-5372 Using hasNext() and next() on mapped aggregation cursor causes map function to be called twice for every document
- Closed