thedodd / wither

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

Handle a cursor bug related to getMore on non-existing coll. #34

Closed thedodd closed 4 years ago

thedodd commented 5 years ago

This commit adds two tests which are intended to isolate a bug related to a spurious DecodeError mentioned over in #33.

todo

Closes #33 Related to mongodb-labs/mongo-rust-driver-prototype#312

thedodd commented 5 years ago

So, I've been thinking quite a bit about this. Here are a few observations.

Previously, the deserialization of each of the documents in the iterator was handled immediately, by way of draining the cursor. Now, the user would have the burden of having to check for decode errors on each iterator element.

Perhaps a happy medium, and one which would compliment MongoDB's dynamic schema, would be to use a custom enum which represents either a successfully deserialized model instance, or a raw BSON document.

Yet another alternative, perhaps the best, would be to continue with the inner result type, but have the error variant be a 2-tuple of (DecodeError, Bson). All in all looking like: Result<Box<dyn Iterator<Result<Self, (DecodeError, Bson)>>>, MongoError>. An example of where a signature like this exists out in the wild is the futures::StreamFuture's associated ::Error type.

@brianwk just because you and I were discussing this over in #33, I figured I would ask you what you think about the above solutions.

brianwk commented 5 years ago

@thedodd I'm not exactly sure of the best way to do this, but how do you check for an error when the iterator returns a 2-tuple? What does it look like when you iterate a result set now and with the breaking API changes?

thedodd commented 5 years ago

@brianwk well, the tuple would still be wrapped in a result, so standard error handling practices would apply.

As far as the current API, it just returns a vector. So the caller can iterate the vector however they need to. The problem though is that we loose the benefits of having a cursor behind the scenes for lazy loading and such.

thedodd commented 5 years ago

@magiclen I'd be interested in getting your feedback on this PR and the discussion here when you have time.

To be honest ... I've actually been dragging my feet a bit on this, because the mongodb crate itself needs a lot of work, and I've been holding out hope that the MongoDB team would release a new futures based Rust driver. Thought I don't even know if @saghm or the MongoDB team is even planning on doing such a thing.

Either way, this changeset does seem to be needed. Using a cursor is fundamental. So any and all feedback on interface design would be appreciated.

thedodd commented 4 years ago

Superseded by #48.