-
Notifications
You must be signed in to change notification settings - Fork 17
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
Return 415 if content type is not supported #26
Comments
axum::Json already does that, but our EthJson and Error were replacing its status codes with 400. The solution was to store JsonRejection directly in Error and delegate to it in Error::status_code. Doing so also makes endpoints return status codes 413 and 422 when appropriate. The duplicated rejection messages came up again. We were able to determine the cause and come up with a better workaround this time. The issue should be fixed for query strings too.
axum::Json already does that, but our EthJson and Error were replacing its status codes with 400. The solution was to store JsonRejection directly in Error and delegate to it in Error::status_code. Doing so also makes endpoints return status codes 413 and 422 when appropriate. The duplicated rejection messages came up again. We were able to determine the cause and come up with a better workaround this time. The issue should be fixed for query strings too.
axum::Json already does that, but our EthJson and Error were replacing its status codes with 400. The solution was to store JsonRejection directly in Error and delegate to it in Error::status_code. Doing so also makes endpoints return status codes 413 and 422 when appropriate. The duplicated rejection messages came up again. We were able to determine the cause and come up with a better workaround this time. The issue should be fixed for query strings too.
hey @nflaig, did you have the chance to test this? Fix has been merged to develop. |
Do you have an image I can use? |
We don't build |
It looks like EF devops builds an image from your dev branch, I can use that one https://hub.docker.com/layers/ethpandaops/grandine/develop-minimal/images/sha256-5135f0aa0bbc82c4ad487a01cea520a86f2d5c6f9a44d5abb191659e67695fda?context=explore |
Confirmed the fix by running the following config: participants:
- el_type: geth
el_image: ethereum/client-go:stable
cl_type: grandine
cl_image: ethpandaops/grandine:develop-minimal
vc_type: lodestar
vc_image: nflaig/lodestar:post-validators
vc_extra_params:
- --http.requestWireFormat=ssz
count: 4
network_params:
genesis_delay: 60
num_validator_keys_per_node: 64
additional_services:
- assertoor
- dora
snooper_enabled: true
disable_peer_scoring: true
assertoor_params:
image: "ethpandaops/assertoor:master"
run_stability_check: false
run_block_proposal_check: false
tests:
- https://raw.githubusercontent.com/ethpandaops/assertoor-test/2a45f2f78dd2c336ac99bf15e61edc076f15ce67/assertoor-tests/block-proposal-check.yaml Thanks for the fix 🙏 |
I am running some interop tests with Lodestar <> Grandine on a feature branch of Lodestar which uses ssz for almost all apis. I noticed that Grandine does complain about the unsupported content type (as expected) but it returns a 400 error instead of 415.
On the Lodestar side, we check the status code, and if it's a 415 we would retry the request with JSON and cache that ssz is not supported for this route, but since Grandine return a 400 status code, the request just fails.
Note, we will use JSON by default if the spec does not specify that the API should support SSZ but it would still be great to have to fallback mechanism in place in case a user overrides the default behavior to always sent SSZ for all apis.
The text was updated successfully, but these errors were encountered: