Merge pull request #14692 from Automattic/vkarpov15/gh-14689

fix(query): remove `count()` and `findOneAndRemove()` from query chaining
Automattic · Jul 1, 2024 · 4ac878e · 4ac878e
2 parents 87c57e6 + 6136846
commit 4ac878e
Show file tree

Hide file tree

Showing 11 changed files with 194 additions and 21 deletions.
diff --git a/lib/document.js b/lib/document.js
@@ -3052,7 +3052,6 @@ Document.prototype.$__validate = function(pathsToValidate, options, callback) {
       const doValidateOptions = {
         ...doValidateOptionsByPath[path],
         path: path,
-        validateModifiedOnly: shouldValidateModifiedOnly,
         validateAllPaths
       };
 

diff --git a/lib/helpers/projection/applyProjection.js b/lib/helpers/projection/applyProjection.js
@@ -35,6 +35,9 @@ function applyExclusiveProjection(doc, projection, hasIncludedChildren, projecti
   if (doc == null || typeof doc !== 'object') {
     return doc;
   }
+  if (Array.isArray(doc)) {
+    return doc.map(el => applyExclusiveProjection(el, projection, hasIncludedChildren, projectionLimb, prefix));
+  }
   const ret = { ...doc };
   projectionLimb = prefix ? (projectionLimb || {}) : projection;
 
@@ -57,6 +60,9 @@ function applyInclusiveProjection(doc, projection, hasIncludedChildren, projecti
   if (doc == null || typeof doc !== 'object') {
     return doc;
   }
+  if (Array.isArray(doc)) {
+    return doc.map(el => applyInclusiveProjection(el, projection, hasIncludedChildren, projectionLimb, prefix));
+  }
   const ret = { ...doc };
   projectionLimb = prefix ? (projectionLimb || {}) : projection;
 

diff --git a/lib/query.js b/lib/query.js
@@ -153,6 +153,12 @@ function Query(conditions, options, model, collection) {
 
 Query.prototype = new mquery();
 Query.prototype.constructor = Query;
+
+// Remove some legacy methods that we removed in Mongoose 8, but
+// are still in mquery 5.
+Query.prototype.count = undefined;
+Query.prototype.findOneAndRemove = undefined;
+
 Query.base = mquery.prototype;
 
 /*!

diff --git a/lib/schema/array.js b/lib/schema/array.js
@@ -607,24 +607,7 @@ function cast$elemMatch(val, context) {
     }
   }
 
-  // Is this an embedded discriminator and is the discriminator key set?
-  // If so, use the discriminator schema. See gh-7449
-  const discriminatorKey = this &&
-    this.casterConstructor &&
-    this.casterConstructor.schema &&
-    this.casterConstructor.schema.options &&
-    this.casterConstructor.schema.options.discriminatorKey;
-  const discriminators = this &&
-  this.casterConstructor &&
-  this.casterConstructor.schema &&
-  this.casterConstructor.schema.discriminators || {};
-  if (discriminatorKey != null &&
-      val[discriminatorKey] != null &&
-      discriminators[val[discriminatorKey]] != null) {
-    return cast(discriminators[val[discriminatorKey]], val, null, this && this.$$context);
-  }
-  const schema = this.casterConstructor.schema ?? context.schema;
-  return cast(schema, val, null, this && this.$$context);
+  return val;
 }
 
 const handle = SchemaArray.prototype.$conditionalHandlers = {};

diff --git a/lib/schema/documentArray.js b/lib/schema/documentArray.js
@@ -11,9 +11,11 @@ const SchemaArray = require('./array');
 const SchemaDocumentArrayOptions =
   require('../options/schemaDocumentArrayOptions');
 const SchemaType = require('../schemaType');
+const cast = require('../cast');
 const discriminator = require('../helpers/model/discriminator');
 const handleIdOption = require('../helpers/schema/handleIdOption');
 const handleSpreadDoc = require('../helpers/document/handleSpreadDoc');
+const isOperator = require('../helpers/query/isOperator');
 const utils = require('../utils');
 const getConstructor = require('../helpers/discriminator/getConstructor');
 const InvalidSchemaOptionError = require('../error/invalidSchemaOption');
@@ -114,6 +116,7 @@ SchemaDocumentArray.options = { castNonArrays: true };
 SchemaDocumentArray.prototype = Object.create(SchemaArray.prototype);
 SchemaDocumentArray.prototype.constructor = SchemaDocumentArray;
 SchemaDocumentArray.prototype.OptionsConstructor = SchemaDocumentArrayOptions;
+SchemaDocumentArray.prototype.$conditionalHandlers = { ...SchemaArray.prototype.$conditionalHandlers };
 
 /*!
  * ignore
@@ -609,6 +612,44 @@ SchemaDocumentArray.setters = [];
 
 SchemaDocumentArray.get = SchemaType.get;
 
+/*!
+ * Handle casting $elemMatch operators
+ */
+
+SchemaDocumentArray.prototype.$conditionalHandlers.$elemMatch = cast$elemMatch;
+
+function cast$elemMatch(val, context) {
+  const keys = Object.keys(val);
+  const numKeys = keys.length;
+  for (let i = 0; i < numKeys; ++i) {
+    const key = keys[i];
+    const value = val[key];
+    if (isOperator(key) && value != null) {
+      val[key] = this.castForQuery(key, value, context);
+    }
+  }
+
+  // Is this an embedded discriminator and is the discriminator key set?
+  // If so, use the discriminator schema. See gh-7449
+  const discriminatorKey = this &&
+    this.casterConstructor &&
+    this.casterConstructor.schema &&
+    this.casterConstructor.schema.options &&
+    this.casterConstructor.schema.options.discriminatorKey;
+  const discriminators = this &&
+  this.casterConstructor &&
+  this.casterConstructor.schema &&
+  this.casterConstructor.schema.discriminators || {};
+  if (discriminatorKey != null &&
+      val[discriminatorKey] != null &&
+      discriminators[val[discriminatorKey]] != null) {
+    return cast(discriminators[val[discriminatorKey]], val, null, this && this.$$context);
+  }
+
+  const schema = this.casterConstructor.schema ?? context.schema;
+  return cast(schema, val, null, this && this.$$context);
+}
+
 /*!
  * Module exports.
  */

diff --git a/test/document.test.js b/test/document.test.js
@@ -2555,6 +2555,88 @@ describe('document', function() {
       // Does not throw
       await Model.create({ name: 'test' });
     });
+
+    it('fully validates modified subdocs (gh-14677)', async function() {
+      const embedSchema = new mongoose.Schema({
+        field1: {
+          type: String,
+          required: true
+        },
+        field2: String
+      });
+      const testSchema = new mongoose.Schema({
+        testField: {
+          type: String,
+          required: true
+        },
+        testArray: [embedSchema]
+      });
+      const TestModel = db.model('Test', testSchema);
+
+      let doc = new TestModel({ testArray: [{ field2: 'test' }] });
+      let err = await doc.validate({ validateModifiedOnly: true }).then(() => null, err => err);
+      assert.ok(err);
+      assert.ok(err.errors['testArray.0.field1']);
+      assert.equal(err.errors['testArray.0.field1'].kind, 'required');
+
+      await TestModel.collection.insertOne(doc.toObject());
+      doc = await TestModel.findById(doc._id).orFail();
+      doc.testArray[0].field2 = 'test modified';
+      err = await doc.validate({ validateModifiedOnly: true }).then(() => null, err => err);
+      assert.ifError(err);
+
+      err = await doc.validate().then(() => null, err => err);
+      assert.ok(err);
+      assert.ok(err.errors['testArray.0.field1']);
+      assert.equal(err.errors['testArray.0.field1'].kind, 'required');
+
+      doc.testArray[0] = { field2: 'test modified 3' };
+      err = await doc.validate({ validateModifiedOnly: true }).then(() => null, err => err);
+      assert.ok(err);
+      assert.ok(err.errors['testArray.0.field1']);
+      assert.equal(err.errors['testArray.0.field1'].kind, 'required');
+    });
+
+    it('fully validates modified single nested subdocs (gh-14677)', async function() {
+      const embedSchema = new mongoose.Schema({
+        field1: {
+          type: String,
+          required: true
+        },
+        field2: String
+      });
+      const testSchema = new mongoose.Schema({
+        testField: {
+          type: String,
+          required: true
+        },
+        subdoc: embedSchema
+      });
+      const TestModel = db.model('Test', testSchema);
+
+      let doc = new TestModel({ subdoc: { field2: 'test' } });
+      let err = await doc.validate({ validateModifiedOnly: true }).then(() => null, err => err);
+      assert.ok(err);
+      assert.ok(err.errors['subdoc.field1']);
+      assert.equal(err.errors['subdoc.field1'].kind, 'required');
+
+      await TestModel.collection.insertOne(doc.toObject());
+      doc = await TestModel.findById(doc._id).orFail();
+      doc.subdoc.field2 = 'test modified';
+      err = await doc.validate({ validateModifiedOnly: true }).then(() => null, err => err);
+      assert.ifError(err);
+
+      err = await doc.validate().then(() => null, err => err);
+      assert.ok(err);
+      assert.ok(err.errors['subdoc.field1']);
+      assert.equal(err.errors['subdoc.field1'].kind, 'required');
+
+      doc.subdoc = { field2: 'test modified 3' };
+      err = await doc.validate({ validateModifiedOnly: true }).then(() => null, err => err);
+      assert.ok(err);
+      assert.ok(err.errors['subdoc.field1']);
+      assert.equal(err.errors['subdoc.field1'].kind, 'required');
+    });
   });
 
   describe('bug fixes', function() {

diff --git a/test/helpers/projection.applyProjection.test.js b/test/helpers/projection.applyProjection.test.js
@@ -21,4 +21,15 @@ describe('applyProjection', function() {
     assert.deepEqual(applyProjection(obj, { 'nested.str2': 0 }), { str: 'test', nested: { num3: 42 } });
     assert.deepEqual(applyProjection(obj, { nested: { num3: 0 } }), { str: 'test', nested: { str2: 'test2' } });
   });
+
+  it('handles projections underneath arrays (gh-14680)', function() {
+    const obj = {
+      _id: 12,
+      testField: 'foo',
+      testArray: [{ _id: 42, field1: 'bar' }]
+    };
+
+    assert.deepEqual(applyProjection(obj, { 'testArray.field1': 1 }), { testArray: [{ field1: 'bar' }] });
+    assert.deepEqual(applyProjection(obj, { 'testArray.field1': 0, _id: 0 }), { testField: 'foo', testArray: [{ _id: 42 }] });
+  });
 });
diff --git a/test/model.query.casting.test.js b/test/model.query.casting.test.js
@@ -453,7 +453,10 @@ describe('model query casting', function() {
       const id = post._id.toString();
 
       await post.save();
-      const doc = await BlogPostB.findOne({ _id: id, comments: { $not: { $elemMatch: { _id: commentId.toString() } } } });
+      const doc = await BlogPostB.findOne({
+        _id: id,
+        comments: { $not: { $elemMatch: { _id: commentId.toString() } } }
+      });
       assert.equal(doc, null);
     });
 

diff --git a/test/model.test.js b/test/model.test.js
@@ -4470,6 +4470,43 @@ describe('Model', function() {
         assert.equal(err.validationErrors[0].path, 'age');
         assert.equal(err.results[0].path, 'age');
       });
+
+      it('casts $elemMatch filter (gh-14678)', async function() {
+        const schema = new mongoose.Schema({
+          name: String,
+          ids: [String]
+        });
+        const TestModel = db.model('Test', schema);
+
+        const { _id } = await TestModel.create({ ids: ['1'] });
+        await TestModel.bulkWrite([
+          {
+            updateOne: {
+              filter: {
+                ids: {
+                  $elemMatch: {
+                    $in: [1]
+                  }
+                }
+              },
+              update: {
+                $set: {
+                  name: 'test'
+                },
+                $addToSet: {
+                  ids: {
+                    $each: [1, '2', 3]
+                  }
+                }
+              }
+            }
+          }
+        ]);
+
+        const doc = await TestModel.findById(_id).orFail();
+        assert.strictEqual(doc.name, 'test');
+        assert.deepStrictEqual(doc.ids, ['1', '2', '3']);
+      });
     });
 
     it('deleteOne with cast error (gh-5323)', async function() {

diff --git a/test/types/connection.test.ts b/test/types/connection.test.ts
@@ -54,6 +54,11 @@ expectType<Promise<string>>(conn.transaction(async(res) => {
   return 'a';
 }, { readConcern: 'majority' }));
 
+expectType<Promise<string>>(conn.withSession(async(res) => {
+  expectType<mongodb.ClientSession>(res);
+  return 'a';
+}));
+
 expectError(conn.user = 'invalid');
 expectError(conn.pass = 'invalid');
 expectError(conn.host = 'invalid');

diff --git a/types/connection.d.ts b/types/connection.d.ts
@@ -247,7 +247,7 @@ declare module 'mongoose' {
     /** Watches the entire underlying database for changes. Similar to [`Model.watch()`](/docs/api/model.html#model_Model-watch). */
     watch<ResultType extends mongodb.Document = any>(pipeline?: Array<any>, options?: mongodb.ChangeStreamOptions): mongodb.ChangeStream<ResultType>;
 
-    withSession<T = any>(executor: (session: ClientSession) => Promise<T>): T;
+    withSession<T = any>(executor: (session: ClientSession) => Promise<T>): Promise<T>;
   }
 
 }