Update to Mongo 3.2

[tmeasday]

PR: #7163

This issue is an attempt to outline what needs to be considered as part of this transition. We plan to look at this in the short/medium term, however if someone is keen to contribute to the project and wants to see it land earlier, this would be a great opportunity to make a larger contribution!

Update Mongo Driver to 3.2 compatible version #5763

• We need to update the version of the node-mongodb-native package used here: https://github.com/meteor/meteor/blob/devel/packages/npm-mongo/package.js to at least 2.1.0 (I guess we may as well get the latest 2.1.18).
• Check our uses of the driver in https://github.com/meteor/meteor/blob/devel/packages/mongo/ and other core code. Supposedly the API is backwards compatible, although there are some small issues mentioned here: Latest MongoDB native driver #5763 (comment)
• Ensure tests pass

(We should also probably remove the npm-mongo wrapper package and just peer-depend on the npm package from mongo, but I think we should wait to do this, it'll be a larger issue we have to solve with the 1.4 move to npm).

Update bundled Mongo in the dev bundle to 3.2 #5809

• Update our fork of Mongo [https://github.com/meteor/mongo] to 3.2 (there has been work done already in this direction: https://github.com/meteor/mongo/tree/ssl-r3.2.0). This fork allows us to statically link Mongo. (Note we didn't actually do this, using vanilla)
• Update https://github.com/meteor/meteor/blob/devel/scripts/build-mongo-for-dev-bundle.sh to build + include Mongo 3.2. (Note we didn't actually do this, using vanilla)
• Update generate-dev-bundle scripts
• Ensure that if you run the 3.2 Mongo server against an (older) 2.6 local database, it gives a meaningful error that suggests that you meteor reset or look into taking steps to upgrading your db contents
• Figure out if we need to turn journalling off or if there's a way to get the minimum journal size down.

Errata

• Update https://github.com/meteor/meteor/blob/devel/scripts/admin/test-packages-with-mongo-versions.rb to also test 3.2
• Ensure above tests pass on Mongo 2.4 or decide to drop support for it.
• Document changed behavior of update return value, as per Update to Mongo 3.2 #6957 (comment)
• Check if instructions for setting up oplog need updating

Notes on testing mongo binary SSL support:

cd /tmp
openssl req -newkey rsa:2048 -new -x509 -days 365 -nodes -out mongodb-cert.crt -keyout mongodb-cert.key
cat mongodb-cert.key mongodb-cert.crt > mongodb.pem
./mongod --sslMode requireSSL --sslPEMKeyFile /tmp/mongodb.pem  --dbpath /tmp/db
./mongo --ssl localhost/test --sslCAFile /tmp/mongodb-cert.crt

I don't think we need to do this:

• Resolve building the binary on developer machines - this means continue to maintain our fork of node-mongodb-native to include this commit mongodb/node-mongodb-native@634759e (which would now need to be against https://github.com/christkv/mongodb-core/blob/1.3/package.json)

[Fabs]

Hi @tmeasday. I am in between jobs right now, and I've been looking to contribute to meteor for a while. I think this is a great opportunity to start with, and something I can tackle.

I have a couple questions, since I've never contributed before.

• Is there any contribution guide you are used to point new contributors to?
• Which channel would be the most appropriate to talk with the people that worked with this part of meteor before? (in case I need help or help deciding an approach to solve something)

Anyways, I will start digging deeper on this issue later today, and if anyone eager to work with me on this shows up, I would be grateful to work together.

[tmeasday]

Hi @Fabs! Thanks for volunteering.

We have some basic contributing guidelines here: https://github.com/meteor/meteor/blob/devel/Contributing.md#making-changes-to-meteor-core

I think for now we can just communicate here on this issue, I'll do my best to be responsive.

I'm not sure if you had a plan around how you wanted to tackle this, but I've created a mongo-3.2 branch -- a start would be to a PR to it that:

1. Updates the npm-mongo package to just use the vanilla node-mongodb-native@2.1.9 package
2. Passes all test-packages tests when run with MONGO_URL and MONGO_OPLOG_URL set to point to a 3.2 server (for now just run Mongo locally on your machine by hand).

We'll still need to evaluate if we need a fork of that package or not (with the overarching concern being: can you install it without a toolchain on our 4 platforms), but this seems like a nice useful self contained piece of work.

[dgtlife]

I have made modifications to mongo and npm-mongo that involve the use of the latest MongoDB Node.js Driver (albeit still in the npm-mongo wrapper) and the addition of support for SSL, and X.509 Auth on SSL via additional environment variables. This is a quick hack to keep me on track while I wait for the "official" package(s) to be made available. You can find the mods at https://github.com/dgtlife/meteor-mongo-for-3.2.x.

Hopefully this helps the effort.

[Fabs]

Nice :). @tmeasday, I set up an environment with mongo 2.6.7 (from dev_build) and 3.2.6 (from homebrew) both with 3 replicas oplog and a single daemon. For the record, I have a failing test httpcall - S: errors true - not true - asyncBlock 0 (2 times). It happens for all setups except 3.2.6 with oplog, this one does not even connect with the current driver.

Anyways, going forward and changing the driver I have a few questions:

1. I made the switch on Npm.depends at npm-mongo to mongodb 2.1.18 (most recent on npm). Was there any particular to cite 2.1.9?
2. I forked meteor and merged devel onto mongo-3.2 there was a couple small changes. I plan to keep merging devel frequently so I don't end up much behind. Do you think this is a good strategy?

Anyway, I think I am set up and I can move forward with the work to fix the errors that I have, now that I changed to the new driver. I will keep you guys updated. @dgtlife, thanks for the link, I took a brief look and I am quite sure I will end up using it for help.

So, the journey begins :)

[Fabs]

Also, very frequently (just one or two times I did not see it) I also get the error accounts C: reconnect auto-login - fail — fail - message Issue #4970 has occured.. I looked #4970 and its closed. It happens with the default tarball mongodb driver, on devel. As it does the httpcall error I mentioned above.

[tmeasday]

Cool!

For the record, I have a failing test

A failing test in a vanilla setup? That would be quite surprising. Or do you mean failing when you run mongo yourself (from the dev bundle) and use MONGO_URL and MONGO_OPLOG_URL?

I made the switch on Npm.depends at npm-mongo to mongodb 2.1.18 (most recent on npm). Was there any particular to cite 2.1.9?

Not at all, I must have mis-read something (or they released 9 versions in the last week :) ). I'll update the top comment to avoid confusion.

I forked meteor and merged devel onto mongo-3.2 there was a couple small changes. I plan to keep merging devel frequently so I don't end up much behind. Do you think this is a good strategy?

No reason not to but I wouldn't expect many mongo-touching changes to devel to land.

[tmeasday]

That httpcall - errors test certainly shouldn't depend on mongo in any way, so something odd is going on. Do you see the error on latest devel?

[Fabs]

That httpcall - errors test certainly shouldn't depend on mongo in any way, so something odd is going on. Do you see the error on latest devel?

I took the time now to read the test carefully, I think maybe I know what the issue could be. I have OSX Server, and the address http://0.0.0.0/ resolves an webpage (apache is running, port 80).

So the assumption behind the test HTTP.call("GET", "http://0.0.0.0/", expect(unknownServerCallback)); does not hold on my machine, and thus the test fail. When I turn the server off, then the test stop failing. http://0.0.0.0:3000/ also resolves to my meteor server. Different from a vanilla mac, I can say I have dnsmasq with some confs for Docker and other machines at my network, even localhost, but nothing explicitly targeting 0.0.0.0

Also, I am at devel and there are few small differences from the past 6 hours (https://github.com/Fabs/meteor/pull/1) but none I believe relevant to the failing test.

[Fabs]

On a side note, already used something from @dgtlife to fix finding the primary replica set upon connection :). Thanks!

[tmeasday]

Whoa, 0.0.0.0 wasn't a great choice! Is there a known "bad" IP address? We should change the test to use one that doesn't resolve to localhost (I'm surprised this hasn't come up before). Perhaps 240.0.0.0 is a better choice? https://tools.ietf.org/html/rfc5735

Re: #4970 -- that test includes a timeout. I wonder if that's what's tripping for you (I've been looking at fixing flaky tests recently and not seen that one before, so I wonder what's different for you). Perhaps you could play around with the timeout?

[tmeasday]

PS I am on leave from tomorrow until Tuesday but @benjamn will jump in and help out with any other questions.

[abernix]

Opened #7038 for that failed test and I'll follow through on it. Nice find @Fabs. Continue as you were! :)

[Fabs]

About #4970, I am thinking about playing with it once I have the mongo done. Is that alright?

Thanks @abernix. Wish you a good time @tmeasday, thanks for the help you gave already.

Just an update, so far, using MONGO_URL without oplog it is looking good. There are about 60 tests broken on mongo-livedata, and a couple more here and there (accounts...). Most of the broken tests are about the results that come from the driver, for instance when Meteor.update is expected to return the number of modified documents, it is now returning an object with a lot of extra stuff...

[benjamn]

👋 folks

[guilhermedecampo]

This is pretty awesome! 👍

[Fabs]

Update time :).

The upsert method, specially simulateUpsertWithInsertedId was causing many failing tests. The current node-mongodb-native update method works with $setOnInsert and offers now, amidst its results, a list of upserted documents, where we can find the ID of the inserted document (which I believe was one of the restrictions preventing using a single update for upsert). I also preserved the NUM_OPTIMISTIC_TRIES behaviour, although I am not sure if it is still necessary.

Anyways, I am still dwelling in some corner cases, but I am confident I will be able to get rid of the previous implementation of upsert (with the doUpdate and doConditionalInsert) in favor of one with a single update and upsert:true. :)

Got 20 tests working today, 40 more to go.

[dgtlife]

From #5809, this may also be relevant https://jira.mongodb.org/browse/SERVER-14322

[Fabs]

More Updates.

I am now 5 failing tests to have it working without oplog and using upsert. All my troubles are with the _id field, but I think I am getting close. I believe by the end of this week I will have it working for this particular case.

If anybody has any ideas, one problem I am dealing is setting the "right" ID, on STRING id type collections when the user supply an upsert (or update with, upsert:true) without any modifier operators (col.upsert({foo: 1},{foo: 2, bar: 3}) for instance). If the insert happens, mongo will generate an ObjectId type ID.

I try using $setOnInsert to add a String ID (and $set for the rest of the update document). This fix the insert but brakes the update, when the expected behaviour would be replacing the entire document {$set does not do that}.

Also I think I break some behaviour that does not have failing tests so I will take time to double check it and add the tests like when (like I don't see a failing test because I used update(selector, {$set: obj}) instead of update(selector, obj) when I am quite sure the latter replaces the entire document).

[4Z4T4R]

@Fabs --- I see you're "personal" mongo-3.2 branch is 42 commits (at the time of this writing) ahead of the "official" mongo-3.2 branch.

Is it safe to say---despite not having the last test completed---that we can clone your "personal" mongo-3.2 in order to begin experimenting with MongoDB 3.2 behind Meteor?

Will this become the PR for 1.4?

I went though the entire gitlog on your branch. What's left to fix after tests are complete and passed?

Thank you for moving this forward! Hats off to you.

[abernix]

It would seem that #4436 will be getting resolved through this upgrade as the transformResult method will use nModified.

I'm not certain, but it would seem that Meteor was previously using the result from update which was records matched, not records modified.

If this is the case, this should be mentioned very clearly in the History.md as a breaking change.

[tmeasday]

@Fabs I've been pondering @abernix's comment above.

At first I thought it made sense to just stick with changing the .update() API to return nModified rather than nMatched as suggested in #4436, and document the change.

But re-reading the issue and thinking about it more, I have some objections to that:

1. It seems more than anything to be a documentation issue---as @davidecantini pointed out there are times where both return values are useful---@mitar's main point seems to be the behaviour doesn't match his expectation from the docs (and the solution seems easy, just change the docs to say "number matched" rather than the vague "number affected").
2. We'd have to change the API of minimongo too to be consistent.
3. It's something people could very easily miss.

There just doesn't seem to be all that a compelling case for sticking with changing it. I'd love to hear your thoughts, but I'm thinking we should change it back to nMatched (and add a test for it).

[mitar]

I have never ever needed number of matched documents, but I have always needed number of changed documents. Can somebody tell me use cases where first would be preferred over others. (Let's imagine that we are designing this from scratch and that we can decide on one value to be returned.)

We also have documentation so that we have an "excuse" to fixing this in a backwards incompatible manner. But we should add some switch to return the whole object for those who really need previous behavior. But I really do not know when you would need number of matched documents.

[Fabs]

I have not changed any of the tests during the driver changes and I returned nModified (or 1 for insertions) for all update tests. So, I believe either the documentation was always wrong, or there is no test where nModified != nMatched and an explicit check on numberAffected expecting it to be nMatched happens (I will make sure to add a test for this behaviour anyway).

My preference would be to do something in the direction of @davidecantini proposal, returning at least two values nMatched and nModified (on the insert, there is the insertedId) and get rid of numberAffected. I agree with @mitar that the nModified seems to me the most useful result, I never ever even noticed that behaviour before, I would always just go oh, the result of update has to be the number of changed things, but I also can see when other developers would reason differently specially with the current documentation.

One feature I think we are in a good position to add now though, is returning the whole rawResult by specifying an option on the operation (update/insert/remove...) fullResult: true or result: 'full' or ... . The idea here is that I will always have a meteor style result as default, but whenever I want the internal driver format (that could even be rethinkDB or mysql or whatever driver) I can have a more complete result without resorting to rawCollection operations (there is the writeConcernError that comes on the driverResult that I can also see being really useful for people worrying about consistency).

Another more simplistic idea, would be returning the internal result object as a property of the meteor result, all the time.

[Fabs]

Guys, am I missing something? I went to the docs, and they read:

Modify one or more documents in the collection, or insert one if no matching documents were found. Returns an object with keys numberAffected (the number of documents modified) and insertedId (the unique _id of the document that was inserted, if any).

Was that updated?

[tmeasday]

Can somebody tell me use cases where first would be preferred over others

This comment explains some examples where both are useful: #4436 (comment). Ultimately, this is clearly why MongoDB changed the wire protocol.

Let's imagine that we are designing this from scratch and that we can decide on one value to be returned

Unfortunately we don't live in a world where we can design things in a white room. Back-compat is a real thing. If I was designing it from scratch, I'd return the full {nModified, nMatched, nUpserted} document that Mongo does over the wire.

However, I'm wary of making that change to the default API, because if anyone has code of the form if (! X.update(...)) (or even the more correct if (X.update(...) === 0)) right now, changing the API to always return an object will definitely break it.

I believe either the documentation was always wrong, or there is no test

The documentation was indeed always wrong[1], and I think you are right that there is no test.

One feature I think we are in a good position to add now though, is returning the whole rawResult by specifying an option

I think this is the right approach. It should be easy to send a PR to do this after 1.4

Guys, am I missing something?

You are looking at the upsert docs. The update docs say: "Returns the number of affected documents."

[1] Or at best confusing.

[tmeasday]

I just don't think there's a strong enough case to be made to break back-compat (even though I agree most people probably would never had made the distinction between nModified and nMatched, esp given the docs implied the first).

I definitely think we should add a test to avoid unintentionally changing this in future (and to ensure consistency between the Mongo Driver and Minimongo).

[mitar]

Hm, but to me it is a bug. I was programming against documentation and I reported a bug that Meteor is not returning correct value. This change will always be incompatible for some users. Or those who read documentation, or those who ignored it. The question is then into which direction we want to move. Which set of users we want to inconvenience?

[tmeasday]

I don't think it's fair to say that it's "breaking compatibility" if people mis-understood the API because of poor documentation and we don't change it.

It's just not a bug, pure and simple. The documentation was wrong/confusing. You think the API should work differently, but that doesn't make it a bug.

[mitar]

I had in one my code for 2 years this bug because I read documentation and do not always check implementation because I trust code. I suspect that others have similar code based on documentation. For them this is a breaking change.

[tmeasday]

How can it be a breaking change when we aren't changing anything? You've lost me..

[mitar]

It is a breaking change because for those who implemented code based on documentation have a hidden bug. I had such bug for two years and I had very nasty issues with it: my code went into an infinite loop blocking the whole server. Of course it was hard to find it. Because it happened only when there was a race condition that document has already the same values as I wanted to update to. Tricky to debug. Probably my case is an extreme case, but still.

Anyway, I do not really case. I fixed it for myself and it works. I just think more people program against documentation than program against implementation.

[davidecantini]

Hi guys,
Considering the two options:

a) change the documentation to reflect the current (and past) implementation;
b) change the implementation, to reflect the current (and past) documentation;

The question should be: which one has less chances of breaking existing codes?

Any real code/app gets tested and it's the implementation, in the end, that causes your tests to pass or fail. So, while it's true that one should follow the documentation, it's also true that as soon as your tests fail (because you expected nModified and actually got nMatched) you'd surely have to follow the implementation (if you want your tests to pass).

That said, it appears reasonable to say that option a) has less chances of breaking existing codes than option b), and, so, a) should be the way to go. If so, would the following be something we can agree on?

1. Replace "Returns the number of affected documents." with "Returns the number of matched documents" to reflect the actual behavior of Meteor.Update and add a "Possible breaking change" note, even though in most cases, if not all, there won't be anything broken (for the reasons mentioned above).
2. Add an option to return the whole raw result (as mentioned by Fabs) which can give full control, for those who need it.

I hope this helps, thanks.

[mitar]

it's also true that as soon as your tests fail (because you expected nModified and actually got nMatched) you'd surely have to follow the implementation (if you want your tests to pass).

Even Meteor do not have tests for this edge case. And this is pretty tricky to test because it might depend on a race condition.

[davidecantini]

@mitar, do you believe that option b) would have less chances of breaking existing codes than option a)?

[mitar]

I think so, but only because of my sample size of 1 (me). :-) Anyway, I will leave to others to decide this. I have spoken too much on this topic.

[davidecantini]

@mitar, it's fine to have opinions, regardless of the sample size :) Here is a simple case from a real app:

let n = Cards.update({_id: card._id}, {$set: card});
if (n === 0) {
    // No match: the document may not exist. Throw an error and show a UI error msg
     throw new Meteor.Error(...);
}

As you can see, that is a case where a user opens the UI to update an item and saves it without modifying anything. It's something quite common and easy to test. Well, if you go for option b) then each and every existing code similar to that would get broken.
So, all in all, option a) appears to be the lesser of two evils here.

[Fabs]

So. I Made numberAffected return nMatchedm the backwards compatible way :).
#7190

I hope once those modifications of the driver hit 1.4, somebody (probably me, but anyone could ;)) will make the rawResult approach we've been discussing.

About the docs. What is the process to fix them (if no one is planning to do that I volunteer)?

@mitar , do you think once we have added the rawResult proposed feature, it will solve the problem for you?

[mitar]

Yea, it is OK.

[tmeasday]

About the docs. What is the process to fix them (if no one is planning to do that I volunteer)?

e2904d9

[jedwards1211]

@tmeasday the fork of mongo is for dev only, right?

[tmeasday]

Actually we don't use a fork any more. But it was just for dev, yeah.

[benjamn]

The changes associated with this issue have been merged into release-1.4, and the corresponding PR is closed, so I think we can close this issue now. Thanks @tmeasday and @Fabs and everyone else for making this happen!