Skip to content

fix: use url-encoded ID in all paths (alternate method) #1820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: use url-encoded ID in all paths (alternate method) #1820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions gitlab/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,15 @@ def get_id(self) -> Any:
return None
return getattr(self, self._id_attr)

@property
def encoded_id(self) -> Any:
"""Ensure that the ID is url-encoded so that it can be safely used in a URL
path"""
obj_id = self.get_id()
if isinstance(obj_id, str):
obj_id = gitlab.utils.EncodedId(obj_id)
return obj_id

@property
def attributes(self) -> Dict[str, Any]:
d = self.__dict__["_updated_attrs"].copy()
Expand Down
42 changes: 19 additions & 23 deletions gitlab/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,7 @@ def get(
GitlabAuthenticationError: If authentication is not correct
GitlabGetError: If the server cannot perform the request
"""
if not isinstance(id, int):
id = utils._url_encode(id)
path = f"{self.path}/{id}"
path = f"{self.path}/{utils.EncodedId(id)}"
if TYPE_CHECKING:
assert self._obj_cls is not None
if lazy is True:
Expand Down Expand Up @@ -173,7 +171,7 @@ def refresh(self, **kwargs: Any) -> None:
GitlabGetError: If the server cannot perform the request
"""
if self._id_attr:
path = f"{self.manager.path}/{self.id}"
path = f"{self.manager.path}/{self.encoded_id}"
else:
if TYPE_CHECKING:
assert self.manager.path is not None
Expand Down Expand Up @@ -391,7 +389,7 @@ def update(
if id is None:
path = self.path
else:
path = f"{self.path}/{id}"
path = f"{self.path}/{utils.EncodedId(id)}"

self._check_missing_update_attrs(new_data)
files = {}
Expand Down Expand Up @@ -444,7 +442,7 @@ def set(self, key: str, value: str, **kwargs: Any) -> base.RESTObject:
Returns:
The created/updated attribute
"""
path = f"{self.path}/{utils._url_encode(key)}"
path = f"{self.path}/{utils.EncodedId(key)}"
data = {"value": value}
server_data = self.gitlab.http_put(path, post_data=data, **kwargs)
if TYPE_CHECKING:
Expand Down Expand Up @@ -477,9 +475,7 @@ def delete(self, id: Union[str, int], **kwargs: Any) -> None:
if id is None:
path = self.path
else:
if not isinstance(id, int):
id = utils._url_encode(id)
path = f"{self.path}/{id}"
path = f"{self.path}/{utils.EncodedId(id)}"
self.gitlab.http_delete(path, **kwargs)


Expand Down Expand Up @@ -545,7 +541,7 @@ def save(self, **kwargs: Any) -> None:
return

# call the manager
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)
Expand Down Expand Up @@ -575,7 +571,7 @@ def delete(self, **kwargs: Any) -> None:
"""
if TYPE_CHECKING:
assert isinstance(self.manager, DeleteMixin)
self.manager.delete(self.get_id(), **kwargs)
self.manager.delete(self.encoded_id, **kwargs)


class UserAgentDetailMixin(_RestObjectBase):
Expand All @@ -598,7 +594,7 @@ def user_agent_detail(self, **kwargs: Any) -> Dict[str, Any]:
GitlabAuthenticationError: If authentication is not correct
GitlabGetError: If the server cannot perform the request
"""
path = f"{self.manager.path}/{self.get_id()}/user_agent_detail"
path = f"{self.manager.path}/{self.encoded_id}/user_agent_detail"
result = self.manager.gitlab.http_get(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(result, requests.Response)
Expand Down Expand Up @@ -631,7 +627,7 @@ def approve(
GitlabUpdateError: If the server fails to perform the request
"""

path = f"{self.manager.path}/{self.id}/approve"
path = f"{self.manager.path}/{self.encoded_id}/approve"
data = {"access_level": access_level}
server_data = self.manager.gitlab.http_put(path, post_data=data, **kwargs)
if TYPE_CHECKING:
Expand Down Expand Up @@ -705,7 +701,7 @@ def subscribe(self, **kwargs: Any) -> None:
GitlabAuthenticationError: If authentication is not correct
GitlabSubscribeError: If the subscription cannot be done
"""
path = f"{self.manager.path}/{self.get_id()}/subscribe"
path = f"{self.manager.path}/{self.encoded_id}/subscribe"
server_data = self.manager.gitlab.http_post(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(server_data, requests.Response)
Expand All @@ -725,7 +721,7 @@ def unsubscribe(self, **kwargs: Any) -> None:
GitlabAuthenticationError: If authentication is not correct
GitlabUnsubscribeError: If the unsubscription cannot be done
"""
path = f"{self.manager.path}/{self.get_id()}/unsubscribe"
path = f"{self.manager.path}/{self.encoded_id}/unsubscribe"
server_data = self.manager.gitlab.http_post(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(server_data, requests.Response)
Expand All @@ -752,7 +748,7 @@ def todo(self, **kwargs: Any) -> None:
GitlabAuthenticationError: If authentication is not correct
GitlabTodoError: If the todo cannot be set
"""
path = f"{self.manager.path}/{self.get_id()}/todo"
path = f"{self.manager.path}/{self.encoded_id}/todo"
self.manager.gitlab.http_post(path, **kwargs)


Expand Down Expand Up @@ -781,7 +777,7 @@ def time_stats(self, **kwargs: Any) -> Dict[str, Any]:
if "time_stats" in self.attributes:
return self.attributes["time_stats"]

path = f"{self.manager.path}/{self.get_id()}/time_stats"
path = f"{self.manager.path}/{self.encoded_id}/time_stats"
result = self.manager.gitlab.http_get(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(result, requests.Response)
Expand All @@ -800,7 +796,7 @@ def time_estimate(self, duration: str, **kwargs: Any) -> Dict[str, Any]:
GitlabAuthenticationError: If authentication is not correct
GitlabTimeTrackingError: If the time tracking update cannot be done
"""
path = f"{self.manager.path}/{self.get_id()}/time_estimate"
path = f"{self.manager.path}/{self.encoded_id}/time_estimate"
data = {"duration": duration}
result = self.manager.gitlab.http_post(path, post_data=data, **kwargs)
if TYPE_CHECKING:
Expand All @@ -819,7 +815,7 @@ def reset_time_estimate(self, **kwargs: Any) -> Dict[str, Any]:
GitlabAuthenticationError: If authentication is not correct
GitlabTimeTrackingError: If the time tracking update cannot be done
"""
path = f"{self.manager.path}/{self.get_id()}/reset_time_estimate"
path = f"{self.manager.path}/{self.encoded_id}/reset_time_estimate"
result = self.manager.gitlab.http_post(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(result, requests.Response)
Expand All @@ -838,7 +834,7 @@ def add_spent_time(self, duration: str, **kwargs: Any) -> Dict[str, Any]:
GitlabAuthenticationError: If authentication is not correct
GitlabTimeTrackingError: If the time tracking update cannot be done
"""
path = f"{self.manager.path}/{self.get_id()}/add_spent_time"
path = f"{self.manager.path}/{self.encoded_id}/add_spent_time"
data = {"duration": duration}
result = self.manager.gitlab.http_post(path, post_data=data, **kwargs)
if TYPE_CHECKING:
Expand All @@ -857,7 +853,7 @@ def reset_spent_time(self, **kwargs: Any) -> Dict[str, Any]:
GitlabAuthenticationError: If authentication is not correct
GitlabTimeTrackingError: If the time tracking update cannot be done
"""
path = f"{self.manager.path}/{self.get_id()}/reset_spent_time"
path = f"{self.manager.path}/{self.encoded_id}/reset_spent_time"
result = self.manager.gitlab.http_post(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(result, requests.Response)
Expand Down Expand Up @@ -893,7 +889,7 @@ def participants(self, **kwargs: Any) -> Dict[str, Any]:
The list of participants
"""

path = f"{self.manager.path}/{self.get_id()}/participants"
path = f"{self.manager.path}/{self.encoded_id}/participants"
result = self.manager.gitlab.http_get(path, **kwargs)
if TYPE_CHECKING:
assert not isinstance(result, requests.Response)
Expand Down Expand Up @@ -967,7 +963,7 @@ def promote(self, **kwargs: Any) -> Dict[str, Any]:
The updated object data (*not* a RESTObject)
"""

path = f"{self.manager.path}/{self.id}/promote"
path = f"{self.manager.path}/{self.encoded_id}/promote"
http_method = self._get_update_method()
result = http_method(path, **kwargs)
if TYPE_CHECKING:
Expand Down
63 changes: 54 additions & 9 deletions gitlab/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.

import urllib.parse
from typing import Any, Callable, Dict, Optional
from typing import Any, Callable, Dict, Optional, Union

import requests

Expand Down Expand Up @@ -56,8 +56,13 @@ def copy_dict(dest: Dict[str, Any], src: Dict[str, Any]) -> None:
dest[k] = v


def _url_encode(id: str) -> str:
"""Encode/quote the characters in the string so that they can be used in a path.
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.

Expand All @@ -69,12 +74,52 @@ def _url_encode(id: str) -> str:
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="")
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."""

# `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 __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__()

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 remove_none_from_dict(data: Dict[str, Any]) -> Dict[str, Any]:
Expand Down
2 changes: 1 addition & 1 deletion gitlab/v4/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def _process_from_parent_attrs(self) -> None:
if key not in self.args:
continue

self.parent_args[key] = gitlab.utils._url_encode(self.args[key])
self.parent_args[key] = gitlab.utils.EncodedId(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]

Expand Down
12 changes: 6 additions & 6 deletions gitlab/v4/objects/commits.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def diff(self, **kwargs: Any) -> Union[gitlab.GitlabList, List[Dict[str, Any]]]:
Returns:
The changes done in this commit
"""
path = f"{self.manager.path}/{self.get_id()}/diff"
path = f"{self.manager.path}/{self.encoded_id}/diff"
return self.manager.gitlab.http_list(path, **kwargs)

@cli.register_custom_action("ProjectCommit", ("branch",))
Expand All @@ -58,7 +58,7 @@ def cherry_pick(self, branch: str, **kwargs: Any) -> None:
GitlabAuthenticationError: If authentication is not correct
GitlabCherryPickError: If the cherry-pick could not be performed
"""
path = f"{self.manager.path}/{self.get_id()}/cherry_pick"
path = f"{self.manager.path}/{self.encoded_id}/cherry_pick"
post_data = {"branch": branch}
self.manager.gitlab.http_post(path, post_data=post_data, **kwargs)

Expand All @@ -80,7 +80,7 @@ def refs(
Returns:
The references the commit is pushed to.
"""
path = f"{self.manager.path}/{self.get_id()}/refs"
path = f"{self.manager.path}/{self.encoded_id}/refs"
query_data = {"type": type}
return self.manager.gitlab.http_list(path, query_data=query_data, **kwargs)

Expand All @@ -101,7 +101,7 @@ def merge_requests(
Returns:
The merge requests related to the commit.
"""
path = f"{self.manager.path}/{self.get_id()}/merge_requests"
path = f"{self.manager.path}/{self.encoded_id}/merge_requests"
return self.manager.gitlab.http_list(path, **kwargs)

@cli.register_custom_action("ProjectCommit", ("branch",))
Expand All @@ -122,7 +122,7 @@ def revert(
Returns:
The new commit data (*not* a RESTObject)
"""
path = f"{self.manager.path}/{self.get_id()}/revert"
path = f"{self.manager.path}/{self.encoded_id}/revert"
post_data = {"branch": branch}
return self.manager.gitlab.http_post(path, post_data=post_data, **kwargs)

Expand All @@ -141,7 +141,7 @@ def signature(self, **kwargs: Any) -> Union[Dict[str, Any], requests.Response]:
Returns:
The commit's signature data
"""
path = f"{self.manager.path}/{self.get_id()}/signature"
path = f"{self.manager.path}/{self.encoded_id}/signature"
return self.manager.gitlab.http_get(path, **kwargs)


Expand Down
2 changes: 1 addition & 1 deletion gitlab/v4/objects/environments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def stop(self, **kwargs: Any) -> Union[Dict[str, Any], requests.Response]:
Returns:
A dict of the result.
"""
path = f"{self.manager.path}/{self.get_id()}/stop"
path = f"{self.manager.path}/{self.encoded_id}/stop"
return self.manager.gitlab.http_post(path, **kwargs)


Expand Down
2 changes: 1 addition & 1 deletion gitlab/v4/objects/epics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def save(self, **kwargs: Any) -> None:
return

# call the manager
obj_id = self.get_id()
obj_id = self.encoded_id
self.manager.update(obj_id, updated_data, **kwargs)


Expand Down
3 changes: 1 addition & 2 deletions gitlab/v4/objects/features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ def set(
Returns:
The created/updated attribute
"""
name = utils._url_encode(name)
path = f"{self.path}/{name}"
path = f"{self.path}/{utils.EncodedId(name)}"
data = {
"value": value,
"feature_group": feature_group,
Expand Down
Loading