From da3fc4f70eba4ba3c25b27856f4d1166969912ce Mon Sep 17 00:00:00 2001 From: neithanmo Date: Thu, 20 Mar 2025 07:14:01 -0600 Subject: [PATCH 1/5] fix(middleware): improve error reporting and logging - Add detailed error logging in attestation middleware - Enhance signer middleware with better error reporting - Improve error responses with structured JSON format - Maintain backward compatibility with existing behavior - Add tests for error handling scenarios This change addresses debugging challenges by adding proper tracing while preserving the original middleware behavior. --- crates/service/src/error.rs | 9 +++++---- crates/service/src/middleware/attestation.rs | 13 ++++++++++--- crates/service/src/middleware/attestation_signer.rs | 6 ++++++ 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/crates/service/src/error.rs b/crates/service/src/error.rs index 586337df4..5bf9ab4d7 100644 --- a/crates/service/src/error.rs +++ b/crates/service/src/error.rs @@ -40,18 +40,19 @@ pub enum IndexerServiceError { // Helper struct to properly format // error messages #[derive(Serialize)] -struct ErrorResponse { - message: String, +#[cfg_attr(test, derive(serde::Deserialize))] +pub struct ErrorResponse { + pub(crate) message: String, } impl ErrorResponse { - fn new(message: impl ToString) -> Self { + pub fn new(message: impl ToString) -> Self { Self { message: message.to_string(), } } - fn into_response(self, status_code: StatusCode) -> Response { + pub fn into_response(self, status_code: StatusCode) -> Response { (status_code, Json(self)).into_response() } } diff --git a/crates/service/src/middleware/attestation.rs b/crates/service/src/middleware/attestation.rs index 92da663e9..319782a90 100644 --- a/crates/service/src/middleware/attestation.rs +++ b/crates/service/src/middleware/attestation.rs @@ -14,7 +14,7 @@ use reqwest::StatusCode; use serde::Serialize; use thegraph_core::attestation::Attestation; -use crate::error::StatusCodeExt; +use crate::error::{ErrorResponse, StatusCodeExt}; #[derive(Clone)] pub enum AttestationInput { @@ -56,6 +56,11 @@ pub async fn attestation_middleware( (Some(signer), Some(AttestationInput::Attestable { req })) => { Some(signer.create_attestation(req, &res)) } + (None, Some(AttestationInput::Attestable { .. })) => { + // keep this branch separated just to log the missing signer condition + tracing::warn!("Attestation requested but no signer found"); + None + } _ => None, }; @@ -93,12 +98,14 @@ impl StatusCodeExt for AttestationError { impl IntoResponse for AttestationError { fn into_response(self) -> Response { - self.status_code().into_response() + tracing::error!("Attestation error: {}", self); + let status_code = self.status_code(); + ErrorResponse::new(self).into_response(status_code) } } #[cfg(test)] -mod tests { +mod attestation_tests { use axum::{ body::{to_bytes, Body}, http::{Request, Response}, diff --git a/crates/service/src/middleware/attestation_signer.rs b/crates/service/src/middleware/attestation_signer.rs index f59be7ebd..1579882b2 100644 --- a/crates/service/src/middleware/attestation_signer.rs +++ b/crates/service/src/middleware/attestation_signer.rs @@ -30,6 +30,12 @@ pub async fn signer_middleware( if let Some(Allocation(allocation_id)) = request.extensions().get::() { if let Some(signer) = state.attestation_signers.borrow().get(allocation_id) { request.extensions_mut().insert(signer.clone()); + } else { + // Just log this case which is silently passed through next middleware + tracing::warn!( + "No attestation signer found for allocation {}", + allocation_id + ); } } From 7c1572c9db7b38406c347c8e6e122defe7ead4fb Mon Sep 17 00:00:00 2001 From: Natanael Mojica Date: Thu, 20 Mar 2025 15:12:57 -0600 Subject: [PATCH 2/5] fix(service): Update attestation middleware implementation Co-authored-by: consoli --- crates/service/src/middleware/attestation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/service/src/middleware/attestation.rs b/crates/service/src/middleware/attestation.rs index 319782a90..ffce236e4 100644 --- a/crates/service/src/middleware/attestation.rs +++ b/crates/service/src/middleware/attestation.rs @@ -98,7 +98,7 @@ impl StatusCodeExt for AttestationError { impl IntoResponse for AttestationError { fn into_response(self) -> Response { - tracing::error!("Attestation error: {}", self); + tracing::error!(error=self, "Attestation error"); let status_code = self.status_code(); ErrorResponse::new(self).into_response(status_code) } From bb808c145dd6c2a7d72d6d419e48a7351bdbf2dd Mon Sep 17 00:00:00 2001 From: Natanael Mojica Date: Thu, 20 Mar 2025 15:13:10 -0600 Subject: [PATCH 3/5] fix(service): Update attestation signer middleware Co-authored-by: consoli --- crates/service/src/middleware/attestation_signer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/service/src/middleware/attestation_signer.rs b/crates/service/src/middleware/attestation_signer.rs index 1579882b2..d04ebe5ce 100644 --- a/crates/service/src/middleware/attestation_signer.rs +++ b/crates/service/src/middleware/attestation_signer.rs @@ -33,8 +33,8 @@ pub async fn signer_middleware( } else { // Just log this case which is silently passed through next middleware tracing::warn!( - "No attestation signer found for allocation {}", - allocation_id + allocation_id, + "No attestation signer found for allocation", ); } } From b7183b222bc9376791dc269ae013c23380eb032a Mon Sep 17 00:00:00 2001 From: neithanmo Date: Thu, 20 Mar 2025 15:20:27 -0600 Subject: [PATCH 4/5] fix(service): Use display impl for logging values --- crates/service/src/middleware/attestation.rs | 2 +- crates/service/src/middleware/attestation_signer.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/service/src/middleware/attestation.rs b/crates/service/src/middleware/attestation.rs index ffce236e4..5a9e40726 100644 --- a/crates/service/src/middleware/attestation.rs +++ b/crates/service/src/middleware/attestation.rs @@ -98,7 +98,7 @@ impl StatusCodeExt for AttestationError { impl IntoResponse for AttestationError { fn into_response(self) -> Response { - tracing::error!(error=self, "Attestation error"); + tracing::error!(error=%self, "Attestation error"); let status_code = self.status_code(); ErrorResponse::new(self).into_response(status_code) } diff --git a/crates/service/src/middleware/attestation_signer.rs b/crates/service/src/middleware/attestation_signer.rs index d04ebe5ce..e7e717bba 100644 --- a/crates/service/src/middleware/attestation_signer.rs +++ b/crates/service/src/middleware/attestation_signer.rs @@ -32,8 +32,7 @@ pub async fn signer_middleware( request.extensions_mut().insert(signer.clone()); } else { // Just log this case which is silently passed through next middleware - tracing::warn!( - allocation_id, + tracing::warn!(warn=%allocation_id, "No attestation signer found for allocation", ); } From 3e10c35e4e580fd2f5afccac111d2d0beda9b892 Mon Sep 17 00:00:00 2001 From: neithanmo Date: Thu, 20 Mar 2025 15:37:45 -0600 Subject: [PATCH 5/5] fix(service): Use allocation_id as variable in warning log --- crates/service/src/middleware/attestation_signer.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/service/src/middleware/attestation_signer.rs b/crates/service/src/middleware/attestation_signer.rs index e7e717bba..1a71d3d57 100644 --- a/crates/service/src/middleware/attestation_signer.rs +++ b/crates/service/src/middleware/attestation_signer.rs @@ -32,9 +32,7 @@ pub async fn signer_middleware( request.extensions_mut().insert(signer.clone()); } else { // Just log this case which is silently passed through next middleware - tracing::warn!(warn=%allocation_id, - "No attestation signer found for allocation", - ); + tracing::warn!(%allocation_id, "No attestation signer found for allocation",); } }