From 48d6da99b7990b03df5043a879db3dece5615ad8 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 4 Aug 2021 15:58:17 -0400 Subject: [PATCH] fix(NODE-3454): projection types are too narrow (#2924) --- src/collection.ts | 16 ++--- src/cursor/aggregation_cursor.ts | 32 ++++++++-- src/cursor/find_cursor.ts | 31 +++++++-- src/mongo_types.ts | 25 ++++---- src/operations/find.ts | 11 ++-- .../community/collection/findX.test-d.ts | 18 ++++-- test/types/community/cursor.test-d.ts | 63 +++++++++++++++++-- 7 files changed, 150 insertions(+), 46 deletions(-) diff --git a/src/collection.ts b/src/collection.ts index da41895076..58f74dde20 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -676,10 +676,10 @@ export class Collection { findOne(callback: Callback): void; findOne(filter: Filter): Promise; findOne(filter: Filter, callback: Callback): void; - findOne(filter: Filter, options: FindOptions): Promise; + findOne(filter: Filter, options: FindOptions): Promise; findOne( filter: Filter, - options: FindOptions, + options: FindOptions, callback: Callback ): void; @@ -687,16 +687,16 @@ export class Collection { findOne(): Promise; findOne(callback: Callback): void; findOne(filter: Filter): Promise; - findOne(filter: Filter, options?: FindOptions): Promise; + findOne(filter: Filter, options?: FindOptions): Promise; findOne( filter: Filter, - options?: FindOptions, + options?: FindOptions, callback?: Callback ): void; findOne( filter?: Filter | Callback, - options?: FindOptions | Callback, + options?: FindOptions | Callback, callback?: Callback ): Promise | void { if (callback != null && typeof callback !== 'function') { @@ -728,9 +728,9 @@ export class Collection { * @param filter - The filter predicate. If unspecified, then all documents in the collection will match the predicate */ find(): FindCursor; - find(filter: Filter, options?: FindOptions): FindCursor; - find(filter: Filter, options?: FindOptions): FindCursor; - find(filter?: Filter, options?: FindOptions): FindCursor { + find(filter: Filter, options?: FindOptions): FindCursor; + find(filter: Filter, options?: FindOptions): FindCursor; + find(filter?: Filter, options?: FindOptions): FindCursor { if (arguments.length > 2) { throw new MongoInvalidArgumentError( 'Method "collection.find()" accepts at most two arguments' diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts index d01428dd79..4afff532b9 100644 --- a/src/cursor/aggregation_cursor.ts +++ b/src/cursor/aggregation_cursor.ts @@ -9,7 +9,6 @@ import type { Callback, MongoDBNamespace } from '../utils'; import type { ClientSession } from '../sessions'; import type { AbstractCursorOptions } from './abstract_cursor'; import type { ExplainVerbosityLike } from '../explain'; -import type { Projection } from '../mongo_types'; /** @public */ export interface AggregationCursorOptions extends AbstractCursorOptions, AggregateOptions {} @@ -135,22 +134,43 @@ export class AggregationCursor extends AbstractCursor = cursor.project<{ a: number }>({ _id: 0, a: true }); + * // Flexible way + * const docs: AggregationCursor = cursor.project({ _id: 0, a: true }); + * ``` + * + * @remarks + * In order to strictly type this function you must provide an interface + * that represents the effect of your projection on the result documents. + * + * Adding a projection changes the return type of the iteration of this cursor, * it **does not** return a new instance of a cursor. This means when calling project, * you should always assign the result to a new variable. Take note of the following example: * * @example * ```typescript * const cursor: AggregationCursor<{ a: number; b: string }> = coll.aggregate([]); - * const projectCursor = cursor.project<{ a: number }>({ a: true }); + * const projectCursor = cursor.project<{ a: number }>({ _id: 0, a: true }); * const aPropOnlyArray: {a: number}[] = await projectCursor.toArray(); + * + * // or always use chaining and save the final cursor + * + * const cursor = coll.aggregate().project<{ a: string }>({ + * _id: 0, + * a: { $convert: { input: '$a', to: 'string' } + * }}); * ``` */ - project($project: Projection): AggregationCursor; - project($project: Document): this { + project($project: Document): AggregationCursor { assertUninitialized(this); this[kPipeline].push({ $project }); - return this; + return (this as unknown) as AggregationCursor; } /** Add a lookup stage to the aggregation pipeline */ diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts index 65821f8f5f..a2a88fa330 100644 --- a/src/cursor/find_cursor.ts +++ b/src/cursor/find_cursor.ts @@ -12,7 +12,6 @@ import type { ClientSession } from '../sessions'; import { formatSort, Sort, SortDirection } from '../sort'; import type { Callback, MongoDBNamespace } from '../utils'; import { AbstractCursor, assertUninitialized } from './abstract_cursor'; -import type { Projection } from '../mongo_types'; /** @internal */ const kFilter = Symbol('filter'); @@ -344,22 +343,42 @@ export class FindCursor extends AbstractCursor { * In order to strictly type this function you must provide an interface * that represents the effect of your projection on the result documents. * - * **NOTE:** adding a projection changes the return type of the iteration of this cursor, + * By default chaining a projection to your cursor changes the returned type to the generic + * {@link Document} type. + * You should specify a parameterized type to have assertions on your final results. + * + * @example + * ```typescript + * // Best way + * const docs: FindCursor<{ a: number }> = cursor.project<{ a: number }>({ _id: 0, a: true }); + * // Flexible way + * const docs: FindCursor = cursor.project({ _id: 0, a: true }); + * ``` + * + * @remarks + * + * Adding a projection changes the return type of the iteration of this cursor, * it **does not** return a new instance of a cursor. This means when calling project, * you should always assign the result to a new variable. Take note of the following example: * * @example * ```typescript * const cursor: FindCursor<{ a: number; b: string }> = coll.find(); - * const projectCursor = cursor.project<{ a: number }>({ a: true }); + * const projectCursor = cursor.project<{ a: number }>({ _id: 0, a: true }); * const aPropOnlyArray: {a: number}[] = await projectCursor.toArray(); + * + * // or always use chaining and save the final cursor + * + * const cursor = coll.find().project<{ a: string }>({ + * _id: 0, + * a: { $convert: { input: '$a', to: 'string' } + * }}); * ``` */ - project(value: Projection): FindCursor; - project(value: Projection): this { + project(value: Document): FindCursor { assertUninitialized(this); this[kBuiltOptions].projection = value; - return this; + return (this as unknown) as FindCursor; } /** diff --git a/src/mongo_types.ts b/src/mongo_types.ts index cf25f75d66..d4107e1418 100644 --- a/src/mongo_types.ts +++ b/src/mongo_types.ts @@ -169,20 +169,19 @@ export type BSONType = typeof BSONType[keyof typeof BSONType]; /** @public */ export type BSONTypeAlias = keyof typeof BSONType; -/** @public */ -export interface ProjectionOperators extends Document { - $elemMatch?: Document; - $slice?: number | [number, number]; - $meta?: string; - /** @deprecated Since MongoDB 3.2, Use FindCursor#max */ - $max?: any; -} +/** + * @public + * Projection is flexible to permit the wide array of aggregation operators + * @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars +export type Projection = Document; -/** @public */ -export type Projection = { - [Key in keyof TSchema]?: ProjectionOperators | 0 | 1 | boolean; -} & - Partial>; +/** + * @public + * @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further + */ +export type ProjectionOperators = Document; /** @public */ export type IsAny = true extends false & Type diff --git a/src/operations/find.ts b/src/operations/find.ts index d3eed86a36..3b1b9c239e 100644 --- a/src/operations/find.ts +++ b/src/operations/find.ts @@ -15,16 +15,19 @@ import { Sort, formatSort } from '../sort'; import { isSharded } from '../cmap/wire_protocol/shared'; import { ReadConcern } from '../read_concern'; import type { ClientSession } from '../sessions'; -import type { Projection } from '../mongo_types'; -/** @public */ -export interface FindOptions extends CommandOperationOptions { +/** + * @public + * @typeParam TSchema - Unused schema definition, deprecated usage, only specify `FindOptions` with no generic + */ +// eslint-disable-next-line @typescript-eslint/no-unused-vars +export interface FindOptions extends CommandOperationOptions { /** Sets the limit of documents returned in the query. */ limit?: number; /** Set to sort the documents coming back from the query. Array of indexes, `[['a', 1]]` etc. */ sort?: Sort; /** The fields to return in the query. Object of fields to either include or exclude (one of, not both), `{'a':1, 'b': 1}` **or** `{'a': 0, 'b': 0}` */ - projection?: Projection; + projection?: Document; /** Set to skip N documents ahead in your query (useful for pagination). */ skip?: number; /** Tell the query to use specific indexes in the query. Object of indexes to use, `{'_id':1}` */ diff --git a/test/types/community/collection/findX.test-d.ts b/test/types/community/collection/findX.test-d.ts index bc17e3179e..16c596807d 100644 --- a/test/types/community/collection/findX.test-d.ts +++ b/test/types/community/collection/findX.test-d.ts @@ -1,5 +1,6 @@ -import { expectNotType, expectType } from 'tsd'; +import { expectAssignable, expectNotType, expectType } from 'tsd'; import { FindCursor, FindOptions, MongoClient, Document } from '../../../../src'; +import type { Projection, ProjectionOperators } from '../../../../src'; import type { PropExists } from '../../utility_types'; // collection.findX tests @@ -35,7 +36,7 @@ await collectionT.findOne( } ); -const optionsWithComplexProjection: FindOptions = { +const optionsWithComplexProjection: FindOptions = { projection: { stringField: { $meta: 'textScore' }, fruitTags: { $min: 'fruitTags' }, @@ -120,14 +121,15 @@ function printCar(car: Car | undefined) { console.log(car ? `A car of ${car.make} make` : 'No car'); } -const options: FindOptions = {}; -const optionsWithProjection: FindOptions = { +const options: FindOptions = {}; +const optionsWithProjection: FindOptions = { projection: { make: 1 } }; -expectNotType>({ +// this is changed in NODE-3454 to be the opposite test since Projection is flexible now +expectAssignable({ projection: { make: 'invalid' } @@ -190,3 +192,9 @@ expectType>(colorCollection.find({ color: // When you use the override, $in doesn't permit readonly colorCollection.find<{ color: string }>({ color: { $in: colorsFreeze } }); colorCollection.find<{ color: string }>({ color: { $in: ['regularArray'] } }); +// This is a regression test that we don't remove the unused generic in FindOptions +const findOptions: FindOptions<{ a: number }> = {}; +expectType(findOptions); +// This is just to check that we still export these type symbols +expectAssignable({}); +expectAssignable({}); diff --git a/test/types/community/cursor.test-d.ts b/test/types/community/cursor.test-d.ts index 2ea825b388..503d70d408 100644 --- a/test/types/community/cursor.test-d.ts +++ b/test/types/community/cursor.test-d.ts @@ -1,6 +1,6 @@ import type { Readable } from 'stream'; import { expectNotType, expectType } from 'tsd'; -import { FindCursor, MongoClient } from '../../../src/index'; +import { FindCursor, MongoClient, Db, Document } from '../../../src/index'; // TODO(NODE-3346): Improve these tests to use expect assertions more @@ -22,7 +22,7 @@ const cursor = collection .min({ age: 18 }) .maxAwaitTimeMS(1) .maxTimeMS(1) - .project({}) + // .project({}) -> projections removes the types from the returned documents .returnKey(true) .showRecordId(true) .skip(1) @@ -31,6 +31,7 @@ const cursor = collection expectType>(cursor); expectType(cursor.stream()); +expectType>(cursor.project({})); collection.find().project({}); collection.find().project({ notExistingField: 1 }); @@ -74,13 +75,13 @@ expectNotType<{ age: number }[]>(await typedCollection.find().project({ name: 1 expectType<{ notExistingField: unknown }[]>( await typedCollection.find().project({ notExistingField: 1 }).toArray() ); -expectNotType(await typedCollection.find().project({ notExistingField: 1 }).toArray()); +expectType(await typedCollection.find().project({ notExistingField: 1 }).toArray()); // Projection operator expectType<{ listOfNumbers: number[] }[]>( await typedCollection .find() - .project({ listOfNumbers: { $slice: [0, 4] } }) + .project<{ listOfNumbers: number[] }>({ listOfNumbers: { $slice: [0, 4] } }) .toArray() ); @@ -98,3 +99,57 @@ void async function () { expectType(item.foo); } }; + +interface InternalMeme { + _id: string; + owner: string; + receiver: string; + createdAt: Date; + expiredAt: Date; + description: string; + likes: string; + private: string; + replyTo: string; + imageUrl: string; +} + +interface PublicMeme { + myId: string; + owner: string; + likes: number; + someRandomProp: boolean; // Projection makes no enforcement on anything + // the convenience parameter project() allows you to define a return type, + // otherwise projections returns generic document +} + +const publicMemeProjection = { + myId: { $toString: '$_id' }, + owner: { $toString: '$owner' }, + receiver: { $toString: '$receiver' }, + likes: '$totalLikes' // <== (NODE-3454) cause of TS2345 error: Argument of type T is not assignable to parameter of type U +}; +const memeCollection = new Db(new MongoClient(''), '').collection('memes'); + +expectType( + await memeCollection + .find({ _id: { $in: [] } }) + .project(publicMemeProjection) // <== + .toArray() +); + +// Returns generic document when no override given +expectNotType( + await memeCollection + .find({ _id: { $in: [] } }) + .project(publicMemeProjection) + .toArray() +); + +// Returns generic document when there is no schema +expectType( + await new Db(new MongoClient(''), '') + .collection('memes') + .find({ _id: { $in: [] } }) + .project(publicMemeProjection) + .toArray() +);