RUST-465 Introduce a `bson::Uuid` wrapper type #314

patrickfreed · 2021-10-19T22:27:25Z

This PR introduces a Uuid type which can be used to correctly serialize to / from BSON, regardless of whether bson::to_bson or bson::to_vec is used. This type requires no feature flags to be used, though the existing flag can be enabled to allow for easy conversion between it and uuid::Uuid (similar to bson::DateTime).

As part of this, I updated the UUID spec implementation to operate on the new bson::Uuid type rather than uuid::Uuid, and as a result, was able to move that out from behind a feature flag too. Since these methods haven't been included in a release yet, this isn't a breaking change.

When serialized/deserializing from other formats, I opted to have bson::Uuid use uuid::Uuid's implementation, rather than sticking with extended JSON (unlike bson::DateTime). Based on past experience, I think users will prefer this, but let me know if this doesn't seem like the right choice.

See #311 (comment) for more context on the motivation behind this change.

TODO: documentation, tests, API exam

patrickfreed · 2021-10-19T22:28:25Z

src/bson.rs

@@ -1076,148 +1073,6 @@ impl Binary {
    }
 }

-/// Enum of the possible representations to use when converting between [Uuid](https://docs.rs/uuid/0.8.2/uuid/) and [`Binary`].


this was all moved to the uuid module. The main difference is that these methods now work with bson::Uuid instead of uuid::Uuid, and that they also can be used without a feature flag.

patrickfreed · 2021-10-19T22:31:10Z

src/ser/serde.rs

        value: &T,
    ) -> crate::ser::Result<Bson>
    where
        T: Serialize,
    {
-        value.serialize(self)


serialization works by serializing a newtype with a name that this serializer can recognize (UUID_NEWTYPE_NAME). When it sees this type, it uses the output of uuid::Uuid's Serialize implementation (for the human readable serializer, a string) and turns it into a subtype 4 Binary.

The same thing is done for the raw serializer, except that it expects uuid::Uuid to produce bytes.

patrickfreed · 2021-10-19T22:31:41Z

src/tests/modules/bson.rs

@@ -396,146 +391,3 @@ fn debug_print() {
    assert_eq!(format!("{:?}", doc), normal_print);
    assert_eq!(format!("{:#?}", doc), pretty_print);
 }
-
-#[cfg(feature = "uuid-0_8")]


these were moved to src/uuid/test.rs

patrickfreed · 2021-10-19T22:37:04Z

src/uuid/mod.rs

+#[cfg(feature = "uuid-0_8")]
+#[cfg_attr(docsrs, doc(cfg(feature = "uuid-0_8")))]
+impl Uuid {
+    /// Convert this [`Uuid`] to a [`uuid::Uuid`] from the [`uuid`](https://docs.rs/uuid/latest) crate.


I figured it would be confusing to call this from_uuid, so I just appended the version number to the end to disambiguate. This will also be useful for when uuid releases another minor version.

patrickfreed · 2021-10-19T22:39:16Z

src/uuid/mod.rs

+    where
+        D: serde::Deserializer<'de>,
+    {
+        match deserializer.deserialize_newtype_struct(UUID_NEWTYPE_NAME, BsonVisitor)? {


deserialization works by signaling to the deserializer, again through the newtype name, that we're looking for a UUID. The BSON deserializers ensure that a binary subtype 4 is present in the data and then pass it on. Non-BSON deserializers will just attempt to deserialize a Bson normally, and from there we deserialize a uuid::Uuid manually.

patrickfreed · 2021-10-19T22:45:09Z

src/uuid/test.rs

+}
+
+#[test]
+fn test_binary_constructors() {


this test and below were moved, the above ones are new.

abr-egn

LGTM! Minor comment nit only.

abr-egn · 2021-10-20T16:12:58Z

src/uuid/mod.rs

+//! # use serde::{Serialize, Deserialize};
+//! # #[derive(Serialize, Deserialize)]
+//! # struct Foo {
+//! #   /// serializes as a String or subtype 0 BSON binary, depending


These comments are somewhat confusing to include in this example.

Whoops, those were copy/paste leftovers, removed.

patrickfreed

One issue I noticed with the current approach to deserialization here is that our custom deserialization logic will be skipped if the Uuid is part of a #[serde(flatten)] chain (see serde-rs/serde#2106). This means that Uuids will erroneously allow deserialization from Binary values with subtype 0 in those cases. The driver currently deserializes all responses using #[serde(flatten)], so in practice this means this behavior will affect most users.

We could fix this issue by using the extended JSON format like we do for ObjectId and DateTime instead of the newtype pattern, but I think using uuid::Uuid's serde implementation in non-BSON contexts is a valuable enough of a feature to warrant living with slightly too permissive deserialization in BSON. Note that this issue doesn't affect serialization, only deserialization. Thoughts?

patrickfreed · 2021-10-20T19:43:18Z

src/uuid/mod.rs

+//! # use serde::{Serialize, Deserialize};
+//! # #[derive(Serialize, Deserialize)]
+//! # struct Foo {
+//! #   /// serializes as a String or subtype 0 BSON binary, depending


Whoops, those were copy/paste leftovers, removed.

abr-egn · 2021-10-21T13:31:00Z

One issue I noticed with the current approach to deserialization here is that our custom deserialization logic will be skipped if the Uuid is part of a #[serde(flatten)] chain (see serde-rs/serde#2106). This means that Uuids will erroneously allow deserialization from Binary values with subtype 0 in those cases. The driver currently deserializes all responses using #[serde(flatten)], so in practice this means this behavior will affect most users.

We could fix this issue by using the extended JSON format like we do for ObjectId and DateTime instead of the newtype pattern, but I think using uuid::Uuid's serde implementation in non-BSON contexts is a valuable enough of a feature to warrant living with slightly too permissive deserialization in BSON. Note that this issue doesn't affect serialization, only deserialization. Thoughts?

I agree, since it's permissive deserialization rather than broken serialization it seems fine to accept as trade-off for the other utility here. Might want to document that it's accidental and unsupported so that if serde fixes the bug it (hopefully) doesn't break user code.

isabelatkinson · 2021-10-21T14:46:30Z

src/uuid/mod.rs

+#[cfg(feature = "uuid-0_8")]
+#[cfg_attr(docsrs, doc(cfg(feature = "uuid-0_8")))]
+impl Uuid {
+    /// Create a [`Uuid`] from a [`uuid::Uuid`](https://docs.rs/uuid/latest/uuid/struct.Uuid.html) from


Do we need these methods in the API in addition to the From impls?

While they are technically redundant, I think they're useful for visibility. This is also consitent with our to_chrono and from_chrono methods on bson::DateTime.

isabelatkinson · 2021-10-21T14:47:51Z

src/uuid/mod.rs

+}
+
+impl Binary {
+    /// Serializes a [Uuid](https://docs.rs/uuid/0.8.2/uuid/) into BSON [`Binary`] type


Should this link the uuid::Uuid type since it's returning a crate::Uuid? Ditto the other methods below.

Good catch, fixed.

src/uuid/mod.rs

isabelatkinson · 2021-10-21T15:17:12Z

I agree, since it's permissive deserialization rather than broken serialization it seems fine to accept as trade-off for the other utility here. Might want to document that it's accidental and unsupported so that if serde fixes the bug it (hopefully) doesn't break user code.

Agreed

Co-authored-by: Isabel Atkinson <isabelatkinson@gmail.com>

patrickfreed · 2021-10-21T17:52:48Z

Might want to document that it's accidental and unsupported so that if serde fixes the bug it (hopefully) doesn't break user code.

Good idea, done.

patrickfreed added 11 commits October 14, 2021 18:20

wip introduce bson::Uuid type

a02bc6a

TODO: documentation, tests, API exam

wip docs, add support to Bson serializer

f9a4ded

fix docs, add test

6ad1d1c

rename as_bytes -> bytes

bb15dcf

move uuid tests from bson tests

846db63

fix test

d91f54a

touch up uuid module docs

8c89639

dont deserialize from subtype 0 in BSON

2278225

fix clippy

0ce106f

add some explanatory comments

3207f99

various cleanup

6bbbb88

patrickfreed commented Oct 19, 2021

View reviewed changes

patrickfreed marked this pull request as ready for review October 19, 2021 23:21

patrickfreed requested review from isabelatkinson and abr-egn October 19, 2021 23:22

add docstring to newtype name, use more unique name

7884027

abr-egn approved these changes Oct 20, 2021

View reviewed changes

remove unnecessary comments

7eb206b

patrickfreed commented Oct 20, 2021

View reviewed changes

isabelatkinson reviewed Oct 21, 2021

View reviewed changes

patrickfreed and others added 3 commits October 21, 2021 13:37

Update src/uuid/mod.rs

b7bb941

Co-authored-by: Isabel Atkinson <isabelatkinson@gmail.com>

fix links

e1d0f7e

add note about serde bug

7c93cc8

isabelatkinson approved these changes Oct 21, 2021

View reviewed changes

fix lint

f2edea9

patrickfreed merged commit 822db72 into mongodb:master Oct 21, 2021

patrickfreed mentioned this pull request Nov 2, 2021

RUST-1082 Implement Serialize and Deserialize for raw BSON types #318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-465 Introduce a `bson::Uuid` wrapper type #314

RUST-465 Introduce a `bson::Uuid` wrapper type #314

patrickfreed commented Oct 19, 2021 •

edited

Loading

patrickfreed Oct 19, 2021

patrickfreed Oct 19, 2021

patrickfreed Oct 19, 2021

patrickfreed Oct 19, 2021

patrickfreed Oct 19, 2021

patrickfreed Oct 19, 2021

abr-egn left a comment

abr-egn Oct 20, 2021

patrickfreed Oct 20, 2021

patrickfreed left a comment

patrickfreed Oct 20, 2021

abr-egn commented Oct 21, 2021

isabelatkinson Oct 21, 2021

patrickfreed Oct 21, 2021

isabelatkinson Oct 21, 2021

patrickfreed Oct 21, 2021

isabelatkinson commented Oct 21, 2021

patrickfreed commented Oct 21, 2021

RUST-465 Introduce a bson::Uuid wrapper type #314

RUST-465 Introduce a bson::Uuid wrapper type #314

Conversation

patrickfreed commented Oct 19, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn commented Oct 21, 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

isabelatkinson commented Oct 21, 2021

patrickfreed commented Oct 21, 2021

RUST-465 Introduce a `bson::Uuid` wrapper type #314

RUST-465 Introduce a `bson::Uuid` wrapper type #314

patrickfreed commented Oct 19, 2021 •

edited

Loading