diff --git a/UnleashClient/_transport.py b/UnleashClient/_transport.py new file mode 100644 index 00000000..6bc9c2ce --- /dev/null +++ b/UnleashClient/_transport.py @@ -0,0 +1,108 @@ +""" +HTTP layer under the Unleash API client: sends one request and returns the +response. +""" + +from dataclasses import dataclass, field +from typing import Any, Mapping, Optional, Protocol, runtime_checkable + +import requests +from requests.adapters import HTTPAdapter +from urllib3 import Retry + +from UnleashClient.constants import REQUEST_TIMEOUT + + +@dataclass(frozen=True, kw_only=True) +class Request: + """ + HTTP request. + + :param method: ``"GET"`` or ``"POST"``. + :param url: Absolute url, without the query string. + :param headers: Headers to send. + :param params: Query string parameters. + :param body: Request body, or None to send none. + """ + + method: str + url: str + headers: Mapping[str, str] + params: Mapping[str, str] = field(default_factory=dict) + body: Optional[bytes] = None + + +@dataclass(frozen=True, kw_only=True) +class Response: + """ + HTTP response. Every status is a response, not an error: deciding what + a status means is the API client's job. + + :param status: HTTP status code. + :param headers: Response headers, looked up case-insensitively. + :param body: Raw response body. + """ + + status: int + headers: Mapping[str, str] + body: bytes + + +@runtime_checkable +class Transport(Protocol): + """Sends requests for the sync client.""" + + def execute(self, request: Request) -> Response: + """ + Send ``request`` once and return the response, whatever its status. + + :raises Exception: only when no response arrived, e.g. a connection + error or a timeout. + """ + + +class RequestsTransport: + """ + Sends requests with ``requests``, on a fresh session for every request. + + :param timeout: Seconds to wait for the connection and for each read. + :param _retries: GET requests are retried this many times on a connection + error or a 500, 502 or 504, by urllib3. + :param _request_kwargs: extra keyword arguments for every ``requests`` call. + """ + + def __init__( + self, + *, + timeout: float = REQUEST_TIMEOUT, + # these two are are going away too + _retries: Optional[int] = None, + _request_kwargs: Optional[Mapping[str, Any]] = None, + ) -> None: + self._timeout = timeout + self._retries = _retries + self._request_kwargs = dict(_request_kwargs or {}) + + def execute(self, request: Request) -> Response: + kwargs: dict = {"headers": dict(request.headers), "timeout": self._timeout} + if request.params: + kwargs["params"] = dict(request.params) + if request.body is not None: + kwargs["data"] = request.body + + with requests.Session() as session: + if request.method == "GET" and self._retries is not None: + adapter = HTTPAdapter( + max_retries=Retry( + total=self._retries, status_forcelist=[500, 502, 504] + ) + ) + session.mount("https://", adapter) + session.mount("http://", adapter) + + resp = session.request( + request.method, request.url, **kwargs, **self._request_kwargs + ) + return Response( + status=resp.status_code, headers=resp.headers, body=resp.content + ) diff --git a/UnleashClient/_unleash_api_client.py b/UnleashClient/_unleash_api_client.py index d95f27c7..3bf99eb0 100644 --- a/UnleashClient/_unleash_api_client.py +++ b/UnleashClient/_unleash_api_client.py @@ -3,13 +3,11 @@ from typing import Any, Dict, NamedTuple, Optional, Tuple import requests -from requests import Response -from requests.adapters import HTTPAdapter from requests.exceptions import InvalidHeader, InvalidSchema, InvalidURL, MissingSchema -from urllib3 import Retry from UnleashClient._event_dispatcher import _EventDispatcher from UnleashClient._headers import _HeaderFactory +from UnleashClient._transport import Request, RequestsTransport, Response, Transport from UnleashClient.config import PollingOptions, UnleashConfig from UnleashClient.constants import FEATURES_URL, METRICS_URL, REGISTER_URL from UnleashClient.events import ( @@ -78,19 +76,27 @@ def _normalized_url(url: str, path: str) -> str: return f"{url}{path}" +def _text(resp: Response) -> str: + return resp.body.decode("utf-8", errors="replace") + + def _log_resp_info(resp: Response) -> None: - LOGGER.debug("HTTP status code: %s", resp.status_code) + LOGGER.debug("HTTP status code: %s", resp.status) LOGGER.debug("HTTP headers: %s", resp.headers) - LOGGER.debug("HTTP content: %s", resp.text) + LOGGER.debug("HTTP content: %s", _text(resp)) class _UnleashApiClient: """ - Sends the SDK's requests to the Unleash server: feature fetches, client - registration and metrics submission. + Talks to the Unleash API: feature fetches, client registration and metrics + submission. Builds each request and decides what its response means; the + transport only sends it. :param config: Client configuration. :param headers: Builds the header set each endpoint needs. + :param events: Receives ``FETCH_COMPLETED`` after every feature fetch. + :param transport: Sends the requests. Defaults to a :class:`RequestsTransport` + built from ``config.http``. """ def __init__( @@ -98,11 +104,19 @@ def __init__( config: UnleashConfig, headers: _HeaderFactory, events: Optional[_EventDispatcher] = None, + transport: Optional[Transport] = None, ) -> None: self._config: UnleashConfig = config self._headers: _HeaderFactory = headers self._events: Optional[_EventDispatcher] = events + self._transport: Transport = transport or RequestsTransport( + timeout=config.http.timeout, + _retries=config.http.retries, + # this is going to go away soon, just like the whole httpOptions in the client constructor + _request_kwargs=config.http.options, + ) + # pylint: disable=broad-except def fetch_features(self, etag: str = "", *, options: PollingOptions) -> FetchResult: """ @@ -146,31 +160,24 @@ def _fetch_features( if options.project_name: base_params = {"project": options.project_name} - adapter = HTTPAdapter( - max_retries=Retry( - total=config.http.retries, status_forcelist=[500, 502, 504] - ) - ) - with requests.Session() as session: - session.mount("https://", adapter) - session.mount("http://", adapter) - resp = session.get( - base_url, + resp = self._transport.execute( + Request( + method="GET", + url=base_url, headers={ **self._headers.polling(options), **request_specific_headers, }, params=base_params, - timeout=config.http.timeout, - **config.http.options, ) + ) - http_status_code = resp.status_code - if resp.status_code not in [200, 304]: + http_status_code = resp.status + if resp.status not in [200, 304]: _log_resp_info(resp) LOGGER.warning( "Unleash Client feature fetch failed due to unexpected HTTP status code: %s", - resp.status_code, + resp.status, ) raise Exception( "Unleash Client feature fetch failed!" @@ -180,13 +187,14 @@ def _fetch_features( if "etag" in resp.headers.keys(): fetched_etag = resp.headers["etag"] - if resp.status_code == 304: + if resp.status == 304: return ( FetchResult(None, fetched_etag, not_modified=True), FetchStatus.NOT_CHANGED, ) - return FetchResult(resp.text, fetched_etag), _fetched_status(resp.text) + raw_state = _text(resp) + return FetchResult(raw_state, fetched_etag), _fetched_status(raw_state) except Exception as exc: LOGGER.exception( "Unleash Client feature fetch failed due to exception: %s", exc @@ -212,19 +220,20 @@ def register(self, payload: Dict[str, Any]) -> bool: LOGGER.info("Registering unleash client with unleash @ %s", config.url) LOGGER.info("Registration request information: %s", payload) - resp = requests.post( - _normalized_url(config.url, REGISTER_URL), - data=json.dumps(payload), - headers=self._headers.base(), - timeout=config.http.timeout, - **config.http.options, + resp = self._transport.execute( + Request( + method="POST", + url=_normalized_url(config.url, REGISTER_URL), + headers=self._headers.base(), + body=json.dumps(payload).encode("utf-8"), + ) ) - if resp.status_code not in {200, 202}: + if resp.status not in {200, 202}: _log_resp_info(resp) LOGGER.warning( "Unleash Client registration failed due to unexpected HTTP status code: %s", - resp.status_code, + resp.status, ) return False @@ -255,19 +264,20 @@ def send_metrics(self, payload: Dict[str, Any]) -> bool: LOGGER.info("Sending messages to with unleash @ %s", config.url) LOGGER.info("unleash metrics information: %s", payload) - resp = requests.post( - _normalized_url(config.url, METRICS_URL), - data=json.dumps(payload), - headers=self._headers.metrics(), - timeout=config.http.timeout, - **config.http.options, + resp = self._transport.execute( + Request( + method="POST", + url=_normalized_url(config.url, METRICS_URL), + headers=self._headers.metrics(), + body=json.dumps(payload).encode("utf-8"), + ) ) - if resp.status_code != 202: + if resp.status != 202: _log_resp_info(resp) LOGGER.warning( "Unleash Client metrics submission due to unexpected HTTP status code: %s", - resp.status_code, + resp.status, ) return False diff --git a/tests/unit_tests/test_transport.py b/tests/unit_tests/test_transport.py new file mode 100644 index 00000000..66e37548 --- /dev/null +++ b/tests/unit_tests/test_transport.py @@ -0,0 +1,158 @@ +import json + +import pytest +import responses +from requests import ConnectionError + +from tests.utilities.testing_constants import URL +from UnleashClient._transport import Request, RequestsTransport, Transport + +ENDPOINT = URL + "/client/features" + + +def test_requests_transport_is_a_transport(): + assert isinstance(RequestsTransport(), Transport) + + +@responses.activate +def test_execute_sends_a_get_with_its_headers_and_params(): + responses.add(responses.GET, ENDPOINT, status=200) + + RequestsTransport().execute( + Request( + method="GET", + url=ENDPOINT, + headers={"X-Test": "value"}, + params={"project": "ivan"}, + ) + ) + + sent = responses.calls[0].request + assert sent.method == "GET" + assert sent.url == ENDPOINT + "?project=ivan" + assert sent.headers["X-Test"] == "value" + assert sent.body is None + + +@responses.activate +def test_execute_sends_a_post_with_its_body(): + responses.add(responses.POST, ENDPOINT, status=202) + + RequestsTransport().execute( + Request(method="POST", url=ENDPOINT, headers={}, body=b'{"appName": "x"}') + ) + + sent = responses.calls[0].request + assert sent.method == "POST" + assert sent.url == ENDPOINT + assert json.loads(sent.body) == {"appName": "x"} + + +@responses.activate +def test_execute_returns_the_status_headers_and_body(): + responses.add( + responses.GET, + ENDPOINT, + body=b'{"version": 1}', + status=200, + headers={"ETag": "v1"}, + ) + + response = RequestsTransport().execute( + Request(method="GET", url=ENDPOINT, headers={}) + ) + + assert response.status == 200 + assert response.body == b'{"version": 1}' + # looked up case-insensitively + assert response.headers["etag"] == "v1" + + +@responses.activate +@pytest.mark.parametrize("status", [304, 401, 500]) +def test_execute_returns_any_status_instead_of_raising(status): + responses.add(responses.GET, ENDPOINT, status=status) + + response = RequestsTransport().execute( + Request(method="GET", url=ENDPOINT, headers={}) + ) + + assert response.status == status + + +@responses.activate +def test_execute_raises_when_no_response_arrives(): + responses.add(responses.GET, ENDPOINT, body=ConnectionError("refused")) + + with pytest.raises(ConnectionError): + RequestsTransport().execute(Request(method="GET", url=ENDPOINT, headers={})) + + +@responses.activate +def test_execute_retries_a_get_when_retries_are_set(): + responses.add(responses.GET, ENDPOINT, status=500) + responses.add(responses.GET, ENDPOINT, body=b"{}", status=200) + + response = RequestsTransport(_retries=1).execute( + Request(method="GET", url=ENDPOINT, headers={}) + ) + + assert len(responses.calls) == 2 + assert response.status == 200 + + +@responses.activate +def test_execute_does_not_retry_without_retries(): + responses.add(responses.GET, ENDPOINT, status=500) + + response = RequestsTransport().execute( + Request(method="GET", url=ENDPOINT, headers={}) + ) + + assert len(responses.calls) == 1 + assert response.status == 500 + + +@responses.activate +def test_execute_never_retries_a_post(): + responses.add(responses.POST, ENDPOINT, status=500) + + response = RequestsTransport(_retries=3).execute( + Request(method="POST", url=ENDPOINT, headers={}, body=b"{}") + ) + + assert len(responses.calls) == 1 + assert response.status == 500 + + +@responses.activate +def test_execute_passes_the_extra_request_kwargs(): + responses.add( + responses.GET, ENDPOINT, status=302, headers={"Location": URL + "/elsewhere"} + ) + + response = RequestsTransport(_request_kwargs={"allow_redirects": False}).execute( + Request(method="GET", url=ENDPOINT, headers={}) + ) + + assert response.status == 302 + + +def test_execute_rejects_an_extra_kwarg_it_already_sets(): + transport = RequestsTransport(_request_kwargs={"timeout": 5}) + + with pytest.raises(TypeError): + transport.execute(Request(method="GET", url=ENDPOINT, headers={})) + + +@responses.activate +def test_execute_uses_a_fresh_session_for_every_request(): + responses.add(responses.GET, ENDPOINT, status=200, headers={"Set-Cookie": "a=1"}) + responses.add(responses.GET, ENDPOINT, status=200) + transport = RequestsTransport() + + transport.execute(Request(method="GET", url=ENDPOINT, headers={})) + transport.execute(Request(method="GET", url=ENDPOINT, headers={})) + + # A shared session would send the cookie back. + assert "Cookie" not in responses.calls[1].request.headers diff --git a/tests/unit_tests/test_unleash_api_client.py b/tests/unit_tests/test_unleash_api_client.py index c556b005..3375b7e5 100644 --- a/tests/unit_tests/test_unleash_api_client.py +++ b/tests/unit_tests/test_unleash_api_client.py @@ -24,6 +24,7 @@ URL, ) from UnleashClient._headers import _HeaderFactory +from UnleashClient._transport import Request, Response from UnleashClient._unleash_api_client import _UnleashApiClient from UnleashClient.config import ( HttpOptions, @@ -418,3 +419,63 @@ def emit_event(self, event): ) assert json.loads(result.raw_state)["version"] == 1 + + +# transport + + +class RecordingTransport: + """Answers every request with one canned response, keeping the requests.""" + + def __init__(self, response: Response) -> None: + self.response = response + self.requests: list = [] + + def execute(self, request: Request) -> Response: + self.requests.append(request) + return self.response + + +def test_fetch_features_sends_its_request_through_the_transport(): + transport = RecordingTransport( + Response(status=200, headers={"etag": ETAG_VALUE}, body=b'{"version": 1}') + ) + config = UnleashConfig(URL, APP_NAME) + api_client = _UnleashApiClient(config, _HeaderFactory(config), transport=transport) + + result = api_client.fetch_features( + "previous-etag", options=PollingOptions(project_name=PROJECT_NAME) + ) + + (request,) = transport.requests + assert request.method == "GET" + assert request.url == FULL_FEATURE_URL + assert request.params == {"project": PROJECT_NAME} + assert request.headers["If-None-Match"] == "previous-etag" + assert result == ('{"version": 1}', ETAG_VALUE, False) + + +def test_register_posts_its_payload_through_the_transport(): + transport = RecordingTransport(Response(status=202, headers={}, body=b"")) + config = UnleashConfig(URL, APP_NAME) + api_client = _UnleashApiClient(config, _HeaderFactory(config), transport=transport) + + assert api_client.register({"appName": APP_NAME}) + + (request,) = transport.requests + assert request.method == "POST" + assert request.url == FULL_REGISTER_URL + assert json.loads(request.body) == {"appName": APP_NAME} + + +def test_send_metrics_posts_its_payload_through_the_transport(): + transport = RecordingTransport(Response(status=202, headers={}, body=b"")) + config = UnleashConfig(URL, APP_NAME) + api_client = _UnleashApiClient(config, _HeaderFactory(config), transport=transport) + + assert api_client.send_metrics({"appName": APP_NAME}) + + (request,) = transport.requests + assert request.method == "POST" + assert request.url == FULL_METRICS_URL + assert json.loads(request.body) == {"appName": APP_NAME}