-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix url parsing for a mongodb+srv url that has commas in the database name #2789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. While this does solve the issue when database names contain a comma, it does however break our tests in the following case:
- Multiple hosts are provided in the URI, ie
mongodb://test1.com,test2.com/dbname
This is because we initially do a URI.parse
which does not know how to handle
multiple hosts, as they are not part of the URI specification. We allow multiple hosts
with the mongodb
scheme but not with the mongodb+srv
scheme. So this
change actually causes our srv validation to pass when it should not be valid.
Example of a failing test in this case: https://github.com/mongodb/node-mongodb-native/blob/3.6/test/spec/connection-string/invalid-uris.yml
Tracked in https://jira.mongodb.org/browse/NODE-3234 |
@durran Thanks for the review. I actually didn't find that there were some tests for I thought of different ways to solve the issue, but the cleanest way I could find of is to check directly with the Let me know if you agree with this approach or if you would it to be different. some other ways that I thought of but were a mess and didn't work:
Thanks! |
@pablomatiasgomez Thanks and I think this looks good. I just need to have a conversation with the other dbx teams since we'll probably want to port those new tests across other drivers potentially. |
Description
Fixes an invalid exception that is thrown when a mongodb+srv uri contains commas (
,
) in the database name. For example:mongodb+srv://valid.example.com/db,with,commas
Unfortunatelly,
mongodb+srv
urls/uris are not tested so I couldn't add a test. I think this is because the url parsing also resolves the dns name, which is what happened when I tried to add a test. Of course we don't want to resolve dns names in tests but doing that would require a bigger refactor.What changed?
Basically there was an if that checks if the numbers of hostnames that were provided were more than one for
mongodb+srv
urls/uris, but the condition was invalid as it was actually checking thepathname
, which in the mongo uri that is the database name. I have a mongodb database with commas in its url and I am not able to connect to it.Are there any files to ignore?
No.