RUST-276 Properly construct findAndModify without options (#117)

Author: saghm

src/operation/find_and_modify/mod.rs

@@ -25,7 +25,7 @@ use crate::{
 pub(crate) struct FindAndModify {
     ns: Namespace,
     query: Document,
-    options: Option<FindAndModifyOptions>,
+    options: FindAndModifyOptions,
 }
 
 impl FindAndModify {
@@ -34,7 +34,8 @@ impl FindAndModify {
         query: Document,
         options: Option<FindOneAndDeleteOptions>,
     ) -> Self {
-        let options = options.map(FindAndModifyOptions::from_find_one_and_delete_options);
+        let options =
+            FindAndModifyOptions::from_find_one_and_delete_options(options.unwrap_or_default());
         FindAndModify { ns, query, options }
     }
 
@@ -45,8 +46,10 @@ impl FindAndModify {
         options: Option<FindOneAndReplaceOptions>,
     ) -> Result<Self> {
         bson_util::replacement_document_check(&replacement)?;
-        let options = options
-            .map(|opts| FindAndModifyOptions::from_find_one_and_replace_options(replacement, opts));
+        let options = FindAndModifyOptions::from_find_one_and_replace_options(
+            replacement,
+            options.unwrap_or_default(),
+        );
         Ok(FindAndModify { ns, query, options })
     }
 
@@ -59,8 +62,10 @@ impl FindAndModify {
         if let UpdateModifications::Document(ref d) = update {
             bson_util::update_document_check(d)?;
         };
-        let options = options
-            .map(|opts| FindAndModifyOptions::from_find_one_and_update_options(update, opts));
+        let options = FindAndModifyOptions::from_find_one_and_update_options(
+            update,
+            options.unwrap_or_default(),
+        );
         Ok(FindAndModify { ns, query, options })
     }
 }
@@ -75,7 +80,7 @@ impl Operation for FindAndModify {
             "query": self.query.clone(),
         };
 
-        append_options(&mut body, self.options.as_ref())?;
+        append_options(&mut body, Some(&self.options))?;
 
         Ok(Command::new(
             Self::NAME.to_string(),

src/operation/find_and_modify/options.rs

@@ -1,7 +1,7 @@
 use std::time::Duration;
 
 use bson::{doc, Document};
-use serde::Serialize;
+use serde::{Serialize, Serializer};
 use typed_builder::TypedBuilder;
 
 use crate::{
@@ -17,18 +17,27 @@ use crate::{
     concern::WriteConcern,
 };
 
+#[derive(Debug, Serialize)]
+pub(super) enum Modification {
+    #[serde(rename = "remove", serialize_with = "self::serialize_true")]
+    Delete,
+    #[serde(rename = "update")]
+    Update(UpdateModifications),
+}
+
+fn serialize_true<S: Serializer>(s: S) -> std::result::Result<S::Ok, S::Error> {
+    s.serialize_bool(true)
+}
+
 #[serde_with::skip_serializing_none]
-#[derive(Debug, Default, TypedBuilder, Serialize)]
+#[derive(Debug, TypedBuilder, Serialize)]
 #[serde(rename_all = "camelCase")]
 pub(super) struct FindAndModifyOptions {
-    #[builder(default)]
-    pub(crate) sort: Option<Document>,
-
-    #[builder(default)]
-    pub(crate) remove: Option<bool>,
+    #[serde(flatten)]
+    pub(crate) modification: Modification,
 
     #[builder(default)]
-    pub(crate) update: Option<UpdateModifications>,
+    pub(crate) sort: Option<Document>,
 
     #[builder(default)]
     pub(crate) new: Option<bool>,
@@ -65,11 +74,11 @@ impl FindAndModifyOptions {
         opts: FindOneAndDeleteOptions,
     ) -> FindAndModifyOptions {
         FindAndModifyOptions::builder()
+            .modification(Modification::Delete)
             .collation(opts.collation)
             .max_time(opts.max_time)
             .projection(opts.projection)
             .sort(opts.sort)
-            .remove(true)
             .write_concern(opts.write_concern)
             .build()
     }
@@ -80,14 +89,14 @@ impl FindAndModifyOptions {
     ) -> FindAndModifyOptions {
         let replacement = UpdateModifications::Document(replacement);
         FindAndModifyOptions::builder()
+            .modification(Modification::Update(replacement))
             .collation(opts.collation)
             .bypass_document_validation(opts.bypass_document_validation)
             .max_time(opts.max_time)
             .projection(opts.projection)
             .new(return_document_to_bool(opts.return_document))
             .sort(opts.sort)
             .upsert(opts.upsert)
-            .update(replacement)
             .write_concern(opts.write_concern)
             .build()
     }
@@ -97,6 +106,7 @@ impl FindAndModifyOptions {
         opts: FindOneAndUpdateOptions,
     ) -> FindAndModifyOptions {
         FindAndModifyOptions::builder()
+            .modification(Modification::Update(update))
             .collation(opts.collation)
             .array_filters(opts.array_filters)
             .bypass_document_validation(opts.bypass_document_validation)
@@ -105,7 +115,6 @@ impl FindAndModifyOptions {
             .new(return_document_to_bool(opts.return_document))
             .sort(opts.sort)
             .upsert(opts.upsert)
-            .update(update)
             .write_concern(opts.write_concern)
             .build()
     }

src/operation/find_and_modify/test.rs

@@ -27,6 +27,36 @@ fn empty_delete() -> FindAndModify {
     FindAndModify::with_delete(ns, filter, None)
 }
 
+#[test]
+fn build_with_delete_no_options() {
+    let ns = Namespace {
+        db: "test_db".to_string(),
+        coll: "test_coll".to_string(),
+    };
+    let filter = doc! { "x": { "$gt": 1 } };
+    let max_time = Duration::from_millis(2u64);
+
+    let op = FindAndModify::with_delete(ns, filter.clone(), None);
+
+    let description = StreamDescription::new_testing();
+    let mut cmd = op.build(&description).unwrap();
+
+    assert_eq!(cmd.name.as_str(), "findAndModify");
+    assert_eq!(cmd.target_db.as_str(), "test_db");
+    assert_eq!(cmd.read_pref.as_ref(), None);
+
+    let mut expected_body = doc! {
+        "findAndModify": "test_coll",
+        "query": filter,
+        "remove": true
+    };
+
+    bson_util::sort_document(&mut cmd.body);
+    bson_util::sort_document(&mut expected_body);
+
+    assert_eq!(cmd.body, expected_body);
+}
+
 #[test]
 fn build_with_delete() {
     let ns = Namespace {
@@ -123,6 +153,36 @@ fn empty_replace() -> FindAndModify {
     FindAndModify::with_replace(ns, filter, replacement, None).unwrap()
 }
 
+#[test]
+fn build_with_replace_no_options() {
+    let ns = Namespace {
+        db: "test_db".to_string(),
+        coll: "test_coll".to_string(),
+    };
+    let filter = doc! { "x": { "$gt": 1 } };
+    let replacement = doc! { "x": { "inc": 1 } };
+
+    let op = FindAndModify::with_replace(ns, filter.clone(), replacement.clone(), None).unwrap();
+
+    let description = StreamDescription::new_testing();
+    let mut cmd = op.build(&description).unwrap();
+
+    assert_eq!(cmd.name.as_str(), "findAndModify");
+    assert_eq!(cmd.target_db.as_str(), "test_db");
+    assert_eq!(cmd.read_pref.as_ref(), None);
+
+    let mut expected_body = doc! {
+        "findAndModify": "test_coll",
+        "query": filter,
+        "update": replacement,
+    };
+
+    bson_util::sort_document(&mut cmd.body);
+    bson_util::sort_document(&mut expected_body);
+
+    assert_eq!(cmd.body, expected_body);
+}
+
 #[test]
 fn build_with_replace() {
     let ns = Namespace {
@@ -224,6 +284,35 @@ fn empty_update() -> FindAndModify {
     FindAndModify::with_update(ns, filter, update, None).unwrap()
 }
 
+#[test]
+fn build_with_update_no_options() {
+    let ns = Namespace {
+        db: "test_db".to_string(),
+        coll: "test_coll".to_string(),
+    };
+    let filter = doc! { "x": { "$gt": 1 } };
+    let update = UpdateModifications::Document(doc! { "$x": { "$inc": 1 } });
+    let op = FindAndModify::with_update(ns, filter.clone(), update.clone(), None).unwrap();
+
+    let description = StreamDescription::new_testing();
+    let mut cmd = op.build(&description).unwrap();
+
+    assert_eq!(cmd.name.as_str(), "findAndModify");
+    assert_eq!(cmd.target_db.as_str(), "test_db");
+    assert_eq!(cmd.read_pref.as_ref(), None);
+
+    let mut expected_body = doc! {
+        "findAndModify": "test_coll",
+        "query": filter,
+        "update": update.to_bson(),
+    };
+
+    bson_util::sort_document(&mut cmd.body);
+    bson_util::sort_document(&mut expected_body);
+
+    assert_eq!(cmd.body, expected_body);
+}
+
 #[test]
 fn build_with_update() {
     let ns = Namespace {