New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make minimongo upsert compliant with mongo behavior #8815

Closed
wants to merge 21 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@sebakerckhof
Contributor

sebakerckhof commented Jun 17, 2017

Attempt to fix: #8806, #5611, #8794, #5294 and #8775

This turned out to be quite painful. Following https://github.com/mongodb/mongo/blob/master/src/mongo/db/update/update_driver.cpp#L176 takes you to a big journey of mongo's source code. So a direct port is not really viable. I tried to infer what I could and also discovered a lot of rules with trial & error. It's possible there are more things I've missed.

However, this PR works with all old tests and makes the upsert behavior compliant with a big range of more complex upserts for every rule I could infer.

But there's a catch: This is possibly a BC break, since the behavior for these more complex selectors is, albeit compliant with mongo, different then what Meteor would have entered in the database before.
E.g. this upsert: {$and: [{foo: "bar"}]}, {$set: {a: 123}} would now correctly result in {foo: 'bar', a: 123}, while in old versions it would be just {a: 123}
To make matters worse, it is not just for minimongo, but because Meteor simulates upserts for Mongo 2.4 compatibility (see: https://github.com/meteor/meteor/blob/devel/packages/mongo/mongo_driver.js#L527) it's also for what ends up in the actual database in some situations.
Now wouldn't be a bad time to remove that simulated upsert stuff (if 2.4 support is no longer a requirement), otherwise behavior might break again in the future.

So this needs a decent review and some warnings in changelogs here and there.

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jun 17, 2017

Okay #2278 (comment) and #7758 made me realize this needs a little more work on the mongo (not minimongo) side of things. However, this would also be simplified if we can do away with the simulated upserts

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jun 19, 2017

@hwillson I know tests are failing but I'm kinda waiting on a decision on the simulated upsert stuff to continue/finish this.

@hwillson

This comment has been minimized.

Member

hwillson commented Jun 20, 2017

Thanks for all of your work on this @sebakerckhof! I've been thinking about the simulated upsert stuff a bit, and don't have an answer yet. We'll definitely have to tread lightly here because of BC, but your changes make a lot of sense. I'll review this more thoroughly tomorrow and post my thoughts. Thanks again!

@hwillson

This comment has been minimized.

Member

hwillson commented Jun 20, 2017

@sebakerckhof - I've thought about this more; I'm okay with dropping simulated upsert's (and Mongo 2.4 support, since it reached end of life on March 2016). As you mentioned, getting rid of simulated upsert's will help prevent future problems in this area (and help clean up the codebase a bit).

That being said, we should definitely get MDG's opinion on this (@benjamn @abernix - quick synopsis; go to packages/mongo/mongo_driver.js#L527 and read that comment. Getting rid of that simulated upsert code and using $setOnInsert will reduce future maintenance headache. Only gotcha is we have to drop Mongo 2.4 support).

We'll be discussing this PR in our issue review meeting tomorrow, so if you don't hear back today, we'll make sure you have an update tomorrow. Thanks!

@abernix

This comment has been minimized.

Member

abernix commented Jun 20, 2017

I think this is reasonable to do. Though, does anyone know what mLab and Compose.io offer for their most "classic" options? (Presumably all their new customers are being on-boarded with at least Mongo 3.4.)

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jun 20, 2017

For mlab it's 2.6 ( http://docs.mlab.com/ops/ ). Couldn't find anything for compose.

@hwillson

This comment has been minimized.

Member

hwillson commented Jun 20, 2017

Compose.io "classic" subscribers get Mongo 2.6.9 as the default for new deploys, but anything Compose supported earlier is still supported, for existing deploys (mLab is probably similar). That being said, I think the changes in this PR are going to break backwards compatibility enough that we might want to tie them to a major release. If that's the case, then mentioning that Mongo 2.4 is no longer supported might be okay (since both Compose and mLab provide upgrade paths and 2.4 (and 2.6) is EOL).

@abernix abernix changed the title from Make minimongo upsert compliant with mongo behavior to [Work in Progress] Make minimongo upsert compliant with mongo behavior Jun 21, 2017

@hwillson

This comment has been minimized.

Member

hwillson commented Jun 21, 2017

@sebakerckhof - we just discussed this in our issue triage meeting, and have agreed that it's okay to drop Mongo 2.4 support (starting from whatever Meteor version this PR gets merged into). So 👍 to removing the simulated upsert code as part of this PR. Thanks!

@sebakerckhof sebakerckhof changed the title from [Work in Progress] Make minimongo upsert compliant with mongo behavior to Make minimongo upsert compliant with mongo behavior Jun 23, 2017

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jun 23, 2017

Okay, this should be ready for review.
Turned out a bit more difficult than I initially thought, which makes that I cannot fully drop simulated upserts where the mod is a replacement and where the _id has to be controlled from meteor but neither the query, nor the replacement contains an id.
E.g. upsert({"foo": "bar"}, {"bar": 1})

Still dropping 2.4 is important because:

  • Upsert behavior (both how the query and modifier is treated) is very different between 2.4 and 2.6+, so minimongo couldn't possibly be correct.
  • This makes it possible to have the result in the database to always be exactly the same as when done via mongo shell, even with the simulated version.
    Old version was dependend on minimongo behavior which was no longer correct and might still not be 100% compliant, but the result in the database will be.
  • Mongo node driver 2.x does not officially support mongo 2.4
  • We need the simulated upserts in fewer cases now.

I just hope it doesn't go: https://imgs.xkcd.com/comics/workflow.png

@hwillson hwillson self-requested a review Jun 28, 2017

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jul 6, 2017

@hwillson Any idea when this can be reviewed? It's just that I'd like to do act on possible change requests when it's still fresh in my head.

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 6, 2017

Sorry for the delay @sebakerckhof - was traveling the past few days. I'll get this reviewed tomorrow for sure!

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 7, 2017

@sebakerckhof Would you mind rebasing when you have a sec?

@sebakerckhof sebakerckhof force-pushed the sebakerckhof:fix/upserts branch from 1450fed to 7934454 Jul 7, 2017

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jul 7, 2017

@hwillson I just had a sec.

@hwillson

Hi @sebakerckhof - I've been testing these changes out, and haven't been able to poke any holes in them! Everything I try seems to be working properly. I've added a few code formatting / organization suggestions, but in terms of functionality I think we're pretty much good to go here. In addition to the code review suggestions, would you mind adding a History.md entry that gives a brief overview of these changes, and that mentions Mongo 2.4 support is being dropped? Thanks!

@@ -70,6 +67,10 @@ LocalCollection._modify = function (doc, mod, options) {
modFunc(target, field, arg, keypath, newDoc);
});
});
if (doc._id && !EJSON.equals(doc._id, newDoc._id)) {
throw MinimongoError(`After applying the update to the document {_id: "${doc._id}" , ...}, the (immutable) field '_id' was found to have been altered to _id: "${newDoc._id}"`);

This comment has been minimized.

@hwillson

hwillson Jul 7, 2017

Member

Please wrap this at 80 chars.

// Fills a document with certain fields from an upsert selector
populateDocumentWithQueryFields = function (query, document = {}) {
if (Object.getPrototypeOf(query) === Object.prototype) {

This comment has been minimized.

@hwillson

hwillson Jul 7, 2017

Member

Please switch to 2 space indents in this file (to match the rest of core).

value.forEach(sq => populateDocumentWithQueryFields(sq, document))
}
// handle $or nodes with exactly 1 child
else if (key === "$or") {

This comment has been minimized.

@hwillson

hwillson Jul 7, 2017

Member

Another small formatting request - please move else if's up on the same line as the closing brace.

// - you can not have '$'-prefixed keys more than one-level deep in an object
// Fills a document with certain fields from an upsert selector
populateDocumentWithQueryFields = function (query, document = {}) {

This comment has been minimized.

@hwillson

hwillson Jul 7, 2017

Member

Instead of using a global here, could you export populateDocumentWithQueryFields and import it in packages/minimongo/minimongo.js? minimongo has modules as a dependency (via ecmascript), so we can start to modernize the codebase a bit.

if (Object.getPrototypeOf(query) === Object.prototype) {
// handle implicit $and
for (let [key, value] of Object.entries(query)) {

This comment has been minimized.

@hwillson

hwillson Jul 7, 2017

Member

Using let [key, value] ... here works, but using Object.keys(query).forEach(... is preferred (this was mentioned in another code review, which I discussed with @benjamn afterwards; turns out the Object.keys(...).forEach(... approach is more performant behind the scenes).

(existingKey.length > key.length && existingKey.indexOf(key) === 0)
|| (key.length > existingKey.length && key.indexOf(existingKey) === 0)
) {
throw new Error(`cannot infer query fields to set, both paths '${existingKey}' and '${key}' are matched`)

This comment has been minimized.

@hwillson

hwillson Jul 7, 2017

Member

Can you make sure longer lines are wrapped at 80 chars? Switching the indentation to 2 chars will help, but some lines (like this one) are still over.

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jul 10, 2017

@hwillson Ok. Did all the requested changes (I think)

@sebakerckhof

This comment has been minimized.

Contributor

sebakerckhof commented Jul 12, 2017

Btw, about let [key, value] of Object.entries(obj) vs Object.keys(obj).forEach(...): a quick benchmark in chrome 59 seems to show there's no real difference anymore with v8 5.9 (which has TF+I on by default). Since meteor 1.6 will likely be released node 8.2+ (which uses v8 5.9) this policy might need to be reconsidered. At least, I think let [key, value] of Object.entries(obj) is more readable if you need both the key and value.
Of course, this won't be true on the client for people using older browsers, but there you don't know if they're using v8, so v8 tricks don't make much sense and in this case the chance is very small a user will be doing thousands of big upserts per second.

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 12, 2017

Your changes look great @sebakerckhof, thanks! And good to know about let [key, value] of .... LGTM!

@benjamn benjamn added this to the Release 1.6 milestone Jul 12, 2017

benjamn added a commit that referenced this pull request Jul 13, 2017

@benjamn

This comment has been minimized.

Member

benjamn commented Jul 13, 2017

Merged into devel as 08f5ea4. Thanks so much for working on this @sebakerckhof!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment