Get typing support on client and do better exception handling

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)
 

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,42 +49,51 @@ 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
         )
 
     @cachedmethod(cache=operator.attrgetter("_cache"))
     def _cached_get(
         self,
         endpoint: str,
-        accept_header: str,
+        accept_header: ValidAcceptHeaders,
         file_path: Path,
     ) -> Response:
         """
         Get data from the config server and cache it.
 
         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,
+            )
         )

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)

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(