Merge lp:~sinzui/launchpad/field-errors-0 into lp:launchpad

[Curtis Hovey]

Catch the ValueError while validating image file

    Launchpad bug: https://bugs.launchpad.net/bugs/50616
    Pre-implementation: no one
    Test command: ./bin/test -vv \
      -t test_fields -t image-widget

Usually when the PIL can't identify an image type, it raises a IOError, but
it might raise a "ValueError: cannot read this XPM file" when uploading a
broken XPM file. The existing LaunchpadValidationError message is correct,
but it is not being raised because ValueError was not expected.

--------------------------------------------------------------------

RULES

    * Reproduce an XPM fragment that raises the ValueError.
    * Add ValueError to the except clause.

QA

    * Upload the abcardwindow.xpm file attached to the bug.
    * Verify the form explain that the image was not recognised.

LINT

    lib/lp/app/widgets/doc/image-widget.txt
    lib/lp/services/fields/__init__.py
    lib/lp/services/fields/tests/test_fields.py

./lib/lp/app/widgets/doc/image-widget.txt
       1: narrative uses a moin header.
      36: narrative uses a moin header.
      77: source exceeds 78 characters.
     233: narrative exceeds 78 characters.
     250: narrative uses a moin header.
     314: narrative exceeds 78 characters.
     317: narrative exceeds 78 characters.
     358: narrative uses a moin header.
     421: narrative exceeds 78 characters.
     476: narrative uses a moin header.
./lib/lp/services/fields/__init__.py
     350: E302 expected 2 blank lines, found 1

^ I will fix this before I land the branch. I do not want to clutter the
diff.

IMPLEMENTATION

Converted the existing BaseImageUpload IOError doctest into a unittest.
    lib/lp/app/widgets/doc/image-widget.txt
    lib/lp/services/fields/tests/test_fields.py

Reproduced the ValueError using a corrupt XPM and added the ValueError to
the exception.
    lib/lp/services/fields/tests/test_fields.py
    lib/lp/services/fields/__init__.py

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/lp/app/widgets/doc/image-widget.txt'
--- lib/lp/app/widgets/doc/image-widget.txt	2011-02-01 21:46:58 +0000
+++ lib/lp/app/widgets/doc/image-widget.txt	2011-02-08 22:06:49 +0000
@@ -1,24 +1,27 @@
-= ImageChange widget =
-
-In Launchpad we have images associated with people, products, distributions,
-etc, and we want to allow people to have full control over their images. That
-is, they must be able to upload a new image and delete (or keep) an existing
-one. For this we created this widget, which can be embeded into any form we
-want, which doesn't require us to add any submit buttons to indicate that the
-image should be kept, deleted or changed.
-
-The widget is composed by a RadioWidget and a FileWidget, where the radio
-specifies the action that should be performed (keep the existing image, change
-back to the default image or change it to an user-uploaded one) and the
-FileWidget gives us the user-uploaded file, in case there is one.
-
-Whenever you have a form in which you want to use the image widget, you have
-to explicitly say whether you want to use its ADD_STYLE or EDIT_STYLE
-incarnation, by passing an extra argument to the widget's constructor
-(or to custom_widget(), if you're using it).
-
-Our policy is not to ask people to upload images when creating a record, but
-instead to expose this as an edit form after the object is created.
+ImageChange widget
+==================
+
+In Launchpad we have images associated with people, products,
+distributions, etc, and we want to allow people to have full control
+over their images. That is, they must be able to upload a new image and
+delete (or keep) an existing one. For this we created this widget, which
+can be embeded into any form we want, which doesn't require us to add
+any submit buttons to indicate that the image should be kept, deleted or
+changed.
+
+The widget is composed by a RadioWidget and a FileWidget, where the
+radio specifies the action that should be performed (keep the existing
+image, change back to the default image or change it to an user-uploaded
+one) and the FileWidget gives us the user-uploaded file, in case there
+is one.
+
+Whenever you have a form in which you want to use the image widget, you
+have to explicitly say whether you want to use its ADD_STYLE or
+EDIT_STYLE incarnation, by passing an extra argument to the widget's
+constructor (or to custom_widget(), if you're using it).
+
+Our policy is not to ask people to upload images when creating a record,
+but instead to expose this as an edit form after the object is created.
 
 Let's use Salgado and the Launchpad Administrators team as an examples
 here, since they haven't uploaded custom logos yet.
@@ -27,22 +30,26 @@
     >>> salgado = getUtility(IPersonSet).getByName('salgado')
     >>> salgado.logo is None
     True
+
     >>> admins_team = getUtility(IPersonSet).getByName('admins')
     >>> admins_team.logo is None
     True
+
     >>> admins_team.icon is None
     True
 
-== The ADD_STYLE/EDIT_STYLE incarnations ==
+
+The ADD_STYLE/EDIT_STYLE incarnations
+-------------------------------------
 
 The only difference between them is that the ADD_STYLE has a different
-set of labels for its options and never returns our special flag to indicate
-that the image should be kept, since there's nothing to be kept. For that
-reason I'll only demonstrate the EDIT_STYLE here.
+set of labels for its options and never returns our special flag to
+indicate that the image should be kept, since there's nothing to be
+kept. For that reason I'll only demonstrate the EDIT_STYLE here.
 
-Since Salgado has no logo, the widget will display the default person-logo
-image and the 'Keep' radio button will be selected. The
-other radio button allows the user to upload a new image.
+Since Salgado has no logo, the widget will display the default person-
+logo image and the 'Keep' radio button will be selected. The other radio
+button allows the user to upload a new image.
 
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
     >>> from lp.app.widgets.image import ImageChangeWidget
@@ -70,17 +77,18 @@
     keep: SELECTED
     change: NOT SELECTED
 
-
-If we set any random file as salgado's logo, we'll see it there, as
-well as an option to delete the image that was just uploaded.
-
-    >>> from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+If we set any random file as salgado's logo, we'll see it there, as well
+as an option to delete the image that was just uploaded.
+
+    >>> from canonical.launchpad.interfaces.librarian import (
+    ...     ILibraryFileAliasSet)
     >>> login('[e-mail]')
     >>> logo = getUtility(ILibraryFileAliasSet)[53]
     >>> salgado.logo = logo
 
     # Need to create a new widget instance since we changed our context
     # manually.
+
     >>> widget = ImageChangeWidget(
     ...     person_logo, LaunchpadTestRequest(), edit_style)
 
@@ -93,9 +101,8 @@
     delete: NOT SELECTED
     change: NOT SELECTED
 
-
-Now we'll stuff values in our request to simulate a user playing with the
-widget. Let's see how it reacts.
+Now we'll stuff values in our request to simulate a user playing with
+the widget. Let's see how it reacts.
 
 First, let's tell it to keep the existing image.
 
@@ -131,24 +138,26 @@
     >>> fileupload = widget.getInputValue()
     >>> fileupload.filename
     u'logo.png'
+
     >>> fileupload.content.filesize == logo.len
     True
 
-In order for this widget to work on add forms, we need to make sure it works
-when its field is bounded to an object that doesn't have the attribute that
-the field represents.
+In order for this widget to work on add forms, we need to make sure it
+works when its field is bounded to an object that doesn't have the
+attribute that the field represents.
 
     >>> personset_logo = IPerson['logo'].bind(getUtility(IPersonSet))
     >>> form = {'field.logo.action': 'keep'}
     >>> widget = ImageChangeWidget(
     ...     personset_logo, LaunchpadTestRequest(form=form), add_style)
 
-Note that in this case the KEEP_SAME_IMAGE flag doesn't make sense, so we
-return None, which is a sensible value that can be fed to a method which
-creates a new database object for us.
+Note that in this case the KEEP_SAME_IMAGE flag doesn't make sense, so
+we return None, which is a sensible value that can be fed to a method
+which creates a new database object for us.
 
     >>> widget.getInputValue() == None
     True
+
     >>> print_radio_items(widget())
     keep: SELECTED
     change: NOT SELECTED
@@ -160,125 +169,47 @@
     >>> print_radio_items(widget())
     keep: NOT SELECTED
     change: SELECTED
+
     >>> widget.getInputValue().content.filesize == logo.len
     True
 
 
-# This section is commented out because we are not doing the autoscaling
-# magic for 1.0 (mark - 2007-03-29)
-
-#=== The LogoTiedWithMugshotWidget ===
-#
-#This is a specialized version of the ImageChangeWidget which also returns a
-#resized version of the uploaded image, in case one was uploaded.
-#
-#    >>> from lp.app.widgets.image import LogoTiedWithMugshotWidget
-#    >>> mugshot_file_name = os.path.join(
-#    ...     os.path.dirname(canonical.launchpad.__file__),
-#    ...     'images/nyet-mugshot.png')
-#    >>> mugshot = StringIO(open(mugshot_file_name, 'r').read())
-#    >>> mugshot.filename = 'mugshot.png'
-#    >>> form = {'field.mugshot.action': 'change',
-#    ...         'field.mugshot.image': mugshot}
-#    >>> widget = LogoTiedWithMugshotWidget(
-#    ...     person_mugshot, LaunchpadTestRequest(form=form), edit_style)
-#    >>> image, smaller_img = widget.getInputValue()
-#
-#    >>> image.content.filesize == mugshot.len
-#    True
-#
-#    # Need to commit so that we can fetch the content from the librarian.
-#    >>> import transaction
-#    >>> transaction.commit()
-#    >>> import PIL.Image
-#    >>> orig_image = PIL.Image.open(StringIO(image.read()))
-#    >>> smaller_image = PIL.Image.open(StringIO(smaller_img.read()))
-#
-#Since the original image's size was smaller than the size we wanted to resize
-#it to, we don't resize it.
-#
-#    >>> smaller_image.size == orig_image.size
-#    True
-#
-#Now that the original image is big, the other one will actually be resized.
-#
-#    >>> big_mugshot_file_name = os.path.join(
-#    ...     os.path.dirname(canonical.launchpad.__file__),
-#    ...     'pagetests/standalone/bigicon.jpg')
-#    >>> mugshot = StringIO(open(big_mugshot_file_name, 'r').read())
-#    >>> mugshot.filename = 'bigicon.jpg'
-#    >>> form = {'field.gotchi.action': 'change',
-#    ...         'field.gotchi.image': mugshot}
-#    >>> widget = GotchiTiedWithHeadingWidget(
-#    ...     person_gotchi, LaunchpadTestRequest(form=form), edit_style)
-#    >>> image, smaller_img = widget.getInputValue()
-#    >>> transaction.commit()
-#    >>> orig_image = PIL.Image.open(StringIO(image.read()))
-#    >>> smaller_image = PIL.Image.open(StringIO(smaller_img.read()))
-#
-#    >>> smaller_image.size != orig_image.size
-#    True
-#
-#    >>> small_width, small_height = smaller_image.size
-#    >>> scale = "%.1f" % (float(small_width) / small_height)
-#    >>> width, height = orig_image.size
-#    >>> orig_scale = "%.1f" % (float(width) / height)
-#
-#    >>> small_width < width and small_height < height
-#    True
-#    >>> scale == orig_scale
-#    True
-#
-#When we want to delete the image, it'll return a two-tuple of None, so that
-#callsites can just unpack it like they would do if it had returned two images.
-#
-#    >>> form = {'field.gotchi.action': 'delete'}
-#    >>> widget = GotchiTiedWithHeadingWidget(
-#    ...     person_gotchi, LaunchpadTestRequest(form=form), edit_style)
-#    >>> widget.getInputValue()
-#    (None, None)
-#
-#The same holds for when we want to keep an image.
-#
-#    >>> form = {'field.gotchi.action': 'keep'}
-#    >>> widget = GotchiTiedWithHeadingWidget(
-#    ...     person_gotchi, LaunchpadTestRequest(form=form), edit_style)
-#    >>> widget.getInputValue() == (KEEP_SAME_IMAGE, KEEP_SAME_IMAGE)
-#    True
-
-
-== The IconImageUpload, LogoImageUpload and MugshotImageUpload fields ==
+The IconImageUpload, LogoImageUpload and MugshotImageUpload fields
+------------------------------------------------------------------
 
 There are three fields which are used for image uploads. They are all
 subsclasses of the same BaseImageUpload class, and the only thing they
-change in each case is the max_size exact dimensions. We will only test the
-IconImageUpload and MugshotImageUpload widgets below.
+change in each case is the max_size exact dimensions. We will only test
+the IconImageUpload and MugshotImageUpload widgets below.
 
 Since this is a special widget which returns a special object
-(KEEP_SAME_IMAGE) to indicate that the image should be kept, we need to use
-a custom field (IconImageUpload) together with it. That field should not be
-used directly, since it specifies some constraints and defaults that are
-specific to each image, so you must subclass it before using.
-
-    >>> from lp.services.fields import (BaseImageUpload,
-    ... IconImageUpload)
-
-Note: the .bind method here is fetching the field from the IPerson schema
-(which should be an IconImageUpload, a subclass of BaseImageUpload) and
-binding it to Launchpad Administrators.
+(KEEP_SAME_IMAGE) to indicate that the image should be kept, we need to
+use a custom field (IconImageUpload) together with it. That field should
+not be used directly, since it specifies some constraints and defaults
+that are specific to each image, so you must subclass it before using.
+
+    >>> from lp.services.fields import (
+    ...     BaseImageUpload, IconImageUpload)
+
+Note: the .bind method here is fetching the field from the IPerson
+schema (which should be an IconImageUpload, a subclass of
+BaseImageUpload) and binding it to Launchpad Administrators.
 
     >>> person_icon = IPerson['icon'].bind(admins_team)
     >>> isinstance(person_icon, BaseImageUpload)
     True
+
     >>> isinstance(person_icon, IconImageUpload)
     True
+
     >>> person_icon.max_size
     5120
+
     >>> person_icon.dimensions
     (14, 14)
 
-If we pass that special object (KEEP_SAME_IMAGE) to IconImageUpload's set()
-method, the current image will be kept.
+If we pass that special object (KEEP_SAME_IMAGE) to IconImageUpload's
+set() method, the current image will be kept.
 
     >>> admins_team.icon = getUtility(ILibraryFileAliasSet)[53]
     >>> existing_img = admins_team.icon
@@ -301,68 +232,20 @@
     >>> person_icon.set(admins_team, fileupload)
     >>> admins_team.icon is None
     False
+
     >>> admins_team.icon == existing_img
     False
 
 
-# This section is disabled until we have implemented a similar but imrpved
-# Wdiget, that lets you OPTIONALLY resize from the mugshot, OR upload a
-# custom Logo
-
-#=== The LargeImageUpload field ===
-#
-#This is a special version of the BaseImageUpload one which knows how to handle
-#the two images returned by GotchiTiedWithHeadingWidget. The first image is
-#stored in the attribute the field represents and the other one is stored on
-#an attribute called gotchi_heading of the same object. Any class in which this
-#field is used must provide a gotchi_heading attribute.
-#
-#    >>> from lp.services.fields import LargeImageUpload
-#    >>> person_gotchi.__class__ == LargeImageUpload
-#    True
-#
-#    >>> existing_img = salgado.gotchi
-#    >>> existing_img is None
-#    False
-#
-#    >>> person_gotchi.set(salgado, (KEEP_SAME_IMAGE, KEEP_SAME_IMAGE))
-#    >>> salgado.gotchi == existing_img
-#    True
-#
-#    >>> person_gotchi.set(salgado, (None, None))
-#    >>> salgado.gotchi is None
-#
-#True
-#
-#Unlike the normal BaseImageUpload implementation, this one doesn't handle a
-#single LibraryFileAlias --it only handles tuples of them.
-#
-#    >>> person_gotchi.set(salgado, fileupload)
-#    Traceback (most recent call last):
-#    ...
-#    AssertionError
-#
-#    >>> person_gotchi.set(
-#    ...     salgado, (fileupload, getUtility(ILibraryFileAliasSet)[53]))
-#    >>> salgado.gotchi is None
-#    False
-#    >>> salgado.gotchi == existing_img
-#    False
-#
-#    >>> salgado.gotchi_heading is None
-#    False
-#    >>> salgado.gotchi_heading == salgado.gotchi
-#    False
-
-
-== Input validation ==
+Input validation
+----------------
 
 The BaseImageUpload field expects an image with the exact dimensions and
 within the stated constraints, so it won't accept anything else.
 
-We will try submit a logo to the mugshot image upload widget. Since we have
-an image with a byte size smaller than person_mugshot.max_size BUT dimensions
-smaller than person_mugshot.dimensions, it must be rejected.
+We will try submit a logo to the mugshot image upload widget. Since we
+have an image with a byte size smaller than person_mugshot.max_size BUT
+dimensions smaller than person_mugshot.dimensions, it must be rejected.
 
     >>> import PIL.Image
     >>> person_mugshot = IPerson['mugshot'].bind(salgado)
@@ -373,6 +256,7 @@
     >>> logo.filename = 'logo.png'
     >>> logo.len <= person_mugshot.max_size
     True
+
     >>> image = PIL.Image.open(logo)
     >>> image.size <= person_mugshot.dimensions
     True
@@ -387,9 +271,8 @@
                        LaunchpadValidationError(u'\nThis image is not exactly
                                                   192x192\npixels in size.'))
 
-
-This is what we see when the image is the correct dimensions, and within the
-max_size:
+This is what we see when the image is the correct dimensions, and within
+the max_size:
 
     >>> mugshot_file_name = os.path.join(
     ...     os.path.dirname(canonical.launchpad.__file__),
@@ -415,11 +298,12 @@
     >>> fileupload = widget.getInputValue()
     >>> fileupload.filename
     u'mugshot.png'
+
     >>> fileupload.content.filesize == mugshot.len
     True
 
-If we change person_mugshot's max_size to be smaller than our test image, we'll
-get a validation error.
+If we change person_mugshot's max_size to be smaller than our test
+image, we'll get a validation error.
 
     >>> person_mugshot.max_size = mugshot.len - 1
     >>> mugshot.seek(0)
@@ -433,8 +317,8 @@
                                                   maximum allowed size in
                                                   bytes.'))
 
-A similar error will be raised if the image's dimensions are bigger than the
-maximum we allow.
+A similar error will be raised if the image's dimensions are bigger than
+the maximum we allow.
 
     >>> person_mugshot.max_size = mugshot.len
     >>> person_mugshot.dimensions = (image.size[0] - 1, image.size[1] + 1)
@@ -459,26 +343,8 @@
                        LaunchpadValidationError(u'\nThis image is not exactly
                                                   193x191\npixels in size.'))
 
-We also won't accept anything that is not an image; that is, we only accept
-what can be parsed by the Python Imaging Library module.
-
-    >>> mugshot = StringIO('foo bar bz')
-    >>> mugshot.filename = 'foo.jpg'
-    >>> form = {'field.mugshot.action': 'change',
-    ...         'field.mugshot.image': mugshot}
-    >>> widget = ImageChangeWidget(
-    ...     person_mugshot, LaunchpadTestRequest(form=form), edit_style)
-    >>> widget.getInputValue()
-    Traceback (most recent call last):
-    ...
-    WidgetInputError: ('field.mugshot', u'Mugshot',
-                       LaunchpadValidationError(u'\nThe file uploaded was not
-                                                  recognized as an image;
-                                                  please\ncheck it and
-                                                  retry.'))
-
-Finally, if the user specifies the 'change' action he must also provide a file
-to be uploaded.
+Finally, if the user specifies the 'change' action he must also provide
+a file to be uploaded.
 
     >>> form = {'field.mugshot.action': 'change', 'field.mugshot.image': ''}
     >>> widget = ImageChangeWidget(
@@ -491,12 +357,12 @@
                                                   want to use.'))
 
 
-== Non-exact Image Dimensions ==
+Non-exact Image Dimensions
+--------------------------
 
-For some input fields, we don't require a particular size for an
-image, but want to enforce a maximum size on the image.  This can be
-achieved by setting the exact_dimensions attribute of the field to
-False:
+For some input fields, we don't require a particular size for an image,
+but want to enforce a maximum size on the image.  This can be achieved
+by setting the exact_dimensions attribute of the field to False:
 
     >>> person_mugshot.exact_dimensions = False
     >>> person_mugshot.dimensions = (64, 64)
@@ -532,3 +398,5 @@
     >>> fileupload = widget.getInputValue()
     >>> fileupload.filename
     u'mugshot.png'
+
+

=== modified file 'lib/lp/services/fields/__init__.py'
--- lib/lp/services/fields/__init__.py	2011-01-24 20:53:10 +0000
+++ lib/lp/services/fields/__init__.py	2011-02-08 22:06:49 +0000
@@ -717,7 +717,7 @@
                 This image exceeds the maximum allowed size in bytes.""")))
         try:
             pil_image = PIL.Image.open(StringIO(image))
-        except IOError:
+        except (IOError, ValueError):
             raise LaunchpadValidationError(_(dedent("""
                 The file uploaded was not recognized as an image; please
                 check it and retry.""")))

=== modified file 'lib/lp/services/fields/tests/test_fields.py'
--- lib/lp/services/fields/tests/test_fields.py	2011-01-06 19:29:11 +0000
+++ lib/lp/services/fields/tests/test_fields.py	2011-02-08 22:06:49 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 import datetime
+from StringIO import StringIO
 import time
 
 from zope.interface import Interface
@@ -15,6 +16,7 @@
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.services.fields import (
+    BaseImageUpload,
     BlacklistableContentNameField,
     FormattableDate,
     StrippableText,
@@ -128,3 +130,33 @@
         date_value = u'fnord'
         login_person(self.team.teamowner)
         self.assertEqual(None, field.validate(date_value))
+
+
+class TestBaseImageUpload(TestCase):
+    """Test for the BaseImageUpload field."""
+
+    class ExampleImageUpload(BaseImageUpload):
+        dimensions = (192, 192)
+        max_size = 100*1024
+
+    def test_validation_corrupt_image(self):
+        # ValueErrors raised by PIL become LaunchpadValidationErrors.
+        field = self.ExampleImageUpload(default_image_resource='dummy')
+        image = StringIO(
+            '/* XPM */\n'
+            'static char *pixmap[] = {\n'
+            '"32 32 253 2",\n'
+            '  "00 c #01CAA3",\n'
+            '  ".. s None c None",\n'
+            '};')
+        image.filename = 'foo.xpm'
+        self.assertRaises(
+            LaunchpadValidationError, field.validate, image)
+
+    def test_validation_non_image(self):
+        # IOError raised by PIL become LaunchpadValidationErrors.
+        field = self.ExampleImageUpload(default_image_resource='dummy')
+        image = StringIO('foo bar bz')
+        image.filename = 'foo.jpg'
+        self.assertRaises(
+            LaunchpadValidationError, field.validate, image)