Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from
May 11, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
26dd227
RUST-765 Remove Deref implementation from `Error` (C-DEREF)
patrickfreed Apr 30, 2021
b112b31
RUST-765 Use consistent ErrorKind variant names (C-WORD-ORDER)
patrickfreed May 4, 2021
dab55f2
RUST-765 Cargo.toml includes all common metadata (C-METADATA)
patrickfreed May 4, 2021
ec8995b
RUST-765 Crate sets html_root_url attribute (C-HTML-ROOT)
patrickfreed May 4, 2021
46a6a4b
minor: document that unacknowledged write concerns are unsupported
patrickfreed May 6, 2021
2602c71
RUST-765 Depend on futures subcrates instead of futures (C-STABLE)
patrickfreed May 6, 2021
b0a0378
RUST-791 use PathBuf instead of String for paths (C-CUSTOM-TYPE)
patrickfreed May 6, 2021
d860375
RUST-788 Use impl AsRef<str> instead of &str (C-GENERIC)
patrickfreed May 5, 2021
c9a6d3f
RUST-786 Update collation fields to use custom types (C-CUSTOM-TYPE)
patrickfreed May 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
authors = ["Saghm Rossi <[email protected]>", "Patrick Freed <[email protected]>", "Isabel Atkinson <[email protected]>"]
description = "The official MongoDB driver for Rust"
edition = "2018"
documentation = "https://docs.rs/mongodb"
keywords = ["mongo", "mongodb", "database", "bson", "nosql"]
categories = ["asynchronous", "database", "web-programming"]
repository = "https://github.com/mongodb/mongo-rust-driver"
license = "Apache-2.0"
readme = "README.md"
Expand Down Expand Up @@ -37,7 +37,10 @@ bitflags = "1.1.0"
bson = { git = "https://github.com/mongodb/bson-rust" }
chrono = "0.4.7"
derivative = "2.1.1"
futures = "0.3.5"
futures-core = "0.3.14"
futures-io = "0.3.14"
futures-util = { version = "0.3.14", features = ["io"] }
futures-executor = "0.3.14"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

hex = "0.4.0"
hmac = "0.10.1"
lazy_static = "1.4.0"
Expand Down Expand Up @@ -110,6 +113,7 @@ features = ["v4"]
approx = "0.4.0"
derive_more = "0.99.13"
function_name = "0.2.0"
futures = "0.3"
pretty_assertions = "0.7.1"
serde_json = "1.0.64"
semver = "0.11.0"
Expand Down
3 changes: 2 additions & 1 deletion src/bson_util/async_encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use futures::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt};
use futures_io::{AsyncRead, AsyncWrite};
use futures_util::{AsyncReadExt, AsyncWriteExt};

use crate::{
bson::Document,
Expand Down
4 changes: 2 additions & 2 deletions src/bson_util/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ pub(crate) fn first_key(document: &Document) -> Option<&str> {
pub(crate) fn replacement_document_check(replacement: &Document) -> Result<()> {
match first_key(replacement) {
Some(s) if !s.starts_with('$') => Ok(()),
_ => Err(ErrorKind::ArgumentError {
_ => Err(ErrorKind::InvalidArgument {
message: "replace document must have first key not starting with '$".to_string(),
}
.into()),
Expand All @@ -67,7 +67,7 @@ pub(crate) fn replacement_document_check(replacement: &Document) -> Result<()> {
pub(crate) fn update_document_check(update: &Document) -> Result<()> {
match first_key(update) {
Some(s) if s.starts_with('$') => Ok(()),
_ => Err(ErrorKind::ArgumentError {
_ => Err(ErrorKind::InvalidArgument {
message: "update document must have first key starting with '$".to_string(),
}
.into()),
Expand Down
26 changes: 13 additions & 13 deletions src/client/auth/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ impl AuthMechanism {
match self {
AuthMechanism::ScramSha1 | AuthMechanism::ScramSha256 => {
if credential.username.is_none() {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "No username provided for SCRAM authentication".to_string(),
}
.into());
Expand All @@ -123,14 +123,14 @@ impl AuthMechanism {
}
AuthMechanism::MongoDbX509 => {
if credential.password.is_some() {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "A password cannot be specified with MONGODB-X509".to_string(),
}
.into());
}

if credential.source.as_deref().unwrap_or("$external") != "$external" {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "only $external may be specified as an auth source for \
MONGODB-X509"
.to_string(),
Expand All @@ -142,21 +142,21 @@ impl AuthMechanism {
}
AuthMechanism::Plain => {
if credential.username.is_none() {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "No username provided for PLAIN authentication".to_string(),
}
.into());
}

if credential.username.as_deref() == Some("") {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "Username for PLAIN authentication must be non-empty".to_string(),
}
.into());
}

if credential.password.is_none() {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "No password provided for PLAIN authentication".to_string(),
}
.into());
Expand All @@ -167,7 +167,7 @@ impl AuthMechanism {
#[cfg(feature = "tokio-runtime")]
AuthMechanism::MongoDbAws => {
if credential.username.is_some() && credential.password.is_none() {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "Username cannot be provided without password for MONGODB-AWS \
authentication"
.to_string(),
Expand Down Expand Up @@ -235,13 +235,13 @@ impl AuthMechanism {
Self::Plain => Ok(None),
#[cfg(feature = "tokio-runtime")]
AuthMechanism::MongoDbAws => Ok(None),
AuthMechanism::MongoDbCr => Err(ErrorKind::AuthenticationError {
AuthMechanism::MongoDbCr => Err(ErrorKind::Authentication {
message: "MONGODB-CR is deprecated and not supported by this driver. Use SCRAM \
for password-based authentication instead"
.into(),
}
.into()),
_ => Err(ErrorKind::AuthenticationError {
_ => Err(ErrorKind::Authentication {
message: format!("Authentication mechanism {:?} not yet implemented.", self),
}
.into()),
Expand Down Expand Up @@ -278,13 +278,13 @@ impl AuthMechanism {
AuthMechanism::MongoDbAws => {
aws::authenticate_stream(stream, credential, server_api, http_client).await
}
AuthMechanism::MongoDbCr => Err(ErrorKind::AuthenticationError {
AuthMechanism::MongoDbCr => Err(ErrorKind::Authentication {
message: "MONGODB-CR is deprecated and not supported by this driver. Use SCRAM \
for password-based authentication instead"
.into(),
}
.into()),
_ => Err(ErrorKind::AuthenticationError {
_ => Err(ErrorKind::Authentication {
message: format!("Authentication mechanism {:?} not yet implemented.", self),
}
.into()),
Expand All @@ -307,12 +307,12 @@ impl FromStr for AuthMechanism {
#[cfg(feature = "tokio-runtime")]
MONGODB_AWS_STR => Ok(AuthMechanism::MongoDbAws),
#[cfg(not(feature = "tokio-runtime"))]
MONGODB_AWS_STR => Err(ErrorKind::ArgumentError {
MONGODB_AWS_STR => Err(ErrorKind::InvalidArgument {
message: "MONGODB-AWS auth is only supported with the tokio runtime".into(),
}
.into()),

_ => Err(ErrorKind::ArgumentError {
_ => Err(ErrorKind::InvalidArgument {
message: format!("invalid mechanism string: {}", str),
}
.into()),
Expand Down
8 changes: 4 additions & 4 deletions src/client/executor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl Client {
) -> Result<T::O> {
// TODO RUST-9: allow unacknowledged write concerns
if !op.is_acknowledged() {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "Unacknowledged write concerns are not supported".to_string(),
}
.into());
Expand Down Expand Up @@ -110,7 +110,7 @@ impl Client {

// Retryable writes are only supported by storage engines with document-level
// locking, so users need to disable retryable writes if using mmapv1.
if let ErrorKind::CommandError(ref mut command_error) = *err.kind {
if let ErrorKind::Command(ref mut command_error) = *err.kind {
if command_error.code == 20
&& command_error.message.starts_with("Transaction numbers")
{
Expand Down Expand Up @@ -218,13 +218,13 @@ impl Client {
session.update_last_use();
}
Some(ref session) if !op.supports_sessions() && !session.is_implicit() => {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: format!("{} does not support sessions", cmd.name),
}
.into());
}
Some(ref session) if !op.is_acknowledged() && !session.is_implicit() => {
return Err(ErrorKind::ArgumentError {
return Err(ErrorKind::InvalidArgument {
message: "Cannot use ClientSessions with unacknowledged write concern"
.to_string(),
}
Expand Down
8 changes: 4 additions & 4 deletions src/client/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm

let options = ClientOptions::parse_uri(uri.as_ref(), None).await?;

Client::with_options(options)
}
Expand Down Expand Up @@ -184,7 +184,7 @@ impl Client {
.into_iter()
.map(|doc| {
let name = doc.get("name").and_then(Bson::as_str).ok_or_else(|| {
ErrorKind::ResponseError {
ErrorKind::InvalidResponse {
message: "Expected \"name\" field in server response, but it was not \
found"
.to_string(),
Expand Down Expand Up @@ -309,7 +309,7 @@ impl Client {
.await;

if !change_occurred {
return Err(ErrorKind::ServerSelectionError {
return Err(ErrorKind::ServerSelection {
message: self
.inner
.topology
Expand Down
Loading