chaging replace document serialization

[maiconpavi]

The Insert uses human_readable = false option to serialize documents, but the Replace uses bson::to_document that uses human_readable = true.

This PR aims to make this consistent across both operations.

Regards,
Maicon

[isabelatkinson]

Hi @maiconpavi, thanks the PR! I've run a patch and it looks like our tests pass with these changes, but I was curious what your motivation is behind the changes you made. Does the internal representation of the replace command have any significance in your use of the driver?

[maiconpavi]

The default value of human_readable is true, with that change you can make a wrapper type around ObjectId if human_readable is true, serializes as str and if it is not then serializes as ObjectId. That way in a back end service you can use the same struct as the response type (serializing as str) and in mongodb::Collection (serializing as ObjectId) to achieve the correct serialization. This behavior is present in mongodb rust driver insert's API's.

[xadaemon]

Hey @isabelatkinson I work with @maiconpavi this change is mainly about improving the consistency of the API, thus making sure this doesn't surprise anyone, and making our code that uses it like Maicon described clearer and simpler.

Regards Matheus Xavier

[isabelatkinson]

Hi @neonimp and @maiconpavi, sorry for the delay in response! Thank you for detailing your use case. I'm concerned that making this change will inadvertently change the serialization format produced for other users who are relying upon the Serializer's current value for human_readable. For example, the following struct/Serialize implementation will produce different BSON if we make the switch you've proposed:

pub struct Data {
    str: String,
}

impl Serialize for Data {
    fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
    where
        S: serde::Serializer,
    {
        if serializer.is_human_readable() {
            serializer.serialize_str(&self.str)
        } else {
            serializer.serialize_bytes(&self.str.as_bytes())
        }
    }
}

which would lead to different data being inserted into the database and could possibly cause deserialization errors. For this reason we'll need to consider this change as breaking and defer it to a major release. I'll file a ticket for this work to consider it if/when we do a major version release in the future if that sounds alright to you!

[maiconpavi]

Now if you uses the insert_one will serialize as bytes and if you uses in replace_one will serialize as str.
After this change that i propose if you uses the insert_one will serialize as bytes and if you uses in replace_one will serialize as bytes

Regards,
Maicon Pavi

[isabelatkinson]

I agree that the inconsistency between the two methods isn't ideal, but it would still be a breaking change to begin to serialize to a different data format in replace_one. Unfortunately we're not able to make changes like this outside of a major version release (which we don't have planned in the near future).

One thing we could possibly do is add serialization options to the replace_one method (and all methods that serialize data) to allow users to configure the value for human_readable on the Serializer. That way you would be able to toggle the value to false yourself. Would that be a feasible solution for you?

[isabelatkinson]

Hi @maiconpavi, I'm going to close this out for now but I filed RUST-1687 to track the option I proposed above. Feel free to follow along/comment there if that functionality would be helpful for you!