add ObjectId deserialization from sequence to support messagepack about bson-rust HOT 8 CLOSED

I filed RUST-1886 to consider using the newtype path for ObjectIDs. The team has discussed doing some broader cleanup work on our serde implementations (which would likely include RUST-426), so we'll look more into this when we prioritize that project. I'm going to close this out, but please let us know if you have any further questions or suggestions!

from bson-rust.

Hey @univerz, thanks for opening this issue! We likely don't want to add the visit method you're suggesting because it makes assumptions about the shape of an ObjectId sequence, namely that a) the entire oid is contained in the first entry and b) that the remaining contents should be ignored. Can you clarify why serialize_object_id_as_hex_string is not an option? Would a custom Deserialize implementation for your struct be feasible instead?

from bson-rust.

because it makes assumptions about the shape of an ObjectId sequence

well, it has to, because ObjectId behavior is special.

Can you clarify why serialize_object_id_as_hex_string is not an option? Would a custom Deserialize implementation for your struct be feasible instead?

it would conflict using the same struct with mongodb.

from bson-rust.

well, it has to, because ObjectId behavior is special.

It's true that ObjectId (like several other BSON types) has special behavior in that it is serialized in extended-JSON format as a key/value pair (i.e. { "$oid": <hex string> }). However, I think the special behavior that's causing your issue here is the fact that rmp_serde::encode::to_vec serializes key/value pairs as arrays, which in this case erases the "$oid" key and sticks the hex string value into a single-element array. Because this behavior is so specific to rmp-serde, it is not something that we want to accommodate in our library for the reasons I mentioned above.

If you need to support dual-functionality for deserialization, I recommend writing a function to use with the deserialize_with attribute that can accommodate both this single-element array format and whichever format you need for MongoDB compatibility. Would something like this work for you?

fn deserialize_object_id_from_array_or_other<'de, D>(
    deserializer: D,
) -> std::result::Result<ObjectId, D::Error>
where
    D: Deserializer<'de>,
{
    #[derive(Deserialize)]
    #[serde(untagged)]
    enum ObjectIdHelper {
        Array(Vec<ObjectId>),
        // Or a different type if default ObjectId deserialization does not work for your other use case.
        ObjectId(ObjectId),
    }

    let object_id = match ObjectIdHelper::deserialize(deserializer)? {
        ObjectIdHelper::Array(array) => array
            .into_iter()
            .next()
            .ok_or_else(|| serde::de::Error::custom("empty array".to_string()))?,
        ObjectIdHelper::ObjectId(object_id) => object_id,
    };

    Ok(object_id)
}

from bson-rust.

There has not been any recent activity on this ticket, so we are marking it as stale. If we do not hear anything further from you, this issue will be automatically closed in one week.

from bson-rust.

Because this behavior is so specific to rmp-serde

derive(Deserialize) generates visit_seq for structs, and other efficient formats like bincode & bitcode also have a problem (i'm partly to blame for this with my previous is_human_readable hack).

i noticed that bson still uses inefficient ObjectId serialization path, so i tried using extjson::models::ObjectId inside bson transparently (where that fake struct {"$oid" => hex-string} format is required) to free oid::ObjectId from custom ser/de implementations (& use serde derive on it so it behaves nicely like any other rust struct).

however, the deserialization path would need more thought, so i decided to just make oid::ObjectId serialization symmetric with deserialization (which uses bytes so visit_seq is not needed).

cargo test is green, behavior inside bson & for human_readable formats is the same & works better for non human_readable with significantly improved performance, similar to my previous investigation of ObjectId deserialization.

diff --git a/src/extjson/models.rs b/src/extjson/models.rs
index efb57ff..b31b4eb 100644
--- a/src/extjson/models.rs
+++ b/src/extjson/models.rs
@@ -93,6 +93,7 @@ impl Decimal128 {
 
 #[derive(Serialize, Deserialize)]
 #[serde(deny_unknown_fields)]
+#[serde(rename = "$oid")]
 pub(crate) struct ObjectId {
     #[serde(rename = "$oid")]
     oid: String,
diff --git a/src/raw/bson_ref.rs b/src/raw/bson_ref.rs
index 8241d87..3986615 100644
--- a/src/raw/bson_ref.rs
+++ b/src/raw/bson_ref.rs
@@ -326,7 +326,9 @@ impl<'a> Serialize for RawBsonRef<'a> {
             RawBsonRef::Null => serializer.serialize_unit(),
             RawBsonRef::Int32(v) => serializer.serialize_i32(*v),
             RawBsonRef::Int64(v) => serializer.serialize_i64(*v),
-            RawBsonRef::ObjectId(oid) => oid.serialize(serializer),
+            RawBsonRef::ObjectId(oid) => {
+                crate::extjson::models::ObjectId::from(*oid).serialize(serializer)
+            }
             RawBsonRef::DateTime(dt) => dt.serialize(serializer),
             RawBsonRef::Binary(b) => b.serialize(serializer),
             RawBsonRef::JavaScriptCode(c) => {
@@ -678,13 +680,13 @@ impl<'a> Serialize for RawDbPointerRef<'a> {
             ref_ns: &'a str,
 
             #[serde(rename = "$id")]
-            id: ObjectId,
+            id: crate::extjson::models::ObjectId,
         }
 
         let mut state = serializer.serialize_struct("$dbPointer", 1)?;
         let body = BorrowedDbPointerBody {
             ref_ns: self.namespace,
-            id: self.id,
+            id: self.id.into(),
         };
         state.serialize_field("$dbPointer", &body)?;
         state.end()
diff --git a/src/ser/serde.rs b/src/ser/serde.rs
index e2866a5..7990a49 100644
--- a/src/ser/serde.rs
+++ b/src/ser/serde.rs
@@ -32,9 +32,11 @@ impl Serialize for ObjectId {
     where
         S: serde::ser::Serializer,
     {
-        let mut ser = serializer.serialize_struct("$oid", 1)?;
-        ser.serialize_field("$oid", &self.to_string())?;
-        ser.end()
+        if !serializer.is_human_readable() {
+            serializer.serialize_bytes(&self.bytes())
+        } else {
+            crate::extjson::models::ObjectId::from(*self).serialize(serializer)
+        }
     }
 }
 
@@ -67,7 +69,9 @@ impl Serialize for Bson {
             Bson::Null => serializer.serialize_unit(),
             Bson::Int32(v) => serializer.serialize_i32(*v),
             Bson::Int64(v) => serializer.serialize_i64(*v),
-            Bson::ObjectId(oid) => oid.serialize(serializer),
+            Bson::ObjectId(oid) => {
+                crate::extjson::models::ObjectId::from(*oid).serialize(serializer)
+            }
             Bson::DateTime(dt) => dt.serialize(serializer),
             Bson::Binary(b) => b.serialize(serializer),
             Bson::JavaScriptCode(c) => {
diff --git a/src/serde_helpers.rs b/src/serde_helpers.rs
index 42c1657..8de4a99 100644
--- a/src/serde_helpers.rs
+++ b/src/serde_helpers.rs
@@ -408,7 +408,7 @@ pub mod hex_string_as_object_id {
     /// Serializes a hex string as an ObjectId.
     pub fn serialize<S: Serializer>(val: &str, serializer: S) -> Result<S::Ok, S::Error> {
         match ObjectId::parse_str(val) {
-            Ok(oid) => oid.serialize(serializer),
+            Ok(oid) => crate::extjson::models::ObjectId::from(oid).serialize(serializer),
             Err(_) => Err(ser::Error::custom(format!(
                 "cannot convert {} to ObjectId",
                 val

from bson-rust.

now i see why it is flawed.
if visit_seq is not the way to go (+ i don't like it because of the hex-string performance penalty), i guess i'll have to wait for RUST-426 which will probably clean it up and is pretty high in the queue.

from bson-rust.

maybe the newtype_struct path, like for uuid, is the way to solve it properly and efficiently?

from bson-rust.