From e308db0b1605e9641f38d34d63b0aa0e65d1b770 Mon Sep 17 00:00:00 2001 From: Giacomo Tagliabue Date: Fri, 14 Jul 2017 10:44:33 -0400 Subject: [PATCH 1/5] fix s3 delete_many Couldn't find any documentation around this issue, but I empirically discovered that `delete_objects` with an empty array causes the request to fail. Also, this is a valid optimization anyway. --- flask_annex/s3.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/flask_annex/s3.py b/flask_annex/s3.py index e833455..4684b42 100644 --- a/flask_annex/s3.py +++ b/flask_annex/s3.py @@ -40,11 +40,13 @@ def delete(self, key): self._client.delete_object(Bucket=self._bucket_name, Key=key) def delete_many(self, keys): + objects = tuple({'Key': key} for key in keys) + if not objects: + return + self._client.delete_objects( Bucket=self._bucket_name, - Delete={ - 'Objects': tuple({'Key': key} for key in keys), - }, + Delete={'Objects': objects}, ) def get_file(self, key, out_file): From 6489b38d6c452508be89f2f1d243944acb6e549a Mon Sep 17 00:00:00 2001 From: Giacomo Tagliabue Date: Fri, 14 Jul 2017 11:56:00 -0400 Subject: [PATCH 2/5] add tests and comments --- flask_annex/s3.py | 2 ++ tests/test_s3.py | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/flask_annex/s3.py b/flask_annex/s3.py index 4684b42..c09e3fe 100644 --- a/flask_annex/s3.py +++ b/flask_annex/s3.py @@ -42,6 +42,8 @@ def delete(self, key): def delete_many(self, keys): objects = tuple({'Key': key} for key in keys) if not objects: + # this is not just an optimization, boto fails if the array + # is empty return self._client.delete_objects( diff --git a/tests/test_s3.py b/tests/test_s3.py index 67e5a66..1850790 100644 --- a/tests/test_s3.py +++ b/tests/test_s3.py @@ -107,6 +107,14 @@ def test_get_upload_info_max_content_length(self, app, client): def assert_app_config_content_length_range(self, conditions): assert get_condition(conditions, 'content-length-range') == [0, 100] + def test_delete_many_empty_list(self, annex, monkeypatch): + def assert_not_called(*args, **kwargs): + assert(False) + + monkeypatch.setattr(annex._client, 'delete_objects', assert_not_called) + + annex.delete_many(tuple()) + class TestS3AnnexFromEnv(TestS3Annex): @pytest.fixture From 79e4e0df1dceff8b020876ebf849a131c4bd9b89 Mon Sep 17 00:00:00 2001 From: Giacomo Tagliabue Date: Fri, 14 Jul 2017 12:01:31 -0400 Subject: [PATCH 3/5] add comment --- flask_annex/s3.py | 1 + 1 file changed, 1 insertion(+) diff --git a/flask_annex/s3.py b/flask_annex/s3.py index c09e3fe..01a7758 100644 --- a/flask_annex/s3.py +++ b/flask_annex/s3.py @@ -40,6 +40,7 @@ def delete(self, key): self._client.delete_object(Bucket=self._bucket_name, Key=key) def delete_many(self, keys): + # casting to tuple because keys might be iterable objects = tuple({'Key': key} for key in keys) if not objects: # this is not just an optimization, boto fails if the array From 4cdc88fbaefaa662567a71d4c4dba18f87e0b537 Mon Sep 17 00:00:00 2001 From: Giacomo Tagliabue Date: Fri, 14 Jul 2017 12:32:22 -0400 Subject: [PATCH 4/5] use mock --- tests/test_s3.py | 9 +++++---- tox.ini | 1 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/test_s3.py b/tests/test_s3.py index 1850790..994695d 100644 --- a/tests/test_s3.py +++ b/tests/test_s3.py @@ -12,6 +12,7 @@ try: import boto3 + from mock import Mock from moto import mock_s3 import requests except ImportError: @@ -108,13 +109,13 @@ def assert_app_config_content_length_range(self, conditions): assert get_condition(conditions, 'content-length-range') == [0, 100] def test_delete_many_empty_list(self, annex, monkeypatch): - def assert_not_called(*args, **kwargs): - assert(False) - - monkeypatch.setattr(annex._client, 'delete_objects', assert_not_called) + mock = Mock() + monkeypatch.setattr(annex._client, 'delete_objects', mock) annex.delete_many(tuple()) + mock.assert_not_called() + class TestS3AnnexFromEnv(TestS3Annex): @pytest.fixture diff --git a/tox.ini b/tox.ini index 15789b6..84beab3 100644 --- a/tox.ini +++ b/tox.ini @@ -11,6 +11,7 @@ deps = s3: moto s3: requests + s3: mock commands = s3: pip install -e .[s3] From 5a0623d414a7741c72ab18c3a5fd6ed7b35b65eb Mon Sep 17 00:00:00 2001 From: Giacomo Tagliabue Date: Fri, 14 Jul 2017 12:43:10 -0400 Subject: [PATCH 5/5] install mock unconditionally --- tests/test_s3.py | 2 +- tox.ini | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_s3.py b/tests/test_s3.py index 994695d..348ddca 100644 --- a/tests/test_s3.py +++ b/tests/test_s3.py @@ -2,6 +2,7 @@ from io import BytesIO import json +from mock import Mock import pytest from flask_annex import Annex @@ -12,7 +13,6 @@ try: import boto3 - from mock import Mock from moto import mock_s3 import requests except ImportError: diff --git a/tox.ini b/tox.ini index 84beab3..101a817 100644 --- a/tox.ini +++ b/tox.ini @@ -6,12 +6,12 @@ usedevelop = True deps = flake8 + mock pytest pytest-cov s3: moto s3: requests - s3: mock commands = s3: pip install -e .[s3]