Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More robust parsing of mongodb connection string. Use new url parser. #1002

Merged
merged 6 commits into from
Sep 24, 2018

Conversation

sagannotcarl
Copy link
Contributor

When using a more complex mongo connection string the current mongodb connection template throws and error trying to get the database name.

In my case I am connecting to compose.io using the database string:
mongodb://[username]:[password]@aws-us-east-1-portal.20.dblayer.com:21660,aws-us-east-1-portal.21.dblayer.com:21660/compose?authSource=admin&ssl=true

And my app would fail to start with the following error:
MongoError: database names cannot contain the character '.'.

So I am using the parseConnectionString function from mongodb-core to get the database name instead of url.parse, since that can't handle the mongodb failover syntax. I pass in an empty function as the callback since MongoClient.connect() will already throw an error if the mongo connection string is not formatted correctly.

I was also getting a DeprecationWarning which was fixed for mongoose here cf613ef, so I applied the same fix to the mongodb template.

@daffl
Copy link
Member

daffl commented Sep 17, 2018

Thank you. Wouldn't this also need mongodb-core added as a dependency?

@sagannotcarl
Copy link
Contributor Author

sagannotcarl commented Sep 17, 2018

Yes it would. Sorry I don't have a good enough understanding of the generators, I assumed that since this was in the mongodb template mongodb-core could be assumed.

If that's not the case could you recommend the best way to make mongodb-core a dependency and I'll add it.

Thanks for the feedback!

@sagannotcarl
Copy link
Contributor Author

@daffl I added the mongodb-core dependency in generators/connection/index.js.

Anything else you can see here you'd like me to change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants