bug: report query parameters with Invalid Value error (#1030)

* bug: report query parameters with Invalid Value error

This is not a resolution. Instead, this will report the query parameters
that lead to a serde parse failure so we can better isolate potential
problems.

(also fixes a typo in a logging message extra parameter)

Issue: #1029

src/db/mysql/batch.rs

@@ -179,7 +179,7 @@ pub fn do_append(
             user_id = user_id,
             bso_id = bso_id,
         )
-    };
+    }
 
     // It's possible for the list of items to contain a duplicate key entry.
     // This means that we can't really call `ON DUPLICATE` here, because that's
@@ -192,7 +192,7 @@ pub fn do_append(
         user_id: i64,
         batch_id: i64,
         id: String,
-    };
+    }
 
     #[derive(AsChangeset)]
     #[table_name = "batch_upload_items"]

src/db/mysql/models.rs

@@ -729,7 +729,7 @@ impl MysqlDb {
             .filter(user_collections::user_id.eq(user_id))
             .first::<Option<i64>>(&self.conn)?
             .unwrap_or_default();
-        Ok(SyncTimestamp::from_i64(modified)?)
+        SyncTimestamp::from_i64(modified)
     }
 
     pub fn get_collection_timestamp_sync(
@@ -766,7 +766,7 @@ impl MysqlDb {
             .first::<i64>(&self.conn)
             .optional()?
             .unwrap_or_default();
-        Ok(SyncTimestamp::from_i64(modified)?)
+        SyncTimestamp::from_i64(modified)
     }
 
     pub fn get_collection_timestamps_sync(

src/db/spanner/batch.rs

@@ -277,7 +277,7 @@ pub async fn do_append_async(
         sortindex: Option<i32>,
         payload: Option<String>,
         ttl: Option<u32>,
-    };
+    }
 
     //prefetch the existing batch_bsos for this user's batch.
     let mut existing = HashSet::new();

src/db/transaction.rs

@@ -39,7 +39,7 @@ fn set_extra(exts: &mut RefMut<'_, Extensions>, connection_info: ConnectionInfo)
         &connection_info.spanner_age.to_string(),
     );
     tags.add_extra(
-        "spanner_sconnection_idle",
+        "spanner_connection_idle",
         &connection_info.spanner_idle.to_string(),
     );
     tags.commit(exts);

src/server/test.rs

@@ -329,7 +329,8 @@ async fn delete_collection() {
         &move |result: DeleteBso| {
             assert!(
                 result == SyncTimestamp::from_seconds(0.00),
-                format!("Bad Bookmarks {:?} != 0", result)
+                "Bad Bookmarks {:?} != 0",
+                result
             );
         },
     )
@@ -340,7 +341,9 @@ async fn delete_collection() {
         &move |result: DeleteBso| {
             assert!(
                 result > start,
-                format!("Bad Bookmarks ids {:?} < {:?}", result, start)
+                "Bad Bookmarks ids {:?} < {:?}",
+                result,
+                start
             );
         },
     )
@@ -351,7 +354,9 @@ async fn delete_collection() {
         &move |result: DeleteBso| {
             assert!(
                 result > start,
-                format!("Bad Bookmarks ids, m {:?} < {:?}", result, start)
+                "Bad Bookmarks ids, m {:?} < {:?}",
+                result,
+                start
             );
         },
     )

src/web/extractors.rs

@@ -1121,7 +1121,7 @@ impl From<u32> for HawkIdentifier {
     }
 }
 
-#[derive(Debug, Default, Clone, Deserialize, Validate)]
+#[derive(Debug, Default, Clone, Deserialize, Validate, PartialEq)]
 #[serde(default)]
 pub struct Offset {
     pub timestamp: Option<SyncTimestamp>,
@@ -1130,10 +1130,14 @@ pub struct Offset {
 
 impl ToString for Offset {
     fn to_string(&self) -> String {
+        // issue559: Disable ':' support for now.
+        self.offset.to_string()
+        /*
         match self.timestamp {
-            None => format!("{}", self.offset),
+            None => self.offset.to_string(),
             Some(ts) => format!("{}:{}", ts.as_i64(), self.offset),
         }
+        */
     }
 }
 
@@ -2294,4 +2298,21 @@ mod tests {
         assert_eq!(result.bsos.invalid.len(), 1);
         assert!(result.bsos.invalid.contains_key("789"));
     }
+
+    #[actix_rt::test]
+    async fn test_offset() {
+        let sample_offset = Offset {
+            timestamp: Some(SyncTimestamp::default()),
+            offset: 1234,
+        };
+
+        //Issue559: only use offset, don't use timestamp, even if set.
+        let test_offset = Offset {
+            timestamp: None,
+            offset: sample_offset.offset,
+        };
+
+        let offset_str = sample_offset.to_string();
+        assert!(test_offset == Offset::from_str(&offset_str).unwrap())
+    }
 }

src/web/middleware/rejectua.rs

@@ -32,6 +32,7 @@ $
     .unwrap();
 }
 
+#[allow(clippy::clippy::upper_case_acronyms)]
 #[derive(Debug, Default)]
 pub struct RejectUA;
 
@@ -52,6 +53,7 @@ where
         future::ok(RejectUAMiddleware { service })
     }
 }
+#[allow(clippy::clippy::upper_case_acronyms)]
 
 pub struct RejectUAMiddleware<S> {
     service: S,

src/web/middleware/sentry.rs

@@ -68,8 +68,7 @@ pub fn queue_report(mut ext: RefMut<'_, Extensions>, err: &Error) {
         if let Some(events) = ext.get_mut::<Vec<Event<'static>>>() {
             events.push(event);
         } else {
-            let mut events: Vec<Event<'static>> = Vec::new();
-            events.push(event);
+            let events: Vec<Event<'static>> = vec![event];
             ext.insert(events);
         }
     }

src/web/tags.rs

@@ -165,11 +165,11 @@ impl FromRequest for Tags {
     }
 }
 
-impl Into<BTreeMap<String, String>> for Tags {
-    fn into(self) -> BTreeMap<String, String> {
+impl From<Tags> for BTreeMap<String, String> {
+    fn from(tags: Tags) -> BTreeMap<String, String> {
         let mut result = BTreeMap::new();
 
-        for (k, v) in self.tags {
+        for (k, v) in tags.tags {
             result.insert(k.clone(), v.clone());
         }