Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions configurations/values.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
import decimal
import os
import sys
import warnings

from django.core import validators
from django.core.exceptions import ValidationError, ImproperlyConfigured
from environ import Env
from django.utils.module_loading import import_string

from .utils import getargspec
Expand Down Expand Up @@ -418,8 +420,24 @@ def setup(self, name):
return value


def env_email_url_config(cls, url, backend=None):
"""
Convert schema to consolemail in order to be compatible with old
console:// schema.

This could be removed and set EmailURLValue.caster to Env.email_url_config
directly after the deprecation is removed.
"""
if url.startswith('console://'):
warnings.warn('Email schema console:// is deprecated. Please use '
'consolemail:// in new code.',
DeprecationWarning)
url = url.replace('console://', 'consolemail://')
return Env.email_url_config(url, backend=backend)


class EmailURLValue(CastingMixin, MultipleMixin, Value):
caster = 'dj_email_url.parse'
caster = env_email_url_config
message = 'Cannot interpret email URL value {0!r}'
late_binding = True

Expand Down Expand Up @@ -454,21 +472,21 @@ def to_python(self, value):


class DatabaseURLValue(DictBackendMixin, CastingMixin, Value):
caster = 'dj_database_url.parse'
caster = Env.db_url_config
message = 'Cannot interpret database URL value {0!r}'
environ_name = 'DATABASE_URL'
late_binding = True


class CacheURLValue(DictBackendMixin, CastingMixin, Value):
caster = 'django_cache_url.parse'
caster = Env.cache_url_config
message = 'Cannot interpret cache URL value {0!r}'
environ_name = 'CACHE_URL'
late_binding = True


class SearchURLValue(DictBackendMixin, CastingMixin, Value):
caster = 'dj_search_url.parse'
caster = Env.search_url_config
message = 'Cannot interpret Search URL value {0!r}'
environ_name = 'SEARCH_URL'
late_binding = True
13 changes: 6 additions & 7 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,15 @@ def read(*parts):
'django-cadmin = configurations.management:execute_from_command_line',
],
},
install_requires=[
'django>=2.2',
'importlib-metadata;python_version<"3.8"',

install_requires=['django-environ',
'django>=2.2',
'importlib-metadata;python_version<"3.8"',
],
extras_require={
'cache': ['django-cache-url'],
'database': ['dj-database-url'],
'email': ['dj-email-url'],
'search': ['dj-search-url'],
'testing': [
'django-discover-runner',
'mock',
'django-cache-url>=1.0.0',
'dj-database-url',
'dj-email-url',
Copy link
Contributor

Choose a reason for hiding this comment

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

These 4 packages need to be removed from the testing extra too.

Expand Down
21 changes: 7 additions & 14 deletions tests/test_values.py
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,6 @@ def test_database_url_value(self):
with env(DATABASE_URL='sqlite://'):
self.assertEqual(value.setup('DATABASE_URL'), {
'default': {
'CONN_MAX_AGE': 0,
'ENGINE': 'django.db.backends.sqlite3',
'HOST': '',
'NAME': ':memory:',
Expand Down Expand Up @@ -411,7 +410,6 @@ def test_email_url_value(self):
'EMAIL_HOST_PASSWORD': 'password',
'EMAIL_HOST_USER': '[email protected]',
'EMAIL_PORT': 587,
'EMAIL_USE_SSL': False,
'EMAIL_USE_TLS': True})
with env(EMAIL_URL='console://'):
self.assertEqual(value.setup('EMAIL_URL'), {
Expand All @@ -420,17 +418,19 @@ def test_email_url_value(self):
'EMAIL_HOST': None,
'EMAIL_HOST_PASSWORD': None,
'EMAIL_HOST_USER': None,
'EMAIL_PORT': None,
'EMAIL_USE_SSL': False,
'EMAIL_USE_TLS': False})
'EMAIL_PORT': None})
with env(EMAIL_URL='console://'):
with patch('warnings.warn') as warn:
value.setup('EMAIL_URL')
warn.asset_called_once()
with env(EMAIL_URL='smtps://[email protected]:[email protected]:wrong'): # noqa: E501
self.assertRaises(ValueError, value.setup, 'TEST')

def test_cache_url_value(self):
cache_setting = {
'default': {
'BACKEND': 'django_redis.cache.RedisCache',
'LOCATION': 'redis://host:6379/1',
'LOCATION': 'redis://user@host:6379/1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently it changes some defaults (more secure by default).

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if I get your point. Is this change acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It changes defaults apparently, so we should bump the major or at least minor version for it.

}
}
cache_url = 'redis://user@host:6379/1'
Expand All @@ -443,13 +443,7 @@ def test_cache_url_value(self):
with env(CACHE_URL='wrong://user@host:port/1'):
with self.assertRaises(Exception) as cm:
value.setup('TEST')
self.assertEqual(cm.exception.args[0], 'Unknown backend: "wrong"')
with env(CACHE_URL='redis://user@host:port/1'):
with self.assertRaises(ValueError) as cm:
value.setup('TEST')
self.assertEqual(
cm.exception.args[0],
"Cannot interpret cache URL value 'redis://user@host:port/1'")
self.assertEqual(cm.exception.args[0], 'wrong')
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks "wrong"?!
Shouldn't it have more info / a full msg?

Copy link
Author

Choose a reason for hiding this comment

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

django-environ simply raises KeyError when looking up a cache scheme by wrong, and here assertRaises catches the KeyError.

It might be good for django-envion to catch the KeyError and reraise a specific error with more descriptive message.

What do you think?

Copy link
Author

@tkdchen tkdchen Oct 11, 2019

Choose a reason for hiding this comment

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

FYI, I made a patch to django-environ, joke2k/django-environ#235

After this patch is accepted and merged, this assertion could be updated accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

@tkdchen , that patch was merged; I guess this can be updated now ?


def test_search_url_value(self):
value = SearchURLValue()
Expand Down Expand Up @@ -503,7 +497,6 @@ class Target:
'EMAIL_HOST_PASSWORD': 'password',
'EMAIL_HOST_USER': '[email protected]',
'EMAIL_PORT': 587,
'EMAIL_USE_SSL': False,
'EMAIL_USE_TLS': True
})
self.assertEqual(
Expand Down