search

melinath / django-daguerre

On-the-fly image manipulation for Django.
http://django-daguerre.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
85 stars 15 forks source link

Add handling of struct.error during Image.verify #78

Closed melinath closed 8 years ago

melinath commented 8 years ago

See python-pillow/Pillow#1755. Pillow now throws struct.error in certain situations where we expect something else (specifically IndexError). Though that may or may not be the proper behavior, there are at least 3 versions of Pillow out with this behavior; we should handle it.

melinath commented 8 years ago

It looks like the latest version now raises either SyntaxError or IOError :-/ See python-pillow/Pillow#1805. Not sure if that's been released yet.

I don't really want to test every version of Pillow with every different version of Python for just this one test. Maybe we could use mock side effects to test that each option is handled?

chigby commented 8 years ago

I tried writing some tests for this and they all passed without me doing anything. Looks like the only place we call Image.verify is here: https://github.com/littleweaver/django-daguerre/blob/master/daguerre/helpers.py#L354 -- and we except everything.

@melinath is your proposal to replace except Exception with the three exceptions we know could potentially be raised?

melinath commented 8 years ago

@chigby This ticket was based on a reported (or experienced) bug. I don't remember the details any more. We except every subclass of Exception, you're right. Maybe struct.error is not a subclass of Exception? But you said you tested it and it was caught?

melinath commented 8 years ago

Oh, the place it was failing was itself a test case which was looking for a specific error.

melinath commented 8 years ago

I guess just updating that test case to make sure the exception is an Exception subclass would be fine.

chigby commented 8 years ago

Ahhh, okay, the problem is in the test. I can fix that.