From d8d61d8260badd87b85accaf5776587f2d355c10 Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 15:56:48 +0000 Subject: [PATCH 1/7] Added tests for missing pyyaml. This test makes sure an YAML import errors are communicated to the caller rather than stating the serializer does not exist. --- tests/fixtures/tests.py | 1 + tests/serializers/tests.py | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/fixtures/tests.py b/tests/fixtures/tests.py index c24e0806fa..d6348e82ff 100644 --- a/tests/fixtures/tests.py +++ b/tests/fixtures/tests.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +import unittest import warnings from django.contrib.sites.models import Site diff --git a/tests/serializers/tests.py b/tests/serializers/tests.py index 381cc5ed87..b32aa22657 100644 --- a/tests/serializers/tests.py +++ b/tests/serializers/tests.py @@ -14,7 +14,7 @@ except ImportError: from django.conf import settings -from django.core import serializers +from django.core import management, serializers from django.db import transaction, connection from django.test import TestCase, TransactionTestCase, Approximate from django.utils import six @@ -440,6 +440,26 @@ class JsonSerializerTransactionTestCase(SerializersTransactionTestBase, Transact }]""" +@unittest.skipIf(HAS_YAML, "Yaml is installed") +class NoYamlSerializerTestCase(TestCase): + """Not having pyyaml installed provides a misleading error + + #12756 + """ + def test_missing_pyyaml_error_message(self): + """Using yaml serializer without pyyaml raises ImportError""" + jane = Author(name="Jane") + self.assertRaises(ImportError, serializers.serialize, "yaml", [jane]) + + def test_deserializer_pyyaml_error_message(self): + """Using yaml deserializer without pyyaml raises ImportError""" + self.assertRaises(ImportError, serializers.deserialize, "yaml", "") + + def test_missing_pyyaml_error_message(self): + self.assertRaisesRegexp(management.CommandError, r'No module named yaml', + management.call_command, 'dumpdata', format='yaml') + + @unittest.skipUnless(HAS_YAML, "No yaml library detected") class YamlSerializerTestCase(SerializersTestBase, TestCase): serializer_name = "yaml" From c72392dab4b4cf6f5d9b35a3dcb582fbfe38ebb8 Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 15:56:06 +0000 Subject: [PATCH 2/7] Added yaml directly into BUILTIN_SERIALIZERS. The serializer definitely exists, but the dependent yaml module may not be installed. The register_serializer() function will catch exceptions and will stub in a fake serializer object that will raise the exception when the serializer is used. --- django/core/serializers/__init__.py | 40 +++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/django/core/serializers/__init__.py b/django/core/serializers/__init__.py index dc3d139d3b..4c9b5ba37a 100644 --- a/django/core/serializers/__init__.py +++ b/django/core/serializers/__init__.py @@ -17,6 +17,7 @@ To add your own serializers, use the SERIALIZATION_MODULES setting:: """ import importlib +import sys from django.conf import settings from django.utils import six @@ -27,17 +28,29 @@ BUILTIN_SERIALIZERS = { "xml" : "django.core.serializers.xml_serializer", "python" : "django.core.serializers.python", "json" : "django.core.serializers.json", + "yaml" : "django.core.serializers.pyyaml", } -# Check for PyYaml and register the serializer if it's available. -try: - import yaml - BUILTIN_SERIALIZERS["yaml"] = "django.core.serializers.pyyaml" -except ImportError: - pass - _serializers = {} + +class BadSerializer(object): + """ + Stub serializer to hold exception raised during registration + + This allows the serializer registration to cache serializers and if there + is an error raised in the process of creating a serializer it will be + raised and passed along to the caller when the serializer is used. + """ + internal_use_only = False + + def __init__(self, exception): + self.exception = exception + + def __call__(self, *args, **kwargs): + raise self.exception + + def register_serializer(format, serializer_module, serializers=None): """Register a new serializer. @@ -53,12 +66,23 @@ def register_serializer(format, serializer_module, serializers=None): """ if serializers is None and not _serializers: _load_serializers() - module = importlib.import_module(serializer_module) + + try: + module = importlib.import_module(serializer_module) + except ImportError, exc: + bad_serializer = BadSerializer(exc) + + module = type('BadSerializerModule', (object,), { + 'Deserializer': bad_serializer, + 'Serializer': bad_serializer, + }) + if serializers is None: _serializers[format] = module else: serializers[format] = module + def unregister_serializer(format): "Unregister a given serializer. This is not a thread-safe operation." if not _serializers: From 076cf131ec8084ce6d9cc3a05b0f4bcc167d6cbc Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 19:24:55 +0000 Subject: [PATCH 3/7] Moved get_serializer() call in dumpdata command. Moved the get_serializer() call within the condition that checks public serializers. This will allow exceptions other than SerializerDoesNotExist to be raised in order to provide the caller with useful information, e.g when pyyaml is not installed. --- django/core/management/commands/dumpdata.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/django/core/management/commands/dumpdata.py b/django/core/management/commands/dumpdata.py index c74eede846..ed58ec79a1 100644 --- a/django/core/management/commands/dumpdata.py +++ b/django/core/management/commands/dumpdata.py @@ -106,11 +106,11 @@ class Command(BaseCommand): # Check that the serialization format exists; this is a shortcut to # avoid collating all the objects and _then_ failing. if format not in serializers.get_public_serializer_formats(): - raise CommandError("Unknown serialization format: %s" % format) + try: + serializers.get_serializer(format) + except serializers.SerializerDoesNotExist: + pass - try: - serializers.get_serializer(format) - except KeyError: raise CommandError("Unknown serialization format: %s" % format) def get_objects(): From ca3ac4a3e37cc844bef7f7553c6b1a6cc642e6e6 Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 21:35:21 +0000 Subject: [PATCH 4/7] Updated NoYamlSerializerTestCase to run with yaml. In order to verify the behavior of using the yaml serializer when yaml is on the system, fake the ImportError when the serializer is being registered. --- tests/serializers/tests.py | 47 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/tests/serializers/tests.py b/tests/serializers/tests.py index b32aa22657..5ed6e7f9cb 100644 --- a/tests/serializers/tests.py +++ b/tests/serializers/tests.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- from __future__ import unicode_literals +import importlib import json from datetime import datetime import re @@ -440,12 +441,54 @@ class JsonSerializerTransactionTestCase(SerializersTransactionTestBase, Transact }]""" -@unittest.skipIf(HAS_YAML, "Yaml is installed") +YAML_IMPORT_ERROR_MESSAGE = r'No module named yaml' +class YamlImportModuleMock(object): + """Provides a wrapped import_module function to simulate yaml ImportError + + In order to run tests that verify the behavior of the YAML serializer + when run on a system that has yaml installed (like the django CI server), + mock import_module, so that it raises an ImportError when the yaml + serializer is being imported. The importlib.import_module() call is + being made in the serializers.register_serializer(). + + #12756 + """ + def __init__(self): + self._import_module = importlib.import_module + + def import_module(self, module_path): + if module_path == serializers.BUILTIN_SERIALIZERS['yaml']: + raise ImportError(YAML_IMPORT_ERROR_MESSAGE) + + return self._import_module(module_path) + + class NoYamlSerializerTestCase(TestCase): """Not having pyyaml installed provides a misleading error #12756 """ + @classmethod + def setUpClass(cls): + """Removes imported yaml and stubs importlib.import_module""" + super(NoYamlSerializerTestCase, cls).setUpClass() + + cls._import_module_mock = YamlImportModuleMock() + importlib.import_module = cls._import_module_mock.import_module + + # clear out cached serializers to emulate yaml missing + serializers._serializers = {} + + @classmethod + def tearDownClass(cls): + """Puts yaml back if necessary""" + super(NoYamlSerializerTestCase, cls).tearDownClass() + + importlib.import_module = cls._import_module_mock._import_module + + # clear out cached serializers to clean out BadSerializer instances + serializers._serializers = {} + def test_missing_pyyaml_error_message(self): """Using yaml serializer without pyyaml raises ImportError""" jane = Author(name="Jane") @@ -456,7 +499,7 @@ class NoYamlSerializerTestCase(TestCase): self.assertRaises(ImportError, serializers.deserialize, "yaml", "") def test_missing_pyyaml_error_message(self): - self.assertRaisesRegexp(management.CommandError, r'No module named yaml', + self.assertRaisesRegexp(management.CommandError, YAML_IMPORT_ERROR_MESSAGE, management.call_command, 'dumpdata', format='yaml') From 0ac5e8d8e945b2232333fd777e7684c4c2c68316 Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 21:57:32 +0000 Subject: [PATCH 5/7] Added name to AUTHORS (per Russell Keith-Magee) --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index c343eeb67b..2735f71e7b 100644 --- a/AUTHORS +++ b/AUTHORS @@ -651,6 +651,7 @@ answer newbie questions, and generally made Django that much better: Gasper Zejn Jarek Zgoda Cheng Zhang + Roberto Aguilar A big THANK YOU goes to: From 9587d4eea09bc8d8a852dfb24c2a9227c74fd8ad Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 22:22:04 +0000 Subject: [PATCH 6/7] Fixed existing tests to handle BadSerializer. When a BadSerializer instance is stubbed in for the yaml serializer, make sure tests do not fail. --- tests/serializers_regress/tests.py | 2 +- tests/timezones/tests.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/serializers_regress/tests.py b/tests/serializers_regress/tests.py index 3173f73985..0a8267a33b 100644 --- a/tests/serializers_regress/tests.py +++ b/tests/serializers_regress/tests.py @@ -523,7 +523,7 @@ def streamTest(format, self): else: self.assertEqual(string_data, stream.content.decode('utf-8')) -for format in serializers.get_serializer_formats(): +for format in filter(lambda x: not isinstance(serializers.get_serializer(x), serializers.BadSerializer), serializers.get_serializer_formats()): setattr(SerializerTests, 'test_' + format + '_serializer', curry(serializerTest, format)) setattr(SerializerTests, 'test_' + format + '_natural_key_serializer', curry(naturalKeySerializerTest, format)) setattr(SerializerTests, 'test_' + format + '_serializer_fields', curry(fieldsTest, format)) diff --git a/tests/timezones/tests.py b/tests/timezones/tests.py index 49169f90f2..9390eb93df 100644 --- a/tests/timezones/tests.py +++ b/tests/timezones/tests.py @@ -599,7 +599,7 @@ class SerializationTests(TestCase): obj = next(serializers.deserialize('xml', data)).object self.assertEqual(obj.dt, dt) - if 'yaml' in serializers.get_serializer_formats(): + if not isinstance(serializers.get_serializer('yaml'), serializers.BadSerializer): data = serializers.serialize('yaml', [Event(dt=dt)]) self.assert_yaml_contains_datetime(data, "2011-09-01 13:20:30") obj = next(serializers.deserialize('yaml', data)).object @@ -623,7 +623,7 @@ class SerializationTests(TestCase): obj = next(serializers.deserialize('xml', data)).object self.assertEqual(obj.dt, dt) - if 'yaml' in serializers.get_serializer_formats(): + if not isinstance(serializers.get_serializer('yaml'), serializers.BadSerializer): data = serializers.serialize('yaml', [Event(dt=dt)]) self.assert_yaml_contains_datetime(data, "2011-09-01 13:20:30.405060") obj = next(serializers.deserialize('yaml', data)).object @@ -647,7 +647,7 @@ class SerializationTests(TestCase): obj = next(serializers.deserialize('xml', data)).object self.assertEqual(obj.dt, dt) - if 'yaml' in serializers.get_serializer_formats(): + if not isinstance(serializers.get_serializer('yaml'), serializers.BadSerializer): data = serializers.serialize('yaml', [Event(dt=dt)]) self.assert_yaml_contains_datetime(data, "2011-09-01 17:20:30.405060+07:00") obj = next(serializers.deserialize('yaml', data)).object @@ -671,7 +671,7 @@ class SerializationTests(TestCase): obj = next(serializers.deserialize('xml', data)).object self.assertEqual(obj.dt, dt) - if 'yaml' in serializers.get_serializer_formats(): + if not isinstance(serializers.get_serializer('yaml'), serializers.BadSerializer): data = serializers.serialize('yaml', [Event(dt=dt)]) self.assert_yaml_contains_datetime(data, "2011-09-01 10:20:30+00:00") obj = next(serializers.deserialize('yaml', data)).object @@ -695,7 +695,7 @@ class SerializationTests(TestCase): obj = next(serializers.deserialize('xml', data)).object self.assertEqual(obj.dt, dt) - if 'yaml' in serializers.get_serializer_formats(): + if not isinstance(serializers.get_serializer('yaml'), serializers.BadSerializer): data = serializers.serialize('yaml', [Event(dt=dt)]) self.assert_yaml_contains_datetime(data, "2011-09-01 13:20:30+03:00") obj = next(serializers.deserialize('yaml', data)).object @@ -719,7 +719,7 @@ class SerializationTests(TestCase): obj = next(serializers.deserialize('xml', data)).object self.assertEqual(obj.dt, dt) - if 'yaml' in serializers.get_serializer_formats(): + if not isinstance(serializers.get_serializer('yaml'), serializers.BadSerializer): data = serializers.serialize('yaml', [Event(dt=dt)]) self.assert_yaml_contains_datetime(data, "2011-09-01 17:20:30+07:00") obj = next(serializers.deserialize('yaml', data)).object From 01a5359477fbc82488dddf9476581fe72c049b51 Mon Sep 17 00:00:00 2001 From: Roberto Aguilar Date: Fri, 6 Sep 2013 22:36:05 +0000 Subject: [PATCH 7/7] Cleanup commit after peer review. --- AUTHORS | 2 +- django/core/serializers/__init__.py | 1 - tests/fixtures/tests.py | 1 - tests/serializers/tests.py | 9 +++++---- tests/serializers_regress/tests.py | 5 ++++- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/AUTHORS b/AUTHORS index 2735f71e7b..75db300513 100644 --- a/AUTHORS +++ b/AUTHORS @@ -58,6 +58,7 @@ answer newbie questions, and generally made Django that much better: Gisle Aas Chris Adams Mathieu Agopian + Roberto Aguilar ajs alang@bright-green.com A S Alam @@ -651,7 +652,6 @@ answer newbie questions, and generally made Django that much better: Gasper Zejn Jarek Zgoda Cheng Zhang - Roberto Aguilar A big THANK YOU goes to: diff --git a/django/core/serializers/__init__.py b/django/core/serializers/__init__.py index 4c9b5ba37a..89d9877ebf 100644 --- a/django/core/serializers/__init__.py +++ b/django/core/serializers/__init__.py @@ -17,7 +17,6 @@ To add your own serializers, use the SERIALIZATION_MODULES setting:: """ import importlib -import sys from django.conf import settings from django.utils import six diff --git a/tests/fixtures/tests.py b/tests/fixtures/tests.py index d6348e82ff..c24e0806fa 100644 --- a/tests/fixtures/tests.py +++ b/tests/fixtures/tests.py @@ -1,6 +1,5 @@ from __future__ import unicode_literals -import unittest import warnings from django.contrib.sites.models import Site diff --git a/tests/serializers/tests.py b/tests/serializers/tests.py index 5ed6e7f9cb..9af8165314 100644 --- a/tests/serializers/tests.py +++ b/tests/serializers/tests.py @@ -451,7 +451,7 @@ class YamlImportModuleMock(object): serializer is being imported. The importlib.import_module() call is being made in the serializers.register_serializer(). - #12756 + Refs: #12756 """ def __init__(self): self._import_module = importlib.import_module @@ -466,7 +466,7 @@ class YamlImportModuleMock(object): class NoYamlSerializerTestCase(TestCase): """Not having pyyaml installed provides a misleading error - #12756 + Refs: #12756 """ @classmethod def setUpClass(cls): @@ -489,7 +489,7 @@ class NoYamlSerializerTestCase(TestCase): # clear out cached serializers to clean out BadSerializer instances serializers._serializers = {} - def test_missing_pyyaml_error_message(self): + def test_serializer_pyyaml_error_message(self): """Using yaml serializer without pyyaml raises ImportError""" jane = Author(name="Jane") self.assertRaises(ImportError, serializers.serialize, "yaml", [jane]) @@ -498,7 +498,8 @@ class NoYamlSerializerTestCase(TestCase): """Using yaml deserializer without pyyaml raises ImportError""" self.assertRaises(ImportError, serializers.deserialize, "yaml", "") - def test_missing_pyyaml_error_message(self): + def test_dumpdata_pyyaml_error_message(self): + """Calling dumpdata produces an error when yaml package missing""" self.assertRaisesRegexp(management.CommandError, YAML_IMPORT_ERROR_MESSAGE, management.call_command, 'dumpdata', format='yaml') diff --git a/tests/serializers_regress/tests.py b/tests/serializers_regress/tests.py index 0a8267a33b..bd71d4da6a 100644 --- a/tests/serializers_regress/tests.py +++ b/tests/serializers_regress/tests.py @@ -523,7 +523,10 @@ def streamTest(format, self): else: self.assertEqual(string_data, stream.content.decode('utf-8')) -for format in filter(lambda x: not isinstance(serializers.get_serializer(x), serializers.BadSerializer), serializers.get_serializer_formats()): +for format in [ + f for f in serializers.get_serializer_formats() + if not isinstance(serializers.get_serializer(f), serializers.BadSerializer) + ]: setattr(SerializerTests, 'test_' + format + '_serializer', curry(serializerTest, format)) setattr(SerializerTests, 'test_' + format + '_natural_key_serializer', curry(naturalKeySerializerTest, format)) setattr(SerializerTests, 'test_' + format + '_serializer_fields', curry(fieldsTest, format))