RUST-1045 Support appending to `RawDocumentBuf` #326

patrickfreed · 2021-11-18T23:21:27Z

This PR adds a RawDocumentBuf::append method which enables RawDocumentBufs to be created and mutated without having to go through Document.

To achieve this, a number of public API additions/changes were needed:

An OwnedRawBson type was added
- This is essentially the same as Bson, except the cases that hold documents/arrays use RawDocumentBuf and RawArrayBuf (also new type)
- A OwnedRawJavaScriptCodeWithScope was also added, since the existing JavaScriptCodeWithScope stored its scope as a Document
A RawDocumentBuf can now de deserialized from arbitrary maps
- Ditto for RawArrayBuf and arbitrary sequences
RawDocumentBuf::new was renamed to RawDocumentBuf::from_bytes
- This allowed RawDocumentBuf::new() to be added for constructing brand new documents
- RawDocument::new was also renamed to be consistent with this
RawDocumentBuf and RawArrayBuf now implement FromIterator
RawBson and OwnedRawBson both implement a number of From traits

This reverts commit 0f96cb0.

patrickfreed · 2021-11-18T23:21:51Z

serde-tests/json.rs

+use serde::{Deserialize, Serialize};
+
+#[test]
+fn all_types_json() {


this test was moved without modification from test.rs. The one after it is new though.

patrickfreed · 2021-11-18T23:22:58Z

src/de/raw.rs

@@ -648,13 +648,20 @@ impl<'d, 'de> serde::de::Deserializer<'de> for DocumentKeyDeserializer<'d, 'de>
        }
    }

+    fn deserialize_newtype_struct<V>(self, _name: &'static str, visitor: V) -> Result<V::Value>


This was needed to support deserializing the Cow wrappers used in deserialization now. It just forwards deserialization to the wrapped type.

patrickfreed · 2021-11-18T23:23:52Z

src/raw/bson.rs

-}
-
-/// A visitor used to deserialize types backed by raw BSON.
-pub(crate) struct RawBsonVisitor;


This was moved to src/raw/serde.rs and slightly modified so that it could be shared with OwnedRawBson.

patrickfreed · 2021-11-18T23:25:54Z

src/raw/document_buf.rs

+    /// assert_eq!(doc.to_document()?, expected);
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn append(&mut self, key: impl Into<String>, value: impl Into<OwnedRawBson>) {


Since this only appends to the end, we can't call it insert (which would imply key lookup). I opted for something different than push though which seems to be coupled with stacks/lists rather than key-value stores. Let me know if these seems right.

patrickfreed · 2021-11-18T23:28:07Z

src/raw/owned_bson.rs

+
+/// A BSON value backed by owned raw BSON bytes.
+#[derive(Debug, Clone, PartialEq)]
+pub enum OwnedRawBson {


This type was needed to have something to append to documents. I originally tried using a RawBson<'a> for this, but it was clunky / awkward, and it would potentially make the rawdoc! macro impossible.

Regarding the name, I originally opted for RawBson being the owned one and RawBsonRef being the borrowed one, but I ended up going with OwnedRawBson and RawBson to match how we added the suffix to the owned variants of RawDocument and RawArray. I'm still not certain this is the correct naming scheme for RawBson though, would love to hear your all's thoughts on this.

I think I'd prefer RawBson / RawBsonRef in this case, although I have a hard time explaining why. OwnedX seems more unusual than XRef as Rust type names go.

I think I agree. I originally thought it would be confusing to have RawDocument typically be borrowed and RawBson be owned, but OwnedRawBson does seem odd enough to warrant the inconsistency. Also, RawDocument technically isn't a borrowed type anyways, though it's always accessed as a wide-pointer (&RawDocument).

agreed here, RawBson and RawBsonRef feel more intuitive to me

Updated to be RawBsonRef and RawBson. I also updated RawBinary<'a> et al to be RawBinaryRef<'a> and what not. OwnedRawJavaScriptCodeWithScope dropped the Owned prefix. I also renamed RawBsonRef::to_owned_raw_bson to just RawBsonRef::to_raw_bson.

patrickfreed · 2021-11-18T23:33:59Z

src/raw/owned_bson.rs

+    }
+
+    /// Gets a [`RawBson`] value referencing this owned raw BSON value.
+    pub fn as_raw_bson(&self) -> RawBson<'_> {


Unfortunately, we can't implement Deref, AsRef, Borrow, or ToOwned between these two types, the first three because the trait requires a reference to be returned and the last one due to the blanket implementation for Clone types. See rust-lang/rust#44950 for more info.

patrickfreed · 2021-11-18T23:36:04Z

src/raw/serde.rs

+}
+
+/// A visitor used to deserialize types backed by raw BSON.
+pub(crate) struct OwnedOrBorrowedRawBsonVisitor;


This is essentially the same as the old RawBsonVisitor, but it now can handle owned stuff (e.g. visit_string, visit_byte_buf) and arbitrary maps and sequences.

abr-egn · 2021-11-19T17:56:41Z

src/raw/array_buf.rs

+/// bytes. This type can be used to construct owned array values, which can be used to append to
+/// [`RawDocumentBuf`] or as a field in a `Deserialize` struct.
+///
+/// Iterating over a [`RawArrayBuf`] yields either an error or a key-value pair that borrows from


key-value pair?

abr-egn · 2021-11-19T19:51:37Z

src/raw/owned_bson.rs

+
+/// A BSON value backed by owned raw BSON bytes.
+#[derive(Debug, Clone, PartialEq)]
+pub enum OwnedRawBson {


I think I'd prefer RawBson / RawBsonRef in this case, although I have a hard time explaining why. OwnedX seems more unusual than XRef as Rust type names go.

abr-egn · 2021-11-19T20:02:56Z

src/raw/owned_bson.rs

+
+    /// Gets a reference to the [`RawArrayBuf`] that's wrapped or returns `None` if the wrapped
+    /// value isn't a BSON array.
+    pub fn as_array(&self) -> Option<&'_ RawArray> {


Since there are now mutating methods for those, I think it'd be good to have as_array_mut -> Option<&mut RawArrayBuf> and similar for as_document_mut.

good idea, added

abr-egn

LGTM modulo naming questions.

patrickfreed added 16 commits November 18, 2021 17:28

wip append

d746652

finish implementaiton, start adding tests

bcf3b9c

wip array buf stuff

021f1f8

wip wip

b2783d0

rename bson.rs -> bson_ref.rs

4c4b93b

document arraybuf

6eaff65

Revert "rename bson.rs -> bson_ref.rs"

3f46173

This reverts commit 0f96cb0.

wip owned bson

706c167

add owned raw bson to corpus field tests

d45ee9d

minor improvements

b8089f2

add serialize/deserialize impl to rawarraybuf, serde tests for owned

ae50178

fix clippy

52e6176

various cleanup

af29b3e

further consolidate deserialization logic

ed680ef

consolidate array deserialization

9f33205

standardize on from/into_bytes

b0cbaef

patrickfreed commented Nov 18, 2021

View reviewed changes

fix tests, compile on MSRV

fc96e7f

patrickfreed marked this pull request as ready for review November 19, 2021 17:08

patrickfreed requested review from isabelatkinson and abr-egn November 19, 2021 17:08

abr-egn reviewed Nov 19, 2021

View reviewed changes

patrickfreed added 2 commits November 22, 2021 13:31

fix comment

6ddebbd

add as_document_mut and as_array_mut

dd3aa9c

abr-egn approved these changes Nov 29, 2021

View reviewed changes

refactor RawBson -> RawBsonRef, OwnedRawBson -> RawBson

9abd6f9

abr-egn approved these changes Nov 30, 2021

View reviewed changes

isabelatkinson approved these changes Nov 30, 2021

View reviewed changes

refactor as_raw_bson -> as_raw_bson_ref

4261922

patrickfreed merged commit 17dc632 into mongodb:master Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-1045 Support appending to `RawDocumentBuf` #326

RUST-1045 Support appending to `RawDocumentBuf` #326

patrickfreed commented Nov 18, 2021

patrickfreed Nov 18, 2021

patrickfreed Nov 18, 2021

patrickfreed Nov 18, 2021

patrickfreed Nov 18, 2021

patrickfreed Nov 18, 2021

abr-egn Nov 19, 2021

patrickfreed Nov 22, 2021

isabelatkinson Nov 29, 2021

patrickfreed Nov 30, 2021

patrickfreed Nov 18, 2021

patrickfreed Nov 18, 2021

abr-egn Nov 19, 2021

patrickfreed Nov 22, 2021

abr-egn Nov 19, 2021

abr-egn Nov 19, 2021

patrickfreed Nov 22, 2021

abr-egn left a comment

RUST-1045 Support appending to RawDocumentBuf #326

RUST-1045 Support appending to RawDocumentBuf #326

Conversation

patrickfreed commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn left a comment

Choose a reason for hiding this comment

RUST-1045 Support appending to `RawDocumentBuf` #326

RUST-1045 Support appending to `RawDocumentBuf` #326