Skip to content

Commit

Permalink
fix(NODE-3468): remove generic overrides from find
Browse files Browse the repository at this point in the history
  • Loading branch information
@nbbeeken
nbbeeken committed Aug 10, 2021
1 parent 48d6da9 commit 86c3055
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 86 deletions.
13 changes: 0 additions & 13 deletions src/collection.ts
View file
Expand Up @@ -682,18 +682,6 @@ export class Collection<TSchema extends Document = Document> {
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): Promise<T | undefined>;
findOne<T = TSchema>(
filter: Filter<T>,
options?: FindOptions,
callback?: Callback<T | undefined>
): void;

findOne(
filter?: Filter<TSchema> | Callback<TSchema | undefined>,
options?: FindOptions | Callback<TSchema | undefined>,
Expand Down Expand Up @@ -729,7 +717,6 @@ export class Collection<TSchema extends Document = Document> {
*/
find(): 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(
Expand Down
39 changes: 19 additions & 20 deletions src/cursor/abstract_cursor.ts
View file
Expand Up @@ -279,8 +279,7 @@ export abstract class AbstractCursor<
return done(undefined, true);
}

next<any>(this, true, (err, doc) => {
// FIXME(NODE):
next<TSchema>(this, true, (err, doc) => {
if (err) return done(err);

if (doc) {
Expand All @@ -295,9 +294,9 @@ export abstract class AbstractCursor<
}

/** Get the next available document from the cursor, returns null if no more documents are available. */
next<T = TSchema>(): Promise<T | null>;
next<T = TSchema>(callback: Callback<T | null>): void;
next<T = TSchema>(callback?: Callback<T | null>): Promise<T | null> | void {
next(): Promise<TSchema | null>;
next(callback: Callback<TSchema | null>): void;
next(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void {
return maybePromise(callback, done => {
if (this[kId] === Long.ZERO) {
return done(new MongoCursorExhaustedError());
Expand All @@ -310,9 +309,9 @@ export abstract class AbstractCursor<
/**
* Try to get the next available document from the cursor or `null` if an empty batch is returned
*/
tryNext<T = TSchema>(): Promise<T | null>;
tryNext<T = TSchema>(callback: Callback<T | null>): void;
tryNext<T = TSchema>(callback?: Callback<T | null>): Promise<T | null> | void {
tryNext(): Promise<TSchema | null>;
tryNext(callback: Callback<TSchema | null>): void;
tryNext(callback?: Callback<TSchema | null>): Promise<TSchema | null> | void {
return maybePromise(callback, done => {
if (this[kId] === Long.ZERO) {
return done(new MongoCursorExhaustedError());
Expand All @@ -328,10 +327,10 @@ export abstract class AbstractCursor<
* @param iterator - The iteration callback.
* @param callback - The end callback.
*/
forEach<T = TSchema>(iterator: (doc: T) => boolean | void): Promise<void>;
forEach<T = TSchema>(iterator: (doc: T) => boolean | void, callback: Callback<void>): void;
forEach<T = TSchema>(
iterator: (doc: T) => boolean | void,
forEach(iterator: (doc: TSchema) => boolean | void): Promise<void>;
forEach(iterator: (doc: TSchema) => boolean | void, callback: Callback<void>): void;
forEach(
iterator: (doc: TSchema) => boolean | void,
callback?: Callback<void>
): Promise<void> | void {
if (typeof iterator !== 'function') {
Expand All @@ -340,7 +339,7 @@ export abstract class AbstractCursor<
return maybePromise(callback, done => {
const transform = this[kTransform];
const fetchDocs = () => {
next<T>(this, true, (err, doc) => {
next<TSchema>(this, true, (err, doc) => {
if (err || doc == null) return done(err);
let result;
// NOTE: no need to transform because `next` will do this automatically
Expand All @@ -357,7 +356,7 @@ export abstract class AbstractCursor<
for (let i = 0; i < internalDocs.length; ++i) {
try {
result = iterator(
(transform ? transform(internalDocs[i]) : internalDocs[i]) as T // TODO(NODE-3283): Improve transform typing
(transform ? transform(internalDocs[i]) : internalDocs[i]) as TSchema // TODO(NODE-3283): Improve transform typing
);
} catch (error) {
return done(error);
Expand Down Expand Up @@ -395,15 +394,15 @@ export abstract class AbstractCursor<
*
* @param callback - The result callback.
*/
toArray<T = TSchema>(): Promise<T[]>;
toArray<T = TSchema>(callback: Callback<T[]>): void;
toArray<T = TSchema>(callback?: Callback<T[]>): Promise<T[]> | void {
toArray(): Promise<TSchema[]>;
toArray(callback: Callback<TSchema[]>): void;
toArray(callback?: Callback<TSchema[]>): Promise<TSchema[]> | void {
return maybePromise(callback, done => {
const docs: T[] = [];
const docs: TSchema[] = [];
const transform = this[kTransform];
const fetchDocs = () => {
// NOTE: if we add a `nextBatch` then we should use it here
next<T>(this, true, (err, doc) => {
next<TSchema>(this, true, (err, doc) => {
if (err) return done(err);
if (doc == null) return done(undefined, docs);

Expand All @@ -413,7 +412,7 @@ export abstract class AbstractCursor<
// these do need to be transformed since they are copying the rest of the batch
const internalDocs = (transform
? this[kDocuments].splice(0, this[kDocuments].length).map(transform)
: this[kDocuments].splice(0, this[kDocuments].length)) as T[]; // TODO(NODE-3283): Improve transform typing
: this[kDocuments].splice(0, this[kDocuments].length)) as TSchema[]; // TODO(NODE-3283): Improve transform typing

if (internalDocs) {
docs.push(...internalDocs);
Expand Down
16 changes: 4 additions & 12 deletions src/db.ts
View file
Expand Up @@ -289,7 +289,7 @@ export class Db {
* @param pipeline - An array of aggregation stages to be executed
* @param options - Optional settings for the command
*/
aggregate(pipeline: Document[] = [], options?: AggregateOptions): AggregationCursor {
aggregate<T = Document>(pipeline: Document[] = [], options?: AggregateOptions): AggregationCursor<T> {
if (arguments.length > 2) {
throw new MongoInvalidArgumentError('Method "db.aggregate()" accepts at most two arguments');
}
Expand Down Expand Up @@ -370,17 +370,9 @@ export class Db {
filter: Document,
options: Exclude<ListCollectionsOptions, 'nameOnly'> & { nameOnly: false }
): ListCollectionsCursor<CollectionInfo>;
listCollections<
T extends Pick<CollectionInfo, 'name' | 'type'> | CollectionInfo =
| Pick<CollectionInfo, 'name' | 'type'>
| CollectionInfo
>(filter?: Document, options?: ListCollectionsOptions): ListCollectionsCursor<T>;
listCollections<
T extends Pick<CollectionInfo, 'name' | 'type'> | CollectionInfo =
| Pick<CollectionInfo, 'name' | 'type'>
| CollectionInfo
>(filter: Document = {}, options: ListCollectionsOptions = {}): ListCollectionsCursor<T> {
return new ListCollectionsCursor<T>(this, filter, resolveOptions(this, options));
listCollections(filter?: Document, options?: ListCollectionsOptions): ListCollectionsCursor;
listCollections(filter: Document = {}, options: ListCollectionsOptions = {}): ListCollectionsCursor {
return new ListCollectionsCursor(this, filter, resolveOptions(this, options));
}

/**
Expand Down
32 changes: 20 additions & 12 deletions test/types/community/collection/findX.test-d.ts
View file
Expand Up @@ -91,16 +91,18 @@ expectType<Bag | undefined>(
await collectionBag.findOne({ color: 'red' }, { projection: { cost: 1 } })
);

const overrideFind = await collectionBag.findOne<{ cost: number }>(
{ color: 'white' },
{ projection: { cost: 1 } }
);
expectType<PropExists<typeof overrideFind, 'color'>>(false);
// NODE-3468 Overriding the return type with a generic is not permitted
// const overrideFind = await collectionBag.findOne<{ cost: number }>(
// { color: 'white' },
// { projection: { cost: 1 } }
// );
// expectType<PropExists<typeof overrideFind, 'color'>>(false);

// Overriding findOne, makes the return that exact type
expectType<{ cost: number } | undefined>(
await collectionBag.findOne<{ cost: number }>({ color: 'red' }, { projection: { cost: 1 } })
);
// NODE-3468 Overriding the return type with a generic is not permitted
// expectType<{ cost: number } | undefined>(
// await collectionBag.findOne<{ cost: number }>({ color: 'red' }, { projection: { cost: 1 } })
// );

interface Car {
make: string;
Expand Down Expand Up @@ -182,16 +184,22 @@ expectType<FindCursor<{ lists: ReadonlyArray<string> }>>(
);

// Before NODE-3452's fix we would get this strange result that included the filter shape joined with the actual schema
// NODE-3468 Overriding the return type with a generic is not permitted
expectNotType<FindCursor<{ color: string | { $in: ReadonlyArray<string> } }>>(
colorCollection.find({ color: { $in: colorsFreeze } })
);

// This is related to another bug that will be fixed in NODE-3454
expectType<FindCursor<{ color: { $in: number } }>>(colorCollection.find({ color: { $in: 3 } }));
// NODE-3454: Using the incorrect $in value doesn't mess with the resulting schema
expectNotType<FindCursor<{ color: { $in: number } }>>(
colorCollection.find({ color: { $in: 3 as any } }) // `as any` is to let us make this mistake and still show the result type isn't broken
);
expectType<FindCursor<{ color: string }>>(colorCollection.find({ color: { $in: 3 as any } }));

// When you use the override, $in doesn't permit readonly
colorCollection.find<{ color: string }>({ color: { $in: colorsFreeze } });
colorCollection.find<{ color: string }>({ color: { $in: ['regularArray'] } });
// NODE-3468 Overriding the return type with a generic is not permitted
// 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);
Expand Down
46 changes: 32 additions & 14 deletions test/types/community/cursor.test-d.ts
View file
Expand Up @@ -62,22 +62,35 @@ expectType<{ name: string }[]>(
);

// override the collection type
typedCollection
.find<{ name2: string; age2: number }>({ name: '123' }, { projection: { age2: 1 } })
.map(x => x.name2 && x.age2);
// NODE-3468: Overriding the return type is no longer allowed
// typedCollection
// .find<{ name2: string; age2: number }>({ name: '123' }, { projection: { age2: 1 } })
// .map(x => x.name2 && x.age2);

// Chaining map calls changes the final cursor
expectType<FindCursor<{ a: string }>>(
typedCollection
.find({ name: '123' })
.map(x => ({ name2: x.name, age2: x.age }))
.map(({ name2, age2 }) => ({ a: `${name2}_${age2}` }))
);

typedCollection.find({ name: '123' }, { projection: { age: 1 } }).map(x => x.tag);

// A known key with a constant projection
expectType<{ name: string }[]>(await typedCollection.find().project({ name: 1 }).toArray());
// NODE-3468: projection returns a generic type and the override removal means no automatic type coercion
// expectType<{ name: string }[]>(await typedCollection.find().project({ name: 1 }).toArray());
//
expectType<Document[]>(await typedCollection.find().project({ name: 1 }).toArray());
expectNotType<{ age: number }[]>(await typedCollection.find().project({ name: 1 }).toArray());

// An unknown key
expectType<{ notExistingField: unknown }[]>(
// An unknown key -- NODE-3468: when using the project, your default return type is Document
expectNotType<{ notExistingField: unknown }[]>(
await typedCollection.find().project({ notExistingField: 1 }).toArray()
);
expectType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray());
expectNotType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray());

// Projection operator
// Projection operator -- NODE-3468: it is recommended that users override the T in project<T>()
expectType<{ listOfNumbers: number[] }[]>(
await typedCollection
.find()
Expand All @@ -87,10 +100,7 @@ expectType<{ listOfNumbers: number[] }[]>(

// Using the override parameter works
expectType<{ name: string }[]>(
await typedCollection
.find()
.project<{ name: string }>({ name: 1 })
.toArray()
await typedCollection.find().project<{ name: string }>({ name: 1 }).toArray()
);

void async function () {
Expand Down Expand Up @@ -128,7 +138,7 @@ const publicMemeProjection = {
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');
const memeCollection = db.collection<InternalMeme>('memes');

expectType<PublicMeme[]>(
await memeCollection
Expand All @@ -145,9 +155,17 @@ expectNotType<InternalMeme[]>(
.toArray()
);

expectType<FindCursor<Document>>(
memeCollection.find({ _id: { $in: [] } }).project(publicMemeProjection)
);

expectNotType<FindCursor<Document>>(memeCollection.find({ _id: { $in: [] } }));
expectType<FindCursor<InternalMeme>>(memeCollection.find({ _id: { $in: [] } }));
``;

// Returns generic document when there is no schema
expectType<Document[]>(
await new Db(new MongoClient(''), '')
await db
.collection('memes')
.find({ _id: { $in: [] } })
.project(publicMemeProjection)
Expand Down
22 changes: 7 additions & 15 deletions test/types/list_collections.test-d.ts
View file
Expand Up @@ -8,38 +8,30 @@ const db = new MongoClient('').db();
type EitherCollectionInfoResult = CollectionInfo | Pick<CollectionInfo, 'name' | 'type'>;

// We default to the CollectionInfo result type
expectType<ListCollectionsCursor<Pick<CollectionInfo, 'name' | 'type'> | CollectionInfo>>(
db.listCollections()
);
expectType<ListCollectionsCursor<CollectionInfo>>(db.listCollections());
// By default it isn't narrowed to either type
expectNotType<ListCollectionsCursor<Pick<CollectionInfo, 'name' | 'type'>>>(db.listCollections());
expectNotType<ListCollectionsCursor<CollectionInfo>>(db.listCollections());
expectType<ListCollectionsCursor<CollectionInfo>>(db.listCollections());

// Testing each argument variation
db.listCollections();
db.listCollections({ a: 2 });
db.listCollections({ a: 2 }, { batchSize: 2 });

const collections = await db.listCollections().toArray();
expectType<EitherCollectionInfoResult[]>(collections);
expectNotType<EitherCollectionInfoResult[]>(collections);

const nameOnly = await db.listCollections({}, { nameOnly: true }).toArray();
expectType<Pick<CollectionInfo, 'name' | 'type'>[]>(nameOnly);

const fullInfo = await db.listCollections({}, { nameOnly: false }).toArray();
expectType<CollectionInfo[]>(fullInfo);

const couldBeEither = await db.listCollections({}, { nameOnly: Math.random() > 0.5 }).toArray();
expectType<EitherCollectionInfoResult[]>(couldBeEither);
const cannotBeEither = await db.listCollections({}, { nameOnly: Math.random() > 0.5 }).toArray();
expectNotType<EitherCollectionInfoResult[]>(cannotBeEither);
expectType<CollectionInfo[]>(cannotBeEither);

// Showing here that:
// regardless of the option the generic parameter can be used to coerce the result if need be
// note the nameOnly: false, yet strings are returned
const overridden = await db
.listCollections<Pick<CollectionInfo, 'name' | 'type'>>({}, { nameOnly: false })
.toArray();
expectType<Pick<CollectionInfo, 'name' | 'type'>[]>(overridden);
const overriddenWithToArray = await db
.listCollections({}, { nameOnly: false })
.toArray<Pick<CollectionInfo, 'name' | 'type'>>();
expectType<Pick<CollectionInfo, 'name' | 'type'>[]>(overriddenWithToArray);
// NODE-3468: No longer permits overriding of return type via a generic

0 comments on commit 86c3055

Please sign in to comment.