Merge pull request #1790 from python-gitlab/jlvillal/parent_attrs

fix(cli): url-encode path components of the URL

gitlab/v4/cli.py

@@ -39,6 +39,7 @@ def __init__(
         self.action = action.lower()
         self.gl = gl
         self.args = args
+        self.parent_args: Dict[str, Any] = {}
         self.mgr_cls: Union[
             Type[gitlab.mixins.CreateMixin],
             Type[gitlab.mixins.DeleteMixin],
@@ -53,7 +54,10 @@ def __init__(
         # the class _path attribute, and replace the value with the result.
         if TYPE_CHECKING:
             assert self.mgr_cls._path is not None
-        self.mgr_cls._path = self.mgr_cls._path.format(**self.args)
+
+        self._process_from_parent_attrs()
+
+        self.mgr_cls._path = self.mgr_cls._path.format(**self.parent_args)
         self.mgr = self.mgr_cls(gl)
 
         if self.mgr_cls._types:
@@ -63,6 +67,18 @@ def __init__(
                     obj.set_from_cli(self.args[attr_name])
                     self.args[attr_name] = obj.get()
 
+    def _process_from_parent_attrs(self) -> None:
+        """Items in the path need to be url-encoded. There is a 1:1 mapping from
+        mgr_cls._from_parent_attrs <--> mgr_cls._path. Those values must be url-encoded
+        as they may contain a slash '/'."""
+        for key in self.mgr_cls._from_parent_attrs:
+            if key not in self.args:
+                continue
+
+            self.parent_args[key] = gitlab.utils.clean_str_id(self.args[key])
+            # If we don't delete it then it will be added to the URL as a query-string
+            del self.args[key]
+
     def __call__(self) -> Any:
         # Check for a method that matches object + action
         method = f"do_{self.what}_{self.action}"
@@ -85,7 +101,7 @@ def do_custom(self) -> Any:
             data = {}
             if self.mgr._from_parent_attrs:
                 for k in self.mgr._from_parent_attrs:
-                    data[k] = self.args[k]
+                    data[k] = self.parent_args[k]
             if not issubclass(self.cls, gitlab.mixins.GetWithoutIdMixin):
                 if TYPE_CHECKING:
                     assert isinstance(self.cls._id_attr, str)

tests/functional/cli/conftest.py

@@ -1,4 +1,7 @@
 import pytest
+import responses
+
+from gitlab.const import DEFAULT_URL
 
 
 @pytest.fixture
@@ -19,3 +22,14 @@ def _gitlab_cli(subcommands):
         return script_runner.run(*command)
 
     return _gitlab_cli
+
+
+@pytest.fixture
+def resp_get_project():
+    return {
+        "method": responses.GET,
+        "url": f"{DEFAULT_URL}/api/v4/projects/1",
+        "json": {"name": "name", "path": "test-path", "id": 1},
+        "content_type": "application/json",
+        "status": 200,
+    }

tests/functional/cli/test_cli.py

@@ -17,17 +17,6 @@
 CI_SERVER_URL = "https://gitlab.example.com"
 
 
-@pytest.fixture
-def resp_get_project():
-    return {
-        "method": responses.GET,
-        "url": f"{DEFAULT_URL}/api/v4/projects/1",
-        "json": {"name": "name", "path": "test-path", "id": 1},
-        "content_type": "application/json",
-        "status": 200,
-    }
-
-
 def test_main_entrypoint(script_runner, gitlab_config):
     ret = script_runner.run("python", "-m", "gitlab", "--config-file", gitlab_config)
     assert ret.returncode == 2
@@ -42,11 +31,8 @@ def test_version(script_runner):
 @responses.activate
 def test_defaults_to_gitlab_com(script_runner, resp_get_project, monkeypatch):
     responses.add(**resp_get_project)
-    with monkeypatch.context() as m:
-        # Ensure we don't pick up any config files that may already exist in the local
-        # environment.
-        m.setattr(config, "_DEFAULT_FILES", [])
-        ret = script_runner.run("gitlab", "project", "get", "--id", "1")
+    monkeypatch.setattr(config, "_DEFAULT_FILES", [])
+    ret = script_runner.run("gitlab", "project", "get", "--id", "1")
     assert ret.success
     assert "id: 1" in ret.stdout
 
@@ -55,6 +41,7 @@ def test_defaults_to_gitlab_com(script_runner, resp_get_project, monkeypatch):
 @responses.activate
 def test_uses_ci_server_url(monkeypatch, script_runner, resp_get_project):
     monkeypatch.setenv("CI_SERVER_URL", CI_SERVER_URL)
+    monkeypatch.setattr(config, "_DEFAULT_FILES", [])
     resp_get_project_in_ci = copy.deepcopy(resp_get_project)
     resp_get_project_in_ci.update(url=f"{CI_SERVER_URL}/api/v4/projects/1")
 
@@ -67,6 +54,7 @@ def test_uses_ci_server_url(monkeypatch, script_runner, resp_get_project):
 @responses.activate
 def test_uses_ci_job_token(monkeypatch, script_runner, resp_get_project):
     monkeypatch.setenv("CI_JOB_TOKEN", CI_JOB_TOKEN)
+    monkeypatch.setattr(config, "_DEFAULT_FILES", [])
     resp_get_project_in_ci = copy.deepcopy(resp_get_project)
     resp_get_project_in_ci.update(
         match=[responses.matchers.header_matcher({"JOB-TOKEN": CI_JOB_TOKEN})],

tests/functional/cli/test_cli_variables.py

@@ -1,3 +1,12 @@
+import copy
+
+import pytest
+import responses
+
+from gitlab import config
+from gitlab.const import DEFAULT_URL
+
+
 def test_list_instance_variables(gitlab_cli, gl):
     cmd = ["variable", "list"]
     ret = gitlab_cli(cmd)
@@ -17,3 +26,29 @@ def test_list_project_variables(gitlab_cli, project):
     ret = gitlab_cli(cmd)
 
     assert ret.success
+
+
+def test_list_project_variables_with_path(gitlab_cli, project):
+    cmd = ["project-variable", "list", "--project-id", project.path_with_namespace]
+    ret = gitlab_cli(cmd)
+
+    assert ret.success
+
+
+@pytest.mark.script_launch_mode("inprocess")
+@responses.activate
+def test_list_project_variables_with_path_url_check(
+    monkeypatch, script_runner, resp_get_project
+):
+    monkeypatch.setattr(config, "_DEFAULT_FILES", [])
+    resp_get_project_variables = copy.deepcopy(resp_get_project)
+    resp_get_project_variables.update(
+        url=f"{DEFAULT_URL}/api/v4/projects/project%2Fwith%2Fa%2Fnamespace/variables"
+    )
+    resp_get_project_variables.update(json=[])
+
+    responses.add(**resp_get_project_variables)
+    ret = script_runner.run(
+        "gitlab", "project-variable", "list", "--project-id", "project/with/a/namespace"
+    )
+    assert ret.success