From 4cbdcf6ca230a5eabe304026b3c8fb6a0e002708 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 3 May 2021 11:19:50 -0700 Subject: [PATCH 1/3] bug: handle bad responses from ADM ADM can sometimes return invalid content (e.g. a 200 response with no "tiles". Report this to sentry, and log, but we return an "empty" value to the client. Closes #54 --- src/adm.rs | 11 ++++++----- src/error.rs | 4 ++++ src/web/handlers.rs | 43 ++++++++++++++++++++++++++++++++----------- 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/adm.rs b/src/adm.rs index 49d56477..a7506a79 100644 --- a/src/adm.rs +++ b/src/adm.rs @@ -1,14 +1,14 @@ use serde::{Deserialize, Serialize}; use url::Url; -use crate::error::HandlerError; +use crate::error::{HandlerError, HandlerErrorKind}; #[derive(Debug, Deserialize, Serialize)] pub struct AdmTileResponse { pub tiles: Vec, } -#[derive(Debug, Deserialize, Serialize)] +#[derive(Debug, Default, Deserialize, Serialize)] pub struct AdmTile { pub id: u64, pub name: String, @@ -44,8 +44,6 @@ pub async fn get_tiles( let adm_url = adm_url.as_str(); trace!("get_tiles GET {}", adm_url); - // XXX: handle empty responses -- AdM sends empty json in that case - // 'Error("missing field `tiles`"' let mut response: AdmTileResponse = reqwest_client .get(adm_url) .header(reqwest::header::USER_AGENT, stripped_ua) @@ -53,7 +51,10 @@ pub async fn get_tiles( .await? .error_for_status()? .json() - .await?; + .await + .map_err(|e| { + HandlerErrorKind::BadAdmResponse(format!("Returned invalid response: {:?}", e)) + })?; response.tiles = response .tiles .into_iter() diff --git a/src/error.rs b/src/error.rs index 0f16c909..f2cfef82 100644 --- a/src/error.rs +++ b/src/error.rs @@ -38,6 +38,9 @@ pub enum HandlerErrorKind { #[error("Location error: {:?}", _0)] Location(String), + + #[error("Bad Adm response: {:?}", _0)] + BadAdmResponse(String), } impl HandlerErrorKind { @@ -55,6 +58,7 @@ impl HandlerErrorKind { HandlerErrorKind::General(_) => 500, HandlerErrorKind::Internal(_) => 510, HandlerErrorKind::Reqwest(_) => 520, + HandlerErrorKind::BadAdmResponse(_) => 521, HandlerErrorKind::Validation(_) => 600, HandlerErrorKind::Location(_) => 530, } diff --git a/src/web/handlers.rs b/src/web/handlers.rs index 99936918..e9bcc2c5 100644 --- a/src/web/handlers.rs +++ b/src/web/handlers.rs @@ -10,6 +10,7 @@ use crate::{ server::{cache, ServerState}, tags::Tags, web::extractors::TilesRequest, + web::middleware::sentry as l_sentry, }; pub async fn get_image( @@ -90,24 +91,44 @@ pub async fn get_tiles( .body(&tiles.json)); } - let response = adm::get_tiles( + let tiles = match adm::get_tiles( &state.reqwest_client, &state.adm_endpoint_url, fake_ip, &stripped_ua, &treq.placement, ) - .await?; - let tiles = serde_json::to_string(&response) - .map_err(|e| HandlerError::internal(&format!("Response failed to serialize: {}", e)))?; - trace!("get_tiles: cache miss: {:?}", audience_key); - metrics.incr("tiles_cache.miss"); - state.tiles_cache.write().await.insert( - audience_key, - cache::Tiles { - json: tiles.clone(), + .await + { + Ok(response) => { + // adM sometimes returns an invalid response. We don't want to cache that. + let tiles = serde_json::to_string(&response).map_err(|e| { + HandlerError::internal(&format!("Response failed to serialize: {}", e)) + })?; + trace!("get_tiles: cache miss: {:?}", audience_key); + metrics.incr("tiles_cache.miss"); + state.tiles_cache.write().await.insert( + audience_key, + cache::Tiles { + json: tiles.clone(), + }, + ); + tiles + } + Err(e) => match e.kind() { + HandlerErrorKind::BadAdmResponse(es) => { + warn!("Bad response from ADM: {:?}", e); + // Report directly to sentry + // (This is starting to become a pattern. 🤔) + let mut tags = Tags::from(request.head()); + tags.add_extra("err", es); + l_sentry::report(&tags, sentry::event_from_error(&e)); + //TODO: probably should do: json!(vec![adm::AdmTile::default()]).to_string() + "[]".to_owned() + } + _ => return Err(e), }, - ); + }; Ok(HttpResponse::Ok() .content_type("application/json") From 2203acc96887226b0c0e34e6f9fd764f584cff2c Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 3 May 2021 12:49:32 -0700 Subject: [PATCH 2/3] f handle server errors --- src/adm.rs | 9 +++++++-- src/web/handlers.rs | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/adm.rs b/src/adm.rs index a7506a79..a78c863c 100644 --- a/src/adm.rs +++ b/src/adm.rs @@ -48,12 +48,17 @@ pub async fn get_tiles( .get(adm_url) .header(reqwest::header::USER_AGENT, stripped_ua) .send() - .await? + .await + .map_err(|e| { + // ADM servers are down, or improperly configured + HandlerErrorKind::BadAdmResponse(format!("ADM Server Error: {:?}", e)) + })? .error_for_status()? .json() .await .map_err(|e| { - HandlerErrorKind::BadAdmResponse(format!("Returned invalid response: {:?}", e)) + // ADM servers are not returning correct information + HandlerErrorKind::BadAdmResponse(format!("ADM provided invalid response: {:?}", e)) })?; response.tiles = response .tiles diff --git a/src/web/handlers.rs b/src/web/handlers.rs index e9bcc2c5..252244c8 100644 --- a/src/web/handlers.rs +++ b/src/web/handlers.rs @@ -122,8 +122,10 @@ pub async fn get_tiles( // (This is starting to become a pattern. 🤔) let mut tags = Tags::from(request.head()); tags.add_extra("err", es); + tags.add_tag("level", "warning"); l_sentry::report(&tags, sentry::event_from_error(&e)); //TODO: probably should do: json!(vec![adm::AdmTile::default()]).to_string() + warn!("ADM Server error: {:?}", e); "[]".to_owned() } _ => return Err(e), From 7adad080763c78339211ef4bbe1458df2aa3679f Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 3 May 2021 16:13:21 -0700 Subject: [PATCH 3/3] f r Return a better formatted empty JSON string --- src/web/handlers.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/web/handlers.rs b/src/web/handlers.rs index 252244c8..bf08bcfe 100644 --- a/src/web/handlers.rs +++ b/src/web/handlers.rs @@ -1,6 +1,7 @@ //! API Handlers use actix_http::http::Uri; use actix_web::{web, HttpRequest, HttpResponse}; +use serde_json::json; use super::user_agent; use crate::{ @@ -126,7 +127,7 @@ pub async fn get_tiles( l_sentry::report(&tags, sentry::event_from_error(&e)); //TODO: probably should do: json!(vec![adm::AdmTile::default()]).to_string() warn!("ADM Server error: {:?}", e); - "[]".to_owned() + json!({"tiles":[]}).to_string() } _ => return Err(e), },