CRUD Spec and Basic Insert/Update/Delete write commands #48
Conversation
Fill CRUD spec for collections; use BTreeMap Added drop collection Delete commands Complete write commands with tests Reorganize coll module and fix test warnings Documentation and cargo toml update
| }; | ||
|
|
||
| let req = try!(Message::with_query(self.get_req_id(), flags, self.namespace.to_owned(), | ||
| options.skip as i32, options.limit, doc, options.projection)); |
There was a problem hiding this comment.
Is there any reason why skip isn't being stored as an i32 in all of the options structs?
There was a problem hiding this comment.
skip can't be negative (as per @alabid), but the server still takes it as i32. u32 prevents it on the client-side. However, perhaps it should be stored as i32 to follow spec?
There was a problem hiding this comment.
What about in CountOptions, where it's 64-bits?
There was a problem hiding this comment.
Same deal with unsigned; 64-bit count options are specified in the spec.
| }; | ||
|
|
||
| let flags = OpQueryFlags { | ||
| tailable_cursor: options.cursor_type != CursorType::NonTailable, |
There was a problem hiding this comment.
We should really have a function to abstract away this logic and the logic on L105. Would be handy to have a constructor that abstracts away this != and == stuff
|
Couple small comments. Excellent work overall! @steveklabnik, comments on the general rustyness of this code would be much appreciated. This implements a lot of the basic functionality we want out of a mongodb driver - inserts, updates, etc. |
|
Sweet. Can you fix the merge conflict please @kyeah ? |
|
I will check out the 'general rustiness' soon :) |
| /// Extracts the collection name from the namespace. | ||
| pub fn name(&self) -> String { | ||
| match self.namespace.find(".") { | ||
| Some(idx) => self.namespace[idx+1..].to_owned(), |
There was a problem hiding this comment.
careful with this. This is a byte index of these strings, are you guaranteed that this is actually correct? Can namespaces have non-ascii characters?
There was a problem hiding this comment.
What would be the best way to grab a non-ascii substring? http://is.gd/PIX31l19 (via Rust forum)?
| assert_eq!(4, results.len()); | ||
|
|
||
| // Assert director attributes | ||
| for i in 0..3 { |
There was a problem hiding this comment.
Test assertions in for loops are very difficult to debug because the line number doesn't exactly correspond to the assertion error (doesn't tell you which value of loop variable the tests failed). Can you flatten this out? As a general rule of thumb, for and if should not be in test code.
There was a problem hiding this comment.
@kyeah can you address this in a separate PR? Don't want to block y'all anymore on this.
There was a problem hiding this comment.
Yep, working on it now and there's a bug in the update_many test, will open a new PR.
CRUD Spec and Basic Insert/Update/Delete write commands
This PR fills the CRUD spec for collections, except for Errors, and implements the basic insert, update, and delete write commands without ObjectID generation. The coll module is now separated into
coll,coll::options, andcoll::results.Other small updates (sorry):
db: Added thedrop_collection()function.connstring: Converted to usingBTreeMapoverHashMap. The standardHashMapimplementation uses a secure crypto hash algorithm that makes it perform significantly slower thanBTreeMap.Cargo.toml: Updated to use the insertion-order implementation of BSON documents, necessary for command ops.The travis environment has been configured as per @saghmrossi's fix in #47 , but it will fail until we get 3.0 testing up and running.