mattcollier commented 4 years ago

Creating this so that someone can spend some time figuring out what is needed out of this URL rewriting business that is being commented out here.

When connecting with Mongodb Atlas, the url must begin with mongodb+srv as shown here: https://docs.mongodb.com/drivers/node

The commented code appears to be dropping that and replacing it with just mongodb which does not work with sharded clusters. Maybe preserving the protocol portion of the URL will just work.

dlongley commented 4 years ago

I imagine this was done originally for a few reasons:

Ensure that authentication doesn't happen in the URL but via the authenticate API. I'm not sure if that's still a requirement or why it was originally done -- it likely had to do with the local database code we used to use. We may be able to simplify that away now entirely if that's the case.
Ensure that individual config values such as database name/host/username/password/etc. matched what is in the url parameter. If we no longer need to rewrite the URL, we could instead just parse it now and compare against the config values and either throw an error if they don't match or make the url take precedence and overwrite them. That would better allow url to continue to evolve where we only need to ensure that a few values from it are parsed for the config.

dlongley commented 4 years ago

Some comments on URL rewriting are in the config: https://github.com/digitalbazaar/bedrock-mongodb/blob/master/lib/config.js#L11-L32

aljones15 commented 4 years ago

Issue related to this here: https://github.com/digitalbazaar/bedrock-mongodb/issues/48

Also srv means ssl = true. We seem to be making lots of assumptions about what a user wants from the url, while mongodb's docs seem to suggest it is completely cool and good form to have really long and specific connection strings.

gannan08 commented 4 years ago

@mattcollier Update? Where are we at on this?

cc: @dlongley @aljones15

mattcollier commented 4 years ago

@aljones15 this PR is obsolete at this point right? I think the released version connects properly to Atlas or to a local database etc.

mattcollier commented 4 years ago

OK, this URL rewriting business still needs to be addressed. The existing url rewriting business does not provide support for the new url method mongodb+srvused by atlas

mongo "mongodb+srv://mycluster.yxkqf.azure.mongodb.net/<dbname>" --username <username>

aljones15 commented 4 years ago

this does still need to be addressed. Please ping me in the PR when ready and thanks for looking into this.

On Wed, Aug 5, 2020 at 4:38 PM mattcollier notifications@github.com wrote:

OK, this URL rewriting business still needs to be addressed. The existing url rewriting business does not provide support for the new url method mongodb+srvused by atlas

mongo "mongodb+srv://mycluster.yxkqf.azure.mongodb.net/" --username

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/digitalbazaar/bedrock-mongodb/pull/42#issuecomment-669493751, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACD6CAO2PT4WXQ3OOAVC7DR7G7MRANCNFSM4N4LSFNA .

-- Andrew Jones

dmitrizagidulin commented 4 years ago

Bump on this issue. This PR's url branch is making its way into various production project code.

codecov-commenter commented 4 years ago

Codecov Report

Merging #42 into master will increase coverage by 1.94%. The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   52.06%   54.01%   +1.94%     
==========================================
  Files           4        4              
  Lines         388      374      -14     
==========================================
  Hits          202      202              
+ Misses        186      172      -14

Impacted Files	Coverage Δ
lib/index.js	`49.41% <71.42%> (+1.95%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a598ac4...022fd8b. Read the comment docs.

aljones15 commented 4 years ago

Bump on this issue. This PR's url branch is making its way into various production project code.

Just as a note: the read me in master has a how to connect using the connectOptions: https://github.com/digitalbazaar/bedrock-mongodb. As far as I can tell this PR is not causing any major issues and provides a good fallback in case the api changes somehow (which has already caused connection issues).

mattcollier commented 4 years ago

I've been observing unexpected behavior with this code when used with a URL.

When a URL is used, it's my expectation that other configuration in bedrock-mongodb will not contradict the settings in the URL, including the authsource.

When working with atlas, the url looks like this:

mongodb://yyyy:xxxxx@aaaaa-shard-00-00.yxkqf.azure.mongodb.net:27016/zzzzzz_database?ssl=true&authSource=admin&retryWrites=true&w=majority

So, in cases where the authSource is specified in the URL the following line of code sends contradictory information into the client unless bedrock.config.mongodb.connectOptions.authSource is explicitly set properly as well.

https://github.com/digitalbazaar/bedrock-mongodb/blob/url/lib/index.js#L690

I don't know what all the command line options are, but I imagine they are extensive and allow one to configure most things about a client connection. Unless we are going to decode the URL and decide which bedrock-config settings might still be operative, it seems like we me must not send in any client config options when a URL has been specified.

digitalbazaar / bedrock-mongodb

Do not rewrite URL #42

Codecov Report