make IFileStorage instances individually namable (#3416)

Author: ewdurbin

tests/unit/packaging/test_init.py

@@ -13,6 +13,8 @@
 import pretend
 import pytest
 
+from functools import partial
+
 from celery.schedules import crontab
 
 from warehouse import packaging
@@ -24,7 +26,9 @@
 
 @pytest.mark.parametrize("with_trending", [True, False])
 def test_includme(monkeypatch, with_trending):
-    storage_class = pretend.stub(create_service=pretend.stub())
+    storage_class = pretend.stub(
+        create_service=pretend.call_recorder(lambda *a, **kw: pretend.stub())
+    )
 
     def key_factory(keystring, iterate_on=None):
         return pretend.call(keystring, iterate_on=iterate_on)
@@ -50,10 +54,18 @@ def key_factory(keystring, iterate_on=None):
 
     packaging.includeme(config)
 
-    assert config.register_service_factory.calls == [
-        pretend.call(storage_class.create_service, IFileStorage, name='files'),
-        pretend.call(storage_class.create_service, IFileStorage, name='docs'),
-    ]
+    assert repr(config.register_service_factory.calls[0]) == repr(
+        pretend.call(
+            partial(storage_class.create_service, name='files'),
+            IFileStorage, name='files'
+        )
+    )
+    assert repr(config.register_service_factory.calls[1]) == repr(
+        pretend.call(
+            partial(storage_class.create_service, name='docs'),
+            IFileStorage, name='docs'
+        )
+    )
     assert config.register_origin_cache_keys.calls == [
         pretend.call(
             File,

tests/unit/packaging/test_services.py

@@ -39,9 +39,18 @@ def test_create_service(self):
                 settings={"files.path": "/the/one/two/"},
             ),
         )
-        storage = LocalFileStorage.create_service(None, request)
+        storage = LocalFileStorage.create_service(None, request, name='files')
         assert storage.base == "/the/one/two/"
 
+    def test_create_service_no_name(self):
+        request = pretend.stub(
+            registry=pretend.stub(
+                settings={"files.path": "/the/one/two/"},
+            ),
+        )
+        with pytest.raises(ValueError):
+            LocalFileStorage.create_service(None, request)
+
     def test_gets_file(self, tmpdir):
         with open(str(tmpdir.join("file.txt")), "wb") as fp:
             fp.write(b"my test file contents")
@@ -152,11 +161,20 @@ def test_create_service(self):
             find_service=pretend.call_recorder(lambda name: session),
             registry=pretend.stub(settings={"files.bucket": "froblob"}),
         )
-        storage = S3FileStorage.create_service(None, request)
+        storage = S3FileStorage.create_service(None, request, name='files')
 
         assert request.find_service.calls == [pretend.call(name="aws.session")]
         assert storage.bucket.name == "froblob"
 
+    def test_create_service_without_name(self):
+        session = boto3.session.Session()
+        request = pretend.stub(
+            find_service=pretend.call_recorder(lambda name: session),
+            registry=pretend.stub(settings={"files.bucket": "froblob"}),
+        )
+        with pytest.raises(ValueError):
+            S3FileStorage.create_service(None, request)
+
     def test_gets_file(self):
         s3key = pretend.stub(get=lambda: {"Body": io.BytesIO(b"my contents")})
         bucket = pretend.stub(Object=pretend.call_recorder(lambda path: s3key))

warehouse/packaging/__init__.py

@@ -10,6 +10,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from functools import partial
+
 from celery.schedules import crontab
 from sqlalchemy.orm.base import NO_VALUE
 
@@ -40,14 +42,16 @@ def includeme(config):
         config.registry.settings["files.backend"],
     )
     config.register_service_factory(
-        files_storage_class.create_service, IFileStorage, name='files'
+        partial(files_storage_class.create_service, name='files'),
+        IFileStorage, name='files'
     )
 
     docs_storage_class = config.maybe_dotted(
         config.registry.settings["docs.backend"],
     )
     config.register_service_factory(
-        docs_storage_class.create_service, IFileStorage, name='docs'
+        partial(docs_storage_class.create_service, name='docs'),
+        IFileStorage, name='docs'
     )
 
     # Register our origin cache keys

warehouse/packaging/interfaces.py

@@ -15,10 +15,10 @@
 
 class IFileStorage(Interface):
 
-    def create_service(context, request):
+    def create_service(context, request, name=None):
         """
         Create the service, given the context and request for which it is being
-        created for.
+        created for, passing a name for settings.
         """
 
     def get(path):

warehouse/packaging/services.py

@@ -39,8 +39,10 @@ def __init__(self, base):
         self.base = base
 
     @classmethod
-    def create_service(cls, context, request):
-        return cls(request.registry.settings["files.path"])
+    def create_service(cls, context, request, name=None):
+        if name is None:
+            raise ValueError('name is required')
+        return cls(request.registry.settings[f"{name}.path"])
 
     def get(self, path):
         return open(os.path.join(self.base, path), "rb")
@@ -69,12 +71,14 @@ def __init__(self, s3_client, bucket, *, prefix=None):
         self.prefix = prefix
 
     @classmethod
-    def create_service(cls, context, request):
+    def create_service(cls, context, request, name=None):
+        if name is None:
+            raise ValueError('name is required')
         session = request.find_service(name="aws.session")
         s3_client = session.client("s3")
         s3 = session.resource("s3")
-        bucket = s3.Bucket(request.registry.settings["files.bucket"])
-        prefix = request.registry.settings.get("files.prefix")
+        bucket = s3.Bucket(request.registry.settings[f"{name}.bucket"])
+        prefix = request.registry.settings.get(f"{name}.prefix")
         return cls(s3_client, bucket, prefix=prefix)
 
     def _get_path(self, path):