Add GCSMediaStorage, and share a CloudMediaStorage base with S3

.github/workflows/tests.yaml

@@ -23,6 +23,7 @@ jobs:
                   pip install -r requirements/test-requirements.txt
                   pip install -r requirements/extras/authenticator.txt
                   pip install -r requirements/extras/pynacl.txt
+                  pip install -r requirements/extras/gcs.txt
 
             - name: Lint
               run: ./scripts/lint.sh
@@ -64,6 +65,7 @@ jobs:
                   pip install -r requirements/test-requirements.txt
                   pip install -r requirements/extras/authenticator.txt
                   pip install -r requirements/extras/pynacl.txt
+                  pip install -r requirements/extras/gcs.txt
             - name: Test with pytest, Postgres
               run: ./scripts/test-postgres.sh
               env:
@@ -93,6 +95,7 @@ jobs:
                   pip install -r requirements/test-requirements.txt
                   pip install -r requirements/extras/authenticator.txt
                   pip install -r requirements/extras/pynacl.txt
+                  pip install -r requirements/extras/gcs.txt
             - name: Test with pytest, SQLite
               run: ./scripts/test-sqlite.sh
             - name: Upload coverage

CHANGES.rst

@@ -1,6 +1,79 @@
 Changes
 =======
 
+Unreleased
+----------
+
+Added ``GCSMediaStorage``, for storing media files in Google Cloud Storage
+(``pip install 'piccolo_api[gcs]'``).
+
+.. note::
+    Signing a URL needs a private key. On GCP compute (e.g. Cloud Run) the
+    Application Default Credentials don't have one, so signing goes through
+    the IAM API instead - grant the runtime service account
+    ``roles/iam.serviceAccountTokenCreator`` on itself. Note that this costs
+    an HTTPS request per URL, so use a service account key if you're showing
+    a lot of files on one page.
+
+Added ``CloudMediaStorage``, which is the shared base class for object storage
+backends. ``S3MediaStorage`` now uses it too - a new backend just has to
+implement the ``_sync`` methods, and the base class takes care of running them
+in an executor.
+
+Storage identity (``__hash__`` / ``__eq__``) moved onto ``MediaStorage``, via a
+``_hash_components`` method which each backend implements. Previously
+``MediaStorage`` defined ``__eq__`` without ``__hash__``, so a custom backend
+had to know to supply its own ``__hash__`` or the inherited ``__eq__`` would
+fail with a confusing ``TypeError``. A custom backend which does define
+``__hash__`` still works exactly as before.
+
+``MediaStorage`` also gained a ``_run_sync`` helper, and an ``executor``
+attribute for the backends which use it.
+
+Fixed several bugs in ``S3MediaStorage``:
+
+* The ``ContentType`` of an upload was written back to ``upload_metadata``, so
+  it leaked into subsequent uploads. With several uploads in flight at once it
+  could also be applied to the wrong file.
+* ``get_file_keys()`` stripped the folder name with ``str.lstrip()``, which
+  removes *characters* rather than a prefix - so a file called
+  ``poster.jpg`` in a ``movie_posters`` folder came back as ``.jpg``.
+* ``bulk_delete_files()`` sent the whole list of keys on every iteration
+  instead of the current batch, and the batch bounds were wrong. Deleting
+  more than 1000 files failed.
+* A ``folder_name`` with a trailing slash stored files in one place and looked
+  for them in another, so ``delete_unused_files()`` never found anything.
+* An uppercase file extension (``photo.JPG``) was uploaded with no
+  ``ContentType``, so browsers downloaded it instead of displaying it.
+
+``S3MediaStorage`` and ``GCSMediaStorage`` now cache their client, instead of
+building one per operation. ``S3MediaStorage.get_unsigned_client`` is new.
+
+``LocalMediaStorage.bulk_delete_files`` and
+``LocalMediaStorage.get_file_keys`` were doing their file system work directly
+in the event loop, rather than in the executor like the other methods.
+``LocalMediaStorage`` now follows the same convention as the other backends,
+with the ``_sync`` methods doing the work and the async ones wrapping them.
+
+``bulk_delete_files`` now ignores files which have already gone, on every
+backend - a single stale key used to abandon the rest of the batch. Any other
+error is raised, including the per-key errors which ``S3MediaStorage`` used to
+discard - deleting from a bucket you don't have ``s3:DeleteObject`` on
+previously looked like a successful clean up.
+
+``folder_name`` is now tidied up when it's stored, so that a trailing slash or
+a repeated slash can't send files somewhere we then fail to list. On Linux and
+macOS this matches the keys files were already stored under, so nothing needs
+migrating.
+
+.. warning::
+    On **Windows** the old code built keys with a backslash
+    (``movie_posters\\my-file.jpeg``), and they're now built with a forward
+    slash. If you've been running on Windows, existing files won't be found
+    until you rename them in the bucket.
+
+-------------------------------------------------------------------------------
+
 1.10.0
 ------
 

docs/source/api_reference/index.rst

@@ -43,3 +43,11 @@ Media
 .. currentmodule:: piccolo_api.media.s3
 
 .. autoclass:: S3MediaStorage
+
+.. currentmodule:: piccolo_api.media.gcs
+
+.. autoclass:: GCSMediaStorage
+
+.. currentmodule:: piccolo_api.media.cloud
+
+.. autoclass:: CloudMediaStorage

piccolo_api/media/base.py

@@ -6,12 +6,17 @@
 import pathlib
 import string
 import uuid
-from collections.abc import Sequence
-from typing import IO, Optional, Union
+from collections.abc import Callable, Sequence
+from typing import IO, TYPE_CHECKING, Optional, TypeVar, Union
 
 from piccolo.apps.user.tables import BaseUser
 from piccolo.columns.column_types import Array, Text, Varchar
 
+if TYPE_CHECKING:  # pragma: no cover
+    from concurrent.futures._base import Executor
+
+T = TypeVar("T")
+
 #: Pass into ``allowed_characters`` to just allow audio files.
 AUDIO_EXTENSIONS = (
     "mp3",
@@ -76,7 +81,8 @@
 class MediaStorage(metaclass=abc.ABCMeta):
     """
     If you want to implement your own custom storage backend, create a subclass
-    of this class. Override each abstract method.
+    of this class. Override each abstract method, and
+    :meth:`_hash_components`.
 
     Typically, just use :class:`LocalMediaStorage <piccolo_admin.media.local.LocalMediaStorage>`
     or :class:`S3MediaStorage <piccolo_admin.media.s3.S3MediaStorage>` instead.
@@ -232,7 +238,8 @@ async def generate_file_url(
     @abc.abstractmethod
     async def get_file(self, file_key: str) -> Optional[IO]:
         """
-        Returns the file object matching the ``file_key``.
+        Returns the file object matching the ``file_key``. The caller is
+        responsible for closing it.
         """
         raise NotImplementedError  # pragma: no cover
 
@@ -315,7 +322,44 @@ async def delete_unused_files(
             ):
                 await self.bulk_delete_files(unused_file_keys)
 
+    ###########################################################################
+
+    #: Subclasses which do blocking work set this, and use :meth:`_run_sync`
+    #: to keep that work off the event loop. If it's left as ``None``, the
+    #: event loop's default executor is used.
+    executor: Optional[Executor] = None
+
+    async def _run_sync(self, func: Callable[[], T]) -> T:
+        """
+        Runs a blocking function in :attr:`executor`, so it doesn't block the
+        event loop.
+        """
+        loop = asyncio.get_running_loop()
+        return await loop.run_in_executor(self.executor, func)
+
+    ###########################################################################
+
+    def _hash_components(self) -> tuple:
+        """
+        The values which identify where this storage keeps its files - for
+        example, the bucket and folder name. Two storages with the same
+        components are treated as the same storage, which is how Piccolo Admin
+        detects columns which would overwrite each other's files.
+
+        Each subclass must implement this.
+        """
+        raise NotImplementedError(
+            f"{type(self).__name__} must implement `_hash_components`, so we "
+            "can tell whether two storages save to the same place."
+        )
+
+    def __hash__(self):
+        return hash(self._hash_components())
+
     def __eq__(self, value):
         if not isinstance(value, MediaStorage):
             return False
+        # We compare hashes rather than the components themselves, so a
+        # custom backend written before `_hash_components` existed - which
+        # would have defined `__hash__` instead - still works.
         return value.__hash__() == self.__hash__()