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

patrickfreed · 2021-04-27T23:54:34Z

This PR updates Client::list_databases and Database::list_collections to return structs that implement Deserialize rather than just Document.

patrickfreed

Tagging in @kmahar for the public API changes.

src/db/options.rs

patrickfreed · 2021-04-28T20:43:15Z

src/db/options.rs

 #[serde(rename_all = "camelCase")]
+#[non_exhaustive]
 pub struct IndexOptionDefaults {


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

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)

patrickfreed · 2021-04-28T20:43:44Z

src/operation/list_databases/mod.rs

@@ -85,5 +85,4 @@ impl Operation for ListDatabases {
 #[derive(Debug, Deserialize)]
 struct ResponseBody {
    databases: Vec<Document>,
-    total_size: Option<i64>,


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.

patrickfreed · 2021-04-28T20:48:44Z

src/results.rs

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


This value is returned as a bunch of different types depending on the server version, so we need a helper to deserialize it from whatever type to an integer. See here for an overview if interested.

any reason for using an i64 here rather than a u64?

I agree u64 does seem to make sense for a size field

I thought so too originally, but this is a value that's deserialized from server responses, meaning it can possibly have negative values. I checked elsewhere in the driver for precedent and saw that we use i64 for similar values, e.g. matched_count in UpdateResult. That being said, it seems like we probably would be totally fine using an unsigned value here if we wanted to, so I've updated it.

I also updated get_int to be a little more lenient when reading integers from doubles in case of a floating point rounding issue.

I filed RUST-764 to discuss updating other such fields.

patrickfreed · 2021-04-28T20:50:34Z

src/results.rs

+    pub collection_type: CollectionType,
+
+    /// The options used to create the collection.
+    pub options: CreateCollectionOptions,


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.

I agree it seems low risk

src/db/options.rs

kmahar · 2021-04-28T22:18:07Z

src/db/options.rs

 #[serde(rename_all = "camelCase")]
+#[non_exhaustive]
 pub struct IndexOptionDefaults {


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)

src/results.rs

kmahar · 2021-04-28T22:49:33Z

src/results.rs

+    pub collection_type: CollectionType,
+
+    /// The options used to create the collection.
+    pub options: CreateCollectionOptions,


I agree it seems low risk

kmahar

LGTM mod Isabel's comment, I don't need to re-review

kmahar · 2021-04-30T15:11:41Z

src/results.rs

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


I agree u64 does seem to make sense for a size field

patrickfreed added 4 commits April 27, 2021 18:22

return cursor of structs from list_collections

636e1ae

list_databases returns structs

af174da

fix rustdoc

4ff67dc

fix handling of views in listcollections

8959a04

patrickfreed changed the title ~~RUST-740~~ RUST-740 Return Deserialize structs from list_databases and list_collections Apr 28, 2021

patrickfreed added 2 commits April 28, 2021 16:45

revert unnecessary changes

8a036b9

flip comment and attributes order

48fa073

patrickfreed commented Apr 28, 2021

View reviewed changes

patrickfreed requested review from isabelatkinson and kmahar April 28, 2021 20:55

patrickfreed marked this pull request as ready for review April 28, 2021 20:55

kmahar reviewed Apr 28, 2021

View reviewed changes

remove forward-compat other case

7163d85

kmahar approved these changes Apr 30, 2021

View reviewed changes

i64 -> u64

8a5b787

isabelatkinson approved these changes Apr 30, 2021

View reviewed changes

patrickfreed merged commit 5aa29c2 into mongodb:master May 3, 2021

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

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

Uh oh!

Conversation

patrickfreed commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickfreed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmahar Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

kmahar Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmahar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickfreed commented Apr 27, 2021 •

edited

Loading

kmahar Apr 28, 2021 •

edited

Loading

kmahar Apr 28, 2021 •

edited

Loading