Return HTTP status code 415 from endpoints that expect JSON (#26)

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.

http_api/src/error.rs

@@ -3,10 +3,12 @@ use std::error::Error as StdError;
 
 use anyhow::Error as AnyhowError;
 use axum::{
+    extract::rejection::JsonRejection,
     http::StatusCode,
     response::{IntoResponse, Response},
     Extension, Json,
 };
+use axum_extra::extract::QueryRejection;
 use bls::SignatureBytes;
 use futures::channel::oneshot::Canceled;
 use itertools::Itertools as _;
@@ -69,7 +71,7 @@ pub enum Error {
     #[error("invalid epoch")]
     InvalidEpoch(#[source] AnyhowError),
     #[error("invalid JSON body")]
-    InvalidJsonBody(#[source] AnyhowError),
+    InvalidJsonBody(#[source] JsonRejection),
     #[error("invalid peer ID")]
     InvalidPeerId(#[source] AnyhowError),
     #[error(
@@ -84,13 +86,13 @@ pub enum Error {
     #[error("invalid proposer slashing, it will never pass validation so it's rejected")]
     InvalidProposerSlashing(#[source] AnyhowError),
     #[error("invalid query string")]
-    InvalidQuery(#[source] AnyhowError),
+    InvalidQuery(#[source] QueryRejection),
     #[error("invalid voluntary exit, it will never pass validation so it's rejected")]
     InvalidSignedVoluntaryExit(#[source] AnyhowError),
     #[error("invalid sync committee messages")]
     InvalidSyncCommitteeMessages(Vec<IndexedError>),
-    #[error("invalid validator index")]
-    InvalidValidatorIndex(#[source] AnyhowError),
+    #[error("invalid validator indices")]
+    InvalidValidatorIndices(#[source] JsonRejection),
     #[error("invalid validator signatures")]
     InvalidValidatorSignatures(Vec<IndexedError>),
     #[error("invalid BLS to execution changes")]
@@ -160,16 +162,26 @@ impl Error {
 
     // `StdError::sources` is not stable as of Rust 1.78.0.
     fn sources(&self) -> impl Iterator<Item = &dyn StdError> {
-        let mut error: Option<&dyn StdError> = Some(self);
+        let first: &dyn StdError = self;
 
-        core::iter::from_fn(move || {
-            let source = error?.source();
-            core::mem::replace(&mut error, source)
-        })
+        let skip_duplicates = || match self {
+            Self::InvalidJsonBody(_) | Self::InvalidValidatorIndices(_) => first.source()?.source(),
+            Self::InvalidQuery(_) => first.source()?.source()?.source(),
+            _ => first.source(),
+        };
+
+        let mut next = skip_duplicates();
+
+        core::iter::once(first).chain(core::iter::from_fn(move || {
+            let source = next?.source();
+            core::mem::replace(&mut next, source)
+        }))
     }
 
-    const fn status_code(&self) -> StatusCode {
+    fn status_code(&self) -> StatusCode {
         match self {
+            Self::InvalidJsonBody(json_rejection)
+            | Self::InvalidValidatorIndices(json_rejection) => json_rejection.status(),
             Self::AttestationNotFound
             | Self::BlockNotFound
             | Self::MatchingAttestationHeadBlockNotFound
@@ -191,7 +203,6 @@ impl Error {
             | Self::InvalidBlockId(_)
             | Self::InvalidContributionAndProofs(_)
             | Self::InvalidEpoch(_)
-            | Self::InvalidJsonBody(_)
             | Self::InvalidQuery(_)
             | Self::InvalidPeerId(_)
             | Self::InvalidProposerSlashing(_)
@@ -201,7 +212,6 @@ impl Error {
             | Self::InvalidSyncCommitteeMessages(_)
             | Self::InvalidRandaoReveal
             | Self::InvalidValidatorId(_)
-            | Self::InvalidValidatorIndex(_)
             | Self::InvalidValidatorSignatures(_)
             | Self::ProposalSlotNotLaterThanStateSlot
             | Self::SlotNotInEpoch
@@ -266,6 +276,7 @@ struct EthErrorResponse<'error> {
 #[allow(clippy::needless_pass_by_value)]
 #[cfg(test)]
 mod tests {
+    use axum::{extract::rejection::MissingJsonContentType, Error as AxumError};
     use serde_json::{json, Result, Value};
     use test_case::test_case;
 
@@ -299,4 +310,34 @@ mod tests {
         assert_eq!(actual_json, expected_json);
         Ok(())
     }
+
+    // `axum::extract::rejection::JsonRejection` duplicates error sources.
+    // The underlying bug in `axum-core` was fixed in <https://github.com/tokio-rs/axum/pull/2030>.
+    // The fix was backported in <https://github.com/tokio-rs/axum/pull/2098> but not released.
+    #[test]
+    fn error_sources_does_not_yield_duplicates_from_json_rejection() {
+        let error = Error::InvalidJsonBody(JsonRejection::from(MissingJsonContentType::default()));
+
+        assert_eq!(
+            error.sources().map(ToString::to_string).collect_vec(),
+            [
+                "invalid JSON body",
+                "Expected request with `Content-Type: application/json`",
+            ],
+        );
+    }
+
+    // `axum_extra::extract::QueryRejection` and `axum::Error` duplicate error sources.
+    // `axum::Error` has not been fixed in any version yet.
+    // `axum::extract::rejection::QueryRejection` is even worse and harder to work around.
+    #[test]
+    fn error_sources_does_not_yield_triplicates_from_query_rejection() {
+        let axum_error = AxumError::new("error");
+        let error = Error::InvalidQuery(QueryRejection::FailedToDeserializeQueryString(axum_error));
+
+        assert_eq!(
+            error.sources().map(ToString::to_string).collect_vec(),
+            ["invalid query string", "error"],
+        );
+    }
 }

http_api/src/extractors.rs

@@ -160,7 +160,6 @@ impl<S, T: DeserializeOwned + 'static> FromRequestParts<S> for EthQuery<T> {
             .extract()
             .await
             .map(|Query(query)| Self(query))
-            .map_err(AnyhowError::msg)
             .map_err(Error::InvalidQuery)
     }
 }
@@ -177,8 +176,7 @@ impl<S> FromRequest<S, Body> for EthJson<Box<ProposerSlashing>> {
             .extract()
             .await
             .map(|Json(slashing)| Self(slashing))
-            .map_err(AnyhowError::new)
-            .map_err(Error::InvalidProposerSlashing)
+            .map_err(Error::InvalidJsonBody)
     }
 }
 
@@ -191,8 +189,7 @@ impl<S> FromRequest<S, Body> for EthJson<Box<SignedVoluntaryExit>> {
             .extract()
             .await
             .map(|Json(slashing)| Self(slashing))
-            .map_err(AnyhowError::new)
-            .map_err(Error::InvalidSignedVoluntaryExit)
+            .map_err(Error::InvalidJsonBody)
     }
 }
 
@@ -205,8 +202,7 @@ impl<S, P: Preset> FromRequest<S, Body> for EthJson<Box<AttesterSlashing<P>>> {
             .extract()
             .await
             .map(|Json(slashing)| Self(slashing))
-            .map_err(AnyhowError::new)
-            .map_err(Error::InvalidAttesterSlashing)
+            .map_err(Error::InvalidJsonBody)
     }
 }
 
@@ -219,7 +215,6 @@ impl<S, P: Preset> FromRequest<S, Body> for EthJson<Vec<Arc<Attestation<P>>>> {
             .extract()
             .await
             .map(|Json(attestation)| Self(attestation))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -233,7 +228,6 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<Value>> {
             .extract()
             .await
             .map(|Json(values)| Self(values))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -250,8 +244,7 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<ValidatorIndex>> {
             .extract()
             .await
             .map(|Json(Wrapper(indices))| Self(indices))
-            .map_err(AnyhowError::new)
-            .map_err(Error::InvalidValidatorIndex)
+            .map_err(Error::InvalidValidatorIndices)
     }
 }
 
@@ -264,8 +257,7 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<ValidatorId>> {
             .extract()
             .await
             .map(|Json(indices)| Self(indices))
-            .map_err(AnyhowError::new)
-            .map_err(Error::InvalidValidatorId)
+            .map_err(Error::InvalidJsonBody)
     }
 }
 
@@ -278,7 +270,6 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<SyncCommitteeSubscription>> {
             .extract()
             .await
             .map(|Json(subscription)| Self(subscription))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -292,7 +283,6 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<BeaconCommitteeSubscription>> {
             .extract()
             .await
             .map(|Json(subscription)| Self(subscription))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -306,7 +296,6 @@ impl<S, P: Preset> FromRequest<S, Body> for EthJson<Vec<Arc<SignedAggregateAndPr
             .extract()
             .await
             .map(|Json(aggregate_and_proof)| Self(aggregate_and_proof))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -320,7 +309,6 @@ impl<S, P: Preset> FromRequest<S, Body> for EthJson<Vec<SignedContributionAndPro
             .extract()
             .await
             .map(|Json(contribution_and_proof)| Self(contribution_and_proof))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -334,7 +322,6 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<ValidatorProposerData>> {
             .extract()
             .await
             .map(|Json(proposer_data)| Self(proposer_data))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }
@@ -348,7 +335,6 @@ impl<S> FromRequest<S, Body> for EthJson<Vec<SignedValidatorRegistrationV1>> {
             .extract()
             .await
             .map(|Json(registrations)| Self(registrations))
-            .map_err(AnyhowError::new)
             .map_err(Error::InvalidJsonBody)
     }
 }

validator/Cargo.toml

@@ -7,9 +7,10 @@ authors = ["Grandine <[email protected]>"]
 workspace = true
 
 [dependencies]
-axum = { workspace = true }
 anyhow = { workspace = true }
 arithmetic = { workspace = true }
+axum = { workspace = true }
+axum-extra = { workspace = true }
 bls = { workspace = true }
 builder_api = { workspace = true }
 cached = { workspace = true }