mongodb-labs / django-mongodb

MongoDB Backend for Django
Apache License 2.0
16 stars 7 forks source link

INTPYTHON-424 Prevent redundant requirement for USER and PASSWORD arguments when URI with authentication is provided #194

Closed Jibola closed 3 days ago

timgraham commented 1 week ago

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 commented 1 week ago

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:

timgraham commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

timgraham commented 3 days ago

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