The autoIndexId option removed in version 3.4

[Gedweb]

Deprecated since version 3.2 (released at Dec 8, 2015)
The autoIndexId option will be removed in version 3.4.

[saghm]

Hi, and thanks for the pull request! Unfortunately we can't just completely get rid of the option, since we plan on still supporting 3.0 and 3.2 for the time being, so the change for this will likely need to be a bit more involved.

@kyeah, have you taken a look at this yet? This is a bit tangential to this PR, but I'm starting to think that our strategy of specifically providing default options (in all commands, not just this one) instead of just only adding the options specified by the user isn't the best way to do things, especially having looked at what other drivers do (for example, what the C++ driver does for this case). Maybe we should go through everywhere that we use something like let some_options = provided_options.unwrap_or_else(SomeOptions::new) and replace that with a manually-built document containing only what's necessary. We wouldn't be able to use the doc! {} macro, unfortunately, but perhaps we could implement From<..> for Document for each of our option types?

[Gedweb]

If you planned support old version, may be add some version feature? And use it for conditional compilation. It simple to use in Cargo.toml and don't break back compatibility.

[saghm]

I don't think a feature flag is the right choice for compatibility of MongoDB versions. For one thing, this raises the barrier of entry for new users, as adding mongodb to the Cargo.toml will no longer "just work" in some cases. It also increases the complexity of testing the code, as new features add an additional dimension to test against. Similarly, as new versions of MongoDB come out every year or so, we'd have to continue adding new features for each MongoDB version, which will complicate things rather quickly. Finally, adding a new feature means that the driver will not be able to remain connected to a cluster that's being upgraded "in-place" (where one node at a time is brought down and then a newer-version node is brought up to replace it).

Given that I'm already strongly inclined to stop providing default parameters in all places that we're currently doing it (including here, which would fix this issue), I think this is the best route forward. Unless @kyeah has any objections, I'll try to make a PR with these changes this weekend, and hopefully we'll be able to get it merged soon after that.

In the meantime, if you absolutely can't wait another week to use the driver with 3.4, feel free to use your fork in the meantime as a temporary solution. Given that you're aware of how to use features with Cargo, I'm guessing you already know how to specify a git dependency in your Cargo.toml, but if not, it's super easy!

[Gedweb]

Yep, I already use Cargo replace option for override current version library to my fork. Awaiting your changes for compatibility with MongoDB v3.4.1

[kyeah]

@saghm cool! I like only adding options that the user explicitly passes in. It seems like we're already manually inserting options into documents for a lot of methods, so it's not a big loss to lose doc!{} in my opinion.

I have some vague macro ideas floating around to make option definition and bson coercion easier to digest, but I'll play around with those ideas in the future.

Since this is going to make everything in SomeOptions an Option, things will get a little weirder. Thankfully, Option<X> implements From<X> since Rust 1.12; it'll add into everywhere, but will make the user experience essentially the same.

struct SomeOptions {
  allow_something: Into<Option<bool>>,
}

let opts = SomeOptions::new();
opts.allow_something = true;

In the future, I think a tweak to derive_builder that creates Into<> parameterized setters may be nice to have?

struct SomeOptions {
  allow_something: Option<bool>,
}

let opts = SomeOptions::new()
  .allow_something(true)
  .clone();

Just flowing thoughts out there. I think there are a lot of opportunities to modernize the driver with recent language and compiler changes, and how we handle our Options is a really nice place for us to play around and see what sticks.

[saghm]

Using Into is a good call; I'm glad you mentioned that before I started changing too much, as I was planning on just making everything plain Option<T>.

Is there any reason not to use derive_builder now? From looking at the repo/docs, it looks like it works on stable, and I think it would be pretty useful.

[kyeah]

I think it'd be beneficial! My only question is whether we can go straight to allow_something: Option<bool> or if we still need the Into — and if we need the Into, would it be worth it in the future to fork derive_builder?

One thing to note is that there's an RFC proposal for default struct values, e.g.

struct SomeOptions {
  allow_a: bool,
  allow_b: bool,
}

let opts = SomeOptions { allow_a: true };

which I think would be cleaner; since this is still in a proposal stage, though, the builder seems like a good option (no pun intended).

[saghm]

From trying out an example, it looks like derive_builder actually isn't able to be used on stable due to it needing to derive a custom trait. Luckily, Rust 1.15 will stabilize this feature, despite the compiler implying otherwise when I compiled the example. I think for now we should go ahead with the change without either Into or custom_derive and then we can hopefully implement custom derive after the new version of Rust is released.

[kyeah]

ah nice, 1.15 is close 🙏. does that example happen to work on nightly without Into?

[saghm]

Weirdly enough, compiling with the latest nightly (1.16.0-nightly (df8debf6d 2017-01-25)) seems to give the same error. I honestly have no idea what's going on there.

I also tried compiling with the latest beta (1.15.0-beta.5 (10893a9a3 2017-01-19)) just because, and it also gave the same error.

The only thing I can think of is that due to the sheer magnitude of the change they've still been working on the implementation throughout the 1.15 development cycle and are just going to release all the new versions (1.15 stable, 1.16 beta, 1.17 nightly) with it at once.

[kyeah]

FWIW, I just tested this explicitly with the custom_derive macro and without Into on 1.13: it was nice and happy and fine. Assuming 1.15 allows custom derives baked in, we should be good to go.

[saghm]

Hey! We just merged #185 and pushed version 0.2.1, which (among other things) fixed the issue with autoIndexId and server versions 3.4.x, so I'm going to close this PR. Thanks again for bringing this to our attention!

[Gedweb]

So, awaiting 0.2.1 on crates.io =)

[saghm]

Oh, shoot, I thought I had already pushed it, but I guess not! It's pushed now.