RUST-765 Conform API to the Rust API guidelines #337

patrickfreed · 2021-05-07T02:15:27Z

RUST-765 / RUST-791 / RUST-788 / RUST-786

This PR makes numerous small changes to the API to conform to the Rust API Guidelines Checklist. Each commit is tagged with the checklist item that its addressing. Most of the commits are just associated with the parent RUST-765 ticket, but for some of the more meaningful changes I filed separate tickets for the sake of the release notes.

patrickfreed · 2021-05-07T02:28:12Z

Cargo.toml

+futures-core = "0.3.14"
+futures-io = "0.3.14"
+futures-util = { version = "0.3.14", features = ["io"] }
+futures-executor = "0.3.14"


The futures crate contains a number of async-related APIs that are widely used in the community, the most important one (for us) being the Stream trait, which our various cursor types conform to (it's basically the async iterator trait). Because we publicly conform to it, we'll need to do so for each major version of futures to ensure full compatibility. The futures crate is really just a collection of rexports from "sub" crates though (many of which are listed here now), each of which has differing stability exceptions. The futures-core one, which contains Stream, is purposefully much more stable than the others, and it will likely go straight from 0.3 to 1.0 without any more breaking versions, whereas futures-util and futures itself will have potentially many major versions. For that reason, I updated this to depend directly on futures-core instead of futures to reduce the number of separate Stream implementations we'll need to provide for the lifetime of 2.0. This page gives a nice overview of the situation and why we need to do this.

patrickfreed · 2021-05-07T02:29:16Z

src/concern/mod.rs

-    /// the driver will not receive a response indicating whether an operation succeeded or failed.
-    /// It also means that the operation cannot be associated with a session. It is reccommended to
-    /// avoid using unacknowledged write concerns.
+    /// Note: specifying 0 here indicates that the write concern is unacknowledged, which is


I meant to include this change in the last PR (I even commented that I did but forgot to push the commit), so I included it here.

patrickfreed · 2021-05-07T02:29:50Z

src/error.rs

@@ -193,6 +193,98 @@ impl Error {
    pub(crate) fn from_resolve_error(error: trust_dns_resolver::error::ResolveError) -> Self {
        ErrorKind::DnsResolveError { message: error.to_string() }.into()
    }
+


These methods were moved from being impl ErrorKind to just being on impl Error since the Deref implementation is gone now. They're all internal anyways so there isn't any impact on the public API.

patrickfreed · 2021-05-07T02:32:46Z

src/error.rs

@@ -257,11 +341,11 @@ pub enum ErrorKind {

    /// Wrapper around `bson::de::Error`.
    #[error("{0}")]
-    BsonDecode(crate::bson::de::Error),
+    BsonDeserializationError(crate::bson::de::Error),


I updated the terminology here and appended the "Error" suffix to the end to match the other cases. I wonder if it would actually be better if we dropped all the "Error" suffixes though, since they're kind of redundant with ErrorKind. I'm not really sure which way would be better, but I just opted for this since it was the least amount of changes.

match *e.kind { ErrorKind::IoError(e) => { ... }, ErrorKind::CommandError(e) => { ... }, ErrorKind::ArgumentError { message } => { ... }, // rest of cases... } // vs match *e.kind { ErrorKind::Io(e) => { ... }, ErrorKind::Command(e) => { ... }, ErrorKind::Argument { message } => { ... }, // rest of cases... }

I don't feel strongly, but I think the choice to break things less is sensible

There's actually a relevant clippy lint here that complains when all enum variants have the same suffix; I ran into this once before when I was making the Retryability enum and all of the variants were suffixed with Retryable (i.e. ReadRetryable, WriteRetryable, NonRetryable). I think we're avoiding it for now since SessionsNotSupported currently doesn't have the Error suffix as you point out below, but I do think it would be nicer to get rid of the Error in the names in the spirit of this lint. I don't feel super strongly about this though so I'll leave it up to you; I agree it's best to minimize changes when we don't have a strong opinion on something.

Oh interesting about that clippy rule. I think given that lint, I think I lean towards dropping Error from everything then, since it seems like we are violating the spirit of it with the current names.

given that, dropping Error SGTM

Updated to drop suffix. I added the Invalid prefix to the Argument, Response, and TlsConfig variants to convey that something malformed was produced somewhere.

src/error.rs

kmahar · 2021-05-07T20:56:31Z

src/error.rs

@@ -257,11 +341,11 @@ pub enum ErrorKind {

    /// Wrapper around `bson::de::Error`.
    #[error("{0}")]
-    BsonDecode(crate::bson::de::Error),
+    BsonDeserializationError(crate::bson::de::Error),


I don't feel strongly, but I think the choice to break things less is sensible

src/lib.rs

src/collation.rs

src/error.rs

isabelatkinson · 2021-05-07T22:17:04Z

src/client/mod.rs

@@ -98,8 +98,8 @@ impl Client {
    ///
    /// See the documentation on
    /// [`ClientOptions::parse`](options/struct.ClientOptions.html#method.parse) for more details.
-    pub async fn with_uri_str(uri: &str) -> Result<Self> {
-        let options = ClientOptions::parse_uri(uri, None).await?;
+    pub async fn with_uri_str(uri: impl AsRef<str>) -> Result<Self> {


I think we should also update the collection/collection_with_options and database/database_with_options methods to take impl AsRef<str> for the name params.

Thank you for bringing this up, I meant to open a thread discussing this in my original review.

Those two methods are a little tricky because they have generic parameters, so if we include impl AsRef as an argument, the turbofish operator will no longer work. If we introduce it as a regular generic argument instead, then users will have to specify both of them if they use the turbofish. When weighing all of these, I figured the inconsistency of requiring &str here was the best option of the three. What do you think? cc @kmahar

Oh right, I forgot about that restriction. I agree with your reasoning; I don't think the small benefit of having impl AsRef here is worth complicating the turbofish stuff.

src/lib.rs

isabelatkinson · 2021-05-07T22:24:40Z

src/error.rs

@@ -257,11 +341,11 @@ pub enum ErrorKind {

    /// Wrapper around `bson::de::Error`.
    #[error("{0}")]
-    BsonDecode(crate::bson::de::Error),
+    BsonDeserializationError(crate::bson::de::Error),


There's actually a relevant clippy lint here that complains when all enum variants have the same suffix; I ran into this once before when I was making the Retryability enum and all of the variants were suffixed with Retryable (i.e. ReadRetryable, WriteRetryable, NonRetryable). I think we're avoiding it for now since SessionsNotSupported currently doesn't have the Error suffix as you point out below, but I do think it would be nicer to get rid of the Error in the names in the spirit of this lint. I don't feel super strongly about this though so I'll leave it up to you; I agree it's best to minimize changes when we don't have a strong opinion on something.

isabelatkinson · 2021-05-11T14:31:54Z

src/client/mod.rs

@@ -98,8 +98,8 @@ impl Client {
    ///
    /// See the documentation on
    /// [`ClientOptions::parse`](options/struct.ClientOptions.html#method.parse) for more details.
-    pub async fn with_uri_str(uri: &str) -> Result<Self> {
-        let options = ClientOptions::parse_uri(uri, None).await?;
+    pub async fn with_uri_str(uri: impl AsRef<str>) -> Result<Self> {


Oh right, I forgot about that restriction. I agree with your reasoning; I don't think the small benefit of having impl AsRef here is worth complicating the turbofish stuff.

RUST-765 Remove Deref implementation from Error (C-DEREF)

26dd227

patrickfreed commented May 7, 2021

View reviewed changes

patrickfreed marked this pull request as ready for review May 7, 2021 16:49

patrickfreed requested review from isabelatkinson and kmahar May 7, 2021 16:49

kmahar reviewed May 7, 2021

View reviewed changes

isabelatkinson reviewed May 7, 2021

View reviewed changes

isabelatkinson approved these changes May 11, 2021

View reviewed changes

kmahar approved these changes May 11, 2021

View reviewed changes

patrickfreed added 8 commits May 11, 2021 12:55

RUST-765 Use consistent ErrorKind variant names (C-WORD-ORDER)

b112b31

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

dab55f2

RUST-765 Crate sets html_root_url attribute (C-HTML-ROOT)

ec8995b

minor: document that unacknowledged write concerns are unsupported

46a6a4b

RUST-765 Depend on futures subcrates instead of futures (C-STABLE)

2602c71

RUST-791 use PathBuf instead of String for paths (C-CUSTOM-TYPE)

b0a0378

RUST-788 Use impl AsRef<str> instead of &str (C-GENERIC)

d860375

RUST-786 Update collation fields to use custom types (C-CUSTOM-TYPE)

c9a6d3f

patrickfreed force-pushed the RUST-765/api-guidelines branch from ae94de5 to c9a6d3f Compare May 11, 2021 16:59

patrickfreed mentioned this pull request May 11, 2021

RUST-765 Ensure API meets Rust API guidelines mongodb/bson-rust#255

Merged

patrickfreed merged commit 79d2782 into mongodb:master May 11, 2021

patrickfreed mentioned this pull request Aug 4, 2022

RUST-1392 Add GridFS support: Implement public API with placeholder code #688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-765 Conform API to the Rust API guidelines #337

RUST-765 Conform API to the Rust API guidelines #337

patrickfreed commented May 7, 2021

patrickfreed May 7, 2021

patrickfreed May 7, 2021

patrickfreed May 7, 2021

patrickfreed May 7, 2021

kmahar May 7, 2021

isabelatkinson May 7, 2021

patrickfreed May 10, 2021

kmahar May 10, 2021

patrickfreed May 11, 2021

kmahar May 7, 2021

isabelatkinson May 7, 2021

patrickfreed May 10, 2021

isabelatkinson May 11, 2021

kmahar May 11, 2021

isabelatkinson May 7, 2021

isabelatkinson May 11, 2021

RUST-765 Conform API to the Rust API guidelines #337

RUST-765 Conform API to the Rust API guidelines #337

Conversation

patrickfreed commented May 7, 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