chore: add EncodedId string class to use to hold URL-encoded paths

JohnVillalovos · JohnVillalovos · commit af3f5aef5e69 · 2022-01-09T22:30:49.000-08:00
Add EncodedId string class. This class returns a URL-encoded string
but ensures it will only URL-encode it once even if recursively
called.

Also added some functional tests of 'lazy' objects to make sure they
work.
diff --git a/gitlab/mixins.py b/gitlab/mixins.py
@@ -542,8 +542,7 @@ def save(self, **kwargs: Any) -> None:
             return
 
         # call the manager
-        # Don't use `self.encoded_id` here as `self.manager.update()` will encode it.
-        obj_id = self.get_id()
+        obj_id = self.encoded_id
         if TYPE_CHECKING:
             assert isinstance(self.manager, UpdateMixin)
         server_data = self.manager.update(obj_id, updated_data, **kwargs)
@@ -573,8 +572,7 @@ def delete(self, **kwargs: Any) -> None:
         """
         if TYPE_CHECKING:
             assert isinstance(self.manager, DeleteMixin)
-        # Don't use `self.encoded_id` here as `self.manager.delete()` will encode it.
-        self.manager.delete(self.get_id(), **kwargs)
+        self.manager.delete(self.encoded_id, **kwargs)
 
 
 class UserAgentDetailMixin(_RestObjectBase):
diff --git a/gitlab/utils.py b/gitlab/utils.py
@@ -56,17 +56,88 @@ def copy_dict(dest: Dict[str, Any], src: Dict[str, Any]) -> None:
             dest[k] = v
 
 
+class EncodedId(str):
+    """A custom `str` class that will return the URL-encoded value of the string.
+
+    Features:
+      * Using it recursively will only url-encode the value once.
+      * Can accept either `str` or `int` as input value.
+      * Can be used in an f-string and output the URL-encoded string.
+
+    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.
+
+
+    When creating an EncodedId instance `__new__` will be called first and then
+    `__init__`.
+    """
+
+    # `original_str` will contain the original string value that was used to create the
+    # first instance of EncodedId. We will use this original value to generate the
+    # URL-encoded value each time.
+    original_str: str
+
+    def __new__(cls, value: Union[str, int, "EncodedId"]) -> "EncodedId":
+        if isinstance(value, int):
+            value = str(value)
+        # Make sure isinstance() for `EncodedId` comes before check for `str` as
+        # `EncodedId` is an instance of `str` and would pass that check.
+        elif isinstance(value, EncodedId):
+            # We use the original string value to URL-encode
+            value = value.original_str
+        elif isinstance(value, str):
+            pass
+        else:
+            raise ValueError(f"Unsupported type received: {type(value)}")
+        # Set the value our string will return
+        value = urllib.parse.quote(value, safe="")
+        return super().__new__(cls, value)
+
+    def __init__(self, value: Union[int, str]) -> None:
+        # At this point `super().__str__()` returns the URL-encoded value. Which means
+        # when using this as a `str` it will return the URL-encoded value.
+        #
+        # But `value` contains the original value passed in `EncodedId(value)`. We use
+        # this to always keep the original string that was received so that no matter
+        # how many times we recurse we only URL-encode our original string once.
+        if isinstance(value, int):
+            value = str(value)
+        # Make sure isinstance() for `EncodedId` comes before check for `str` as
+        # `EncodedId` is an instance of `str` and would pass that check.
+        elif isinstance(value, EncodedId):
+            # This is the key part as we are always keeping the original string even
+            # through multiple recursions.
+            value = value.original_str
+        elif isinstance(value, str):
+            pass
+        else:
+            raise ValueError(f"Unsupported type received: {type(value)}")
+        self.original_str = value
+        super().__init__()
+
+
 @overload
 def _url_encode(id: int) -> int:
     ...
 
 
 @overload
-def _url_encode(id: str) -> str:
+def _url_encode(id: Union[str, EncodedId]) -> EncodedId:
     ...
 
 
-def _url_encode(id: Union[int, str]) -> Union[int, str]:
+def _url_encode(id: Union[int, str, EncodedId]) -> Union[int, EncodedId]:
     """Encode/quote the characters in the string so that they can be used in a path.
 
     Reference to documentation on why this is necessary.
@@ -84,9 +155,9 @@ def _url_encode(id: Union[int, str]) -> Union[int, str]:
     parameters.
 
     """
-    if isinstance(id, int):
+    if isinstance(id, (int, EncodedId)):
         return id
-    return urllib.parse.quote(id, safe="")
+    return EncodedId(id)
 
 
 def remove_none_from_dict(data: Dict[str, Any]) -> Dict[str, Any]:
diff --git a/gitlab/v4/objects/merge_request_approvals.py b/gitlab/v4/objects/merge_request_approvals.py
@@ -75,7 +75,7 @@ def set_approvers(
 
         if TYPE_CHECKING:
             assert self._parent is not None
-        path = f"/projects/{self._parent.get_id()}/approvers"
+        path = f"/projects/{self._parent.encoded_id}/approvers"
         data = {"approver_ids": approver_ids, "approver_group_ids": approver_group_ids}
         result = self.gitlab.http_put(path, post_data=data, **kwargs)
         if TYPE_CHECKING:
diff --git a/tests/functional/api/test_groups.py b/tests/functional/api/test_groups.py
@@ -100,6 +100,7 @@ def test_groups(gl):
     member = group1.members.get(user2.id)
     assert member.access_level == gitlab.const.OWNER_ACCESS
 
+    gl.auth()
     group2.members.delete(gl.user.id)
 
 
@@ -198,6 +199,11 @@ def test_group_subgroups_projects(gl, user):
     assert gr1_project.namespace["id"] == group1.id
     assert gr2_project.namespace["parent_id"] == group1.id
 
+    gr1_project.delete()
+    gr2_project.delete()
+    group3.delete()
+    group4.delete()
+
 
 @pytest.mark.skip
 def test_group_wiki(group):
diff --git a/tests/functional/api/test_lazy_objects.py b/tests/functional/api/test_lazy_objects.py
@@ -0,0 +1,39 @@
+import pytest
+
+import gitlab
+
+
+@pytest.fixture
+def lazy_project(gl, project):
+    assert "/" in project.path_with_namespace
+    return gl.projects.get(project.path_with_namespace, lazy=True)
+
+
+def test_lazy_id(project, lazy_project):
+    assert isinstance(lazy_project.id, str)
+    assert isinstance(lazy_project.id, gitlab.utils.EncodedId)
+    assert lazy_project.id == gitlab.utils._url_encode(project.path_with_namespace)
+
+
+def test_refresh_after_lazy_get_with_path(project, lazy_project):
+    lazy_project.refresh()
+    assert lazy_project.id == project.id
+
+
+def test_save_after_lazy_get_with_path(project, lazy_project):
+    lazy_project.description = "A new description"
+    lazy_project.save()
+    assert lazy_project.id == project.id
+    assert lazy_project.description == "A new description"
+
+
+def test_delete_after_lazy_get_with_path(gl, group, wait_for_sidekiq):
+    project = gl.projects.create({"name": "lazy_project", "namespace_id": group.id})
+    result = wait_for_sidekiq(timeout=60)
+    assert result is True, "sidekiq process should have terminated but did not"
+    lazy_project = gl.projects.get(project.path_with_namespace, lazy=True)
+    lazy_project.delete()
+
+
+def test_method_call(gl, lazy_project):
+    lazy_project.mergerequests.list()
diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
@@ -406,7 +406,8 @@ def user(gl):
     yield user
 
     try:
-        user.delete()
+        # Use `hard_delete=True` or a 'Ghost User' may be created.
+        user.delete(hard_delete=True)
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"User already deleted: {e}")
 
diff --git a/tests/unit/test_base.py b/tests/unit/test_base.py
@@ -158,6 +158,10 @@ def test_encoded_id(self, fake_manager):
         obj.id = "a/path"
         assert "a%2Fpath" == obj.encoded_id
 
+        # If you assign it again it does not double URL-encode
+        obj.id = obj.encoded_id
+        assert "a%2Fpath" == obj.encoded_id
+
     def test_custom_id_attr(self, fake_manager):
         class OtherFakeObject(FakeObject):
             _id_attr = "foo"
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
@@ -15,6 +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 json
+
 from gitlab import utils
 
 
@@ -35,3 +37,63 @@ def test_url_encode():
     src = "docs/README.md"
     dest = "docs%2FREADME.md"
     assert dest == utils._url_encode(src)
+
+
+class TestEncodedId:
+    def test_init_str(self):
+        obj = utils.EncodedId("Hello")
+        assert "Hello" == str(obj)
+        assert "Hello" == f"{obj}"
+
+        obj = utils.EncodedId("this/is a/path")
+        assert "this%2Fis%20a%2Fpath" == str(obj)
+        assert "this%2Fis%20a%2Fpath" == f"{obj}"
+
+    def test_init_int(self):
+        obj = utils.EncodedId(23)
+        assert "23" == str(obj)
+        assert "23" == f"{obj}"
+
+    def test_init_encodeid_str(self):
+        value = "Goodbye"
+        obj_init = utils.EncodedId(value)
+        obj = utils.EncodedId(obj_init)
+        assert value == str(obj)
+        assert value == f"{obj}"
+        assert value == obj.original_str
+
+        value = "we got/a/path"
+        expected = "we%20got%2Fa%2Fpath"
+        obj_init = utils.EncodedId(value)
+        assert value == obj_init.original_str
+        assert expected == str(obj_init)
+        assert expected == f"{obj_init}"
+        # Show that no matter how many times we recursively call it we still only
+        # URL-encode it once.
+        obj = utils.EncodedId(
+            utils.EncodedId(utils.EncodedId(utils.EncodedId(utils.EncodedId(obj_init))))
+        )
+        assert expected == str(obj)
+        assert expected == f"{obj}"
+        # We have stored a copy of our original string
+        assert value == obj.original_str
+
+        # Show assignments still only encode once
+        obj2 = obj
+        assert expected == str(obj2)
+        assert expected == f"{obj2}"
+
+    def test_init_encodeid_int(self):
+        value = 23
+        expected = f"{value}"
+        obj_init = utils.EncodedId(value)
+        obj = utils.EncodedId(obj_init)
+        assert expected == str(obj)
+        assert expected == f"{obj}"
+
+    def test_json_serializable(self):
+        obj = utils.EncodedId("someone")
+        assert '"someone"' == json.dumps(obj)
+
+        obj = utils.EncodedId("we got/a/path")
+        assert '"we%20got%2Fa%2Fpath"' == json.dumps(obj)
