Fixed #28236 - Integrate dj-database-url into Django #8562

orf · 2017-05-27T21:55:59Z

This PR integrates dj-database-url into Django, allowing users to configure their caches and databases via URLs:

# settings.py
from django.conf.service_url import configure_db, configure_cache, register_db_backend

register_db_backend('customDB', 'my.custom.db.adapter')
register_cache_backend('customCache', 'my.custom.cache.adapter')

# These values would come from environment variables rater than be hard-coded like this

DATABASES = {
    'default': configure_db('postgres://user:password@localhost/myDb?sslmode=verify-full'),
    'sqlite': configure_db('sqlite://:memory:'),
    'custom': configure_db('customDB://10.10.10.10/something?else=1')
}

CACHES = {
    'default': confgure_cache('memcached://127.0.0.1:11211'),
    'file': configure_cache('file:///tmp/cache'),
    'custom': configure_db('customCache://10.10.10.10/something?else=1')
}

It's extensible, allowing third party adapters to plug in and extend the parsing. It does this by using a config_from_url static method to the base cache adapters and DB wrappers, which allows subclasses to adapt the result - i.e mysql parses some options from the querystring into specific configuration options.

The test suite is copied/adapted from dj-database-url as are a lot of the quirks parsing, so thanks to @kennethreitz for those.

To-Do:

Documentation
More full-suite tests, ensuring the resulting config actually works
Add support for easy overriding config options via kwargs

kennethreitz · 2017-05-27T22:01:45Z

✨🍰✨

kennethreitz · 2017-06-09T15:55:16Z

Let's get some docs added to this, @orf!

orf · 2017-06-09T16:03:41Z

I'm a bit busy with my filtered aggregates stuff right now, but I've been following the discussions on the mailing list. There are some good ideas I'd like to try first before I make any docs. I would also really like to figure out a generic way of handling other settings, like caches, and I've got a few ideas about handing off specific parts of the parsing to related classes.

kennethreitz · 2017-06-09T16:42:03Z

👍

kennethreitz · 2017-06-09T16:42:13Z

If there's anything I can do to help, let me know!

wkschwartz · 2017-08-15T05:11:39Z

django/utils/database_url.py

No longer need the "_psycopg2" part.

orf · 2017-08-20T13:21:58Z

I've finally got around to pushing what I've got so far - sorry about the delay.

So my idea is to add a configure_from_url classmethod to BaseDatabaseWrapper (and in future to the base cache class). This gets passed a scheme and url argument.

I think a class based solution is best here, and keeping it inside the database class itself is a good idea. I'll keep working on it, but please let me know what you think.

wkschwartz · 2017-08-20T15:45:19Z

tests/utils_tests/test_database_url.py

Since these tests don't actually connect to a database, is it possible to remove the skipUnless (here and below)?

The downside of this approach is that the DB module needs to be imported, which will fail unless the DB-specific adapters are installed (psycopg2 etc).

wkschwartz · 2017-08-20T15:46:17Z

django/db/backends/__init__.py

Could register_db_backend be a decorator?

See above, these need to be available pre-import

auvipy · 2017-09-14T07:00:15Z

is this going to be make in django 2.0?

orf · 2017-09-14T08:18:06Z

Unfortunately not. There is a fair bit to do on the PR to get it up to scratch.

wkschwartz · 2017-09-14T13:39:21Z

django/db/backends/oracle/base.py

Should this be classmethod?

Yep, good catch 👍

wkschwartz · 2017-09-14T14:11:49Z

django/utils/url_config.py

Perhaps TypeError would be more appropriate here and in the next place below? Having a method is usually a property of a type.

wkschwartz · 2017-09-14T14:16:16Z

django/utils/url_config.py

If a user who writes his own backend makes a typo for backend_type, this function will return without error but the registration will mysteriously not have worked. The two options I see are to make a whitelist of allowed backend_types, or make this function private (by adding an underscore in front of the name) and just expose the convenience functions such as register_db_backend.

Good point. I've made them private.

wkschwartz · 2017-09-14T14:17:16Z

django/utils/url_config.py

This function's name starts with "add" but the convenience functions' names start with "register". Would it help to keep these uniform?

wkschwartz · 2017-09-14T14:22:34Z

django/db/backends/postgresql/base.py

In the base class's method, it checks whether URL is already parsed. Does this class need to do the same check before calling urlparse?

See also the SQLite backend.

It's a bit iffy, but I added the check to only the base class so that if the method was over-ridden and the url was parsed the result could be passed in without needing to re-call urlparse. config_from_url, when over-ridden, is always going to parse the URL, but the public API should expect only strings to be passed in.

What if someone wants to subclass the Postgres backend directly?

wkschwartz · 2017-09-14T14:24:48Z

django/db/backends/postgresql/base.py

_ is used elsewhere for gettext. Maybe better to just take the [0] slice instead?

wkschwartz · 2017-09-14T14:49:18Z

tests/utils_tests/test_database_url.py

Can you define dsn once and do url += dsn to reduce duplication?

orf · 2017-09-14T21:39:27Z

Thank you for your review @wkschwartz! All your comments seemed very reasonable so I've made the changes requested. If anyone else has any time to review this it would be awesome. Documentation still needs to be added, but this could be ready for 2.0.

orf · 2017-09-15T00:09:21Z

I've added support for configuring caches as well. It's a little bit more tricky than database configurations.

I ended up creating a parse_url method to deal with common (in db/caches) quirks in the standard library urlparse, namely case-insensitive hostnames. Still a few more tests to run, but seems to work quite well.

wkschwartz

I don't know much about caches, so someone else should review those parts of the code.

Thanks for working on this @orf! I'm looking forward to this feature being baked into Django.

wkschwartz · 2017-09-15T02:01:19Z

django/utils/url_config.py

+
+BACKENDS = defaultdict(dict)
+
+BaseParsedURL = namedtuple('ParsedURL', 'scheme username password hostname port path options')


Could you inherit from urllib.parse.ParseResult to make the class hierarchy easier for people already familiar with the stdlib's data types? Then you could do away with the namedtuple import and simply do

from urllib import parse class ParseResult(parse.ParseResult): @property def netloc(self): # ...what you had written below...

This should override ParseResult.netloc correctly.

Great idea! 👍

Ahh yes. I remember why I did all this, it's because really the ParseResult is pretty unsuited for this due to a lot of annoying quirks. I think it should be a class entirely distinct from ParseResult.

wkschwartz · 2017-09-15T02:03:16Z

django/utils/url_config.py

@@ -0,0 +1,135 @@
+import inspect


Would it make more sense to have url_config go in django/conf/? If you do that, url_config might be a confusing name. Maybe something like services_url_config? I'm trying to think of something (like "services") that captures both DBs and caches, and the fact that they might be hosted somewhere else.

Yeah I think you're right, I think it should be under conf. service_url sounds like a good module name I think?

wkschwartz · 2017-09-15T02:04:55Z

django/utils/url_config.py

+
+class ParsedURL(BaseParsedURL):
+    @property
+    def netloc(self):


Helpful if this should be a public class: A docstring for either ParsedURL, netloc, or both, explaining what netloc does and how ParsedURL differs from urllib.parse.ParseResult

It should definitely have a doc-string, because I've completely forgotten how it differs....!

wkschwartz · 2017-09-15T02:09:16Z

django/utils/url_config.py

+    return BACKENDS[backend_type][scheme]
+
+
+def configure(backend_type, value):


Should configure be private in the same way/for the same reason that _register_backend is?

wkschwartz · 2017-09-15T02:12:32Z

django/utils/url_config.py

+
+
+def register_db_backend(scheme, path):
+    if not path.endswith('.base.DatabaseWrapper'):


I can't find the documentation for how to make a database or cache backend. I'm guessing this is not the only place that explicitly requires this path, but if I'm wrong, should we document this for anyone looking to create their own backends?

wkschwartz · 2017-09-15T02:33:11Z

The other thing I just noticed is that dj-database-url allows you to pass in option overrides such as conn_max_age to the config function. Perhaps simple **kwargs forwarding from configure_db to configure to Database.config_from_url would do the trick. Then config_from_url would simply append kwargs to the OPTIONS value, making the key in the kwargs capitalized. I think this will make configure_db a drop-in replacement for dj-database-url's config.

Thoughts? Is something similar necessary for the cache configurator (at least for uniformity's sake)? (I don't know me much caches.)

various quirks

orf · 2018-02-04T17:44:00Z

I think that needs some thought @wkschwartz, but I agree it's a good feature to have. Implementing overriding individual attributes would be easy, i.e `password='abc', but how we handle nested options (like under 'OPTIONS' etc) would be tricky.

Also, thanks for the review, I've fixed up the branch, rebased it and got the tests to pass. I've made a lot of changes but I think it's a lot neater now. The biggest one was removing the named tuple and instead returning a simple, mutable dictionary from parse_url. I've also moved all the code under django.conf as it does seem like the right place to have this.

I'll spend some more time fixing up a few small things, any feedback on the code would be much appreciated.

wkschwartz · 2018-02-05T16:05:20Z

django/core/cache/backends/memcached.py


    def close(self, **kwargs):
-        # libmemcached manages its own connections. Don't call disconnect_all()
+        # libmemcached managepip install pipenvs its own connections. Don't call disconnect_all()


"managepip"?

oh nuts, I accidentally pasted that there and didn't spot it, I'll remove it.

wkschwartz · 2018-02-05T16:15:55Z

Maybe we don't even need to support all possible option overrides, just the most common, such as conn_max_age. For more complicated things, we could document this pattern:

DATABASES = {'default': configure_db(os.environ['DATABASE_URL'])}
DATABASES['default']['OPTIONS'] = {'FOO': 'bar'}

LuRsT · 2019-09-17T15:07:44Z

@orf Need help here?

wlonk · 2020-06-05T19:06:42Z

@LuRsT and @orf I'm also willing to provide help. I'm interested in reducing the distance from startproject to deployable, and I think this would help.

orf · 2020-07-07T17:19:36Z

Hey @wlonk, feel free to take the ticket and have a go. This is on my to-do list but it’s pretty far down, so please anyone who wants to build on this go for it!

felixxm · 2020-07-08T05:21:06Z

Closing due to inactivity.

orf changed the title ~~Fixed #28236 - Integrate dj-database-url into Django~~ WIP: Fixed #28236 - Integrate dj-database-url into Django Jun 18, 2017

orf changed the title ~~WIP: Fixed #28236 - Integrate dj-database-url into Django~~ WIP: Fixed #28236 - Integrate dj-database-url into Django Jun 18, 2017

wkschwartz reviewed Aug 15, 2017

View reviewed changes

django/utils/database_url.py Outdated

Copy link

Contributor

wkschwartz Aug 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer need the "_psycopg2" part.

wkschwartz reviewed Aug 20, 2017

View reviewed changes

wkschwartz reviewed Sep 14, 2017

View reviewed changes

wkschwartz reviewed Sep 15, 2017

View reviewed changes

orf added 12 commits February 4, 2018 15:42

Add parse_uri and parse_from_environment util functions

bc38ba2

Add class based parsing

aadee90

Review fixes

69deb49

Refactor the implementation by using a parse_url function to handle

41af682

various quirks

Add support for configuring caches via url configs

e390670

Test fixes

5153ff8

Use a plain dictionary over a named tuple

4ff5f3c

Ensure port is an integer

3b08f8d

Remove hostname_with_port, as it's not great to duplicate like that

81e690d

Rename configure to _get_config

e214211

Move url_config to conf.service_url

1406e92

Fix isort

d6ba76c

orf changed the title ~~WIP: Fixed #28236 - Integrate dj-database-url into Django~~ Fixed #28236 - Integrate dj-database-url into Django Feb 4, 2018

wkschwartz reviewed Feb 5, 2018

View reviewed changes

Remove accidental paste

9699db7

rsalmaso mentioned this pull request Dec 23, 2018

[WIP] Fixed #28236 - Adapt dj-database-url into Django #10786

Closed

felixxm closed this Jul 8, 2020

rtpg mentioned this pull request Nov 27, 2022

Fixed #28236 - Integrate dj-database-url into Django #16331

Closed


		BACKENDS = defaultdict(dict)

		BaseParsedURL = namedtuple('ParsedURL', 'scheme username password hostname port path options')

		return BACKENDS[backend_type][scheme]


		def configure(backend_type, value):



		def register_db_backend(scheme, path):
		if not path.endswith('.base.DatabaseWrapper'):

Uh oh!

Fixed #28236 - Integrate dj-database-url into Django #8562

Fixed #28236 - Integrate dj-database-url into Django #8562

Uh oh!

Conversation

orf commented May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennethreitz commented May 27, 2017

Uh oh!

kennethreitz commented Jun 9, 2017

Uh oh!

orf commented Jun 9, 2017 via email

Uh oh!

kennethreitz commented Jun 9, 2017

Uh oh!

kennethreitz commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orf commented Aug 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auvipy commented Sep 14, 2017

Uh oh!

orf commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orf commented Sep 14, 2017

Uh oh!

orf commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wkschwartz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orf commented May 27, 2017 •

edited

Loading

orf commented Sep 15, 2017 •

edited

Loading