Skip to content
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

Clarify server behavior if content type is not supported #457

Merged
merged 10 commits into from
Sep 11, 2024

Conversation

nflaig
Copy link
Collaborator

@nflaig nflaig commented Jul 8, 2024

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 #250

@mcdee
Copy link
Contributor

mcdee commented Jul 8, 2024

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
Copy link
Collaborator Author

nflaig commented Jul 8, 2024

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.

Copy link
Contributor

@mcdee mcdee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but its an interesting one.
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...
anyway, i agree with the intent, it's just not that simple to have predictability in this.

@nflaig
Copy link
Collaborator Author

nflaig commented Jul 11, 2024

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

  • Content-Type header set by all in requests and responses
  • 415 if content type not supported (all clients but Nimbus support this right now)
  • Accept header with multiple entries and quality values, all client seem to properly handle those

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.

@rolfyone rolfyone merged commit bb60e92 into master Sep 11, 2024
3 checks passed
@rolfyone rolfyone deleted the nflaig/clarify-status-codes branch September 11, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants