Skip to content

RUST-740 Return Deserialize structs from list_databases and list_collections #328

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.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 26 additions & 3 deletions src/bson_util/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
pub(crate) mod async_encoding;

use std::time::Duration;
use std::{convert::TryFrom, time::Duration};

use serde::{ser, Deserialize, Deserializer, Serialize, Serializer};
use serde::{de::Error, ser, Deserialize, Deserializer, Serialize, Serializer};

use crate::{
bson::{doc, oid::ObjectId, Binary, Bson, Document, JavaScriptCodeWithScope, Regex},
Expand All @@ -15,7 +15,18 @@ pub(crate) fn get_int(val: &Bson) -> Option<i64> {
match *val {
Bson::Int32(i) => Some(i64::from(i)),
Bson::Int64(i) => Some(i),
Bson::Double(f) if f == f as i64 as f64 => Some(f as i64),
Bson::Double(f) if (f - (f as i64 as f64)).abs() <= f64::EPSILON => Some(f as i64),
_ => None,
}
}

/// Coerce numeric types into an `u64` if it would be lossless to do so. If this Bson is not numeric
/// or the conversion would be lossy (e.g. 1.5 -> 1), this returns `None`.
pub(crate) fn get_u64(val: &Bson) -> Option<u64> {
match *val {
Bson::Int32(i) => u64::try_from(i).ok(),
Bson::Int64(i) => u64::try_from(i).ok(),
Bson::Double(f) if (f - (f as u64 as f64)).abs() <= f64::EPSILON => Some(f as u64),
_ => None,
}
}
Expand Down Expand Up @@ -125,6 +136,18 @@ pub(crate) fn serialize_batch_size<S: Serializer>(
}
}

/// Deserialize an u64 from any BSON number type if it could be done losslessly.
pub(crate) fn deserialize_u64_from_bson_number<'de, D>(
deserializer: D,
) -> std::result::Result<u64, D::Error>
where
D: Deserializer<'de>,
{
let bson = Bson::deserialize(deserializer)?;
get_u64(&bson)
.ok_or_else(|| D::Error::custom(format!("could not deserialize u64 from {:?}", bson)))
}

pub fn doc_size_bytes(doc: &Document) -> usize {
//
// * i32 length prefix (4 bytes)
Expand Down
12 changes: 9 additions & 3 deletions src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ pub mod session;

use std::{sync::Arc, time::Duration};

use bson::Bson;
use derivative::Derivative;
use std::time::Instant;

#[cfg(test)]
use crate::options::StreamAddress;
use crate::{
bson::{Bson, Document},
bson::Document,
concern::{ReadConcern, WriteConcern},
db::Database,
error::{ErrorKind, Result},
Expand All @@ -25,6 +26,7 @@ use crate::{
SelectionCriteria,
SessionOptions,
},
results::DatabaseSpecification,
sdam::{SelectedServer, SessionSupportStatus, Topology},
ClientSession,
};
Expand Down Expand Up @@ -161,9 +163,13 @@ impl Client {
&self,
filter: impl Into<Option<Document>>,
options: impl Into<Option<ListDatabasesOptions>>,
) -> Result<Vec<Document>> {
) -> Result<Vec<DatabaseSpecification>> {
let op = ListDatabases::new(filter.into(), false, options.into());
self.execute_operation(op, None).await
self.execute_operation(op, None).await.and_then(|dbs| {
dbs.into_iter()
.map(|db_spec| bson::from_document(db_spec).map_err(crate::error::Error::from))
.collect()
})
}

/// Gets the names of the databases present in the cluster the Client is connected to.
Expand Down
5 changes: 3 additions & 2 deletions src/db/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use crate::{
DropDatabaseOptions,
ListCollectionsOptions,
},
results::CollectionSpecification,
selection_criteria::SelectionCriteria,
Client,
ClientSession,
Expand Down Expand Up @@ -194,7 +195,7 @@ impl Database {
&self,
filter: impl Into<Option<Document>>,
options: impl Into<Option<ListCollectionsOptions>>,
) -> Result<Cursor<Document>> {
) -> Result<Cursor<CollectionSpecification>> {
let list_collections = ListCollections::new(
self.name().to_string(),
filter.into(),
Expand All @@ -215,7 +216,7 @@ impl Database {
filter: impl Into<Option<Document>>,
options: impl Into<Option<ListCollectionsOptions>>,
session: &mut ClientSession,
) -> Result<SessionCursor<Document>> {
) -> Result<SessionCursor<CollectionSpecification>> {
let list_collections = ListCollections::new(
self.name().to_string(),
filter.into(),
Expand Down
12 changes: 6 additions & 6 deletions src/db/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub struct DatabaseOptions {
/// These are the valid options for creating a collection with
/// [`Database::create_collection`](../struct.Database.html#method.create_collection).
#[skip_serializing_none]
#[derive(Debug, Default, TypedBuilder, Serialize)]
#[derive(Clone, Debug, Default, TypedBuilder, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
#[builder(field_defaults(default, setter(strip_option)))]
#[non_exhaustive]
Expand All @@ -55,8 +55,7 @@ pub struct CreateCollectionOptions {
/// Specifies a validator to restrict the schema of documents which can exist in the
/// collection. Expressions can be specified using any query operators except `$near`,
/// `$nearSphere`, `$text`, and `$where`.
#[serde(rename = "validator")]
pub validation: Option<Document>,
pub validator: Option<Document>,

/// Specifies how strictly the database should apply the validation rules to existing documents
/// during an update.
Expand Down Expand Up @@ -86,7 +85,7 @@ pub struct CreateCollectionOptions {

/// Specifies how strictly the database should apply validation rules to existing documents during
/// an update.
#[derive(Debug, Serialize)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub enum ValidationLevel {
Expand All @@ -101,7 +100,7 @@ pub enum ValidationLevel {

/// Specifies whether the database should return an error or simply raise a warning if inserted
/// documents do not pass the validation.
#[derive(Debug, Serialize)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub enum ValidationAction {
Expand All @@ -112,8 +111,9 @@ pub enum ValidationAction {
}

/// Specifies default configuration for indexes created on a collection, including the _id index.
#[derive(Clone, Debug, PartialEq, Serialize)]
#[derive(Clone, Debug, TypedBuilder, PartialEq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub struct IndexOptionDefaults {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This struct was missing a few attributes so I added them. I don't think any of our users will use this though.

Copy link
Contributor

@kmahar kmahar Apr 28, 2021

Choose a reason for hiding this comment

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

TIL create accepts indexOptionDefaults

(I guess we do support it in Swift, so I knew this at some point, but we just take in a document so it was easier to forget about it than if it had a concrete type)

/// The `storageEngine` document should be in the following form:
///
Expand Down
2 changes: 1 addition & 1 deletion src/operation/create/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ async fn build_validator() {
coll: "test_coll".to_string(),
},
Some(CreateCollectionOptions {
validation: Some(query.clone()),
validator: Some(query.clone()),
..Default::default()
}),
);
Expand Down
1 change: 0 additions & 1 deletion src/operation/list_databases/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,4 @@ impl Operation for ListDatabases {
#[derive(Debug, Deserialize)]
struct ResponseBody {
databases: Vec<Document>,
total_size: Option<i64>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't being used and in fact wasn't even being deserialized, since the server returned this value as a double until 5.0.

}
79 changes: 77 additions & 2 deletions src/results.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

use std::collections::{HashMap, VecDeque};

use crate::bson::{Bson, Document};
use crate::{bson::{Bson, Document}, db::options::CreateCollectionOptions};

use serde::Serialize;
use bson::Binary;
use serde::{Deserialize, Serialize};

/// The result of a [`Collection::insert_one`](../struct.Collection.html#method.insert_one)
/// operation.
Expand Down Expand Up @@ -71,3 +72,77 @@ pub(crate) struct GetMoreResult {
pub(crate) batch: VecDeque<Document>,
pub(crate) exhausted: bool,
}

/// Describes the type of data store returned when executing
/// [`Database::list_collections`](../struct.Database.html#method.list_collections).
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub enum CollectionType {
/// Indicates that the data store is a view.
View,

/// Indicates that the data store is a collection.
Collection,
}

/// Info about the collection that is contained in the `CollectionSpecification::info` field of a specification returned from
/// [`Database::list_collections`](../struct.Database.html#method.list_collections).
///
/// See the MongoDB [manual](https://docs.mongodb.com/manual/reference/command/listCollections/#listCollections.cursor)
/// for more information.
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub struct CollectionSpecificationInfo {
/// Indicates whether or not the data store is read-only.
pub read_only: bool,

/// The collection's UUID - once established, this does not change and remains the same across replica
/// set members and shards in a sharded cluster. If the data store is a view, this field is `None`.
pub uuid: Option<Binary>,
}

/// Information about a collection as reported by [`Database::list_collections`](../struct.Database.html#method.list_collections).
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub struct CollectionSpecification {
/// The name of the collection.
pub name: String,

/// Type of the data store.
#[serde(rename="type")]
pub collection_type: CollectionType,

/// The options used to create the collection.
pub options: CreateCollectionOptions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about creating a separate type for this in case they ever diverge (and also because it contains fields like write_concern that don't make sense in this context), but the docs explicitly state that this field will contain the exact options provided to createCollection, and looking through the server code confirms also this, so I figured this was pretty low risk. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it seems low risk

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm


/// Additional info pertaining to the collection.
pub info: CollectionSpecificationInfo,

/// Provides information on the _id index for the collection
/// For views, this is `None`.
pub id_index: Option<Document>,
}

/// A struct modeling the information about an individual database returned from
/// [`Client::list_databases`](../struct.Client.html#method.list_databases).
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq)]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
pub struct DatabaseSpecification {
/// The name of the database.
pub name: String,

/// The amount of disk space in bytes that is consumed by the database.
#[serde(deserialize_with = "crate::bson_util::deserialize_u64_from_bson_number")]
pub size_on_disk: u64,

/// Whether the database has any data.
pub empty: bool,

/// For sharded clusters, this field includes a document which maps each shard to the size in bytes of the database
/// on disk on that shard. For non sharded environments, this field is `None`.
pub shards: Option<Document>,
}
3 changes: 2 additions & 1 deletion src/sync/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::{
SelectionCriteria,
SessionOptions,
},
results::DatabaseSpecification,
Client as AsyncClient,
ClientSession,
RUNTIME,
Expand Down Expand Up @@ -116,7 +117,7 @@ impl Client {
&self,
filter: impl Into<Option<Document>>,
options: impl Into<Option<ListDatabasesOptions>>,
) -> Result<Vec<Document>> {
) -> Result<Vec<DatabaseSpecification>> {
RUNTIME.block_on(
self.async_client
.list_databases(filter.into(), options.into()),
Expand Down
5 changes: 3 additions & 2 deletions src/sync/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::{
SelectionCriteria,
WriteConcern,
},
results::CollectionSpecification,
Database as AsyncDatabase,
RUNTIME,
};
Expand Down Expand Up @@ -140,7 +141,7 @@ impl Database {
&self,
filter: impl Into<Option<Document>>,
options: impl Into<Option<ListCollectionsOptions>>,
) -> Result<Cursor<Document>> {
) -> Result<Cursor<CollectionSpecification>> {
RUNTIME
.block_on(
self.async_database
Expand All @@ -157,7 +158,7 @@ impl Database {
filter: impl Into<Option<Document>>,
options: impl Into<Option<ListCollectionsOptions>>,
session: &mut ClientSession,
) -> Result<SessionCursor<Document>> {
) -> Result<SessionCursor<CollectionSpecification>> {
RUNTIME
.block_on(self.async_database.list_collections_with_session(
filter.into(),
Expand Down
15 changes: 5 additions & 10 deletions src/test/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,7 @@ async fn list_databases() {
let prev_dbs = client.list_databases(None, None).await.unwrap();

for name in expected_dbs {
assert!(!prev_dbs
.iter()
.any(|doc| doc.get("name") == Some(&Bson::String(name.to_string()))));
assert!(!prev_dbs.iter().any(|doc| doc.name.as_str() == name));

let db = client.database(name);

Expand All @@ -176,20 +174,17 @@ async fn list_databases() {
let new_dbs = client.list_databases(None, None).await.unwrap();
let new_dbs: Vec<_> = new_dbs
.into_iter()
.filter(|doc| match doc.get("name") {
Some(&Bson::String(ref name)) => expected_dbs.contains(name),
_ => false,
})
.filter(|db_spec| expected_dbs.contains(&db_spec.name))
.collect();
assert_eq!(new_dbs.len(), expected_dbs.len());

for name in expected_dbs {
let db_doc = new_dbs
.iter()
.find(|doc| doc.get("name") == Some(&Bson::String(name.to_string())))
.find(|db_spec| db_spec.name.as_str() == name)
.unwrap();
assert!(db_doc.contains_key("sizeOnDisk"));
assert!(db_doc.contains_key("empty"));
assert!(db_doc.size_on_disk > 0);
assert!(!db_doc.empty);
}
}

Expand Down
Loading