-
Notifications
You must be signed in to change notification settings - Fork 180
RUST-1401 Additional GridFS API #763
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
pub async fn delete(&self, id: Bson) -> Result<()> { | ||
let delete_result = self | ||
.files() | ||
.delete_one(doc! { "_id": id.clone() }, None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to validate - delete
doesn't return an error if zero documents match, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, just verified in the shell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment but LGTM otherwise
src/gridfs/options.rs
Outdated
@@ -82,9 +82,28 @@ pub struct GridFsFindOptions { | |||
/// The maximum amount of time to allow the query to run. | |||
pub max_time: Option<Duration>, | |||
|
|||
/// The server normally times out idle cursors after an inactivity period to prevent excess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back when Sana did the API sketch we actually decided against supporting this option as it has no effect on MongoDB 4.4+ and we have next to no users on MongoDB < 5.0 - see Shane's comment here: mongodb/specifications#1289 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I had forgotten about that, I thought we had missed it in the first pass. Removed.
Implements the remaining API methods defined in the GridFS spec, including the advanced API.