Skip to content

Commit

Permalink
feat(NODE-3729): add withId to default return type for collection.fin…
Browse files Browse the repository at this point in the history
…d and collection.findOne (#3039)
  • Loading branch information
PaulEndri committed Nov 16, 2021
1 parent 1f8b539 commit 52520aa
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 52 deletions.
36 changes: 20 additions & 16 deletions src/collection.ts
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>>;

This comment has been minimized.

Copy link
@avaly

avaly Nov 18, 2021

Contributor

@PaulEndri Adding WithId to the filter type seems like a mistake. The other filter types in this file are not using WithId.

find<T>(filter: Filter<WithId<TSchema>>, options?: FindOptions): FindCursor<T>;
find(filter?: Filter<WithId<TSchema>>, options?: FindOptions): FindCursor<WithId<TSchema>> {
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
@@ -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
@@ -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
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
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

0 comments on commit 52520aa

Please sign in to comment.