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

fix(NODE-5374): do not apply cursor transform in Cursor.hasNext #3746

Merged
merged 11 commits into from
Jul 5, 2023
Merged

Conversation

baileympearson
Copy link
Contributor

@baileympearson baileympearson commented Jun 27, 2023

Description

What is changing?

Primarily this PR fixes two bugs in our AbstractCursor.

  • transform should not be applied when using hasNext.
  • when transform throws, we should catch the error and propagate it to the user.

To facilitate this, the generic next<T> was refactored to use async-await as well.

The PR is broken down into commits which might be easier to view. The first commits are the async-await refactor, then the bug fixes.

Is there new documentation needed for these changes?

No.

What is the motivation for this change?

Release Highlight

Cursor.map transform bug fixes

The cursor API provides the ability to apply a map function to each document in the cursor:

const cursor = collection.find({ name: 'john doe' }).map(({ name }) => name);
for await (const document of cursor) {
  console.error(document); // only prints the `name` field from each document
}

Cursor iteration properly catches errors thrown from the transform applied in Cursor.map

Starting in version 4.0 of the driver, if the transform function throws an error, there are certain scenarios where the driver does not correctly catch this error and an uncaught exception is thrown:

const cursor = collection.find({ name: 'john doe' }).map(() => {
  throw new Error('oh no! error here'); // 
});
await cursor.next(); // process crashes with uncaught error

This release adds logic to ensure that whenever we transform a cursor document, we handle any errors properly. Any errors thrown from a transform function are caught and returned to the user.

Cursor.hasNext no longer transforms documents if a transform has been applied to the cursor

Version 4.0 introduced a bug that would apply a transform function to documents in the cursor when the cursor was iterated using Cursor.hasNext(). When combined with Cursor.next(), this would result in transforming documents multiple times.

const cursor = collection.find({ name: 'john doe' }).map((document) => document.name);
while (await cursor.hasNext()) { // this transforms the first document in the cursor once
	const doc = await cursor.next(); // the second document in the cursor is transformed again
}

This release removes the transform logic from Cursor.hasNext, preventing cursor documents from being transformed twice when iterated using hasNext.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@baileympearson baileympearson force-pushed the NODE-5372 branch 5 times, most recently from 2da8f1a to dd008e0 Compare June 29, 2023 22:58
@baileympearson baileympearson marked this pull request as ready for review June 30, 2023 14:38
test/integration/node-specific/abstract_cursor.test.ts Outdated Show resolved Hide resolved
test/integration/node-specific/abstract_cursor.test.ts Outdated Show resolved Hide resolved
cursor: AbstractCursor<T>,
blocking: boolean,
callback: Callback<T | null>
): void {
transform = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we are defaulting to true here as more methods would transform vs. methods that don't (hasNext)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my logic. I also considered two other options:

  • providing no default
  • combining blocking and transform into an options object and requiring all callsites to explicitly set them for clarity (i.e., next(this, { blocking: true, transform: false })

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lean towards providing no default, for internal I prefer to make each call site define its inputs and because this isn't saving us a lot of refactors. I don't feel strongly though, feel free to resolve if we agree to leave it as is.

@durran durran self-assigned this Jul 3, 2023
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 3, 2023
@nbbeeken nbbeeken requested review from nbbeeken and dariakp July 3, 2023 14:48
nbbeeken
nbbeeken previously approved these changes Jul 5, 2023
@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 5, 2023
@durran durran merged commit 0668cd8 into main Jul 5, 2023
@durran durran deleted the NODE-5372 branch July 5, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants