From 69dfb5d818f6a8b73695593fe11243ddc3c9a5c8 Mon Sep 17 00:00:00 2001 From: Craig de Stigter Date: Tue, 24 Nov 2015 09:53:02 +1300 Subject: [PATCH 1/4] Fix overzealous RelatedField queryset validation. Otherwise if you want to subclass RelatedField in a read+write capacity you end up with this weird situation: ``` class MyField(serializers.RelatedField): def __init__(self, **kwargs): if not kwargs.get('read_only'): kwargs.setdefault('queryset', MyModel.objects.all()) super(MyField, self).__init__(**kwargs) ``` This commit removes the check preventing you from just supplying the queryset anyway. Why shouldn't you? --- rest_framework/relations.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/rest_framework/relations.py b/rest_framework/relations.py index 744823c48..0bcdcdd95 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -69,10 +69,6 @@ class RelatedField(Field): 'Relational field must provide a `queryset` argument, ' 'or set read_only=`True`.' ) - assert not (self.queryset is not None and kwargs.get('read_only', None)), ( - 'Relational fields should not provide a `queryset` argument, ' - 'when setting read_only=`True`.' - ) kwargs.pop('many', None) kwargs.pop('allow_empty', None) super(RelatedField, self).__init__(**kwargs) From 3747e9baa88f4614aed04df799637fe0c8839b98 Mon Sep 17 00:00:00 2001 From: Craig de Stigter Date: Fri, 4 Dec 2015 14:11:55 +1300 Subject: [PATCH 2/4] Add support for overriding fields in serializer subclasses. This is an updated version of tomchristie/django-rest-framework#1053 applied to current master. --- rest_framework/serializers.py | 12 ++++- tests/test_serializer.py | 82 +++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 99d36a8a5..e168abb25 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -280,11 +280,19 @@ class SerializerMetaclass(type): # If this class is subclassing another Serializer, add that Serializer's # fields. Note that we loop over the bases in *reverse*. This is necessary # in order to maintain the correct order of fields. + all_fields = OrderedDict() for base in reversed(bases): if hasattr(base, '_declared_fields'): - fields = list(base._declared_fields.items()) + fields + for name, field in base._declared_fields.items(): + if name in all_fields: + # Throw away old ordering, then replace with new one + all_fields.pop(name) + all_fields[name] = field - return OrderedDict(fields) + # if there are fields in both base_fields and fields, OrderedDict + # uses the *last* one defined. So fields needs to go last. + all_fields.update(OrderedDict(fields)) + return all_fields def __new__(cls, name, bases, attrs): attrs['_declared_fields'] = cls._get_declared_fields(bases, attrs) diff --git a/tests/test_serializer.py b/tests/test_serializer.py index 741c6ab17..65dd4aca2 100644 --- a/tests/test_serializer.py +++ b/tests/test_serializer.py @@ -309,3 +309,85 @@ class TestCacheSerializerData: pickled = pickle.dumps(serializer.data) data = pickle.loads(pickled) assert data == {'field1': 'a', 'field2': 'b'} + + +class TestSerializerSupportsOverriddenFields: + def setup(self): + class Base1(serializers.Serializer): + a_field = serializers.CharField() + self.Base1 = Base1 + + class Base2(serializers.Serializer): + a_field = serializers.IntegerField() + self.Base2 = Base2 + + def test_base_fields_unchanged(self): + """ + Overriding a field in a subclassed serializer shouldn't change the + field on the superclass + """ + class OverriddenFields(self.Base1): + a_field = serializers.FloatField() + + assert isinstance( + self.Base1._declared_fields['a_field'], + serializers.CharField, + ) + s = self.Base1() + assert isinstance(s.fields['a_field'], serializers.CharField) + + def test_overridden_fields_single_base(self): + """ + Subclassing a serializer and overriding a field should mean the field + on the subclass wins. + """ + class OverriddenFieldsWithSingleBase(self.Base1): + a_field = serializers.FloatField() + + assert isinstance( + OverriddenFieldsWithSingleBase._declared_fields['a_field'], + serializers.FloatField, + ) + s = OverriddenFieldsWithSingleBase() + assert isinstance(s.fields['a_field'], serializers.FloatField) + + def test_overridden_fields_multiple_bases(self): + """ + For serializers with multiple bases, the field on the first base wins + (as per normal python method resolution order) + """ + class OverriddenFieldsMultipleBases1(self.Base1, self.Base2): + # first base takes precedence; a_field should be a CharField. + pass + + assert isinstance( + OverriddenFieldsMultipleBases1._declared_fields['a_field'], + serializers.CharField, + ) + s = OverriddenFieldsMultipleBases1() + assert isinstance(s.fields['a_field'], serializers.CharField) + + class OverriddenFieldsMultipleBases2(self.Base2, self.Base1): + # first base takes precedence; a_field should be a IntegerField. + pass + + assert isinstance( + OverriddenFieldsMultipleBases2._declared_fields['a_field'], + serializers.IntegerField, + ) + s = OverriddenFieldsMultipleBases2() + assert isinstance(s.fields['a_field'], serializers.IntegerField) + + def test_overridden_fields_multiple_bases_overridden(self): + """ + For serializers with multiple bases, locally defined fields still win. + """ + class OverriddenFieldsMultipleBasesOverridden(self.Base1, self.Base2): + a_field = serializers.FloatField() + + assert isinstance( + OverriddenFieldsMultipleBasesOverridden._declared_fields['a_field'], + serializers.FloatField, + ) + s = OverriddenFieldsMultipleBasesOverridden() + assert isinstance(s.fields['a_field'], serializers.FloatField) From 8f108e8bcf2d5cbef52fe70d522f1f03f293d649 Mon Sep 17 00:00:00 2001 From: Craig de Stigter Date: Wed, 6 Jan 2016 16:04:23 +1300 Subject: [PATCH 3/4] Fix nested validation error being rendered incorrectly. Previously an extra list wrapped nested validation errors raised from serializer's validate() methods. That was inconsistent with the format of validation errors raised by validate_ methods. i.e. these two resulted in *different* behaviour: def validate_foo(self): raise ValidationError(['bar']) def validate(self): raise ValidationError({'foo': ['bar']}) --- rest_framework/serializers.py | 2 +- tests/test_validation.py | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index e168abb25..3756aec01 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -314,7 +314,7 @@ def get_validation_error_detail(exc): # If errors may be a dict we use the standard {key: list of values}. # Here we ensure that all the values are *lists* of errors. return { - key: value if isinstance(value, list) else [value] + key: value if isinstance(value, (list, dict)) else [value] for key, value in exc.detail.items() } elif isinstance(exc.detail, list): diff --git a/tests/test_validation.py b/tests/test_validation.py index 855ff20e0..b6f274219 100644 --- a/tests/test_validation.py +++ b/tests/test_validation.py @@ -49,6 +49,24 @@ class ShouldValidateModelSerializer(serializers.ModelSerializer): fields = ('renamed',) +class TestNestedValidationError(TestCase): + def test_nested_validation_error_detail(self): + """ + Ensure nested validation error detail is rendered correctly. + """ + e = serializers.ValidationError({ + 'nested': { + 'field': ['error'], + } + }) + + self.assertEqual(serializers.get_validation_error_detail(e), { + 'nested': { + 'field': ['error'], + } + }) + + class TestPreSaveValidationExclusionsSerializer(TestCase): def test_renamed_fields_are_model_validated(self): """ From b6df77f2801c6e597ffc9dcb1d663a70e9cb4933 Mon Sep 17 00:00:00 2001 From: Craig de Stigter Date: Tue, 19 Jan 2016 09:16:46 +1300 Subject: [PATCH 4/4] Remove the two most irritating style warnings that I keep banging my head against. In dynamic usage with moderately complex serializer hierarchy, these are a real PITA. * Why should I specify a queryset= kwarg on my RelatedField subclass when I have a get_queryset() method? * When source=dynamic_thing(), don't make me check if it's equal to the field_name. Who cares? This also prevents calling bind() twice. --- rest_framework/fields.py | 10 ---------- rest_framework/relations.py | 4 ---- 2 files changed, 14 deletions(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 8541bc43a..4911b365a 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -322,16 +322,6 @@ class Field(object): Called when a field is added to the parent serializer instance. """ - # In order to enforce a consistent style, we error if a redundant - # 'source' argument has been used. For example: - # my_field = serializer.CharField(source='my_field') - assert self.source != field_name, ( - "It is redundant to specify `source='%s'` on field '%s' in " - "serializer '%s', because it is the same as the field name. " - "Remove the `source` keyword argument." % - (field_name, self.__class__.__name__, parent.__class__.__name__) - ) - self.field_name = field_name self.parent = parent diff --git a/rest_framework/relations.py b/rest_framework/relations.py index 0bcdcdd95..083b56bf8 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -65,10 +65,6 @@ class RelatedField(Field): self.queryset = kwargs.pop('queryset', self.queryset) self.html_cutoff = kwargs.pop('html_cutoff', self.html_cutoff) self.html_cutoff_text = kwargs.pop('html_cutoff_text', self.html_cutoff_text) - assert self.queryset is not None or kwargs.get('read_only', None), ( - 'Relational field must provide a `queryset` argument, ' - 'or set read_only=`True`.' - ) kwargs.pop('many', None) kwargs.pop('allow_empty', None) super(RelatedField, self).__init__(**kwargs)