From 07836299823ac32c0591d8034a5f9f9d92321596 Mon Sep 17 00:00:00 2001 From: David Vogt Date: Fri, 5 Mar 2021 15:42:03 +0100 Subject: [PATCH] fix(dependencies): update minio client to adjust for changed exceptions Minio in the newest version changed some API sturucture. Mainly, the specific exceptions got wrapped into the `S3Error` exception. While we're at it, also remove some duplicate code and move it into a decorator (Any other place would imply more boilerplate). --- caluma/caluma_form/storage_clients.py | 80 ++++++++++++++++---------- caluma/caluma_form/tests/test_minio.py | 64 +++++++++++++++++++++ 2 files changed, 115 insertions(+), 29 deletions(-) diff --git a/caluma/caluma_form/storage_clients.py b/caluma/caluma_form/storage_clients.py index 17adb5404..1b90e99ab 100644 --- a/caluma/caluma_form/storage_clients.py +++ b/caluma/caluma_form/storage_clients.py @@ -1,8 +1,43 @@ from datetime import timedelta +from functools import wraps +from logging import getLogger import minio import urllib3 from django.conf import settings +from minio.error import S3Error + +log = getLogger(__name__) + + +class Retry(BaseException): + pass + + +def _retry_on_missing_bucket(fn): + """Create missing bucket if needed (decorator). + + If enabled in the settings, try to create the bucket if it + doesn't exist yet, then retry. + """ + + @wraps(fn) + def wrapper(self, *args, **kwargs): + try: + return fn(self, *args, **kwargs) + except S3Error as exc: + if ( + exc.code == "NoSuchBucket" + and settings.MINIO_STORAGE_AUTO_CREATE_MEDIA_BUCKET + ): + log.warning( + b"Minio bucket '{self.bucket}' missing, trying to create it" + ) + self.client.make_bucket(self.bucket) + return fn(self, *args, **kwargs) + raise + + return wrapper class Minio: @@ -34,6 +69,7 @@ def __init__(self): ) self.bucket = settings.MINIO_STORAGE_MEDIA_BUCKET_NAME + @_retry_on_missing_bucket def stat_object(self, object_name): """ Get stat of object in bucket. @@ -43,39 +79,25 @@ def stat_object(self, object_name): """ try: return self.client.stat_object(self.bucket, object_name) - except minio.error.NoSuchKey: # pragma: no cover - # object does not exist - pass - except minio.error.NoSuchBucket: # pragma: no cover - if settings.MINIO_STORAGE_AUTO_CREATE_MEDIA_BUCKET: - self.client.make_bucket(self.bucket) - return self.stat_object(object_name) - except minio.error.ResponseError: # pragma: no cover - pass + except S3Error as exc: + log.error(f"Minio error, cannot stat object: {exc.code}") + return None + @_retry_on_missing_bucket def download_url(self, object_name): - try: - return self.client.presigned_get_object( - self.bucket, - object_name, - timedelta(minutes=settings.MINIO_PRESIGNED_TTL_MINUTES), - ) - except minio.error.NoSuchBucket: # pragma: no cover - if settings.MINIO_STORAGE_AUTO_CREATE_MEDIA_BUCKET: - self.client.make_bucket(self.bucket) - return self.download_url(object_name) + return self.client.presigned_get_object( + self.bucket, + object_name, + timedelta(minutes=settings.MINIO_PRESIGNED_TTL_MINUTES), + ) + @_retry_on_missing_bucket def upload_url(self, object_name): - try: - return self.client.presigned_put_object( - self.bucket, - object_name, - timedelta(minutes=settings.MINIO_PRESIGNED_TTL_MINUTES), - ) - except minio.error.NoSuchBucket: # pragma: no cover - if settings.MINIO_STORAGE_AUTO_CREATE_MEDIA_BUCKET: - self.client.make_bucket(self.bucket) - return self.upload_url(object_name) + return self.client.presigned_put_object( + self.bucket, + object_name, + timedelta(minutes=settings.MINIO_PRESIGNED_TTL_MINUTES), + ) def remove_object(self, object_name): self.client.remove_object(self.bucket, object_name) diff --git a/caluma/caluma_form/tests/test_minio.py b/caluma/caluma_form/tests/test_minio.py index e71b84d58..f860de44b 100644 --- a/caluma/caluma_form/tests/test_minio.py +++ b/caluma/caluma_form/tests/test_minio.py @@ -1,3 +1,4 @@ +import minio.error import pytest import urllib3 @@ -28,3 +29,66 @@ def test_minio_disable_cert_checks(db, settings, disable_cert_checks, debug): with pytest.raises(urllib3.exceptions.RequestError): # This should fail, as we are in fact verifying certificates client.client._http.urlopen("get", "https://self-signed.badssl.com/") + + +def _put_side_effect(*_, **__): + # first call, raise an exception, second time, succeed + yield minio.error.S3Error( + "NoSuchBucket", + "Bucket does not exist", + resource="adsf", + request_id="fake", + host_id="fake", + response="fake", + ) + yield "upload_url_successful" + + +@pytest.fixture +def patched_minio(mocker): + make_bucket = mocker.patch("minio.api.Minio.make_bucket") + put_object = mocker.patch( + "minio.api.Minio.presigned_put_object", side_effect=_put_side_effect() + ) + return make_bucket, put_object + + +def test_minio_auto_create_bucket_enabled(db, settings, patched_minio): + settings.MINIO_STORAGE_AUTO_CREATE_MEDIA_BUCKET = True + client = storage_clients.Minio() + + make_bucket, _ = patched_minio + + assert client.upload_url("asdf") == "upload_url_successful" + assert make_bucket.call_count == 1 + + +def test_minio_auto_create_bucket_disabled(db, settings, patched_minio): + settings.MINIO_STORAGE_AUTO_CREATE_MEDIA_BUCKET = False + client = storage_clients.Minio() + make_bucket, _ = patched_minio + + with pytest.raises(minio.error.S3Error): + client.upload_url("asdf") + + assert make_bucket.call_count == 0 + + +@pytest.mark.parametrize("exc_code", ["NoSuchBucket", "NoSuchKey"]) +def test_minio_handle_exceptions(exc_code, caplog, mocker): + # The "happy path" is tested in various places already + mocker.patch( + "minio.api.Minio.stat_object", + side_effect=minio.error.S3Error( + code=exc_code, + message="", + resource="test_object", + request_id="", + host_id="", + response=None, + ), + ) + client = storage_clients.Minio() + stat = client.stat_object("test_object") + assert stat is None + assert caplog.messages == [f"Minio error, cannot stat object: {exc_code}"]