fix(NODE-3803): Fix _id typing on collection create operations (#3077)

Author: baileympearson

src/collection.ts

@@ -12,7 +12,7 @@ import type { PkFactory } from './mongo_client';
 import type {
   Filter,
   Flatten,
-  OptionalId,
+  OptionalUnlessRequiredId,
   TODO_NODE_3286,
   UpdateFilter,
   WithId,
@@ -264,16 +264,22 @@ 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
    */
-  insertOne(doc: OptionalId<TSchema>): Promise<InsertOneResult<TSchema>>;
-  insertOne(doc: OptionalId<TSchema>, callback: Callback<InsertOneResult<TSchema>>): void;
-  insertOne(doc: OptionalId<TSchema>, options: InsertOneOptions): Promise<InsertOneResult<TSchema>>;
+  insertOne(doc: OptionalUnlessRequiredId<TSchema>): Promise<InsertOneResult<TSchema>>;
   insertOne(
-    doc: OptionalId<TSchema>,
+    doc: OptionalUnlessRequiredId<TSchema>,
+    callback: Callback<InsertOneResult<TSchema>>
+  ): void;
+  insertOne(
+    doc: OptionalUnlessRequiredId<TSchema>,
+    options: InsertOneOptions
+  ): Promise<InsertOneResult<TSchema>>;
+  insertOne(
+    doc: OptionalUnlessRequiredId<TSchema>,
     options: InsertOneOptions,
     callback: Callback<InsertOneResult<TSchema>>
   ): void;
   insertOne(
-    doc: OptionalId<TSchema>,
+    doc: OptionalUnlessRequiredId<TSchema>,
     options?: InsertOneOptions | Callback<InsertOneResult<TSchema>>,
     callback?: Callback<InsertOneResult<TSchema>>
   ): Promise<InsertOneResult<TSchema>> | void {
@@ -308,19 +314,22 @@ 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
    */
-  insertMany(docs: OptionalId<TSchema>[]): Promise<InsertManyResult<TSchema>>;
-  insertMany(docs: OptionalId<TSchema>[], callback: Callback<InsertManyResult<TSchema>>): void;
+  insertMany(docs: OptionalUnlessRequiredId<TSchema>[]): Promise<InsertManyResult<TSchema>>;
+  insertMany(
+    docs: OptionalUnlessRequiredId<TSchema>[],
+    callback: Callback<InsertManyResult<TSchema>>
+  ): void;
   insertMany(
-    docs: OptionalId<TSchema>[],
+    docs: OptionalUnlessRequiredId<TSchema>[],
     options: BulkWriteOptions
   ): Promise<InsertManyResult<TSchema>>;
   insertMany(
-    docs: OptionalId<TSchema>[],
+    docs: OptionalUnlessRequiredId<TSchema>[],
     options: BulkWriteOptions,
     callback: Callback<InsertManyResult<TSchema>>
   ): void;
   insertMany(
-    docs: OptionalId<TSchema>[],
+    docs: OptionalUnlessRequiredId<TSchema>[],
     options?: BulkWriteOptions | Callback<InsertManyResult<TSchema>>,
     callback?: Callback<InsertManyResult<TSchema>>
   ): Promise<InsertManyResult<TSchema>> | void {
@@ -1526,7 +1535,7 @@ export class Collection<TSchema extends Document = Document> {
    * @param callback - An optional callback, a Promise will be returned if none is provided
    */
   insert(
-    docs: OptionalId<TSchema>[],
+    docs: OptionalUnlessRequiredId<TSchema>[],
     options: BulkWriteOptions,
     callback: Callback<InsertManyResult<TSchema>>
   ): Promise<InsertManyResult<TSchema>> | void {

src/index.ts

@@ -286,11 +286,13 @@ export type {
   KeysOfAType,
   KeysOfOtherType,
   MatchKeysAndValues,
+  NonObjectIdLikeDocument,
   NotAcceptedFields,
   NumericType,
   OneOrMore,
   OnlyFieldsOfType,
   OptionalId,
+  OptionalUnlessRequiredId,
   Projection,
   ProjectionOperators,
   PullAllOperator,

src/mongo_types.ts

@@ -1,3 +1,4 @@
+import type { ObjectIdLike } from 'bson';
 import { EventEmitter } from 'events';
 
 import type {
@@ -17,13 +18,15 @@ import type { Sort } from './sort';
 export type TODO_NODE_3286 = any;
 
 /** Given an object shaped type, return the type of the _id field or default to ObjectId @public */
-export type InferIdType<TSchema> = TSchema extends { _id: infer IdType } // user has defined a type for _id
-  ? // eslint-disable-next-line @typescript-eslint/ban-types
-    {} extends IdType // TODO(NODE-3285): Improve type readability
-    ? // eslint-disable-next-line @typescript-eslint/ban-types
-      Exclude<IdType, {}>
-    : unknown extends IdType
-    ? ObjectId
+export type InferIdType<TSchema> = TSchema extends { _id: infer IdType }
+  ? // user has defined a type for _id
+    Record<any, never> extends IdType
+    ? never // explicitly forbid empty objects as the type of _id
+    : IdType
+  : TSchema extends { _id?: infer IdType }
+  ? // optional _id defined - return ObjectId | IdType
+    unknown extends IdType
+    ? ObjectId // infer the _id type as ObjectId if the type of _id is unknown
     : IdType
   : ObjectId; // user has not defined _id on schema
 
@@ -33,17 +36,23 @@ export type WithId<TSchema> = EnhancedOmit<TSchema, '_id'> & { _id: InferIdType<
 /**
  * Add an optional _id field to an object shaped type
  * @public
+ */
+export type OptionalId<TSchema> = EnhancedOmit<TSchema, '_id'> & { _id?: InferIdType<TSchema> };
+
+/**
+ * Adds an optional _id field to an object shaped type, unless the _id field is requried on that type.
+ * In the case _id is required, this method continues to require_id.
+ *
+ * @public
  *
  * @privateRemarks
  * `ObjectId extends TSchema['_id']` is a confusing ordering at first glance. Rather than ask
  * `TSchema['_id'] extends ObjectId` which translated to "Is the _id property ObjectId?"
  * we instead ask "Does ObjectId look like (have the same shape) as the _id?"
  */
-export type OptionalId<TSchema> = TSchema extends { _id?: any }
-  ? ObjectId extends TSchema['_id'] // a Schema with ObjectId _id type or "any" or "indexed type" provided
-    ? EnhancedOmit<TSchema, '_id'> & { _id?: InferIdType<TSchema> } // a Schema provided but _id type is not ObjectId
-    : WithId<TSchema>
-  : EnhancedOmit<TSchema, '_id'> & { _id?: InferIdType<TSchema> }; // TODO(NODE-3285): Improve type readability
+export type OptionalUnlessRequiredId<TSchema> = TSchema extends { _id: any }
+  ? TSchema
+  : OptionalId<TSchema>;
 
 /** TypeScript Omit (Exclude to be specific) does not work for objects with an "any" indexed type, and breaks discriminated unions @public */
 export type EnhancedOmit<TRecordOrUnion, KeyUnion> = string extends keyof TRecordOrUnion
@@ -91,8 +100,16 @@ export interface RootFilterOperators<TSchema> extends Document {
   $comment?: string | Document;
 }
 
+/**
+ * @public
+ * A type that extends Document but forbids anything that "looks like" an object id.
+ */
+export type NonObjectIdLikeDocument = {
+  [key in keyof ObjectIdLike]?: never;
+} & Document;
+
 /** @public */
-export interface FilterOperators<TValue> extends Document {
+export interface FilterOperators<TValue> extends NonObjectIdLikeDocument {
   // Comparison
   $eq?: TValue;
   $gt?: TValue;

test/types/basic_schema.test-d.ts

@@ -1,4 +1,4 @@
-import { expectAssignable, expectNotType, expectType } from 'tsd';
+import { expectAssignable, expectNotAssignable, expectNotType, expectType } from 'tsd';
 
 import { ObjectId } from '../../src/bson';
 import { Collection } from '../../src/collection';
@@ -31,7 +31,7 @@ expectNotType<InsertOneArgOf<ACounter>>({ a: 2, b: 34 });
 // With _id
 expectAssignable<InsertOneArgOf<ACounterWithId>>({ _id: new ObjectId(), a: 3 });
 // Without _id
-expectAssignable<InsertOneArgOf<ACounterWithId>>({ a: 3 });
+expectNotAssignable<InsertOneArgOf<ACounterWithId>>({ a: 3 });
 // Does not permit extra keys
 expectNotType<InsertOneArgOf<ACounterWithId>>({ a: 2, b: 34 });
 

test/types/community/collection/filterQuery.test-d.ts

@@ -1,7 +1,7 @@
 import { BSONRegExp, Decimal128, ObjectId } from 'bson';
-import { expectAssignable, expectNotType, expectType } from 'tsd';
+import { expectAssignable, expectError, expectNotType, expectType } from 'tsd';
 
-import { Filter, MongoClient, WithId } from '../../../../src';
+import { Collection, Filter, MongoClient, WithId } from '../../../../src';
 
 /**
  * test the Filter type using collection.find<T>() method
@@ -236,3 +236,51 @@ 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 } });
+
+// ObjectId are not allowed to be used as a query predicate (issue described here: NODE-3758)
+// this only applies to schemas where the _id is not of type ObjectId.
+declare const nonObjectIdCollection: Collection<{ _id: number; otherField: string }>;
+expectError(
+  nonObjectIdCollection.find({
+    _id: new ObjectId()
+  })
+);
+expectError(
+  nonObjectIdCollection.find({
+    otherField: new ObjectId()
+  })
+);
+nonObjectIdCollection.find({
+  fieldThatDoesNotExistOnSchema: new ObjectId()
+});
+
+// we only forbid objects that "look like" object ids, so other random objects are permitted
+nonObjectIdCollection.find({
+  _id: {
+    hello: 'world'
+  }
+});
+nonObjectIdCollection.find({
+  otherField: {
+    hello: 'world'
+  }
+});
+
+declare const nonSpecifiedCollection: Collection;
+nonSpecifiedCollection.find({
+  _id: new ObjectId()
+});
+nonSpecifiedCollection.find({
+  otherField: new ObjectId()
+});
+
+nonSpecifiedCollection.find({
+  _id: {
+    hello: 'world'
+  }
+});
+nonSpecifiedCollection.find({
+  otherField: {
+    hello: 'world'
+  }
+});

test/types/community/collection/insertX.test-d.ts

@@ -123,12 +123,35 @@ await collectionWithObjectId.insertOne({
   numberField: 23,
   fruitTags: ['hi']
 });
-// should not demand _id if it is ObjectId
-await collectionWithObjectId.insertOne({
-  stringField: 'hola',
-  numberField: 23,
-  fruitTags: ['hi']
-});
+// if _id is defined on the schema, it must be passed to insert operations
+expectError(
+  collectionWithObjectId.insertOne({
+    stringField: 'hola',
+    numberField: 23,
+    fruitTags: ['hi']
+  })
+);
+
+// defined _id's that are not of type ObjectId cannot be cast to ObjectId
+const collectionWithRequiredNumericId =
+  db.collection<{ _id: number; otherField: string }>('testCollection');
+
+expectError(
+  collectionWithRequiredNumericId.insertOne({
+    _id: new ObjectId(),
+    otherField: 'hola'
+  })
+);
+
+const collectionWithOptionalNumericId =
+  db.collection<{ _id?: number; otherField: string }>('testCollection');
+
+expectError(
+  collectionWithOptionalNumericId.insertOne({
+    _id: new ObjectId(),
+    otherField: 'hola'
+  })
+);
 
 /**
  * test indexed types

test/types/schema_helpers.test-d.ts

@@ -1,10 +1,11 @@
 import { Document, ObjectId } from 'bson';
-import { expectAssignable, expectNotType, expectType } from 'tsd';
+import { expectAssignable, expectError, expectNotType, expectType } from 'tsd';
 
 import type {
   EnhancedOmit,
   InferIdType,
   OptionalId,
+  OptionalUnlessRequiredId,
   WithId,
   WithoutId
 } from '../../src/mongo_types';
@@ -13,6 +14,11 @@ import type {
 expectType<InferIdType<Document>>(new ObjectId());
 expectType<InferIdType<{ _id: number }>>(1 + 1);
 expectType<InferIdType<{ a: number } | { b: string }>>(new ObjectId());
+expectType<InferIdType<{ _id?: number }>>(1 + 1);
+expectType<InferIdType<{ _id?: unknown }>>(new ObjectId());
+expectError<InferIdType<{ _id: Record<string, any> }>>({});
+
+// union types could have an id of either type
 expectAssignable<InferIdType<{ _id: number } | { b: string }>>(new ObjectId());
 expectAssignable<InferIdType<{ _id: number } | { b: string }>>(1 + 1);
 
@@ -38,6 +44,14 @@ class MyId {}
 expectNotType<OptionalId<{ _id: MyId; a: number }>>({ a: 3 });
 expectNotType<OptionalId<{ _id: MyId; a: number }>>({ _id: new ObjectId(), a: 3 });
 
+declare function functionReturningOptionalId(): OptionalId<{
+  _id?: ObjectId | undefined;
+  a: number;
+}>;
+// OptionalUnlessRequiredId
+expectType<OptionalUnlessRequiredId<{ _id: ObjectId; a: number }>>({ a: 3, _id: new ObjectId() });
+expectType<OptionalUnlessRequiredId<{ _id?: ObjectId; a: number }>>(functionReturningOptionalId());
+
 // WithoutId removes _id whether defined in the schema or not
 expectType<WithoutId<{ _id: number; a: number }>>({ a: 2 });
 expectNotType<WithoutId<{ _id: number; a: number }>>({ _id: 3, a: 2 });

test/types/union_schema.test-d.ts

@@ -18,7 +18,7 @@ interface Rectangle {
 type Shape = Circle | Rectangle;
 
 type ShapeInsert = InsertOneFirstParam<Shape>;
-expectAssignable<ShapeInsert>({ height: 2, width: 2, radius: 2 }); // This is permitted...
+expectNotAssignable<ShapeInsert>({ height: 2, width: 2, radius: 2 }); // This is not permitted...
 // error cases, should not insert a portion of a type
 expectNotAssignable<ShapeInsert>({ height: 2 });
 expectError<ShapeInsert>({
@@ -27,8 +27,8 @@ expectError<ShapeInsert>({
   _id: new ObjectId()
 });
 // valid cases
-expectAssignable<ShapeInsert>({ height: 4, width: 4 });
-expectAssignable<ShapeInsert>({ radius: 4 });
+expectAssignable<ShapeInsert>({ height: 4, width: 4, _id: new ObjectId() });
+expectAssignable<ShapeInsert>({ radius: 4, _id: new ObjectId() });
 
 const c: Collection<Shape> = null as never;
 expectType<Promise<WithId<Shape> | null>>(c.findOne({ height: 4, width: 4 }));