From d3756032e605782d3143cacd99b8a9f4566322e6 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 9 Jul 2024 10:31:27 -0400 Subject: [PATCH 01/24] types: avoid automatically inferring lean result type when assigning to explicitly typed variable Fix #14697 --- test/types/document.test.ts | 5 +++-- test/types/plugin.test.ts | 2 +- test/types/queries.test.ts | 4 ++-- types/query.d.ts | 18 ++++++++++++++---- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/test/types/document.test.ts b/test/types/document.test.ts index d492cebd6e7..84451edf0f2 100644 --- a/test/types/document.test.ts +++ b/test/types/document.test.ts @@ -10,7 +10,7 @@ import { DefaultSchemaOptions } from 'mongoose'; import { DeleteResult } from 'mongodb'; -import { expectAssignable, expectError, expectType } from 'tsd'; +import { expectAssignable, expectError, expectNotAssignable, expectType } from 'tsd'; import { autoTypedModel } from './models.test'; import { autoTypedModelConnection } from './connection.test'; import { AutoTypedSchemaType } from './schema.test'; @@ -42,7 +42,8 @@ void async function main() { expectType(await doc.deleteOne()); expectType(await doc.deleteOne().findOne()); - expectType<{ _id: Types.ObjectId, name?: string } | null>(await doc.deleteOne().findOne().lean()); + expectAssignable<{ _id: Types.ObjectId, name?: string } | null>(await doc.deleteOne().findOne().lean()); + expectNotAssignable(await doc.deleteOne().findOne().lean()); }(); diff --git a/test/types/plugin.test.ts b/test/types/plugin.test.ts index 0b579ccc6b9..fc1deddf900 100644 --- a/test/types/plugin.test.ts +++ b/test/types/plugin.test.ts @@ -49,7 +49,7 @@ interface TestStaticMethods { findSomething(this: TestModel): Promise; } type TestDocument = HydratedDocument; -type TestQuery = Query & TestQueryHelpers; +type TestQuery = Query & TestQueryHelpers; interface TestQueryHelpers { whereSomething(this: TestQuery): this } diff --git a/test/types/queries.test.ts b/test/types/queries.test.ts index f10f26b3ad2..d84022526e4 100644 --- a/test/types/queries.test.ts +++ b/test/types/queries.test.ts @@ -38,11 +38,11 @@ const schema: Schema, {}, QueryHelpers> = new endDate: Date }); -schema.query._byName = function(name: string): QueryWithHelpers { +schema.query._byName = function(name: string): QueryWithHelpers { return this.find({ name }); }; -schema.query.byName = function(name: string): QueryWithHelpers { +schema.query.byName = function(name: string): QueryWithHelpers { expectError(this.notAQueryHelper()); return this._byName(name); }; diff --git a/types/query.d.ts b/types/query.d.ts index 44d9a71062f..850d06eb25a 100644 --- a/types/query.d.ts +++ b/types/query.d.ts @@ -224,7 +224,7 @@ declare module 'mongoose' { : MergeType : MergeType; - class Query> implements SessionOperation { + class Query> implements SessionOperation { _mongooseOptions: MongooseQueryOptions; /** @@ -548,9 +548,19 @@ declare module 'mongoose' { j(val: boolean | null): this; /** Sets the lean option. */ - lean< - LeanResultType = GetLeanResultType - >( + lean( + val?: boolean | any + ): QueryWithHelpers< + ResultType extends null + ? GetLeanResultType | null + : GetLeanResultType, + DocType, + THelpers, + RawDocType, + QueryOp, + TInstanceMethods + >; + lean( val?: boolean | any ): QueryWithHelpers< ResultType extends null From 779bfe5996562de436690de823f795d8688dd832 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 9 Jul 2024 13:27:13 -0400 Subject: [PATCH 02/24] types: make `_id` required on Document type Fix #14660 --- test/types/docArray.test.ts | 2 +- test/types/schema.test.ts | 2 +- types/document.d.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/types/docArray.test.ts b/test/types/docArray.test.ts index c296ce6fea7..78650845ba2 100644 --- a/test/types/docArray.test.ts +++ b/test/types/docArray.test.ts @@ -91,7 +91,7 @@ async function gh13424() { const TestModel = model('Test', testSchema); const doc = new TestModel(); - expectType(doc.subDocArray[0]._id); + expectType(doc.subDocArray[0]._id); } async function gh14367() { diff --git a/test/types/schema.test.ts b/test/types/schema.test.ts index 9dab60d2022..49f0f277a4c 100644 --- a/test/types/schema.test.ts +++ b/test/types/schema.test.ts @@ -1386,7 +1386,7 @@ function gh13424() { const TestModel = model('TestModel', new Schema(testSchema)); const doc = new TestModel({}); - expectType(doc.subDocArray[0]._id); + expectType(doc.subDocArray[0]._id); } function gh14147() { diff --git a/types/document.d.ts b/types/document.d.ts index c0fb5589240..93d1718fea9 100644 --- a/types/document.d.ts +++ b/types/document.d.ts @@ -22,7 +22,7 @@ declare module 'mongoose' { constructor(doc?: any); /** This documents _id. */ - _id?: T; + _id: T; /** This documents __v. */ __v?: any; From 22b41df59fdee5b6d93ac9b46fb619c77f9b5230 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Fri, 12 Jul 2024 14:19:57 -0400 Subject: [PATCH 03/24] feat(QueryCursor): add getDriverCursor() function that returns the raw driver cursor --- lib/cursor/queryCursor.js | 20 ++++++++++++++++++++ test/query.cursor.test.js | 19 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/lib/cursor/queryCursor.js b/lib/cursor/queryCursor.js index ff59f6aeba8..2c908de50d0 100644 --- a/lib/cursor/queryCursor.js +++ b/lib/cursor/queryCursor.js @@ -10,6 +10,7 @@ const eachAsync = require('../helpers/cursor/eachAsync'); const helpers = require('../queryHelpers'); const kareem = require('kareem'); const immediate = require('../helpers/immediate'); +const { once } = require('node:events'); const util = require('util'); /** @@ -135,6 +136,25 @@ QueryCursor.prototype._read = function() { }); }; +/** + * Returns the underlying cursor from the MongoDB Node driver that this cursor uses. + * + * @method getDriverCursor + * @memberOf QueryCursor + * @returns {Cursor} MongoDB Node driver cursor instance + * @instance + * @api public + */ + +QueryCursor.prototype.getDriverCursor = async function getDriverCursor() { + if (this.cursor) { + return this.cursor; + } + + await once(this, 'cursor'); + return this.cursor; +}; + /** * Registers a transform function which subsequently maps documents retrieved * via the streams interface or `.next()` diff --git a/test/query.cursor.test.js b/test/query.cursor.test.js index d835135a0b2..a5f7afc2027 100644 --- a/test/query.cursor.test.js +++ b/test/query.cursor.test.js @@ -900,6 +900,25 @@ describe('QueryCursor', function() { assert.ok(err); assert.ok(err.message.includes('skipMiddlewareFunction'), err.message); }); + + it('returns the underlying Node driver cursor with getDriverCursor()', async function() { + const schema = new mongoose.Schema({ name: String }); + + const Movie = db.model('Movie', schema); + + await Movie.deleteMany({}); + await Movie.create([ + { name: 'Kickboxer' }, + { name: 'Ip Man' }, + { name: 'Enter the Dragon' } + ]); + + const cursor = await Movie.find({}).cursor(); + assert.ok(!cursor.cursor); + const driverCursor = await cursor.getDriverCursor(); + assert.ok(cursor.cursor); + assert.equal(driverCursor, cursor.cursor); + }); }); async function delay(ms) { From 330dd360e86cea2ac5a2ae773a0b457e22d7f472 Mon Sep 17 00:00:00 2001 From: Alex Coleman <77301670+alex-statsig@users.noreply.github.com> Date: Fri, 26 Jul 2024 15:47:33 -0700 Subject: [PATCH 04/24] Refine query selector to highlight unexpected query keys Previously, there was a catch-all case in RootQuerySelector which would accept any key with type "any". This still allows nice IDE typehints, but can lead to bugs where the incorrect key is used for a filter with no warning that the key is clearly wrong. With this PR, only keys that contain a "." are unsafely typed as any, which should reduce the blast radius of the hacky types dramatically. --- types/query.d.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/types/query.d.ts b/types/query.d.ts index 44d9a71062f..5784987882d 100644 --- a/types/query.d.ts +++ b/types/query.d.ts @@ -115,8 +115,9 @@ declare module 'mongoose' { /** @see https://www.mongodb.com/docs/manual/reference/operator/query/comment/#op._S_comment */ $comment?: string; // we could not find a proper TypeScript generic to support nested queries e.g. 'user.friends.name' - // this will mark all unrecognized properties as any (including nested queries) - [key: string]: any; + // this will mark all unrecognized properties as any (including nested queries) only if + // they include a "." (to avoid generically allowing any unexpected keys) + [nestedSelector: `${string}.${string}`]: any; }; interface QueryTimestampsConfig { From 641250cea3f59c38f51b90355f35b96e21e4fbe8 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 1 Aug 2024 11:06:22 -0400 Subject: [PATCH 05/24] feat(model+query): support `options` parameter for distinct() Fix #8006 --- lib/model.js | 8 ++++++-- lib/query.js | 11 +++++++++-- test/docs/transactions.test.js | 20 ++++++++++++++++++++ test/query.test.js | 10 ++++++++++ test/types/queries.test.ts | 1 + types/models.d.ts | 3 ++- 6 files changed, 48 insertions(+), 5 deletions(-) diff --git a/lib/model.js b/lib/model.js index b3497669be5..7b9223d2408 100644 --- a/lib/model.js +++ b/lib/model.js @@ -2115,17 +2115,21 @@ Model.countDocuments = function countDocuments(conditions, options) { * * @param {String} field * @param {Object} [conditions] optional + * @param {Object} [options] optional * @return {Query} * @api public */ -Model.distinct = function distinct(field, conditions) { +Model.distinct = function distinct(field, conditions, options) { _checkContext(this, 'distinct'); - if (typeof arguments[0] === 'function' || typeof arguments[1] === 'function') { + if (typeof arguments[0] === 'function' || typeof arguments[1] === 'function' || typeof arguments[2] === 'function') { throw new MongooseError('Model.distinct() no longer accepts a callback'); } const mq = new this.Query({}, {}, this, this.$__collection); + if (options != null) { + mq.setOptions(options); + } return mq.distinct(field, conditions); }; diff --git a/lib/query.js b/lib/query.js index ccca65f4192..20532ad1f8f 100644 --- a/lib/query.js +++ b/lib/query.js @@ -2870,21 +2870,24 @@ Query.prototype.__distinct = async function __distinct() { * * #### Example: * + * distinct(field, conditions, options) * distinct(field, conditions) * distinct(field) * distinct() * * @param {String} [field] * @param {Object|Query} [filter] + * @param {Object} [options] * @return {Query} this * @see distinct https://www.mongodb.com/docs/manual/reference/method/db.collection.distinct/ * @api public */ -Query.prototype.distinct = function(field, conditions) { +Query.prototype.distinct = function(field, conditions, options) { if (typeof field === 'function' || typeof conditions === 'function' || - typeof arguments[2] === 'function') { + typeof options === 'function' || + typeof arguments[3] === 'function') { throw new MongooseError('Query.prototype.distinct() no longer accepts a callback'); } @@ -2903,6 +2906,10 @@ Query.prototype.distinct = function(field, conditions) { this._distinct = field; } + if (typeof options === 'object' && options != null) { + this.setOptions(options); + } + return this; }; diff --git a/test/docs/transactions.test.js b/test/docs/transactions.test.js index 2b19aef9ee3..b2e208fe17d 100644 --- a/test/docs/transactions.test.js +++ b/test/docs/transactions.test.js @@ -338,6 +338,26 @@ describe('transactions', function() { assert.deepEqual(fromDb, { name: 'Tyrion Lannister' }); }); + it('distinct (gh-8006)', async function() { + const Character = db.model('gh8006_Character', new Schema({ name: String, rank: String }, { versionKey: false })); + + + const session = await db.startSession(); + + session.startTransaction(); + await Character.create([{ name: 'Will Riker', rank: 'Commander' }, { name: 'Jean-Luc Picard', rank: 'Captain' }], { session }); + + let names = await Character.distinct('name', {}, { session }); + assert.deepStrictEqual(names.sort(), ['Jean-Luc Picard', 'Will Riker']); + + names = await Character.distinct('name', { rank: 'Captain' }, { session }); + assert.deepStrictEqual(names.sort(), ['Jean-Luc Picard']); + + // Undo both update and delete since doc should pull from `$session()` + await session.abortTransaction(); + session.endSession(); + }); + it('save() with no changes (gh-8571)', async function() { db.deleteModel(/Test/); const Test = db.model('Test', Schema({ name: String })); diff --git a/test/query.test.js b/test/query.test.js index 553ab04e152..4f75d0f2fab 100644 --- a/test/query.test.js +++ b/test/query.test.js @@ -1088,6 +1088,16 @@ describe('Query', function() { assert.equal(q.op, 'distinct'); }); + + it('using options parameter for distinct', function() { + const q = new Query({}); + const options = { collation: { locale: 'en', strength: 2 } }; + + q.distinct('blah', {}, options); + + assert.equal(q.op, 'distinct'); + assert.deepEqual(q.options.collation, options.collation); + }); }); describe('findOne', function() { diff --git a/test/types/queries.test.ts b/test/types/queries.test.ts index d84022526e4..5396f384cad 100644 --- a/test/types/queries.test.ts +++ b/test/types/queries.test.ts @@ -109,6 +109,7 @@ Test.find({ name: { $gte: 'Test' } }, null, { collation: { locale: 'en-us' } }). Test.findOne().orFail(new Error('bar')).then((doc: ITest | null) => console.log('Found! ' + doc)); Test.distinct('name').exec().then((res: Array) => console.log(res[0])); +Test.distinct('name', {}, { collation: { locale: 'en', strength: 2 } }).exec().then((res: Array) => console.log(res[0])); Test.findOneAndUpdate({ name: 'test' }, { name: 'test2' }).exec().then((res: ITest | null) => console.log(res)); Test.findOneAndUpdate({ name: 'test' }, { name: 'test2' }).then((res: ITest | null) => console.log(res)); diff --git a/types/models.d.ts b/types/models.d.ts index 27c43612ac2..c042305a828 100644 --- a/types/models.d.ts +++ b/types/models.d.ts @@ -621,7 +621,8 @@ declare module 'mongoose' { /** Creates a `distinct` query: returns the distinct values of the given `field` that match `filter`. */ distinct( field: DocKey, - filter?: FilterQuery + filter?: FilterQuery, + options?: QueryOptions ): QueryWithHelpers< Array< DocKey extends keyof WithLevel1NestedPaths From ea2a4ef7b20f73217b82dd21068a2f85bc9b197d Mon Sep 17 00:00:00 2001 From: alex-statsig Date: Sat, 3 Aug 2024 11:42:33 -0700 Subject: [PATCH 06/24] Add separate RootFilterQuery to type some cases better, add test cases --- test/types/models.test.ts | 27 +++++++++++++++ types/models.d.ts | 72 +++++++++++++++++++-------------------- types/query.d.ts | 46 +++++++++++++------------ 3 files changed, 87 insertions(+), 58 deletions(-) diff --git a/test/types/models.test.ts b/test/types/models.test.ts index 218c4c90569..80bcce46e1f 100644 --- a/test/types/models.test.ts +++ b/test/types/models.test.ts @@ -216,6 +216,7 @@ function find() { Project.find({ name: 'Hello' }); // just callback + // @ts-expect-error: Callback to find is longer supported Project.find((error: CallbackError, result: IProject[]) => console.log(error, result)); // filter + projection @@ -977,3 +978,29 @@ function testWithLevel1NestedPaths() { 'foo.one': string | null | undefined }>({} as Test2); } + +function gh14764TestFilterQueryRestrictions() { + const TestModel = model<{ validKey: number }>('Test', new Schema({})); + // @ts-expect-error: A key not in the schema should be invalid + TestModel.find({ invalidKey: 0 }); + // @ts-expect-error: A key not in the schema should be invalid for simple root operators + TestModel.find({ $and: [{ invalidKey: 0 }] }); + + // Any "nested" keys should be valid + TestModel.find({ 'validKey.subkey': 0 }); + + // And deeply "nested" keys should be valid + TestModel.find({ 'validKey.deep.nested.key': 0 }); + TestModel.find({ validKey: { deep: { nested: { key: 0 } } } }); + + // Any Query should be accepted as the root argument (due to merge support) + TestModel.find(TestModel.find()); + // @ts-expect-error: A Query should not be a valid type for a FilterQuery within an op like $and + TestModel.find({ $and: [TestModel.find()] }); + + const id = new Types.ObjectId(); + // Any ObjectId should be accepted as the root argument + TestModel.find(id); + // @ts-expect-error: A ObjectId should not be a valid type for a FilterQuery within an op like $and + TestModel.find({ $and: [id] }); +} diff --git a/types/models.d.ts b/types/models.d.ts index 27c43612ac2..28678161f2f 100644 --- a/types/models.d.ts +++ b/types/models.d.ts @@ -188,7 +188,7 @@ declare module 'mongoose' { export interface ReplaceOneModel { /** The filter to limit the replaced document. */ - filter: FilterQuery; + filter: RootFilterQuery; /** The document with which to replace the matched document. */ replacement: mongodb.WithoutId; /** Specifies a collation. */ @@ -203,7 +203,7 @@ declare module 'mongoose' { export interface UpdateOneModel { /** The filter to limit the updated documents. */ - filter: FilterQuery; + filter: RootFilterQuery; /** A document or pipeline containing update operators. */ update: UpdateQuery; /** A set of filters specifying to which array elements an update should apply. */ @@ -220,7 +220,7 @@ declare module 'mongoose' { export interface UpdateManyModel { /** The filter to limit the updated documents. */ - filter: FilterQuery; + filter: RootFilterQuery; /** A document or pipeline containing update operators. */ update: UpdateQuery; /** A set of filters specifying to which array elements an update should apply. */ @@ -237,7 +237,7 @@ declare module 'mongoose' { export interface DeleteOneModel { /** The filter to limit the deleted documents. */ - filter: FilterQuery; + filter: RootFilterQuery; /** Specifies a collation. */ collation?: mongodb.CollationOptions; /** The index to use. If specified, then the query system will only consider plans using the hinted index. */ @@ -246,7 +246,7 @@ declare module 'mongoose' { export interface DeleteManyModel { /** The filter to limit the deleted documents. */ - filter: FilterQuery; + filter: RootFilterQuery; /** Specifies a collation. */ collation?: mongodb.CollationOptions; /** The index to use. If specified, then the query system will only consider plans using the hinted index. */ @@ -318,7 +318,7 @@ declare module 'mongoose' { /** Creates a `countDocuments` query: counts the number of documents that match `filter`. */ countDocuments( - filter?: FilterQuery, + filter?: RootFilterQuery, options?: (mongodb.CountOptions & MongooseBaseQueryOptions) | null ): QueryWithHelpers< number, @@ -357,7 +357,7 @@ declare module 'mongoose' { * regardless of the `single` option. */ deleteMany( - filter?: FilterQuery, + filter?: RootFilterQuery, options?: (mongodb.DeleteOptions & MongooseBaseQueryOptions) | null ): QueryWithHelpers< mongodb.DeleteResult, @@ -368,7 +368,7 @@ declare module 'mongoose' { TInstanceMethods >; deleteMany( - filter: FilterQuery + filter: RootFilterQuery ): QueryWithHelpers< mongodb.DeleteResult, THydratedDocumentType, @@ -384,7 +384,7 @@ declare module 'mongoose' { * `single` option. */ deleteOne( - filter?: FilterQuery, + filter?: RootFilterQuery, options?: (mongodb.DeleteOptions & MongooseBaseQueryOptions) | null ): QueryWithHelpers< mongodb.DeleteResult, @@ -395,7 +395,7 @@ declare module 'mongoose' { TInstanceMethods >; deleteOne( - filter: FilterQuery + filter: RootFilterQuery ): QueryWithHelpers< mongodb.DeleteResult, THydratedDocumentType, @@ -446,7 +446,7 @@ declare module 'mongoose' { /** Finds one document. */ findOne( - filter: FilterQuery, + filter: RootFilterQuery, projection: ProjectionType | null | undefined, options: QueryOptions & { lean: true } ): QueryWithHelpers< @@ -458,16 +458,16 @@ declare module 'mongoose' { TInstanceMethods >; findOne( - filter?: FilterQuery, + filter?: RootFilterQuery, projection?: ProjectionType | null, options?: QueryOptions | null ): QueryWithHelpers; findOne( - filter?: FilterQuery, + filter?: RootFilterQuery, projection?: ProjectionType | null ): QueryWithHelpers; findOne( - filter?: FilterQuery + filter?: RootFilterQuery ): QueryWithHelpers; /** @@ -621,7 +621,7 @@ declare module 'mongoose' { /** Creates a `distinct` query: returns the distinct values of the given `field` that match `filter`. */ distinct( field: DocKey, - filter?: FilterQuery + filter?: RootFilterQuery ): QueryWithHelpers< Array< DocKey extends keyof WithLevel1NestedPaths @@ -650,7 +650,7 @@ declare module 'mongoose' { * the given `filter`, and `null` otherwise. */ exists( - filter: FilterQuery + filter: RootFilterQuery ): QueryWithHelpers< { _id: InferId } | null, THydratedDocumentType, @@ -662,7 +662,7 @@ declare module 'mongoose' { /** Creates a `find` query: gets a list of documents that match `filter`. */ find( - filter: FilterQuery, + filter: RootFilterQuery, projection: ProjectionType | null | undefined, options: QueryOptions & { lean: true } ): QueryWithHelpers< @@ -674,16 +674,16 @@ declare module 'mongoose' { TInstanceMethods >; find( - filter: FilterQuery, + filter: RootFilterQuery, projection?: ProjectionType | null | undefined, options?: QueryOptions | null | undefined ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'find', TInstanceMethods>; find( - filter: FilterQuery, + filter: RootFilterQuery, projection?: ProjectionType | null | undefined ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'find', TInstanceMethods>; find( - filter: FilterQuery + filter: RootFilterQuery ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'find', TInstanceMethods>; find( ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'find', TInstanceMethods>; @@ -711,7 +711,7 @@ declare module 'mongoose' { /** Creates a `findOneAndUpdate` query, filtering by the given `_id`. */ findByIdAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { includeResultMetadata: true, lean: true } ): QueryWithHelpers< @@ -756,7 +756,7 @@ declare module 'mongoose' { /** Creates a `findOneAndDelete` query: atomically finds the given document, deletes it, and returns the document as it was before deletion. */ findOneAndDelete( - filter: FilterQuery, + filter: RootFilterQuery, options: QueryOptions & { lean: true } ): QueryWithHelpers< GetLeanResultType | null, @@ -767,17 +767,17 @@ declare module 'mongoose' { TInstanceMethods >; findOneAndDelete( - filter: FilterQuery, + filter: RootFilterQuery, options: QueryOptions & { includeResultMetadata: true } ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'findOneAndDelete', TInstanceMethods>; findOneAndDelete( - filter?: FilterQuery | null, + filter?: RootFilterQuery | null, options?: QueryOptions | null ): QueryWithHelpers; /** Creates a `findOneAndReplace` query: atomically finds the given document and replaces it with `replacement`. */ findOneAndReplace( - filter: FilterQuery, + filter: RootFilterQuery, replacement: TRawDocType | AnyObject, options: QueryOptions & { lean: true } ): QueryWithHelpers< @@ -789,24 +789,24 @@ declare module 'mongoose' { TInstanceMethods >; findOneAndReplace( - filter: FilterQuery, + filter: RootFilterQuery, replacement: TRawDocType | AnyObject, options: QueryOptions & { includeResultMetadata: true } ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'findOneAndReplace', TInstanceMethods>; findOneAndReplace( - filter: FilterQuery, + filter: RootFilterQuery, replacement: TRawDocType | AnyObject, options: QueryOptions & { upsert: true } & ReturnsNewDoc ): QueryWithHelpers; findOneAndReplace( - filter?: FilterQuery, + filter?: RootFilterQuery, replacement?: TRawDocType | AnyObject, options?: QueryOptions | null ): QueryWithHelpers; /** Creates a `findOneAndUpdate` query: atomically find the first document that matches `filter` and apply `update`. */ findOneAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { includeResultMetadata: true, lean: true } ): QueryWithHelpers< @@ -818,7 +818,7 @@ declare module 'mongoose' { TInstanceMethods >; findOneAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { lean: true } ): QueryWithHelpers< @@ -830,24 +830,24 @@ declare module 'mongoose' { TInstanceMethods >; findOneAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { includeResultMetadata: true } ): QueryWithHelpers, ResultDoc, TQueryHelpers, TRawDocType, 'findOneAndUpdate', TInstanceMethods>; findOneAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { upsert: true } & ReturnsNewDoc ): QueryWithHelpers; findOneAndUpdate( - filter?: FilterQuery, + filter?: RootFilterQuery, update?: UpdateQuery, options?: QueryOptions | null ): QueryWithHelpers; /** Creates a `replaceOne` query: finds the first document that matches `filter` and replaces it with `replacement`. */ replaceOne( - filter?: FilterQuery, + filter?: RootFilterQuery, replacement?: TRawDocType | AnyObject, options?: (mongodb.ReplaceOptions & MongooseQueryOptions) | null ): QueryWithHelpers; @@ -860,14 +860,14 @@ declare module 'mongoose' { /** Creates a `updateMany` query: updates all documents that match `filter` with `update`. */ updateMany( - filter?: FilterQuery, + filter?: RootFilterQuery, update?: UpdateQuery | UpdateWithAggregationPipeline, options?: (mongodb.UpdateOptions & MongooseUpdateQueryOptions) | null ): QueryWithHelpers; /** Creates a `updateOne` query: updates the first document that matches `filter` with `update`. */ updateOne( - filter?: FilterQuery, + filter?: RootFilterQuery, update?: UpdateQuery | UpdateWithAggregationPipeline, options?: (mongodb.UpdateOptions & MongooseUpdateQueryOptions) | null ): QueryWithHelpers; diff --git a/types/query.d.ts b/types/query.d.ts index 5784987882d..24eb15ebb6c 100644 --- a/types/query.d.ts +++ b/types/query.d.ts @@ -10,9 +10,11 @@ declare module 'mongoose' { * { age: { $gte: 30 } } * ``` */ - type FilterQuery = { + type RootFilterQuery = FilterQuery | Query | Types.ObjectId; + + type FilterQuery ={ [P in keyof T]?: Condition; - } & RootQuerySelector; + } & RootQuerySelector & { _id?: Condition; }; type MongooseBaseQueryOptionKeys = | 'context' @@ -304,7 +306,7 @@ declare module 'mongoose' { /** Specifies this query as a `countDocuments` query. */ countDocuments( - criteria?: FilterQuery, + criteria?: RootFilterQuery, options?: QueryOptions ): QueryWithHelpers; @@ -320,10 +322,10 @@ declare module 'mongoose' { * collection, regardless of the value of `single`. */ deleteMany( - filter?: FilterQuery, + filter?: RootFilterQuery, options?: QueryOptions ): QueryWithHelpers; - deleteMany(filter: FilterQuery): QueryWithHelpers< + deleteMany(filter: RootFilterQuery): QueryWithHelpers< any, DocType, THelpers, @@ -339,10 +341,10 @@ declare module 'mongoose' { * option. */ deleteOne( - filter?: FilterQuery, + filter?: RootFilterQuery, options?: QueryOptions ): QueryWithHelpers; - deleteOne(filter: FilterQuery): QueryWithHelpers< + deleteOne(filter: RootFilterQuery): QueryWithHelpers< any, DocType, THelpers, @@ -355,7 +357,7 @@ declare module 'mongoose' { /** Creates a `distinct` query: returns the distinct values of the given `field` that match `filter`. */ distinct( field: DocKey, - filter?: FilterQuery + filter?: RootFilterQuery ): QueryWithHelpers< Array< DocKey extends keyof WithLevel1NestedPaths @@ -407,52 +409,52 @@ declare module 'mongoose' { /** Creates a `find` query: gets a list of documents that match `filter`. */ find( - filter: FilterQuery, + filter: RootFilterQuery, projection?: ProjectionType | null, options?: QueryOptions | null ): QueryWithHelpers, DocType, THelpers, RawDocType, 'find', TInstanceMethods>; find( - filter: FilterQuery, + filter: RootFilterQuery, projection?: ProjectionType | null ): QueryWithHelpers, DocType, THelpers, RawDocType, 'find', TInstanceMethods>; find( - filter: FilterQuery + filter: RootFilterQuery ): QueryWithHelpers, DocType, THelpers, RawDocType, 'find', TInstanceMethods>; find(): QueryWithHelpers, DocType, THelpers, RawDocType, 'find', TInstanceMethods>; /** Declares the query a findOne operation. When executed, returns the first found document. */ findOne( - filter?: FilterQuery, + filter?: RootFilterQuery, projection?: ProjectionType | null, options?: QueryOptions | null ): QueryWithHelpers; findOne( - filter?: FilterQuery, + filter?: RootFilterQuery, projection?: ProjectionType | null ): QueryWithHelpers; findOne( - filter?: FilterQuery + filter?: RootFilterQuery ): QueryWithHelpers; /** Creates a `findOneAndDelete` query: atomically finds the given document, deletes it, and returns the document as it was before deletion. */ findOneAndDelete( - filter?: FilterQuery, + filter?: RootFilterQuery, options?: QueryOptions | null ): QueryWithHelpers; /** Creates a `findOneAndUpdate` query: atomically find the first document that matches `filter` and apply `update`. */ findOneAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { includeResultMetadata: true } ): QueryWithHelpers, DocType, THelpers, RawDocType, 'findOneAndUpdate', TInstanceMethods>; findOneAndUpdate( - filter: FilterQuery, + filter: RootFilterQuery, update: UpdateQuery, options: QueryOptions & { upsert: true } & ReturnsNewDoc ): QueryWithHelpers; findOneAndUpdate( - filter?: FilterQuery, + filter?: RootFilterQuery, update?: UpdateQuery, options?: QueryOptions | null ): QueryWithHelpers; @@ -593,7 +595,7 @@ declare module 'mongoose' { maxTimeMS(ms: number): this; /** Merges another Query or conditions object into this one. */ - merge(source: Query | FilterQuery): this; + merge(source: RootFilterQuery): this; /** Specifies a `$mod` condition, filters documents for documents whose `path` property is a number that is equal to `remainder` modulo `divisor`. */ mod(path: K, val: number): this; @@ -712,7 +714,7 @@ declare module 'mongoose' { * not accept any [atomic](https://www.mongodb.com/docs/manual/tutorial/model-data-for-atomic-operations/#pattern) operators (`$set`, etc.) */ replaceOne( - filter?: FilterQuery, + filter?: RootFilterQuery, replacement?: DocType | AnyObject, options?: QueryOptions | null ): QueryWithHelpers; @@ -820,7 +822,7 @@ declare module 'mongoose' { * the `multi` option. */ updateMany( - filter?: FilterQuery, + filter?: RootFilterQuery, update?: UpdateQuery | UpdateWithAggregationPipeline, options?: QueryOptions | null ): QueryWithHelpers; @@ -830,7 +832,7 @@ declare module 'mongoose' { * `update()`, except it does not support the `multi` or `overwrite` options. */ updateOne( - filter?: FilterQuery, + filter?: RootFilterQuery, update?: UpdateQuery | UpdateWithAggregationPipeline, options?: QueryOptions | null ): QueryWithHelpers; From b810a9d0970a39086f8a2d4af323d2d219c82739 Mon Sep 17 00:00:00 2001 From: alex-statsig Date: Mon, 5 Aug 2024 10:54:44 -0700 Subject: [PATCH 07/24] Fix new typescript error assertions --- test/types/models.test.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/types/models.test.ts b/test/types/models.test.ts index 80bcce46e1f..1ae8ba073a3 100644 --- a/test/types/models.test.ts +++ b/test/types/models.test.ts @@ -981,10 +981,10 @@ function testWithLevel1NestedPaths() { function gh14764TestFilterQueryRestrictions() { const TestModel = model<{ validKey: number }>('Test', new Schema({})); - // @ts-expect-error: A key not in the schema should be invalid - TestModel.find({ invalidKey: 0 }); - // @ts-expect-error: A key not in the schema should be invalid for simple root operators - TestModel.find({ $and: [{ invalidKey: 0 }] }); + // A key not in the schema should be invalid + expectError(TestModel.find({ invalidKey: 0 })); + // A key not in the schema should be invalid for simple root operators + expectError(TestModel.find({ $and: [{ invalidKey: 0 }] })); // Any "nested" keys should be valid TestModel.find({ 'validKey.subkey': 0 }); @@ -995,12 +995,12 @@ function gh14764TestFilterQueryRestrictions() { // Any Query should be accepted as the root argument (due to merge support) TestModel.find(TestModel.find()); - // @ts-expect-error: A Query should not be a valid type for a FilterQuery within an op like $and - TestModel.find({ $and: [TestModel.find()] }); + // A Query should not be a valid type for a FilterQuery within an op like $and + expectError(TestModel.find({ $and: [TestModel.find()] })); const id = new Types.ObjectId(); // Any ObjectId should be accepted as the root argument TestModel.find(id); - // @ts-expect-error: A ObjectId should not be a valid type for a FilterQuery within an op like $and - TestModel.find({ $and: [id] }); + // A ObjectId should not be a valid type for a FilterQuery within an op like $and + expectError(TestModel.find({ $and: [id] })); } From baf3ca20f4076c35afc3d435c52bda457751ae22 Mon Sep 17 00:00:00 2001 From: alex-statsig Date: Mon, 5 Aug 2024 13:11:38 -0700 Subject: [PATCH 08/24] Fix one more ts-expect-error to use expectError --- test/types/models.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/types/models.test.ts b/test/types/models.test.ts index 1ae8ba073a3..fbe411225f0 100644 --- a/test/types/models.test.ts +++ b/test/types/models.test.ts @@ -215,9 +215,8 @@ function find() { Project.find({}); Project.find({ name: 'Hello' }); - // just callback - // @ts-expect-error: Callback to find is longer supported - Project.find((error: CallbackError, result: IProject[]) => console.log(error, result)); + // just callback; this is no longer supported on .find() + expectError(Project.find((error: CallbackError, result: IProject[]) => console.log(error, result))); // filter + projection Project.find({}, undefined); From d94d8d36e2670d139c5e7694afc0359ab7108071 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 7 Aug 2024 19:56:55 -0400 Subject: [PATCH 09/24] Update test/docs/transactions.test.js Co-authored-by: hasezoey --- test/docs/transactions.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/docs/transactions.test.js b/test/docs/transactions.test.js index b2e208fe17d..de2ecfc9952 100644 --- a/test/docs/transactions.test.js +++ b/test/docs/transactions.test.js @@ -341,7 +341,6 @@ describe('transactions', function() { it('distinct (gh-8006)', async function() { const Character = db.model('gh8006_Character', new Schema({ name: String, rank: String }, { versionKey: false })); - const session = await db.startSession(); session.startTransaction(); From f6be45a95bfb8fb4c192e519059de83f459f1a25 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 7 Aug 2024 20:05:24 -0400 Subject: [PATCH 10/24] types(query): add options param to distinct() re: #8006 --- types/query.d.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/types/query.d.ts b/types/query.d.ts index 850d06eb25a..66a0042d342 100644 --- a/types/query.d.ts +++ b/types/query.d.ts @@ -354,7 +354,8 @@ declare module 'mongoose' { /** Creates a `distinct` query: returns the distinct values of the given `field` that match `filter`. */ distinct( field: DocKey, - filter?: FilterQuery + filter?: FilterQuery, + options?: QueryOptions ): QueryWithHelpers< Array< DocKey extends keyof WithLevel1NestedPaths From 58f384dbf04cc9df23e19113094b836ffa6438c7 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 7 Aug 2024 20:05:53 -0400 Subject: [PATCH 11/24] fix(query): consistent handling for non-object options with countDocuments(), estimatedDocumentCount(), distinct() --- lib/query.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/query.js b/lib/query.js index 20532ad1f8f..fa5d2cd856e 100644 --- a/lib/query.js +++ b/lib/query.js @@ -2773,7 +2773,7 @@ Query.prototype.estimatedDocumentCount = function(options) { this.op = 'estimatedDocumentCount'; this._validateOp(); - if (typeof options === 'object' && options != null) { + if (options != null) { this.setOptions(options); } @@ -2832,7 +2832,7 @@ Query.prototype.countDocuments = function(conditions, options) { this.merge(conditions); } - if (typeof options === 'object' && options != null) { + if (options != null) { this.setOptions(options); } @@ -2906,7 +2906,7 @@ Query.prototype.distinct = function(field, conditions, options) { this._distinct = field; } - if (typeof options === 'object' && options != null) { + if (options != null) { this.setOptions(options); } From 8d6422b6631cd87aa3a2da9d3ec4313a88fd91f3 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sat, 10 Aug 2024 18:31:40 -0400 Subject: [PATCH 12/24] Update models.d.ts --- types/models.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/models.d.ts b/types/models.d.ts index fb847989e6e..4c2403fd51b 100644 --- a/types/models.d.ts +++ b/types/models.d.ts @@ -621,7 +621,7 @@ declare module 'mongoose' { /** Creates a `distinct` query: returns the distinct values of the given `field` that match `filter`. */ distinct( field: DocKey, - filter?: RootFilterQuery + filter?: RootFilterQuery, options?: QueryOptions ): QueryWithHelpers< Array< From e5c9b7b0d064dc9895291915bdb6a38c7074742f Mon Sep 17 00:00:00 2001 From: alex-statsig Date: Sun, 11 Aug 2024 17:09:03 -0700 Subject: [PATCH 13/24] Fix queryhelpers test --- test/types/queryhelpers.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/types/queryhelpers.test.ts b/test/types/queryhelpers.test.ts index c96aaffbaa2..34a55b6bd22 100644 --- a/test/types/queryhelpers.test.ts +++ b/test/types/queryhelpers.test.ts @@ -8,7 +8,7 @@ interface Project { type ProjectModelType = Model; // Query helpers should return `Query> & ProjectQueryHelpers` // to enable chaining. -type ProjectModelQuery = Query, ProjectQueryHelpers> & ProjectQueryHelpers; +type ProjectModelQuery = Query, ProjectQueryHelpers, any> & ProjectQueryHelpers; interface ProjectQueryHelpers { byName(this: ProjectModelQuery, name: string): ProjectModelQuery; } From b24696d51ccf6dc3e1ba786165bec9e1c994244c Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sat, 17 Aug 2024 14:58:12 -0400 Subject: [PATCH 14/24] feat: upgrade mongodb -> 6.8.0, handle throwing error on closed cursor in Mongoose --- lib/cursor/queryCursor.js | 5 +++++ package.json | 2 +- test/query.cursor.test.js | 3 ++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/cursor/queryCursor.js b/lib/cursor/queryCursor.js index 2c908de50d0..49359e40dfe 100644 --- a/lib/cursor/queryCursor.js +++ b/lib/cursor/queryCursor.js @@ -43,6 +43,7 @@ function QueryCursor(query) { this.cursor = null; this.skipped = false; this.query = query; + this._closeCalled = false; const model = query.model; this._mongooseOptions = {}; this._transforms = []; @@ -229,6 +230,7 @@ QueryCursor.prototype.close = async function close() { } try { await this.cursor.close(); + this._closeCalled = true; this.emit('close'); } catch (error) { this.listeners('error').length > 0 && this.emit('error', error); @@ -266,6 +268,9 @@ QueryCursor.prototype.next = async function next() { if (typeof arguments[0] === 'function') { throw new MongooseError('QueryCursor.prototype.next() no longer accepts a callback'); } + if (this._closeCalled) { + throw new MongooseError('Cannot call `next()` on a closed cursor'); + } return new Promise((resolve, reject) => { _next(this, function(error, doc) { if (error) { diff --git a/package.json b/package.json index 013c7325c21..9c9097adc18 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "dependencies": { "bson": "^6.7.0", "kareem": "2.6.3", - "mongodb": "6.7.0", + "mongodb": "6.8.0", "mpath": "0.9.0", "mquery": "5.0.0", "ms": "2.1.3", diff --git a/test/query.cursor.test.js b/test/query.cursor.test.js index a5f7afc2027..d80264c5f2d 100644 --- a/test/query.cursor.test.js +++ b/test/query.cursor.test.js @@ -415,7 +415,8 @@ describe('QueryCursor', function() { await cursor.next(); assert.ok(false); } catch (error) { - assert.equal(error.name, 'MongoCursorExhaustedError'); + assert.equal(error.name, 'MongooseError'); + assert.ok(error.message.includes('closed cursor'), error.message); } }); }); From 3bc8123bfab36d5ee528ce491362255d6c3b96a0 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sat, 17 Aug 2024 15:03:43 -0400 Subject: [PATCH 15/24] rename closeCalled -> closed for more consistent semantics --- lib/cursor/queryCursor.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/cursor/queryCursor.js b/lib/cursor/queryCursor.js index 49359e40dfe..6f00a316794 100644 --- a/lib/cursor/queryCursor.js +++ b/lib/cursor/queryCursor.js @@ -43,7 +43,7 @@ function QueryCursor(query) { this.cursor = null; this.skipped = false; this.query = query; - this._closeCalled = false; + this._closed = false; const model = query.model; this._mongooseOptions = {}; this._transforms = []; @@ -230,7 +230,7 @@ QueryCursor.prototype.close = async function close() { } try { await this.cursor.close(); - this._closeCalled = true; + this._closed = true; this.emit('close'); } catch (error) { this.listeners('error').length > 0 && this.emit('error', error); @@ -268,7 +268,7 @@ QueryCursor.prototype.next = async function next() { if (typeof arguments[0] === 'function') { throw new MongooseError('QueryCursor.prototype.next() no longer accepts a callback'); } - if (this._closeCalled) { + if (this._closed) { throw new MongooseError('Cannot call `next()` on a closed cursor'); } return new Promise((resolve, reject) => { From 7bc60e9faad760d031aef2a91d63de4d937905db Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 19 Aug 2024 10:51:43 -0400 Subject: [PATCH 16/24] types: make toObject() and toJSON() not generic by default to avoid type widening Fix #12883 --- test/types/schema.test.ts | 9 ++++++--- types/document.d.ts | 5 ++++- types/inferrawdoctype.d.ts | 5 ++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/test/types/schema.test.ts b/test/types/schema.test.ts index 2d946adc93c..448858750c2 100644 --- a/test/types/schema.test.ts +++ b/test/types/schema.test.ts @@ -22,6 +22,7 @@ import { model, ValidateOpts } from 'mongoose'; +import { IsPathRequired } from '../../types/inferschematype'; import { expectType, expectError, expectAssignable } from 'tsd'; import { ObtainDocumentPathType, ResolvePathType } from '../../types/inferschematype'; @@ -1502,16 +1503,18 @@ function gh13772() { const schemaDefinition = { name: String, docArr: [{ name: String }] - }; + } as const; const schema = new Schema(schemaDefinition); + + const TestModel = model('User', schema); type RawDocType = InferRawDocType; expectAssignable< - { name?: string | null, docArr?: Array<{ name?: string | null }> } + { name?: string | null, docArr?: Array<{ name?: string | null }> | null } >({} as RawDocType); - const TestModel = model('User', schema); const doc = new TestModel(); expectAssignable(doc.toObject()); + expectAssignable(doc.toJSON()); } function gh14696() { diff --git a/types/document.d.ts b/types/document.d.ts index c0fb5589240..6fb01c910c7 100644 --- a/types/document.d.ts +++ b/types/document.d.ts @@ -259,11 +259,14 @@ declare module 'mongoose' { set(value: string | Record): this; /** The return value of this method is used in calls to JSON.stringify(doc). */ + toJSON(options?: ToObjectOptions & { flattenMaps?: true }): FlattenMaps>; + toJSON(options: ToObjectOptions & { flattenMaps: false }): Require_id; toJSON>(options?: ToObjectOptions & { flattenMaps?: true }): FlattenMaps; toJSON>(options: ToObjectOptions & { flattenMaps: false }): T; /** Converts this document into a plain-old JavaScript object ([POJO](https://masteringjs.io/tutorials/fundamentals/pojo)). */ - toObject>(options?: ToObjectOptions): Require_id; + toObject(options?: ToObjectOptions): Require_id; + toObject(options?: ToObjectOptions): Require_id; /** Clears the modified state on the specified path. */ unmarkModified(path: T): void; diff --git a/types/inferrawdoctype.d.ts b/types/inferrawdoctype.d.ts index e2b1d52b6b9..5ef52e13251 100644 --- a/types/inferrawdoctype.d.ts +++ b/types/inferrawdoctype.d.ts @@ -1,4 +1,5 @@ import { + IsPathRequired, IsSchemaTypeFromBuiltinClass, RequiredPaths, OptionalPaths, @@ -14,7 +15,9 @@ declare module 'mongoose' { [ K in keyof (RequiredPaths & OptionalPaths) - ]: ObtainRawDocumentPathType; + ]: IsPathRequired extends true + ? ObtainRawDocumentPathType + : ObtainRawDocumentPathType | null; }, TSchemaOptions>; /** From 91ebc9b8acb4d490e5147f6e0e38655ca9878f1e Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 20 Aug 2024 16:18:35 -0400 Subject: [PATCH 17/24] fix(ChangeStream): rely on promises over events, make tests more durable --- lib/cursor/changeStream.js | 52 +++++++++++++++++++++++--------------- test/connection.test.js | 2 ++ test/model.test.js | 37 +++++++++++++++++++++------ test/model.watch.test.js | 5 ++-- 4 files changed, 65 insertions(+), 31 deletions(-) diff --git a/lib/cursor/changeStream.js b/lib/cursor/changeStream.js index ec5cac5705f..55cdecfcdc2 100644 --- a/lib/cursor/changeStream.js +++ b/lib/cursor/changeStream.js @@ -33,15 +33,18 @@ class ChangeStream extends EventEmitter { ); } - // This wrapper is necessary because of buffering. - changeStreamThunk((err, driverChangeStream) => { - if (err != null) { - this.emit('error', err); - return; - } + this.$driverChangeStreamPromise = new Promise((resolve, reject) => { + // This wrapper is necessary because of buffering. + changeStreamThunk((err, driverChangeStream) => { + if (err != null) { + this.emit('error', err); + return reject(err); + } - this.driverChangeStream = driverChangeStream; - this.emit('ready'); + this.driverChangeStream = driverChangeStream; + this.emit('ready'); + resolve(); + }); }); } @@ -53,20 +56,23 @@ class ChangeStream extends EventEmitter { this.bindedEvents = true; if (this.driverChangeStream == null) { - this.once('ready', () => { - this.driverChangeStream.on('close', () => { - this.closed = true; - }); + this.$driverChangeStreamPromise.then( + () => { + this.driverChangeStream.on('close', () => { + this.closed = true; + }); - driverChangeStreamEvents.forEach(ev => { - this.driverChangeStream.on(ev, data => { - if (data != null && data.fullDocument != null && this.options && this.options.hydrate) { - data.fullDocument = this.options.model.hydrate(data.fullDocument); - } - this.emit(ev, data); + driverChangeStreamEvents.forEach(ev => { + this.driverChangeStream.on(ev, data => { + if (data != null && data.fullDocument != null && this.options && this.options.hydrate) { + data.fullDocument = this.options.model.hydrate(data.fullDocument); + } + this.emit(ev, data); + }); }); - }); - }); + }, + () => {} // No need to register events if opening change stream failed + ); return; } @@ -142,8 +148,12 @@ class ChangeStream extends EventEmitter { this.closed = true; if (this.driverChangeStream) { return this.driverChangeStream.close(); + } else { + return this.$driverChangeStreamPromise.then( + () => this.driverChangeStream.close(), + () => {} // No need to close if opening the change stream failed + ); } - return Promise.resolve(); } } diff --git a/test/connection.test.js b/test/connection.test.js index d395be5511b..03f87b40f3d 100644 --- a/test/connection.test.js +++ b/test/connection.test.js @@ -1032,6 +1032,8 @@ describe('connections:', function() { await nextChange; assert.equal(changes.length, 1); assert.equal(changes[0].operationType, 'insert'); + + await changeStream.close(); await conn.close(); }); diff --git a/test/model.test.js b/test/model.test.js index b73757e4721..52d04a92b9a 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -7,6 +7,7 @@ const sinon = require('sinon'); const start = require('./common'); const assert = require('assert'); +const { once } = require('events'); const random = require('./util').random; const util = require('./util'); @@ -3560,14 +3561,21 @@ describe('Model', function() { it('fullDocument (gh-11936)', async function() { const MyModel = db.model('Test', new Schema({ name: String })); + const doc = await MyModel.create({ name: 'Ned Stark' }); const changeStream = await MyModel.watch([], { fullDocument: 'updateLookup', hydrate: true }); + await changeStream.$driverChangeStreamPromise; - const doc = await MyModel.create({ name: 'Ned Stark' }); - - const p = changeStream.next(); + const p = new Promise((resolve) => { + changeStream.once('change', change => { + resolve(change); + }); + }); + // Need to wait for resume token to be set after the event listener, + // otherwise change stream might not pick up the update. + await once(changeStream.driverChangeStream, 'resumeTokenChanged'); await MyModel.updateOne({ _id: doc._id }, { name: 'Tony Stark' }); const changeData = await p; @@ -3576,6 +3584,8 @@ describe('Model', function() { doc._id.toHexString()); assert.ok(changeData.fullDocument.$__); assert.equal(changeData.fullDocument.get('name'), 'Tony Stark'); + + await changeStream.close(); }); it('fullDocument with immediate watcher and hydrate (gh-14049)', async function() { @@ -3583,15 +3593,22 @@ describe('Model', function() { const doc = await MyModel.create({ name: 'Ned Stark' }); + let changeStream = null; const p = new Promise((resolve) => { - MyModel.watch([], { + changeStream = MyModel.watch([], { fullDocument: 'updateLookup', hydrate: true - }).on('change', change => { + }); + + changeStream.on('change', change => { resolve(change); }); }); + // Need to wait for cursor to be initialized and for resume token to + // be set, otherwise change stream might not pick up the update. + await changeStream.$driverChangeStreamPromise; + await once(changeStream.driverChangeStream, 'resumeTokenChanged'); await MyModel.updateOne({ _id: doc._id }, { name: 'Tony Stark' }); const changeData = await p; @@ -3600,6 +3617,8 @@ describe('Model', function() { doc._id.toHexString()); assert.ok(changeData.fullDocument.$__); assert.equal(changeData.fullDocument.get('name'), 'Tony Stark'); + + await changeStream.close(); }); it('respects discriminators (gh-11007)', async function() { @@ -3654,11 +3673,12 @@ describe('Model', function() { setTimeout(resolve, 500, false); }); - changeStream.close(); - await db; + const close = changeStream.close(); + await db.asPromise(); const readyCalled = await ready; assert.strictEqual(readyCalled, false); + await close; await db.close(); }); @@ -3675,10 +3695,11 @@ describe('Model', function() { await MyModel.create({ name: 'Hodor' }); - changeStream.close(); + const close = changeStream.close(); const closedData = await closed; assert.strictEqual(closedData, true); + await close; await db.close(); }); diff --git a/test/model.watch.test.js b/test/model.watch.test.js index 84d41dc5b14..612bb84a75f 100644 --- a/test/model.watch.test.js +++ b/test/model.watch.test.js @@ -37,12 +37,13 @@ describe('model: watch: ', function() { const changeData = await changed; assert.equal(changeData.operationType, 'insert'); assert.equal(changeData.fullDocument.name, 'Ned Stark'); + await changeStream.close(); }); it('watch() close() prevents buffered watch op from running (gh-7022)', async function() { const MyModel = db.model('Test', new Schema({})); const changeStream = MyModel.watch(); - const ready = new global.Promise(resolve => { + const ready = new Promise(resolve => { changeStream.once('data', () => { resolve(true); }); @@ -64,7 +65,7 @@ describe('model: watch: ', function() { await MyModel.init(); const changeStream = MyModel.watch(); - const closed = new global.Promise(resolve => { + const closed = new Promise(resolve => { changeStream.once('close', () => resolve(true)); }); From 56f4adbd118f7f8a5f1d76f13fd16551e39689da Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 20 Aug 2024 16:23:27 -0400 Subject: [PATCH 18/24] bump max # of instantiations for now to fix tests --- scripts/tsc-diagnostics-check.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/tsc-diagnostics-check.js b/scripts/tsc-diagnostics-check.js index 3e1f6c66282..c23da8c1f55 100644 --- a/scripts/tsc-diagnostics-check.js +++ b/scripts/tsc-diagnostics-check.js @@ -3,7 +3,7 @@ const fs = require('fs'); const stdin = fs.readFileSync(0).toString('utf8'); -const maxInstantiations = isNaN(process.argv[2]) ? 127500 : parseInt(process.argv[2], 10); +const maxInstantiations = isNaN(process.argv[2]) ? 135000 : parseInt(process.argv[2], 10); console.log(stdin); From a8bb7d6339062c6a90780876beb37156347adae2 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 20 Aug 2024 16:44:20 -0400 Subject: [PATCH 19/24] alternative approach to avoid change stream test failures --- lib/cursor/changeStream.js | 4 ++++ test/model.test.js | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/cursor/changeStream.js b/lib/cursor/changeStream.js index 55cdecfcdc2..dec8508d58f 100644 --- a/lib/cursor/changeStream.js +++ b/lib/cursor/changeStream.js @@ -86,6 +86,10 @@ class ChangeStream extends EventEmitter { if (data != null && data.fullDocument != null && this.options && this.options.hydrate) { data.fullDocument = this.options.model.hydrate(data.fullDocument); } + if (ev === 'error' && this.closed) { + // If we've already closed the stream, no need to emit further error events + return; + } this.emit(ev, data); }); }); diff --git a/test/model.test.js b/test/model.test.js index 52d04a92b9a..a84b874b669 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -3695,11 +3695,10 @@ describe('Model', function() { await MyModel.create({ name: 'Hodor' }); - const close = changeStream.close(); + changeStream.close(); const closedData = await closed; assert.strictEqual(closedData, true); - await close; await db.close(); }); From 1b5fecc64fa51750ec69c34d32f8c7f0b140f93c Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 21 Aug 2024 07:30:14 -0400 Subject: [PATCH 20/24] alternative fix for change stream tests re: #14177 --- lib/cursor/changeStream.js | 4 ---- test/model.test.js | 7 +++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/cursor/changeStream.js b/lib/cursor/changeStream.js index dec8508d58f..55cdecfcdc2 100644 --- a/lib/cursor/changeStream.js +++ b/lib/cursor/changeStream.js @@ -86,10 +86,6 @@ class ChangeStream extends EventEmitter { if (data != null && data.fullDocument != null && this.options && this.options.hydrate) { data.fullDocument = this.options.model.hydrate(data.fullDocument); } - if (ev === 'error' && this.closed) { - // If we've already closed the stream, no need to emit further error events - return; - } this.emit(ev, data); }); }); diff --git a/test/model.test.js b/test/model.test.js index a84b874b669..e03cd5bd13b 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -3680,6 +3680,9 @@ describe('Model', function() { await close; await db.close(); + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); }); it('watch() close() closes the stream (gh-7022)', async function() { @@ -3700,6 +3703,10 @@ describe('Model', function() { assert.strictEqual(closedData, true); await db.close(); + + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); }); it('bubbles up resumeTokenChanged events (gh-14349)', async function() { From e0d598423ea216605d3d29a11d451c6797864d68 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 21 Aug 2024 11:47:16 -0400 Subject: [PATCH 21/24] listen for error events on change stream to avoid change stream tests flaking --- test/model.watch.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/model.watch.test.js b/test/model.watch.test.js index 612bb84a75f..099101e764f 100644 --- a/test/model.watch.test.js +++ b/test/model.watch.test.js @@ -56,6 +56,9 @@ describe('model: watch: ', function() { assert.strictEqual(readyCalled, false); await close; + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); }); it('watch() close() closes the stream (gh-7022)', async function() { @@ -75,6 +78,9 @@ describe('model: watch: ', function() { const closedData = await closed; assert.strictEqual(closedData, true); + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); }); }); }); From b08c93e5789ba5a0c3d5b12728c6bc9af1143ce6 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 21 Aug 2024 11:57:43 -0400 Subject: [PATCH 22/24] shuffle around error handler re: ChangeStream is closed errors --- test/model.test.js | 8 ++++---- test/model.watch.test.js | 13 +++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/test/model.test.js b/test/model.test.js index e03cd5bd13b..d2060fd9d00 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -3678,11 +3678,11 @@ describe('Model', function() { const readyCalled = await ready; assert.strictEqual(readyCalled, false); - await close; - await db.close(); // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream // may still poll after close. changeStream.on('error', () => {}); + await close; + await db.close(); }); it('watch() close() closes the stream (gh-7022)', async function() { @@ -3702,11 +3702,11 @@ describe('Model', function() { const closedData = await closed; assert.strictEqual(closedData, true); - await db.close(); - // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream // may still poll after close. changeStream.on('error', () => {}); + + await db.close(); }); it('bubbles up resumeTokenChanged events (gh-14349)', async function() { diff --git a/test/model.watch.test.js b/test/model.watch.test.js index 099101e764f..3e2ad733a24 100644 --- a/test/model.watch.test.js +++ b/test/model.watch.test.js @@ -50,15 +50,15 @@ describe('model: watch: ', function() { setTimeout(resolve, 500, false); }); + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); const close = changeStream.close(); await db.asPromise(); const readyCalled = await ready; assert.strictEqual(readyCalled, false); await close; - // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream - // may still poll after close. - changeStream.on('error', () => {}); }); it('watch() close() closes the stream (gh-7022)', async function() { @@ -74,13 +74,14 @@ describe('model: watch: ', function() { await MyModel.create({ name: 'Hodor' }); + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); + await changeStream.close(); const closedData = await closed; assert.strictEqual(closedData, true); - // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream - // may still poll after close. - changeStream.on('error', () => {}); }); }); }); From 8c6bb8b2ce3e4522d758a8fd99f715d164fb864e Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 21 Aug 2024 13:12:22 -0400 Subject: [PATCH 23/24] shuffle around some more error handlers --- test/model.test.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/test/model.test.js b/test/model.test.js index d2060fd9d00..9524e2cdbe7 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -3673,14 +3673,15 @@ describe('Model', function() { setTimeout(resolve, 500, false); }); + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); + const close = changeStream.close(); await db.asPromise(); const readyCalled = await ready; assert.strictEqual(readyCalled, false); - // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream - // may still poll after close. - changeStream.on('error', () => {}); await close; await db.close(); }); @@ -3698,14 +3699,14 @@ describe('Model', function() { await MyModel.create({ name: 'Hodor' }); - changeStream.close(); - const closedData = await closed; - assert.strictEqual(closedData, true); - // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream // may still poll after close. changeStream.on('error', () => {}); + changeStream.close(); + const closedData = await closed; + assert.strictEqual(closedData, true); + await db.close(); }); From a725a7565c03935d6788284dca016f37718b51dc Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 21 Aug 2024 13:21:26 -0400 Subject: [PATCH 24/24] add some more error handling to hopefully prevent flaking change stream tests --- test/model.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/model.test.js b/test/model.test.js index 9524e2cdbe7..f2d88586e63 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -3509,6 +3509,9 @@ describe('Model', function() { } changeStream.removeListener('change', listener); listener = null; + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); changeStream.close(); changeStream = null; }); @@ -3658,6 +3661,9 @@ describe('Model', function() { assert.equal(changeData.operationType, 'insert'); assert.equal(changeData.fullDocument.name, 'Ned Stark'); + // Change stream may still emit "MongoAPIError: ChangeStream is closed" because change stream + // may still poll after close. + changeStream.on('error', () => {}); await changeStream.close(); await db.close(); });