From 945a5623efa64e470294577a352c996ed11fcd62 Mon Sep 17 00:00:00 2001 From: Nabil Date: Sun, 21 Sep 2014 18:50:44 -0400 Subject: [PATCH 01/11] Fixed minor spelling error in README.rst --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index e098bae..80e0e75 100644 --- a/README.rst +++ b/README.rst @@ -136,7 +136,7 @@ particularly when the processing being done depends on user input. format = 'JPEG' options = {'quality': 60} -It's probaby not surprising that this class is capable of processing an image +It's probably not surprising that this class is capable of processing an image in the exact same way as our ImageSpecField above. However, unlike with the image spec model field, this class doesn't define what source the spec is acting on, or what should be done with the result; that's up to you: From f5b23a67bd4ab60203343741c923b0920a753fe0 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Tue, 23 Sep 2014 14:59:20 -0400 Subject: [PATCH 02/11] Remove test dir __init__.py Related: matthewwithanm/pilkit#14 --- tests/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 tests/__init__.py diff --git a/tests/__init__.py b/tests/__init__.py deleted file mode 100644 index e69de29..0000000 From 9f4192a7c6f8e185b9cc4e04941ef570d66ec335 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Tue, 23 Sep 2014 18:29:52 -0400 Subject: [PATCH 03/11] Ignore my Python3 virtualenv --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 40b09c6..575a800 100644 --- a/.gitignore +++ b/.gitignore @@ -10,4 +10,5 @@ dist /tests/media/* !/tests/media/lenna.png /venv +/venv3 /.env From c92f53c1b09f5e7dc01790d7a5f611a1bd2f0856 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Tue, 23 Sep 2014 18:32:00 -0400 Subject: [PATCH 04/11] Test that Optimistic strategy doesn't cause reads Using the Optimistic strategy should prevent IO ops when you cast the file as a boolean. --- setup.py | 1 + tests/test_optimistic_strategy.py | 37 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 tests/test_optimistic_strategy.py diff --git a/setup.py b/setup.py index 9a65738..8f60701 100644 --- a/setup.py +++ b/setup.py @@ -48,6 +48,7 @@ setup( 'nose-progressive==1.5', 'django-nose==1.2', 'Pillow<3.0', + 'mock==1.0.1', ], test_suite='testrunner.run_tests', install_requires=[ diff --git a/tests/test_optimistic_strategy.py b/tests/test_optimistic_strategy.py new file mode 100644 index 0000000..d3ce959 --- /dev/null +++ b/tests/test_optimistic_strategy.py @@ -0,0 +1,37 @@ +from nose.tools import assert_true, assert_false +from imagekit.cachefiles import ImageCacheFile +from mock import Mock +from .utils import create_image +from django.core.files.storage import FileSystemStorage +from imagekit.cachefiles.backends import Simple as SimpleCFBackend +from imagekit.cachefiles.strategies import Optimistic as OptimisticStrategy + + +class ImageGenerator(object): + def generate(self): + return create_image() + + def get_hash(self): + return 'abc123' + + +def get_image_cache_file(): + storage = Mock(FileSystemStorage) + backend = SimpleCFBackend() + strategy = OptimisticStrategy() + generator = ImageGenerator() + return ImageCacheFile(generator, storage=storage, + cachefile_backend=backend, + cachefile_strategy=strategy) + + +def test_no_io_on_bool(): + """ + When checking the truthiness of an ImageCacheFile, the storage shouldn't + peform IO operations. + + """ + file = get_image_cache_file() + bool(file) + assert_false(file.storage.exists.called) + assert_false(file.storage.open.called) From 00b4388245f7777276af351e3afd68af2417c426 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Tue, 23 Sep 2014 18:35:38 -0400 Subject: [PATCH 05/11] Support should_verify_existence on strategies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents extra IO. Different defaults are used for async backends since we can’t assume that `existence_required` resulted in existence synchronously. --- imagekit/cachefiles/__init__.py | 9 ++++++++- imagekit/cachefiles/backends.py | 2 ++ imagekit/cachefiles/strategies.py | 3 +++ tests/utils.py | 2 ++ 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/imagekit/cachefiles/__init__.py b/imagekit/cachefiles/__init__.py index 34892ec..2ba5cb8 100644 --- a/imagekit/cachefiles/__init__.py +++ b/imagekit/cachefiles/__init__.py @@ -128,7 +128,14 @@ class ImageCacheFile(BaseIKFile, ImageFile): # Dispatch the existence_required signal before checking to see if the # file exists. This gives the strategy a chance to create the file. existence_required.send(sender=self, file=self) - return self.cachefile_backend.exists(self) + + try: + check = self.cachefile_strategy.should_verify_existence(self) + except AttributeError: + # All synchronous backends should have created the file as part of + # `existence_required` if they wanted to. + check = getattr(self.cachefile_backend, 'is_async', False) + return self.cachefile_backend.exists(self) if check else True def __getstate__(self): state = copy(self.__dict__) diff --git a/imagekit/cachefiles/backends.py b/imagekit/cachefiles/backends.py index 25ce43f..1512f51 100644 --- a/imagekit/cachefiles/backends.py +++ b/imagekit/cachefiles/backends.py @@ -121,6 +121,8 @@ class BaseAsync(Simple): """ Base class for cache file backends that generate files asynchronously. """ + is_async = True + def generate(self, file, force=False): # Schedule the file for generation, unless we know for sure we don't # need to. If an already-generated file sneaks through, that's okay; diff --git a/imagekit/cachefiles/strategies.py b/imagekit/cachefiles/strategies.py index 98f712e..bcd8211 100644 --- a/imagekit/cachefiles/strategies.py +++ b/imagekit/cachefiles/strategies.py @@ -29,6 +29,9 @@ class Optimistic(object): def on_source_saved(self, file): file.generate() + def should_verify_existence(self, file): + return False + class DictStrategy(object): def __init__(self, callbacks): diff --git a/tests/utils.py b/tests/utils.py index 5a9f1c5..b6c4952 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -77,5 +77,7 @@ class DummyAsyncCacheFileBackend(Simple): A cache file backend meant to simulate async generation. """ + is_async = True + def generate(self, file, force=False): pass From bbf48a79539493fcade9b5cdb4b1c637b64961ee Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Tue, 23 Sep 2014 18:39:33 -0400 Subject: [PATCH 06/11] Test that there isn't IO done when you get a URL --- tests/test_optimistic_strategy.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_optimistic_strategy.py b/tests/test_optimistic_strategy.py index d3ce959..18b8f3f 100644 --- a/tests/test_optimistic_strategy.py +++ b/tests/test_optimistic_strategy.py @@ -35,3 +35,15 @@ def test_no_io_on_bool(): bool(file) assert_false(file.storage.exists.called) assert_false(file.storage.open.called) + + +def test_no_io_on_url(): + """ + When getting the URL of an ImageCacheFile, the storage shouldn't be + checked. + + """ + file = get_image_cache_file() + file.url + assert_false(file.storage.exists.called) + assert_false(file.storage.open.called) From 8d35dad5fc63de919936d0407d105c36c87a1b14 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Fri, 26 Sep 2014 20:42:53 -0400 Subject: [PATCH 07/11] Add test to illustrate GH-295 --- tests/test_no_extra_queries.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 tests/test_no_extra_queries.py diff --git a/tests/test_no_extra_queries.py b/tests/test_no_extra_queries.py new file mode 100644 index 0000000..89c68cb --- /dev/null +++ b/tests/test_no_extra_queries.py @@ -0,0 +1,16 @@ +from nose.tools import assert_false +from mock import Mock, PropertyMock, patch +from .models import Photo + + +def test_dont_access_source(): + """ + Touching the source may trigger an unneeded query. + See + + """ + pmock = PropertyMock() + pmock.__get__ = Mock() + with patch.object(Photo, 'original_image', pmock): + photo = Photo() # noqa + assert_false(pmock.__get__.called) From 78a1ccaf2fc6bc04bffa8edb8523307387b6f81e Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Fri, 26 Sep 2014 22:21:24 -0400 Subject: [PATCH 08/11] Only include fetched fields in initial hash of sources Should avoid unnecessary queries, as detailed in GH-295. --- imagekit/specs/sourcegroups.py | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/imagekit/specs/sourcegroups.py b/imagekit/specs/sourcegroups.py index 2008f59..d8ea3a7 100644 --- a/imagekit/specs/sourcegroups.py +++ b/imagekit/specs/sourcegroups.py @@ -72,18 +72,19 @@ class ModelSignalRouter(object): """ self.init_instance(instance) - instance._ik['source_hashes'] = dict((attname, hash(file_field)) - for attname, file_field in self.get_field_dict(instance).items()) + instance._ik['source_hashes'] = dict( + (attname, hash(getattr(instance, attname))) + for attname in self.get_source_fields(instance)) return instance._ik['source_hashes'] - def get_field_dict(self, instance): + def get_source_fields(self, instance): """ - Returns the source fields for the given instance, in a dictionary whose - keys are the field names and values are the fields themselves. + Returns a list of the source fields for the given instance. """ - return dict((src.image_field, getattr(instance, src.image_field)) for - src in self._source_groups if isinstance(instance, src.model_class)) + return set(src.image_field + for src in self._source_groups + if isinstance(instance, src.model_class)) @ik_model_receiver def post_save_receiver(self, sender, instance=None, created=False, raw=False, **kwargs): @@ -91,14 +92,22 @@ class ModelSignalRouter(object): self.init_instance(instance) old_hashes = instance._ik.get('source_hashes', {}).copy() new_hashes = self.update_source_hashes(instance) - for attname, file in self.get_field_dict(instance).items(): - if file and old_hashes[attname] != new_hashes[attname]: + for attname in self.get_source_fields(instance): + file = getattr(instance, attname) + if file and old_hashes.get(attname) != new_hashes[attname]: self.dispatch_signal(source_saved, file, sender, instance, attname) @ik_model_receiver def post_init_receiver(self, sender, instance=None, **kwargs): - self.update_source_hashes(instance) + self.init_instance(instance) + source_fields = self.get_source_fields(instance) + local_fields = dict((field.name, field) + for field in instance._meta.local_fields + if field.name in source_fields) + instance._ik['source_hashes'] = dict( + (attname, hash(file_field)) + for attname, file_field in local_fields.items()) def dispatch_signal(self, signal, file, model_class, instance, attname): """ From e2ae8508661f9e66b3d47106049a2df968d2e93f Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Sat, 27 Sep 2014 17:52:21 -0400 Subject: [PATCH 09/11] Revert "Remove test dir __init__.py" This reverts commit f5b23a67bd4ab60203343741c923b0920a753fe0. I forgot we were using 'tests.settings' as a settings module path. --- tests/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/__init__.py diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 From 3a2150e51587152623093dea176caa061c4c09ec Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Sat, 27 Sep 2014 18:03:07 -0400 Subject: [PATCH 10/11] Exclude tests from dist Related: matthewwithanm/pilkit#14 --- MANIFEST.in | 1 + 1 file changed, 1 insertion(+) diff --git a/MANIFEST.in b/MANIFEST.in index 0adbc50..94be0c5 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -3,3 +3,4 @@ include LICENSE include README.rst recursive-include docs * recursive-include imagekit/templates * +prune tests From 5f4f7070f4db89b8210d0da87ae78819a551e601 Mon Sep 17 00:00:00 2001 From: Bryan Veloso Date: Sat, 27 Sep 2014 22:20:40 -0700 Subject: [PATCH 11/11] Bump the version to 3.2.3. --- imagekit/pkgmeta.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imagekit/pkgmeta.py b/imagekit/pkgmeta.py index 81478c3..b7d0e81 100644 --- a/imagekit/pkgmeta.py +++ b/imagekit/pkgmeta.py @@ -1,5 +1,5 @@ __title__ = 'django-imagekit' __author__ = 'Matthew Tretter, Eric Eldredge, Bryan Veloso, Greg Newman, Chris Drackett, Justin Driscoll' -__version__ = '3.2.2' +__version__ = '3.2.3' __license__ = 'BSD' __all__ = ['__title__', '__author__', '__version__', '__license__']