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

How to write a generic function to resolve PopulatedDoc type variables? #15121

Closed
1 task done
nikzanda opened this issue Dec 19, 2024 · 14 comments · Fixed by #15261
Closed
1 task done

How to write a generic function to resolve PopulatedDoc type variables? #15121

nikzanda opened this issue Dec 19, 2024 · 14 comments · Fixed by #15261
Labels
help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@nikzanda
Copy link

Prerequisites

  • I have written a descriptive issue title

Mongoose version

8.9.1

Node.js version

20.10.0

MongoDB version

6.0.2

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

No response

Issue

In my project, I often find myself having to implement this code:

// Check if 'myPopulatedDocProp' has already been populated in a previous part of the code
if (myDocument.myPopulatedDocProp instanceof AnotherModel) {
  return myDocument.myPopulatedDocProp;
}

// If 'myPopulatedDocProp' was not populated, perform a database query to find and populate the document property
const result = await AnotherModel
  .findById(myDocument.myPopulatedDocProp) // Query the AnotherModel to find the document by ID
  .orFail(); // Throw an error if the document is not found
return result; // Return the populated document property

Is it possible to write a generic function so that I don't have to rewrite the same exact code but with different models?
I tried with this code but it doesn't work:

export const findOrReturnInstance = async <T extends Document>(
  populatedDoc: PopulatedDoc<T>,
  model: Model<T>,
) => {
  if (populatedDoc instanceof model) {
    return populatedDoc;
  }
  const result = await model
    .findById(populatedDoc)
    .orFail();
  return result;
};

Is it the right approach or am I doing something wrong?
Attention, with this generic function I want to ensure that any virtual variables and instance methods, etc., are preserved.

Thank you for your help. 🙏🏻

@nikzanda nikzanda added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary help wanted labels Dec 19, 2024
@vkarpov15 vkarpov15 added this to the 8.9.4 milestone Dec 27, 2024
@vkarpov15
Copy link
Collaborator

The approach is reasonable, you just need to type model correctly. I would also recommend against using PopulatedDoc<> type for reasons explained here.

Take a look at the following script, would this approach work for you?

import mongoose, { Schema } from 'mongoose';

const ParentModel = mongoose.model('Parent', new Schema({
  child: { type: Schema.Types.ObjectId, ref: 'Child' },
  name: String
}));

const childSchema: Schema = new Schema({ name: String });
const ChildModel = mongoose.model('Child', childSchema);
type ChildHydratedDocType = ReturnType<(typeof ChildModel)['hydrate']>;

(async function() {
  const doc = await ParentModel.findOne({}).populate<{ child: ChildHydratedDocType }>('child').orFail();
  const res1: ChildHydratedDocType = await findOrReturnInstance(doc.child, ChildModel);

  const doc2 = await ParentModel.findOne().orFail();
  const res2: ChildHydratedDocType = await findOrReturnInstance(doc.child, ChildModel);
})();

async function findOrReturnInstance<HydratedDocType extends mongoose.Document>(
  docOrId: HydratedDocType | mongoose.Types.ObjectId,
  Model: mongoose.Model<any, any, any, any, HydratedDocType>
) {
  if (docOrId instanceof mongoose.Document) {
    return docOrId;
  }
  
  return Model.findById(docOrId).orFail();
}  

@vkarpov15 vkarpov15 removed this from the 8.9.4 milestone Jan 6, 2025
@nikzanda
Copy link
Author

nikzanda commented Jan 8, 2025

The script is fine, although I would have preferred the type to be inferred already by findOrReturnInstance and not specified manually.

I have another question: if I avoid using PopulatedDoc, how do I handle the following situation?

function fn(p: ParentInstance) {
   // TODO
}

const parent = await Parent.findOne({}).populate<{ child: ChildInstance }>('child').orFail();

// There appears to be a compilation error here, because if I populate child, it will no longer be of type ObjectId.
fn(parent); 

@vkarpov15
Copy link
Collaborator

Can you please elaborate on "although I would have preferred the type to be inferred already by findOrReturnInstance and not specified manually."?

Re: PopulatedDoc, that seems like expected behavior because parent is no longer strictly the same type as ParentInstance. You can always do something like fn(p: ParentInstance & { child: ChildInstance }) if you want to allow populated docs to be passed into fn().

@nikzanda
Copy link
Author

nikzanda commented Jan 9, 2025

Let me explain better: in the example you wrote, for res1 and res2 you explicitly declared their types. Do you think it is possible to make the findOrReturnInstance function return the correct type without having to specify it?

@vkarpov15
Copy link
Collaborator

Yeah the script compiles fine if you remove : ChildHydratedDocType from const res2: ChildHydratedDocType. I added the explicit type to demonstrate that the function's return type was correct.

@nikzanda
Copy link
Author

Yes, the script compiles fine, but if you remove the types from the res1 and res2 variables, they are typed as any. Also, the findOrReturnInstance function's return type is Promise<any>.

Reproduction link here

Image
Image

Do you think there's a way to make the function return the correct type without having to specify it manually each time?

@vkarpov15 vkarpov15 added this to the 8.9.6 milestone Jan 14, 2025
@vkarpov15 vkarpov15 added has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue and removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary labels Jan 14, 2025
@vkarpov15
Copy link
Collaborator

How about the following? I explicitly set the return type on findOrReturnInstance, and also removed the : Schema type definition on childSchema that was causing TS to infer ChildModel as { [key: string]: unknown }

import mongoose, { Schema } from 'mongoose';

const ParentModel = mongoose.model('Parent', new Schema({
  child: { type: Schema.Types.ObjectId, ref: 'Child' },
  name: String
}));

const childSchema = new Schema({ name: String });
const ChildModel = mongoose.model('Child', childSchema);
type ChildHydratedDocType = ReturnType<(typeof ChildModel)['hydrate']>;

(async function() {
  const doc = await ParentModel.findOne({}).populate<{ child: ChildHydratedDocType }>('child').orFail();
  const res1 = await findOrReturnInstance(doc.child, ChildModel);

  const doc2 = await ParentModel.findOne().orFail();
  const res2 = await findOrReturnInstance(doc.child, ChildModel);

  const name1 = res1.name;
  const name2 = res2.name;
  f2(res1, name1);
  f2(res2, name2);
})();

async function findOrReturnInstance<HydratedDocType extends mongoose.Document>(
  docOrId: HydratedDocType | mongoose.Types.ObjectId,
  Model: mongoose.Model<any, any, any, any, HydratedDocType>
): Promise<HydratedDocType> {
  if (docOrId instanceof Model) {
    return docOrId;
  }

  return Model.findById(docOrId).orFail();
}

function f2(doc: ChildHydratedDocType, name?: string | null) {
  console.log(doc.name, name, doc.save());
}

Compiles correctly, and my editor ([email protected]) seems to pick up correct type as shown in the following screenshot

Image

@vkarpov15 vkarpov15 added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary and removed has repro script There is a repro script, the Mongoose devs need to confirm that it reproduces the issue labels Jan 17, 2025
@vkarpov15 vkarpov15 removed this from the 8.9.6 milestone Jan 17, 2025
@nikzanda
Copy link
Author

Yes, with the changes you introduced, the returned type is correct in this example.
I have updated the demo with a more complex example:
If the type of child is not explicitly stated in the query, the variable returned by the findOrReturnInstance function is not able to add any document overrides or virtuals. Furthermore, by invoking the toObject function, the result is a variable of type any.

Explicitly specifying the type:

Image
Image

Not explicitly specifying the type:

Image
Image

Therefore, by not explicitly specifying the type, any embedded types will be taken from the main model interface rather than from the virtuals and/or document overrides interfaces.
A best practice might be to always explicitly specify the type during the query. However, if, for example, I have a function that accepts ParentInstance as a parameter, I cannot verify whether child has been populated or not, and it would be better not to have to manually add typings such as: fn(parent: ParentInstance & { child: ChildInstance }).

Do you think it is possible to resolve this last issue as well?
If solved, do you think this function could be integrated into the mongoose library? Or should I create my own npm package to export it?

Thank you for your help. 🙏🏻

@vkarpov15 vkarpov15 added this to the 8.9.6 milestone Jan 20, 2025
@vkarpov15 vkarpov15 added typescript Types or Types-test related issue / Pull Request and removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary labels Jan 20, 2025
@vkarpov15 vkarpov15 modified the milestones: 8.9.6, 8.9.7, 8.10.1 Jan 31, 2025
@vkarpov15
Copy link
Collaborator

Simply removing PopulatedDoc resolves the issue, just define Parent as follows:

export interface IParent {
  code: string;
  name: string;
  child: Types.ObjectId;
  createdAt: Date;
  updatedAt: Date;
}

Image

Another option if you want to keep PopulatedDoc is the following, which ensures that PopulatedDoc is aware that the populated doc's raw doc type is IChild. In general, you should always use HydratedDocument<> rather than Document & RawDocType.

export interface IParent {
  code: string;
  name: string;
  child: PopulatedDoc<HydratedDocument<IChild>>;
  createdAt: Date;
  updatedAt: Date;
}

As for "I cannot verify whether child has been populated or not", I don't think that is correct. You have a couple of options there.

  1. By default, await doc.populate('child') is a no-op if child is already populated. So you can just call populate() an extra time to ensure you get the populated value.
  2. Documents also have a $assertPopulated() method that throws an error if any paths listed are not populated.

Further, I think fn(parent: Omit<ParentInstance, 'child'> & { child: ChildInstance }) is likely a better approach than relying on a complex generic function that exists primarily to appease the compiler. That way the compiler can enforce for you that you're populating fields correctly ahead of time, and we typically recommend populating data all at once to maintain a consistent data shape.

Another alternative you might consider is populate virtuals, which is a bit cleaner in this approach since a populate virtual is undefined if it isn't populated.

@vkarpov15 vkarpov15 removed this from the 8.10.1 milestone Feb 12, 2025
@vkarpov15 vkarpov15 added the help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary label Feb 12, 2025
@nikzanda
Copy link
Author

I agree with you that for a consistent data shape, it's better to populate all the data at once. However, my case is more complex: in my project, I use GraphQL, and the sample function I wrote when I opened the issue is often used in the resolvers for fields of type PopulatedDoc. In the resolver, I don't know if a populate has been done previously or not. Moreover, if a GraphQL query only extracts "id" and "name" for the Parent model, for performance reasons, it is not necessary and I don't want the populate on "child" to be executed, as it will not be extracted.

That said, by modifying the typing to PopulatedDoc<HydratedDocument<IChild>>, the problem is partially solved: res2Obj is no longer of type any, but the Virtuals and DocumentOverrides types are not added. Once this last problem is resolved, I will finally be able to close the issue 🙏🏻.

I have updated the reproduction link.
Thank you again for your help and patience! 🫂

res1 ✅
Image
Image

res2 ❌
Image
Image

@nikzanda
Copy link
Author

I finally found the solution, instead of PopulatedDoc<HydratedDocument<IChild>>, the right type is PopulatedDoc<ChildInstance>! 🥳

But unfortunately I found another error 😫 I reported it in the demo.
If I execute a lean query, the result type for parent variable (see the new added model User in the demo) is different from the IUser['parent'] type: lean query returns FlattenMaps type for parent, but IUser interface keeps the hydrated document type.

How can I fix it? Thank you. 🙏🏻

@vkarpov15 vkarpov15 modified the milestones: 8.10.1, 8.10.2 Feb 13, 2025
@vkarpov15
Copy link
Collaborator

vkarpov15 commented Feb 16, 2025

You're right, I didn't remember that Child model had virtuals, which means ChildInstance isn't equivalent to HydratedDocument<IChild>, you would need HydratedDocument<IChild, IChildVirtuals & ChildDocumentOverrides>. I'm looking into the FlattenMaps issue you mentioned. As a workaround, you can explicitly type the return value using const user = await UserModel.findOne().lean<IUser>().orFail();

@vkarpov15
Copy link
Collaborator

I figured out why your demo is failing: you haven't installed @types/node, and Mongoose's BufferToBinary type helper converts every value to Binary. Because Buffer becomes any if @types/node isn't installed, so T extends Buffer ? mongodb.Binary : ... always takes the true path.

We will make Mongoose more resilient to this weird TypeScript behavior (unknown type becomes any seems like suboptimal DX) but the solution for now is just npm i @types/node --save-dev

@nikzanda
Copy link
Author

Ok, thank you very much for the help! Now that the objective of this issue has been achieved, I can finally close it. If I encounter any further problems, I will open another issue.

vkarpov15 added a commit that referenced this issue Feb 18, 2025
types: make type inference logic resilient to no Buffer type due to missing `@types/node`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary typescript Types or Types-test related issue / Pull Request
Projects
None yet
2 participants