Add call to get server info after logging in user (authenticated connection)

[dlongley]

We need to ensure that the connection to the database works after trying to establish an authenticated connection. We may be able to ask for the server info again (or a similar call) after doing loginUser. Just returning after loginUser doesn't catch connection errors which leads to a lot of trouble.

[davidlehn]

Maybe the minimal ping command is enough?

[dlongley]

Maybe the minimal ping command is enough?

Maybe. We'll need to experimentally confirm whatever solution we try.

[mattcollier]

Here's something else that might be useful: https://mongodb.github.io/node-mongodb-native/3.7/api/Db.html#event:fullsetup

[aljones15]

ok so some issues on issues:

Originally the mongodb driver had a method db.authenticate (bedrock-mongodb < 7):

https://github.com/digitalbazaar/bedrock-mongodb/blob/10924b42c578857fefc998e872409ba5392e13bf/lib/index.js#L692-L696

This was ported over to client.connect as db.authenticate was deprecated when mongodb's node driver upgraded (bedrock-mongodb >= 7). mongodb node 3.7 driver Db Docs

https://github.com/digitalbazaar/bedrock-mongodb/blob/9552919fd068b3423a8afc50b9ae33729105faea/lib/index.js#L672-L682

https://github.com/digitalbazaar/bedrock-mongodb/blob/9552919fd068b3423a8afc50b9ae33729105faea/lib/index.js#L722-L739

So I believe we can actually remove the function loginUser now as changes to the _connect function were made that use auth if a username and password are specified:

(bedrock-mongodb >= 8) https://github.com/digitalbazaar/bedrock-mongodb/blob/d44a19830cac1d9634058a997ae790fa25929698/lib/authn.js#L138-L149

So basically, we have legacy code ported from an older driver that is resulting in multiple logins. That might be one of the underlying issues. This also means we need to check if auth is required before calling on _connect.

[aljones15]

ok so further issue with this issue:

calls on admin.serverInfo() don't require authentication, but listDatabases does (which is weird because in mongo shell listDatabases will just return nothing if you're not authenticated.) So serverInfo should come first, check the semver to make sure it matches the expected semver and then we need to check if auth is required after that.

[mattcollier]

I believe I've found the root cause of the issue we're trying to address which is that the BedrockError being thrown here is being dropped on the floor due to the nested try/finally pattern being used. If this is somehow refactored to actually throw this error, I think it will solve the issue I'm seeing in deployments.

https://github.com/digitalbazaar/bedrock-mongodb/blob/main/lib/index.js#L180-L184

[aljones15]

I believe I've found the root cause of the issue we're trying to address which is that the BedrockError being thrown here is being dropped on the floor due to the nested try/finally pattern being used. If this is somehow refactored to actually throw this error, I think it will solve the issue I'm seeing in deployments.

https://github.com/digitalbazaar/bedrock-mongodb/blob/main/lib/index.js#L180-L184

function t() { 
  try{
    notExist() 
  } catch(e) {
    throw e
  } finally{
    console.log('finally')} 
}

t()
finally
Uncaught ReferenceError: notExist is not defined
    at t (<anonymous>:1:21)
    at <anonymous>:1:1

looks like finally fires before the throw which could result in the error being delayed or swallowed if the close connection function never returns.

so maybe we need

finally {
    // force client to close connections (do not reuse connections used to init
    // database as other connections will be used later that may have different
    // credentials)
    if(client) {
      client.close(force).catch(error => {
        logger.error(
          'failed to close client used to initialize database', {error});
    }

and not await the promise. An experiment will need to be made using something that waits like 5 seconds and then resolves to see what our code does now.

[dlongley]

Addressed by #85.