-
Notifications
You must be signed in to change notification settings - Fork 1
Request format of file contents using headers #80
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.
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
Changes from all commits
77179b1
886268e
4f86289
c3f4124
dd94bdd
2184ace
d392aae
4069f66
da4fb1d
c21a08f
6aaca84
576e4f2
963cd9f
2f1fe49
1d8a85a
b346c1c
ae3b136
f8d2fc4
8505b37
ad6285f
daf933e
2f6700a
ce81e02
cc20f8d
10d3cf3
ed83486
e5997b1
9f36624
e89f98e
d08508e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "typeCheckingMode": "strict", | ||
| "reportMissingImports": false, | ||
| "reportPrivateUsage": true, | ||
| "executionEnvironments": [ | ||
| { | ||
| "root": "./tests", | ||
| "reportPrivateUsage": false | ||
| } | ||
| ], | ||
| "reportMissingTypeStubs": false, | ||
| "extraPaths": ["."] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,15 @@ | ||
| import json | ||
| import os | ||
| from enum import StrEnum | ||
| from pathlib import Path | ||
|
|
||
| import uvicorn | ||
| from fastapi import FastAPI | ||
| from fastapi import FastAPI, HTTPException, Request | ||
| from fastapi.middleware.cors import CORSMiddleware | ||
| from fastapi.responses import JSONResponse, Response | ||
|
|
||
| from daq_config_server.constants import ENDPOINTS | ||
| from daq_config_server.log import LOGGER | ||
|
|
||
| app = FastAPI( | ||
| title="DAQ config server", | ||
|
|
@@ -22,17 +27,79 @@ | |
| __all__ = ["main"] | ||
|
|
||
|
|
||
| @app.get(ENDPOINTS.CONFIG + "/{file_path:path}") | ||
| def get_configuration(file_path: Path): | ||
| """Read a file and return its contents completely unformatted as a string. After | ||
| https://github.com/DiamondLightSource/daq-config-server/issues/67, this endpoint | ||
| will convert commonly read files to a dictionary format | ||
| class ValidAcceptHeaders(StrEnum): | ||
| JSON = "application/json" | ||
| PLAIN_TEXT = "text/plain" | ||
| RAW_BYTES = "application/octet-stream" | ||
|
|
||
|
|
||
| @app.get( | ||
| ENDPOINTS.CONFIG + "/{file_path:path}", | ||
| responses={ | ||
| 200: { | ||
| "description": "Returns JSON, plain text, or binary file.", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "additionalProperties": True, | ||
| "example": { | ||
| "key": "value", | ||
| "list": [1, 2, 3], | ||
| "nested": {"a": 1}, | ||
| }, | ||
| } | ||
| }, | ||
| "text/plain": { | ||
| "schema": { | ||
| "type": "string", | ||
| "example": "This is a plain text response", | ||
| } | ||
| }, | ||
| "application/octet-stream": { | ||
| "schema": {"type": "string", "format": "binary"}, | ||
| }, | ||
| }, | ||
| }, | ||
|
Comment on lines
+38
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to hardcode this?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so, yes. It's used by the SWAGGER UI to give you more info about accepted requests/responses. Things still work without it though |
||
| }, | ||
| ) | ||
| def get_configuration( | ||
| file_path: Path, | ||
| request: Request, | ||
| ): | ||
| """ | ||
| Read a file and return its contents in a format specified by the accept header. | ||
| """ | ||
| if not file_path.is_file(): | ||
| raise FileNotFoundError(f"File {file_path} cannot be found") | ||
| raise HTTPException(status_code=404, detail=f"File {file_path} cannot be found") | ||
|
|
||
| file_name = os.path.basename(file_path) | ||
| accept = request.headers.get("accept", ValidAcceptHeaders.PLAIN_TEXT) | ||
|
|
||
| try: | ||
| match accept: | ||
| case ValidAcceptHeaders.JSON: | ||
| with file_path.open("r", encoding="utf-8") as f: | ||
| content = json.loads(f.read()) | ||
olliesilvester marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return JSONResponse( | ||
| content=content, | ||
| ) | ||
| case ValidAcceptHeaders.PLAIN_TEXT: | ||
| with file_path.open("r", encoding="utf-8") as f: | ||
| content = f.read() | ||
| return Response(content=content, media_type=accept) | ||
| case _: | ||
| pass | ||
| 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() | ||
|
|
||
| with file_path.open("r", encoding="utf-8") as f: | ||
| return f.read() | ||
| return Response(content=content, media_type=ValidAcceptHeaders.RAW_BYTES) | ||
|
|
||
|
|
||
| def main(): | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,13 +1,24 @@ | ||||||||||||||
| import operator | ||||||||||||||
| from enum import StrEnum | ||||||||||||||
| from logging import Logger, getLogger | ||||||||||||||
| from pathlib import Path | ||||||||||||||
| from typing import Any | ||||||||||||||
|
|
||||||||||||||
| import requests | ||||||||||||||
| from cachetools import TTLCache, cachedmethod | ||||||||||||||
| from requests import Response | ||||||||||||||
|
|
||||||||||||||
| from daq_config_server.app import ValidAcceptHeaders | ||||||||||||||
|
|
||||||||||||||
| from .constants import ENDPOINTS | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class RequestedResponseFormats(StrEnum): | ||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really like the names for this enum's values. I would like it to be obvious to the user what format the response would be in - open to suggestions here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tried to find something, couldn't |
||||||||||||||
| 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 | ||||||||||||||
|
Comment on lines
+16
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
If you just want to be able to pass the desired return type into the request instead, this could be moved inside get_file_contents?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, this works well. The downside is that it does mean we lose the behaviour where it will just give you the output in bytes if it fails to convert to whatever you asked, since the validation here will fail. Having this validation instead of defaulting to bytes may be better anyway though, what do you think?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, because then you'd need
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, so that we can have nice static typing on this method, we will leave it to the user to do any try's with different types
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the sake of easier reviews, I'm going to push this change into a new PR, see #86 |
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class ConfigServer: | ||||||||||||||
| def __init__( | ||||||||||||||
| self, | ||||||||||||||
|
|
@@ -27,77 +38,105 @@ def __init__( | |||||||||||||
| """ | ||||||||||||||
| self._url = url.rstrip("/") | ||||||||||||||
| self._log = log if log else getLogger("daq_config_server.client") | ||||||||||||||
| self._cache: TTLCache[tuple[str, str | None], str] = TTLCache( | ||||||||||||||
| self._cache: TTLCache[tuple[str, str, Path], str] = TTLCache( | ||||||||||||||
| maxsize=cache_size, ttl=cache_lifetime_s | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| def _get( | ||||||||||||||
| self, | ||||||||||||||
| endpoint: str, | ||||||||||||||
| item: str | None = None, | ||||||||||||||
| reset_cached_result: bool = False, | ||||||||||||||
| ) -> Any: | ||||||||||||||
| """ | ||||||||||||||
| Get data from the config server with cache management. | ||||||||||||||
| If a cached response doesn't already exist, makes a request to | ||||||||||||||
| the config server. | ||||||||||||||
| If reset_cached_result is true, remove the cache entry for that request and | ||||||||||||||
| make a new request | ||||||||||||||
|
|
||||||||||||||
| Args: | ||||||||||||||
| endpoint: API endpoint. | ||||||||||||||
| item: Optional item identifier. | ||||||||||||||
| reset_cached_result: Whether to reset cache. | ||||||||||||||
|
|
||||||||||||||
| Returns: | ||||||||||||||
| The response data. | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| if (endpoint, item) in self._cache and reset_cached_result: | ||||||||||||||
| del self._cache[(endpoint, item)] | ||||||||||||||
| return self._cached_get(endpoint, item) | ||||||||||||||
|
|
||||||||||||||
| @cachedmethod(cache=operator.attrgetter("_cache")) | ||||||||||||||
| def _cached_get( | ||||||||||||||
| self, | ||||||||||||||
| endpoint: str, | ||||||||||||||
| item: str | None = None, | ||||||||||||||
| ) -> Any: | ||||||||||||||
| accept_header: str, | ||||||||||||||
| file_path: Path, | ||||||||||||||
| ) -> Response: | ||||||||||||||
| """ | ||||||||||||||
| Get data from the config server and cache it. | ||||||||||||||
|
|
||||||||||||||
| Args: | ||||||||||||||
| endpoint: API endpoint. | ||||||||||||||
| item: Optional item identifier. | ||||||||||||||
| file_path: absolute path to the file which will be read | ||||||||||||||
|
|
||||||||||||||
| Returns: | ||||||||||||||
| The response data. | ||||||||||||||
| """ | ||||||||||||||
| url = self._url + endpoint + (f"/{item}" if item else "") | ||||||||||||||
|
|
||||||||||||||
| try: | ||||||||||||||
| r = requests.get(url) | ||||||||||||||
| request_url = self._url + endpoint + (f"/{file_path}") | ||||||||||||||
| r = requests.get(request_url, headers={"Accept": accept_header}) | ||||||||||||||
| r.raise_for_status() | ||||||||||||||
| data = r.json() | ||||||||||||||
| self._log.debug(f"Cache set for {endpoint}/{item}.") | ||||||||||||||
| return data | ||||||||||||||
| 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 | ||||||||||||||
|
|
||||||||||||||
| def read_unformatted_file( | ||||||||||||||
| self, file_path: str, reset_cached_result: bool = False | ||||||||||||||
| def _get( | ||||||||||||||
| self, | ||||||||||||||
| endpoint: str, | ||||||||||||||
| accept_header: str, | ||||||||||||||
| file_path: Path, | ||||||||||||||
| reset_cached_result: bool = False, | ||||||||||||||
| ): | ||||||||||||||
| """ | ||||||||||||||
| Get data from the config server with cache management and use | ||||||||||||||
| 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: | ||||||||||||||
| del self._cache[(endpoint, accept_header, file_path)] | ||||||||||||||
| r = self._cached_get(endpoint, accept_header, file_path) | ||||||||||||||
|
|
||||||||||||||
| content_type = r.headers["content-type"].split(";")[0].strip() | ||||||||||||||
|
|
||||||||||||||
| if content_type != accept_header: | ||||||||||||||
| self._log.warning( | ||||||||||||||
| f"Server failed to parse the file as requested. Requested \ | ||||||||||||||
| {accept_header} but response came as content-type {content_type}" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| try: | ||||||||||||||
| match content_type: | ||||||||||||||
| case ValidAcceptHeaders.JSON: | ||||||||||||||
| content = r.json() | ||||||||||||||
| case ValidAcceptHeaders.PLAIN_TEXT: | ||||||||||||||
| content = r.text | ||||||||||||||
| 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 | ||||||||||||||
|
|
||||||||||||||
| return content | ||||||||||||||
|
|
||||||||||||||
| def get_file_contents( | ||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
| self, | ||||||||||||||
| file_path: Path, | ||||||||||||||
| requested_response_format: RequestedResponseFormats = ( | ||||||||||||||
| RequestedResponseFormats.DECODED_STRING | ||||||||||||||
| ), | ||||||||||||||
| reset_cached_result: bool = False, | ||||||||||||||
| ) -> Any: | ||||||||||||||
| """ | ||||||||||||||
| Read an unformatted file from the config server. | ||||||||||||||
| 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. | ||||||||||||||
|
|
||||||||||||||
| Args: | ||||||||||||||
| file_path: Path to the file. | ||||||||||||||
| reset_cached_result: Whether to reset cache. | ||||||||||||||
|
|
||||||||||||||
| requested_response_format: Specify how to parse the response. | ||||||||||||||
| reset_cached_result: If true, make a request and store response in cache, | ||||||||||||||
| otherwise look for cached response before making | ||||||||||||||
| new request | ||||||||||||||
| Returns: | ||||||||||||||
| The file content. | ||||||||||||||
| The file contents, in the format specified. | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
| return self._get( | ||||||||||||||
| ENDPOINTS.CONFIG, file_path, reset_cached_result=reset_cached_result | ||||||||||||||
| ENDPOINTS.CONFIG, | ||||||||||||||
| requested_response_format, | ||||||||||||||
| file_path, | ||||||||||||||
| reset_cached_result=reset_cached_result, | ||||||||||||||
| ) | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| 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") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,9 @@ | ||
| from pathlib import Path | ||
|
|
||
| TEST_DATA_DIR = Path(f"{Path(__file__).parent}/test_data") | ||
| TEST_DATA_DIR_PATH = Path(f"{Path(__file__).parent}/test_data") | ||
|
|
||
| TEST_BEAMLINE_PARAMETERS_PATH = TEST_DATA_DIR_PATH.joinpath("beamline_parameters.txt") | ||
|
|
||
| TEST_BAD_JSON_PATH = TEST_DATA_DIR_PATH.joinpath("test_bad_json") | ||
|
|
||
| TEST_GOOD_JSON_PATH = TEST_DATA_DIR_PATH.joinpath("test_good_json.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came with the copier template, but I replaced it with
pyrightconfig.jsonas it seems to allow more options, like ignoring certain rules in unit tests