Skip to content
This repository was archived by the owner on Oct 18, 2021. It is now read-only.

remove specifications of default options #185

Merged
merged 1 commit into from
Feb 12, 2017

Conversation

saghm
Copy link
Contributor

@saghm saghm commented Feb 3, 2017

Replaces #183 and moves us a bit closer towards #179

This is a big one; some of the changes of the options structs were relatively easy to integrate, but others (I'm looking you, FindOptions) required quite a bit of finagling with parts of the code I hadn't looked closely at in a while.

All tests pass on 3.2, and the only two failing tests on 3.4 are apm::logging (I'll need to look into this more closely) and client::db::create_and_get_users (which seems to return the users in a different order than before), both of which appear to be issues with the tests not being compatible with 3.4 rather than the driver itself. This means the driver should work with 3.4 after this is merged!

@@ -10,33 +10,54 @@ pub struct DeleteModel {
pub multi: bool,
}

impl DeleteModel {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that a lot of our code grouped the definitions of the types together at the top and the implementations below. This made it a bit annoying to work with (i.e. needing either two buffers or to scroll back and forth to see struct signatures at the same time as modifying the implementations), so I'm thinking that going forward we should try to group implementations with definitions.


let pipeline_map = pipeline.iter()
.map(|bdoc| Bson::Document(bdoc.to_owned()))
let pipeline_map: Vec<_> = pipeline.into_iter()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we were copying all of the document instead of just moving them, which seems more idiomatic since they aren't being used afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure there are many, many places were we make unnecessary copies. I look forward to destroying all of them in the future.

match result.get("n") {
Some(&Bson::I32(ref n)) => Ok(*n as i64),
Some(&Bson::I64(ref n)) => Ok(*n),
Some(&Bson::I32(n)) => Ok(n as i64),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numeric types (such as i32 and i64) implement Copy, so there isn't any need to capture them by reference.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

if filter.is_some() {
spec.insert("query", Bson::Document(filter.unwrap()));

if let Some(filter_doc) = filter {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although using is_some() and unwrap() is technically safe, it's more idiomatic to use if let Some(...), so I changed to this style here and a few other places.

filter: bson::Document,
options: bson::Document,
_max_time_ms: Option<i64>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed we don't seem to actually use this option anywhere in the driver. I think it might be a good idea to look at how other drivers use it, since it probably is included in the spec for something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

@@ -57,7 +57,8 @@ fn logging() {
let _ = fs::remove_file("test_apm_log.txt");

// Reset State
let reset_client = Client::connect("localhost", 27017).expect("Failed to connect to localhost:27017");
let reset_client = Client::connect("localhost", 27017)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most recent large rustfmt/clippy PR only checked src (and not tests), so rustfmt picked up a few things.

@@ -91,8 +92,7 @@ fn logging() {

// Create collection started
read_first_non_monitor_line(&mut file, &mut line);
assert_eq!("COMMAND.create_collection 127.0.0.1:27017 STARTED: { create: \"logging\", \
capped: false, auto_index_id: true, flags: 1 }\n",
assert_eq!("COMMAND.create_collection 127.0.0.1:27017 STARTED: { create: \"logging\" }\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here (and in several places below) the logging output changed due to the default fields no longer being present.

@@ -170,7 +170,7 @@ macro_rules! run_insert_one_test {
macro_rules! run_replace_one_test {
( $db:expr, $coll:expr, $filter:expr, $replacement:expr, $upsert:expr,
$outcome:expr ) => {{
let options = ReplaceOptions::new($upsert, None);
let options = ReplaceOptions { upsert: $upsert, write_concern: None };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into a weird thing here when I tried to use the ..Default::default() syntax; the compiler seemed to be treating the .. as part of a range expression. Strangely, it only seemed to occur in certain places in macro definitions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macros: still sketch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I probably only implemented all this in macros at the time because I had finally gotten to the point where I understood them and therefore wanted to celebrate by using them everywhere.

Let's open an issue to convert the test frameworks from macros to legitimate Rust code; it'll be a bit of work, but it'll probably save a lot of hassle in the long run.

Some(Bson::Document(doc)) => Some(doc),
_ => None,
};
if let Some(Bson::Document(projection)) = object.get("projection").map(Bson::from_json) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest; I have absolutely no idea why I implemented this (and the similar cases below) in such a convoluted way originally. I'll just have to chalk it up to my lack of familiarity with Rust at that point.

pub struct CountOptions {
pub skip: u64,
pub limit: i64,
pub skip: Option<i64>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure why an unsigned integer was used here (and in another place below). The spec doesn't use unsigned integers (probably due to there not being an easy why to enforce it in dynamically typed languages)

@kyeah kyeah self-requested a review February 4, 2017 22:42
Copy link
Contributor

@kyeah kyeah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the longest PR 🥇

overall, looks like a good movement towards a cleaner codebase, and one step removed from making use of derive_builder and Into<Option>. I think there are areas where we can do even less cloning (looking at you, read/write preferences), but we can look at that separately.

@@ -10,33 +10,54 @@ pub struct DeleteModel {
pub multi: bool,
}

impl DeleteModel {
pub fn new(filter: Document, multi: bool) -> DeleteModel {
Copy link
Contributor

@kyeah kyeah Feb 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use #[derive(new)] now :)

https://github.com/nrc/derive-new

EDIT: Document might need to derive this, actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it's worth adding that in the bson crate.

src/cursor.rs Outdated
@@ -27,7 +27,8 @@ use {Client, CommandType, Error, ErrorCode, Result, ThreadedClient};
use apm::{CommandStarted, CommandResult, EventRunner};

use bson::{self, Bson};
use common::{ReadMode, ReadPreference};
use common::{ReadMode, merge_options, ReadPreference};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiniest nit: let's move the function import to the end (unless that's from rustfmt, in which case... 🙄)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I actually meant to put it alphabetically, which would be at the beginning. Is that okay too?

query.clone(),
return_field_selector);
namespace.clone(),
options.skip.unwrap_or(0) as i32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna guess it's pre-emptive specification — if they change the wire-protocol to accept i64, driver users won't have to change their code, as the API won't change. Although I recall i32s in other parts of the spec, so... 🙃


let pipeline_map = pipeline.iter()
.map(|bdoc| Bson::Document(bdoc.to_owned()))
let pipeline_map: Vec<_> = pipeline.into_iter()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure there are many, many places were we make unnecessary copies. I look forward to destroying all of them in the future.

match result.get("n") {
Some(&Bson::I32(ref n)) => Ok(*n as i64),
Some(&Bson::I64(ref n)) => Ok(*n),
Some(&Bson::I32(n)) => Ok(n as i64),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

update.insert("q", Bson::Document(model.filter));
update.insert("u", Bson::Document(model.update));
update.insert("upsert", Bson::Boolean(model.upsert));
let mut update = bson::Document::from(model);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

"update" => (self.name()),
"updates" => updates,
"writeConcern" => (wc.to_bson())
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@@ -95,3 +95,8 @@ impl WriteConcern {
bson
}
}

pub fn merge_options<T: Into<bson::Document>>(document: bson::Document, options: T) -> bson::Document {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this make more sense as a function on bson::Document? Feels reaal funny to have this global function, and languages like ruby have this type of method on the hash object itself. doc.merge(other_doc)


let mut flags = 0;

if let Some(true) = options.use_power_of_two_sizes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice! yeah, it only has two flags, so probably not worth it.

@@ -170,7 +170,7 @@ macro_rules! run_insert_one_test {
macro_rules! run_replace_one_test {
( $db:expr, $coll:expr, $filter:expr, $replacement:expr, $upsert:expr,
$outcome:expr ) => {{
let options = ReplaceOptions::new($upsert, None);
let options = ReplaceOptions { upsert: $upsert, write_concern: None };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macros: still sketch.

@saghm saghm force-pushed the remove-default-options branch from 433a2c2 to 5991a24 Compare February 11, 2017 20:12
Copy link
Contributor

@kyeah kyeah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool; let's revisit bson crate items in a future PR.

@saghm
Copy link
Contributor Author

saghm commented Feb 12, 2017

Sounds good!

@saghm saghm merged commit cd7fe74 into mongodb-labs:master Feb 12, 2017
@saghm saghm deleted the remove-default-options branch April 14, 2017 05:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants