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

Fix url parsing for a mongodb+srv url that has commas in the database name #2789

Merged
merged 6 commits into from May 25, 2021
Merged

Fix url parsing for a mongodb+srv url that has commas in the database name #2789

merged 6 commits into from May 25, 2021

Conversation

pablomatiasgomez
Copy link

@pablomatiasgomez pablomatiasgomez commented Apr 25, 2021

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 the pathname, 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.

@agolin95 agolin95 requested review from a team, durran, emadum and nbbeeken and removed request for a team April 26, 2021 15:49
Copy link
Member

@durran durran left a 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

@agolin95
Copy link
Contributor

@agolin95 agolin95 added the tracked-in-jira Ticket filed in MongoDB's Jira system label Apr 27, 2021
@pablomatiasgomez
Copy link
Author

pablomatiasgomez commented Apr 27, 2021

@durran Thanks for the review.

I actually didn't find that there were some tests for +srv uris so now I was able to run them and I also added a few for this case.

I thought of different ways to solve the issue, but the cleanest way I could find of is to check directly with the url instead of the parsed one, because there is no easy way to tell if the pathname contains part of the hostname or not.

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:

  • search for . on the pathname, but someone could still provide a hostname without a dot.
  • count the amount of slashes in the pathname, but there are a few edge cases, like dbname not provided (no pathanme), trailing slash, etc.

Thanks!

@durran
Copy link
Member

durran commented May 6, 2021

@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.

@durran durran merged commit 58c4e69 into mongodb:3.6 May 25, 2021
@pablomatiasgomez pablomatiasgomez deleted the fix-srv-url-parse branch May 28, 2021 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracked-in-jira Ticket filed in MongoDB's Jira system
Projects
None yet
4 participants