RUST-1478 GridFS upload methods #751
Conversation
isabelatkinson
force-pushed
the
gridfs-upload
branch
from
92d586e to
1fb4d2c
Compare
| }; | ||
|
|
||
| impl GridFsBucket { | ||
| /// Uploads a user file to a GridFS bucket. Bytes are read from `source` and stored in chunks in |
There was a problem hiding this comment.
Same as last PR, docs will be updated/examples will be added when I mark everything public.
isabelatkinson
changed the title
isabelatkinson
requested review from
a team and
kmahar
and removed request for
a team
src/gridfs/upload.rs
Outdated
| let mut n = 0; | ||
|
|
||
| loop { | ||
| let mut buf = vec![0u8; chunk_size as usize]; |
There was a problem hiding this comment.
Micro-optimization nit: since chunk_size is static during iteration, it might be more efficient to construct buf outside the loop.
There was a problem hiding this comment.
As part of this, rather than truncating each time, we'll probably need to just slice the buffer.
| async move { | ||
| let bucket = test_runner.get_bucket(id).await; | ||
| let hex_bytes = self.source.get("$$hexBytes").unwrap().as_str().unwrap(); | ||
| // Iterates over two characters of the hex string at a time, parses each pair into a |
There was a problem hiding this comment.
Could this use hex::decode?
There was a problem hiding this comment.
yep, this must've been implemented before i added that dependency. updated.
src/gridfs/upload.rs
Outdated
| let mut n = 0; | ||
|
|
||
| loop { | ||
| let mut buf = vec![0u8; chunk_size as usize]; |
There was a problem hiding this comment.
As part of this, rather than truncating each time, we'll probably need to just slice the buffer.
src/gridfs/upload.rs
Outdated
| // Indexes should be considered equivalent regardless of numeric value type. | ||
| // e.g. { "filename": 1, "uploadDate": 1 } is equivalent to | ||
| // { "filename": 1.0, "uploadDate": 1.0 } | ||
| let number_matches = |key: &str| index_model.keys.get(key).and_then(get_int) == Some(1); |
There was a problem hiding this comment.
I know the indexes we create all currently use 1 for the value, but I think it would be good to avoid future pitfalls by checking against get_int of the input document too instead of Some(1).
There was a problem hiding this comment.
My only hesitation with doing this when originally implementing was needing to account for extraneous BSON types that aren't really relevant here, something like this:
let number_matches = |key: &str, value: &Bson| {
if let model_value = index_model.keys.get(key) {
match get_int(value) {
Some(num) => get_int(model_value) == Some(num),
// this case is unreachable in practice
None => model_value == value,
}
} else {
false
}
}
The reason I felt okay with hard-coding Some(1) here was because that value is significant in that it represents an increasing index, which is the only kind that has relevance for the various GridFS operations. Given that, and the fact that GridFS is a fairly static spec, it seemed like a fine tradeoff for the sake of readability.
This is all fairly pedantic, however, so if you think we should make the change I'm happy to do so.
There was a problem hiding this comment.
I think your proposed code looks great actually 🙂
That said, I agree that this is a really minor detail and I'm happy to defer to your judgment on whether this change is worth making.
src/gridfs/upload.rs
Outdated
| // Indexes should be considered equivalent regardless of numeric value type. | ||
| // e.g. { "filename": 1, "uploadDate": 1 } is equivalent to | ||
| // { "filename": 1.0, "uploadDate": 1.0 } | ||
| let number_matches = |key: &str| index_model.keys.get(key).and_then(get_int) == Some(1); |
There was a problem hiding this comment.
I think your proposed code looks great actually 🙂
That said, I agree that this is a really minor detail and I'm happy to defer to your judgment on whether this change is worth making.
isabelatkinson
force-pushed
the
gridfs-upload
branch
from
f230dbd to
cc03d0f
Compare
Implements the
upload_from_streamandupload_from_stream_with_idmethods described in this section of the spec: https://github.com/mongodb/specifications/blob/master/source/gridfs/gridfs-spec.rst#file-upload