Fix collectstatic manifest file handling & revert jschneier#955 (jsch…

…neier#968)

* Revert "S3: Workaround boto bug to fix collectstatic issue (jschneier#955)"

This reverts commit 7771782.

* Add separate manifest files handling

docs/backends/amazon-S3.rst

@@ -22,7 +22,11 @@ To upload your media files to S3 set::
 
 To allow ``django-admin collectstatic`` to automatically put your static files in your bucket set the following in your settings.py::
 
-    STATICFILES_STORAGE = 'storages.backends.s3boto3.S3Boto3Storage'
+    STATICFILES_STORAGE = 'storages.backends.s3boto3.S3StaticStorage'
+
+If you want to use something like `ManifestStaticFilesStorage`_ then you must instead use::
+
+    STATICFILES_STORAGE = 'storages.backends.s3boto3.S3ManifestStaticStorage'
 
 ``AWS_ACCESS_KEY_ID``
     Your Amazon Web Services access key, as a string.
@@ -120,6 +124,7 @@ To allow ``django-admin collectstatic`` to automatically put your static files i
 .. _S3 region list: http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region
 .. _list of canned ACLs: https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl
 .. _Boto3 docs for uploading files: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.put_object
+.. _ManifestStaticFilesStorage: https://docs.djangoproject.com/en/3.1/ref/contrib/staticfiles/#manifeststaticfilesstorage
 
 .. _migrating-boto-to-boto3:
 

storages/backends/s3boto3.py

@@ -2,12 +2,14 @@
 import mimetypes
 import os
 import posixpath
+import tempfile
 import threading
 from datetime import datetime, timedelta
 from gzip import GzipFile
 from tempfile import SpooledTemporaryFile
 from urllib.parse import parse_qsl, urlsplit
 
+from django.contrib.staticfiles.storage import ManifestFilesMixin
 from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
 from django.core.files.base import File
 from django.utils.deconstruct import deconstructible
@@ -16,8 +18,8 @@
 
 from storages.base import BaseStorage
 from storages.utils import (
-    NonCloseableBufferedReader, check_location, get_available_overwrite_name,
-    lookup_env, safe_join, setting, to_bytes,
+    check_location, get_available_overwrite_name, lookup_env, safe_join,
+    setting, to_bytes,
 )
 
 try:
@@ -442,8 +444,7 @@ def _save(self, name, content):
 
         obj = self.bucket.Object(name)
         content.seek(0, os.SEEK_SET)
-        with NonCloseableBufferedReader(content) as reader:
-            obj.upload_fileobj(reader, ExtraArgs=params)
+        obj.upload_fileobj(content, ExtraArgs=params)
         return cleaned_name
 
     def delete(self, name):
@@ -582,3 +583,22 @@ def get_available_name(self, name, max_length=None):
         if self.file_overwrite:
             return get_available_overwrite_name(name, max_length)
         return super().get_available_name(name, max_length)
+
+
+class S3StaticStorage(S3Boto3Storage):
+    """Querystring auth must be disabled so that url() returns a consistent output."""
+    querystring_auth = False
+
+
+class S3ManifestStaticStorage(ManifestFilesMixin, S3StaticStorage):
+    """Copy the file before saving for compatibility with ManifestFilesMixin
+    which does not play nicely with boto3 automatically closing the file.
+
+    See: https://github.com/boto/s3transfer/issues/80#issuecomment-562356142
+    """
+
+    def _save(self, name, content):
+        content.seek(0)
+        with tempfile.SpooledTemporaryFile() as tmp:
+            tmp.write(content.read())
+            return super()._save(name, tmp)

storages/utils.py

@@ -1,4 +1,3 @@
-import io
 import os
 import posixpath
 
@@ -127,10 +126,3 @@ def get_available_overwrite_name(name, max_length):
             'allows sufficient "max_length".' % name
         )
     return os.path.join(dir_name, "{}{}".format(file_root, file_ext))
-
-
-# A buffered reader that does not actually close, workaround for
-# https://github.com/boto/s3transfer/issues/80#issuecomment-562356142
-class NonCloseableBufferedReader(io.BufferedReader):
-    def close(self):
-        self.flush()

tests/test_s3boto3.py

@@ -22,12 +22,6 @@ def setUp(self):
         self.storage._connections.connection = mock.MagicMock()
 
 
-def assert_called_upload(mock_uploadobj, content, extra):
-    assert mock_uploadobj.call_count == 1
-    assert mock_uploadobj.call_args[0][0].raw == content
-    assert mock_uploadobj.call_args[1] == extra
-
-
 class S3Boto3StorageTests(S3Boto3TestCase):
 
     def test_clean_name(self):
@@ -113,12 +107,12 @@ def test_storage_save(self):
         self.storage.bucket.Object.assert_called_once_with(name)
 
         obj = self.storage.bucket.Object.return_value
-        extra = {
-            'ExtraArgs': {
+        obj.upload_fileobj.assert_called_with(
+            content,
+            ExtraArgs={
                 'ContentType': 'text/plain',
             }
-        }
-        assert_called_upload(obj.upload_fileobj, content, extra)
+        )
 
     def test_storage_save_with_default_acl(self):
         """
@@ -131,13 +125,13 @@ def test_storage_save_with_default_acl(self):
         self.storage.bucket.Object.assert_called_once_with(name)
 
         obj = self.storage.bucket.Object.return_value
-        extra = {
-            'ExtraArgs': {
+        obj.upload_fileobj.assert_called_with(
+            content,
+            ExtraArgs={
                 'ContentType': 'text/plain',
                 'ACL': 'private',
             }
-        }
-        assert_called_upload(obj.upload_fileobj, content, extra)
+        )
 
     def test_storage_object_parameters_not_overwritten_by_default(self):
         """
@@ -151,13 +145,13 @@ def test_storage_object_parameters_not_overwritten_by_default(self):
         self.storage.bucket.Object.assert_called_once_with(name)
 
         obj = self.storage.bucket.Object.return_value
-        extra = {
-            'ExtraArgs': {
+        obj.upload_fileobj.assert_called_with(
+            content,
+            ExtraArgs={
                 'ContentType': 'text/plain',
                 'ACL': 'private',
             }
-        }
-        assert_called_upload(obj.upload_fileobj, content, extra)
+        )
 
     def test_content_type(self):
         """
@@ -170,12 +164,12 @@ def test_content_type(self):
         self.storage.bucket.Object.assert_called_once_with(name)
 
         obj = self.storage.bucket.Object.return_value
-        extra = {
-            'ExtraArgs': {
+        obj.upload_fileobj.assert_called_with(
+            content,
+            ExtraArgs={
                 'ContentType': 'image/jpeg',
             }
-        }
-        assert_called_upload(obj.upload_fileobj, content, extra)
+        )
 
     def test_storage_save_gzipped(self):
         """
@@ -185,13 +179,13 @@ def test_storage_save_gzipped(self):
         content = ContentFile("I am gzip'd")
         self.storage.save(name, content)
         obj = self.storage.bucket.Object.return_value
-        extra = {
-            'ExtraArgs': {
+        obj.upload_fileobj.assert_called_with(
+            content,
+            ExtraArgs={
                 'ContentType': 'application/octet-stream',
                 'ContentEncoding': 'gzip',
             }
-        }
-        assert_called_upload(obj.upload_fileobj, content, extra)
+        )
 
     def test_storage_save_gzip(self):
         """