From 88ea5d6bb002186be171aaa56e5467adb5212ae1 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Mon, 29 Dec 2014 13:00:22 +0000 Subject: [PATCH 01/11] emit a warning when a setting has an unexpected type --- rest_framework/exceptions.py | 4 ++++ rest_framework/settings.py | 13 +++++++++++++ tests/test_settings.py | 20 ++++++++++++++++++++ 3 files changed, 37 insertions(+) diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 1f381e4ef..710da96b8 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -145,3 +145,7 @@ class Throttled(APIException): self.detail += ' ' + force_text( self.extra_detail % {'wait': self.wait} ) + + +class RESTFrameworkSettingHasUnexpectedClassWarning(Warning): + pass diff --git a/rest_framework/settings.py b/rest_framework/settings.py index 33f848138..3396635d8 100644 --- a/rest_framework/settings.py +++ b/rest_framework/settings.py @@ -18,9 +18,11 @@ REST framework settings, checking for user settings first, then falling back to the defaults. """ from __future__ import unicode_literals +import warnings from django.conf import settings from django.utils import importlib, six from rest_framework import ISO_8601 +from rest_framework.exceptions import RESTFrameworkSettingHasUnexpectedClassWarning USER_SETTINGS = getattr(settings, 'REST_FRAMEWORK', None) @@ -187,6 +189,17 @@ class APISettings(object): except KeyError: # Fall back to defaults val = self.defaults[attr] + else: + expected_class = self.defaults[attr].__class__ + if val.__class__ != expected_class: + warnings.warn( + "The `{val}` setting has the class {val.__class__}, " + "but the expected class was {expected_class}".format( + **locals() + ), + RESTFrameworkSettingHasUnexpectedClassWarning, + stacklevel=3 + ) # Coerce import strings into classes if val and attr in self.import_strings: diff --git a/tests/test_settings.py b/tests/test_settings.py index f2ff4ca14..f5bde4b06 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -1,5 +1,8 @@ from __future__ import unicode_literals +import warnings from django.test import TestCase +from rest_framework.exceptions import \ + RESTFrameworkSettingHasUnexpectedClassWarning from rest_framework.settings import APISettings @@ -15,3 +18,20 @@ class TestSettings(TestCase): }) with self.assertRaises(ImportError): settings.DEFAULT_RENDERER_CLASSES + + def test_bad_setting_class_raises_warning(self): + """ + Make sure warnings are emitted when settings have an unexpected class. + """ + settings = APISettings({ + 'DEFAULT_RENDERER_CLASSES': 'rest_framework.renderers.JSONRenderer' + }) + + with warnings.catch_warnings(record=True) as w: + # Trigger a warning. + settings.DEFAULT_RENDERER_CLASSES + # Verify that a warning is thrown + assert len(w) == 1 + assert issubclass( + w[-1].category, RESTFrameworkSettingHasUnexpectedClassWarning + ) From eb04133bb9264f06f56719ef820b8edf403311ca Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Mon, 29 Dec 2014 13:15:54 +0000 Subject: [PATCH 02/11] emit a warning when a setting has an unexpected type --- rest_framework/settings.py | 21 +++++++++++++++------ tests/test_settings.py | 24 ++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/rest_framework/settings.py b/rest_framework/settings.py index 3396635d8..20594926b 100644 --- a/rest_framework/settings.py +++ b/rest_framework/settings.py @@ -18,6 +18,7 @@ REST framework settings, checking for user settings first, then falling back to the defaults. """ from __future__ import unicode_literals +from collections import Iterable import warnings from django.conf import settings from django.utils import importlib, six @@ -190,13 +191,21 @@ class APISettings(object): # Fall back to defaults val = self.defaults[attr] else: - expected_class = self.defaults[attr].__class__ - if val.__class__ != expected_class: + # Verify that the user hasn't accidentally given a string instead + # of an iterable, or vice-versa + default = self.defaults[attr] + if issubclass(val.__class__, Iterable) \ + and (not issubclass(default.__class__, Iterable) + or isinstance(val, basestring)): warnings.warn( - "The `{val}` setting has the class {val.__class__}, " - "but the expected class was {expected_class}".format( - **locals() - ), + "The `{attr}` setting must be iterable".format(**locals()), + RESTFrameworkSettingHasUnexpectedClassWarning, + stacklevel=3 + ) + elif isinstance(default, basestring) and not \ + isinstance(val, basestring): + warnings.warn( + "The `{attr}` setting must be a string".format(**locals()), RESTFrameworkSettingHasUnexpectedClassWarning, stacklevel=3 ) diff --git a/tests/test_settings.py b/tests/test_settings.py index f5bde4b06..337759726 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -19,9 +19,10 @@ class TestSettings(TestCase): with self.assertRaises(ImportError): settings.DEFAULT_RENDERER_CLASSES - def test_bad_setting_class_raises_warning(self): + def test_bad_iterable_setting_class_raises_warning(self): """ - Make sure warnings are emitted when settings have an unexpected class. + Make sure warnings are emitted when settings which should be iterable + are not. """ settings = APISettings({ 'DEFAULT_RENDERER_CLASSES': 'rest_framework.renderers.JSONRenderer' @@ -35,3 +36,22 @@ class TestSettings(TestCase): assert issubclass( w[-1].category, RESTFrameworkSettingHasUnexpectedClassWarning ) + + def test_bad_string_setting_class_raises_warning(self): + """ + Make sure warnings are emitted when settings which should be strings are + not. + """ + settings = APISettings({ + 'DEFAULT_METADATA_CLASS': [] + }) + + with warnings.catch_warnings(record=True) as w: + # Trigger a warning. + settings.DEFAULT_METADATA_CLASS + + # Verify that a warning is thrown + assert len(w) == 1 + assert issubclass( + w[-1].category, RESTFrameworkSettingHasUnexpectedClassWarning + ) From 7308e34c2ef3ed175b62d27636c4f003d830ed1b Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Wed, 31 Dec 2014 18:11:09 +0000 Subject: [PATCH 03/11] python 3 compat --- rest_framework/settings.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rest_framework/settings.py b/rest_framework/settings.py index 20594926b..a5b28fa19 100644 --- a/rest_framework/settings.py +++ b/rest_framework/settings.py @@ -196,14 +196,14 @@ class APISettings(object): default = self.defaults[attr] if issubclass(val.__class__, Iterable) \ and (not issubclass(default.__class__, Iterable) - or isinstance(val, basestring)): + or isinstance(val, six.string_types)): warnings.warn( "The `{attr}` setting must be iterable".format(**locals()), RESTFrameworkSettingHasUnexpectedClassWarning, stacklevel=3 ) - elif isinstance(default, basestring) and not \ - isinstance(val, basestring): + elif isinstance(default, six.string_types) and not \ + isinstance(val, six.string_types): warnings.warn( "The `{attr}` setting must be a string".format(**locals()), RESTFrameworkSettingHasUnexpectedClassWarning, From 49ad4750c692d0ad88058f1356d0adcaa1f56f09 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 17:37:41 +0000 Subject: [PATCH 04/11] raise ImproperlyConfigured error when settings have the wrong type --- rest_framework/exceptions.py | 3 --- rest_framework/settings.py | 16 ++++++---------- tests/test_settings.py | 36 +++++++++++++++++------------------- 3 files changed, 23 insertions(+), 32 deletions(-) diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 710da96b8..9b324a743 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -146,6 +146,3 @@ class Throttled(APIException): self.extra_detail % {'wait': self.wait} ) - -class RESTFrameworkSettingHasUnexpectedClassWarning(Warning): - pass diff --git a/rest_framework/settings.py b/rest_framework/settings.py index a5b28fa19..7e8ea7449 100644 --- a/rest_framework/settings.py +++ b/rest_framework/settings.py @@ -19,11 +19,10 @@ back to the defaults. """ from __future__ import unicode_literals from collections import Iterable -import warnings +from django.core.exceptions import ImproperlyConfigured from django.conf import settings from django.utils import importlib, six from rest_framework import ISO_8601 -from rest_framework.exceptions import RESTFrameworkSettingHasUnexpectedClassWarning USER_SETTINGS = getattr(settings, 'REST_FRAMEWORK', None) @@ -197,17 +196,14 @@ class APISettings(object): if issubclass(val.__class__, Iterable) \ and (not issubclass(default.__class__, Iterable) or isinstance(val, six.string_types)): - warnings.warn( - "The `{attr}` setting must be iterable".format(**locals()), - RESTFrameworkSettingHasUnexpectedClassWarning, - stacklevel=3 + raise ImproperlyConfigured( + 'The "{attr}" setting must be a list or a tuple' + .format(attr=attr) ) elif isinstance(default, six.string_types) and not \ isinstance(val, six.string_types): - warnings.warn( - "The `{attr}` setting must be a string".format(**locals()), - RESTFrameworkSettingHasUnexpectedClassWarning, - stacklevel=3 + raise ImproperlyConfigured( + 'The "{attr}" setting must be a string'.format(attr=attr) ) # Coerce import strings into classes diff --git a/tests/test_settings.py b/tests/test_settings.py index 337759726..cabc9a056 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -1,8 +1,8 @@ from __future__ import unicode_literals +from django.core.exceptions import ImproperlyConfigured +import pytest import warnings from django.test import TestCase -from rest_framework.exceptions import \ - RESTFrameworkSettingHasUnexpectedClassWarning from rest_framework.settings import APISettings @@ -21,37 +21,35 @@ class TestSettings(TestCase): def test_bad_iterable_setting_class_raises_warning(self): """ - Make sure warnings are emitted when settings which should be iterable - are not. + Make sure errors are raised when settings which should be iterable are + not. """ settings = APISettings({ 'DEFAULT_RENDERER_CLASSES': 'rest_framework.renderers.JSONRenderer' }) - with warnings.catch_warnings(record=True) as w: - # Trigger a warning. + # Trigger the exception + with pytest.raises(ImproperlyConfigured) as exc_info: settings.DEFAULT_RENDERER_CLASSES - # Verify that a warning is thrown - assert len(w) == 1 - assert issubclass( - w[-1].category, RESTFrameworkSettingHasUnexpectedClassWarning - ) + expected_error = ( + u'The "DEFAULT_RENDERER_CLASSES" setting must be a list or a tuple' + ) + assert exc_info.value[0] == expected_error def test_bad_string_setting_class_raises_warning(self): """ - Make sure warnings are emitted when settings which should be strings are + Make sure errors are raised when settings which should be strings are not. """ settings = APISettings({ 'DEFAULT_METADATA_CLASS': [] }) - with warnings.catch_warnings(record=True) as w: - # Trigger a warning. + # Trigger the exception + with pytest.raises(ImproperlyConfigured) as exc_info: settings.DEFAULT_METADATA_CLASS - # Verify that a warning is thrown - assert len(w) == 1 - assert issubclass( - w[-1].category, RESTFrameworkSettingHasUnexpectedClassWarning - ) + expected_error = ( + u'The "DEFAULT_METADATA_CLASS" setting must be a string' + ) + assert exc_info.value[0] == expected_error From e3169de5cd6e37ab7c32b9d745843ba7907d7280 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 17:50:50 +0000 Subject: [PATCH 05/11] remove unused import --- rest_framework/settings.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/rest_framework/settings.py b/rest_framework/settings.py index 7e8ea7449..b3c05fc52 100644 --- a/rest_framework/settings.py +++ b/rest_framework/settings.py @@ -197,13 +197,20 @@ class APISettings(object): and (not issubclass(default.__class__, Iterable) or isinstance(val, six.string_types)): raise ImproperlyConfigured( - 'The "{attr}" setting must be a list or a tuple' - .format(attr=attr) + 'The "{settings_key}" setting must be a list or tuple, but ' + 'got type "{type_name}" with value "{value}".'.format( + settings_key=attr, type_name=val.__class__.__name__, + value=val + ) ) elif isinstance(default, six.string_types) and not \ isinstance(val, six.string_types): raise ImproperlyConfigured( - 'The "{attr}" setting must be a string'.format(attr=attr) + 'The "{settings_key}" setting must be a string, but ' + 'got type "{type_name}" with value "{value}".'.format( + settings_key=attr, type_name=val.__class__.__name__, + value=val + ) ) # Coerce import strings into classes From b88aea76e97f43eb2c31e25c05d8cc20dc51a194 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 17:51:03 +0000 Subject: [PATCH 06/11] update expected error messages --- tests/test_settings.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index cabc9a056..bbf62b667 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -1,7 +1,6 @@ from __future__ import unicode_literals from django.core.exceptions import ImproperlyConfigured import pytest -import warnings from django.test import TestCase from rest_framework.settings import APISettings @@ -32,7 +31,9 @@ class TestSettings(TestCase): with pytest.raises(ImproperlyConfigured) as exc_info: settings.DEFAULT_RENDERER_CLASSES expected_error = ( - u'The "DEFAULT_RENDERER_CLASSES" setting must be a list or a tuple' + u'The "DEFAULT_RENDERER_CLASSES" setting must be a list or ' + u'tuple, but got type "unicode" with value ' + u'"rest_framework.renderers.JSONRenderer".' ) assert exc_info.value[0] == expected_error @@ -50,6 +51,7 @@ class TestSettings(TestCase): settings.DEFAULT_METADATA_CLASS expected_error = ( - u'The "DEFAULT_METADATA_CLASS" setting must be a string' + u'The "DEFAULT_METADATA_CLASS" setting must be a string, but got ' + u'type "list" with value "[]".' ) assert exc_info.value[0] == expected_error From e2bbd8845b47725e680557ce968a7f9f81a1d0bf Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 17:54:01 +0000 Subject: [PATCH 07/11] remove extra blank line --- rest_framework/exceptions.py | 1 - 1 file changed, 1 deletion(-) diff --git a/rest_framework/exceptions.py b/rest_framework/exceptions.py index 9b324a743..1f381e4ef 100644 --- a/rest_framework/exceptions.py +++ b/rest_framework/exceptions.py @@ -145,4 +145,3 @@ class Throttled(APIException): self.detail += ' ' + force_text( self.extra_detail % {'wait': self.wait} ) - From ce5835f10ad7df307591790f3c5b23875da12b24 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 17:58:27 +0000 Subject: [PATCH 08/11] dont index directly on to exceptions --- tests/test_settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index bbf62b667..928115a36 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -35,7 +35,7 @@ class TestSettings(TestCase): u'tuple, but got type "unicode" with value ' u'"rest_framework.renderers.JSONRenderer".' ) - assert exc_info.value[0] == expected_error + assert exc_info.value.args[0] == expected_error def test_bad_string_setting_class_raises_warning(self): """ @@ -54,4 +54,4 @@ class TestSettings(TestCase): u'The "DEFAULT_METADATA_CLASS" setting must be a string, but got ' u'type "list" with value "[]".' ) - assert exc_info.value[0] == expected_error + assert exc_info.value.args[0] == expected_error From 28744d99bde588da4e698fb7d072fc6653028a89 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 18:01:48 +0000 Subject: [PATCH 09/11] calculate the expected error message depending on the python version --- tests/test_settings.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index 928115a36..cf0952122 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,6 +2,7 @@ from __future__ import unicode_literals from django.core.exceptions import ImproperlyConfigured import pytest from django.test import TestCase +import six from rest_framework.settings import APISettings @@ -30,10 +31,15 @@ class TestSettings(TestCase): # Trigger the exception with pytest.raises(ImproperlyConfigured) as exc_info: settings.DEFAULT_RENDERER_CLASSES + + # Construct the expected error message + text_type_name = str(six.text_type.__name__) expected_error = ( u'The "DEFAULT_RENDERER_CLASSES" setting must be a list or ' - u'tuple, but got type "unicode" with value ' - u'"rest_framework.renderers.JSONRenderer".' + u'tuple, but got type "{text_type_name}" with value ' + u'"rest_framework.renderers.JSONRenderer".'.format( + text_type_name=text_type_name + ) ) assert exc_info.value.args[0] == expected_error From 0aed9a8ed2a0e86c100891bf288e85fdacbb0edc Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 18:05:42 +0000 Subject: [PATCH 10/11] use correct six import --- tests/test_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index cf0952122..a923c9db0 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -2,7 +2,7 @@ from __future__ import unicode_literals from django.core.exceptions import ImproperlyConfigured import pytest from django.test import TestCase -import six +from django.utils import six from rest_framework.settings import APISettings From 513f8525dba376bfd58e57b15252153d14eab323 Mon Sep 17 00:00:00 2001 From: Craig Blaszczyk Date: Thu, 8 Jan 2015 18:20:35 +0000 Subject: [PATCH 11/11] switch to non-unicode strings to support Python 3.2 --- tests/test_settings.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index a923c9db0..335957df6 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -35,9 +35,9 @@ class TestSettings(TestCase): # Construct the expected error message text_type_name = str(six.text_type.__name__) expected_error = ( - u'The "DEFAULT_RENDERER_CLASSES" setting must be a list or ' - u'tuple, but got type "{text_type_name}" with value ' - u'"rest_framework.renderers.JSONRenderer".'.format( + 'The "DEFAULT_RENDERER_CLASSES" setting must be a list or ' + 'tuple, but got type "{text_type_name}" with value ' + '"rest_framework.renderers.JSONRenderer".'.format( text_type_name=text_type_name ) ) @@ -57,7 +57,7 @@ class TestSettings(TestCase): settings.DEFAULT_METADATA_CLASS expected_error = ( - u'The "DEFAULT_METADATA_CLASS" setting must be a string, but got ' - u'type "list" with value "[]".' + 'The "DEFAULT_METADATA_CLASS" setting must be a string, but got ' + 'type "list" with value "[]".' ) assert exc_info.value.args[0] == expected_error