Skip to content

refactor: Replacing http_requests return type hint #2435

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

refactor: Replacing http_requests return type hint #2435

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
10 changes: 5 additions & 5 deletions docs/api-levels.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ Lower-lower-level API - HTTP requests
higher-level APIs. To lessen the chances of a change to the interface impacting your code, we
recommend using keyword arguments when calling the interfaces.

At the lowest level, HTTP methods call ``http_request()``, which performs the actual request and takes
At the lowest level, HTTP methods call ``backend_request()``, which performs the actual request and takes
care of details such as timeouts, retries, and handling rate-limits.

This method can be invoked directly to or customize this behavior for a single request, or to call custom
Expand All @@ -87,19 +87,19 @@ For example, if for whatever reason you want to fetch allowed methods for an end

>>> gl = gitlab.Gitlab(private_token=private_token)
>>>
>>> response = gl.http_request(verb="OPTIONS", path="/projects")
>>> response.headers["Allow"]
>>> backend_response = gl.backend_request(verb="OPTIONS", path="/projects")
>>> backend_response.headers["Allow"]
'OPTIONS, GET, POST, HEAD'

Or get the total number of a user's events with a customized HEAD request:

.. code-block:: python

>>> response = gl.http_request(
>>> backend_response = gl.backend_request(
verb="HEAD",
path="/events",
query_params={"sudo": "some-user"},
timeout=10
)
>>> response.headers["X-Total"]
>>> backend_response.headers["X-Total"]
'123'
121 changes: 89 additions & 32 deletions gitlab/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ def _check_redirects(result: requests.Response) -> None:
)
)

def http_request(
def backend_request(
self,
verb: str,
path: str,
Expand All @@ -653,7 +653,7 @@ def http_request(
retry_transient_errors: Optional[bool] = None,
max_retries: int = 10,
**kwargs: Any,
) -> requests.Response:
) -> _backends.DefaultResponse:
"""Make an HTTP request to the Gitlab server.

Args:
Expand Down Expand Up @@ -722,7 +722,7 @@ def http_request(
cur_retries = 0
while True:
try:
result = self._backend.http_request(
backend_response = self._backend.http_request(
method=verb,
url=url,
json=send_data.json,
Expand All @@ -744,20 +744,26 @@ def http_request(

raise

self._check_redirects(result.response)
self._check_redirects(backend_response.response)

if 200 <= result.status_code < 300:
return result.response
if 200 <= backend_response.status_code < 300:
return backend_response

def should_retry() -> bool:
if result.status_code == 429 and obey_rate_limit:
if backend_response.status_code == 429 and obey_rate_limit:
return True

if not retry_transient_errors:
return False
if result.status_code in gitlab.const.RETRYABLE_TRANSIENT_ERROR_CODES:
if (
backend_response.status_code
in gitlab.const.RETRYABLE_TRANSIENT_ERROR_CODES
):
return True
if result.status_code == 409 and "Resource lock" in result.reason:
if (
backend_response.status_code == 409
and "Resource lock" in backend_response.reason
):
return True

return False
Expand All @@ -767,36 +773,74 @@ def should_retry() -> bool:
# https://docs.gitlab.com/ee/user/admin_area/settings/user_and_ip_rate_limits.html#response-headers
if max_retries == -1 or cur_retries < max_retries:
wait_time = 2**cur_retries * 0.1
if "Retry-After" in result.headers:
wait_time = int(result.headers["Retry-After"])
elif "RateLimit-Reset" in result.headers:
wait_time = int(result.headers["RateLimit-Reset"]) - time.time()
if "Retry-After" in backend_response.headers:
wait_time = int(backend_response.headers["Retry-After"])
elif "RateLimit-Reset" in backend_response.headers:
wait_time = (
int(backend_response.headers["RateLimit-Reset"])
- time.time()
)
cur_retries += 1
time.sleep(wait_time)
continue

error_message = result.content
error_message = backend_response.content
try:
error_json = result.json()
error_json = backend_response.json()
for k in ("message", "error"):
if k in error_json:
error_message = error_json[k]
except (KeyError, ValueError, TypeError):
pass

if result.status_code == 401:
if backend_response.status_code == 401:
raise gitlab.exceptions.GitlabAuthenticationError(
response_code=result.status_code,
response_code=backend_response.status_code,
error_message=error_message,
response_body=result.content,
response_body=backend_response.content,
)

raise gitlab.exceptions.GitlabHttpError(
response_code=result.status_code,
response_code=backend_response.status_code,
error_message=error_message,
response_body=result.content,
response_body=backend_response.content,
)

def http_request(
self,
verb: str,
path: str,
query_data: Optional[Dict[str, Any]] = None,
post_data: Optional[Union[Dict[str, Any], bytes]] = None,
raw: bool = False,
streamed: bool = False,
files: Optional[Dict[str, Any]] = None,
timeout: Optional[float] = None,
obey_rate_limit: bool = True,
retry_transient_errors: Optional[bool] = None,
max_retries: int = 10,
**kwargs: Any,
) -> requests.Response:
utils.warn(
"`http_request()` is deprecated and will be removed in a future version.\n"
"Please use `backend_request()` instead.",
category=DeprecationWarning,
)
Comment on lines +824 to +828
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit wary of this, it would feel wrong deprecating this after we already had a similar discussion in #1842. What would be the issue with having a http_request in the public client?

Keeping in mind that the user does not care about the backends generally.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, so this would need to be at least communicated to users via a breaking change trailer (and triger 4.0.0).

I've thought it would make sense using this method and not to trigger 4.0.0 for now.

Copy link
Member

@nejch nejch Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lmilbaum what I meant was what would be wrong with simply keeping http_backend on the client, while having backend-specific implementations in the backends package?

This way it's quite consistent with the existing http_* methods (there is also some history in the other MR, where we discussed we'd keep it in the public client as it is).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following. http_backend is kept on the client while having backend-specific implementation in the backend package.
I was just trying to address the valid concern you have raised regarding the breaking change.

return self.backend_request(
verb,
path,
query_data,
post_data,
raw,
streamed,
files,
timeout,
obey_rate_limit,
retry_transient_errors,
max_retries,
**kwargs,
).response

def http_get(
self,
path: str,
Expand Down Expand Up @@ -825,10 +869,13 @@ def http_get(
GitlabParsingError: If the json data could not be parsed
"""
query_data = query_data or {}
result = self.http_request(
backend_response = self.backend_request(
"get", path, query_data=query_data, streamed=streamed, **kwargs
)
content_type = utils.get_content_type(result.headers.get("Content-Type"))
content_type = utils.get_content_type(
backend_response.headers.get("Content-Type")
)
result = backend_response.response

if content_type == "application/json" and not streamed and not raw:
try:
Expand Down Expand Up @@ -861,8 +908,10 @@ def http_head(
"""

query_data = query_data or {}
result = self.http_request("head", path, query_data=query_data, **kwargs)
return result.headers
backend_response = self.http_request(
"head", path, query_data=query_data, **kwargs
)
return backend_response.headers

def http_list(
self,
Expand Down Expand Up @@ -1018,7 +1067,7 @@ def http_post(
query_data = query_data or {}
post_data = post_data or {}

result = self.http_request(
backend_response = self.backend_request(
"post",
path,
query_data=query_data,
Expand All @@ -1027,7 +1076,10 @@ def http_post(
raw=raw,
**kwargs,
)
content_type = utils.get_content_type(result.headers.get("Content-Type"))
content_type = utils.get_content_type(
backend_response.headers.get("Content-Type")
)
result = backend_response.response

try:
if content_type == "application/json":
Expand Down Expand Up @@ -1072,7 +1124,7 @@ def http_put(
query_data = query_data or {}
post_data = post_data or {}

result = self.http_request(
backend_response = self.http_request(
"put",
path,
query_data=query_data,
Expand All @@ -1082,7 +1134,7 @@ def http_put(
**kwargs,
)
try:
json_result = result.json()
json_result = backend_response.json()
if TYPE_CHECKING:
assert isinstance(json_result, dict)
return json_result
Expand Down Expand Up @@ -1121,7 +1173,7 @@ def http_patch(
query_data = query_data or {}
post_data = post_data or {}

result = self.http_request(
backend_response = self.http_request(
"patch",
path,
query_data=query_data,
Expand All @@ -1130,7 +1182,7 @@ def http_patch(
**kwargs,
)
try:
json_result = result.json()
json_result = backend_response.json()
if TYPE_CHECKING:
assert isinstance(json_result, dict)
return json_result
Expand All @@ -1153,7 +1205,8 @@ def http_delete(self, path: str, **kwargs: Any) -> requests.Response:
Raises:
GitlabHttpError: When the return code is not 2xx
"""
return self.http_request("delete", path, **kwargs)
backend_response = self.backend_request("delete", path, **kwargs)
return backend_response.response

@gitlab.exceptions.on_http_error(gitlab.exceptions.GitlabSearchError)
def search(
Expand Down Expand Up @@ -1207,7 +1260,11 @@ def _query(
self, url: str, query_data: Optional[Dict[str, Any]] = None, **kwargs: Any
) -> None:
query_data = query_data or {}
result = self._gl.http_request("get", url, query_data=query_data, **kwargs)
backend_response = self._gl.backend_request(
"get", url, query_data=query_data, **kwargs
)
result = backend_response.response

try:
next_url = result.links["next"]["url"]
except KeyError:
Expand Down