diff --git a/src/daq_config_server/app.py b/src/daq_config_server/app.py index 82f25a7..ad78f80 100644 --- a/src/daq_config_server/app.py +++ b/src/daq_config_server/app.py @@ -7,9 +7,9 @@ from fastapi import FastAPI, HTTPException, Request from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse, Response +from starlette import status from daq_config_server.constants import ENDPOINTS -from daq_config_server.log import LOGGER app = FastAPI( title="DAQ config server", @@ -71,7 +71,10 @@ def get_configuration( Read a file and return its contents in a format specified by the accept header. """ if not file_path.is_file(): - raise HTTPException(status_code=404, detail=f"File {file_path} cannot be found") + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"File {file_path} cannot be found", + ) file_name = os.path.basename(file_path) accept = request.headers.get("accept", ValidAcceptHeaders.PLAIN_TEXT) @@ -89,15 +92,16 @@ def get_configuration( content = f.read() return Response(content=content, media_type=accept) case _: - pass + with file_path.open("rb") as f: + content = f.read() except Exception as e: - LOGGER.warning( - f"Failed to convert {file_name} to {accept} and caught \ - exception: {e} \nSending file as raw bytes instead" - ) - - with file_path.open("rb") as f: - content = f.read() + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + f"Failed to convert {file_name} to {accept}. " + "Try requesting this file as a different type." + ), + ) from e return Response(content=content, media_type=ValidAcceptHeaders.RAW_BYTES) diff --git a/src/daq_config_server/client.py b/src/daq_config_server/client.py index 45d734a..155adc0 100644 --- a/src/daq_config_server/client.py +++ b/src/daq_config_server/client.py @@ -1,22 +1,33 @@ import operator -from enum import StrEnum +from collections import defaultdict from logging import Logger, getLogger from pathlib import Path -from typing import Any +from typing import Any, TypeVar import requests from cachetools import TTLCache, cachedmethod +from pydantic import TypeAdapter from requests import Response +from requests.exceptions import HTTPError from daq_config_server.app import ValidAcceptHeaders from .constants import ENDPOINTS +T = TypeVar("T", str, bytes, dict[Any, Any]) -class RequestedResponseFormats(StrEnum): - DICT = ValidAcceptHeaders.JSON # Convert to dict using Response.json() - DECODED_STRING = ValidAcceptHeaders.PLAIN_TEXT # Use utf-8 decoding in response - RAW_BYTE_STRING = ValidAcceptHeaders.RAW_BYTES # Use raw bytes in response + +return_type_to_mime_type: dict[type, ValidAcceptHeaders] = defaultdict( + lambda: ValidAcceptHeaders.PLAIN_TEXT, + { + dict[Any, Any]: ValidAcceptHeaders.JSON, + str: ValidAcceptHeaders.PLAIN_TEXT, + bytes: ValidAcceptHeaders.RAW_BYTES, + }, +) + + +class TypeConversionException(Exception): ... class ConfigServer: @@ -38,7 +49,7 @@ def __init__( """ self._url = url.rstrip("/") self._log = log if log else getLogger("daq_config_server.client") - self._cache: TTLCache[tuple[str, str, Path], str] = TTLCache( + self._cache: TTLCache[tuple[str, str, Path], Response] = TTLCache( maxsize=cache_size, ttl=cache_lifetime_s ) @@ -46,7 +57,7 @@ def __init__( def _cached_get( self, endpoint: str, - accept_header: str, + accept_header: ValidAcceptHeaders, file_path: Path, ) -> Response: """ @@ -54,26 +65,35 @@ def _cached_get( Args: endpoint: API endpoint. + accept_header: Accept header MIME type file_path: absolute path to the file which will be read Returns: The response data. """ + request_url = self._url + endpoint + (f"/{file_path}") + r = requests.get(request_url, headers={"Accept": accept_header}) + # Intercept http exceptions from server so that the client + # can include the response `detail` sent by the server try: - request_url = self._url + endpoint + (f"/{file_path}") - r = requests.get(request_url, headers={"Accept": accept_header}) r.raise_for_status() - self._log.debug(f"Cache set for {request_url}.") - return r - except requests.exceptions.HTTPError as e: - self._log.error(f"HTTP error: {e}") - raise + except requests.exceptions.HTTPError as err: + try: + error_detail = r.json().get("detail") + self._log.error(error_detail) + raise HTTPError(error_detail) from err + except ValueError: + self._log.error("Response raised HTTP error but no details provided") + raise HTTPError from err + + self._log.debug(f"Cache set for {request_url}.") + return r def _get( self, endpoint: str, - accept_header: str, + accept_header: ValidAcceptHeaders, file_path: Path, reset_cached_result: bool = False, ): @@ -82,7 +102,11 @@ def _get( the content-type response header to format the return value. If data parsing fails, return the response contents in bytes """ - if (endpoint, accept_header, file_path) in self._cache and reset_cached_result: + if ( + endpoint, + accept_header, + file_path, + ) in self._cache and reset_cached_result: del self._cache[(endpoint, accept_header, file_path)] r = self._cached_get(endpoint, accept_header, file_path) @@ -103,40 +127,42 @@ def _get( case _: content = r.content except Exception as e: - self._log.warning( - f"Failed trying to convert to content-type {content_type} due to\ - exception {e} \nReturning as bytes instead" - ) - content = r.content + raise TypeConversionException( + f"Failed trying to convert to content-type {content_type}." + ) from e return content def get_file_contents( self, - file_path: Path, - requested_response_format: RequestedResponseFormats = ( - RequestedResponseFormats.DECODED_STRING - ), + file_path: Path | str, + desired_return_type: type[T] = str, reset_cached_result: bool = False, - ) -> Any: + ) -> T: """ Get contents of a file from the config server in the format specified. - If data parsing fails, contents will return as raw bytes. Optionally look - for cached result before making request. + Optionally look for cached result before making request. + + Current supported return types are: str, bytes, dict[str, str]. This option will + determine how the server attempts to decode the file Args: file_path: Path to the file. requested_response_format: Specify how to parse the response. - reset_cached_result: If true, make a request and store response in cache, + desired_return_type: If true, make a request and store response in cache, otherwise look for cached response before making new request Returns: The file contents, in the format specified. """ - - return self._get( - ENDPOINTS.CONFIG, - requested_response_format, - file_path, - reset_cached_result=reset_cached_result, + file_path = Path(file_path) + accept_header = return_type_to_mime_type[desired_return_type] + + return TypeAdapter(desired_return_type).validate_python( # type: ignore - to allow any dict + self._get( + ENDPOINTS.CONFIG, + accept_header, + file_path, + reset_cached_result=reset_cached_result, + ) ) diff --git a/src/daq_config_server/log.py b/src/daq_config_server/log.py deleted file mode 100644 index 599dcc7..0000000 --- a/src/daq_config_server/log.py +++ /dev/null @@ -1,22 +0,0 @@ -import logging - - -def get_default_logger(name: str = __name__) -> logging.Logger: - logger = logging.getLogger(name) - logger.setLevel(logging.INFO) - - # Prevent adding handlers multiple times - if not logger.handlers: - console_handler = logging.StreamHandler() - formatter = logging.Formatter( - "%(asctime)s - %(levelname)s - %(message)s", datefmt="%Y-%m-%d %H:%M:%S" - ) - console_handler.setFormatter(formatter) - logger.addHandler(console_handler) - - return logger - - -# For now use a basic console-writing logger. Integrate properly with kubernetes in the -# future -LOGGER = get_default_logger("daq-config-server") diff --git a/tests/system_tests/test_client.py b/tests/system_tests/test_client.py index 4a46648..41a459b 100644 --- a/tests/system_tests/test_client.py +++ b/tests/system_tests/test_client.py @@ -1,8 +1,12 @@ import json +import os +from typing import Any +from unittest.mock import MagicMock import pytest +import requests -from daq_config_server.client import ConfigServer, RequestedResponseFormats +from daq_config_server.client import ConfigServer from tests.constants import ( TEST_BAD_JSON_PATH, TEST_BEAMLINE_PARAMETERS_PATH, @@ -49,7 +53,7 @@ def test_read_file_as_bytes(server: ConfigServer): assert ( server.get_file_contents( file_path, - requested_response_format=RequestedResponseFormats.RAW_BYTE_STRING, + bytes, ) == expected_response ) @@ -64,22 +68,25 @@ def test_read_good_json_as_dict(server: ConfigServer): assert ( server.get_file_contents( file_path, - requested_response_format=RequestedResponseFormats.DICT, + dict[Any, Any], ) == expected_response ) @pytest.mark.requires_local_server -def test_bad_json_read_as_bytes(server: ConfigServer): +def test_bad_json_gives_http_error_with_details(server: ConfigServer): file_path = TEST_BAD_JSON_PATH - with open(file_path, "rb") as f: - expected_response = f.read() + file_name = os.path.basename(file_path) + expected_detail = ( + f"Failed to convert {file_name} to application/json. " + "Try requesting this file as a different type." + ) - assert ( + server._log.error = MagicMock() + with pytest.raises(requests.exceptions.HTTPError): server.get_file_contents( file_path, - requested_response_format=RequestedResponseFormats.DICT, + dict[Any, Any], ) - == expected_response - ) + server._log.error.assert_called_once_with(expected_detail) diff --git a/tests/unit_tests/test_api.py b/tests/unit_tests/test_api.py index 7f8517f..f734fb0 100644 --- a/tests/unit_tests/test_api.py +++ b/tests/unit_tests/test_api.py @@ -1,6 +1,5 @@ import json from pathlib import Path -from unittest.mock import MagicMock, patch import pytest from fastapi import status @@ -104,27 +103,16 @@ async def test_get_configuration_on_json_file(mock_app: TestClient): ) -@patch("daq_config_server.app.LOGGER.warning") -async def test_get_configuration_warns_and_uses_raw_bytes_on_failed_utf_8_encoding( - mock_warn: MagicMock, mock_app: TestClient, tmpdir: Path +async def test_get_configuration_gives_http_422_on_failed_conversion( + mock_app: TestClient, tmpdir: Path ): file_path = Path(f"{tmpdir}/test_bad_utf_8.txt") with open(file_path, "wb") as f: f.write(b"\x80\x81\xfe\xff") - expected_contents = b"\x80\x81\xfe\xff" - expected_type = ValidAcceptHeaders.RAW_BYTES - expected_response = Response( - content=expected_contents, - headers={"content-type": expected_type}, - status_code=status.HTTP_200_OK, - ) - await _assert_get_and_response( - mock_app, - f"{ENDPOINTS.CONFIG}/{file_path}", - expected_response, - accept_header={"Accept": ValidAcceptHeaders.PLAIN_TEXT}, + response = mock_app.get( + f"{ENDPOINTS.CONFIG}/{file_path}", headers=ACCEPT_HEADER_DEFAULT ) - mock_warn.assert_called_once() + assert response.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY async def test_health_check_returns_code_200( diff --git a/tests/unit_tests/test_client.py b/tests/unit_tests/test_client.py index 8ca2e10..a5db136 100644 --- a/tests/unit_tests/test_client.py +++ b/tests/unit_tests/test_client.py @@ -1,4 +1,5 @@ from pathlib import Path +from typing import Any from unittest.mock import MagicMock, patch import pytest @@ -8,7 +9,7 @@ from requests import RequestException from daq_config_server.app import ValidAcceptHeaders -from daq_config_server.client import ConfigServer, RequestedResponseFormats +from daq_config_server.client import ConfigServer, TypeConversionException from daq_config_server.constants import ENDPOINTS test_path = Path("test") @@ -48,7 +49,7 @@ def test_get_file_contents_default_header(mock_request: MagicMock): assert server.get_file_contents(test_path) == "test" mock_request.assert_called_once_with( url + ENDPOINTS.CONFIG + "/" + str(test_path), - headers={"Accept": RequestedResponseFormats.DECODED_STRING}, + headers={"Accept": ValidAcceptHeaders.PLAIN_TEXT}, ) @@ -61,15 +62,13 @@ def test_get_file_contents_with_bytes(mock_request: MagicMock): url = "url" server = ConfigServer(url) assert ( - server.get_file_contents( - test_path, requested_response_format=RequestedResponseFormats.DICT - ) + server.get_file_contents(test_path, desired_return_type=bytes) == test_str.encode() ) @patch("daq_config_server.client.requests.get") -def test_get_file_contents_warns_and_gives_bytes_on_invalid_json( +def test_get_file_contents_gives_exception_on_invalid_json( mock_request: MagicMock, ): content_type = ValidAcceptHeaders.JSON @@ -79,17 +78,8 @@ def test_get_file_contents_warns_and_gives_bytes_on_invalid_json( ) url = "url" server = ConfigServer(url) - server._log.warning = MagicMock() - assert ( - server.get_file_contents( - test_path, requested_response_format=RequestedResponseFormats.DICT - ) - == bad_json.encode() - ) - assert ( - f"Failed trying to convert to content-type {content_type} due to " - in server._log.warning.call_args[0][0] - ) + with pytest.raises(TypeConversionException): + server.get_file_contents(test_path, desired_return_type=dict[Any, Any]) @patch("daq_config_server.client.requests.get") @@ -110,12 +100,34 @@ def test_get_file_contents_caching( @patch("daq_config_server.client.requests.get") -def test_read_unformatted_file_reading_not_OK(mock_request: MagicMock): +def test_bad_responses_no_details_raises_error(mock_request: MagicMock): """Test that a non-200 response raises a RequestException.""" mock_request.return_value = make_test_response( "1st_read", status.HTTP_204_NO_CONTENT, raise_exc=requests.exceptions.HTTPError ) - url = "url" - server = ConfigServer(url) + server = ConfigServer("url") + server._log.error = MagicMock() + with pytest.raises(requests.exceptions.HTTPError): + server.get_file_contents(test_path) + server._log.error.assert_called_once_with( + "Response raised HTTP error but no details provided" + ) + + +@patch("daq_config_server.client.requests.get") +def test_bad_responses_with_details_raises_error(mock_request: MagicMock): + """Test that a non-200 response raises a RequestException.""" + + detail = "test detail" + mock_request.return_value = make_test_response( + "1st_read", + status.HTTP_204_NO_CONTENT, + raise_exc=requests.exceptions.HTTPError, + json_value="test", + ) + mock_request.return_value.json = MagicMock(return_value={"detail": detail}) + server = ConfigServer("url") + server._log.error = MagicMock() with pytest.raises(requests.exceptions.HTTPError): server.get_file_contents(test_path) + server._log.error.assert_called_once_with(detail)