RUST-765 Ensure API meets Rust API guidelines #255

patrickfreed · 2021-05-10T23:28:54Z

RUST-765 / RUST-789 / RUST-788 / RUST-739

This PR makes a number of breaking changes to the API to ensure it meets the Rust API Guidelines checklist.

Notable changes:

Restructured or eliminated public dependencies on unstable crates to be compatible with future major releases of those crates
- chrono:
  - Bson::DateTime(chrono::DateTime<Utc>) => Bson::DateTime(crate::DateTime)
  - removed pub from the wrapped chrono::DateTime in bson::DateTime
  - serde_helpers::chrono_datetime_as_bson_datetime => serde_helpers::chrono_0_4_datetime_as_bson_datetime
  - ObjectId::timestamp now returns a crate::DateTime
- uuid:
  - serde_helpers::uuid_as_binary => serde_helpers::uuid_0_8_as_binary
- hex: rewrote oid::Error to exclude it
All Document getters now accept impl AsRef<str> instead of &str
Updated ObjectId's constructors to match the names of uuid::Uuid
Attempting to serialize a DateTime with sub-millisecond precision to BSON data will result in an error
Renamed error variants to be less redundant and more consistent
- de::Error:
  - Error:Io(...) => Error::Io(...)
  - Error::FromUtf8Error(...) => Error::InvalidUtf8String(...)
  - Error::SyntaxError removed, replaced by Error::DeserializationError
- ser::Error:
  - Error::IoError(...) => Error::Io(...)

…TER-TY)

patrickfreed · 2021-05-10T23:51:40Z

src/bson.rs

-        Utc.timestamp(num_secs, num_millis as u32 * 1_000_000)
-            .into()
+impl crate::DateTime {
+    pub(crate) fn from_millis(date: i64) -> Self {


I thought about making this public, but apparently too-large values here can cause panics (RUST-799), so I just left it private for now until we decide how / if we want to tackle this publicly.

patrickfreed · 2021-05-10T23:52:17Z

src/bson.rs

-impl From<chrono::DateTime<Utc>> for DateTime {
-    fn from(x: chrono::DateTime<Utc>) -> Self {
-        DateTime(x)
+impl<T: chrono::TimeZone> From<chrono::DateTime<T>> for crate::DateTime {


I updated this to be more general since it could be a little annoying to convert from DateTimes with other timezones otherwise.

patrickfreed · 2021-05-10T23:54:15Z

src/serde_helpers.rs

 ///     pub id: Uuid,
 /// }
 /// ```
-pub mod uuid_as_binary {
+pub mod uuid_0_8_as_binary {


Note that with #239, each major version of uuid will come with 4 serde helper modules, which will pollute the namespace a bit. That doesn't seem too bad to me but we could consider gating it behind a feature flag if we want to avoid that.

I don't feel very strongly either way

Yeah I don't think that's a huge deal, I'm fine leaving as-is.

patrickfreed · 2021-05-11T17:19:07Z

In light of our decision in mongodb/mongo-rust-driver#337 to drop the Error suffix from our error variants, I updated BSON's errors similarly.

kmahar

all seems pretty good, just some minor questions/comments

examples/serialize.rs

kmahar · 2021-05-11T23:33:43Z

src/serde_helpers.rs

 ///     pub id: Uuid,
 /// }
 /// ```
-pub mod uuid_as_binary {
+pub mod uuid_0_8_as_binary {


I don't feel very strongly either way

kmahar · 2021-05-11T23:39:16Z

src/bson.rs

 ///
-/// Just a helper for convenience
+/// This type differs from [`chrono::DateTime`] in that it serializes to and deserializes from a
+/// BSON datetime rather than an ISO-8601 formatted string. This means that it will serialize to and


what does encoding as an extended JSON object entail in a non-BSON, non-extended JSON format? using the extJSON key names?

Yep, so the way the crate implemented Serialize / Deserialize for the various types was to do BSON in BSON and extended JSON (or its equivalent) for all other serialization formats (This is something I suggested we try in Swift a while back). Users have expressed a decent amount of frustration around this (particularly around DateTime), though the serde helpers have helped a lot. I added this comment in here to further help clarify the situation, though I agree that part is a bit confusing. Updated the wording to be a bit more specific.

Given that we're breaking API, we could consider changing this approach, though I think that would probably require a lot of work. We could additively support alternate default serialization formats in the future via feature flags too, so it's not completely necessary to do it now.

ah I see. yeah, I don't think we should try to tackle this now given there are ways to work around it, but seems like something good to keep in mind for the future.

Filed RUST-801.

patrickfreed · 2021-05-12T02:01:05Z

src/bson.rs

 ///
-/// Just a helper for convenience
+/// This type differs from [`chrono::DateTime`] in that it serializes to and deserializes from a
+/// BSON datetime rather than an ISO-8601 formatted string. This means that it will serialize to and


Yep, so the way the crate implemented Serialize / Deserialize for the various types was to do BSON in BSON and extended JSON (or its equivalent) for all other serialization formats (This is something I suggested we try in Swift a while back). Users have expressed a decent amount of frustration around this (particularly around DateTime), though the serde helpers have helped a lot. I added this comment in here to further help clarify the situation, though I agree that part is a bit confusing. Updated the wording to be a bit more specific.

Given that we're breaking API, we could consider changing this approach, though I think that would probably require a lot of work. We could additively support alternate default serialization formats in the future via feature flags too, so it's not completely necessary to do it now.

patrickfreed · 2021-05-12T02:02:03Z

src/ser/error.rs

@@ -17,6 +17,9 @@ pub enum Error {
        key: Bson,
    },

+    /// Attempted to serialize a sub-millisecond precision datetime.
+    SubMillisecondPrecisionDateTime(crate::DateTime),


Does this error name seem okay? it's a little verbose but I couldn't come up with anything much better.

Yeah it's a little lengthy but I think it's fine

agree it's fine. might be worth expanding the comment a little to clarify why that's an error though, e.g. adding "...which BSON does not support" or something like that?

Good idea, added.

isabelatkinson · 2021-05-12T14:48:20Z

src/document.rs

@@ -500,14 +508,14 @@ impl Document {
    }

    /// Attempts to serialize the `Document` into a byte stream.
-    pub fn to_writer<W: Write + ?Sized>(&self, writer: &mut W) -> crate::ser::Result<()> {
+    pub fn to_writer<W: Write>(&self, mut writer: W) -> crate::ser::Result<()> {


What is the reasoning behind this change?

This is in line with the C-RW-VALUE guideline. Though it does mention to document this change, so I updated the docstring to contain some explanation and an example.

isabelatkinson · 2021-05-12T14:55:23Z

src/oid.rs

    }

    /// Constructs a new ObjectId wrapper around the raw byte representation.
-    pub fn with_bytes(bytes: [u8; 12]) -> ObjectId {
+    pub const fn from_bytes(bytes: [u8; 12]) -> ObjectId {


TIL Rust has const functions. Is this suggested as part of the API guidelines?

Nope, though there probably should be some mention of it there. I think it was quite limited for a while, but each release of Rust adds more things that can be done in const. In this case, I used it to match uuid::Uuid::from_bytes.

isabelatkinson · 2021-05-12T14:58:09Z

src/ser/error.rs

@@ -17,6 +17,9 @@ pub enum Error {
        key: Bson,
    },

+    /// Attempted to serialize a sub-millisecond precision datetime.
+    SubMillisecondPrecisionDateTime(crate::DateTime),


Yeah it's a little lengthy but I think it's fine

isabelatkinson · 2021-05-12T15:01:14Z

src/serde_helpers.rs

 ///     pub id: Uuid,
 /// }
 /// ```
-pub mod uuid_as_binary {
+pub mod uuid_0_8_as_binary {


Yeah I don't think that's a huge deal, I'm fine leaving as-is.

kmahar

couple minor comments (sorry I screwed up and forgot to batch...) but don't need to re-review, lgtm

kmahar · 2021-05-12T20:42:06Z

oh and extremely small nit, in your PR title (which I think will become the commit message by default?): "guideliness" -> "guidelines"

…VALUE)

" (C-HTML-ROOT)

patrickfreed added 3 commits May 6, 2021 14:41

RUST-765 Remove deprecated conversion methods from Decimal128 (C-CONV)

5692c3f

RUST-765 Iterator type names match the methods that produce them (C-I…

2503d00

…TER-TY)

RUST-789 Use more standard conversion constuctors for ObjectId

bbde2e8

patrickfreed marked this pull request as ready for review May 10, 2021 23:50

patrickfreed requested review from isabelatkinson and kmahar May 10, 2021 23:50

patrickfreed commented May 10, 2021

View reviewed changes

kmahar reviewed May 11, 2021

View reviewed changes

patrickfreed commented May 12, 2021

View reviewed changes

isabelatkinson reviewed May 12, 2021

View reviewed changes

kmahar approved these changes May 12, 2021

View reviewed changes

isabelatkinson approved these changes May 12, 2021

View reviewed changes

patrickfreed changed the title ~~RUST-765 Ensure API meets Rust API guideliness~~ RUST-765 Ensure API meets Rust API guidelines May 12, 2021

patrickfreed added 7 commits May 12, 2021 18:17

RUST-765 from_reader and to_writer take reader/writer by value (C-RW-…

96edeba

…VALUE)

RUST-765 Cargo.toml includes all common metadata (C-METADATA)

9b5666a

RUST-765 Crate sets html_root_url attribute "https://docs.rs/CRATE/X.Y.Z

7638ca3

" (C-HTML-ROOT)

RUST-788 Accept impl AsRef<str> in Document methods (C-GENERIC)

3afd1b8

RUST-739 Eliminate public dependencies on unstable crates (C-STABLE)

6d3fad4

RUST-765 Ensure public structs are future proof (C-STRUCT-PRIVATE)

44e1076

RUST-765 Drop "Error" suffixes where appropriate (C-WORD-ORDER)

6ae63bc

patrickfreed force-pushed the RUST-765/api-guidelines branch from 4a52ec0 to 6ae63bc Compare May 12, 2021 22:18

patrickfreed merged commit 097fbe5 into mongodb:master May 13, 2021

RUST-765 Ensure API meets Rust API guidelines #255

RUST-765 Ensure API meets Rust API guidelines #255

Uh oh!

Conversation

patrickfreed commented May 10, 2021 • 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!

patrickfreed commented May 11, 2021

Uh oh!

kmahar left a comment

Choose a reason for hiding this comment

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!

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!

kmahar left a comment

Choose a reason for hiding this comment

Uh oh!

kmahar commented May 12, 2021

Uh oh!

Uh oh!

patrickfreed commented May 10, 2021 •

edited

Loading