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

[TypeScript] Usage of 'any' in HydratedDocument and Document types causes document's _id property to have 'any' type #11085

Closed
drewkht opened this issue Dec 12, 2021 · 11 comments
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@drewkht
Copy link

drewkht commented Dec 12, 2021

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

After defining an interface T, creating a schema Schema<T> by calling new Schema<T>(), then generating a model of type Model<T> by calling mongoose.model('ModelName', schema), the type of the _id property on a document created via either new ModelName() or await ModelName.create() resolves to any in most situations, instead of T['_id'] or Types.ObjectId as expected.

Based on the definition of Mongoose's internal Require_id<T> type which is used in HydratedDocument, I believe the intent is for a document's _id to never resolve to any, but rather default to Types.ObjectId when an explicit type isn't provided for _id?

This bug appears to be caused by usage of any as a default type parameter in the definitions of the Document and HydratedDocument types.

I believe it stems from how TypeScript handles a union an intersection of two types containing the same property name, where the property is any in one type and some explicit type in the other.

Edit: Forgot to add that there's one specific way of defining the interface that will produce an _id property that resolves to Types.ObjectId in the resulting documents: If the interface both includes _id: Types.ObjectId and extends Document. This is demonstrated in the full example further below. I assume that this isn't the intended behavior, as the Mongoose docs recommend against extending Document.

Example:

import { Types } from 'mongoose';

interface Type1 {
  _id?: any;
}

interface Type2 {
  _id: Types.ObjectId;
}

type IntersectionType = Type1 & Type2;

const intersectionVar: IntersectionType = { _id: new Types.ObjectId() };

intersectionVar._id;    // _id: any

However, using unknown instead of any generates a different result:

import { Types } from 'mongoose';

interface Type1 {
  _id?: unknown;
}

interface Type2 {
  _id: Types.ObjectId;
}

type IntersectionType = Type1 & Type2;

const intersectionVar: IntersectionType = { _id: new Types.ObjectId() };

intersectionVar._id;    // _id: Types.ObjectId

Essentially, if I'm understanding this right, any & T = any whereas unknown & T = T. I believe the latter behavior is what's desired in the HydratedDocument and Require_id types provided by Mongoose.

Simply editing Mongoose's index.d.ts file in node_modules as follows seems to resolve the problem in my testing, but I'm not sure what sort of cascading changes this might cause elsewhere:

Before:

export type HydratedDocument<DocType, TMethods = {}, TVirtuals = {}> = DocType extends Document ? Require_id<DocType>  :  (Document<any, any, DocType> & Require_id<DocType> & TVirtuals & TMethods);

class Document<T = any, TQueryHelpers = any, DocType = any> {
    ...
}

After:

export type HydratedDocument<DocType, TMethods = {}, TVirtuals = {}> = DocType extends Document ? Require_id<DocType>  :  (Document<unknown, any, DocType> & Require_id<DocType> & TVirtuals & TMethods);

class Document<T = unknown, TQueryHelpers = any, DocType = any> {
    ...
}

If the current behavior is a bug, please provide the steps to reproduce.

import { model, Schema, Types, Document } from 'mongoose';

interface User {
  username: string;
  email: string;
}

interface UserDocument extends Document {
  username: string;
  email: string;
}

interface UserWithId {
  _id: Types.ObjectId;
  username: string;
  email: string;
}

interface UserWithStringId {
  _id: string;
  username: string;
  email: string;
}

interface UserWithIdDocument extends Document {
  _id: Types.ObjectId;
  username: string;
  email: string;
}

interface UserWithStringIdDocument extends Document {
  _id: string;
  username: string;
  email: string;
}

const userSchema = new Schema<User>({
  username: String,
  email: String,
});

const userDocumentSchema = new Schema<UserDocument>({
  username: String,
  email: String,
});

const userWithIdSchema = new Schema<UserWithId>({
  username: String,
  email: String,
});

const userWithStringIdSchema = new Schema<UserWithStringId>({
  username: String,
  email: String,
});

const userWithIdDocumentSchema = new Schema<UserWithIdDocument>({
  username: String,
  email: String,
});

const userWithStringIdDocumentSchema = new Schema<UserWithStringIdDocument>({
  username: String,
  email: String,
});

const UserModel = model('User', userSchema);
const UserWithIdModel = model('UserWithId', userWithIdSchema);
const UserWithStringIdModel = model('UserWithStringId', userWithStringIdSchema);
const UserDocumentModel = model('UserDocument', userDocumentSchema);
const UserWithIdDocumentModel = model(
  'UserWithIdDocument',
  userWithIdDocumentSchema
);
const UserWithStringIdDocumentModel = model(
  'UserWithStringIdDocument',
  userWithStringIdDocumentSchema
);

const newUser = new UserModel();
const newUserWithId = new UserWithIdModel();
const newUserWithStringId = new UserWithStringIdModel();
const newUserDocument = new UserDocumentModel();
const newUserWithIdDocument = new UserWithIdDocumentModel();
const newUserWithStringIdDocument = new UserWithStringIdDocumentModel();

newUser._id; // _id: any
newUserWithId._id; // _id: any
newUserWithStringId._id; // _id: any
newUserDocument._id; // _id: any
newUserWithIdDocument._id; // _id: Types.ObjectId
newUserWithStringIdDocument._id; // _id: string

tsconfig.json:

{
    "compilerOptions": {
      "allowJs": true,
      "allowSyntheticDefaultImports": true,
      "alwaysStrict": true,
      "baseUrl": "src",
      "declaration": true,
      "declarationMap": true,
      "esModuleInterop": true,
      "forceConsistentCasingInFileNames": true,
      "importHelpers": true,
      "incremental": true,
      "isolatedModules": true,
      "lib": [
        "dom",
        "dom.iterable",
        "ESNext"
      ],
      "module": "ESNext",
      "moduleResolution": "node",
      "noEmit": true,
      "noFallthroughCasesInSwitch": true,
      "noImplicitAny": true,
      "noImplicitThis": true,
      "noUncheckedIndexedAccess": true,
      "resolveJsonModule": true,
      "skipLibCheck": true,
      "sourceMap": true,
      "strict": true,
      "target": "ESNext",
      "noErrorTruncation": true
    },
    "exclude": [
      "node_modules",
      "**/node_modules"
    ],
    "include": [
      "src"
    ],
    "ts-node": {
      "compilerOptions": {
        "module": "CommonJS"
      },
      "experimentalReplAwait": true,
      "files": true,
      "transpileOnly": true
    }
  }

What is the expected behavior?

Assuming we have an interface of type T, used to create a schema by calling new Schema<T>(), which is passed to mongoose.model('ModelName', schema) to create a model of type Model<T>, a new document created by either new ModelName() or await ModelName.create() should have an _id property of either type T['_id'] if T extends { _id?: any } or Types.ObjectId otherwise.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Node: 16.10.0
TypeScript: 4.5.3
Mongoose: 6.1.1
MongoDB: 4.2.1

Edit 2: Realized I've been referring to unions when I'm actually talking about intersections. Always reverse those two mentally, oops.

@drewkht
Copy link
Author

drewkht commented Dec 16, 2021

Another issue I think I found in the course of testing this stuff is that it looks like any types that extend Document aren't unioned intersected with the TMethods and TVirtuals types passed to HydratedDocument from Model.new, which means TypeScript won't see instance methods on documents created by that model.

This may be intended, as I know the Mongoose docs advise against extending Document in document interfaces because that can make it hard for TypeScript to infer certain properties (which is why I discovered the issue described above in the first place - I was trying to avoid extending Document but kept having _id resolve to any when doing so).

Example:

interface IBook extends Document {
  _id: Types.ObjectId;
  author: string;
  title: string;
}

interface BookInstanceMethods {
  titleAndAuthor: (this: IBook) => string;
}

type BookModel = Model<IBook, Record<string, unknown>, BookInstanceMethods>;

const bookSchema = new Schema<IBook, BookModel, BookInstanceMethods>({
  author: String,
  title: String
});

bookSchema.method('titleAndAuthor', function (this: IBook) {
  return `'${this.title}' by ${this.author}`;
});

const Book = model('Book', bookSchema);

const book = new Book();

book.titleAndAuthor();  // titleAndAuthor is undefined
book._id; // _id is Types.ObjectId

In this example, "_id" is correctly recognized as an ObjectId by TypeScript because an '_id' property was included in IBook and IBook extends Document (as illustrated in the issue above). However, the titleAndAuthor instance method is seen as undefined by TypeScript.

Behind the scenes what appears to be happening is: When new Book() is called Book.new returns HydratedDocument<IBook, BookInstancesMethods, TVirtuals> which, per the definition of HydratedDocument, resolves to simply Require_id<IBook> (because IBook extends Document). In turn, this resolves to IBook & { _id: IBook['_id'] } and therefore the instance method isn't unionedintersected into the final result.

If the example is changed only by removing extends Document from IBook:

interface IBook {
  _id: Types.ObjectId;
  author: string;
  title: string;
}

TypeScript now correctly recognizes the titleAndAuthor instance method on new Documents created via the Book model, but _id is now resolving to any again.

If I modify the definition of HydratedDocument by simply changing the parentheses it solves both problems (though the issues mentioned in the main issue above still exist).

Before:

export type HydratedDocument<
  DocType,
  TMethods = {},
  TVirtuals = {}
> = DocType extends Document
  ? Require_id<DocType>
  : (Document<any, any, DocType> & Require_id<DocType> & TVirtuals & TMethods);

After:

export type HydratedDocument<
  DocType,
  TMethods = {},
  TVirtuals = {}
> = (DocType extends Document
  ? Require_id<DocType>
  : Document<any, any, DocType> & Require_id<DocType>) &
  TVirtuals &
  TMethods;

It could also be simplified a bit to not repeat Require_id if desired:

export type HydratedDocument<
  DocType,
  TMethods = {},
  TVirtuals = {}
> = Require_id<DocType> &
  (DocType extends Document ? {} : Document<any, any, DocType>) &
  TVirtuals &
  TMethods;

If this actually is unintended behavior it might deserve its own issue, which I'd be happy to submit.

@IslandRhythms IslandRhythms added the can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. label Dec 16, 2021
@IslandRhythms
Copy link
Collaborator

newUser._id; // _id: any
newUserWithId._id; // _id: any
newUserWithStringId._id; // _id: any
newUserDocument._id; // _id: any
newUserWithIdDocument._id; // _id: Types.ObjectId
newUserWithStringIdDocument._id; // _id: any

for me is instead

newUser._id; // _id: Types.ObjectId
newUserWithId._id; // _id: Types.ObjectId
newUserWithStringId._id; // _id: string
newUserDocument._id; // _id: any
newUserWithIdDocument._id; // _id: Types.ObjectId
newUserWithStringIdDocument._id; // _id: string

@drewkht
Copy link
Author

drewkht commented Dec 16, 2021

Hm, well that's embarrassing. Were you using the same versions of everything that I was, or if not could you share your versions so I can test with those?

Edit:
I just upgraded various packages and tested with the following:
Node: 17.2.0
Mongoose: 6.1.2
TypeScript: 4.5.4
MongoDb: 4.2.2 (from Mongoose dependency)

I'm still getting the same results as before. Maybe I should've been more specific instead of assuming it'd be implied, but I'm using the VS Code (version 1.64.0-insider) intellisense popups to determine these type results, and the actual compile errors are being produced by both the VS Code ESLint extension and when I try to execute the code using ts-node.

@drewkht
Copy link
Author

drewkht commented Dec 17, 2021

Also, I'm curious if you get the same result from the first, smaller example I gave:

import { Types } from 'mongoose';

interface Type1 {
  _id?: any;
}

interface Type2 {
  _id: Types.ObjectId;
}

type IntersectionType = Type1 & Type2;

const intersectionVar: IntersectionType = { _id: new Types.ObjectId() };

intersectionVar._id;    // _id: any

If _id successfully resolves to Types.ObjectId for you there, might it be some difference in our tsconfig.json?

@drewkht
Copy link
Author

drewkht commented Dec 17, 2021

I managed to find this several-year-old issue on the TypeScript GitHub where one of the authors of the project says T & any = any is the intended behavior. Not sure if anything's changed in the four years since.

Nevermind - this is talking about unions while we're dealing with intersections, so may not be relevant. I've been reversing the two in my mind. Both unions and intersections seem to have the same behavior in this case, though.

@drewkht
Copy link
Author

drewkht commented Dec 17, 2021

Sorry to keep making further comments/edits, I'm just trying to figure this out as it's affecting the project I'm working on.

Here's a repro on the TypeScript playground showing the same Intellisense results I'm seeing in my VS Code environment. It's just using their default tsconfig settings on there.

@DavideViolante
Copy link
Contributor

If you add extends Document on interface UserWithId you get the correct type for newUserWithId._id;
Complete example, try with and without extends Document

interface UserWithId extends Document {
  _id: Types.ObjectId;
  username: string;
  email: string;
}

const userWithIdSchema = new Schema<UserWithId>({
  username: String,
  email: String,
});

const UserWithIdModel = model('UserWithId', userWithIdSchema);

const newUserWithId = new UserWithIdModel();

newUserWithId._id; // Check type of _id

Docs does not recommend extending Document though, but I can't get why. On my projects I always had extended.

@drewkht
Copy link
Author

drewkht commented Dec 27, 2021

@DavideViolante Yeah, exactly. I only encountered the issue because I was trying to do as the Mongoose documentation suggests and avoid extending Document. The examples in the docs also omit the _id property from their interfaces, which causes type problems as well (that's why I included so many different interfaces in my example above).

Currently the only way I can get _id to have the right type in a document created from or queried by a model is by including { _id: Types.ObjectId } in the interface while also extending Document as mentioned.

@vkarpov15 vkarpov15 added this to the 6.1.7 milestone Dec 30, 2021
@vkarpov15 vkarpov15 removed the can't reproduce Mongoose devs have been unable to reproduce this issue. Close after 14 days of inactivity. label Dec 30, 2021
@IslandRhythms
Copy link
Collaborator

@drewkht On the smaller example it resolves to _id: any as oppossed to types.objectId like it should.

@IslandRhythms IslandRhythms added the typescript Types or Types-test related issue / Pull Request label Jan 6, 2022
@vkarpov15
Copy link
Collaborator

Re: OP's first comment, we'll apply the any -> unknown change you suggested

Re: OP's 2nd comment, with the any -> unknown change applied the script you provided works, you just need to pass generics to model<>:

import { model, Schema, Types, Document, Model } from 'mongoose';

interface IBook {
  _id: Types.ObjectId;
  author: string;
  title: string;
}

interface BookInstanceMethods {
  titleAndAuthor: (this: IBook) => string;
}

type BookModel = Model<IBook, Record<string, unknown>, BookInstanceMethods>;

const bookSchema = new Schema<IBook, BookModel, BookInstanceMethods>({
  author: String,
  title: String
});

bookSchema.method('titleAndAuthor', function (this: IBook) {
  return `'${this.title}' by ${this.author}`;
});

const Book = model<IBook, BookModel>('Book', bookSchema); // <-- note the generics here

const book = new Book();

book.titleAndAuthor();  // titleAndAuthor is undefined
book._id; // _id is Types.ObjectId

// "Type 'ObjectId' is not assignable to type 'number'." as expected
const _id: number = book._id;

It is a bit tricky to test whether _id is any, because intellisense can be a bit unreliable. The best way I've found to test that _id isn't any is to try to assign it to a different type. If _id: any, const _id: number = book._id compiles fine. But if _id: Types.ObjectId, const _id: number = book._id fails.

@DavideViolante
Copy link
Contributor

DavideViolante commented Jan 13, 2022

Thanks, just for the records, the example above works now:

import { Types, Schema, model, Model } from 'mongoose'

interface UserWithId {
  _id: Types.ObjectId;
  username: string;
  email: string;
}

type UserModel = Model<UserWithId, Record<string, unknown>>;

const userWithIdSchema = new Schema<UserWithId, UserModel>({
  username: String,
  email: String,
});

const UserWithIdModel = model<UserWithId, UserModel>('UserWithId', userWithIdSchema);

const newUserWithId = new UserWithIdModel();

newUserWithId._id; // _id is Types.ObjectId

(even without using UserModel type)
info: this is not released yet, I just edited index.d.ts like the commit above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

4 participants