From b150e5ddf676afcc2fde314c6bc03efa9d1a0ed6 Mon Sep 17 00:00:00 2001 From: Frank Pape Date: Fri, 28 Feb 2014 15:45:53 -0500 Subject: [PATCH 1/3] Test to demonstrate inconsistent full update behavior with omitted fields. --- rest_framework/tests/models.py | 2 + rest_framework/tests/test_serializer.py | 64 ++++++++++++++++++++++--- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/rest_framework/tests/models.py b/rest_framework/tests/models.py index 32a726c0b1..7bb68ce667 100644 --- a/rest_framework/tests/models.py +++ b/rest_framework/tests/models.py @@ -43,8 +43,10 @@ class SlugBasedModel(RESTFrameworkModel): class DefaultValueModel(RESTFrameworkModel): + required_field = models.CharField(max_length=100) text = models.CharField(default='foobar', max_length=100) extra = models.CharField(blank=True, null=True, max_length=100) + extra_not_nullable = models.CharField(blank=True, max_length=100) class CallableDefaultValueModel(RESTFrameworkModel): diff --git a/rest_framework/tests/test_serializer.py b/rest_framework/tests/test_serializer.py index 6b1e333e49..748aff0b1e 100644 --- a/rest_framework/tests/test_serializer.py +++ b/rest_framework/tests/test_serializer.py @@ -847,37 +847,89 @@ class Meta: self.objects = DefaultValueModel.objects def test_create_using_default(self): - data = {} + data = {'required_field': 'required_field_value'} serializer = self.serializer_class(data=data) self.assertEqual(serializer.is_valid(), True) instance = serializer.save() self.assertEqual(len(self.objects.all()), 1) self.assertEqual(instance.pk, 1) self.assertEqual(instance.text, 'foobar') + self.assertEqual(instance.required_field, 'required_field_value') + self.assertEqual(instance.extra, None) + self.assertEqual(instance.extra_not_nullable, '') def test_create_overriding_default(self): - data = {'text': 'overridden'} + data = {'required_field': 'required_field_value', 'text': 'overridden'} serializer = self.serializer_class(data=data) self.assertEqual(serializer.is_valid(), True) instance = serializer.save() self.assertEqual(len(self.objects.all()), 1) self.assertEqual(instance.pk, 1) self.assertEqual(instance.text, 'overridden') + self.assertEqual(instance.required_field, 'required_field_value') + self.assertEqual(instance.extra, None) + self.assertEqual(instance.extra_not_nullable, '') def test_partial_update_default(self): - """ Regression test for issue #532 """ - data = {'text': 'overridden'} + """ + Regression test for issue #532. Ensure a partial update does not modify omitted + fields to their default values. + """ + data = { + 'required_field': 'required_field_value', + 'text': 'overridden', + 'extra': 'extra_value', + 'extra_not_nullable': 'extra_not_nullable_value', + } serializer = self.serializer_class(data=data, partial=True) self.assertEqual(serializer.is_valid(), True) instance = serializer.save() + self.assertEqual(instance.required_field, 'required_field_value') + self.assertEqual(instance.text, 'overridden') + self.assertEqual(instance.extra, 'extra_value') + self.assertEqual(instance.extra_not_nullable, 'extra_not_nullable_value') - data = {'extra': 'extra_value'} + data = {'extra': 'extra_updated'} serializer = self.serializer_class(instance=instance, data=data, partial=True) self.assertEqual(serializer.is_valid(), True) instance = serializer.save() + self.assertEqual(instance.extra, 'extra_updated') + self.assertEqual(instance.text, 'overridden') + self.assertEqual(instance.required_field, 'required_field_value') + self.assertEqual(instance.extra_not_nullable, 'extra_not_nullable_value') - self.assertEqual(instance.extra, 'extra_value') + def test_non_partial_update_default(self): + """ + Omitted values in a full update should be reset. + """ + data = { + 'required_field': 'required_field_value', + 'text': 'overridden', + 'extra': 'extra_value', + 'extra_not_nullable': 'extra_not_nullable_value', + } + serializer = self.serializer_class(data=data) + self.assertTrue(serializer.is_valid()) + instance = serializer.save() + self.assertEqual(instance.required_field, 'required_field_value') self.assertEqual(instance.text, 'overridden') + self.assertEqual(instance.extra, 'extra_value') + self.assertEqual(instance.extra_not_nullable, 'extra_not_nullable_value') + + # Omitting a required field is not valid. + data = {'text': 'text_updated'} + serializer = self.serializer_class(instance=instance, data=data) + self.assertFalse(serializer.is_valid()) + + # The field with a default value should be reset to default when omitted, and those + # without a default value should be set to None or a blank value when omitted. + data = {'required_field': 'required_field_updated'} + serializer = self.serializer_class(instance=instance, data=data) + self.assertTrue(serializer.is_valid()) + instance = serializer.save() + self.assertEqual(instance.text, 'foobar') + self.assertEqual(instance.extra, None) + self.assertEqual(instance.extra_not_nullable, '') class CallableDefaultValueTests(TestCase): From 93573305cef13d6129874a1a6d2a0ae15cff52f3 Mon Sep 17 00:00:00 2001 From: Frank Pape Date: Mon, 3 Mar 2014 10:10:49 -0500 Subject: [PATCH 2/3] Raise ValidationError on non-partial update if a field without a default value is not specified. --- rest_framework/fields.py | 3 ++- rest_framework/tests/test_serializer.py | 31 +++++++++++++++++++++---- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 05daaab76a..379e5a9b06 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -354,7 +354,8 @@ def field_from_native(self, data, files, field_name, into): else: native = self.default else: - if self.required: + incomplete_update = self.root.object is not None and not self.partial + if self.required or incomplete_update: raise ValidationError(self.error_messages['required']) return diff --git a/rest_framework/tests/test_serializer.py b/rest_framework/tests/test_serializer.py index 748aff0b1e..28a54c6cc7 100644 --- a/rest_framework/tests/test_serializer.py +++ b/rest_framework/tests/test_serializer.py @@ -921,15 +921,36 @@ def test_non_partial_update_default(self): serializer = self.serializer_class(instance=instance, data=data) self.assertFalse(serializer.is_valid()) - # The field with a default value should be reset to default when omitted, and those - # without a default value should be set to None or a blank value when omitted. - data = {'required_field': 'required_field_updated'} + # Omitting an optional field with no default value is not valid + data = { + 'text': 'text_updated', + 'required_field': 'required_field_updated', + 'extra': 'extra_updated', + } + serializer = self.serializer_class(instance=instance, data=data) + self.assertFalse(serializer.is_valid()) + + data = { + 'text': 'text_updated', + 'required_field': 'required_field_updated', + 'extra_not_nullable': 'extra_not_nullable_updated', + } + serializer = self.serializer_class(instance=instance, data=data) + self.assertFalse(serializer.is_valid()) + + # A field with a default value should be reset to default when omitted + data = { + 'required_field': 'required_field_updated', + 'extra': 'extra_updated', + 'extra_not_nullable': 'extra_not_nullable_updated', + } serializer = self.serializer_class(instance=instance, data=data) self.assertTrue(serializer.is_valid()) instance = serializer.save() self.assertEqual(instance.text, 'foobar') - self.assertEqual(instance.extra, None) - self.assertEqual(instance.extra_not_nullable, '') + self.assertEqual(instance.required_field, 'required_field_updated') + self.assertEqual(instance.extra, 'extra_updated') + self.assertEqual(instance.extra_not_nullable, 'extra_not_nullable_updated') class CallableDefaultValueTests(TestCase): From 3c0ea81fcfd23ae1e2a93184588a3479945501e0 Mon Sep 17 00:00:00 2001 From: Frank Pape Date: Mon, 3 Mar 2014 10:23:55 -0500 Subject: [PATCH 3/3] Update deserialization documentation for updates. --- docs/api-guide/serializers.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/api-guide/serializers.md b/docs/api-guide/serializers.md index 7ee060af46..c4d6501a24 100644 --- a/docs/api-guide/serializers.md +++ b/docs/api-guide/serializers.md @@ -105,7 +105,9 @@ When deserializing data, we can either create a new instance, or update an exist serializer = CommentSerializer(data=data) # Create new instance serializer = CommentSerializer(comment, data=data) # Update `comment` -By default, serializers must be passed values for all required fields or they will throw validation errors. You can use the `partial` argument in order to allow partial updates. +When creating a new instance, serializers must be passed values for all required fields or they will throw validation errors. + +When updating an existing instance, serializers must be passed values for every field that does not have a default specified, or they will throw validation errors. Omitted fields will be reset to their default values. You can use the `partial` argument in order to allow partial updates, in which case omitted fields will not be changed. serializer = CommentSerializer(comment, data={'content': u'foo bar'}, partial=True) # Update `comment` with partial data