ethereum / beacon-APIs

Collection of RESTful APIs provided by Ethereum Beacon nodes
https://ethereum.github.io/beacon-APIs/
Creative Commons Zero v1.0 Universal
328 stars 167 forks source link

Clarify server behavior if content type is not supported #457

Closed nflaig closed 3 days ago

nflaig commented 2 months ago

This clarifies server behavior if a content type is not supported, allowing clients to handle the error, e.g. by retrying a SSZ request using JSON if the server responds with a 415 status code.

The noted behavior is not specific to the beacon API as it is generally how a well-behaved HTTP server should respond but it becomes especially important here because it allows to gradually adopt SSZ for more routes in a backward compatible way without bumping the API versions.

The most important parts a server needs to support are Accept header with multiple entries and quality values (e.g. Accept: application/octet-stream;q=1.0,application/json;q=0.9) and 415 status code if the Content-Type header in the request specifies a format that is not supported.

Related to https://github.com/ethereum/beacon-APIs/issues/250

mcdee commented 2 months ago

Part of our problem is that we're already in a situation where JSON is assumed for both input and output. In an ideal world we could be strict about the content types in and out, but I suspect that may create issues with existing implementations. So I would be inclined to tweak this as follows:

When handling requests, the server should return a 415 status code if the "Content-Type" header in the request specifies a format that is not supported. I would add something here to state that if no Content-Type header is present then it can be assumed to be application/json.

it should return a 406 status code if none of the content types specified in the "Accept" header of the request are supported or alternatively, default to return data in JSON format. I'm not a fan of the either/or here, I'd be inclined to change this to be "...supported; if no Accept header is provided then it is assumed to be application/json."

This should allow us to retain compatibility with existing implementations that don't provide content information, whilst still allowing SSZ to be made available for individual endpoints.

nflaig commented 2 months ago

This should allow us to retain compatibility with existing implementations that don't provide content information, whilst still allowing SSZ to be made available for individual endpoints.

Based on my interop testing, all clients provide content type information in both the request and response.

I would add something here to state that if no Content-Type header is present then it can be assumed to be application/json.

The Accept header can be omitted but imho if a client does not provide a Conent-Type header in a request with a body the best it to just error, most HTTP servers likely do this by default already. I would like to avoid adding a note to the spec that advocates for something like this.

The existing note also already clarified this and has been added 3 years ago. We might wanna specifically mention that responses should also add a Content-type header as well, but I am not sure if it's worth repeating what's already expected of a well-behaved HTTP server and rather keep the notes on the spec more specific to the beacon api.

nflaig commented 2 months ago

If clients already don't have 406 implemented, its likely that it'll just be a json response if you request SSZ for example, because they probably dont look at the header anyway...

yeah, 406 is likely the least supported by clients right now but it's not as important as we can just use a q-value weighted Accept header. It could become more relevant if at some point we wanna adopt SSZ-only clients as suggested by @arnetheduck as you would be able to determine compatibility with beacon node but even there it would be sufficient to check the content type of response.

Current state of clients it pretty good though in terms of what's noted in the PR

We might not get to the point where all clients strictly follow this for all APIs, but using tools like Kurtosis it's pretty easy to find compatibility issues.