thedodd / wither

An ODM for MongoDB built on the official MongoDB Rust driver.
https://docs.rs/wither
Other
325 stars 40 forks source link

Model::find returns DecoderError when collection is empty #33

Closed brianwk closed 4 years ago

brianwk commented 5 years ago

I currently have to do the following in order to find documents without raising an error:

let count = Submission::count(db.clone(), None, None).unwrap();
let submissions = if count > 0 { Submission::find(db.clone(), None, None).unwrap() } else { Vec::new() };
Json(json!({ "submissions": submissions }))

Shouldn't Submission::find() return an empty Vec on a empty collection and not raise a DecoderError? I am using MongoDB 4.0.3, should I try to downgrade to a later version?

thedodd commented 5 years ago

@brianwk I don't see anything in the Wither code which should be causing an error like that. It might be an issue with the underlying driver.

There are currently tests in place for the find operation, but none for when the collection is empty. I'll add some tests for that and see if the issue is present in older versions of mongo as well.

thedodd commented 5 years ago

@brianwk so, I've create a PR which adds two new tests to help isolate this issue: https://github.com/thedodd/wither/pull/34

Looks like an actual empty collection is just fine. No DecodeError. However, if the collection does not exist at all, it causes the DecodeError to come up with every version of mongo that we test against in this project (3.2, 3.4, 3.6 & 4.0).

The way that you get around this issue then is to ensure that your data models have been synced with your database. See the docs here on how to do so.

An example of how you might do this can be found in the project's README (as part of the PR I mentioned above). Sorry for not having an example of how to use Model::sync anywhere else, but based on these tests, syncing your model with the DB before using it should do the trick. Also, please note: syncing the model should only every have to be done per process lifecycle. Eg, when you API is booted, just call sync on your models before your API starts taking requests.

Let me know if any of that helps.

thedodd commented 5 years ago

@brianwk also, I did a little more digging to see why this issue is coming up, and it looks like it is coming from the call to cursor.drain_current_batch in the find method.

Here is the breakdown for the case where the collection doesn't exist at all:

I've looked through the underlying drivers code, and I have found exactly where the bug is. I'm opening an issue for it right now. I will push a change to circumvent this issue on the Wither side as well though.

However, I think the way to ultimately solve this the correct way is to not try to drain the cursor on the Wither side (I'm not sure why I ever did that in the first place). I think for the 1.0 release, I will have the various cursor-based methods return an iterator.

brianwk commented 5 years ago

@thedodd Thanks for being so responsive, I really appreciate it!

thedodd commented 5 years ago

@brianwk np! Ok, so just to summarize (and to hopefully unblock you right now):

That should be it. I will finish up the work on the PR so that we wont get a DecodeError from the bug in the MongoDB driver in the future.

brianwk commented 5 years ago

Sounds good. You are correct that the collection doesn't exist, but this actually shouldn't cause an error in MongoDB. The Rust driver for Mongo definitely needs more love...

thedodd commented 5 years ago

Agreed.

thedodd commented 5 years ago

Ok, popped the issue in the driver: https://github.com/mongodb-labs/mongo-rust-driver-prototype/issues/312

thedodd commented 4 years ago

@brianwk just a quick update here. I’m going to be starting work on cutting over to the new driver version soon. When I do so, as that will already constitute a breaking change, I’m going to update the various model methods to use the cursor behind the scenes so that we don’t run into this issue anymore.