fix(operations): avoid hardcoding checkKeys for insert operations

[vkarpov15]

Description

What changed?

It looks like all insert operations hardcode checkKeys: true. I'm not sure if this change was intentional, I tracked the change to this commit: 07fd317 . If this change was intentional, it would make sense to make a note in the changelog.

Are there any files to ignore?

Nope

[durran]

LGTM

[nbbeeken]

I think this change was made to prevent a footgun scenario:

These lines are from the mongo shell

db.collection.insertOne({'a.b': 2})
{acknowledged: true, insertedId: ObjectId("60199236030903c61aacb425") }
> db.collection.find({})
{ _id: ObjectId("60199236030903c61aacb425"), "a.b": 2 }
> db.collection.find({'a.b': 2}).toArray()
[ ]

The shell permits you to insert a document with such keys but due to a known limitation of keys with either dots or dollar signs. It is not possible to query such documents based on the offending key. I think we would be better served here to default checkKeys to true if nothing is provided and otherwise allow users who need to insert such documents to override with an explicit false setting.

@vkarpov15 let us know if you'd want us to tackle that work instead we can handle making the changes and tests accordingly.

Tracking with NODE-1652.

[vkarpov15]

As far as I know, checkKeys is true by default and you need to explicitly set checkKeys: false in order to opt in to storing dotted keys. I don't have a built-in example using the MongoDB driver, but here's an example of how this works with Mongoose: https://github.com/Automattic/mongoose/blob/ca16599d7e0cbe6b3a51b4ca37bed0b7e137c581/test/document.test.js#L6926-L6940

[nbbeeken]

Ah my mistake, I just double checked 3.6 functionality, I can corroborate that this is what occurs:

await c.db('test').collection('collection').insertOne({'a.b': 2})
Thrown:
Error: key a.b must not contain '.'
   ...
> await c.db('test').collection('collection').insertOne({'a.b': 2}, {checkKeys: false})
   ... CommandResult
>

So yes the defaulting behavior is what we want to preserve.

[nbbeeken]

This change removes the default to true behavior, I didn't dig further into 3.6 to see where we were doing the defaulting but for the sake of this version we can do it here in the constructor.

[durran]

As a note, for server version 3.6 the server started allowing "." and "$" in field names, so it seems making the default not validate them here makes sense going forward.

[vkarpov15]

Either way, we should make a note in the changelog if the checkKeys behavior changed. In node driver 3.6, you couldn't use dotted keys unless you explicitly set checkKeys: false

[nbbeeken]

Just to clarify: as it stands with the merged change, the behavior is the same as 3.6 (no dots/dollars allowed unless checkKeys: false).

I would say even though the server permits dots and dollars it seems fair to leave the ability to insert documents with such keys under a flag given the inability at this time to query documents with such keys. There is work planned to allow querying such documents here SERVER-30575. Maybe that's a time to reconsider gating this behavior with a flag.

However since this driver supports server versions back to 2.6 where these keys wouldn't be permitted I think its an acceptable footgun prevention measure we can hold on to.

[nbbeeken]

We're putting out a bump of the beta today and it would be nice to get this quick fix in hence my commit here.
Just waiting on CI and @emadum