From f4cdfa1459b3028e33539e7be9dfbfe456650ca1 Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sat, 4 Mar 2023 14:45:24 -0800 Subject: [PATCH 1/2] chore: use a dataclass to return values from `prepare_send_data` I found the tuple of three values confusing. So instead use a dataclass to return the three values. It is still confusing but a little bit less so. Also add some unit tests --- gitlab/_backends/requests_backend.py | 29 +++++++++----- gitlab/client.py | 10 ++--- tests/unit/_backends/__init__.py | 0 tests/unit/_backends/test_requests_backend.py | 38 +++++++++++++++++++ 4 files changed, 62 insertions(+), 15 deletions(-) create mode 100644 tests/unit/_backends/__init__.py create mode 100644 tests/unit/_backends/test_requests_backend.py diff --git a/gitlab/_backends/requests_backend.py b/gitlab/_backends/requests_backend.py index fd40baa64..d70cf4291 100644 --- a/gitlab/_backends/requests_backend.py +++ b/gitlab/_backends/requests_backend.py @@ -1,6 +1,7 @@ from __future__ import annotations -from typing import Any, Dict, Optional, Tuple, TYPE_CHECKING, Union +import dataclasses +from typing import Any, Dict, Optional, TYPE_CHECKING, Union import requests from requests.structures import CaseInsensitiveDict @@ -9,6 +10,20 @@ from . import protocol +@dataclasses.dataclass +class SendData: + content_type: str + data: Optional[Union[Dict[str, Any], MultipartEncoder]] = None + json: Optional[Union[Dict[str, Any], bytes]] = None + + def __post_init__(self) -> None: + if self.json is not None and self.data is not None: + raise ValueError( + f"`json` and `data` are mutually exclusive. Only one can be set. " + f"json={self.json!r} data={self.data!r}" + ) + + class RequestsResponse(protocol.BackendResponse): def __init__(self, response: requests.Response) -> None: self._response: requests.Response = response @@ -50,11 +65,7 @@ def prepare_send_data( files: Optional[Dict[str, Any]] = None, post_data: Optional[Union[Dict[str, Any], bytes]] = None, raw: bool = False, - ) -> Tuple[ - Optional[Union[Dict[str, Any], bytes]], - Optional[Union[Dict[str, Any], MultipartEncoder]], - str, - ]: + ) -> SendData: if files: if post_data is None: post_data = {} @@ -70,12 +81,12 @@ def prepare_send_data( post_data["avatar"] = files.get("avatar") data = MultipartEncoder(post_data) - return (None, data, data.content_type) + return SendData(data=data, content_type=data.content_type) if raw and post_data: - return (None, post_data, "application/octet-stream") + return SendData(data=post_data, content_type="application/octet-stream") - return (post_data, None, "application/json") + return SendData(json=post_data, content_type="application/json") def http_request( self, diff --git a/gitlab/client.py b/gitlab/client.py index c3982f39d..495ed4407 100644 --- a/gitlab/client.py +++ b/gitlab/client.py @@ -716,10 +716,8 @@ def http_request( retry_transient_errors = self.retry_transient_errors # We need to deal with json vs. data when uploading files - json, data, content_type = self._backend.prepare_send_data( - files, post_data, raw - ) - opts["headers"]["Content-type"] = content_type + send_data = self._backend.prepare_send_data(files, post_data, raw) + opts["headers"]["Content-type"] = send_data.content_type cur_retries = 0 while True: @@ -727,8 +725,8 @@ def http_request( result = self._backend.http_request( method=verb, url=url, - json=json, - data=data, + json=send_data.json, + data=send_data.data, params=params, timeout=timeout, verify=verify, diff --git a/tests/unit/_backends/__init__.py b/tests/unit/_backends/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/unit/_backends/test_requests_backend.py b/tests/unit/_backends/test_requests_backend.py new file mode 100644 index 000000000..d83703d2b --- /dev/null +++ b/tests/unit/_backends/test_requests_backend.py @@ -0,0 +1,38 @@ +import pytest +from requests_toolbelt.multipart.encoder import MultipartEncoder # type: ignore + +from gitlab._backends import requests_backend + + +class TestSendData: + def test_senddata_json(self) -> None: + result = requests_backend.SendData( + json={"a": 1}, content_type="application/json" + ) + assert result.data is None + + def test_senddata_data(self) -> None: + result = requests_backend.SendData( + data={"b": 2}, content_type="application/octet-stream" + ) + assert result.json is None + + def test_senddata_json_and_data(self) -> None: + with pytest.raises(ValueError, match=r"json={'a': 1} data={'b': 2}"): + requests_backend.SendData( + json={"a": 1}, data={"b": 2}, content_type="application/json" + ) + + +class TestRequestsBackend: + def test_prepare_send_data_str_parentid(self) -> None: + file = "12345" + files = {"file": ("file.tar.gz", file, "application/octet-stream")} + post_data = {"parent_id": "12"} + + result = requests_backend.RequestsBackend.prepare_send_data( + files=files, post_data=post_data, raw=False + ) + assert result.json is None + assert result.content_type.startswith("multipart/form-data") + assert isinstance(result.data, MultipartEncoder) From a41539c403211f56b857f7ba9a856c82a873af3b Mon Sep 17 00:00:00 2001 From: "John L. Villalovos" Date: Sat, 4 Mar 2023 18:23:25 -0800 Subject: [PATCH 2/2] fix: support int for `parent_id` in `import_group` This will also fix other use cases where an integer is passed in to MultipartEncoder. Added unit tests to show it works. Closes: #2506 --- gitlab/_backends/requests_backend.py | 12 +++++++---- gitlab/v4/objects/groups.py | 4 ++-- tests/unit/_backends/test_requests_backend.py | 21 +++++++++++++++---- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/gitlab/_backends/requests_backend.py b/gitlab/_backends/requests_backend.py index d70cf4291..839b69e1f 100644 --- a/gitlab/_backends/requests_backend.py +++ b/gitlab/_backends/requests_backend.py @@ -70,17 +70,21 @@ def prepare_send_data( if post_data is None: post_data = {} else: - # booleans does not exists for data (neither for MultipartEncoder): - # cast to string int to avoid: 'bool' object has no attribute 'encode' + # When creating a `MultipartEncoder` instance with data-types + # which don't have an `encode` method it will cause an error: + # object has no attribute 'encode' + # So convert common non-string types into strings. if TYPE_CHECKING: assert isinstance(post_data, dict) for k, v in post_data.items(): if isinstance(v, bool): - post_data[k] = str(int(v)) + v = int(v) + if isinstance(v, (complex, float, int)): + post_data[k] = str(v) post_data["file"] = files.get("file") post_data["avatar"] = files.get("avatar") - data = MultipartEncoder(post_data) + data = MultipartEncoder(fields=post_data) return SendData(data=data, content_type=data.content_type) if raw and post_data: diff --git a/gitlab/v4/objects/groups.py b/gitlab/v4/objects/groups.py index 0eb516fc7..4dc4fd13e 100644 --- a/gitlab/v4/objects/groups.py +++ b/gitlab/v4/objects/groups.py @@ -378,7 +378,7 @@ def import_group( file: BinaryIO, path: str, name: str, - parent_id: Optional[str] = None, + parent_id: Optional[Union[int, str]] = None, **kwargs: Any, ) -> Union[Dict[str, Any], requests.Response]: """Import a group from an archive file. @@ -399,7 +399,7 @@ def import_group( A representation of the import status. """ files = {"file": ("file.tar.gz", file, "application/octet-stream")} - data = {"path": path, "name": name} + data: Dict[str, Any] = {"path": path, "name": name} if parent_id is not None: data["parent_id"] = parent_id diff --git a/tests/unit/_backends/test_requests_backend.py b/tests/unit/_backends/test_requests_backend.py index d83703d2b..2dd36f80d 100644 --- a/tests/unit/_backends/test_requests_backend.py +++ b/tests/unit/_backends/test_requests_backend.py @@ -25,10 +25,21 @@ def test_senddata_json_and_data(self) -> None: class TestRequestsBackend: - def test_prepare_send_data_str_parentid(self) -> None: - file = "12345" - files = {"file": ("file.tar.gz", file, "application/octet-stream")} - post_data = {"parent_id": "12"} + @pytest.mark.parametrize( + "test_data,expected", + [ + (False, "0"), + (True, "1"), + ("12", "12"), + (12, "12"), + (12.0, "12.0"), + (complex(-2, 7), "(-2+7j)"), + ], + ) + def test_prepare_send_data_non_strings(self, test_data, expected) -> None: + assert isinstance(expected, str) + files = {"file": ("file.tar.gz", "12345", "application/octet-stream")} + post_data = {"test_data": test_data} result = requests_backend.RequestsBackend.prepare_send_data( files=files, post_data=post_data, raw=False @@ -36,3 +47,5 @@ def test_prepare_send_data_str_parentid(self) -> None: assert result.json is None assert result.content_type.startswith("multipart/form-data") assert isinstance(result.data, MultipartEncoder) + assert isinstance(result.data.fields["test_data"], str) + assert result.data.fields["test_data"] == expected