From e329373400ff143f8aa259b64ce5642bad0c116c Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 13 Jan 2021 14:04:29 -0500 Subject: [PATCH 1/4] fix: hasAtomicOperator check respects toBSON transformation 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 --- lib/utils.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 17f53850fae..fbd2730f6d6 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -809,8 +809,12 @@ function hasAtomicOperators(doc) { return doc.reduce((err, u) => err || hasAtomicOperators(u), null); } - const keys = Object.keys(doc); - return keys.length > 0 && keys[0][0] === '$'; + let docToTest = doc; + if (typeof doc.toBSON === 'function') { + docToTest = doc.toBSON() + } + + return Object.keys(docToTest).map(k => k[0]).indexOf('$') >= 0 } module.exports = { From bfa07dcdc44475b54a0d8f8b8f0a39a4b6c434b9 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 13 Jan 2021 14:39:37 -0500 Subject: [PATCH 2/4] fix: lint and use ternary --- lib/utils.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index fbd2730f6d6..0ebbb4565af 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -809,12 +809,11 @@ function hasAtomicOperators(doc) { return doc.reduce((err, u) => err || hasAtomicOperators(u), null); } - let docToTest = doc; - if (typeof doc.toBSON === 'function') { - docToTest = doc.toBSON() - } - - return Object.keys(docToTest).map(k => k[0]).indexOf('$') >= 0 + return ( + Object.keys(typeof doc.toBSON !== 'function' ? doc : doc.toBSON()) + .map(k => k[0]) + .indexOf('$') >= 0 + ); } module.exports = { From ac46c92c75041d65cb94ea783822214f7528906e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 14 Jan 2021 12:28:53 -0500 Subject: [PATCH 3/4] test: add unit and integration test for respecting toBSON transform --- test/functional/bulk.test.js | 66 ++++++++++++++++++++++++++++++++++++ test/unit/utils.test.js | 14 ++++++++ 2 files changed, 80 insertions(+) diff --git a/test/functional/bulk.test.js b/test/functional/bulk.test.js index cb58c86d1d4..e4a5e620fd9 100644 --- a/test/functional/bulk.test.js +++ b/test/functional/bulk.test.js @@ -1662,4 +1662,70 @@ 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(err).to.be.instanceOf( + TypeError, + 'Replacement document must not use atomic operators' + ); + } + }) + .finally(() => { + return client.close(); + }); + }); }); diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index 494664f37da..eb34e21044a 100644 --- a/test/unit/utils.test.js +++ b/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; + }); }); From 4407643f93b8ed703b7fb4f653afcecedaca5d21 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 14 Jan 2021 12:34:59 -0500 Subject: [PATCH 4/4] fix test --- test/functional/bulk.test.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/functional/bulk.test.js b/test/functional/bulk.test.js index e4a5e620fd9..d42655c6b3a 100644 --- a/test/functional/bulk.test.js +++ b/test/functional/bulk.test.js @@ -1718,10 +1718,7 @@ describe('Bulk', function() { } ); } catch (err) { - expect(err).to.be.instanceOf( - TypeError, - 'Replacement document must not use atomic operators' - ); + expect.fail(); // shouldn't throw any error } }) .finally(() => {