refactor: Storage constructor accepts a single dictionary with settings

ckanext/files/base.py

@@ -113,14 +113,24 @@ class MultipartData(BaseData[model.Multipart]):
     location: str = ""
 
 
-def make_storage(name: str, settings: dict[str, Any]) -> Storage:
+def make_storage(
+    name: str,
+    settings: dict[str, Any],
+    prepare_settings: bool = False,
+) -> Storage:
     """Initialize storage instance with specified settings.
 
     Storage adapter is defined by `type` key of the settings. All other
     settings depend on the specific adapter.
 
+    It's recommended to enable `prepare_settings` flag. When it's enabled, all
+    standard parameters(max_size, supported_types) are added to settings if
+    they are missing. But default this flag is disabled, because storages
+    usually initialized using CKAN configuration, which is already validated by
+    config declarations.
+
     Example:
-    >>> storage = make_storage("memo", {"type": "files:redis"})
+    >>> storage = make_storage("memo", {"type": "files:redis"}, True)
     """
 
     adapter_type = settings.pop("type", None)
@@ -129,7 +139,11 @@ def make_storage(name: str, settings: dict[str, Any]) -> Storage:
         raise exceptions.UnknownAdapterError(adapter_type)
 
     settings.setdefault("name", name)
-    return adapter(**settings)
+
+    if prepare_settings:
+        settings = adapter.prepare_settings(settings)
+
+    return adapter(settings)
 
 
 def get_storage(name: str | None = None) -> Storage:
@@ -143,7 +157,6 @@ def get_storage(name: str | None = None) -> Storage:
     Example:
     >>> default_storage = get_storage()
     >>> storage = get_storage("storage name")
-
     """
 
     if name is None:
@@ -207,6 +220,10 @@ def ensure_settings(self):
 class Uploader(StorageService):
     """Service responsible for writing data into a storage.
 
+    `Storage` internally calls methods of this service. For example,
+    `Storage.upload(location, upload, **kwargs)` results in
+    `Uploader.upload(location, upload, kwargs)`.
+
     Example:
     >>> class MyUploader(Uploader):
     >>>     def upload(
@@ -222,6 +239,7 @@ class Uploader(StorageService):
     >>>             upload.content_type,
     >>>             reader.get_hash()
     >>>         )
+
     """
 
     def upload(
@@ -273,13 +291,17 @@ def multipart_complete(
 class Manager(StorageService):
     """Service responsible for maintenance file operations.
 
+    `Storage` internally calls methods of this service. For example,
+    `Storage.remove(data, **kwargs)` results in `Manager.remove(data, kwargs)`.
+
     Example:
     >>> class MyManager(Manager):
     >>>     def remove(
     >>>         self, data: FileData|MultipartData, extras: dict[str, Any]
     >>>     ) -> bool:
     >>>         os.remove(data.location)
     >>>         return True
+
     """
 
     def remove(self, data: FileData | MultipartData, extras: dict[str, Any]) -> bool:
@@ -340,12 +362,16 @@ def analyze(self, location: str, extras: dict[str, Any]) -> FileData:
 class Reader(StorageService):
     """Service responsible for reading data from the storage.
 
+    `Storage` internally calls methods of this service. For example,
+    `Storage.stream(data, **kwargs)` results in `Reader.stream(data, kwargs)`.
+
     Example:
     >>> class MyReader(Reader):
     >>>     def stream(
     >>>         self, data: FileData, extras: dict[str, Any]
     >>>     ) -> Iterable[bytes]:
     >>>         return open(data.location, "rb")
+
     """
 
     def stream(self, data: FileData, extras: dict[str, Any]) -> Iterable[bytes]:
@@ -420,17 +446,42 @@ class Storage(OptionChecker, abc.ABC):
     >>>         return MyManager(self)
     """
 
+    # do not show storage adapter in CLI's `files adapters` output
     hidden = False
+
+    # operations that storage performs. Will be overriden by capabilities of
+    # services inside constructor.
     capabilities = utils.Capability.NONE
 
     def __str__(self):
         return self.settings.get("name", "unknown")
 
-    def __init__(self, **settings: Any):
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
+        """Add all required items to settings.
+
+        This is usually done by config declarations. But when storage is
+        initialized manually, via `make_storage`, settings are not validated.
+
+        Use this method to transform arbitrary dictionary into expected form of
+        settings. Don't do too much work, just adding missing parameters should
+        be enough.
+
+        Passing sane values with valid types is still responsibility of the
+        developer.
+
+        Example:
+        >>> settings = Storage.prepare_settings({})
+        >>> storage = Storage(settings)
+        """
+
         settings.setdefault("override_existing", False)
         settings.setdefault("supported_types", [])
         settings.setdefault("max_size", 0)
 
+        return settings
+
+    def __init__(self, settings: dict[str, Any], /):
         self.settings = settings
 
         self.uploader = self.make_uploader()

ckanext/files/storage/db.py

@@ -23,7 +23,7 @@ def declare_config_options(
         declaration.declare(key.location_column).required()
         declaration.declare(key.content_column).required()
 
-    def __init__(self, **settings: Any):
+    def __init__(self, settings: Any):
         db_url = self.ensure_option(settings, "db_url")
 
         self.engine = sa.create_engine(db_url)
@@ -37,7 +37,7 @@ def __init__(self, **settings: Any):
             self.content_column,
         )
 
-        super().__init__(**settings)
+        super().__init__(settings)
 
     def make_reader(self):
         return DbReader(self)

ckanext/files/storage/filebin.py

@@ -15,9 +15,10 @@
 class FilebinStorage(shared.Storage):
     hidden = True
 
-    def __init__(self, **settings: Any):
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
         settings.setdefault("timeout", 10)
-        super().__init__(**settings)
+        return super().prepare_settings(settings)
 
     @property
     def bin(self):

ckanext/files/storage/fs.py

@@ -39,21 +39,26 @@ def make_reader(self):
     def make_manager(self):
         return FsManager(self)
 
-    def __init__(self, **settings: Any):
-        path = self.ensure_option(settings, "path")
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
         settings.setdefault("create_path", False)
         settings.setdefault("recursive", False)
 
+        return super().prepare_settings(settings)
+
+    def __init__(self, settings: Any):
+        path = self.ensure_option(settings, "path")
+
         if not os.path.exists(path):
-            if tk.asbool(settings["create_path"]):
+            if tk.asbool(self.ensure_option(settings, "create_path")):
                 os.makedirs(path)
             else:
                 raise exceptions.InvalidStorageConfigurationError(
                     type(self),
                     f"path `{path}` does not exist",
                 )
 
-        super().__init__(**settings)
+        super().__init__(settings)
 
     @classmethod
     def declare_config_options(cls, declaration: Declaration, key: Key):
@@ -384,6 +389,6 @@ def make_reader(self):
 
 
 class CkanResourceFsStorage(FsStorage):
-    def __init__(self, **settings: Any):
+    def __init__(self, settings: Any):
         settings["recursive"] = True
-        super().__init__(**settings)
+        super().__init__(settings)

ckanext/files/storage/google_cloud.py

@@ -280,24 +280,29 @@ def remove(self, data: FileData | MultipartData, extras: dict[str, Any]) -> bool
 class GoogleCloudStorage(Storage):
     hidden = True
 
-    def __init__(self, **settings: Any):
-        settings["path"] = settings.setdefault("path", "").lstrip("/")
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
+        settings.setdefault("path", "")
         settings.setdefault("resumable_origin", tk.config["ckan.site_url"])
+        settings.get("credentials_file")
+        return super().prepare_settings(settings)
 
+    def __init__(self, settings: Any):
+        settings["path"] = self.ensure_option(settings, "path").lstrip("/")
         credentials = None
-        credentials_file = settings.get("credentials_file", None)
-        if credentials_file:
+
+        if cf := self.ensure_option(settings, "credentials_file"):
             try:
-                credentials = Credentials.from_service_account_file(credentials_file)
+                credentials = Credentials.from_service_account_file(cf)
             except OSError as err:
                 raise exceptions.InvalidStorageConfigurationError(
                     type(self),
-                    f"file `{credentials_file}` does not exist",
+                    f"file `{cf}` does not exist",
                 ) from err
 
         self.client = Client(credentials=credentials)
 
-        super().__init__(**settings)
+        super().__init__(settings)
 
     def make_uploader(self):
         return GoogleCloudUploader(self)

ckanext/files/storage/libcloud.py

@@ -23,12 +23,18 @@ class LibCloudStorage(shared.Storage):
     driver: StorageDriver
     container: Container
 
-    def __init__(self, **settings: Any):
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
+        settings.setdefault("secret", None)
+        settings.setdefault("params", {})
+        return super().prepare_settings(settings)
+
+    def __init__(self, settings: Any):
         provider = self.ensure_option(settings, "provider")
         key = self.ensure_option(settings, "key")
         container = self.ensure_option(settings, "container")
-        secret = settings.setdefault("secret", None)
-        params = settings.setdefault("params", {})
+        secret = self.ensure_option(settings, "secret")
+        params = self.ensure_option(settings, "params")
 
         try:
             factory = get_driver(DriverType.STORAGE, provider)
@@ -51,7 +57,7 @@ def __init__(self, **settings: Any):
                 str(err),
             ) from err
 
-        super().__init__(**settings)
+        super().__init__(settings)
 
     def make_uploader(self):
         return LibCloudUploader(self)

ckanext/files/storage/link.py

@@ -23,9 +23,10 @@ def make_uploader(self):
     def make_manager(self):
         return LinkManager(self)
 
-    def __init__(self, **settings: Any):
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
         settings.setdefault("timeout", 5)
-        super().__init__(**settings)
+        return super().prepare_settings(settings)
 
     @classmethod
     def declare_config_options(

ckanext/files/storage/opendal.py

@@ -19,9 +19,14 @@ def make_reader(self):
     def make_manager(self):
         return OpenDalManager(self)
 
-    def __init__(self, **settings: Any):
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
+        settings.setdefault("params", {})
+        return super().prepare_settings(settings)
+
+    def __init__(self, settings: Any):
         scheme = self.ensure_option(settings, "scheme")
-        params = settings.setdefault("params", {})
+        params = self.ensure_option(settings, "params")
 
         try:
             self.operator = opendal.Operator(scheme, **params)
@@ -31,7 +36,7 @@ def __init__(self, **settings: Any):
                 str(err),
             ) from err
 
-        super().__init__(**settings)
+        super().__init__(settings)
 
     def compute_capabilities(self) -> shared.Capability:
         cluster = super().compute_capabilities()

ckanext/files/storage/redis.py

@@ -274,13 +274,14 @@ def make_manager(self):
     def make_reader(self):
         return RedisReader(self)
 
-    def __init__(self, **settings: Any):
-        settings.setdefault(
-            "prefix",
-            _default_prefix(),
-        )
+    @classmethod
+    def prepare_settings(cls, settings: dict[str, Any]):
+        settings.setdefault("prefix", _default_prefix())
+        return super().prepare_settings(settings)
+
+    def __init__(self, settings: Any):
         self.redis: redis.Redis = connect_to_redis()
-        super().__init__(**settings)
+        super().__init__(settings)
 
     @classmethod
     def declare_config_options(cls, declaration: Declaration, key: Key):

ckanext/files/templates/files/snippets/file_table.html

@@ -93,12 +93,14 @@
                                 {% endif %}
 
 
-                                <a class="btn btn-danger btn-sm" aria-label="Remove the file"
-                                    href="{{ h.url_for('files.delete_file',
-                                              file_id=file.id,
-                                              came_from=request.path, owner_type=owner_type, owner_id=owner_id) }}">
-                                    <i class="fa fa-remove"></i>
-                                </a>
+                                {% if h.check_access("files_file_download", {"id": file.id}) %}
+                                    <a class="btn btn-danger btn-sm" aria-label="Remove the file"
+                                        href="{{ h.url_for('files.delete_file',
+                                                  file_id=file.id,
+                                                  came_from=request.path, owner_type=owner_type, owner_id=owner_id) }}">
+                                        <i class="fa fa-remove"></i>
+                                    </a>
+                                {% endif %}
 
                             {% endblock %}
                         </td>

ckanext/files/tests/storage/test_fs.py

@@ -15,7 +15,9 @@
 
 @pytest.fixture()
 def storage(tmp_path: Path):
-    return fs.FsStorage(name="test", path=str(tmp_path))
+    return fs.FsStorage(
+        fs.FsStorage.prepare_settings({"name": "test", "path": str(tmp_path)}),
+    )
 
 
 class TestUploader:
@@ -190,11 +192,15 @@ def test_missing(self, storage: fs.FsStorage, faker: Faker):
 class TestStorage:
     def test_missing_path(self, tmp_path: Path):
         with pytest.raises(exceptions.InvalidStorageConfigurationError):
-            fs.FsStorage(path=os.path.join(str(tmp_path), "not-real"))
+            fs.FsStorage(
+                fs.FsStorage.prepare_settings(
+                    {"path": os.path.join(str(tmp_path), "not-real")},
+                ),
+            )
 
     def test_missing_path_created(self, tmp_path: Path):
         path = os.path.join(str(tmp_path), "not-real")
         assert not os.path.exists(path)
 
-        fs.FsStorage(path=path, create_path=True)
+        fs.FsStorage(fs.FsStorage.prepare_settings({"path": path, "create_path": True}))
         assert os.path.exists(path)