RUST-1992 Make serde an optional feature #554
Conversation
examples/deserialize.rs
Outdated
There was a problem hiding this comment.
The examples/ directory had some very basic and very stale examples that AFAICT weren't set up to be tested or linked to by anything, so I removed them in favor of relying on the main rustdoc.
| @@ -56,6 +53,19 @@ impl RawArrayBuf { | |||
| } | |||
| } | |||
|
|
|||
| #[allow(clippy::should_implement_trait)] | |||
| pub fn from_iter<B, I>(iter: I) -> crate::error::Result<Self> | |||
There was a problem hiding this comment.
Unfortunately, there's no TryFromIterator trait (it's been proposed a couple of times over the years and discussions always end up in the weeds), so a constructor function is the best we can do.
Clippy gets grumpy that this is named the same as the FromIterator trait method but I think it's the discoverable thing to do, hence the allow.
| pub fn from_document(doc: &Document) -> Result<Self> { | ||
| let mut out = RawDocumentBuf::new(); | ||
| for (k, v) in doc { | ||
| let val: RawBson = v.clone().try_into()?; |
There was a problem hiding this comment.
In theory, RawDocumentBuf::from_document should be able to iterate over a borrowed Document and build the new RawDocumentBuf without any intermediate copies needed. Where this breaks down is that appending to a bson byte buffer is defined at its core in terms of RawBsonRefs, and there's no way to get one of those from a managed-type &Bson without allocating intermediate storage.
We could add logic to translate the managed &Bson type directly to bytes, but we've already got too many slightly-different versions of that logic already. If the clone() here turns out to be a profiling hotspot we can revisit.
There was a problem hiding this comment.
We could add logic to translate the managed &Bson type directly to bytes
Is this something you think will land for 3.0? If not, the update to TryFrom<Document> for RawDocumentBuf got me thinking that maybe this method should take an owned Document - that way, we can avoid unnecessary clones for owned data, and it will be more clear for users with borrowed documents that their data is going to be cloned. As-is, it doesn't seem obvious that RawDocumentBuf::from_document(document) is less performant than RawDocumentBuf::from(document) for an owned document (which is also an argument against my earlier suggestion to make the input here impl Borrow<Document>).
Alternatively, we could remove this method entirely in favor of the TryFrom impls, but it does seem like a nice utility from a discoverability standpoint.
There was a problem hiding this comment.
My preference would be to remove from_document entirely in favor of the TryFrom impls and have a big documentation stanza as part of the work for RUST-2228.
src/raw/array_buf.rs
Outdated
| type Error = crate::error::Error; | ||
|
|
||
| fn try_from(value: crate::Array) -> Result<Self, Self::Error> { | ||
| Self::try_from(&value) |
There was a problem hiding this comment.
nit: suggest iterating over value.into_iter() here to avoid cloning each element
There was a problem hiding this comment.
Good call; also updated the similar impl for RawDocumentBuf.
Co-authored-by: Isabel Atkinson <[email protected]>
Co-authored-by: Isabel Atkinson <[email protected]>
Co-authored-by: Isabel Atkinson <[email protected]>
abr-egn
force-pushed
the
RUST-1992/decouple-serde
branch
from
0a87952 to
a8804a9
Compare
I think this PR's big enough so I'll leave that for later, but I did have to do a rebase :) |
RUST-1992
This splits everything
serde-related into feature-gated optional functionality, with a whole lot of downstream changes:to_vecexisted in both and were way too easy to confuse otherwise)Document::to_writerpreviously round-tripped throughserdefor ... reasons? ... and now just directly encodes (and I added a convenienceencode_to_vecmethod because 99% of the time that's the writer target).bson::to_vec(doc)are now directly encodingRawDocumentBuf::appendandRawArrayBuf::pushpreviously would panic if given a value that couldn't be encoded, which made for a slightly more convenient API but is also egregiously unrustacean, so they now returnResult<()>.rawdoc!macro, which uses the above, still has the panic behavior because there's not really anything else it can do. Everywhere else downstream of those is properly fallible now.Things under the umbrella of RUST-1992 to come in follow-up PRs:
serde_jsonmight also be reasonable to be an optional dependency. I'm less certain of this, though, it'll need some exploration.