From 117ac24ff1a1de1b98c7db8ce912e7300bff3830 Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Mon, 3 Feb 2014 15:21:46 +0100 Subject: [PATCH 1/7] Added a test case for dotted sources for regular fields. --- rest_framework/tests/test_serializer.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/rest_framework/tests/test_serializer.py b/rest_framework/tests/test_serializer.py index 75d6e7859..ac74baa4c 100644 --- a/rest_framework/tests/test_serializer.py +++ b/rest_framework/tests/test_serializer.py @@ -1840,3 +1840,20 @@ class BoolenFieldTypeTest(TestCase): ''' bfield = self.serializer.get_fields()['started'] self.assertEqual(type(bfield), fields.BooleanField) + + +class RelationSpanningSerializerTest(TestCase): + def test_regular_field_can_span_a_relation(self): + class TicketSerializer(serializers.ModelSerializer): + name = fields.CharField(source='assigned.name') + + class Meta: + model = Ticket + fields = ('name',) + + owner = Person(name='john') + reviewer = Person(name='reviewer') + ticket = Ticket(assigned=owner, reviewer=reviewer) + serializer = TicketSerializer(ticket, data={'name': 'doe'}) + self.assertTrue(serializer.is_valid()) + self.assertEqual(serializer.object.assigned.name, 'doe') From d5dd3772e905f5e13c3b2666d84dd2065ecb639a Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Mon, 3 Feb 2014 15:52:25 +0100 Subject: [PATCH 2/7] Naive implementation for the dotted sources for model serializer. --- rest_framework/serializers.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 536b040bc..91436701e 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -948,14 +948,20 @@ class ModelSerializer(Serializer): # Update an existing instance... if instance is not None: for key, val in attrs.items(): + # TODO: check whether we need to do something about dotted paths + dest = instance + keys = key.split('.') + for related_instance in keys[:-1]: + dest = getattr(dest, related_instance) try: - setattr(instance, key, val) + setattr(dest, keys[-1], val) except ValueError: self._errors[key] = self.error_messages['required'] # ...or create a new instance else: instance = self.opts.model(**attrs) + # TODO: check whether we need to do something about dotted paths # Any relations that cannot be set until we've # saved the model get hidden away on these From 214ce0777d3832e1f07ff45374ce4f4b68379b3a Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Mon, 3 Feb 2014 21:16:51 +0100 Subject: [PATCH 3/7] Raise error for dotted sources on creation. --- rest_framework/fields.py | 1 + rest_framework/serializers.py | 29 ++++++++++++++++--- rest_framework/tests/test_serializer.py | 38 +++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/rest_framework/fields.py b/rest_framework/fields.py index 2f475d6ea..7c64af5c1 100644 --- a/rest_framework/fields.py +++ b/rest_framework/fields.py @@ -251,6 +251,7 @@ class WritableField(Field): default_error_messages = { 'required': _('This field is required.'), 'invalid': _('Invalid value.'), + 'missing': _('Related object does not exist.'), } widget = widgets.TextInput default = None diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 91436701e..7838efbf8 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -20,6 +20,7 @@ from django.core.paginator import Page from django.db import models from django.forms import widgets from django.utils.datastructures import SortedDict +from django.utils.translation import ugettext_lazy as _ from rest_framework.compat import get_concrete_model, six from rest_framework.settings import api_settings @@ -919,6 +920,7 @@ class ModelSerializer(Serializer): """ m2m_data = {} related_data = {} + related_models_to_save = {} nested_forward_relations = {} meta = self.opts.model._meta @@ -948,11 +950,22 @@ class ModelSerializer(Serializer): # Update an existing instance... if instance is not None: for key, val in attrs.items(): - # TODO: check whether we need to do something about dotted paths + # Follow relations if needed dest = instance keys = key.split('.') - for related_instance in keys[:-1]: - dest = getattr(dest, related_instance) + try: + for related_instance in keys[:-1]: + dest = getattr(dest, related_instance) + + except AttributeError: + self._errors[key] = self.error_messages['missing'] + continue + + # If there's a relation, mark the object to save + if len(keys) > 1: + related_models_to_save[key] = dest + + # Assign the value try: setattr(dest, keys[-1], val) except ValueError: @@ -960,8 +973,11 @@ class ModelSerializer(Serializer): # ...or create a new instance else: + for key, value in ((k, v) for k, v in attrs.items() if '.' in k): + self._errors[key] = 'You can not set dotted sources during creation.' + del attrs[key] + print value instance = self.opts.model(**attrs) - # TODO: check whether we need to do something about dotted paths # Any relations that cannot be set until we've # saved the model get hidden away on these @@ -970,6 +986,7 @@ class ModelSerializer(Serializer): instance._related_data = related_data instance._m2m_data = m2m_data instance._nested_forward_relations = nested_forward_relations + instance._related_models_to_save = related_models_to_save return instance @@ -1028,6 +1045,10 @@ class ModelSerializer(Serializer): setattr(obj, accessor_name, related) del(obj._related_data) + if getattr(obj, '_related_models_to_save', None): + for related in obj._related_models_to_save.values(): + self.save_object(related) + class HyperlinkedModelSerializerOptions(ModelSerializerOptions): """ diff --git a/rest_framework/tests/test_serializer.py b/rest_framework/tests/test_serializer.py index ac74baa4c..cb0a6aa7b 100644 --- a/rest_framework/tests/test_serializer.py +++ b/rest_framework/tests/test_serializer.py @@ -1843,7 +1843,8 @@ class BoolenFieldTypeTest(TestCase): class RelationSpanningSerializerTest(TestCase): - def test_regular_field_can_span_a_relation(self): + def test_model_traversal_creation(self): + """Update a field through a foreign key during a creation.""" class TicketSerializer(serializers.ModelSerializer): name = fields.CharField(source='assigned.name') @@ -1851,9 +1852,40 @@ class RelationSpanningSerializerTest(TestCase): model = Ticket fields = ('name',) + serializer = TicketSerializer(data={'name': 'doe'}) + self.assertFalse(serializer.is_valid()) + self.assertEqual(serializer.errors, {'name': 'You can not set dotted sources during creation.'}) + + def test_model_traversal_update(self): + """Update a field through a foreign key during an update.""" + class TicketSerializer(serializers.ModelSerializer): + name = fields.CharField(source='assigned.name') + + class Meta: + model = Ticket + fields = ('name',) + + owner = Person.objects.create(name='john') + reviewer = Person.objects.create(name='reviewer') + ticket = Ticket.objects.create(assigned=owner, reviewer=reviewer) + serializer = TicketSerializer(ticket, data={'name': 'doe'}) + self.assertTrue(serializer.is_valid()) + self.assertEqual(serializer.object.assigned.name, 'doe') + serializer.save() + self.assertEqual(Person.objects.get(id=owner.id).name, 'doe') + + def test_failing_model_traversal(self): + """Update a field through an unknown relation.""" + class TicketSerializer(serializers.ModelSerializer): + name = fields.CharField(source='demo.name') + + class Meta: + model = Ticket + fields = ('name',) + owner = Person(name='john') reviewer = Person(name='reviewer') ticket = Ticket(assigned=owner, reviewer=reviewer) serializer = TicketSerializer(ticket, data={'name': 'doe'}) - self.assertTrue(serializer.is_valid()) - self.assertEqual(serializer.object.assigned.name, 'doe') + self.assertFalse(serializer.is_valid()) + self.assertEqual(serializer.errors, {'name': 'Related object does not exist.'}) From be7183f196fd0313fba49a709eef7264fa9723df Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Thu, 13 Feb 2014 09:45:22 +0100 Subject: [PATCH 4/7] Altered the code to avoid spanning more than one relation. --- rest_framework/serializers.py | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 7838efbf8..5ee2536d2 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -950,24 +950,28 @@ class ModelSerializer(Serializer): # Update an existing instance... if instance is not None: for key, val in attrs.items(): - # Follow relations if needed - dest = instance keys = key.split('.') - try: - for related_instance in keys[:-1]: - dest = getattr(dest, related_instance) - except AttributeError: - self._errors[key] = self.error_messages['missing'] + # Work on the current instance for this attribute + attr_instance = instance + + # Raise an error if we span more than one relation + if len(keys) > 2: + self._errors[key] = 'Can not span more than a relation' continue - # If there's a relation, mark the object to save - if len(keys) > 1: - related_models_to_save[key] = dest + # Mark the related instance as the one to save + if len(keys) == 2: + try: + attr_instance = getattr(instance, keys[0]) + related_models_to_save[key] = attr_instance + except AttributeError: + self._errors[key] = self.error_messages['missing'] + continue # Assign the value try: - setattr(dest, keys[-1], val) + setattr(attr_instance, keys[-1], val) except ValueError: self._errors[key] = self.error_messages['required'] @@ -976,7 +980,6 @@ class ModelSerializer(Serializer): for key, value in ((k, v) for k, v in attrs.items() if '.' in k): self._errors[key] = 'You can not set dotted sources during creation.' del attrs[key] - print value instance = self.opts.model(**attrs) # Any relations that cannot be set until we've From de0a072d5a19bdc1eb8e34860f210ffff8d737f5 Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Thu, 13 Feb 2014 09:46:00 +0100 Subject: [PATCH 5/7] Changed name to username to avoid confusion with the related name field. --- rest_framework/tests/test_serializer.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/rest_framework/tests/test_serializer.py b/rest_framework/tests/test_serializer.py index cb0a6aa7b..7a95c6510 100644 --- a/rest_framework/tests/test_serializer.py +++ b/rest_framework/tests/test_serializer.py @@ -1846,29 +1846,29 @@ class RelationSpanningSerializerTest(TestCase): def test_model_traversal_creation(self): """Update a field through a foreign key during a creation.""" class TicketSerializer(serializers.ModelSerializer): - name = fields.CharField(source='assigned.name') + username = fields.CharField(source='assigned.name') class Meta: model = Ticket - fields = ('name',) + fields = ('username',) - serializer = TicketSerializer(data={'name': 'doe'}) + serializer = TicketSerializer(data={'username': 'doe'}) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'name': 'You can not set dotted sources during creation.'}) + self.assertEqual(serializer.errors, {'username': 'You can not set dotted sources during creation.'}) def test_model_traversal_update(self): """Update a field through a foreign key during an update.""" class TicketSerializer(serializers.ModelSerializer): - name = fields.CharField(source='assigned.name') + username = fields.CharField(source='assigned.name') class Meta: model = Ticket - fields = ('name',) + fields = ('username',) owner = Person.objects.create(name='john') reviewer = Person.objects.create(name='reviewer') ticket = Ticket.objects.create(assigned=owner, reviewer=reviewer) - serializer = TicketSerializer(ticket, data={'name': 'doe'}) + serializer = TicketSerializer(ticket, data={'username': 'doe'}) self.assertTrue(serializer.is_valid()) self.assertEqual(serializer.object.assigned.name, 'doe') serializer.save() @@ -1877,15 +1877,15 @@ class RelationSpanningSerializerTest(TestCase): def test_failing_model_traversal(self): """Update a field through an unknown relation.""" class TicketSerializer(serializers.ModelSerializer): - name = fields.CharField(source='demo.name') + username = fields.CharField(source='demo.name') class Meta: model = Ticket - fields = ('name',) + fields = ('username',) owner = Person(name='john') reviewer = Person(name='reviewer') ticket = Ticket(assigned=owner, reviewer=reviewer) - serializer = TicketSerializer(ticket, data={'name': 'doe'}) + serializer = TicketSerializer(ticket, data={'username': 'doe'}) self.assertFalse(serializer.is_valid()) - self.assertEqual(serializer.errors, {'name': 'Related object does not exist.'}) + self.assertEqual(serializer.errors, {'username': 'Related object does not exist.'}) From d2ded0ddad3c096ee68f5f6d515c1d7056833a34 Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Thu, 13 Feb 2014 09:52:38 +0100 Subject: [PATCH 6/7] Added the test case to report if we span more than one relation. --- rest_framework/serializers.py | 2 +- rest_framework/tests/test_serializer.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/rest_framework/serializers.py b/rest_framework/serializers.py index 5ee2536d2..9a58d966f 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -957,7 +957,7 @@ class ModelSerializer(Serializer): # Raise an error if we span more than one relation if len(keys) > 2: - self._errors[key] = 'Can not span more than a relation' + self._errors[key] = 'Can not span more than a relation.' continue # Mark the related instance as the one to save diff --git a/rest_framework/tests/test_serializer.py b/rest_framework/tests/test_serializer.py index 7a95c6510..7edf7ea96 100644 --- a/rest_framework/tests/test_serializer.py +++ b/rest_framework/tests/test_serializer.py @@ -1889,3 +1889,19 @@ class RelationSpanningSerializerTest(TestCase): serializer = TicketSerializer(ticket, data={'username': 'doe'}) self.assertFalse(serializer.is_valid()) self.assertEqual(serializer.errors, {'username': 'Related object does not exist.'}) + + def test_multiple_model_traversal_update(self): + """Update a field through a foreign key during an update.""" + class TicketSerializer(serializers.ModelSerializer): + username = fields.CharField(source='assigned.demo.name') + + class Meta: + model = Ticket + fields = ('username',) + + owner = Person.objects.create(name='john') + reviewer = Person.objects.create(name='reviewer') + ticket = Ticket.objects.create(assigned=owner, reviewer=reviewer) + serializer = TicketSerializer(ticket, data={'username': 'doe'}) + self.assertFalse(serializer.is_valid()) + self.assertEqual(serializer.errors, {'username': 'Can not span more than a relation.'}) From 848c3740f2985826774ac8ccd9a3cecd2c5b8a4f Mon Sep 17 00:00:00 2001 From: Xavier Ordoquy Date: Fri, 16 May 2014 19:33:21 +0200 Subject: [PATCH 7/7] Fixed regression caused by merge. --- 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 5b06cd8d7..8aedfbdfb 100644 --- a/rest_framework/serializers.py +++ b/rest_framework/serializers.py @@ -990,7 +990,7 @@ class ModelSerializer(Serializer): try: attr_instance = getattr(instance, keys[0]) related_models_to_save[key] = attr_instance - except AttributeError: + except (AttributeError, ObjectDoesNotExist): self._errors[key] = self.error_messages['missing'] continue