Add support for ReplSet and associated options

RandomSeeded commented 6 years ago

Hey again @wzrdtales !

Sorry if the title is unclear. Right now in the mongo driver we have the following:

db = config.db || new MongoClient(new Server(host, port));

And I'm hoping to change it to something which has support for ReplSet and associated options:

    db = config.db || new MongoClient(new ReplSet(servers, options));

I think my general approach would be to try to avoid changing the API and add additional optional options to the config. Perhaps something like:

var mongoClient;
if (config.replSet) {
  var hosts = config.replSet.split(',');
  var servers = hosts.map(function(host) {
    return new Server(host, port, config.replSetOptions);
  });
  db = config.db || new MongoClient(new ReplSet(servers, config.replSetOptions));
} else {
  db = config.db || new MongoClient(new Server(host, port));
}

Before I went ahead and figured the rest out I figured I'd double check with you that this seems reasonable and you'd be amenable to a PR for this. Also please let me know if you have any implementation requests. And, as always, thanks for the work you do :)

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/57175517-add-support-for-replset-and-associated-options?utm_campaign=plugin&utm_content=tracker%2F12293389&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F12293389&utm_medium=issues&utm_source=github).

RandomSeeded commented 6 years ago

For now I'll proceed with ^ and make a PR when it's ready

RandomSeeded commented 6 years ago

@wzrdtales pinging as requested here too :)

MartinLoeper commented 5 years ago

Hi @RandomSeeded and thank you for sharing your thoughts here. We at nesto-software are dealing with replica sets in mongo, too. However, since the time of your writing, MongoDB released the SRV connection string feature.

I forked this repository and updated its dependencies in order to have this feature available. Furthermore, similar to your proposed changes, I gave the user the option to specify a "mongodb+srv" records.

I am writing this comment to let everybody know who has a similar issue (e.g. users of Mongo Inc. Atlas clusters). Here is a diff of my changes in order to make it work: https://github.com/db-migrate/mongodb/compare/master...nesto-software:master

All you have to do is, to specify a database.json config like the following one:

{
    "defaultEnv": "my-default",
    "my-default": {
        "driver": "mongodb-nesto-fork",
        "database": "your-db",
        "useSrvRecord": "true"
    }
}

Notice: There is a new variable useSrvRecord which is used to change the protocol. If it is set 'true', the connection string is prefixed with "mongodb+srv", otherwise "mongodb" as before. That is, the forked version retains backward compatibility.

It is important to set the driver name to the name of your custom npm package or just use the one which I uploaded after creating the fork (which is db-migrate-mongodb-nesto-fork).

chill-cod3r commented 4 years ago

@MartinLoeper is there a way to configure that while still just using a database uri and not individually configuring each piece? (ie host, user, password, database, etc.)

MartinLoeper commented 4 years ago

@wolfejw86 Are you referring to the mongodb protocol or the mongodb+srv protocol? I guess host and database must be set any case with the present code from my fork. That is not an optimal solution since the database can be discovered from the srv record when using the latter protocol.

Could you provide an example of a configuration which you would like to set (i.e. the minimal set of keys you want to set)?

chill-cod3r commented 4 years ago

Really I’d like it just to be consistent with the rest of dB-migrates api. So

{
  "dev": { "ENV": "MYMONGO_SRV_URI" }
}

I would be happy to contribute.

MartinLoeper commented 4 years ago

Ok, I had to think about this, but finally I got your point. If I understand correctly, you state that:

db-migrate is not database vendor specific
the mongodb plugin for db-migrate does not conform to the de-facto standard of passing the connection string to the underlying db mongo driver
we should use a single attribute which contains the full connection string

I guess this is actually the way to go! I do not see any reason why it has to be so complicated as it currently is. However, we must either provide an adapter to be backward compatible or we introduce a breaking change. Personally, I would go for the first option.

However, I do not have the time to do this right now. If you want to do this @wolfejw86, go for it! It makes very much sense IMO.

chill-cod3r commented 4 years ago

@MartinLoeper thanks for the response! Sorry if I wasn't as clear as I should have been, I was typing that last message on my phone 🤦 .

Regarding implementation, would a "one or the other" type implementation make sense instead of either of your suggested options? I'm looking at this line in particular:

https://github.com/db-migrate/mongodb/blob/f820c1359dc848326afdb0b93e2442746505d474/index.js#L495

If we could just do a preliminary check above that line that says if (typeof config === 'string') then treat it like a mongodb connection string right? That way we could parse it all out. There would still be additional work for the srv portion I believe but I think logic gates at the start of that configuration check could make it easier.

Let me know if you think this makes sense, I'm a first time user of this package and the functionality is great however it appears it has a decent amount of backstory as to why things are the way they are and I don't want to "step" on that.

MartinLoeper commented 4 years ago

Ok, I looked at the implementation again and noticed the following:

a) You can fully restructure the exports.connect method. The only thing necessary is to return an instance of MongodbDriver. b) The database must be extracted inside the connect method and passed to the MongodbDriver since I think it is used by some of the other methods, e.g. to switch back to default database. You just have to extract it from your connection string somehow. I guess mongo has released an API since then which makes it easier to parse connection strings, so this should be no issue. c) backstory? Yes, I think there is. I think the connection string parsing was not as easy as today. When I forked this package and built the "useSrvRecord" option into it, my goal was to do as little refactoring as possible. However, I guess today the exports.connect method should be completely rewritten to adhere to mongo best practices and being better usable. d) I do not think that the db-migrate base module imposes any restrictions on the config that can be passed. At least nothing I can find inside the docs [1]. I guess each database driver might specify its own configuration keys. That should make it possible to add a property "connectionString". But don't pin me down to that. This is really just a guess.

[1] https://db-migrate.readthedocs.io/en/latest/Getting%20Started/configuration/

db-migrate / mongodb

Add support for ReplSet and associated options #25