fix(NODE-3468): remove generic overrides from find (#2935)

nbbeeken · web-flow · commit 74bd7bdd7a9d · 2021-08-24T13:54:53.000-04:00
diff --git a/package.json b/package.json
@@ -107,8 +107,9 @@
     "build:docs": "typedoc",
     "check:bench": "node test/benchmarks/driverBench",
     "check:coverage": "nyc npm run check:test",
-    "check:lint": "npm run build:dts && npm run check:dts && npm run check:eslint",
+    "check:lint": "npm run build:dts && npm run check:dts && npm run check:eslint && npm run check:tsd",
     "check:eslint": "eslint -v && eslint --max-warnings=0 --ext '.js,.ts' src test",
+    "check:tsd": "tsd --version && tsd",
     "check:dts": "tsc --noEmit mongodb.d.ts && tsd",
     "check:test": "mocha --recursive test/functional test/unit",
     "check:ts": "tsc -v && tsc --noEmit",
@@ -124,6 +125,12 @@
     "test": "npm run check:lint && npm run check:test"
   },
   "tsd": {
-    "directory": "test/types"
+    "directory": "test/types",
+    "compilerOptions": {
+      "strict": true,
+      "target": "esnext",
+      "module": "commonjs",
+      "moduleResolution": "node"
+    }
   }
 }
diff --git a/src/collection.ts b/src/collection.ts
@@ -414,34 +414,39 @@ export class Collection<TSchema extends Document = Document> {
   updateOne(
     filter: Filter<TSchema>,
     update: UpdateFilter<TSchema> | Partial<TSchema>
-  ): Promise<UpdateResult | Document>;
+  ): Promise<UpdateResult>;
   updateOne(
     filter: Filter<TSchema>,
     update: UpdateFilter<TSchema> | Partial<TSchema>,
-    callback: Callback<UpdateResult | Document>
+    callback: Callback<UpdateResult>
   ): void;
   updateOne(
     filter: Filter<TSchema>,
     update: UpdateFilter<TSchema> | Partial<TSchema>,
     options: UpdateOptions
-  ): Promise<UpdateResult | Document>;
+  ): Promise<UpdateResult>;
   updateOne(
     filter: Filter<TSchema>,
     update: UpdateFilter<TSchema> | Partial<TSchema>,
     options: UpdateOptions,
-    callback: Callback<UpdateResult | Document>
+    callback: Callback<UpdateResult>
   ): void;
   updateOne(
     filter: Filter<TSchema>,
     update: UpdateFilter<TSchema> | Partial<TSchema>,
-    options?: UpdateOptions | Callback<UpdateResult | Document>,
-    callback?: Callback<UpdateResult | Document>
-  ): Promise<UpdateResult | Document> | void {
+    options?: UpdateOptions | Callback<UpdateResult>,
+    callback?: Callback<UpdateResult>
+  ): Promise<UpdateResult> | void {
     if (typeof options === 'function') (callback = options), (options = {});
 
     return executeOperation(
       getTopology(this),
-      new UpdateOneOperation(this as TODO_NODE_3286, filter, update, resolveOptions(this, options)),
+      new UpdateOneOperation(
+        this as TODO_NODE_3286,
+        filter,
+        update,
+        resolveOptions(this, options)
+      ) as TODO_NODE_3286,
       callback
     );
   }
@@ -685,10 +690,10 @@ export class Collection<TSchema extends Document = Document> {
   // 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<TSchema>): Promise<T | undefined>;
+  findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions): Promise<T | undefined>;
   findOne<T = TSchema>(
-    filter: Filter<T>,
+    filter: Filter<TSchema>,
     options?: FindOptions,
     callback?: Callback<T | undefined>
   ): void;
@@ -725,7 +730,7 @@ 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<T>(filter: Filter<TSchema>, options?: FindOptions): FindCursor<T>;
   find(filter?: Filter<TSchema>, options?: FindOptions): FindCursor<TSchema> {
     if (arguments.length > 2) {
       throw new MongoInvalidArgumentError(
diff --git a/src/cursor/abstract_cursor.ts b/src/cursor/abstract_cursor.ts
@@ -280,8 +280,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) {
@@ -296,9 +295,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());
@@ -311,9 +310,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());
@@ -329,10 +328,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') {
@@ -341,7 +340,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
@@ -358,7 +357,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);
@@ -402,15 +401,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);
 
@@ -420,7 +419,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);
@@ -458,11 +457,12 @@ export abstract class AbstractCursor<
    * Map all documents using the provided function
    * If there is a transform set on the cursor, that will be called first and the result passed to
    * this function's transform.
-   * @remarks
    *
-   * **NOTE:** adding a transform changes the return type of the iteration of this cursor, it **does not** return
-   * a new instance of a cursor. This means when calling map, you should always assign the result to a new
-   * variable. Take note of the following example:
+   * @remarks
+   * **Note for Typescript Users:** adding a transform changes the return type of the iteration of this cursor,
+   * it **does not** return a new instance of a cursor. This means when calling map,
+   * you should always assign the result to a new variable in order to get a correctly typed cursor variable.
+   * Take note of the following example:
    *
    * @example
    * ```typescript
diff --git a/src/cursor/aggregation_cursor.ts b/src/cursor/aggregation_cursor.ts
@@ -149,9 +149,10 @@ 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.
    *
-   * Adding a projection changes the return type of the iteration of this cursor,
+   * **Note for Typescript Users:** adding a transform 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:
+   * you should always assign the result to a new variable in order to get a correctly typed cursor variable.
+   * Take note of the following example:
    *
    * @example
    * ```typescript
diff --git a/src/cursor/find_cursor.ts b/src/cursor/find_cursor.ts
@@ -357,9 +357,10 @@ export class FindCursor<TSchema = Document> extends AbstractCursor<TSchema> {
    *
    * @remarks
    *
-   * Adding a projection changes the return type of the iteration of this cursor,
+   * **Note for Typescript Users:** adding a transform 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:
+   * you should always assign the result to a new variable in order to get a correctly typed cursor variable.
+   * Take note of the following example:
    *
    * @example
    * ```typescript
diff --git a/src/db.ts b/src/db.ts
@@ -289,7 +289,10 @@ 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');
     }
diff --git a/src/error.ts b/src/error.ts
@@ -78,6 +78,11 @@ export interface ErrorDescription extends Document {
 export class MongoError extends Error {
   /** @internal */
   [kErrorLabels]: Set<string>;
+  /**
+   * This is a number in MongoServerError and a string in MongoDriverError
+   * @privateRemarks
+   * Define the type override on the subclasses when we can use the override keyword
+   */
   code?: number | string;
   topologyVersion?: TopologyVersion;
 
@@ -132,12 +137,10 @@ export class MongoError extends Error {
  * @category Error
  */
 export class MongoServerError extends MongoError {
-  code?: number;
   codeName?: string;
   writeConcernError?: Document;
   errInfo?: Document;
   ok?: number;
-  topologyVersion?: TopologyVersion;
   [key: string]: any;
 
   constructor(message: ErrorDescription) {
@@ -164,7 +167,6 @@ export class MongoServerError extends MongoError {
  * @category Error
  */
 export class MongoDriverError extends MongoError {
-  code?: string;
   constructor(message: string) {
     super(message);
   }
diff --git a/src/operations/list_collections.ts b/src/operations/list_collections.ts
@@ -115,7 +115,9 @@ export interface CollectionInfo extends Document {
 
 /** @public */
 export class ListCollectionsCursor<
-  T extends Pick<CollectionInfo, 'name' | 'type'> | CollectionInfo = CollectionInfo
+  T extends Pick<CollectionInfo, 'name' | 'type'> | CollectionInfo =
+    | Pick<CollectionInfo, 'name' | 'type'>
+    | CollectionInfo
 > extends AbstractCursor<T> {
   parent: Db;
   filter: Document;
diff --git a/test/types/.eslintrc.json b/test/types/.eslintrc.json
@@ -24,6 +24,8 @@
         "prettier/prettier": "error",
         "tsdoc/syntax": "warn",
         "@typescript-eslint/no-explicit-any": "off",
-        "@typescript-eslint/no-unused-vars": "error"
+        "@typescript-eslint/no-unused-vars": "error",
+        "@typescript-eslint/ban-ts-comment": "off",
+        "@typescript-eslint/no-empty-function":"off"
     }
 }
diff --git a/test/types/community/collection/filterQuery.test-d.ts b/test/types/community/collection/filterQuery.test-d.ts
@@ -50,6 +50,7 @@ const collectionT = db.collection<PetModel>('test.filterQuery');
 // Assert that collection.find uses the Filter helper like so:
 const filter: Filter<PetModel> = {};
 collectionT.find(filter);
+collectionT.find(spot); // a whole model definition is also a valid filter
 // Now tests below can directly test the Filter helper, and are implicitly checking collection.find
 
 /**
@@ -226,3 +227,7 @@ await collectionT.find({ playmates: { $elemMatch: { name: 'MrMeow' } } }).toArra
 expectNotType<Filter<PetModel>>({ name: { $all: ['world', 'world'] } });
 expectNotType<Filter<PetModel>>({ age: { $elemMatch: [1, 2] } });
 expectNotType<Filter<PetModel>>({ type: { $size: 2 } });
+
+// dot key case that shows it is assignable even when the referenced key is the wrong type
+expectAssignable<Filter<PetModel>>({ 'bestFriend.name': 23 }); // using dot notation permits any type for the key
+expectNotType<Filter<PetModel>>({ bestFriend: { name: 23 } });
diff --git a/test/types/community/collection/findX.test-d.ts b/test/types/community/collection/findX.test-d.ts
@@ -102,6 +102,16 @@ expectType<{ cost: number } | undefined>(
   await collectionBag.findOne<{ cost: number }>({ color: 'red' }, { projection: { cost: 1 } })
 );
 
+// NODE-3468 The generic in find and findOne no longer affect the filter type
+type Pet = { type: string; age: number };
+const pets = db.collection<Pet>('pets');
+
+expectType<{ crazy: number }[]>(
+  await pets
+    .find<{ crazy: number }>({ type: 'dog', age: 1 })
+    .toArray()
+);
+
 interface Car {
   make: string;
 }
@@ -186,12 +196,16 @@ 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'] } });
+
 // This is a regression test that we don't remove the unused generic in FindOptions
 const findOptions: FindOptions<{ a: number }> = {};
 expectType<FindOptions>(findOptions);
diff --git a/test/types/community/cursor.test-d.ts b/test/types/community/cursor.test-d.ts
diff --git a/test/types/list_collections.test-d.ts b/test/types/list_collections.test-d.ts
diff --git a/test/types/tsconfig.json b/test/types/tsconfig.json
diff --git a/test/types/type_errors.test-d.ts b/test/types/type_errors.test-d.ts
diff --git a/test/unit/type_check.test.js b/test/unit/type_check.test.js

Original file line number	Diff line number	Diff line change
`@@ -149,9 +149,10 @@ export class AggregationCursor<TSchema = Document> extends AbstractCursor<TSchem`
`149`	`149`	`* In order to strictly type this function you must provide an interface`
`150`	`150`	`* that represents the effect of your projection on the result documents.`
`151`	`151`	`*`
`152`		`- * Adding a projection changes the return type of the iteration of this cursor,`
	`152`	`+ * Note for Typescript Users: adding a transform changes the return type of the iteration of this cursor,`
`153`	`153`	`* it does not return a new instance of a cursor. This means when calling project,`
`154`		`- * you should always assign the result to a new variable. Take note of the following example:`
	`154`	`+ * you should always assign the result to a new variable in order to get a correctly typed cursor variable.`
	`155`	`+ * Take note of the following example:`
`155`	`156`	`*`
`156`	`157`	`* @example`
`157`	`158`	* ```typescript
Original file line number	Diff line number	Diff line change
`@@ -357,9 +357,10 @@ export class FindCursor<TSchema = Document> extends AbstractCursor<TSchema> {`
`357`	`357`	`*`
`358`	`358`	`* @remarks`
`359`	`359`	`*`
`360`		`- * Adding a projection changes the return type of the iteration of this cursor,`
	`360`	`+ * Note for Typescript Users: adding a transform changes the return type of the iteration of this cursor,`
`361`	`361`	`* it does not return a new instance of a cursor. This means when calling project,`
`362`		`- * you should always assign the result to a new variable. Take note of the following example:`
	`362`	`+ * you should always assign the result to a new variable in order to get a correctly typed cursor variable.`
	`363`	`+ * Take note of the following example:`
`363`	`364`	`*`
`364`	`365`	`* @example`
`365`	`366`	* ```typescript
Original file line number	Diff line number	Diff line change
`@@ -24,6 +24,8 @@`
`24`	`24`	`"prettier/prettier": "error",`
`25`	`25`	`"tsdoc/syntax": "warn",`
`26`	`26`	`"@typescript-eslint/no-explicit-any": "off",`
`27`		`- "@typescript-eslint/no-unused-vars": "error"`
	`27`	`+ "@typescript-eslint/no-unused-vars": "error",`
	`28`	`+ "@typescript-eslint/ban-ts-comment": "off",`
	`29`	`+ "@typescript-eslint/no-empty-function":"off"`
`28`	`30`	`}`
`29`	`31`	`}`