Skip to content

Conversation

Jibola
Copy link
Contributor

@Jibola Jibola commented Nov 21, 2024

No description provided.

@timgraham
Copy link
Collaborator

Django doesn't natively support passing URLs in HOST. There is dj-database-url to parse a URL into the DATABASES dictionary format that Django expects.

If we go down the road of allowing HOST to contain USER and PASSWORD, we'd need to audit this package (e.g. https://github.com/mongodb-labs/django-mongodb/blob/ca8ac6aaf0e5db548728f2290630350dc4ce5ca1/django_mongodb/client.py#L14-L18) and Django itself for usages of these values to make sure both ways user/password could be provided are accounted for. I don't like the extra complexity this introduces. It's possible this could cause incompatibilities with third-party packages that assume the usual DATABASES values as well.

@aclark4life
Copy link
Collaborator

Right, although as @Jibola mentioned to me there is some precedent for this, in that HOST can take things-that-aren't-a-hostname. Things-that-are-credentials may be too much to ask though.

Also note that this came up because I tried to use mongodb+srv (with Atlas) and PyMongo only supports SRV via the URI syntax. I don't like that either, but we're not going to fix that any time soon or possibly ever.

If we were to merge this, the URI could be passed in without having to "redundantly specify user and pass" and PyMongo will correctly parse the URI and SRV connections will work "as expected".

If we don't merge this, we still have to support SRV connections so I propose the following changes to our backend:

  • Raise InvalidHostError if HOST is not a hostname
  • Add is_srv parameter to OPTIONS which if True we add mongodb+srv:// to the connection string passed to PyMongo.
  • Add support to dj-database-url to include { "OPTIONS" { "is_srv": True }} if mongodb+srv:// is found in the URI.

@timgraham
Copy link
Collaborator

If we want the user to be able to configure this backend with something like, mongodb+srv://mycluster.abcd1.mongodb.net/myFirstDatabase (which seems to be a requirement), then I think we need to parse it using either dj-database-url or, if we don't want the external dependency, a utility in this library.

From a Django perspective, I think it'll be problematic for the user to provide a "complex" URI that contains user, password, and/or database as the DATABASES['HOST'] because of code that assumes values like database name are parsed out.

@Jibola
Copy link
Contributor Author

Jibola commented Nov 22, 2024

If we want the user to be able to configure this backend with something like, mongodb+srv://mycluster.abcd1.mongodb.net/myFirstDatabase (which seems to be a requirement), then I think we need to parse it using either dj-database-url or, if we don't want the external dependency, a utility in this library.

From a Django perspective, I think it'll be problematic for the user to provide a "complex" URI that contains user, password, and/or database as the DATABASES['HOST'] because of code that assumes values like database name are parsed out.

We can make a small wrapper around the mongodb parse_uri functionality and let folks use that if they'd like. https://pymongo.readthedocs.io/en/stable/api/pymongo/uri_parser.html#pymongo.uri_parser.parse_uri

@timgraham
Copy link
Collaborator

Right, where the wrapper takes the parsed result and translates into the format of Django's settings.DATABASES.

@aclark4life aclark4life changed the title INTPYTHON-424 Prevent redundant requirement for USER and PASSWORD arguments when URI with authentication is provided INTPYTHON-424 Support passing URLs via django_mongodb.parse Nov 26, 2024
@timgraham timgraham changed the title INTPYTHON-424 Support passing URLs via django_mongodb.parse INTPYTHON-424 Prevent redundant requirement for USER and PASSWORD arguments when URI with authentication is provided Nov 26, 2024
@timgraham
Copy link
Collaborator

The solution has gone in a different direction, see #195.

@timgraham timgraham closed this Nov 26, 2024
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.

3 participants