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

sanav33 · 2022-07-01T15:45:24Z

WIP: Adding GridFS support to the Rust Driver.

isabelatkinson

looking good! i'm also going to work on adding some design comments for the other reviewers once we tag them in

src/lib.rs

src/gridfs.rs

isabelatkinson

looking good! a few more style/design comments.

src/gridfs.rs

isabelatkinson · 2022-07-11T20:08:44Z

src/gridfs.rs

+    }
+
+    /// Finds and returns the files collection documents that match the filter.
+    pub fn find<T>(&self, filter: Document, options: GridFsBucketOptions) -> Result<Cursor<T>> {


Because we know the exact schema of the documents that will be returned by this find operation, it might be more useful to users to add the FilesCollectionDocument type to the public API and return a <Result<Cursor<FilesCollectionDocument>> here. While we do use generic types for most of our find-related methods, I'm not sure how useful that would actually be here given that there isn't any flexibility in what's returned. What do you think?

I was thinking it might make sense to get rid of the FilesCollectionDocument struct altogether? We could just return a cursor over bson::Documents that way. The only reason I can think of to keep the struct is for code completion and discovery purposes, since accessing the fields of the document can be achieved through the bson::Document API anyway. Thoughts?

The nice thing about using the FilesCollectionDocument struct rather than a plain Document is that the validation of the data returned is done ahead of time by the cursor, so a user wanting to inspect a field just needs to access it on the struct. With a Document, a user wanting to inspect the length field would need to do something like:

doc.get_i64("length").unwrap();

which requires knowing what the type of that field should be. Even though the unwrap should never fail it's nice to have the guarantee of the type system rather than needing to assume an invariant/write extra error handling.

That said, I'd be interested in what other members of the team think about using a defined struct here; we can leave this thread open for their input once I tag them in for review.

I like the suggestion to have a FilesCollectionDocument type, for the reasons Isabel mentioned; it's a well-defined format and when possible we generally try to model those types

isabelatkinson

looking good! just one more comment

isabelatkinson · 2022-07-27T20:56:38Z

src/db/mod.rs

+    ) -> GridFsBucket {
+        GridFsBucket {
+            db: self.clone(),
+            options: options.into(),


I think it would be useful to store the options that get inherited from the parent database directly in the options structs here. We currently do this in some of our other structs, e.g. Collection::new(). That way we won't get possibly conflicting values when calling, for example, the read_concern() method vs. inspecting the read_concern field directly on the options.

I think it would be a good idea to include to store the fields specified in the GridFsBucketOptions struct directly within GridFsBucket, especially since chunk_size_bytes and bucket_name are used frequently in the upload and download methods and it makes the code harder to read to figure out those values from all the layers of Option around those fields.

src/gridfs.rs

patrickfreed · 2022-07-28T23:58:57Z

src/gridfs.rs

+
+    /// Downloads the contents of the stored file specified by `id` and writes
+    /// the contents to the destination [`GridFsDownloadStream`]. Uses the `futures` crate.
+    pub async fn download_to_stream_futures(


As ugly as it is, I think we may need to specify futures_0_3 here, since if an 0.4 or 1.0 version of that crate is released, we'll still need to maintain this.

src/gridfs.rs

kmahar

looks good! just various nits around the types used

src/db/mod.rs

src/gridfs/options.rs

src/gridfs.rs

src/db/mod.rs

src/gridfs.rs

src/gridfs/options.rs

src/gridfs.rs

abr-egn · 2022-08-04T17:28:38Z

Cargo.toml

@@ -59,6 +59,7 @@ bson = { git = "https://github.com/mongodb/bson-rust", branch = "main" }
 chrono = "0.4.7"
 derivative = "2.1.1"
 flate2 = { version = "1.0", optional = true }
+futures = "0.3.14"


I think we don't need to explicitly pull in futures-core, -util, or -executor if we're using futures.

We could do away with those but that would require changing every import of futures-core, -util and -executor in the driver. Is that something we want to do?

The various individual futures-x crates have differing stability guarantees, which is why we depend on them directly. See this comment for more context / reasoning.

Given that, I think we'll want to continue to depend on the various subcrates directly.

Ah I see, that makes sense. Fixed!

patrickfreed

LGTM!

sanav33 requested a review from isabelatkinson July 1, 2022 15:45

sanav33 changed the title ~~push public api skeleton~~ RUST-527 Add GridFS support Jul 1, 2022

sanav33 marked this pull request as draft July 1, 2022 15:45

isabelatkinson reviewed Jul 6, 2022

View reviewed changes

sanav33 changed the title ~~RUST-527 Add GridFS support~~ RUST-1392 Add GridFS support: Implement public API with placeholder code Jul 7, 2022

sanav33 requested a review from isabelatkinson July 7, 2022 14:54

isabelatkinson reviewed Jul 11, 2022

View reviewed changes

sanav33 requested a review from isabelatkinson July 13, 2022 12:32

isabelatkinson requested review from kmahar, patrickfreed and abr-egn July 13, 2022 18:51

sanav33 marked this pull request as ready for review July 26, 2022 18:35

isabelatkinson reviewed Jul 27, 2022

View reviewed changes

patrickfreed reviewed Jul 29, 2022

View reviewed changes

kmahar suggested changes Aug 1, 2022

View reviewed changes

sanav33 requested a review from kmahar August 1, 2022 20:57

patrickfreed reviewed Aug 3, 2022

View reviewed changes

src/db/mod.rs Outdated Show resolved Hide resolved

src/db/mod.rs Outdated Show resolved Hide resolved

src/gridfs.rs Outdated Show resolved Hide resolved

src/gridfs.rs Show resolved Hide resolved

src/gridfs.rs Show resolved Hide resolved

kmahar reviewed Aug 3, 2022

View reviewed changes

src/gridfs/options.rs Outdated Show resolved Hide resolved

src/gridfs.rs Outdated Show resolved Hide resolved

sanav33 requested review from patrickfreed, kmahar and isabelatkinson August 4, 2022 13:39

abr-egn reviewed Aug 4, 2022

View reviewed changes

kmahar approved these changes Aug 8, 2022

View reviewed changes

patrickfreed approved these changes Aug 8, 2022

View reviewed changes

abr-egn approved these changes Aug 8, 2022

View reviewed changes

sanav33 added 5 commits August 8, 2022 14:01

push public api skeleton

756869f

add public api documentation

b245a67

convert generic tfileid to bson

796c1c0

run fmt

d915614

split GridFsStream type

8cbfffe

sanav33 added 17 commits August 8, 2022 14:01

add finish

e5f6e99

make gridfs stream methods return result

49cdb7d

add abort method for upload stream

6a8fddb

eliminate options in gridfsbucket struct

2849c7c

change method names in gridfs api

72b39b3

make type changes

6fa8267

run fmt

e7ae7a7

remove noCursorTimeout option

1487008

fixed docs for options

8f8f0fc

change stream id field name to remvoe ambiguity

b34e2b5

update download stream id field name

92b2a53

store options directly in bucket

86ac29d

change helpers for bucket

81672d4

add futures version to methods

9c94237

remove futures-util import from gridfs

4aa6cd9

remove futures dependency

fac86a0

fix destination trait bound

01238f5

sanav33 force-pushed the RUST-527/add-gridfs-support branch from caefc84 to 01238f5 Compare August 8, 2022 18:02

remove link from doc for bucket constructor

8325459

isabelatkinson approved these changes Aug 8, 2022

View reviewed changes

sanav33 merged commit 3dcf919 into mongodb:main Aug 9, 2022

sanav33 deleted the RUST-527/add-gridfs-support branch August 9, 2022 02:50

kmahar mentioned this pull request Sep 2, 2022

RUST-527 - Implementation of gridfs #235

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sanav33 commented Jul 1, 2022

isabelatkinson left a comment

isabelatkinson left a comment

isabelatkinson Jul 11, 2022

sanav33 Jul 12, 2022

isabelatkinson Jul 12, 2022

kmahar Aug 3, 2022

isabelatkinson left a comment

isabelatkinson Jul 27, 2022

sanav33 Jul 28, 2022

patrickfreed Jul 28, 2022

kmahar left a comment

abr-egn Aug 4, 2022

sanav33 Aug 4, 2022

patrickfreed Aug 4, 2022

sanav33 Aug 5, 2022

patrickfreed left a comment

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

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

Conversation

sanav33 commented Jul 1, 2022

isabelatkinson left a comment

Choose a reason for hiding this comment

isabelatkinson 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

isabelatkinson 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

kmahar 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

patrickfreed left a comment

Choose a reason for hiding this comment