Merge pull request #88 from mozilla-services/feat/delete_bsos_handler

feat: plugin delete_bsos to the delete_collection handler

migrations/2018-08-28-010336_init/up.sql

@@ -1,6 +1,4 @@
-CREATE DATABASE IF NOT EXISTS `syncstorage` /*!40100 DEFAULT CHARACTER SET latin1 */;
-
-USE `syncstorage`;
+-- /*!40100 DEFAULT CHARACTER SET latin1 */
 
 -- DROP TABLE IF EXISTS `bso`;
 -- XXX: bsov1, etc

src/db/results.rs

@@ -5,6 +5,7 @@
 use std::collections::HashMap;
 
 use diesel::sql_types::{BigInt, Integer, Nullable, Text};
+use serde::Serialize;
 
 use super::params;
 use db::util::SyncTimestamp;
@@ -48,7 +49,10 @@ pub struct GetBso {
 }
 
 #[derive(Debug, Default)]
-pub struct Paginated<T> {
+pub struct Paginated<T>
+where
+    T: Serialize,
+{
     pub items: Vec<T>,
     pub offset: Option<i64>, // XXX: i64?
 }

src/db/util.rs

@@ -7,6 +7,7 @@ use diesel::{
     deserialize::{self, FromSql},
     sql_types::BigInt,
 };
+use serde::{Deserialize, Deserializer, Serializer};
 
 use super::{DbError, DbErrorKind};
 
@@ -19,14 +20,20 @@ pub fn ms_since_epoch() -> i64 {
 ///
 /// Internally represents a Sync timestamp as a u64 representing milliseconds since the epoch.
 #[derive(Copy, Clone, Debug, PartialEq, PartialOrd, Deserialize, Serialize, FromSqlRow)]
-pub struct SyncTimestamp(u64);
+pub struct SyncTimestamp(
+    #[serde(
+        deserialize_with = "deserialize_ts",
+        serialize_with = "serialize_ts"
+    )]
+    u64,
+);
 
 impl SyncTimestamp {
     /// Create a string value compatible with existing Sync Timestamp headers
     ///
     /// Represents the timestamp as second since epoch with two decimal places of precision.
     pub fn as_header(&self) -> String {
-        format!("{:.*}", 2, &self.0)
+        format!("{:.*}", 2, self.0 as f64 / 1000.0)
     }
 
     /// Create a `SyncTimestamp` from a string header
@@ -43,7 +50,6 @@ impl SyncTimestamp {
                 }
             }).map(|v: f64| (v * 1_000f64) as u64)
             .map(SyncTimestamp::from_milliseconds)
-            .map_err(|_| "Invalid value")
     }
 
     /// Create a `SyncTimestamp` from an i64
@@ -104,3 +110,17 @@ where
             .map_err(|e| format!("Invalid SyncTimestamp i64 {}", e).into())
     }
 }
+
+pub fn deserialize_ts<'de, D>(d: D) -> Result<u64, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    Deserialize::deserialize(d).map(|result: f64| (result * 1_000f64) as u64)
+}
+
+fn serialize_ts<S>(x: &u64, s: S) -> Result<S::Ok, S::Error>
+where
+    S: Serializer,
+{
+    s.serialize_f64(*x as f64 / 1000.0)
+}

src/server/test.rs

@@ -12,7 +12,7 @@ use sha2::Sha256;
 use super::*;
 use db::mysql::pool::MysqlDbPool;
 use db::params;
-use db::results::{GetBso, PostBsos, PutBso};
+use db::results::{DeleteBso, GetBso, PostBsos, PutBso};
 use db::util::SyncTimestamp;
 use settings::{Secrets, ServerLimits};
 use web::auth::HawkPayload;
@@ -183,16 +183,21 @@ fn delete_all() {
 
 #[test]
 fn delete_collection() {
-    test_endpoint(http::Method::DELETE, "/1.5/42/storage/bookmarks", "0");
-    test_endpoint(
+    let start = SyncTimestamp::default();
+    test_endpoint(http::Method::DELETE, "/1.5/42/storage/bookmarks", "0.0");
+    test_endpoint_with_response(
         http::Method::DELETE,
         "/1.5/42/storage/bookmarks?ids=1,",
-        "0",
+        &move |result: DeleteBso| {
+            assert!(result > start);
+        },
     );
-    test_endpoint(
+    test_endpoint_with_response(
         http::Method::DELETE,
         "/1.5/42/storage/bookmarks?ids=1,2,3",
-        "0",
+        &move |result: DeleteBso| {
+            assert!(result > start);
+        },
     );
 }
 

src/web/handlers.rs

@@ -3,8 +3,9 @@ use std::collections::HashMap;
 
 use actix_web::{http::StatusCode, FutureResponse, HttpResponse, State};
 use futures::future::{self, Future};
+use serde::Serialize;
 
-use db::{params, DbErrorKind};
+use db::{params, results::Paginated, DbError, DbErrorKind};
 use server::ServerState;
 use web::extractors::{
     BsoPutRequest, BsoRequest, CollectionPostRequest, CollectionRequest, HawkIdentifier,
@@ -75,38 +76,58 @@ pub fn delete_all(meta: MetaRequest) -> FutureResponse<HttpResponse> {
 }
 
 pub fn delete_collection(coll: CollectionRequest) -> FutureResponse<HttpResponse> {
+    let delete_bsos = !coll.query.ids.is_empty();
+    let fut = if delete_bsos {
+        coll.db.delete_bsos(params::DeleteBsos {
+            user_id: coll.user_id.clone(),
+            collection: coll.collection.clone(),
+            ids: coll.query.ids.clone(),
+        })
+    } else {
+        coll.db.delete_collection(params::DeleteCollection {
+            user_id: coll.user_id.clone(),
+            collection: coll.collection.clone(),
+        })
+    };
+
     Box::new(
-        coll.db
-            .delete_collection(params::DeleteCollection {
-                user_id: coll.user_id.clone(),
-                collection: coll.collection.clone(),
-                // XXX: handle both cases (delete_collection & delete_bsos also)
-                // XXX: If we are passed id's to delete bso's, also set X-Last-Modified
-                /*
-                ids: coll
-                    .query
-                    .ids
-                    .as_ref()
-                    .map_or_else(|| Vec::new(), |ids| ids.clone()),
-                */
-            }).or_else(move |e| match e.kind() {
-                DbErrorKind::CollectionNotFound | DbErrorKind::BsoNotFound => {
-                    coll.db.get_storage_modified(coll.user_id)
-                }
-                _ => Box::new(future::err(e)),
-            }).map_err(From::from)
-            .map(|result| HttpResponse::Ok().json(result)),
+        fut.or_else(move |e| match e.kind() {
+            DbErrorKind::CollectionNotFound | DbErrorKind::BsoNotFound => {
+                coll.db.get_storage_modified(coll.user_id)
+            }
+            _ => Box::new(future::err(e)),
+        }).map_err(From::from)
+        .map(move |result| {
+            HttpResponse::Ok()
+                .if_true(delete_bsos, |resp| {
+                    resp.header("X-Last-Modified", result.as_header());
+                }).json(result)
+        }),
     )
 }
 
 pub fn get_collection(coll: CollectionRequest) -> FutureResponse<HttpResponse> {
+    let params = params::GetBsos {
+        user_id: coll.user_id.clone(),
+        collection: coll.collection.clone(),
+        params: coll.query.clone(),
+    };
+    if coll.query.full {
+        let fut = coll.db.get_bsos(params);
+        finish_get_collection(coll, fut)
+    } else {
+        let fut = coll.db.get_bso_ids(params);
+        finish_get_collection(coll, fut)
+    }
+}
+
+fn finish_get_collection<F, T>(coll: CollectionRequest, fut: F) -> FutureResponse<HttpResponse>
+where
+    F: Future<Item = Paginated<T>, Error = DbError> + 'static,
+    T: Serialize + 'static,
+{
     Box::new(
-        coll.db
-            .get_bsos(params::GetBsos {
-                user_id: coll.user_id.clone(),
-                collection: coll.collection.clone(),
-                params: coll.query.clone(),
-            }).map_err(From::from)
+        fut.map_err(From::from)
             .and_then(|result| {
                 coll.db
                     .extract_resource(coll.user_id, Some(coll.collection), None)