Merge pull request #1816 from python-gitlab/jlvillal/remove_replace

fix: remove custom URL encoding

8 files changed, 50 insertions, 22 deletions

diff --git a/gitlab/mixins.py b/gitlab/mixins.py
index 1abffa1..c02f4c0 100644
--- a/gitlab/mixins.py
+++ b/gitlab/mixins.py
@@ -100,7 +100,7 @@ class GetMixin(_RestManagerBase):
             GitlabGetError: If the server cannot perform the request
         """
         if not isinstance(id, int):
-            id = utils.clean_str_id(id)
+            id = utils._url_encode(id)
         path = f"{self.path}/{id}"
         if TYPE_CHECKING:
             assert self._obj_cls is not None
@@ -444,7 +444,7 @@ class SetMixin(_RestManagerBase):
         Returns:
             The created/updated attribute
         """
-        path = f"{self.path}/{utils.clean_str_id(key)}"
+        path = f"{self.path}/{utils._url_encode(key)}"
         data = {"value": value}
         server_data = self.gitlab.http_put(path, post_data=data, **kwargs)
         if TYPE_CHECKING:
@@ -478,7 +478,7 @@ class DeleteMixin(_RestManagerBase):
             path = self.path
         else:
             if not isinstance(id, int):
-                id = utils.clean_str_id(id)
+                id = utils._url_encode(id)
             path = f"{self.path}/{id}"
         self.gitlab.http_delete(path, **kwargs)
 
diff --git a/gitlab/utils.py b/gitlab/utils.py
index a1dcb45..1f29104 100644
--- a/gitlab/utils.py
+++ b/gitlab/utils.py
@@ -15,8 +15,8 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import urllib.parse
 from typing import Any, Callable, Dict, Optional
-from urllib.parse import quote
 
 import requests
 
@@ -56,8 +56,25 @@ def copy_dict(dest: Dict[str, Any], src: Dict[str, Any]) -> None:
             dest[k] = v
 
 
-def clean_str_id(id: str) -> str:
-    return quote(id, safe="")
+def _url_encode(id: str) -> str:
+    """Encode/quote the characters in the string so that they can be used in a path.
+
+    Reference to documentation on why this is necessary.
+
+    https://docs.gitlab.com/ee/api/index.html#namespaced-path-encoding
+
+    If using namespaced API requests, make sure that the NAMESPACE/PROJECT_PATH is
+    URL-encoded. For example, / is represented by %2F
+
+    https://docs.gitlab.com/ee/api/index.html#path-parameters
+
+    Path parameters that are required to be URL-encoded must be followed. If not, it
+    doesn’t match an API endpoint and responds with a 404. If there’s something in front
+    of the API (for example, Apache), ensure that it doesn’t decode the URL-encoded path
+    parameters.
+
+    """
+    return urllib.parse.quote(id, safe="")
 
 
 def remove_none_from_dict(data: Dict[str, Any]) -> Dict[str, Any]:
diff --git a/gitlab/v4/cli.py b/gitlab/v4/cli.py
index 5b276ae..a76b133 100644
--- a/gitlab/v4/cli.py
+++ b/gitlab/v4/cli.py
@@ -75,7 +75,7 @@ class GitlabCLI(object):
             if key not in self.args:
                 continue
 
-            self.parent_args[key] = gitlab.utils.clean_str_id(self.args[key])
+            self.parent_args[key] = gitlab.utils._url_encode(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]
 
diff --git a/gitlab/v4/objects/features.py b/gitlab/v4/objects/features.py
index 2e92596..69689fa 100644
--- a/gitlab/v4/objects/features.py
+++ b/gitlab/v4/objects/features.py
@@ -52,7 +52,8 @@ class FeatureManager(ListMixin, DeleteMixin, RESTManager):
         Returns:
             The created/updated attribute
         """
-        path = f"{self.path}/{name.replace('/', '%2F')}"
+        name = utils._url_encode(name)
+        path = f"{self.path}/{name}"
         data = {
             "value": value,
             "feature_group": feature_group,
diff --git a/gitlab/v4/objects/files.py b/gitlab/v4/objects/files.py
index c3a8ec8..64046f9 100644
--- a/gitlab/v4/objects/files.py
+++ b/gitlab/v4/objects/files.py
@@ -56,7 +56,7 @@ class ProjectFile(SaveMixin, ObjectDeleteMixin, RESTObject):
         """
         self.branch = branch
         self.commit_message = commit_message
-        self.file_path = self.file_path.replace("/", "%2F")
+        self.file_path = utils._url_encode(self.file_path)
         super(ProjectFile, self).save(**kwargs)
 
     @exc.on_http_error(exc.GitlabDeleteError)
@@ -76,7 +76,7 @@ class ProjectFile(SaveMixin, ObjectDeleteMixin, RESTObject):
             GitlabAuthenticationError: If authentication is not correct
             GitlabDeleteError: If the server cannot perform the request
         """
-        file_path = self.get_id().replace("/", "%2F")
+        file_path = utils._url_encode(self.get_id())
         self.manager.delete(file_path, branch, commit_message, **kwargs)
 
 
@@ -144,7 +144,7 @@ class ProjectFileManager(GetMixin, CreateMixin, UpdateMixin, DeleteMixin, RESTMa
             assert data is not None
         self._check_missing_create_attrs(data)
         new_data = data.copy()
-        file_path = new_data.pop("file_path").replace("/", "%2F")
+        file_path = utils._url_encode(new_data.pop("file_path"))
         path = f"{self.path}/{file_path}"
         server_data = self.gitlab.http_post(path, post_data=new_data, **kwargs)
         if TYPE_CHECKING:
@@ -173,7 +173,7 @@ class ProjectFileManager(GetMixin, CreateMixin, UpdateMixin, DeleteMixin, RESTMa
         """
         new_data = new_data or {}
         data = new_data.copy()
-        file_path = file_path.replace("/", "%2F")
+        file_path = utils._url_encode(file_path)
         data["file_path"] = file_path
         path = f"{self.path}/{file_path}"
         self._check_missing_update_attrs(data)
@@ -203,7 +203,8 @@ class ProjectFileManager(GetMixin, CreateMixin, UpdateMixin, DeleteMixin, RESTMa
             GitlabAuthenticationError: If authentication is not correct
             GitlabDeleteError: If the server cannot perform the request
         """
-        path = f"{self.path}/{file_path.replace('/', '%2F')}"
+        file_path = utils._url_encode(file_path)
+        path = f"{self.path}/{file_path}"
         data = {"branch": branch, "commit_message": commit_message}
         self.gitlab.http_delete(path, query_data=data, **kwargs)
 
@@ -238,7 +239,7 @@ class ProjectFileManager(GetMixin, CreateMixin, UpdateMixin, DeleteMixin, RESTMa
         Returns:
             The file content
         """
-        file_path = file_path.replace("/", "%2F").replace(".", "%2E")
+        file_path = utils._url_encode(file_path)
         path = f"{self.path}/{file_path}/raw"
         query_data = {"ref": ref}
         result = self.gitlab.http_get(
@@ -265,7 +266,7 @@ class ProjectFileManager(GetMixin, CreateMixin, UpdateMixin, DeleteMixin, RESTMa
         Returns:
             A list of commits/lines matching the file
         """
-        file_path = file_path.replace("/", "%2F").replace(".", "%2E")
+        file_path = utils._url_encode(file_path)
         path = f"{self.path}/{file_path}/blame"
         query_data = {"ref": ref}
         result = self.gitlab.http_list(path, query_data, **kwargs)
diff --git a/gitlab/v4/objects/repositories.py b/gitlab/v4/objects/repositories.py
index b520ab7..b52add3 100644
--- a/gitlab/v4/objects/repositories.py
+++ b/gitlab/v4/objects/repositories.py
@@ -39,7 +39,7 @@ class RepositoryMixin(_RestObjectBase):
             GitlabPutError: If the submodule could not be updated
         """
 
-        submodule = submodule.replace("/", "%2F")  # .replace('.', '%2E')
+        submodule = utils._url_encode(submodule)
         path = f"/projects/{self.get_id()}/repository/submodules/{submodule}"
         data = {"branch": branch, "commit_sha": commit_sha}
         if "commit_message" in kwargs:
diff --git a/tests/functional/api/test_repository.py b/tests/functional/api/test_repository.py
index f08a029..9f5d4be 100644
--- a/tests/functional/api/test_repository.py
+++ b/tests/functional/api/test_repository.py
@@ -1,4 +1,5 @@
 import base64
+import os
 import sys
 import tarfile
 import time
@@ -13,13 +14,13 @@ import gitlab
 def test_repository_files(project):
     project.files.create(
         {
-            "file_path": "README",
+            "file_path": "README.md",
             "branch": "main",
             "content": "Initial content",
             "commit_message": "Initial commit",
         }
     )
-    readme = project.files.get(file_path="README", ref="main")
+    readme = project.files.get(file_path="README.md", ref="main")
     readme.content = base64.b64encode(b"Improved README").decode()
 
     time.sleep(2)
@@ -42,6 +43,9 @@ def test_repository_files(project):
     blame = project.files.blame(file_path="README.rst", ref="main")
     assert blame
 
+    raw_file = project.files.raw(file_path="README.rst", ref="main")
+    assert os.fsdecode(raw_file) == "Initial content"
+
 
 def test_repository_tree(project):
     tree = project.repository_tree()
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index 706285e..edb545b 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -18,15 +18,20 @@
 from gitlab import utils
 
 
-def test_clean_str_id():
+def test_url_encode():
     src = "nothing_special"
     dest = "nothing_special"
-    assert dest == utils.clean_str_id(src)
+    assert dest == utils._url_encode(src)
 
     src = "foo#bar/baz/"
     dest = "foo%23bar%2Fbaz%2F"
-    assert dest == utils.clean_str_id(src)
+    assert dest == utils._url_encode(src)
 
     src = "foo%bar/baz/"
     dest = "foo%25bar%2Fbaz%2F"
-    assert dest == utils.clean_str_id(src)
+    assert dest == utils._url_encode(src)
+
+    # periods/dots should not be modified
+    src = "docs/README.md"
+    dest = "docs%2FREADME.md"
+    assert dest == utils._url_encode(src)