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

Ignore undefined fields when inserting/updating in Mongo #9444

Merged
merged 3 commits into from Dec 13, 2017

Conversation

Projects
None yet
5 participants
@hwillson
Member

hwillson commented Dec 6, 2017

Meteor's current behaviour when handling Mongo inserts/updates with undefined fields differs when coming from the client versus the server:

Client:

widgetCollection.insert({ name: undefined });

leads to the following in Mongo

{ id: '...' }

Server:

widgetCollection.insert({ name: undefined });

leads to the following in Mongo

{ id: '...', name: null }

As discussed in issue #6051, the approach taken for queries initiated from the client or server should be consistent.

This is a long standing issue that is caused by a few things:

  • undefined is not a valid JSON/EJSON type so when client side Mongo inserts/updates are sent to the server for handling, the undefined fields are stripped.
  • Mongo's own BSON spec has flagged undefined as deprecated, which means the use of undefined within the Mongo landscape is not recommended. This has lead to some difficulties when using undefined in Mongo queries as how undefined should be handled has been interpreted in different ways by different Mongo client authors. By default the official Mongo Node client that Meteor uses converts undefined to null when inserting/updating.

There has been a lot of discussion on the interwebs around how the Mongo Node driver handles undefined fields, and the problems that can arise from just converting undefined to null. To that end, the Mongo Node driver team added support for a ignoreUndefined connection option a while back (which is false by default for backwards compatibility reasons). When ignoreUndefined is true, fields with undefined values are dropped when inserting/updating, instead of being converted to null.

This PR sets the ignoreUndefined option to true for all Mongo connections, which makes the handling of client/server initiated Mongo inserts/updates consistent.

This could potentially be a backwards incompatible change if an app is say expecting an undefined field to be inserted as a null field. This change does better align with Mongo's recommendation of not using undefined however, so although some backwards compatibility might be sacrificed, that sacrifice is essential to better align with Mongo. Mentioning this in the History.md should hopefully be enough.

Fixes #6051.

hwillson added some commits Dec 6, 2017

Ignore undefined fields when inserting/updating in Mongo
The Mongo Node driver that Meteor uses currently replaces
`undefined` field values with `null`, when doing an
insert/update. This approach can lead to unexpected behaviour,
as outlined in #1646, #6051 and several other issues. This commit
configures the default Mongo connection to `ignoreUndefined`
fields, which means `undefined` fields are not inserted/updated,
instead of being inserted/updated as `null`.

Fixes #6051.

@benjamn benjamn merged commit ce3885b into meteor:devel Dec 13, 2017

2 of 12 checks passed

ci/circleci: Group 0 CircleCI is running your tests
Details
ci/circleci: Group 1 CircleCI is running your tests
Details
ci/circleci: Group 2 CircleCI is running your tests
Details
ci/circleci: Group 3 CircleCI is running your tests
Details
ci/circleci: Group 4 CircleCI is running your tests
Details
ci/circleci: Group 5 CircleCI is running your tests
Details
ci/circleci: Group 6 CircleCI is running your tests
Details
ci/circleci: Group 7 CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details

@benjamn benjamn added this to the Package Patches milestone Dec 13, 2017

@romainkoenig

This comment has been minimized.

romainkoenig commented Apr 13, 2018

This could potentially be a backwards incompatible change if an app is say expecting an undefined field to be inserted as a null field. This change does better align with Mongo's recommendation of not using undefined however, so although some backwards compatibility might be sacrificed, that sacrifice is essential to better align with Mongo. Mentioning this in the History.md should hopefully be enough.

It wasn't, for such a breaking change, updating the patch version is not enough.
Then for a findOne({ name: variable }) with a variable potentially undefined. It goes from :
No data returned to the first document of the collection returned, or even more with find ?

Do you imagine with : find({ userId }) -> from no one to everyone :-(

It takes time to find such a bug, because it does not throw...
I don't understand, this is not a patch, this is a contract being modified

@hwillson

This comment has been minimized.

Member

hwillson commented Apr 13, 2018

@romainkoenig Yes, this is a painful issue to resolve. It's a bug either way; either you end up with unexpected results when you have your undefined field stripped in a find and you weren't expecting it to be, or you end up having null stored from an insert in your collection when you weren't expecting it to be. It's a tricky situation, and to mitigate the risk, this PR should be followed up with an implementation that addresses the original intent of the sister PR #9671. There is a new issue open to track this work - see #9780.

Just to sum up though - the changes made here were intended to help address an existing bug caused by Meteor's automatic assumption that undefined could be converted to null when sent over to Mongo. This assumption has lead to numerous other errors (and GitHub issues), so this PR was intended to help line Meteor up with what Mongo is expecting (which is not to use undefined). I definitely agree that there is more work to do here, which #9780 is tracking (and which is pull-requests-encouraged if you're interested).

@markreid

This comment has been minimized.

Contributor

markreid commented Sep 6, 2018

This has caused a major issue in one of our apps. At first we thought it was only under quite specific circumstances but actually it's across the board.

I understand the motivation for the change in design, makes sense, but I'm basically speechless that even after @romainkoenig mentioned here that it's a really major change and really should be documented more clearly in the changelog, he was just ignored?

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