Request format of file contents using headers #80

olliesilvester · 2025-05-21T15:07:41Z

Fixes #67

Notes for reviewers:

I think the Enum names are a bit confusing - not sure if a user would prefer to request their data using MIME names or what I currently have (dict, raw_byte_string, decoded_string)
Can check the unit + system tests pass by:
- Open code in dev container
- in a terminal in dev container run the config server with daq-config-server .
- in a separate terminal in dev container run pytest .
With the config server running locally, go to http://localhost:8556/docs and try out get_configuration with the test filepath /workspaces/daq-config-server/tests/test_data/beamline_parameters.txt , and try out the three different accepted "Media types" on the dropdown box

Co-authored-by: Joseph Ware <[email protected]>

…nused_features

codecov · 2025-05-21T15:07:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.23%. Comparing base (e8c3643) to head (9f36624).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
+ Coverage   98.63%   99.23%   +0.60%     
==========================================
  Files           5        6       +1     
  Lines          73      131      +58     
==========================================
+ Hits           72      130      +58     
  Misses          1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

olliesilvester · 2025-05-21T16:55:24Z

src/daq_config_server/client.py

+
+        return content
+
+    def get_file_contents(


~~Can anyone think of a neat way to do better typing here?~~ I don't think we can avoid it having Any as a return type :(

olliesilvester · 2025-05-22T09:25:31Z

src/daq_config_server/client.py

 BlParamDType = str | int | float | bool


+class RequestedResponseFormats(StrEnum):


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

tried to find something, couldn't

olliesilvester · 2025-05-22T16:39:34Z

pyproject.toml

 [tool.setuptools_scm]
 version_file = "src/daq_config_server/_version.py"

-[tool.pyright]


This came with the copier template, but I replaced it with pyrightconfig.json as it seems to allow more options, like ignoring certain rules in unit tests

src/daq_config_server/app.py

…ameter

DiamondJoseph

I need to read through the tests just to make sure everything is behaving how I would expect it to, but the code looks good. Some questions around typing that I'll look at with a fresh head tomorrow

src/daq_config_server/client.py

stan-dot

lgtm, minor cosmetic questions

stan-dot · 2025-05-27T17:20:54Z

src/daq_config_server/app.py

+    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"},
+                },
+            },
+        },


do we need to hardcode this?

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

stan-dot · 2025-05-27T17:25:04Z

src/daq_config_server/client.py

        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, str], str] = TTLCache(


can we write a custom type for what is this generic thing?

Isn't it just in/out of _cached_get? In which case shouldn't it be tuple[str, str, str], response?

stan-dot · 2025-05-27T17:26:40Z

src/daq_config_server/client.py

 BlParamDType = str | int | float | bool


+class RequestedResponseFormats(StrEnum):


tried to find something, couldn't

DiamondJoseph · 2025-05-28T08:31:24Z

src/daq_config_server/client.py

+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


Suggested change

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

response_type: dict[type, ValidAcceptHeaders] = defaultdict(lambda: PLAIN_TEXT, {dict: JSON, str: PLAIN_TEXT, byte: RAW_BYTES})

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?

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?

No, because then you'd need -> T | bytes and to check every time you call it. I would have try -> T catch ValidationException try -> bytes as something the client can do, with the cached return from the server. I also would probably prefer to accept a str rather than bytes? How often are you getting something other than text?

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

For the sake of easier reviews, I'm going to push this change into a new PR, see #86

olliesilvester and others added 16 commits April 28, 2025 16:25

Remove unused features

77179b1

Remove CI for GUI

886268e

Update helmchart/values.yaml

4f86289

Co-authored-by: Joseph Ware <[email protected]>

Changes to CI

c3f4124

remove paths from ci

dd94bdd

update readme

2184ace

Update helmchart

d392aae

Merge remote-tracking branch 'origin/get_cI_working' into 64_remove_u…

4069f66

…nused_features

create main get_configuration endpoint and add tests

da4fb1d

Improve CI

c21a08f

Improve codecov

6aaca84

Merge branch 'main' into 65_create_main_endpoint

576e4f2

Response from review

963cd9f

Small tidy on build bash scri[t

2f1fe49

feat: get_config endpoint uses accept header to format response

1d8a85a

Client uses content-type from response to format output

b346c1c

olliesilvester changed the base branch from main to 65_create_main_endpoint May 21, 2025 15:09

Increase code coverage

ae3b136

olliesilvester commented May 21, 2025

View reviewed changes

olliesilvester added 2 commits May 21, 2025 17:00

Better names and comments

f8d2fc4

Warn on unexpected header, use HTTPException

8505b37

olliesilvester commented May 22, 2025

View reviewed changes

Make sure test hits elusive line for code coverage

ad6285f

Base automatically changed from 65_create_main_endpoint to main May 22, 2025 10:36

olliesilvester added 4 commits May 22, 2025 14:55

Merge formatting features with caching

daf933e

Merge with copier update, including type fixing

2f6700a

Add missed files

ce81e02

Make codecov happy

cc20f8d

olliesilvester marked this pull request as ready for review May 22, 2025 15:37

olliesilvester changed the title ~~Specify format of file contents~~ Request format of file contents using headers May 22, 2025

olliesilvester commented May 22, 2025

View reviewed changes

Minor rewordings

10d3cf3

olliesilvester requested review from DiamondJoseph and Relm-Arrowny May 22, 2025 17:18

olliesilvester commented May 22, 2025

View reviewed changes

src/daq_config_server/app.py Show resolved Hide resolved

olliesilvester added 3 commits May 23, 2025 10:15

Don't do json validation in server, improve server unit tests

ed83486

Readd server converting file contents to json

e5997b1

Add OpenAPI schema, get accept header from request rather than as par…

9f36624

…ameter

DiamondJoseph reviewed May 27, 2025

View reviewed changes

src/daq_config_server/client.py Outdated Show resolved Hide resolved

stan-dot approved these changes May 27, 2025

View reviewed changes

DiamondJoseph reviewed May 28, 2025

View reviewed changes

use Path type for client's get function

e89f98e

This was referenced May 29, 2025

Get static typing support on the client #86

Closed

Include exception details on client and don't default to bytes #87

Closed

Merge branch 'main' into 67_format_json_files

d08508e

DiamondJoseph approved these changes May 29, 2025

View reviewed changes

olliesilvester merged commit fc4a119 into main May 29, 2025
21 checks passed

olliesilvester deleted the 67_format_json_files branch May 29, 2025 12:16

		BlParamDType = str \| int \| float \| bool


		class RequestedResponseFormats(StrEnum):

Request format of file contents using headers #80

Request format of file contents using headers #80

Uh oh!

Conversation

olliesilvester commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for reviewers:

Uh oh!

codecov bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

olliesilvester May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DiamondJoseph left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stan-dot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiamondJoseph May 28, 2025 • edited by olliesilvester Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olliesilvester commented May 21, 2025 •

edited

Loading

codecov bot commented May 21, 2025 •

edited

Loading

olliesilvester May 21, 2025 •

edited

Loading

DiamondJoseph May 28, 2025 •

edited by olliesilvester

Loading