fix(NODE-4074): ensure getTopology doesn't throw synchronously

[baileympearson]

Description

This PR is a re-do of the work done by Daniel Lando (original PR: #3170). I messed up the rebase and accidentally removed all changes.

What is changing?

All uses of getTopology that are used in an asynchronous manner now catch any errors thrown and pass the error along to the appropriate callback.

• All calls to getTopology that were used in conjunction with executeOperation were moved into executeOperation
• Tests were added to ensure we're handling the errors appropriately with both callbacks and promises
• Two other uses of getTopology (indexInformation and AddUserOperation.execute) were calling getTopology in an asynchronous context, so these two uses now handle the potential error thrown from getTopology.
• getTopology was enhanced to take other topology providers that are used throughout the driver

Is there new documentation needed for these changes?

Nope!

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>
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[nbbeeken]

Just noticed 😅 We've got some lint errors to sort out

[robertsLando]

Any news on this?

[robertsLando]

Seems that there are still some collections methods that could throw:

https://github.com/mongodb/node-mongodb-native/blob/main/src/collection.ts#L762

https://github.com/mongodb/node-mongodb-native/blob/main/src/collection.ts#L1411

cc @baileympearson

[baileympearson]

Hey @robertsLando! These methods can still throw. However, they're actually synchronous methods so throwing is the expected behavior (any method that returns a cursor is synchronous because cursors are lazy and don't fetch data until they're consumed).

The other methods in this PR needed to be fixed because they took callbacks. Previously, if we didn't catch the error and the user passed a callback, we could crash the user's process because we weren't propagating the error up the callback chain.

[robertsLando]

Yeah I know that (I'm the user who opened the issue and PR for this). I just saw the new release today and when I tested it I got this issue. While I understand what you mean I don't think this behaviour is correct as it's not working as before, in my opinion the error should be thrown when next is called (like it was before). Wrapping every find with a try catch is much less user friendly

Before:

collection.find({}).next( (err, doc) => {
// err contains the error connection is not opened
})

Now:

try {
	  collection.find({}).next( (err, doc) => {
	 		  // never reached
	  })
} catch(error) {
 // error contains the error connection is not opened
}

[baileympearson]

Just to clarify - you're saying that the behavior changed in v4.5.0? I've tried in all versions 4.3.0 -> 4.5.0 and in all cases I see the error being caught in the catch block, not in the next function.

import { MongoClient } from "mongodb";

async function run() {
  const client = new MongoClient(< uri >);

  const db = client.db('nodeTest');
  const collection = db.collection('nodeTest');

  try {
    collection.find({}).next(( )=> {
      console.log('caught error inside next');
    });
  } catch (err) {
    console.log('error caught in catch')
  }
}

run();

[robertsLando]

Just to clarify - you're saying that the behavior changed in v4.5.0

Nope this has changed in 4+ versions but I didn't see it reported as breaking change. Also as you can see in my example the code looks a bit less user friendly