Changing mongodb connection string causes generated tests to fail

[Huggzorx]

Steps to reproduce

I generate a new app using the Feathers CLI, using all the default selections except for making the users service type 'mongodb'. I set up both a local mongodb instance and one running elsewhere (I'm using the Atlas hosted database service). Running npm test without changing anything works, all tests pass. Next I change the default connection string from localhost to point at my external database and run npm test again.

The connection string looks like: "mongodb+srv://[username]:[password]@testing-cluster-ziimk.mongodb.net/test"

Expected behavior

All tests should pass, same as if I was using the local database

Actual behavior

The test 'authenticates user and creates accessToken' fails with TypeError: Cannot read property 'find' of undefined.

I know that the database can be accessed, because if I do npm start and create a user with a POST request I get a HTTP 201 response and the user is successfully created in the remote database.

I have noticed that I get the following warning after npm start: DeprecationWarning: current Server Discovery and Monitoring engine is deprecated, and will be removed in a future version. To use the new Server Discover and Monitoring engine, pass option { useUnifiedTopology: true } to the MongoClient constructor. but I don't think this is related.

System configuration

Tried on both Ubuntu and Windows 10.
On Ubuntu, I have:

• Feathers 4.2.3

• Node v10.17.0

• NPM 6.11.3

• MongoDB 4.0

[daffl]

I don't think this is a Feathers specific issue. The connection initialization probably wasn't successful. One thing that could be improved in the generated connection file (src/mongodb.js) is to log errors, changing https://github.com/feathersjs/generator-feathers/blob/master/generators/connection/templates/js/mongodb.js#L6 to

  const mongoClient = MongoClient.connect(connection, { useNewUrlParser: true })
    .then(client => client.db(database));
    .catch(console.error);

[Huggzorx]

I think the issue is actually that mongoClient ends up as Promise { <pending> }, and there is no waiting for the promise to resolve before the tests are trying to do things with the connection.

In the before hook of the test, I added
console.log(app.get('mongoClient'));
and got
Promise { <pending> }

I then added
await app.get('mongoClient');
before the console.log and it prints out

Promise {
  Db {
    _events: [Object: null prototype] {},
    _eventsCount: 0,
    _maxListeners: undefined,
    s: {
      dbCache: {},
      children: [],
      topology: [ReplSet],
      options: [Object],
      logger: [Logger],
      bson: BSON {},
      readPreference: [ReadPreference],
      bufferMaxEntries: -1,
      parentDb: null,
      pkFactory: undefined,
      nativeParser: undefined,
      promiseLibrary: [Function: Promise],
      noListener: false,
      readConcern: undefined,
      writeConcern: undefined,
      namespace: [MongoDBNamespace]
    },
    serverConfig: [Getter],
    bufferMaxEntries: [Getter],
    databaseName: [Getter]
  }
}

The test then passes, as the db connection is ready.
I also tried this with a manual delay instead of await and got the same result.

In users.class.js we have

    app.get('mongoClient').then(db => {
      this.Model = db.collection('users');
    });

I'm still struggling to get my head around promises a little, but I think this essentially just adds another .then to the promise chain - so again, by the time we get to the end of this file, there is no guarantee that mongoClient is anything but a pending promise.

I changed the code to this just to prove this to myself:

    console.log('--------- Users Service Class ---------');
    console.log(app.get('mongoClient'));
    app.get('mongoClient').then(db => {
      this.Model = db.collection('users');
    });
    console.log('--------- /// ---------');
    console.log(app.get('mongoClient'));
    console.log('---------------------------------------');

Which results in

--------- Users Service Class ---------
Promise { <pending> }
--------- /// ---------
Promise { <pending> }
---------------------------------------

So in the tests, where we have const app = require('../src/app');, this app is used immediately with no guarantee that the promises are resolved.

Edit: Changed some stuff in this post to make what is going on more clear (I hope)

[daffl]

The promise will be resolved in the .then handler (or after an await in an async function). If you need your tests to wait you can add a

before(async () => {
  await app.get('mongoClient');
});

In each test.

[Huggzorx]

Thanks for the response. I will definitely do an await before all of my tests for now to prevent them starting before the database connection is ready. I suppose it also makes sense to do this in the main index.js before app.listen(); It is unlikely, but possible that as the app comes up that it could receive a REST request before the database connection promise (or any chained services) is resolved.

It would be nice if there was default way to wait for all promises in the app object to be resolved before it was useable, in the case of having many app.configure() instances that do app.set() with a promise.

[daffl]

Yep, this will be possible in the next version. The issue for this is #509 so going to close this one.