fix: hasAtomicOperator check respects toBSON transformation (#2696)

Certain documents cannot contain atomic operators
i.e. keys with a leading dollar sign, documents can contain
toBSON transformation functions that would modify such keys the check
now respect the transformation

NODE-2741

Author: nbbeeken

lib/utils.js

@@ -809,8 +809,11 @@ function hasAtomicOperators(doc) {
     return doc.reduce((err, u) => err || hasAtomicOperators(u), null);
   }
 
-  const keys = Object.keys(doc);
-  return keys.length > 0 && keys[0][0] === '$';
+  return (
+    Object.keys(typeof doc.toBSON !== 'function' ? doc : doc.toBSON())
+      .map(k => k[0])
+      .indexOf('$') >= 0
+  );
 }
 
 module.exports = {

test/functional/bulk.test.js

@@ -1662,4 +1662,67 @@ describe('Bulk', function() {
         );
     });
   });
+
+  it('should enforce no atomic operators', function() {
+    const client = this.configuration.newClient();
+    return client
+      .connect()
+      .then(() => {
+        const collection = client.db().collection('noAtomicOp');
+        return collection
+          .drop()
+          .catch(ignoreNsNotFound)
+          .then(() => collection);
+      })
+      .then(collection => {
+        return collection.insertMany([{ a: 1 }, { a: 1 }, { a: 1 }]).then(() => collection);
+      })
+      .then(collection => {
+        try {
+          return collection.replaceOne({ a: 1 }, { $atomic: 1 });
+        } catch (err) {
+          expect(err).to.be.instanceOf(
+            TypeError,
+            'Replacement document must not use atomic operators'
+          );
+        }
+      })
+      .finally(() => {
+        return client.close();
+      });
+  });
+
+  it('should respect toBSON conversion when checking for atomic operators', function() {
+    const client = this.configuration.newClient();
+    return client
+      .connect()
+      .then(() => {
+        const collection = client.db().collection('noAtomicOp');
+        return collection
+          .drop()
+          .catch(ignoreNsNotFound)
+          .then(() => collection);
+      })
+      .then(collection => {
+        return collection.insertMany([{ a: 1 }, { a: 1 }, { a: 1 }]).then(() => collection);
+      })
+      .then(collection => {
+        try {
+          return collection.replaceOne(
+            { a: 1 },
+            {
+              $atomic: 1,
+              toBSON() {
+                return { atomic: this.$atomic };
+              }
+            }
+          );
+        } catch (err) {
+          expect.fail(); // shouldn't throw any error
+        }
+      })
+      .finally(() => {
+        return client.close();
+      });
+  });
 });

test/unit/utils.test.js

@@ -2,6 +2,7 @@
 const eachAsync = require('../../lib/core/utils').eachAsync;
 const makeInterruptableAsyncInterval = require('../../lib/utils').makeInterruptableAsyncInterval;
 const now = require('../../lib/utils').now;
+const hasAtomicOperators = require('../../lib/utils').hasAtomicOperators;
 const expect = require('chai').expect;
 const sinon = require('sinon');
 
@@ -163,4 +164,17 @@ describe('utils', function() {
       this.clock.tick(250);
     });
   });
+
+  it('should assert hasAtomicOperators and respect toBSON conversion', function() {
+    expect(hasAtomicOperators({ $key: 2.3 })).to.be.true;
+    expect(hasAtomicOperators({ nonAtomic: 1, $key: 2.3 })).to.be.true;
+    expect(
+      hasAtomicOperators({
+        $key: 2.3,
+        toBSON() {
+          return { key: this.$key };
+        }
+      })
+    ).to.be.false;
+  });
 });