LeanDocument Type doesn't include _id or id

[noseworthy]

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

What is the current behavior?
I believe that the types defined here are incorrect:

mongoose/types/index.d.ts

Lines 2261 to 2267 in c22152c

	/**
	* Documents returned from queries with the lean option enabled.
	* Plain old JavaScript object documents (POJO).
	* @see https://mongoosejs.com/docs/tutorials/lean.html
	*/
	export type LeanDocument<T> = BaseDocumentType<T> extends Document ? _LeanDocument<BaseDocumentType<T>> :
	Omit<_LeanDocument<T>, Exclude<keyof Document, '_id' | 'id' | '__v'> | '$isSingleNested'>;

This says that LeanDocuments (that is documents returned from .toObject() or when using the lean option don't have the _id or id fields. I don't think this is correct.

This issue only presented itself in version 6.3.2 and wasn't present in 6.3.1

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

import mongoose from 'mongoose'

async function test() {
  const thingSchema = new mongoose.Schema({
    name: mongoose.Schema.Types.String,
  });

  const ThingModel = mongoose.model('Thing', thingSchema);

  // TypeScript will complain here that `_id` doesn't exist
  const { _id, ...thing1 } = (await ThingModel.create({name: 'Thing 1'})).toObject();

  // if I ignore the above warning and this log happens, _id is a valid ObjectId.
  console.log({_id, thing1});
}

What is the expected behavior?

_id and id are present on LeanDocuments (I think...)
What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

Node.js: v16.15.0
TypeScript: v4.6.2
mongoose: v6.3.2

[Uzlopak]

I think, that id is not on a lean document if you did not define id in the schema explicitly, as id is actually a virtual field.

@taxilian

[Uzlopak]

I guess you can avoid the bug for now, by explicitly defining _id: Schema.Types.ObjectId in your Schema.

[noseworthy]

Yeah, I'm just @ts-ignore-ing the few errors we're getting for now. However, I don't feel like we should be required to define _id in the schema. The POJO that we get back has the _id on it regardless of the schema definition. I think the TypeScript type should reflect that.

That said, I don't have super deep understanding of the inner workings of all this and I could be missing something. 🤷‍♂️

It's why I raised a bug rather than open an MR. Didn't feel comfortable mucking about on this one, haha. Thanks for taking a look at it! 😁

[taxilian]

This is one of those things which could be fixed, but there are a lot of potential issues that it can cause; I would strongly suggest that you just define _id yourself in your schema. It's a simple one-time fix and it removes a lot of complexity in the typescript

[Uzlopak]

This is a clear regression. This is comparable to when Apple had antenna issues with their iphones, if you hold the IPhones in your hand and they basically said, that it is the consumers fault, because the consumer hold the iPhone "wrong"...

This is not acceptable.

@mohammad0-0ahmad
We need to fix this. Any idea how we can fix that in a timely manner?

[mohammad0-0ahmad]

I've tested the code snippet that is provided by @noseworthy, but I couldn't reproduce TS warning because the returned value is "any".

but If a schema type is provided as first generic in schema constructor, then I get the TS warning.

[taxilian]

Sorry, I'm looking more closely at this and there are several things that don't make sense.

First, the assumption stated in the issue is incorrect: "This says that LeanDocuments (that is documents returned from .toObject() or when using the lean option don't have the _id or id fields." -- that's actually exactly opposite. The reference to _id is in a double negative -- it's telling it to omit any fields from Document except those ones.

I'm not claiming that there is no issue here because clearly someone is seeing an issue, but I can't reproduce it either because -- like @mohammad0-0ahmad says -- the type here is any. If someone can show a unit test reproducing the issue I'd be happy to take a look at it, but the problem is absolutely not what is postulated in the issue description because that code was there before I meddled.

[mohammad0-0ahmad]

The regression has appeared after merging this commit e4b007b3fb2f6a92a4e33579f3757b8964613b1f
@vkarpov15 might take a look before opening a PR to solve this issue.

[mohammad0-0ahmad]

@taxilian I think he forgot to provide schema type as generic in the code snippet as I said above.

[taxilian]

I very very strongly recommend against making it add _id in that case. it's up to you, but in the long run it's going to add a nontrivial amount of complexity which far exceeds the problems that it solves.

The benefit is not worth the cost.

[Uzlopak]

I respectfully disagree. We dont break user space without making a major release. Now we force devs to patch their schemas or write ts-ignore. Also it would mean that the typings do not represent the javascript reality.

@mohammad0-0ahmad Would the autotyping PR fix this implicitly?

[ghost91-]

Not sure if this is strictly related, but the change to the types here also broke some stuff for us. In particular making the return type of toObject always the LeanDocument<T> breaks stuff for us, since we are using a generic type ourselves for T, and that doesn't match up with our types now anymore.

The most confusing thing is that this is now actually inconsistent between .lean<T>() ans .toObject<T>(), because the former actually resolves to T (when the query is awaited), but the latter resolves to LeanDocument<T>.

[mohammad0-0ahmad]

Hello @taxilian !
I've created PR to solve this issue, and it's fixed so far. To be honest, there are a lot to do to improve types, but It just a quick fix.
I've deleted some unnecessary types "I think so" that you have created in your latest PR, I guess it would be great if you review the changes in the mentioned PR.

[mohammad0-0ahmad]

Not sure if this is strictly related, but the change to the types here also broke some stuff for us. In particular making the return type of toObject always the LeanDocument<T> breaks stuff for us, since we are using a generic type ourselves for T, and that doesn't match up with our types now anymore.

The most confusing thing is that this is now actually inconsistent between .lean<T>() ans .toObject<T>(), because the former actually resolves to T (when the query is awaited), but the latter resolves to LeanDocument<T>.

Hi @ghost91- !
Thanks for your comment, Can you please provide code snippet ?

[mohammad0-0ahmad]

I respectfully disagree. We dont break user space without making a major release. Now we force devs to patch their schemas or write ts-ignore. Also it would mean that the typings do not represent the javascript reality.

@mohammad0-0ahmad Would the autotyping PR fix this implicitly?

I've created new PR instead.😉

[ghost91-]

Hi @ghost91- ! Thanks for your comment, Can you please provide code snippet ?

This is a dumbed down example of what we are doing:

class GenericRepository<T extends { id: string }> {
  constructor(private readonly GenericModel: Model<T>) {}

  public async create(t: T): Promise<T> {
    const document = new this.GenericModel(t);
    await document.save();
    return document.toObject<T>(); // worked in 6.3.1, broken in 6.3.2, as it returns `LeanDocument<T>` instead of `T` now, which might not be compatible with `T`
  }

  public async update(t: T): Promise<T | null> {
    const document = await this.GenericModel.findOneAndUpdate({ id: t.id }, t).lean<T | null>(); // still OK in 6.3.2
    return document;
  }
}

The underlying problem is that TypeScript doesn't know that T does not have any properties that are omitted by LeanDocument. Unfortunately it's not possible in TypeScript to create a constraint to T so that it can not have any of these properties (due the nature of the structural type system).

I realize that the new way may actually be a little more correct (not sure what would happen at runtime if a schema defined properties that clash with things added by Document). But it's weird that the typing works differently for lean and toObject. The old way was a handy way for us to get what we need, but I suppose we could work around the new way with a simple type assertion. But in that case, I think it should work the same for both lean and toObject.

[mohammad0-0ahmad]

After merging mentioned PR, you will be able to do like following:

  class GenericRepository<T extends { id: string }> {
    constructor(private readonly GenericModel: Model<T>) {}

    public async create(t: T): Promise<LeanDocument<T>> { // Wrap T with LeanDocument
      const document = new this.GenericModel(t);
      await document.save();
      return document.toObject<T>();
    }

    public async update(t: T): Promise<T | null> {
      const document = await this.GenericModel.findOneAndUpdate({ id: t.id }, t).lean<T | null>();
      return document;
    }
  }

What do you think about that @ghost91-?

[ghost91-]

The point is that we want to return Promise<T>, not Promise<LeanDocument<T>>, since we know that there will be no conflicting properties. If that requires a type assertion, that's probably acceptable.

Maybe to give a bit of context on why we don't want to use LeanDocument: It would be leaking an implementation detail. The repository is suppsed to completely encapsulate our usage of mongoose, from outside, you shouldn't need to know that it's used under the hood (which is why we use lean queries and not the Documents). but if we put LeanDocument into the return type, that implementation is leaked to consumers.

Anyways, I still think lean and toObject should work the same in this regard, and right now, they don't. They basically fulfill the same purpose, so I really think they should provide the same interface (on a type level). I mean, why would there be a difference between

const object  = await this.GenericModel.findOneAndUpdate({ id: t.id }, t).lean<T | null>(); // currently of type T | null

and

const object = (await this.GenericModel.findOneAndUpdate({ id: t.id }, t)).toObject<T | null>(); // currently of type LeanDocument<T | null>

?

[mohammad0-0ahmad]

@ghost91-
According to the docs, lean() and toObject() does not return same type.
Take a look:
https://mongoosejs.com/docs/api.html#document_Document-toObject
https://mongoosejs.com/docs/api.html#query_Query-lean
Docs say as well that it should be POJO, It is confusing.
@Uzlopak @vkarpov15 can help with that and check this example here:
https://mongoosejs.com/docs/tutorials/lean.html#lean-and-populate

[noseworthy]

Woah, this blew up over night. Thanks for looking at it folks and sorry for the incomplete and misleading issue description. You're right, I was missing the schema type generic and totally misread that LeanDocument type definition. It was at the end of a sprint planning day and I was sort of spent 😅

[taxilian]

oh please please please don't use that PR (#11767) -- that undoes literally days of work. If this is that big of a concern then I will find a way to fix it. That PR oversimplifies to the point that you fix this one issue and create lots and lots of other ones.

Let me take a look at this and fix it the right way, not by just undoing a bunch of needed things

[taxilian]

At this point I'm not sure I 100% understand the things people are trying to do -- if you can give me a couple of tests which fail right now and should not fail I will make them work without completely neutering the improvements that I made. Let's please not throw the baby out with the bathwater.

It's very possible -- and quite easy -- to have types which are actually useful and help you locate code which is going to be problematic. It frustrates me that we're so determined to have code work which seemed intuitive because of weird assumptions that we're willing to throw out usefulness as well, but if it's that important then let's fix it -- but let's fix it, not just say "well, that improvement made things harder so forget it".

If these are things which are important then let's get tests around them to make sure they don't break and let's fix them.

[taxilian]

Hello @taxilian !
I've created PR to solve this issue, and it's fixed so far. To be honest, there are a lot to do to improve types, but It just a quick fix.

Sorry, responding in a more reasoned way now =] If we have to do this as a "quick fix" I understand.

I've deleted some unnecessary types "I think so" that you have created in your latest PR, I guess it would be great if you review the changes in the mentioned PR.

Those are 100% not unnecessary types. They are very necessary to cover some very specific cases while still allowing useful types. I'm happy to discuss specifics, though not sure if this is the most useful place to do so -- are you on the mongoose slack? Just let me know when/how you want to discuss.

[Uzlopak]

Maybe we should really do it the strict TDD was and collect typings tests, and then implement the typings.

[mohammad0-0ahmad]

Hello @taxilian !
I've created PR to solve this issue, and it's fixed so far. To be honest, there are a lot to do to improve types, but It just a quick fix.

Sorry, responding in a more reasoned way now =] If we have to do this as a "quick fix" I understand.

I've deleted some unnecessary types "I think so" that you have created in your latest PR, I guess it would be great if you review the changes in the mentioned PR.

Those are 100% not unnecessary types. They are very necessary to cover some very specific cases while still allowing useful types. I'm happy to discuss specifics, though not sure if this is the most useful place to do so -- are you on the mongoose slack? Just let me know when/how you want to discuss.

I am not on the mongoose slack.

[taxilian]

Maybe we should really do it the strict TDD was and collect typings tests, and then implement the typings.

I've been trying to do that with all my changes -- working on a fix for this doing that method. That's one reason that I'm concerned that the largest change in the proposed PR involves removing 40 lines of unit test code which would no longer work....

[simllll]

Not quite sure if it is the same, but 6.3.2 causes also a lot of troulbes on our side. toObject() seems to not match the plain type anymore?

e.g.

error TS2345: Argument of type 'Omit<_LeanDocument<any>, "$getAllSubdocs" | "$ignore" | "$isDefault" | "$isDeleted" | "$getPopulatedDocs" | "$isEmpty" | "$isValid" | "$locals" | "$markValid" | ... 44 more ... | "$isSingleNested">' is not assignable to parameter of type '_LeanDocument<IDBOrganization>'.
  Type 'Omit<_LeanDocument<any>, "$getAllSubdocs" | "$ignore" | "$isDefault" | "$isDeleted" | "$getPopulatedDocs" | "$isEmpty" | "$isValid" | "$locals" | "$markValid" | ... 44 more ... | "$isSingleNested">' is missing the following properties from type '_LeanDocument<IDBOrganization>': _id, name, searchName, createdAt, and 51 more.

or
TS2740: Type 'Omit_LeanDocument , "save" | "validate" | "remove" | "populate" | "$getAllSubdocs" | "$ignore" | "$isDefault" | "$isDeleted" | "$getPopulatedDocs" | "$isEmpty" | ... 43 more ... | "$isSingleNested">' is missing the following properties from type 'IWebUpload': _id, _user, type, typeid, and 3 more.

[mohammad0-0ahmad]

Maybe we should really do it the strict TDD was and collect typings tests, and then implement the typings.

I've been trying to do that with all my changes -- working on a fix for this doing that method. That's one reason that I'm concerned that the largest change in the proposed PR involves removing 40 lines of unit test code which would no longer work....

They are working if I keep them, but there are some types that are used in these tests and doesn't use internally.
Please check out the PR and double check what I said by undo the latest commit.

We can discuss that further in the mentioned PR instead of here.

[taxilian]

Moving discussion there =]

[taxilian]

@simllll is there any way you can make a short example showing the problem? I'm happy to take a look at it but I'm not following your explanation. If you can give me a snippet I can put in a unit test we can a) address it and b) make sure it doesn't come back.

[simllll]

@taxilian Nothing special actually, what we do is for example something like this:

MyModel:

interface IDBMyModel extends Document {
	_id: string;
	someprop: string;
}

const MyModel: Model<IDBMyModel> = model<IDBMyModel>(
	'MyModel',
	new Schema({
	someprop: { type: String }
        })
)

and this one causes already erros with 6.3.2:

interface IWebUpload {
	_id: string;
	someprop: string;
}
function async a(): Promise<IWebUpload> {
const model = new MyModel({someprop: 'asdf'});
await model.save();
return model.toObject(); <-- errors
}

[taxilian]

@taxilian Nothing special actually, what we do is for example something like this:

MyModel:

interface IDBMyModel extends Document {
	_id: string;
	someprop: string;
}

const MyModel: Model<IDBMyModel> = model<IDBMyModel>(
	'MyModel',
	new Schema({
	someprop: { type: String }
        })
)

and this one causes already erros with 6.3.2:

interface IWebUpload {
	_id: string;
	someprop: string;
}
function async a(): Promise<IWebUpload> {
const model = new MyModel({someprop: 'asdf'});
await model.save();
return model.toObject(); <-- errors
}

I can look at this -- and will because I want to prevent regressions -- but part of the issue is that you're using what I would call an anti-pattern -- don't have your interface extend Document. Instead, do this:

interface MyModel {
	_id: string;
	someprop: string;
}
type IDBMyModel = HydratedDocument<MyModel>;

const myModelSchema = new Schema<MyModel>({someProp: String});
const MyModel = model('MyModel', myModelSchema);\

You'll find your types all work better that way. ... that said, let me see if I can repro your issue in my PR branch

[taxilian]

@simllll also just to verify -- you realize your types are wrong for _id? The default type of _id is ObjectId, not string, so if you use that you're going to have inaccurate types unless you define _id as a string in your schema

[taxilian]

@simllll I see what is causing the issue, and it's a frustrating one because there isn't a good way to fix it, though there is a simple workaround. Here is the problem:

• Because you're extending Document in your base interface there is no way to get the interface which describes just your document vs what your document would look like without mongoose
• Because of how types work, the only way for .toObject to reference that type and try to figure out what it should be is to use this as part of the return type, which is what was used before 6.3.2 -- however, the reason it was changed is because in a lot of the tests I ran it very easily created circular dependencies on the types when I did that.
• The workaround is to pass the type into toObject -- i.e. change it to return model.toObject<IWebUpload>

Because of this breaking change it perhaps should have gone into a 6.4 release instead of a patch release :-/

The problem is that while this has a minor but annoying workaround the problems caused by the other way don't have any workaround that I could discover.

That said, if you just restructure so that you aren't extending Document (which is a really bad idea for lots of reasons anyway) then you won't have the issue and everything will Just Work (tm) =]

[simllll]

@simllll also just to verify -- you realize your types are wrong for _id? The default type of _id is ObjectId, not string, so if you use that you're going to have inaccurate types unless you define _id as a string in your schema

Yeah I just was wirting the example directly inside the comment box, we actually use some more sophisticated typescript type we call "TypeObjectId<>" that is a generic so we can distinguish different object ids from each other.

Regarding the extends Document, I will look into it right now and see if it's an easy fix.

[taxilian]

if not then just providing the type to the .toObject will be. I'm also happy to help figure out better ways to structure typescript stuff in general if you need -- I actually enjoy that sort of thing (probably a neurological disorder) and can be found on the mongoose slack server.

[vkarpov15]

Still reviewing, but quick comment that this is related to #11118

[simllll]

For me I think I sorted it out by removing all the extends Document and adding some magic with _id conversion. The mentioned pr of @vkarpov15 was also something I experienced, a toObject() is missing the _id field.

Hopefully all this effort has also some positive effects like typescript performance? 🫣👍

[mohammad0-0ahmad]

Still reviewing, but quick comment that this is related to #11118

I think it is already covered in #11767

[Uzlopak]

I went to sleep, woke up and saw the notifications and I thought: "Oh I hope the devs did not get frustrated that one and not the other PR was merged". First of all I thank you both, @taxilian and @mohammad0-0ahmad and others . I know that it could been frustrating but I think that you both had a respectful and valuable discussion why doing this or that.

And yeah, it is kind of pedantic/neurological disorder to type complex code :). I have that too. I always try to make the typings strict as possible and correct as possible. People love and hate it 🗡️

Anyway. I think that we have to rethink the whole typings. I actually also dont understand why we have LeanDocument in the first place as it should be a POJO, which shape is based on the projection and if the lean flag or function was set. So I actually wonder if the following would make sense:

The first thing is to get the automatic typing from mohammad0 getting properly reviewed and merged. I invite you, @taxilian to also review it. I will probably also review it and try and some test cases.

But actually what should happen is, that when we create a query e.g. findOne, it should return Document if not lean option was set to true or lean() was called and it should return T, were T is in both cases the resulting type of the potential fields/projection setting.

[mohammad0-0ahmad]

It's totally fine, it doesn't matter which one will be merged the important thing it to get the best possible result :)

[taxilian]

Agreed -- and while LeanDocument did originate with me I'm not so prideful that I can't see the limitations =] (I mean, it's a close thing, but still :-P)

Which PR is the "automatic typings"? I'd love to review it. I really feel like there is an "ideal" solution out there but that we haven't found it yet, so maybe that's it ;-)

I keep finding myself wondering if there is a way to make an alternate usage pattern for mongoose where you literally just pass around POJOs and then instead of "hydrating" documents you can either use them with "operate on this object" type functions or else with a backwards-compatibility style Proxy... but a) that is so far outside of this discussion that I should be shot for bringing it up and b) I'm well aware of just how much work that can end up being.

Sorry for my impatience yesterday and thanks to all who contributed; I think the fix (mine) which was merged is the safest short term solution, but likely not the best long term solution so definitely want to keep involved with the discussions. I have two different jobs where I rely on all of this =]

[Uzlopak]

#11563