feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.06k stars 752 forks source link

Feathers fails to connect to MongoDB when connection string contains multiple routers #1044

Closed EliSadaka closed 6 years ago

EliSadaka commented 6 years ago

Steps to reproduce

I'm running into an issue where when my MongoDB connection string points to multiple mongos instances instead of just one, it fails to connect and returns the error database names cannot contain the character '.' even though my database name does not contain that character. When my connection string contains only one router, it doesn't return this error and works fine. I'm also able to connect to MongoDB using the mongo shell even when specifying multiple routers.

The error is originating from here in my mongodb.js:

const url = require('url');
const MongoClient = require('mongodb').MongoClient;
const logger = require('winston');

module.exports = function (app) {
  const config = app.get('mongodb');
  const dbName = url.parse(config).path.substring(1);
  const promise = MongoClient.connect(config, {useNewUrlParser:true}).then(client => {
    // For mongodb <= 2.2
    if(client.collection) {
      return client;
    }

    return client.db(dbName);
  });

  promise.catch(error => logger.log("error", error)) //error is caught here

  app.set('mongoClient', promise);
};

Here is an example of what my connection string looks like: "mongodb": "mongodb://feathers:password@host1.com:27017,host2.com:27017/database"

Expected behavior

The server should connect to MongoDB successfully.

Actual behavior

The server fails to connect and returns the following error:

{ MongoError: database names cannot contain the character '.'
   at Function.create (/var/api/node_modules/mongodb-core/lib/error.js:43:12)
   at validateDatabaseName (/var/api/node_modules/mongodb/lib/operations/db_ops.js:704:24)
   at new Db (/var/api/node_modules/mongodb/lib/db.js:180:3)
   at MongoClient.db (/var/api/node_modules/mongodb/lib/mongo_client.js:268:14)
   at MongoClient.connect.then.client (/var/api/src/mongodb.js:14:19)
   at process._tickCallback (internal/process/next_tick.js:68:7)
   driver: true,
   name: 'MongoError',
   [Symbol(mongoErrorContextSymbol)]: {} }

System configuration

Module versions (especially the part that's not working):

"@feathersjs/feathers": "3.2.3" "feathers-mongodb": "3.3.0" "mongodb": "3.1.6"

NodeJS version: 10.11.0

EliSadaka commented 6 years ago

The same error also occurs when the connection string contains two members of a replica set. It seems to occur no matter what where there is more than one host defined in the connection string.

claustres commented 6 years ago

I think this has nothing to do with Feathers, there is actually no code related to Feathers in your post, this is probably an issue with your mongo client. If I run the following code:

const url = require('url')
const config = 'mongodb://feathers:password@host1.com:27017,host2.com:27017/database'
console.log(url.parse(config).path.substring(1))

Here is the output: :27017,host2.com/database, which I guess is causing your error because you use this in client.db(dbName).

EliSadaka commented 6 years ago

My mongodb.js file is the default one generated by the Feathers CLI. So I guess by default it doesn't support multiple hosts without manual changes to that file?

claustres commented 6 years ago

If this code has been generated by the CLI then it is a CLI issue because it does not work as expected. I guess you can patch by splitting the string first by , and use the last url to extract the db name.

EliSadaka commented 6 years ago

Yes, I was able to fix the issue and get it working but ideally the generated file should work by default with any number of hosts in the connection string.

daffl commented 6 years ago

The best way to make that happen would be a pull request into the generator with the fix you used to get it working 😄

claustres commented 6 years ago

Seems to be actually the case: https://github.com/feathersjs/feathers/pull/1002.

daffl commented 6 years ago

Good point. This will work as long as you are using @feathersjs/cli version 3.8.5 or later.

lock[bot] commented 5 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.