From 24c9c455feaa47487196a2c9343746d7d5bdd962 Mon Sep 17 00:00:00 2001 From: Brian Zambrano Date: Mon, 13 May 2013 10:51:51 -0700 Subject: [PATCH 1/5] Allow for missing non-required nested objects. Serializer fields which are themselves serializers should not be required. Specifically, if a nested object is set to "required=False", it should be possible to serialize the main object and have the sub-object set to None/null. --- rest_framework/fields.py | 2 +- rest_framework/tests/serializer.py | 47 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index c83ee5ecf..1f38b7959 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -50,7 +50,7 @@ def get_component(obj, attr_name): return that attribute on the object. """ if isinstance(obj, dict): - val = obj[attr_name] + val = obj.get(attr_name) else: val = getattr(obj, attr_name) diff --git a/rest_framework/tests/serializer.py b/rest_framework/tests/serializer.py index 84e1ee4e0..6e7323275 100644 --- a/rest_framework/tests/serializer.py +++ b/rest_framework/tests/serializer.py @@ -43,6 +43,17 @@ class CommentSerializer(serializers.Serializer): return instance +class NamesSerializer(serializers.Serializer): + first = serializers.CharField() + last = serializers.CharField(required=False, default='') + initials = serializers.CharField(required=False, default='') + + +class PersonIdentifierSerializer(serializers.Serializer): + ssn = serializers.CharField() + names = NamesSerializer(source='names', required=False) + + class BookSerializer(serializers.ModelSerializer): isbn = serializers.RegexField(regex=r'^[0-9]{13}$', error_messages={'invalid': 'isbn has to be exact 13 numbers'}) @@ -141,6 +152,42 @@ class BasicTests(TestCase): self.assertFalse(serializer.object is expected) self.assertEqual(serializer.data['sub_comment'], 'And Merry Christmas!') + def test_create_nested(self): + """Test a serializer with nested data.""" + names = {'first': 'John', 'last': 'Doe', 'initials': 'jd'} + data = {'ssn': '1234567890', 'names': names} + serializer = PersonIdentifierSerializer(data=data) + + self.assertEqual(serializer.is_valid(), True) + self.assertEqual(serializer.object, data) + self.assertFalse(serializer.object is data) + self.assertEqual(serializer.data['names'], names) + + def test_create_partial_nested(self): + """Test a serializer with nested data which has missing fields.""" + names = {'first': 'John'} + data = {'ssn': '1234567890', 'names': names} + serializer = PersonIdentifierSerializer(data=data) + + expected_names = {'first': 'John', 'last': '', 'initials': ''} + data['names'] = expected_names + + self.assertEqual(serializer.is_valid(), True) + self.assertEqual(serializer.object, data) + self.assertFalse(serializer.object is expected_names) + self.assertEqual(serializer.data['names'], expected_names) + + def test_null_nested(self): + """Test a serializer with a nonexistent nested field""" + data = {'ssn': '1234567890'} + serializer = PersonIdentifierSerializer(data=data) + + self.assertEqual(serializer.is_valid(), True) + self.assertEqual(serializer.object, data) + self.assertFalse(serializer.object is data) + expected = {'ssn': '1234567890', 'names': None} + self.assertEqual(serializer.data, expected) + def test_update(self): serializer = CommentSerializer(self.comment, data=self.data) expected = self.comment From de5cc8de423a22009d2a643f6c268805f715b212 Mon Sep 17 00:00:00 2001 From: Pablo Recio Date: Sat, 18 May 2013 12:40:25 +0200 Subject: [PATCH 2/5] A model's field is required if is null or blank --- rest_framework/serializers.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 7707de7a7..500bb3066 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -705,15 +705,14 @@ class ModelSerializer(Serializer): Creates a default instance of a basic non-relational field. """ kwargs = {} - has_default = model_field.has_default() - if model_field.null or model_field.blank or has_default: + if model_field.null or model_field.blank: kwargs['required'] = False if isinstance(model_field, models.AutoField) or not model_field.editable: kwargs['read_only'] = True - if has_default: + if model_field.has_default(): kwargs['default'] = model_field.get_default() if issubclass(model_field.__class__, models.TextField): From ab8bd566f9db327a4c463317011818d421bbf89c Mon Sep 17 00:00:00 2001 From: Pablo Recio Date: Sat, 18 May 2013 12:40:25 +0200 Subject: [PATCH 3/5] Adding `BLANK_CHOICE_DASH` as a choice if the model's field isn't required --- rest_framework/fields.py | 3 ++ rest_framework/tests/fields.py | 28 ++++++++++++++++++- rest_framework/tests/models.py | 26 +++++++++++++++++ rest_framework/tests/serializer.py | 45 +++++++++++++++++++++++++++++- 4 files changed, 100 insertions(+), 2 deletions(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index c83ee5ecf..7fd4c6381 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -15,6 +15,7 @@ import warnings from django.core import validators from django.core.exceptions import ValidationError from django.conf import settings +from django.db.models.fields import BLANK_CHOICE_DASH from django import forms from django.forms import widgets from django.utils.encoding import is_protected_type @@ -402,6 +403,8 @@ class ChoiceField(WritableField): def __init__(self, choices=(), *args, **kwargs): super(ChoiceField, self).__init__(*args, **kwargs) self.choices = choices + if not self.required: + self.choices = BLANK_CHOICE_DASH + self.choices def _get_choices(self): return self._choices diff --git a/rest_framework/tests/fields.py b/rest_framework/tests/fields.py index 3cdfa0f62..f313ba60a 100644 --- a/rest_framework/tests/fields.py +++ b/rest_framework/tests/fields.py @@ -645,4 +645,30 @@ class DecimalFieldTest(TestCase): s = DecimalSerializer(data={'decimal_field': '12345.6'}) self.assertFalse(s.is_valid()) - self.assertEqual(s.errors, {'decimal_field': ['Ensure that there are no more than 4 digits in total.']}) \ No newline at end of file + self.assertEqual(s.errors, {'decimal_field': ['Ensure that there are no more than 4 digits in total.']}) + + +class ChoiceFieldTests(TestCase): + """ + Tests for the ChoiceField options generator + """ + + SAMPLE_CHOICES = [ + ('red', 'Red'), + ('green', 'Green'), + ('blue', 'Blue'), + ] + + def test_choices_required(self): + """ + Make sure proper choices are rendered if field is required + """ + f = serializers.ChoiceField(required=True, choices=self.SAMPLE_CHOICES) + self.assertEqual(f.choices, self.SAMPLE_CHOICES) + + def test_choices_not_required(self): + """ + Make sure proper choices (plus blank) are rendered if the field isn't required + """ + f = serializers.ChoiceField(required=False, choices=self.SAMPLE_CHOICES) + self.assertEqual(f.choices, models.fields.BLANK_CHOICE_DASH + self.SAMPLE_CHOICES) diff --git a/rest_framework/tests/models.py b/rest_framework/tests/models.py index 40e41a644..5d98b04bd 100644 --- a/rest_framework/tests/models.py +++ b/rest_framework/tests/models.py @@ -117,6 +117,32 @@ class OptionalRelationModel(RESTFrameworkModel): other = models.ForeignKey('OptionalRelationModel', blank=True, null=True) +# Model for issue #725 +class SeveralChoicesModel(RESTFrameworkModel): + color = models.CharField( + max_length=10, + choices=[('red', 'Red'), ('green', 'Green'), ('blue', 'Blue')], + blank=False + ) + drink = models.CharField( + max_length=10, + choices=[('beer', 'Beer'), ('wine', 'Wine'), ('cider', 'Cider')], + blank=False, + default='beer' + ) + os = models.CharField( + max_length=10, + choices=[('linux', 'Linux'), ('osx', 'OSX'), ('windows', 'Windows')], + blank=True + ) + music_genre = models.CharField( + max_length=10, + choices=[('rock', 'Rock'), ('metal', 'Metal'), ('grunge', 'Grunge')], + blank=True, + default='metal' + ) + + # Model for RegexField class Book(RESTFrameworkModel): isbn = models.CharField(max_length=13) diff --git a/rest_framework/tests/serializer.py b/rest_framework/tests/serializer.py index db3881f9a..3f39308de 100644 --- a/rest_framework/tests/serializer.py +++ b/rest_framework/tests/serializer.py @@ -1,10 +1,11 @@ from __future__ import unicode_literals +from django.db.models.fields import BLANK_CHOICE_DASH from django.utils.datastructures import MultiValueDict from django.test import TestCase from rest_framework import serializers from rest_framework.tests.models import (HasPositiveIntegerAsChoice, Album, ActionItem, Anchor, BasicModel, BlankFieldModel, BlogPost, BlogPostComment, Book, CallableDefaultValueModel, DefaultValueModel, - ManyToManyModel, Person, ReadOnlyManyToManyModel, Photo) + ManyToManyModel, Person, ReadOnlyManyToManyModel, Photo, SeveralChoicesModel) import datetime import pickle @@ -1018,6 +1019,48 @@ class SerializerPickleTests(TestCase): repr(pickle.loads(pickle.dumps(data, 0))) +# test for issue #725 +class SerializerChoiceFields(TestCase): + + def setUp(self): + super(SerializerChoiceFields, self).setUp() + + class SeveralChoicesSerializer(serializers.ModelSerializer): + class Meta: + model = SeveralChoicesModel + fields = ('color', 'drink', 'os', 'music_genre') + + self.several_choices_serializer = SeveralChoicesSerializer + + def test_choices_blank_false_not_default(self): + serializer = self.several_choices_serializer() + self.assertEqual( + serializer.fields['color'].choices, + [('red', 'Red'), ('green', 'Green'), ('blue', 'Blue')] + ) + + def test_choices_blank_false_with_default(self): + serializer = self.several_choices_serializer() + self.assertEqual( + serializer.fields['drink'].choices, + [('beer', 'Beer'), ('wine', 'Wine'), ('cider', 'Cider')] + ) + + def test_choices_blank_true_not_default(self): + serializer = self.several_choices_serializer() + self.assertEqual( + serializer.fields['os'].choices, + BLANK_CHOICE_DASH + [('linux', 'Linux'), ('osx', 'OSX'), ('windows', 'Windows')] + ) + + def test_choices_blank_true_with_default(self): + serializer = self.several_choices_serializer() + self.assertEqual( + serializer.fields['music_genre'].choices, + BLANK_CHOICE_DASH + [('rock', 'Rock'), ('metal', 'Metal'), ('grunge', 'Grunge')] + ) + + class DepthTest(TestCase): def test_implicit_nesting(self): From 8fe43236a22e56d1741b49b92f0c53e01cd9e5f6 Mon Sep 17 00:00:00 2001 From: Pablo Recio Date: Sat, 18 May 2013 13:23:38 +0200 Subject: [PATCH 4/5] Moved test model into closer to the testcase --- rest_framework/tests/models.py | 26 -------------------------- rest_framework/tests/serializer.py | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/rest_framework/tests/models.py b/rest_framework/tests/models.py index 5d98b04bd..40e41a644 100644 --- a/rest_framework/tests/models.py +++ b/rest_framework/tests/models.py @@ -117,32 +117,6 @@ class OptionalRelationModel(RESTFrameworkModel): other = models.ForeignKey('OptionalRelationModel', blank=True, null=True) -# Model for issue #725 -class SeveralChoicesModel(RESTFrameworkModel): - color = models.CharField( - max_length=10, - choices=[('red', 'Red'), ('green', 'Green'), ('blue', 'Blue')], - blank=False - ) - drink = models.CharField( - max_length=10, - choices=[('beer', 'Beer'), ('wine', 'Wine'), ('cider', 'Cider')], - blank=False, - default='beer' - ) - os = models.CharField( - max_length=10, - choices=[('linux', 'Linux'), ('osx', 'OSX'), ('windows', 'Windows')], - blank=True - ) - music_genre = models.CharField( - max_length=10, - choices=[('rock', 'Rock'), ('metal', 'Metal'), ('grunge', 'Grunge')], - blank=True, - default='metal' - ) - - # Model for RegexField class Book(RESTFrameworkModel): isbn = models.CharField(max_length=13) diff --git a/rest_framework/tests/serializer.py b/rest_framework/tests/serializer.py index 85b952835..c043f4175 100644 --- a/rest_framework/tests/serializer.py +++ b/rest_framework/tests/serializer.py @@ -1,11 +1,12 @@ from __future__ import unicode_literals +from django.db import models from django.db.models.fields import BLANK_CHOICE_DASH from django.utils.datastructures import MultiValueDict from django.test import TestCase from rest_framework import serializers from rest_framework.tests.models import (HasPositiveIntegerAsChoice, Album, ActionItem, Anchor, BasicModel, BlankFieldModel, BlogPost, BlogPostComment, Book, CallableDefaultValueModel, DefaultValueModel, - ManyToManyModel, Person, ReadOnlyManyToManyModel, Photo, SeveralChoicesModel) + ManyToManyModel, Person, ReadOnlyManyToManyModel, Photo) import datetime import pickle @@ -1003,6 +1004,31 @@ class SerializerPickleTests(TestCase): # test for issue #725 +class SeveralChoicesModel(models.Model): + color = models.CharField( + max_length=10, + choices=[('red', 'Red'), ('green', 'Green'), ('blue', 'Blue')], + blank=False + ) + drink = models.CharField( + max_length=10, + choices=[('beer', 'Beer'), ('wine', 'Wine'), ('cider', 'Cider')], + blank=False, + default='beer' + ) + os = models.CharField( + max_length=10, + choices=[('linux', 'Linux'), ('osx', 'OSX'), ('windows', 'Windows')], + blank=True + ) + music_genre = models.CharField( + max_length=10, + choices=[('rock', 'Rock'), ('metal', 'Metal'), ('grunge', 'Grunge')], + blank=True, + default='metal' + ) + + class SerializerChoiceFields(TestCase): def setUp(self): From a0e3c44c99a61a6dc878308bdf0890fbb10c41e4 Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Sat, 18 May 2013 13:40:20 +0200 Subject: [PATCH 5/5] Added @craigds, @pyriku, @brianz - Yay for sprints! --- docs/topics/credits.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/topics/credits.md b/docs/topics/credits.md index 8151b4d3a..5998b4ca4 100644 --- a/docs/topics/credits.md +++ b/docs/topics/credits.md @@ -124,6 +124,9 @@ The following people have helped make REST framework great. * Marlon Bailey - [avinash240] * James Summerfield - [jsummerfield] * Andy Freeland - [rouge8] +* Craig de Stigter - [craigds] +* Pablo Recio - [pyriku] +* Brian Zambrano - [brianz] Many thanks to everyone who's contributed to the project. @@ -284,3 +287,6 @@ You can also contact [@_tomchristie][twitter] directly on twitter. [avinash240]: https://github.com/avinash240 [jsummerfield]: https://github.com/jsummerfield [rouge8]: https://github.com/rouge8 +[craigds]: https://github.com/craigds +[pyriku]: https://github.com/pyriku +[brianz]: https://github.com/brianz