Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-3454): projection types are too narrow #2924

Merged
merged 9 commits into from Aug 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/collection.ts
Expand Up @@ -676,27 +676,27 @@ export class Collection<TSchema extends Document = Document> {
findOne(callback: Callback<TSchema | undefined>): void;
findOne(filter: Filter<TSchema>): Promise<TSchema | undefined>;
findOne(filter: Filter<TSchema>, callback: Callback<TSchema | undefined>): void;
findOne(filter: Filter<TSchema>, options: FindOptions<TSchema>): Promise<TSchema | undefined>;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<TSchema | undefined>;
findOne(
filter: Filter<TSchema>,
options: FindOptions<TSchema>,
options: FindOptions,
callback: Callback<TSchema | undefined>
): void;

// allow an override of the schema.
findOne<T = TSchema>(): Promise<T | undefined>;
findOne<T = TSchema>(callback: Callback<T | undefined>): void;
findOne<T = TSchema>(filter: Filter<T>): Promise<T | undefined>;
findOne<T = TSchema>(filter: Filter<T>, options?: FindOptions<T>): Promise<T | undefined>;
findOne<T = TSchema>(filter: Filter<T>, options?: FindOptions): Promise<T | undefined>;
findOne<T = TSchema>(
filter: Filter<T>,
options?: FindOptions<T>,
options?: FindOptions,
callback?: Callback<T | undefined>
): void;

findOne(
filter?: Filter<TSchema> | Callback<TSchema | undefined>,
options?: FindOptions<TSchema> | Callback<TSchema | undefined>,
options?: FindOptions | Callback<TSchema | undefined>,
callback?: Callback<TSchema>
): Promise<TSchema | undefined> | void {
if (callback != null && typeof callback !== 'function') {
Expand Down Expand Up @@ -728,9 +728,9 @@ export class Collection<TSchema extends Document = Document> {
* @param filter - The filter predicate. If unspecified, then all documents in the collection will match the predicate
*/
find(): FindCursor<TSchema>;
find(filter: Filter<TSchema>, options?: FindOptions<TSchema>): FindCursor<TSchema>;
find<T = TSchema>(filter: Filter<T>, options?: FindOptions<T>): FindCursor<T>;
find(filter?: Filter<TSchema>, options?: FindOptions<TSchema>): FindCursor<TSchema> {
find(filter: Filter<TSchema>, options?: FindOptions): FindCursor<TSchema>;
find<T = TSchema>(filter: Filter<T>, options?: FindOptions): FindCursor<T>;
find(filter?: Filter<TSchema>, options?: FindOptions): FindCursor<TSchema> {
if (arguments.length > 2) {
throw new MongoInvalidArgumentError(
'Method "collection.find()" accepts at most two arguments'
Expand Down
32 changes: 26 additions & 6 deletions src/cursor/aggregation_cursor.ts
Expand Up @@ -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 {}
Expand Down Expand Up @@ -135,22 +134,43 @@ export class AggregationCursor<TSchema = Document> extends AbstractCursor<TSchem
* 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: AggregationCursor<{ a: number }> = cursor.project<{ a: number }>({ _id: 0, a: true });
* // Flexible way
* const docs: AggregationCursor<Document> = 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<T = TSchema>($project: Projection<T>): AggregationCursor<T>;
project($project: Document): this {
project<T extends Document = Document>($project: Document): AggregationCursor<T> {
assertUninitialized(this);
this[kPipeline].push({ $project });
return this;
return (this as unknown) as AggregationCursor<T>;
}

/** Add a lookup stage to the aggregation pipeline */
Expand Down
31 changes: 25 additions & 6 deletions src/cursor/find_cursor.ts
Expand Up @@ -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');
Expand Down Expand Up @@ -344,22 +343,42 @@ export class FindCursor<TSchema = Document> extends AbstractCursor<TSchema> {
* 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<Document> = 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<T = TSchema>(value: Projection<T>): FindCursor<T>;
project(value: Projection<TSchema>): this {
project<T extends Document = Document>(value: Document): FindCursor<T> {
assertUninitialized(this);
this[kBuiltOptions].projection = value;
return this;
return (this as unknown) as FindCursor<T>;
}

/**
Expand Down
25 changes: 12 additions & 13 deletions src/mongo_types.ts
Expand Up @@ -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<TSchema extends Document = Document> = Document;

/** @public */
export type Projection<TSchema> = {
[Key in keyof TSchema]?: ProjectionOperators | 0 | 1 | boolean;
} &
Partial<Record<string, ProjectionOperators | 0 | 1 | boolean>>;
/**
* @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<Type, ResultIfAny, ResultIfNotAny> = true extends false & Type
Expand Down
11 changes: 7 additions & 4 deletions src/operations/find.ts
Expand Up @@ -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<TSchema = Document> 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<TSchema extends Document = Document> 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<TSchema>;
projection?: Document;
Copy link
Contributor

@PaulEndri PaulEndri Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't setting this to TSchema make the default functionality the same but still allow a more granular level of control without breaking things? And remove the need for the eslint disable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean setting projection?: TSchema? I think that would cause issue with the typical projection of inclusion / exclusion like { _id: 0, name: 1 }. where name: string in the TSchema

/** 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}` */
Expand Down
18 changes: 13 additions & 5 deletions 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
Expand Down Expand Up @@ -35,7 +36,7 @@ await collectionT.findOne(
}
);

const optionsWithComplexProjection: FindOptions<TestModel> = {
const optionsWithComplexProjection: FindOptions = {
projection: {
stringField: { $meta: 'textScore' },
fruitTags: { $min: 'fruitTags' },
Expand Down Expand Up @@ -120,14 +121,15 @@ function printCar(car: Car | undefined) {
console.log(car ? `A car of ${car.make} make` : 'No car');
}

const options: FindOptions<Car> = {};
const optionsWithProjection: FindOptions<Car> = {
const options: FindOptions = {};
const optionsWithProjection: FindOptions = {
projection: {
make: 1
}
};

expectNotType<FindOptions<Car>>({
// this is changed in NODE-3454 to be the opposite test since Projection is flexible now
expectAssignable<FindOptions>({
projection: {
make: 'invalid'
}
Expand Down Expand Up @@ -190,3 +192,9 @@ expectType<FindCursor<{ color: { $in: number } }>>(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>(findOptions);
// This is just to check that we still export these type symbols
expectAssignable<Projection>({});
expectAssignable<ProjectionOperators>({});
63 changes: 59 additions & 4 deletions 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

Expand All @@ -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)
Expand All @@ -31,6 +31,7 @@ const cursor = collection

expectType<FindCursor<{ foo: number }>>(cursor);
expectType<Readable>(cursor.stream());
expectType<FindCursor<Document>>(cursor.project({}));

collection.find().project({});
collection.find().project({ notExistingField: 1 });
Expand Down Expand Up @@ -74,13 +75,13 @@ expectNotType<{ age: number }[]>(await typedCollection.find().project({ name: 1
expectType<{ notExistingField: unknown }[]>(
await typedCollection.find().project({ notExistingField: 1 }).toArray()
);
expectNotType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray());
expectType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was supposed to yield Document[], which shouldn't match TypedDoc[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected the test, I think this is correct, unless we want to change the return to always be generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the larger issue that we discovered, we'll tackle it in NODE-3468


// Projection operator
expectType<{ listOfNumbers: number[] }[]>(
await typedCollection
.find()
.project({ listOfNumbers: { $slice: [0, 4] } })
.project<{ listOfNumbers: number[] }>({ listOfNumbers: { $slice: [0, 4] } })
.toArray()
);

Expand All @@ -98,3 +99,57 @@ void async function () {
expectType<number>(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<X>() 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<InternalMeme>('memes');

expectType<PublicMeme[]>(
await memeCollection
.find({ _id: { $in: [] } })
.project<PublicMeme>(publicMemeProjection) // <==
.toArray()
);

// Returns generic document when no override given
expectNotType<InternalMeme[]>(
await memeCollection
.find({ _id: { $in: [] } })
.project(publicMemeProjection)
.toArray()
);

// Returns generic document when there is no schema
expectType<Document[]>(
await new Db(new MongoClient(''), '')
.collection('memes')
.find({ _id: { $in: [] } })
.project(publicMemeProjection)
.toArray()
);