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

feat(NODE-3729): add withId to default return type for collection.find and collection.findOne #3039

Merged
merged 2 commits into from
Nov 16, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -676,12 +676,16 @@ export class Collection<TSchema extends Document = Document> {
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
findOne(): Promise<TSchema | null>;
findOne(callback: Callback<TSchema | null>): void;
findOne(filter: Filter<TSchema>): Promise<TSchema | null>;
findOne(filter: Filter<TSchema>, callback: Callback<TSchema | null>): void;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<TSchema | null>;
findOne(filter: Filter<TSchema>, options: FindOptions, callback: Callback<TSchema | null>): void;
findOne(): Promise<WithId<TSchema> | null>;
findOne(callback: Callback<WithId<TSchema> | null>): void;
findOne(filter: Filter<TSchema>): Promise<WithId<TSchema> | null>;
findOne(filter: Filter<TSchema>, callback: Callback<WithId<TSchema> | null>): void;
findOne(filter: Filter<TSchema>, options: FindOptions): Promise<WithId<TSchema> | null>;
findOne(
filter: Filter<TSchema>,
options: FindOptions,
callback: Callback<WithId<TSchema> | null>
): void;

// allow an override of the schema.
findOne<T = TSchema>(): Promise<T | null>;
Expand All @@ -695,18 +699,18 @@ export class Collection<TSchema extends Document = Document> {
): void;

findOne(
filter?: Filter<TSchema> | Callback<TSchema | null>,
options?: FindOptions | Callback<TSchema | null>,
callback?: Callback<TSchema | null>
): Promise<TSchema | null> | void {
filter?: Filter<TSchema> | Callback<WithId<TSchema> | null>,
options?: FindOptions | Callback<WithId<TSchema> | null>,
callback?: Callback<WithId<TSchema> | null>
): Promise<WithId<TSchema> | null> | void {
if (callback != null && typeof callback !== 'function') {
throw new MongoInvalidArgumentError(
'Third parameter to `findOne()` must be a callback or undefined'
);
}

if (typeof filter === 'function') {
callback = filter as Callback<TSchema | null>;
callback = filter as Callback<WithId<TSchema> | null>;
filter = {};
options = {};
}
Expand All @@ -725,10 +729,10 @@ 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): FindCursor<TSchema>;
find<T>(filter: Filter<TSchema>, options?: FindOptions): FindCursor<T>;
find(filter?: Filter<TSchema>, options?: FindOptions): FindCursor<TSchema> {
find(): FindCursor<WithId<TSchema>>;
find(filter: Filter<WithId<TSchema>>, options?: FindOptions): FindCursor<WithId<TSchema>>;
find<T>(filter: Filter<WithId<TSchema>>, options?: FindOptions): FindCursor<T>;
find(filter?: Filter<WithId<TSchema>>, options?: FindOptions): FindCursor<WithId<TSchema>> {
Comment on lines +733 to +735
Copy link
Contributor

Choose a reason for hiding this comment

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

@PaulEndri I think adding WithId to the Filter type here was a mistake. The other Filter types in this file are not using it.

if (arguments.length > 2) {
throw new MongoInvalidArgumentError(
'Method "collection.find()" accepts at most two arguments'
Expand All @@ -738,7 +742,7 @@ export class Collection<TSchema extends Document = Document> {
throw new MongoInvalidArgumentError('Argument "options" must not be function');
}

return new FindCursor<TSchema>(
return new FindCursor<WithId<TSchema>>(
getTopology(this),
this.s.namespace,
filter,
Expand Down
22 changes: 13 additions & 9 deletions test/types/community/collection/filterQuery.test-d.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BSONRegExp, Decimal128, ObjectId } from 'bson';
import { expectAssignable, expectNotType, expectType } from 'tsd';
import { Filter, MongoClient } from '../../../../src';
import { Filter, MongoClient, WithId } from '../../../../src';

/**
* test the Filter type using collection.find<T>() method
Expand Down Expand Up @@ -57,12 +57,16 @@ collectionT.find(spot); // a whole model definition is also a valid filter
* test simple field queries e.g. `{ name: 'Spot' }`
*/
/// it should query __string__ fields
expectType<PetModel[]>(await collectionT.find({ name: 'Spot' }).toArray());
expectType<WithId<PetModel>[]>(await collectionT.find({ name: 'Spot' }).toArray());
// it should query string fields by regex
expectType<PetModel[]>(await collectionT.find({ name: /Blu/i }).toArray());
expectType<WithId<PetModel>[]>(await collectionT.find({ name: /Blu/i }).toArray());
// it should query string fields by RegExp object, and bson regex
expectType<PetModel[]>(await collectionT.find({ name: new RegExp('MrMeow', 'i') }).toArray());
expectType<PetModel[]>(await collectionT.find({ name: new BSONRegExp('MrMeow', 'i') }).toArray());
expectType<WithId<PetModel>[]>(
await collectionT.find({ name: new RegExp('MrMeow', 'i') }).toArray()
);
expectType<WithId<PetModel>[]>(
await collectionT.find({ name: new BSONRegExp('MrMeow', 'i') }).toArray()
);
/// it should not accept wrong types for string fields
expectNotType<Filter<PetModel>>({ name: 23 });
expectNotType<Filter<PetModel>>({ name: { suffix: 'Jr' } });
Expand Down Expand Up @@ -90,9 +94,9 @@ expectNotType<Filter<PetModel>>({ bestFriend: [{ family: 'Andersons' }] });
/// it should query __array__ fields by exact match
await collectionT.find({ treats: ['kibble', 'bone'] }).toArray();
/// it should query __array__ fields by element type
expectType<PetModel[]>(await collectionT.find({ treats: 'kibble' }).toArray());
expectType<PetModel[]>(await collectionT.find({ treats: /kibble/i }).toArray());
expectType<PetModel[]>(await collectionT.find({ friends: spot }).toArray());
expectType<WithId<PetModel>[]>(await collectionT.find({ treats: 'kibble' }).toArray());
expectType<WithId<PetModel>[]>(await collectionT.find({ treats: /kibble/i }).toArray());
expectType<WithId<PetModel>[]>(await collectionT.find({ friends: spot }).toArray());
/// it should not query array fields by wrong types
expectNotType<Filter<PetModel>>({ treats: 12 });
expectNotType<Filter<PetModel>>({ friends: { name: 'not a full model' } });
Expand Down Expand Up @@ -206,7 +210,7 @@ await collectionT.find({ $where: 'function() { return true }' }).toArray();
await collectionT
.find({
$where: function () {
expectType<PetModel>(this);
expectType<WithId<PetModel>>(this);
return this.name === 'MrMeow';
}
})
Expand Down
99 changes: 76 additions & 23 deletions test/types/community/collection/findX.test-d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import { expectAssignable, expectNotType, expectType } from 'tsd';
import { FindCursor, FindOptions, MongoClient, Document, Collection, Db } from '../../../../src';
import {
FindCursor,
FindOptions,
MongoClient,
Document,
Collection,
Db,
WithId,
ObjectId
} from '../../../../src';
import type { Projection, ProjectionOperators } from '../../../../src';
import type { PropExists } from '../../utility_types';

Expand All @@ -10,7 +19,11 @@ const collection = db.collection('test.find');

// Locate all the entries using find
collection.find({}).toArray((_err, fields) => {
expectType<Document[] | undefined>(fields);
expectType<WithId<Document>[] | undefined>(fields);
if (fields) {
expectType<ObjectId>(fields[0]._id);
expectNotType<ObjectId | undefined>(fields[0]._id);
}
});

// test with collection type
Expand All @@ -26,7 +39,7 @@ collectionT.find({
$and: [{ numberField: { $gt: 0 } }, { numberField: { $lt: 100 } }],
readonlyFruitTags: { $all: ['apple', 'pear'] }
});
expectType<FindCursor<TestModel>>(collectionT.find({}));
expectType<FindCursor<WithId<TestModel>>>(collectionT.find({}));

await collectionT.findOne(
{},
Expand Down Expand Up @@ -72,22 +85,24 @@ interface Bag {

const collectionBag = db.collection<Bag>('bag');

const cursor: FindCursor<Bag> = collectionBag.find({ color: 'black' });
const cursor: FindCursor<WithId<Bag>> = collectionBag.find({ color: 'black' });

cursor.toArray((_err, bags) => {
expectType<Bag[] | undefined>(bags);
expectType<WithId<Bag>[] | undefined>(bags);
});

cursor.forEach(
bag => {
expectType<Bag>(bag);
expectType<WithId<Bag>>(bag);
},
() => {
return null;
}
);

expectType<Bag | null>(await collectionBag.findOne({ color: 'red' }, { projection: { cost: 1 } }));
expectType<WithId<Bag> | null>(
await collectionBag.findOne({ color: 'red' }, { projection: { cost: 1 } })
);

const overrideFind = await collectionBag.findOne<{ cost: number }>(
{ color: 'white' },
Expand Down Expand Up @@ -150,40 +165,48 @@ const colorsFreeze: ReadonlyArray<string> = Object.freeze(['blue', 'red']);
const colorsWritable: Array<string> = ['blue', 'red'];

// Permitted Readonly fields
expectType<FindCursor<{ color: string }>>(colorCollection.find({ color: { $in: colorsFreeze } }));
expectType<FindCursor<{ color: string }>>(colorCollection.find({ color: { $in: colorsWritable } }));
expectType<FindCursor<{ color: string }>>(colorCollection.find({ color: { $nin: colorsFreeze } }));
expectType<FindCursor<{ color: string }>>(
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $in: colorsFreeze } })
);
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $in: colorsWritable } })
);
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $nin: colorsFreeze } })
);
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $nin: colorsWritable } })
);
// $all and $elemMatch works against single fields (it's just redundant)
expectType<FindCursor<{ color: string }>>(colorCollection.find({ color: { $all: colorsFreeze } }));
expectType<FindCursor<{ color: string }>>(
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $all: colorsFreeze } })
);
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $all: colorsWritable } })
);
expectType<FindCursor<{ color: string }>>(
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $elemMatch: colorsFreeze } })
);
expectType<FindCursor<{ color: string }>>(
expectType<FindCursor<WithId<{ color: string }>>>(
colorCollection.find({ color: { $elemMatch: colorsWritable } })
);

const countCollection = client.db('test_db').collection<{ count: number }>('test_collection');
expectType<FindCursor<{ count: number }>>(
expectType<FindCursor<WithId<{ count: number }>>>(
countCollection.find({ count: { $bitsAnySet: Object.freeze([1, 0, 1]) } })
);
expectType<FindCursor<{ count: number }>>(
expectType<FindCursor<WithId<{ count: number }>>>(
countCollection.find({ count: { $bitsAnySet: [1, 0, 1] as number[] } })
);

const listsCollection = client.db('test_db').collection<{ lists: string[] }>('test_collection');
await listsCollection.updateOne({}, { list: { $pullAll: Object.freeze(['one', 'two']) } });
expectType<FindCursor<{ lists: string[] }>>(listsCollection.find({ lists: { $size: 1 } }));
expectType<FindCursor<WithId<{ lists: string[] }>>>(listsCollection.find({ lists: { $size: 1 } }));

const rdOnlyListsCollection = client
.db('test_db')
.collection<{ lists: ReadonlyArray<string> }>('test_collection');
expectType<FindCursor<{ lists: ReadonlyArray<string> }>>(
expectType<FindCursor<WithId<{ lists: ReadonlyArray<string> }>>>(
rdOnlyListsCollection.find({ lists: { $size: 1 } })
);

Expand All @@ -196,7 +219,9 @@ expectNotType<FindCursor<{ color: string | { $in: ReadonlyArray<string> } }>>(
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 } }));
expectType<FindCursor<WithId<{ 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 } });
Expand Down Expand Up @@ -228,12 +253,40 @@ interface TypedDb extends Db {
const typedDb = client.db('test2') as TypedDb;

const person = typedDb.collection('people').findOne({});
expectType<Promise<Person | null>>(person);
expectType<Promise<WithId<Person> | null>>(person);

typedDb.collection('people').findOne({}, function (_err, person) {
expectType<Person | null | undefined>(person); // null is if nothing is found, undefined is when there is an error defined
expectType<WithId<Person> | null | undefined>(person); // null is if nothing is found, undefined is when there is an error defined
});

typedDb.collection('things').findOne({}, function (_err, thing) {
expectType<Thing | null | undefined>(thing);
expectType<WithId<Thing> | null | undefined>(thing);
});

interface SchemaWithTypicalId {
_id: ObjectId;
name: string;
}
const schemaWithTypicalIdCol = db.collection<SchemaWithTypicalId>('a');
expectType<WithId<SchemaWithTypicalId> | null>(await schemaWithTypicalIdCol.findOne());
expectAssignable<SchemaWithTypicalId | null>(await schemaWithTypicalIdCol.findOne());

interface SchemaWithOptionalTypicalId {
_id?: ObjectId;
name: string;
}
const schemaWithOptionalTypicalId = db.collection<SchemaWithOptionalTypicalId>('a');
expectType<WithId<SchemaWithOptionalTypicalId> | null>(await schemaWithOptionalTypicalId.findOne());
expectAssignable<SchemaWithOptionalTypicalId | null>(await schemaWithOptionalTypicalId.findOne());

interface SchemaWithUserDefinedId {
_id: number;
name: string;
}
const schemaWithUserDefinedId = db.collection<SchemaWithUserDefinedId>('a');
expectType<WithId<SchemaWithUserDefinedId> | null>(await schemaWithUserDefinedId.findOne());
const result = await schemaWithUserDefinedId.findOne();
if (result !== null) {
expectType<number>(result._id);
}
expectAssignable<SchemaWithUserDefinedId | null>(await schemaWithUserDefinedId.findOne());
4 changes: 2 additions & 2 deletions test/types/mongodb.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { AggregationCursor } from '../../src/cursor/aggregation_cursor';
import type { FindCursor } from '../../src/cursor/find_cursor';
import type { ChangeStreamDocument } from '../../src/change_stream';
import type { Document } from 'bson';
import { Db } from '../../src';
import { Db, WithId } from '../../src';
import { Topology } from '../../src/sdam/topology';
import * as MongoDBDriver from '../../src';

Expand All @@ -30,7 +30,7 @@ const client = new MongoClient('');
const db = client.db('test');
const coll = db.collection('test');
const findCursor = coll.find();
expectType<Document | null>(await findCursor.next());
expectType<WithId<Document> | null>(await findCursor.next());
const mappedFind = findCursor.map<number>(obj => Object.keys(obj).length);
expectType<FindCursor<number>>(mappedFind);
expectType<number | null>(await mappedFind.next());
Expand Down
4 changes: 2 additions & 2 deletions test/types/union_schema.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expectType, expectError, expectNotType, expectNotAssignable, expectAssi

import type { Collection } from '../../src/collection';
import { ObjectId } from '../../src/bson';
import type { Filter } from '../../src/mongo_types';
import type { Filter, WithId } from '../../src/mongo_types';

type InsertOneFirstParam<Schema> = Parameters<Collection<Schema>['insertOne']>[0];

Expand Down Expand Up @@ -31,7 +31,7 @@ expectAssignable<ShapeInsert>({ height: 4, width: 4 });
expectAssignable<ShapeInsert>({ radius: 4 });

const c: Collection<Shape> = null as never;
expectType<Promise<Shape | null>>(c.findOne({ height: 4, width: 4 }));
expectType<Promise<WithId<Shape> | null>>(c.findOne({ height: 4, width: 4 }));
// collection API can only respect TSchema given, cannot pick a type inside a union
expectNotType<Promise<Rectangle | null>>(c.findOne({ height: 4, width: 4 }));

Expand Down