Changes to `ImageFileDescriptor` and `ImageField` to fix a few cases of setting image dimension fields.

 * Moved dimension field update logic out of `ImageFileDescriptor.__set__` and into its own method on `ImageField`.
 * New `ImageField.update_dimension_fields` method is attached to model instance's `post_init` signal so that:
   * Dimension fields are set when defined before the ImageField.
   * Dimension fields are set when the field is assigned in the model constructor (fixes #11196), but only if the dimension fields don't already have values, so we avoid updating the dimensions every time an object is loaded from the database (fixes #11084).
 * Clear dimension fields when the ImageField is set to None, which also causes dimension fields to be cleared when `ImageFieldFile.delete()` is used.
 * Added many more tests for ImageField that test edge cases we weren't testing before, and moved the ImageField tests out of `file_storage` and into their own module within `model_fields`.


git-svn-id: http://code.djangoproject.com/svn/django/trunk@10858 bcc190cf-cafb-0310-a4f2-bffc1f526a37

5 files changed, 521 insertions, 5 deletions

diff --git a/tests/regressiontests/model_fields/4x8.png b/tests/regressiontests/model_fields/4x8.png
new file mode 100644
index 0000000000..ffce444d48
--- /dev/null
+++ b/tests/regressiontests/model_fields/4x8.png
diff --git a/tests/regressiontests/model_fields/8x4.png b/tests/regressiontests/model_fields/8x4.png
new file mode 100644
index 0000000000..60e6d69ee1
--- /dev/null
+++ b/tests/regressiontests/model_fields/8x4.png
diff --git a/tests/regressiontests/model_fields/imagefield.py b/tests/regressiontests/model_fields/imagefield.py
new file mode 100644
index 0000000000..09bda6bb2d
--- /dev/null
+++ b/tests/regressiontests/model_fields/imagefield.py
@@ -0,0 +1,403 @@
+import os
+import shutil
+
+from django.core.files import File
+from django.core.files.base import ContentFile
+from django.core.files.images import ImageFile
+from django.test import TestCase
+
+from models import Image, Person, PersonWithHeight, PersonWithHeightAndWidth, \
+        PersonDimensionsFirst, PersonTwoImages, TestImageFieldFile
+
+
+# If PIL available, do these tests.
+if Image:
+
+    from models import temp_storage_dir
+
+
+    class ImageFieldTestMixin(object):
+        """
+        Mixin class to provide common functionality to ImageField test classes.
+        """
+
+        # Person model to use for tests.
+        PersonModel = PersonWithHeightAndWidth
+        # File class to use for file instances.
+        File = ImageFile
+
+        def setUp(self):
+            """
+            Creates a pristine temp directory (or deletes and recreates if it
+            already exists) that the model uses as its storage directory.
+
+            Sets up two ImageFile instances for use in tests.
+            """
+            if os.path.exists(temp_storage_dir):
+                shutil.rmtree(temp_storage_dir)
+            os.mkdir(temp_storage_dir)
+
+            file_path1 = os.path.join(os.path.dirname(__file__), "4x8.png")
+            self.file1 = self.File(open(file_path1, 'rb'))
+
+            file_path2 = os.path.join(os.path.dirname(__file__), "8x4.png")
+            self.file2 = self.File(open(file_path2, 'rb'))
+
+        def tearDown(self):
+            """
+            Removes temp directory and all its contents.
+            """
+            shutil.rmtree(temp_storage_dir)
+
+        def check_dimensions(self, instance, width, height,
+                             field_name='mugshot'):
+            """
+            Asserts that the given width and height values match both the
+            field's height and width attributes and the height and width fields
+            (if defined) the image field is caching to.
+
+            Note, this method will check for dimension fields named by adding
+            "_width" or "_height" to the name of the ImageField.  So, the
+            models used in these tests must have their fields named
+            accordingly.
+
+            By default, we check the field named "mugshot", but this can be
+            specified by passing the field_name parameter.
+            """
+            field = getattr(instance, field_name)
+            # Check height/width attributes of field.
+            if width is None and height is None:
+                self.assertRaises(ValueError, getattr, field, 'width')
+                self.assertRaises(ValueError, getattr, field, 'height')
+            else:
+                self.assertEqual(field.width, width)
+                self.assertEqual(field.height, height)
+
+            # Check height/width fields of model, if defined.
+            width_field_name = field_name + '_width'
+            if hasattr(instance, width_field_name):
+                self.assertEqual(getattr(instance, width_field_name), width)
+            height_field_name = field_name + '_height'
+            if hasattr(instance, height_field_name):
+                self.assertEqual(getattr(instance, height_field_name), height)
+
+
+    class ImageFieldTests(ImageFieldTestMixin, TestCase):
+        """
+        Tests for ImageField that don't need to be run with each of the
+        different test model classes.
+        """
+
+        def test_equal_notequal_hash(self):
+            """
+            Bug #9786: Ensure '==' and '!=' work correctly.
+            Bug #9508: make sure hash() works as expected (equal items must
+            hash to the same value).
+            """
+            # Create two Persons with different mugshots.
+            p1 = self.PersonModel(name="Joe")
+            p1.mugshot.save("mug", self.file1)
+            p2 = self.PersonModel(name="Bob")
+            p2.mugshot.save("mug", self.file2)
+            self.assertEqual(p1.mugshot == p2.mugshot, False)
+            self.assertEqual(p1.mugshot != p2.mugshot, True)
+
+            # Test again with an instance fetched from the db.
+            p1_db = self.PersonModel.objects.get(name="Joe")
+            self.assertEqual(p1_db.mugshot == p2.mugshot, False)
+            self.assertEqual(p1_db.mugshot != p2.mugshot, True)
+
+            # Instance from db should match the local instance.
+            self.assertEqual(p1_db.mugshot == p1.mugshot, True)
+            self.assertEqual(hash(p1_db.mugshot), hash(p1.mugshot))
+            self.assertEqual(p1_db.mugshot != p1.mugshot, False)
+
+        def test_instantiate_missing(self):
+            """
+            If the underlying file is unavailable, still create instantiate the
+            object without error.
+            """
+            p = self.PersonModel(name="Joan")
+            p.mugshot.save("shot", self.file1)
+            p = self.PersonModel.objects.get(name="Joan")
+            path = p.mugshot.path
+            shutil.move(path, path + '.moved')
+            p2 = self.PersonModel.objects.get(name="Joan")
+
+        def test_delete_when_missing(self):
+            """
+            Bug #8175: correctly delete an object where the file no longer
+            exists on the file system.
+            """
+            p = self.PersonModel(name="Fred")
+            p.mugshot.save("shot", self.file1)
+            os.remove(p.mugshot.path)
+            p.delete()
+
+        def test_size_method(self):
+            """
+            Bug #8534: FileField.size should not leave the file open.
+            """
+            p = self.PersonModel(name="Joan")
+            p.mugshot.save("shot", self.file1)
+
+            # Get a "clean" model instance
+            p = self.PersonModel.objects.get(name="Joan")
+            # It won't have an opened file.
+            self.assertEqual(p.mugshot.closed, True)
+
+            # After asking for the size, the file should still be closed.
+            _ = p.mugshot.size
+            self.assertEqual(p.mugshot.closed, True)
+
+
+    class ImageFieldTwoDimensionsTests(ImageFieldTestMixin, TestCase):
+        """
+        Tests behavior of an ImageField and its dimensions fields.
+        """
+
+        def test_constructor(self):
+            """
+            Tests assigning an image field through the model's constructor.
+            """
+            p = self.PersonModel(name='Joe', mugshot=self.file1)
+            self.check_dimensions(p, 4, 8)
+            p.save()
+            self.check_dimensions(p, 4, 8)
+
+        def test_image_after_constructor(self):
+            """
+            Tests behavior when image is not passed in constructor.
+            """
+            p = self.PersonModel(name='Joe')
+            # TestImageField value will default to being an instance of its
+            # attr_class, a  TestImageFieldFile, with name == None, which will
+            # cause it to evaluate as False.
+            self.assertEqual(isinstance(p.mugshot, TestImageFieldFile), True)
+            self.assertEqual(bool(p.mugshot), False)
+
+            # Test setting a fresh created model instance.
+            p = self.PersonModel(name='Joe')
+            p.mugshot = self.file1
+            self.check_dimensions(p, 4, 8)
+
+        def test_create(self):
+            """
+            Tests assigning an image in Manager.create().
+            """
+            p = self.PersonModel.objects.create(name='Joe', mugshot=self.file1)
+            self.check_dimensions(p, 4, 8)
+
+        def test_default_value(self):
+            """
+            Tests that the default value for an ImageField is an instance of
+            the field's attr_class (TestImageFieldFile in this case) with no
+            name (name set to None).
+            """
+            p = self.PersonModel()
+            self.assertEqual(isinstance(p.mugshot, TestImageFieldFile), True)
+            self.assertEqual(bool(p.mugshot), False)
+
+        def test_assignment_to_None(self):
+            """
+            Tests that assigning ImageField to None clears dimensions.
+            """
+            p = self.PersonModel(name='Joe', mugshot=self.file1)
+            self.check_dimensions(p, 4, 8)
+
+            # If image assigned to None, dimension fields should be cleared.
+            p.mugshot = None
+            self.check_dimensions(p, None, None)
+
+            p.mugshot = self.file2
+            self.check_dimensions(p, 8, 4)
+
+        def test_field_save_and_delete_methods(self):
+            """
+            Tests assignment using the field's save method and deletion using
+            the field's delete method.
+            """
+            p = self.PersonModel(name='Joe')
+            p.mugshot.save("mug", self.file1)
+            self.check_dimensions(p, 4, 8)
+
+            # A new file should update dimensions.
+            p.mugshot.save("mug", self.file2)
+            self.check_dimensions(p, 8, 4)
+
+            # Field and dimensions should be cleared after a delete.
+            p.mugshot.delete(save=False)
+            self.assertEqual(p.mugshot, None)
+            self.check_dimensions(p, None, None)
+
+        def test_dimensions(self):
+            """
+            Checks that dimensions are updated correctly in various situations.
+            """
+            p = self.PersonModel(name='Joe')
+
+            # Dimensions should get set if file is saved.
+            p.mugshot.save("mug", self.file1)
+            self.check_dimensions(p, 4, 8)
+
+            # Test dimensions after fetching from database.
+            p = self.PersonModel.objects.get(name='Joe')
+            # Bug 11084: Dimensions should not get recalculated if file is
+            # coming from the database.  We test this by checking if the file
+            # was opened.
+            self.assertEqual(p.mugshot.was_opened, False)
+            self.check_dimensions(p, 4, 8)
+            # After checking dimensions on the image field, the file will have
+            # opened.
+            self.assertEqual(p.mugshot.was_opened, True)
+            # Dimensions should now be cached, and if we reset was_opened and
+            # check dimensions again, the file should not have opened.
+            p.mugshot.was_opened = False
+            self.check_dimensions(p, 4, 8)
+            self.assertEqual(p.mugshot.was_opened, False)
+
+            # If we assign a new image to the instance, the dimensions should
+            # update.
+            p.mugshot = self.file2
+            self.check_dimensions(p, 8, 4)
+            # Dimensions were recalculated, and hence file should have opened.
+            self.assertEqual(p.mugshot.was_opened, True)
+
+
+    class ImageFieldNoDimensionsTests(ImageFieldTwoDimensionsTests):
+        """
+        Tests behavior of an ImageField with no dimension fields.
+        """
+
+        PersonModel = Person
+
+
+    class ImageFieldOneDimensionTests(ImageFieldTwoDimensionsTests):
+        """
+        Tests behavior of an ImageField with one dimensions field.
+        """
+
+        PersonModel = PersonWithHeight
+
+
+    class ImageFieldDimensionsFirstTests(ImageFieldTwoDimensionsTests):
+        """
+        Tests behavior of an ImageField where the dimensions fields are
+        defined before the ImageField.
+        """
+
+        PersonModel = PersonDimensionsFirst
+
+
+    class ImageFieldUsingFileTests(ImageFieldTwoDimensionsTests):
+        """
+        Tests behavior of an ImageField when assigning it a File instance
+        rather than an ImageFile instance.
+        """
+
+        PersonModel = PersonDimensionsFirst
+        File = File
+
+
+    class TwoImageFieldTests(ImageFieldTestMixin, TestCase):
+        """
+        Tests a model with two ImageFields.
+        """
+
+        PersonModel = PersonTwoImages
+
+        def test_constructor(self):
+            p = self.PersonModel(mugshot=self.file1, headshot=self.file2)
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+            p.save()
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+
+        def test_create(self):
+            p = self.PersonModel.objects.create(mugshot=self.file1,
+                                                headshot=self.file2)
+            self.check_dimensions(p, 4, 8)
+            self.check_dimensions(p, 8, 4, 'headshot')
+
+        def test_assignment(self):
+            p = self.PersonModel()
+            self.check_dimensions(p, None, None, 'mugshot')
+            self.check_dimensions(p, None, None, 'headshot')
+
+            p.mugshot = self.file1
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, None, None, 'headshot')
+            p.headshot = self.file2
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+
+            # Clear the ImageFields one at a time.
+            p.mugshot = None
+            self.check_dimensions(p, None, None, 'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+            p.headshot = None
+            self.check_dimensions(p, None, None, 'mugshot')
+            self.check_dimensions(p, None, None, 'headshot')
+
+        def test_field_save_and_delete_methods(self):
+            p = self.PersonModel(name='Joe')
+            p.mugshot.save("mug", self.file1)
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, None, None, 'headshot')
+            p.headshot.save("head", self.file2)
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+
+            # We can use save=True when deleting the image field with null=True
+            # dimension fields and the other field has an image.
+            p.headshot.delete(save=True)
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, None, None, 'headshot')
+            p.mugshot.delete(save=False)
+            self.check_dimensions(p, None, None, 'mugshot')
+            self.check_dimensions(p, None, None, 'headshot')
+
+        def test_dimensions(self):
+            """
+            Checks that dimensions are updated correctly in various situations.
+            """
+            p = self.PersonModel(name='Joe')
+
+            # Dimensions should get set for the saved file.
+            p.mugshot.save("mug", self.file1)
+            p.headshot.save("head", self.file2)
+            self.check_dimensions(p, 4, 8, 'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+
+            # Test dimensions after fetching from database.
+            p = self.PersonModel.objects.get(name='Joe')
+            # Bug 11084: Dimensions should not get recalculated if file is
+            # coming from the database.  We test this by checking if the file
+            # was opened.
+            self.assertEqual(p.mugshot.was_opened, False)
+            self.assertEqual(p.headshot.was_opened, False)
+            self.check_dimensions(p, 4, 8,'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+            # After checking dimensions on the image fields, the files will
+            # have been opened.
+            self.assertEqual(p.mugshot.was_opened, True)
+            self.assertEqual(p.headshot.was_opened, True)
+            # Dimensions should now be cached, and if we reset was_opened and
+            # check dimensions again, the file should not have opened.
+            p.mugshot.was_opened = False
+            p.headshot.was_opened = False
+            self.check_dimensions(p, 4, 8,'mugshot')
+            self.check_dimensions(p, 8, 4, 'headshot')
+            self.assertEqual(p.mugshot.was_opened, False)
+            self.assertEqual(p.headshot.was_opened, False)
+
+            # If we assign a new image to the instance, the dimensions should
+            # update.
+            p.mugshot = self.file2
+            p.headshot = self.file1
+            self.check_dimensions(p, 8, 4, 'mugshot')
+            self.check_dimensions(p, 4, 8, 'headshot')
+            # Dimensions were recalculated, and hence file should have opened.
+            self.assertEqual(p.mugshot.was_opened, True)
+            self.assertEqual(p.headshot.was_opened, True)
diff --git a/tests/regressiontests/model_fields/models.py b/tests/regressiontests/model_fields/models.py
index 43ef667d00..81e6c22f09 100644
--- a/tests/regressiontests/model_fields/models.py
+++ b/tests/regressiontests/model_fields/models.py
@@ -1,10 +1,23 @@
-from django.db import models
+import os
+import tempfile
 
 try:
     import decimal
 except ImportError:
     from django.utils import _decimal as decimal    # Python 2.3 fallback
 
+try:
+    # Checking for the existence of Image is enough for CPython, but for PyPy,
+    # you need to check for the underlying modules.
+    from PIL import Image, _imaging
+except ImportError:
+    Image = None
+
+from django.core.files.storage import FileSystemStorage
+from django.db import models
+from django.db.models.fields.files import ImageFieldFile, ImageField
+
+
 class Foo(models.Model):
     a = models.CharField(max_length=10)
     d = models.DecimalField(max_digits=5, decimal_places=3)
@@ -31,9 +44,97 @@ class Whiz(models.Model):
         (0,'Other'),
     )
     c = models.IntegerField(choices=CHOICES, null=True)
-    
+
 class BigD(models.Model):
     d = models.DecimalField(max_digits=38, decimal_places=30)
 
 class BigS(models.Model):
-    s = models.SlugField(max_length=255)
\ No newline at end of file
+    s = models.SlugField(max_length=255)
+
+
+###############################################################################
+# ImageField
+
+# If PIL available, do these tests.
+if Image:
+    class TestImageFieldFile(ImageFieldFile):
+        """
+        Custom Field File class that records whether or not the underlying file
+        was opened.
+        """
+        def __init__(self, *args, **kwargs):
+            self.was_opened = False
+            super(TestImageFieldFile, self).__init__(*args,**kwargs)
+        def open(self):
+            self.was_opened = True
+            super(TestImageFieldFile, self).open()
+
+    class TestImageField(ImageField):
+        attr_class = TestImageFieldFile
+
+    # Set up a temp directory for file storage.
+    temp_storage_dir = tempfile.mkdtemp()
+    temp_storage = FileSystemStorage(temp_storage_dir)
+    temp_upload_to_dir = os.path.join(temp_storage.location, 'tests')
+
+    class Person(models.Model):
+        """
+        Model that defines an ImageField with no dimension fields.
+        """
+        name = models.CharField(max_length=50)
+        mugshot = TestImageField(storage=temp_storage, upload_to='tests')
+
+    class PersonWithHeight(models.Model):
+        """
+        Model that defines an ImageField with only one dimension field.
+        """
+        name = models.CharField(max_length=50)
+        mugshot = TestImageField(storage=temp_storage, upload_to='tests',
+                                 height_field='mugshot_height')
+        mugshot_height = models.PositiveSmallIntegerField()
+
+    class PersonWithHeightAndWidth(models.Model):
+        """
+        Model that defines height and width fields after the ImageField.
+        """
+        name = models.CharField(max_length=50)
+        mugshot = TestImageField(storage=temp_storage, upload_to='tests',
+                                 height_field='mugshot_height',
+                                 width_field='mugshot_width')
+        mugshot_height = models.PositiveSmallIntegerField()
+        mugshot_width = models.PositiveSmallIntegerField()
+
+    class PersonDimensionsFirst(models.Model):
+        """
+        Model that defines height and width fields before the ImageField.
+        """
+        name = models.CharField(max_length=50)
+        mugshot_height = models.PositiveSmallIntegerField()
+        mugshot_width = models.PositiveSmallIntegerField()
+        mugshot = TestImageField(storage=temp_storage, upload_to='tests',
+                                 height_field='mugshot_height',
+                                 width_field='mugshot_width')
+
+    class PersonTwoImages(models.Model):
+        """
+        Model that:
+        * Defines two ImageFields
+        * Defines the height/width fields before the ImageFields
+        * Has a nullalble ImageField
+        """
+        name = models.CharField(max_length=50)
+        mugshot_height = models.PositiveSmallIntegerField()
+        mugshot_width = models.PositiveSmallIntegerField()
+        mugshot = TestImageField(storage=temp_storage, upload_to='tests',
+                                 height_field='mugshot_height',
+                                 width_field='mugshot_width')
+        headshot_height = models.PositiveSmallIntegerField(
+                blank=True, null=True)
+        headshot_width = models.PositiveSmallIntegerField(
+                blank=True, null=True)
+        headshot = TestImageField(blank=True, null=True,
+                                  storage=temp_storage, upload_to='tests',
+                                  height_field='headshot_height',
+                                  width_field='headshot_width')
+
+###############################################################################
diff --git a/tests/regressiontests/model_fields/tests.py b/tests/regressiontests/model_fields/tests.py
index 0815d9ef3c..7a6fee5a2a 100644
--- a/tests/regressiontests/model_fields/tests.py
+++ b/tests/regressiontests/model_fields/tests.py
@@ -6,13 +6,26 @@ from django import forms
 from django.db import models
 from django.core.exceptions import ValidationError
 
-from models import Foo, Bar, Whiz, BigD, BigS
+from models import Foo, Bar, Whiz, BigD, BigS, Image
 
 try:
     from decimal import Decimal
 except ImportError:
     from django.utils._decimal import Decimal
 
+
+# If PIL available, do these tests.
+if Image:
+    from imagefield import \
+            ImageFieldTests, \
+            ImageFieldTwoDimensionsTests, \
+            ImageFieldNoDimensionsTests, \
+            ImageFieldOneDimensionTests, \
+            ImageFieldDimensionsFirstTests, \
+            ImageFieldUsingFileTests, \
+            TwoImageFieldTests
+
+
 class DecimalFieldTests(django.test.TestCase):
     def test_to_python(self):
         f = models.DecimalField(max_digits=4, decimal_places=2)
@@ -131,4 +144,3 @@ class SlugFieldTests(django.test.TestCase):
         bs = BigS.objects.create(s = 'slug'*50)
         bs = BigS.objects.get(pk=bs.pk)
         self.assertEqual(bs.s, 'slug'*50)
-