RUST-1992 Make serde an optional feature #554

abr-egn · 2025-06-11T16:57:23Z

RUST-1992

This splits everything serde-related into feature-gated optional functionality, with a whole lot of downstream changes:

Functions got renamed to make it clear which of the encode/decode or serialize/deserialize family they were part of (things like to_vec existed in both and were way too easy to confuse otherwise)
Document::to_writer previously round-tripped through serde for ... reasons? ... and now just directly encodes (and I added a convenience encode_to_vec method because 99% of the time that's the writer target).
A lot of things that were calling bson::to_vec(doc) are now directly encoding
RawDocumentBuf::append and RawArrayBuf::push previously 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 return Result<()>.
The 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.
Core functionality (constants, utility functions) to binary bson handling was moved out of serde modules.

Things under the umbrella of RUST-1992 to come in follow-up PRs:

user documentation for the distinction between "encode" (concrete bson type to bytestream) and "serialize" (arbitrary type, possibly containing bson, to arbitrary format)
serde_json might also be reasonable to be an optional dependency. I'm less certain of this, though, it'll need some exploration.
clean up and consolidation of the core binary-format bson bytestream handling functions, they're still a bit scattered

abr-egn · 2025-06-11T16:58:41Z

examples/deserialize.rs

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.

abr-egn · 2025-06-12T14:56:28Z

src/raw/array_buf.rs

    }

+    #[allow(clippy::should_implement_trait)]
+    pub fn from_iter<B, I>(iter: I) -> crate::error::Result<Self>


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.

abr-egn · 2025-06-12T15:14:18Z

src/raw/document_buf.rs

+    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()?;


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.

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.

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.

sounds good to me

isabelatkinson · 2025-06-16T15:29:03Z

src/raw/array_buf.rs

+    type Error = crate::error::Error;
+
+    fn try_from(value: crate::Array) -> Result<Self, Self::Error> {
+        Self::try_from(&value)


nit: suggest iterating over value.into_iter() here to avoid cloning each element

Good call; also updated the similar impl for RawDocumentBuf.

src/raw/bson_ref.rs

src/raw/document_buf.rs

isabelatkinson

LGTM, I can re-approve if you want to do the from_document removal as part of this PR

Co-authored-by: Isabel Atkinson <[email protected]>

abr-egn · 2025-06-17T19:27:41Z

LGTM, I can re-approve if you want to do the from_document removal as part of this PR

I think this PR's big enough so I'll leave that for later, but I did have to do a rebase :)

abr-egn commented Jun 12, 2025

View reviewed changes

abr-egn marked this pull request as ready for review June 12, 2025 15:28

abr-egn requested a review from a team as a code owner June 12, 2025 15:28

abr-egn requested a review from isabelatkinson June 12, 2025 15:28

isabelatkinson reviewed Jun 16, 2025

View reviewed changes

abr-egn requested a review from isabelatkinson June 16, 2025 18:08

isabelatkinson previously approved these changes Jun 17, 2025

View reviewed changes

abr-egn and others added 23 commits June 17, 2025 14:57

wip

2983501

wip

5ece829

wip

99fe2e4

infallible encoding

8ab107a

rename fuzz targets

995b832

pick up renamed files

18f7a57

checkpoint before I start unwinding infallibility

265e56b

failing fallibility

6977793

fixed test

2ec289f

rename flurry

29b5f69

back to compilation

8f20e97

flix cippy

0db43b3

rix fustfmt

47ac17d

rust fixdoc

3801d87

fix tests

f59d075

fix fuzz

d7c5086

actually fix rustdoc

acbed81

maybe fix fuzzer?

c9b6a34

names are hard

d4816b1

Update src/raw/bson_ref.rs

dd031cb

Co-authored-by: Isabel Atkinson <[email protected]>

Update src/raw/document_buf.rs

8cf30d6

Co-authored-by: Isabel Atkinson <[email protected]>

Update src/raw/document_buf.rs

b255d5a

Co-authored-by: Isabel Atkinson <[email protected]>

review updates

a8804a9

abr-egn dismissed isabelatkinson’s stale review via a8804a9 June 17, 2025 19:06

abr-egn force-pushed the RUST-1992/decouple-serde branch from 0a87952 to a8804a9 Compare June 17, 2025 19:06

undo accidental unrename

94f7405

abr-egn requested a review from isabelatkinson June 17, 2025 19:27

isabelatkinson approved these changes Jun 17, 2025

View reviewed changes

abr-egn merged commit 1669bc0 into mongodb:main Jun 17, 2025
11 checks passed

isabelatkinson mentioned this pull request Jul 23, 2025

RUST-2228 Normalize conversions between raw types and base crate types #580

Merged

RUST-1992 Make serde an optional feature #554

RUST-1992 Make serde an optional feature #554

Uh oh!

Conversation

abr-egn commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isabelatkinson left a comment

Choose a reason for hiding this comment

Uh oh!

abr-egn commented Jun 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abr-egn commented Jun 11, 2025 •

edited

Loading