fix: return 204 no contents for cache hits

Closes #191

src/server/cache.rs

@@ -9,7 +9,7 @@ use std::{
 use cadence::StatsdClient;
 use dashmap::DashMap;
 
-use crate::{metrics::Metrics, web::FormFactor};
+use crate::{adm::TileResponse, error::HandlerError, metrics::Metrics, web::FormFactor};
 
 /// AudienceKey is the primary key used to store and fetch tiles from the
 /// local cache.
@@ -55,16 +55,29 @@ impl Deref for TilesCache {
     }
 }
 
-#[derive(Debug)]
+#[derive(Clone, Debug)]
 pub struct Tiles {
-    pub json: String,
+    pub content: TilesContent,
     expiry: SystemTime,
 }
 
 impl Tiles {
-    pub fn new(json: String, ttl: u32) -> Self {
+    pub fn new(tile_response: TileResponse, ttl: u32) -> Result<Self, HandlerError> {
+        let empty = Self::empty(ttl);
+        if tile_response.tiles.is_empty() {
+            return Ok(empty);
+        }
+        let json = serde_json::to_string(&tile_response)
+            .map_err(|e| HandlerError::internal(&format!("Response failed to serialize: {}", e)))?;
+        Ok(Self {
+            content: TilesContent::Json(json),
+            ..empty
+        })
+    }
+
+    fn empty(ttl: u32) -> Self {
         Self {
-            json,
+            content: TilesContent::Empty,
             expiry: SystemTime::now() + Duration::from_secs(ttl as u64),
         }
     }
@@ -74,6 +87,21 @@ impl Tiles {
     }
 }
 
+#[derive(Clone, Debug)]
+pub enum TilesContent {
+    Json(String),
+    Empty,
+}
+
+impl TilesContent {
+    fn size(&self) -> usize {
+        match self {
+            Self::Json(json) => json.len(),
+            _ => 0,
+        }
+    }
+}
+
 async fn tiles_cache_periodic_reporter(cache: &TilesCache, metrics: &Metrics) {
     trace!("tiles_cache_periodic_reporter");
     // calculate the size and GC (for seldomly used Tiles) while we're at it
@@ -82,7 +110,7 @@ async fn tiles_cache_periodic_reporter(cache: &TilesCache, metrics: &Metrics) {
     cache.retain(|_, tiles| {
         if !tiles.expired() {
             cache_count += 1;
-            cache_size += tiles.json.len();
+            cache_size += tiles.content.size();
             return true;
         }
         false

src/web/handlers.rs

@@ -45,9 +45,7 @@ pub async fn get_tiles(
             if !expired {
                 trace!("get_tiles: cache hit: {:?}", audience_key);
                 metrics.incr("tiles_cache.hit");
-                return Ok(HttpResponse::Ok()
-                    .content_type("application/json")
-                    .body(&tiles.json));
+                return Ok(content_response(&tiles.content, &metrics));
             }
         }
     }
@@ -67,29 +65,17 @@ pub async fn get_tiles(
     )
     .await;
 
-    let json = match result {
+    match result {
         Ok(response) => {
-            let json = serde_json::to_string(&response).map_err(|e| {
-                HandlerError::internal(&format!("Response failed to serialize: {}", e))
-            })?;
-
+            let tiles = cache::Tiles::new(response, state.settings.tiles_ttl)?;
             trace!(
                 "get_tiles: cache miss{}: {:?}",
                 if expired { " (expired)" } else { "" },
                 &audience_key
             );
             metrics.incr("tiles_cache.miss");
-            state.tiles_cache.insert(
-                audience_key,
-                cache::Tiles::new(json.clone(), state.settings.tiles_ttl),
-            );
-
-            if response.tiles.is_empty() {
-                metrics.incr_with_tags("tiles.empty", Some(&tags));
-                return Ok(HttpResponse::NoContent().finish());
-            };
-
-            json
+            state.tiles_cache.insert(audience_key, tiles.clone());
+            Ok(content_response(&tiles.content, &metrics))
         }
         Err(e) => {
             match e.kind() {
@@ -102,16 +88,23 @@ pub async fn get_tiles(
                     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);
-                    return Ok(HttpResponse::NoContent().finish());
+                    Ok(HttpResponse::NoContent().finish())
                 }
-                _ => return Err(e),
-            };
+                _ => Err(e),
+            }
         }
-    };
+    }
+}
 
-    Ok(HttpResponse::Ok()
-        .content_type("application/json")
-        .body(json))
+fn content_response(content: &cache::TilesContent, metrics: &Metrics) -> HttpResponse {
+    match content {
+        cache::TilesContent::Json(json) => HttpResponse::Ok()
+            .content_type("application/json")
+            .body(json),
+        cache::TilesContent::Empty => {
+            metrics.incr("tiles.empty");
+            HttpResponse::NoContent().finish()
+        }
+    }
 }

src/web/test.rs

@@ -436,3 +436,29 @@ async fn maxmind_lookup() {
     assert_eq!(params.get("country-code"), Some(&"US".to_owned()));
     assert_eq!(params.get("region-code"), Some(&"WA".to_owned()));
 }
+
+#[actix_rt::test]
+async fn empty_tiles() {
+    let adm = init_mock_adm(MOCK_RESPONSE1.to_owned());
+    // no adm_settings filters everything out
+    let mut settings = Settings {
+        adm_endpoint_url: adm.endpoint_url.clone(),
+        ..get_test_settings()
+    };
+    let mut app = init_app!(settings).await;
+
+    let req = test::TestRequest::get()
+        .uri("/v1/tiles")
+        .header(header::USER_AGENT, UA)
+        .to_request();
+    let resp = test::call_service(&mut app, req).await;
+    assert_eq!(resp.status(), StatusCode::NO_CONTENT);
+
+    // Ensure same result from cache
+    let req = test::TestRequest::get()
+        .uri("/v1/tiles")
+        .header(header::USER_AGENT, UA)
+        .to_request();
+    let resp = test::call_service(&mut app, req).await;
+    assert_eq!(resp.status(), StatusCode::NO_CONTENT);
+}