RUST-1400 GridFS download methods #747

isabelatkinson · 2022-09-16T18:48:45Z

Implements the download_to_stream method described here and the download_to_stream_by_name method described here

isabelatkinson · 2022-09-16T20:45:11Z

src/test/spec/unified_runner/test_runner.rs

+            // to run a single test, set the TEST_DESCRIPTION environment variable to the test's
+            // description
+            if let Ok(description) = std::env::var("TEST_DESCRIPTION") {
+                if description != test_case.description {
+                    continue;
+                }
+            }


I think we all probably write something to this effect when trying to debug an individual test, so I thought it would be useful to have this in the code permanently

Could we make this contains instead of strict equality? I normally add something like that in case I want to run a few tests at once. Great idea to add this btw; I think it'll save a ton of time for all of us going forward.

isabelatkinson · 2022-09-16T20:47:21Z

src/gridfs.rs

-    files_id: Bson,
-    n: u32,
+#[derive(Debug, Deserialize, Serialize)]
+pub struct Chunk {


going to disable the lint suppression for missing docs in places like these and add them in when I do RUST-1395

isabelatkinson · 2022-09-26T19:08:40Z

converting to draft as I pull in some improvements from #751, feel free to hold off on reviewing for now

isabelatkinson · 2022-09-26T21:17:01Z

ready for review again

patrickfreed

Sorry for the delay on the review!

src/gridfs.rs

patrickfreed · 2022-09-28T12:07:11Z

src/gridfs.rs

    id: ObjectId,
    files_id: Bson,
    n: u32,
    // default size is 255 KiB
-    data: Vec<u8>,
+    data: Binary,
 }

 /// A collection in which information about stored files is stored. There will be one files


I know this is existing code, but I think this docstring could be a tad clearer. For instance, this struct models the documents in the files collection, rather than being the collection itself. Also, it could be better to say something like "metadata" rather than information, and also could be useful to mention the collection in which the files are actually stored (the "chunks" collection, right?).

Ah I left a comment about doing a later pass over documentation but it became outdated when I made some subsequent changes: #747 (comment)

I think most of the documentation needs some cleaning up so I figured I'd do it in one swoop at the end (although I made a few update in this PR) -- does that sound good re the rest of your documentation comments or would you rather have them addressed here?

Follow up PR sounds good to me! Sorry, yeah I missed your previous comment.

patrickfreed · 2022-09-28T12:08:23Z

src/gridfs.rs

+    pub upload_date: DateTime,
+    pub filename: Option<String>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub metadata: Option<Document>,


Would be good to document each of these fields, similar to what we do for ChangeStreamEvent.

src/gridfs/download.rs

patrickfreed · 2022-09-28T12:14:32Z

src/gridfs/download.rs

+    options::{FindOneOptions, FindOptions},
+};
+
+impl GridFsBucket {


Could we provide doctest examples for each of these? (sorry i know it will be a lot of similar examples, but I think it's something users really appreciate)

src/gridfs/download.rs

patrickfreed · 2022-09-28T12:49:14Z

src/test/spec/unified_runner/test_runner.rs

+            // to run a single test, set the TEST_DESCRIPTION environment variable to the test's
+            // description
+            if let Ok(description) = std::env::var("TEST_DESCRIPTION") {
+                if description != test_case.description {
+                    continue;
+                }
+            }


Could we make this contains instead of strict equality? I normally add something like that in case I want to run a few tests at once. Great idea to add this btw; I think it'll save a ton of time for all of us going forward.

src/gridfs/download.rs

patrickfreed · 2022-09-28T12:56:11Z

src/gridfs/download.rs

+    }
+
+    /// Downloads the contents of the stored file specified by `id` and writes
+    /// the contents to the `destination`.


The method name implies this of course, but can we explicitly mention that the destination here implements the AsyncWrite trait from the futures 0.3 crate, and link to it? Ditto for the tokio ones. Might also be worth mentioning that once std stabilizes AsyncWrite, this method will be deprecated (ditto for tokio ones on this too).

Since the to_futures_0_3 methods are just using compat_write under the hood and are temporary, what would people think about leaving them out entirely and recommending users do this directly if needed in documentation?

@abr-egn I'd be fine with that

It's a little inconsistent with our various cursor APIs that implement the futures version of Stream, but having seen what the implementation actually entails here, I think I agree that maybe these separate methods probably aren't worth it. Instead, if we want to have consistency, maybe we should just also implement tokio's Stream on our cursor types. Then omitting the futures one here isn't as inconsistent, since we do have at least one crate's traits implemented across the board.

Would it make more sense to have only the futures implementations then? That way we're runtime-agnostic and in line with what we already support for Cursor. The tokio compat utils go both ways, so I think the user conversion code would be basically the same regardless of which library we choose to support in our API.

isabelatkinson · 2022-09-28T14:02:21Z

src/gridfs.rs

    id: ObjectId,
    files_id: Bson,
    n: u32,
    // default size is 255 KiB
-    data: Vec<u8>,
+    data: Binary,
 }

 /// A collection in which information about stored files is stored. There will be one files


Ah I left a comment about doing a later pass over documentation but it became outdated when I made some subsequent changes: #747 (comment)

I think most of the documentation needs some cleaning up so I figured I'd do it in one swoop at the end (although I made a few update in this PR) -- does that sound good re the rest of your documentation comments or would you rather have them addressed here?

src/gridfs/download.rs

isabelatkinson · 2022-09-29T14:09:27Z

src/error.rs

+pub enum GridFsErrorKind {
+    /// The file with the given identifier was not found.
+    #[non_exhaustive]
+    FileNotFound { identifier: GridFsFileIdentifier },


This is a little convoluted but the FileNotFound error can be returned from either download (which has an id) or download_by_name (which has a filename), and it didn't seem much better to have separate error cases for those.

src/gridfs/download.rs

src/error.rs

patrickfreed · 2022-09-30T11:30:41Z

src/gridfs.rs

    id: ObjectId,
    files_id: Bson,
    n: u32,
    // default size is 255 KiB
-    data: Vec<u8>,
+    data: Binary,
 }

 /// A collection in which information about stored files is stored. There will be one files


Follow up PR sounds good to me! Sorry, yeah I missed your previous comment.

src/gridfs/download.rs

src/gridfs.rs

patrickfreed

LGTM! tagging in the other reviews.

Going to leave my docs related commends unresolved so you can go back to them when doing the follow on PR.

abr-egn

LGTM!

abr-egn · 2022-10-05T18:05:46Z

src/gridfs/download.rs

+    }
+
+    /// Downloads the contents of the stored file specified by `id` and writes
+    /// the contents to the `destination`.


Since the to_futures_0_3 methods are just using compat_write under the hood and are temporary, what would people think about leaving them out entirely and recommending users do this directly if needed in documentation?

kmahar

a few minor questions/comments but LGTM overall!

src/gridfs.rs

src/test/spec/unified_runner/operation.rs

src/error.rs

kmahar · 2022-10-05T20:43:44Z

src/gridfs/download.rs

+    }
+
+    /// Downloads the contents of the stored file specified by `id` and writes
+    /// the contents to the `destination`.


@abr-egn I'd be fine with that

isabelatkinson · 2022-10-11T19:45:14Z

Updated to remove the tokio methods. I initially wrote a test to verify that this works but removed it in favor of adding a doc example when I refresh all of the documentation which will give the same coverage.

isabelatkinson commented Sep 16, 2022

View reviewed changes

isabelatkinson marked this pull request as ready for review September 16, 2022 21:21

isabelatkinson requested review from a team and patrickfreed and removed request for a team September 16, 2022 21:21

isabelatkinson marked this pull request as draft September 26, 2022 19:08

isabelatkinson force-pushed the gridfs-download branch 2 times, most recently from 210d6c8 to c4f4acf Compare September 26, 2022 20:54

isabelatkinson marked this pull request as ready for review September 26, 2022 21:16

patrickfreed reviewed Sep 28, 2022

View reviewed changes

isabelatkinson commented Sep 29, 2022

View reviewed changes

patrickfreed reviewed Sep 30, 2022

View reviewed changes

patrickfreed approved these changes Oct 4, 2022

View reviewed changes

patrickfreed requested review from kmahar and abr-egn October 4, 2022 15:09

abr-egn approved these changes Oct 5, 2022

View reviewed changes

kmahar reviewed Oct 5, 2022

View reviewed changes

isabelatkinson added 2 commits October 11, 2022 12:54

RUST-1400 GridFS download methods

2ac6dfb

remove tokio methods

d5129df

isabelatkinson force-pushed the gridfs-download branch from ca56821 to d5129df Compare October 11, 2022 19:40

patrickfreed approved these changes Oct 11, 2022

View reviewed changes

kmahar approved these changes Oct 11, 2022

View reviewed changes

fix fmt

01c2e75

isabelatkinson merged commit c9c4e3f into mongodb:main Oct 12, 2022

isabelatkinson deleted the gridfs-download branch March 6, 2023 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-1400 GridFS download methods #747

RUST-1400 GridFS download methods #747

isabelatkinson commented Sep 16, 2022 •

edited

Loading

isabelatkinson Sep 16, 2022

patrickfreed Sep 28, 2022

kmahar Oct 5, 2022

isabelatkinson Sep 16, 2022

isabelatkinson commented Sep 26, 2022

isabelatkinson commented Sep 26, 2022

patrickfreed left a comment

patrickfreed Sep 28, 2022

isabelatkinson Sep 28, 2022

patrickfreed Sep 30, 2022

patrickfreed Sep 28, 2022

patrickfreed Sep 28, 2022

patrickfreed Sep 28, 2022

patrickfreed Sep 28, 2022

abr-egn Oct 5, 2022

kmahar Oct 5, 2022

patrickfreed Oct 5, 2022 •

edited

Loading

isabelatkinson Oct 6, 2022

isabelatkinson Sep 28, 2022

isabelatkinson Sep 29, 2022

patrickfreed Sep 30, 2022

patrickfreed left a comment

abr-egn left a comment

abr-egn Oct 5, 2022

kmahar left a comment •

edited

Loading

kmahar Oct 5, 2022

isabelatkinson commented Oct 11, 2022

RUST-1400 GridFS download methods #747

RUST-1400 GridFS download methods #747

Conversation

isabelatkinson commented Sep 16, 2022 • edited Loading

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

isabelatkinson commented Sep 26, 2022

isabelatkinson commented Sep 26, 2022

patrickfreed left a 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

patrickfreed Oct 5, 2022 • edited Loading

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

patrickfreed left a comment

Choose a reason for hiding this comment

abr-egn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmahar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelatkinson commented Oct 11, 2022

isabelatkinson commented Sep 16, 2022 •

edited

Loading

patrickfreed Oct 5, 2022 •

edited

Loading

kmahar left a comment •

edited

Loading