diff --git a/swh/objstorage/api/server.py b/swh/objstorage/api/server.py --- a/swh/objstorage/api/server.py +++ b/swh/objstorage/api/server.py @@ -175,8 +175,7 @@ # retro compatibility configuration settings app["config"] = config - _cfg = config["objstorage"] - app["objstorage"] = get_objstorage(_cfg["cls"], _cfg["args"]) + app["objstorage"] = get_objstorage(**config["objstorage"]) app.router.add_route("GET", "/", index) app.router.add_route("POST", "/check_config", check_config) @@ -237,20 +236,14 @@ missing_keys = [] vcfg = cfg["objstorage"] - for key in ("cls", "args"): - v = vcfg.get(key) - if v is None: - missing_keys.append(key) - - if missing_keys: - raise KeyError( - "Invalid configuration; missing %s config entry" - % (", ".join(missing_keys),) - ) - - cls = vcfg.get("cls") + if "cls" not in vcfg: + raise KeyError("Invalid configuration; missing cls config entry") + + cls = vcfg["cls"] if cls == "pathslicing": - args = vcfg["args"] + # Backwards-compatibility: either get the deprecated `args` from the + # objstorage config, or use the full config itself to check for keys + args = vcfg.get("args", vcfg) for key in ("root", "slicing"): v = args.get(key) if v is None: @@ -258,7 +251,7 @@ if missing_keys: raise KeyError( - "Invalid configuration; missing args.%s config entry" + "Invalid configuration; missing %s config entry" % (", ".join(missing_keys),) ) diff --git a/swh/objstorage/factory.py b/swh/objstorage/factory.py --- a/swh/objstorage/factory.py +++ b/swh/objstorage/factory.py @@ -4,6 +4,7 @@ # See top-level LICENSE file for more information from typing import Callable, Dict, Union +import warnings from swh.objstorage.api.client import RemoteObjStorage from swh.objstorage.backends.generator import RandomGeneratorObjStorage @@ -59,15 +60,15 @@ _STORAGE_CLASSES_MISSING["swift"] = e.args[0] -def get_objstorage(cls, args): +def get_objstorage(cls: str, args=None, **kwargs): """ Create an ObjStorage using the given implementation class. Args: - cls (str): objstorage class unique key contained in the + cls: objstorage class unique key contained in the _STORAGE_CLASSES dict. - args (dict): arguments for the required class of objstorage - that must match exactly the one in the `__init__` method of the - class. + kwargs: arguments for the required class of objstorage + that must match exactly the one in the `__init__` method of the + class. Returns: subclass of ObjStorage that match the given `storage_class` argument. Raises: @@ -75,7 +76,17 @@ key. """ if cls in _STORAGE_CLASSES: - return _STORAGE_CLASSES[cls](**args) + if args is not None: + warnings.warn( + 'Explicit "args" key is deprecated for objstorage initialization, ' + "use class arguments keys directly instead.", + DeprecationWarning, + ) + # TODO: when removing this, drop the "args" backwards compatibility + # from swh.objstorage.api.server configuration checker + kwargs = args + + return _STORAGE_CLASSES[cls](**kwargs) else: raise ValueError( "Storage class {} is not available: {}".format( diff --git a/swh/objstorage/tests/test_multiplexer_filter.py b/swh/objstorage/tests/test_multiplexer_filter.py --- a/swh/objstorage/tests/test_multiplexer_filter.py +++ b/swh/objstorage/tests/test_multiplexer_filter.py @@ -28,12 +28,13 @@ self.tmpdir = tempfile.mkdtemp() pstorage = { "cls": "pathslicing", - "args": {"root": self.tmpdir, "slicing": "0:5"}, + "root": self.tmpdir, + "slicing": "0:5", } base_storage = get_objstorage(**pstorage) base_storage.id = compute_hash self.storage = get_objstorage( - "filtered", {"storage_conf": pstorage, "filters_conf": [read_only()]} + "filtered", storage_conf=pstorage, filters_conf=[read_only()] ) self.valid_content = b"pre-existing content" self.invalid_content = b"invalid_content" diff --git a/swh/objstorage/tests/test_objstorage_api.py b/swh/objstorage/tests/test_objstorage_api.py --- a/swh/objstorage/tests/test_objstorage_api.py +++ b/swh/objstorage/tests/test_objstorage_api.py @@ -26,11 +26,9 @@ self.config = { "objstorage": { "cls": "pathslicing", - "args": { - "root": self.tmpdir, - "slicing": "0:1/0:5", - "allow_delete": True, - }, + "root": self.tmpdir, + "slicing": "0:1/0:5", + "allow_delete": True, }, "client_max_size": 8 * 1024 * 1024, } diff --git a/swh/objstorage/tests/test_server.py b/swh/objstorage/tests/test_server.py --- a/swh/objstorage/tests/test_server.py +++ b/swh/objstorage/tests/test_server.py @@ -59,17 +59,13 @@ def test_load_and_check_config_invalid_configuration(tmpdir): """Invalid configuration raises""" - for data, missing_keys in [ - ({"objstorage": {"something": "useless"}}, ["cls", "args"]), - ({"objstorage": {"cls": "something"}}, ["args"]), - ]: - config_path = prepare_config_file(tmpdir, content=data) - with pytest.raises(KeyError) as e: - load_and_check_config(config_path) + config_path = prepare_config_file( + tmpdir, content={"objstorage": {"something": "useless"}} + ) + with pytest.raises(KeyError) as e: + load_and_check_config(config_path) - assert e.value.args[0] == "Invalid configuration; missing %s config entry" % ( - ", ".join(missing_keys), - ) + assert "missing cls config entry" in e.value.args[0] def test_load_and_check_config_invalid_configuration_level2(tmpdir): @@ -88,10 +84,7 @@ with pytest.raises(KeyError) as e: load_and_check_config(config_path) - assert ( - e.value.args[0] - == "Invalid configuration; missing args.%s config entry" % key - ) + assert "missing %s config entry" % key in e.value.args[0] def test_load_and_check_config_fine(tmpdir): @@ -108,8 +101,26 @@ assert cfg == config +def test_load_and_check_config_no_args(tmpdir): + """pathslicing configuration with no args key loads ok""" + config = { + "objstorage": {"cls": "pathslicing", "root": "root", "slicing": "slicing"} + } + + config_path = prepare_config_file(tmpdir, config) + cfg = load_and_check_config(config_path) + assert cfg == config + + def test_load_and_check_config_fine2(tmpdir): - config = {"client_max_size": "10", "objstorage": {"cls": "remote", "args": {}}} + config = {"client_max_size": "10", "objstorage": {"cls": "memory", "args": {}}} + config_path = prepare_config_file(tmpdir, config) + cfg = load_and_check_config(config_path) + assert cfg == config + + +def test_load_and_check_config_only_cls(tmpdir): + config = {"client_max_size": "10", "objstorage": {"cls": "memory"}} config_path = prepare_config_file(tmpdir, config) cfg = load_and_check_config(config_path) assert cfg == config