From 727e40c879f893a2c336e396aafdcad60b5d224c Mon Sep 17 00:00:00 2001 From: Richard Eames Date: Thu, 26 Mar 2015 12:31:09 -0600 Subject: [PATCH] Fixed #19820 -- Added more helpful error messages to Python deserializer. --- django/core/serializers/base.py | 9 +- django/core/serializers/python.py | 45 +++++--- tests/serializers/models.py | 25 +++++ tests/serializers/tests.py | 166 +++++++++++++++++++++++++++++- 4 files changed, 228 insertions(+), 17 deletions(-) diff --git a/django/core/serializers/base.py b/django/core/serializers/base.py index 2c18d7b477..0f7535a148 100644 --- a/django/core/serializers/base.py +++ b/django/core/serializers/base.py @@ -17,7 +17,14 @@ class SerializationError(Exception): class DeserializationError(Exception): """Something bad happened during deserialization.""" - pass + + @classmethod + def WithData(cls, original_exc, model, fk, field_value): + """ + Factory method for creating a deserialization error which has a more + explanatory messsage. + """ + return cls("%s: (%s:pk=%s) field_value was '%s'" % (original_exc, model, fk, field_value)) class Serializer(object): diff --git a/django/core/serializers/python.py b/django/core/serializers/python.py index d209f7ebd0..c97ab627a3 100644 --- a/django/core/serializers/python.py +++ b/django/core/serializers/python.py @@ -100,7 +100,10 @@ def Deserializer(object_list, **options): raise data = {} if 'pk' in d: - data[Model._meta.pk.attname] = Model._meta.pk.to_python(d.get("pk", None)) + try: + data[Model._meta.pk.attname] = Model._meta.pk.to_python(d.get('pk')) + except Exception as e: + raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), None) m2m_data = {} field_names = {f.name for f in Model._meta.get_fields()} @@ -128,30 +131,42 @@ def Deserializer(object_list, **options): return force_text(field.remote_field.model._meta.pk.to_python(value), strings_only=True) else: m2m_convert = lambda v: force_text(field.remote_field.model._meta.pk.to_python(v), strings_only=True) - m2m_data[field.name] = [m2m_convert(pk) for pk in field_value] + + try: + m2m_data[field.name] = [] + for pk in field_value: + m2m_data[field.name].append(m2m_convert(pk)) + except Exception as e: + raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), pk) # Handle FK fields elif field.remote_field and isinstance(field.remote_field, models.ManyToOneRel): if field_value is not None: - if hasattr(field.remote_field.model._default_manager, 'get_by_natural_key'): - if hasattr(field_value, '__iter__') and not isinstance(field_value, six.text_type): - obj = field.remote_field.model._default_manager.db_manager(db).get_by_natural_key(*field_value) - value = getattr(obj, field.remote_field.field_name) - # If this is a natural foreign key to an object that - # has a FK/O2O as the foreign key, use the FK value - if field.remote_field.model._meta.pk.remote_field: - value = value.pk + try: + if hasattr(field.remote_field.model._default_manager, 'get_by_natural_key'): + if hasattr(field_value, '__iter__') and not isinstance(field_value, six.text_type): + obj = field.remote_field.model._default_manager.db_manager(db).get_by_natural_key(*field_value) + value = getattr(obj, field.remote_field.field_name) + # If this is a natural foreign key to an object that + # has a FK/O2O as the foreign key, use the FK value + if field.remote_field.model._meta.pk.remote_field: + value = value.pk + else: + value = field.remote_field.model._meta.get_field(field.remote_field.field_name).to_python(field_value) + data[field.attname] = value else: - value = field.remote_field.model._meta.get_field(field.remote_field.field_name).to_python(field_value) - data[field.attname] = value - else: - data[field.attname] = field.remote_field.model._meta.get_field(field.remote_field.field_name).to_python(field_value) + data[field.attname] = field.remote_field.model._meta.get_field(field.remote_field.field_name).to_python(field_value) + except Exception as e: + raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), field_value) else: data[field.attname] = None # Handle all other fields else: - data[field.name] = field.to_python(field_value) + try: + data[field.name] = field.to_python(field_value) + except Exception as e: + raise base.DeserializationError.WithData(e, d['model'], d.get('pk'), field_value) obj = base.build_instance(Model, data, db) yield base.DeserializedObject(obj, m2m_data) diff --git a/tests/serializers/models.py b/tests/serializers/models.py index 08dc860821..5e7a31902e 100644 --- a/tests/serializers/models.py +++ b/tests/serializers/models.py @@ -14,9 +14,33 @@ from django.utils import six from django.utils.encoding import python_2_unicode_compatible +class CategoryMetaDataManager(models.Manager): + + def get_by_natural_key(self, kind, name): + return self.get(kind=kind, name=name) + + +@python_2_unicode_compatible +class CategoryMetaData(models.Model): + kind = models.CharField(max_length=10) + name = models.CharField(max_length=10) + value = models.CharField(max_length=10) + objects = CategoryMetaDataManager() + + class Meta: + unique_together = (('kind', 'name'),) + + def __str__(self): + return '[%s:%s]=%s' % (self.kind, self.name, self.value) + + def natural_key(self): + return (self.kind, self.name) + + @python_2_unicode_compatible class Category(models.Model): name = models.CharField(max_length=20) + meta_data = models.ForeignKey(CategoryMetaData, null=True, default=None) class Meta: ordering = ('name',) @@ -42,6 +66,7 @@ class Article(models.Model): headline = models.CharField(max_length=50) pub_date = models.DateTimeField() categories = models.ManyToManyField(Category) + meta_data = models.ManyToManyField(CategoryMetaData) class Meta: ordering = ('pub_date',) diff --git a/tests/serializers/tests.py b/tests/serializers/tests.py index 1d0a0bead5..db2f07d41d 100644 --- a/tests/serializers/tests.py +++ b/tests/serializers/tests.py @@ -321,6 +321,7 @@ class XmlSerializerTestCase(SerializersTestBase, TestCase): Poker has no place on ESPN 2006-06-16T11:00:00 + """ @@ -373,6 +374,7 @@ class XmlSerializerTransactionTestCase(SerializersTransactionTestBase, Transacti + Agnes @@ -404,7 +406,8 @@ class JsonSerializerTestCase(SerializersTestBase, TestCase): "categories": [ %(first_category_pk)s, %(second_category_pk)s - ] + ], + "meta_data": [] } } ] @@ -447,6 +450,166 @@ class JsonSerializerTestCase(SerializersTestBase, TestCase): if re.search(r'.+,\s*$', line): self.assertEqual(line, line.rstrip()) + def test_helpful_error_message_invalid_pk(self): + """ + If there is an invalid primary key, the error message should contain + the model associated with it. + """ + test_string = """[{ + "pk": "badpk", + "model": "serializers.player", + "fields": { + "name": "Bob", + "rank": 1, + "team": "Team" + } + }]""" + with self.assertRaisesMessage(serializers.base.DeserializationError, "(serializers.player:pk=badpk)"): + list(serializers.deserialize('json', test_string)) + + def test_helpful_error_message_invalid_field(self): + """ + If there is an invalid field value, the error message should contain + the model associated with it. + """ + test_string = """[{ + "pk": "1", + "model": "serializers.player", + "fields": { + "name": "Bob", + "rank": "invalidint", + "team": "Team" + } + }]""" + expected = "(serializers.player:pk=1) field_value was 'invalidint'" + with self.assertRaisesMessage(serializers.base.DeserializationError, expected): + list(serializers.deserialize('json', test_string)) + + def test_helpful_error_message_for_foreign_keys(self): + """ + Invalid foreign keys with a natural key should throw a helpful error + message, such as what the failing key is. + """ + test_string = """[{ + "pk": 1, + "model": "serializers.category", + "fields": { + "name": "Unknown foreign key", + "meta_data": [ + "doesnotexist", + "metadata" + ] + } + }]""" + key = ["doesnotexist", "metadata"] + expected = "(serializers.category:pk=1) field_value was '%r'" % key + with self.assertRaisesMessage(serializers.base.DeserializationError, expected): + list(serializers.deserialize('json', test_string)) + + def test_helpful_error_message_for_many2many_non_natural(self): + """ + Invalid many-to-many keys should throw a helpful error message. + """ + test_string = """[{ + "pk": 1, + "model": "serializers.article", + "fields": { + "author": 1, + "headline": "Unknown many to many", + "pub_date": "2014-09-15T10:35:00", + "categories": [1, "doesnotexist"] + } + }, { + "pk": 1, + "model": "serializers.author", + "fields": { + "name": "Agnes" + } + }, { + "pk": 1, + "model": "serializers.category", + "fields": { + "name": "Reference" + } + }]""" + expected = "(serializers.article:pk=1) field_value was 'doesnotexist'" + with self.assertRaisesMessage(serializers.base.DeserializationError, expected): + list(serializers.deserialize('json', test_string)) + + def test_helpful_error_message_for_many2many_natural1(self): + """ + Invalid many-to-many keys should throw a helpful error message. + This tests the code path where one of a list of natural keys is invalid. + """ + test_string = """[{ + "pk": 1, + "model": "serializers.categorymetadata", + "fields": { + "kind": "author", + "name": "meta1", + "value": "Agnes" + } + }, { + "pk": 1, + "model": "serializers.article", + "fields": { + "author": 1, + "headline": "Unknown many to many", + "pub_date": "2014-09-15T10:35:00", + "meta_data": [ + ["author", "meta1"], + ["doesnotexist", "meta1"], + ["author", "meta1"] + ] + } + }, { + "pk": 1, + "model": "serializers.author", + "fields": { + "name": "Agnes" + } + }]""" + key = ["doesnotexist", "meta1"] + expected = "(serializers.article:pk=1) field_value was '%r'" % key + with self.assertRaisesMessage(serializers.base.DeserializationError, expected): + for obj in serializers.deserialize('json', test_string): + obj.save() + + def test_helpful_error_message_for_many2many_natural2(self): + """ + Invalid many-to-many keys should throw a helpful error message. This + tests the code path where a natural many-to-many key has only a single + value. + """ + test_string = """[{ + "pk": 1, + "model": "serializers.article", + "fields": { + "author": 1, + "headline": "Unknown many to many", + "pub_date": "2014-09-15T10:35:00", + "meta_data": [1, "doesnotexist"] + } + }, { + "pk": 1, + "model": "serializers.categorymetadata", + "fields": { + "kind": "author", + "name": "meta1", + "value": "Agnes" + } + }, { + "pk": 1, + "model": "serializers.author", + "fields": { + "name": "Agnes" + } + }]""" + expected = "(serializers.article:pk=1) field_value was 'doesnotexist'" + with self.assertRaisesMessage(serializers.base.DeserializationError, expected): + for obj in serializers.deserialize('json', test_string, ignore=False): + obj.save() + class JsonSerializerTransactionTestCase(SerializersTransactionTestBase, TransactionTestCase): serializer_name = "json" @@ -576,6 +739,7 @@ class YamlSerializerTestCase(SerializersTestBase, TestCase): headline: Poker has no place on ESPN pub_date: 2006-06-16 11:00:00 categories: [%(first_category_pk)s, %(second_category_pk)s] + meta_data: [] """ @staticmethod