From 7415b74980824e9032b646b89c6712c09e9d7d90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Sat, 15 Nov 2014 12:39:12 +0100 Subject: [PATCH 1/4] Providing meaningful error message if you forget many=True. Closes #1914. --- rest_framework/serializers.py | 11 ++++++++++- tests/test_serializer.py | 13 +++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 2d5c843e5..6c1a461ed 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -70,6 +70,7 @@ class BaseSerializer(Field): self.partial = kwargs.pop('partial', False) self._context = kwargs.pop('context', {}) kwargs.pop('many', None) + kwargs.pop('many_init', None) super(BaseSerializer, self).__init__(**kwargs) def __new__(cls, *args, **kwargs): @@ -77,6 +78,14 @@ class BaseSerializer(Field): # `ListSerializer` classes instead when `many=True` is set. if kwargs.pop('many', False): return cls.many_init(*args, **kwargs) + if not kwargs.pop('many_init', False): + if not issubclass(cls, ListSerializer): + instance = kwargs.get('instance', args[0] if args else None) + if isinstance(instance, (list, tuple, models.QuerySet)): + msg = ( + 'You have passed a %s as `instance` argument but did ' + 'not set `many=True`.' % instance.__class__.__name__) + raise AssertionError(msg) return super(BaseSerializer, cls).__new__(cls, *args, **kwargs) @classmethod @@ -87,7 +96,7 @@ class BaseSerializer(Field): control which keyword arguments are passed to the parent, and which are passed to the child. """ - child_serializer = cls(*args, **kwargs) + child_serializer = cls(many_init=True, *args, **kwargs) list_kwargs = {'child': child_serializer} list_kwargs.update(dict([ (key, value) for key, value in kwargs.items() diff --git a/tests/test_serializer.py b/tests/test_serializer.py index 77d5c3199..265369591 100644 --- a/tests/test_serializer.py +++ b/tests/test_serializer.py @@ -1,3 +1,4 @@ +from django.db.models.query import QuerySet from rest_framework import serializers import pytest @@ -42,6 +43,18 @@ class TestSerializer: with pytest.raises(AttributeError): serializer.data + def test_missing_many_kwarg(self): + self.Serializer([], many=True) + with pytest.raises(AssertionError): + self.Serializer([]) + with pytest.raises(AssertionError): + self.Serializer(()) + with pytest.raises(AssertionError): + self.Serializer(QuerySet()) + self.Serializer(None) + self.Serializer(object()) + self.Serializer({}) + class TestValidateMethod: def test_non_field_error_validate_method(self): From 22f059a25ab5a1f03f59fc38c73ef3601188cccb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Sat, 15 Nov 2014 14:35:37 +0100 Subject: [PATCH 2/4] Fixing QuerySet import for Django < 1.7. --- rest_framework/serializers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 6c1a461ed..f938704d7 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -12,6 +12,7 @@ response content is handled by parsers and renderers. """ from django.core.exceptions import ImproperlyConfigured from django.db import models +from django.db.models.query import QuerySet from django.db.models.fields import FieldDoesNotExist from django.utils import six from django.utils.translation import ugettext_lazy as _ @@ -81,7 +82,7 @@ class BaseSerializer(Field): if not kwargs.pop('many_init', False): if not issubclass(cls, ListSerializer): instance = kwargs.get('instance', args[0] if args else None) - if isinstance(instance, (list, tuple, models.QuerySet)): + if isinstance(instance, (list, tuple, QuerySet)): msg = ( 'You have passed a %s as `instance` argument but did ' 'not set `many=True`.' % instance.__class__.__name__) From 30ce4c6413c4da54252ad8c893a8bb48390ee9bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Mon, 17 Nov 2014 23:26:07 +0100 Subject: [PATCH 3/4] Catching the error that might arise from not passing many=True to a serializer in the data property instead of the serializer's __init__ method. --- rest_framework/serializers.py | 28 ++++++++++++++--------- tests/test_serializer.py | 43 +++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 21 deletions(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index f938704d7..2c14f000d 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -35,6 +35,7 @@ from rest_framework.validators import ( ) import copy import inspect +import sys import warnings # Note: We do the following so that users of the framework can use this style: @@ -71,7 +72,6 @@ class BaseSerializer(Field): self.partial = kwargs.pop('partial', False) self._context = kwargs.pop('context', {}) kwargs.pop('many', None) - kwargs.pop('many_init', None) super(BaseSerializer, self).__init__(**kwargs) def __new__(cls, *args, **kwargs): @@ -79,14 +79,6 @@ class BaseSerializer(Field): # `ListSerializer` classes instead when `many=True` is set. if kwargs.pop('many', False): return cls.many_init(*args, **kwargs) - if not kwargs.pop('many_init', False): - if not issubclass(cls, ListSerializer): - instance = kwargs.get('instance', args[0] if args else None) - if isinstance(instance, (list, tuple, QuerySet)): - msg = ( - 'You have passed a %s as `instance` argument but did ' - 'not set `many=True`.' % instance.__class__.__name__) - raise AssertionError(msg) return super(BaseSerializer, cls).__new__(cls, *args, **kwargs) @classmethod @@ -97,7 +89,7 @@ class BaseSerializer(Field): control which keyword arguments are passed to the parent, and which are passed to the child. """ - child_serializer = cls(many_init=True, *args, **kwargs) + child_serializer = cls(*args, **kwargs) list_kwargs = {'child': child_serializer} list_kwargs.update(dict([ (key, value) for key, value in kwargs.items() @@ -169,7 +161,21 @@ class BaseSerializer(Field): def data(self): if not hasattr(self, '_data'): if self.instance is not None and not getattr(self, '_errors', None): - self._data = self.to_representation(self.instance) + try: + self._data = self.to_representation(self.instance) + except Exception as exc: + if isinstance(self.instance, (list, tuple, QuerySet)): + msg = ( + 'The reason for this exception might be that you ' + 'have passed a %s as `instance` argument to the ' + 'serializer but did not set `many=True`.' + % type(self.instance).__name__) + six.reraise( + type(exc), + type(exc)(str(exc) + '. ' + msg), + sys.exc_info()[2]) + else: + six.reraise(*sys.exc_info()) elif hasattr(self, '_validated_data') and not getattr(self, '_errors', None): self._data = self.to_representation(self.validated_data) else: diff --git a/tests/test_serializer.py b/tests/test_serializer.py index 265369591..b4d5bd441 100644 --- a/tests/test_serializer.py +++ b/tests/test_serializer.py @@ -44,16 +44,39 @@ class TestSerializer: serializer.data def test_missing_many_kwarg(self): - self.Serializer([], many=True) - with pytest.raises(AssertionError): - self.Serializer([]) - with pytest.raises(AssertionError): - self.Serializer(()) - with pytest.raises(AssertionError): - self.Serializer(QuerySet()) - self.Serializer(None) - self.Serializer(object()) - self.Serializer({}) + serializer = self.Serializer([], many=True) + assert serializer.data == [] + + expect_failure = ( + (), + [], + QuerySet() + ) + + for failure in expect_failure: + serializer = self.Serializer(failure) + with pytest.raises(AttributeError): + serializer.data + + # Check that message was correctly annotated. + try: + serializer.data + except Exception as exc: + assert 'many=True' in str(exc) + + serializer = self.Serializer(None) + assert serializer.data + + serializer = self.Serializer(object()) + with pytest.raises(AttributeError): + serializer.data + + # Check that message was NOT annotated. object() is not list but does + # not has the required `char` and `integer` attributes. + try: + serializer.data + except Exception as exc: + assert 'many=True' not in str(exc) class TestValidateMethod: From 2b3d4a378353e52c3038e3c74f26ab8366b8b10a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gregor=20M=C3=BCllegger?= Date: Fri, 21 Nov 2014 08:32:53 +0100 Subject: [PATCH 4/4] Checking for a more specific exception when user might have passed in a list without specifying many=True. --- rest_framework/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 2c14f000d..1a06308e7 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -163,7 +163,7 @@ class BaseSerializer(Field): if self.instance is not None and not getattr(self, '_errors', None): try: self._data = self.to_representation(self.instance) - except Exception as exc: + except AttributeError as exc: if isinstance(self.instance, (list, tuple, QuerySet)): msg = ( 'The reason for this exception might be that you '