feat: Handle empty bso body on /meta/global

This adds some additional debug around handling BSOs to try and identify
the source of the server returning 400 on PUT `/1.5/xxx/storage/meta/global`.
There may be a related client bug where it's sending no BSO on a
meta/global PUT, thus the very specific fix.

* also new clippy, new crap to clean up.

Issue #342

src/db/spanner/models.rs

@@ -739,8 +739,8 @@ impl SpannerDb {
             "INSERT INTO user_collections (fxa_uid, fxa_kid, collection_id, modified)
              VALUES (@fxa_uid, @fxa_kid, @collection_id, @modified)",
         )?
-        .params(params.clone())
-        .param_types(types.clone())
+        .params(params)
+        .param_types(types)
         .execute(&self.conn)?;
         // Return timestamp, because sometimes there's a delay between writing and
         // reading the database.
@@ -928,25 +928,25 @@ impl SpannerDb {
         let mut sqltypes = HashMap::new();
 
         if let Some(older) = older {
-            query = format!("{} AND modified < @older", query).to_string();
+            query = format!("{} AND modified < @older", query);
             sqlparams.insert("older".to_string(), as_value(older.as_rfc3339()?));
             sqltypes.insert("older".to_string(), as_type(TypeCode::TIMESTAMP));
         }
         if let Some(newer) = newer {
-            query = format!("{} AND modified > @newer", query).to_string();
+            query = format!("{} AND modified > @newer", query);
             sqlparams.insert("newer".to_string(), as_value(newer.as_rfc3339()?));
             sqltypes.insert("newer".to_string(), as_type(TypeCode::TIMESTAMP));
         }
 
         if !ids.is_empty() {
-            query = format!("{} AND bso_id IN UNNEST(@ids)", query).to_string();
+            query = format!("{} AND bso_id IN UNNEST(@ids)", query);
             sqlparams.insert("ids".to_owned(), as_list_value(ids.into_iter()));
         }
 
         query = match sort {
-            Sorting::Index => format!("{} ORDER BY sortindex DESC", query).to_string(),
-            Sorting::Newest => format!("{} ORDER BY modified DESC", query).to_string(),
-            Sorting::Oldest => format!("{} ORDER BY modified ASC", query).to_string(),
+            Sorting::Index => format!("{} ORDER BY sortindex DESC", query),
+            Sorting::Newest => format!("{} ORDER BY modified DESC", query),
+            Sorting::Oldest => format!("{} ORDER BY modified ASC", query),
             _ => query,
         };
 
@@ -967,7 +967,7 @@ impl SpannerDb {
         if offset != 0 {
             // XXX: copy over this optimization:
             // https://github.com/mozilla-services/server-syncstorage/blob/a0f8117/syncstorage/storage/sql/__init__.py#L404
-            query = format!("{} OFFSET {}", query, offset).to_string();
+            query = format!("{} OFFSET {}", query, offset);
         }
 
         self.sql(&query)?

src/db/spanner/pool.rs

@@ -82,7 +82,7 @@ impl SpannerDbPool {
                     .build(),
             ),
             coll_cache: Default::default(),
-            metrics: metrics.clone(),
+            metrics,
         })
     }
 

src/settings.rs

@@ -112,7 +112,7 @@ impl Settings {
             Ok(s) => {
                 // Adjust the max values if required.
                 if s.uses_spanner() {
-                    let mut ms = s.clone();
+                    let mut ms = s;
                     ms.limits.max_total_bytes =
                         min(ms.limits.max_total_bytes, MAX_SPANNER_LOAD_SIZE as u32);
                     return Ok(ms);

src/web/error.rs

@@ -107,6 +107,7 @@ impl From<Context<HawkErrorKind>> for HawkError {
 
 impl From<Context<ValidationErrorKind>> for ValidationError {
     fn from(inner: Context<ValidationErrorKind>) -> Self {
+        debug!("Validation Error: {:?}", inner.get_context());
         let status = match inner.get_context() {
             ValidationErrorKind::FromDetails(ref _description, ref location, Some(ref name))
                 if *location == RequestErrorLocation::Header =>

src/web/extractors.rs

@@ -8,7 +8,7 @@ use actix_web::{
     dev::{ConnectionInfo, Extensions, Payload},
     error::ErrorInternalServerError,
     http::{
-        header::{qitem, Accept, ContentType, Header, HeaderMap},
+        header::{qitem, Accept, ContentType, Header, HeaderMap, HeaderValue},
         Uri,
     },
     web::{Json, Query},
@@ -367,8 +367,29 @@ impl FromRequest for BsoBody {
 
         let max_payload_size = state.limits.max_record_payload_bytes as usize;
 
+        // The `meta/global` BSO is special in that it's used as a command to
+        // reset the timestamp. A possible client bug is sending an empty
+        // body. Handle it, but issue an info so we can track the event.
+        if req.path().ends_with("/meta/global")
+            && req
+                .headers()
+                .get("content-length")
+                .unwrap_or(&HeaderValue::from(0))
+                == HeaderValue::from(0)
+        {
+            info!("⚠️ Got an empty meta BSO");
+            return Box::new(future::ok(BsoBody {
+                id: None,
+                sortindex: None,
+                payload: None,
+                ttl: None,
+                _ignored_modified: None,
+            }));
+        }
+
         let fut = <Json<BsoBody>>::from_request(&req, payload)
             .map_err(|e| {
+                warn!("⚠️ Could not parse BSO Body: {:?}", e);
                 let err: ApiError = ValidationErrorKind::FromDetails(
                     e.to_string(),
                     RequestErrorLocation::Body,
@@ -421,6 +442,7 @@ impl BsoParam {
         let elements: Vec<&str> = uri.path().split('/').collect();
         let elem = elements.get(3);
         if elem.is_none() || elem != Some(&"storage") || elements.len() != 6 {
+            warn!("⚠️ Unexpected BSO URI: {:?}", uri.path());
             return Err(ValidationErrorKind::FromDetails(
                 "Invalid BSO".to_owned(),
                 RequestErrorLocation::Path,
@@ -429,7 +451,7 @@ impl BsoParam {
         }
         if let Some(v) = elements.get(5) {
             let sv = String::from_str(v).map_err(|e| {
-                debug!("⚠️ BsoParam Error element:{:?} error:{:?}", v, e);
+                warn!("⚠️ BsoParam Error element:{:?} error:{:?}", v, e);
                 ValidationErrorKind::FromDetails(
                     "Invalid BSO".to_owned(),
                     RequestErrorLocation::Path,
@@ -438,6 +460,7 @@ impl BsoParam {
             })?;
             Ok(Self { bso: sv })
         } else {
+            warn!("⚠️ Missing BSO: {:?}", uri.path());
             Err(ValidationErrorKind::FromDetails(
                 "Missing BSO".to_owned(),
                 RequestErrorLocation::Path,
@@ -674,7 +697,7 @@ impl FromRequest for CollectionPostRequest {
             BsoBodies,
         )>::from_request(&req, payload)
         .and_then(move |(user_id, db, collection, query, mut bsos)| {
-            let collection = collection.collection.clone();
+            let collection = collection.collection;
             if collection == "crypto" {
                 // Verify the client didn't mess up the crypto if we have a payload
                 for bso in &bsos.valid {
@@ -746,9 +769,7 @@ impl FromRequest for BsoRequest {
         let user_id = HawkIdentifier::from_request(req, payload)?;
         let db = <Box<dyn Db>>::from_request(req, payload)?;
         let query = BsoQueryParams::from_request(req, payload)?;
-        let collection = CollectionParam::from_request(req, payload)?
-            .collection
-            .clone();
+        let collection = CollectionParam::from_request(req, payload)?.collection;
         let bso = BsoParam::from_request(req, payload)?;
 
         Ok(BsoRequest {
@@ -756,7 +777,7 @@ impl FromRequest for BsoRequest {
             db,
             user_id,
             query,
-            bso: bso.bso.clone(),
+            bso: bso.bso,
             metrics: metrics::Metrics::from(req),
         })
     }
@@ -792,7 +813,7 @@ impl FromRequest for BsoPutRequest {
             BsoBody,
         )>::from_request(req, payload)
         .and_then(|(user_id, db, collection, query, bso, body)| {
-            let collection = collection.collection.clone();
+            let collection = collection.collection;
             if collection == "crypto" {
                 // Verify the client didn't mess up the crypto if we have a payload
                 if let Some(ref data) = body.payload {
@@ -813,7 +834,7 @@ impl FromRequest for BsoPutRequest {
                 db,
                 user_id,
                 query,
-                bso: bso.bso.clone(),
+                bso: bso.bso,
                 body,
                 metrics,
             })
@@ -897,7 +918,7 @@ impl HawkIdentifier {
         let elements: Vec<&str> = uri.path().split('/').collect();
         if let Some(v) = elements.get(2) {
             u64::from_str(v).map_err(|e| {
-                debug!("⚠️ HawkIdentifier Error {:?} {:?}", v, e);
+                info!("⚠️ HawkIdentifier Error invalid UID {:?} {:?}", v, e);
                 ValidationErrorKind::FromDetails(
                     "Invalid UID".to_owned(),
                     RequestErrorLocation::Path,
@@ -906,6 +927,7 @@ impl HawkIdentifier {
                 .into()
             })
         } else {
+            info!("⚠️ HawkIdentifier Error missing UID {:?}", uri);
             Err(ValidationErrorKind::FromDetails(
                 "Missing UID".to_owned(),
                 RequestErrorLocation::Path,
@@ -947,7 +969,9 @@ impl HawkIdentifier {
         uri: &Uri,
     ) -> Result<Self, Error> {
         let payload = HawkPayload::extrude(header, method, secrets, connection_info, uri)?;
-        if payload.user_id != Self::uid_from_path(&uri)? {
+        let puid = Self::uid_from_path(&uri)?;
+        if payload.user_id != puid {
+            info!("⚠️ Hawk UID not in URI: {:?} {:?}", payload.user_id, uri);
             Err(ValidationErrorKind::FromDetails(
                 "conflicts with payload".to_owned(),
                 RequestErrorLocation::Path,
@@ -1143,7 +1167,7 @@ impl FromRequest for BatchRequestOpt {
                 })?,
                 None => continue,
             };
-            let count = value.parse::<(u32)>().map_err(|_| {
+            let count = value.parse::<u32>().map_err(|_| {
                 let err: ApiError = ValidationErrorKind::FromDetails(
                     format!("Invalid integer value: {}", value),
                     RequestErrorLocation::Header,

src/web/middleware.rs

@@ -402,7 +402,7 @@ where
             }
         };
         let bso = BsoParam::extrude(&sreq.uri(), &mut sreq.extensions_mut()).ok();
-        let bso_opt = bso.clone().map(|b| b.bso);
+        let bso_opt = bso.map(|b| b.bso);
 
         let mut service = Rc::clone(&self.service);
         Box::new(