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

When migrating from mongoose 5 to 7, how to get LeanDocument funcitonality? #13424

Closed
1 task done
EcksDy opened this issue May 21, 2023 · 13 comments · Fixed by #14095
Closed
1 task done

When migrating from mongoose 5 to 7, how to get LeanDocument funcitonality? #13424

EcksDy opened this issue May 21, 2023 · 13 comments · Fixed by #14095
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@EcksDy
Copy link

EcksDy commented May 21, 2023

Prerequisites

  • I have written a descriptive issue title

Mongoose version

7.2.0

Node.js version

18.16.0

MongoDB version

6.0

Operating system

macOS

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

No response

Issue

I'm trying to migrate our nestjs/mongoose codebase from version 5 to 7.

We've been using using T.lean() or T.toObject() and our return types would be LeanDocument<T>. Mongoose 7 no longer exports LeanDocument, and the existing migration guide suggests using the following setup:

// Do this instead, no `extends Document`
interface ITest {
  name?: string;
}
const Test = model<ITest>('Test', schema);

// If you need to access the hydrated document type, use the following code
type TestDocument = ReturnType<(typeof Test)['hydrate']>;

But this gives me HydratedDocument that I can get by HydratedDocument<T>, which is not what I want since it has all the document methods on it.
As an alternative I can use just T as my return type, but then any Document<T> is matching T.

I'd like to enforce that the result is a POJO, LeanDocument or just the documents fields and virtuals, and not a Document.
What approach can I take?

P.S.
I've asked the same question on stackoverflow, feel free to answer there also, I'd gladly accept the answer.

@EcksDy EcksDy added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary help wanted labels May 21, 2023
@vkarpov15
Copy link
Collaborator

Can you please provide an example of how you're defining your schemas and models? We removed LeanDocument<> because lean() and toObject() now return the raw document interface, which is the first generic param to mongoose.Schema<>.

@EcksDy
Copy link
Author

EcksDy commented May 24, 2023

Thank you for the quick response.

For background, we're using nestjs, so schema definition is done via decorators as such:

@Schema(/* options */)
export class SomeClass {
  id: string;

  @Prop({ required: true })
  someProp: string;
}

export const SomeClassSchema = SchemaFactory.createForClass(SomeClass);

SchemaFactory.createForClass creates a schema according to mongoose docs:

  static createForClass<TClass = any, _TDeprecatedTypeArgument = any>(
    target: Type<TClass>,
  ): mongoose.Schema<TClass> {
    const schemaDefinition = DefinitionsFactory.createForClass(target);
    const schemaMetadata =
      TypeMetadataStorage.getSchemaMetadataByTarget(target);

    // HERE:
    return new mongoose.Schema<TClass>(
      schemaDefinition as SchemaDefinition<SchemaDefinitionType<TClass>>,
      schemaMetadata && schemaMetadata.options,
    );
  }

===========

Now that I look at it more closely LeanDocument<SomeClass> and SomeClass types are the same when looking at the available properties.

I'm assuming it's safe to use SomeClass as the return type of functions as such:

async function getById(id: string): Promise<SomeClass> {
  const result = await this.model.findById(id);
  return result.toObject();
}

What I noticed is that if I return the document without converting .toObject() it still passes, as the document is SomeClass with document properties added to it. So the document leaks out of my DAL and can be serialized when leaving the service.

I guess my question is:
Is there a way to enforce the usage of .toObject() to prevent documents from being returned out of my DAL classes?
Or in other words, what ??? needs to be? :)

async function getById(id: string): Promise<???> {
  const result = await this.model.findById(id);
  return result;
            //  ^ is a document, use `.toObject()`
}

@vkarpov15
Copy link
Collaborator

Why not just Promise<TClass>? It looks like TClass is just a plain old class with no document-specific stuff attached to it.

@EcksDy
Copy link
Author

EcksDy commented May 24, 2023

TClass in this case is the SomeClass for which I create the schema:

export const SomeClassSchema = SchemaFactory.createForClass(SomeClass);

My question is how to prevent the types from lying about what there is in runtime:

async function getById(id: string): Promise<SomeClass> {
  const result = await this.model.findById(id);
  console.log(result); // result will have document methods, __v, _id on it.
  return result;

async function testFunc(id: string) {
  const result = await this.getById(id);
  // `typeof result` will be SomeClass, as if there are no additional properties on the result
  console.log(result); // but in runtime result still has document methods, __v, _id on it, since we didnt do .toObject()
}

@vkarpov15
Copy link
Collaborator

If you want to add a type check that enforces that getById() returns a lean document rather than a fully hydrated Mongoose document, I'd recommend making the return value Promise<SomeClass & { $locals?: never }>. That will make TS complain about "Types of property '$locals' are incompatible." if you return a hydrated document. Below is an example using vanilla Mongoose:

import mongoose from 'mongoose';

interface User {
  name: string;
  age: number;
}

const UserModel = mongoose.model('User', new mongoose.Schema<User>({
  name: { type: String, required: true },
  age: { type: Number, required: true }
}));

async function getById(id: string): Promise<User & { $locals?: never }> {
  const user = await UserModel.findById(id).lean().orFail(); // <-- comment out `lean()` and this fails
  return user;
}

@JavaScriptBach
Copy link
Contributor

@vkarpov15 Can we recommend a replacement solution in the docs to assist migration?

.lean() doesn't return the raw interface, it also includes things like ._id, .__v, and .id if the user is using mongoose-lean-virtuals. We used the LeanDocument<> type everywhere in our codebase, and now I don't really know how to migrate to Mongoose 7 now that it's been removed.

Frankly it seems correct to add the type back. I still don't understand why it was removed.

@vkarpov15
Copy link
Collaborator

Because seemingly simple tasks like omitting properties are surprisingly nuanced in TypeScript. Features like private fields, generics, etc. make omitting properties tricky, so relying on LeanDocument by default caused a lot of issues. You can always overwrite the return type of lean() using lean<Omit<DocType, '__v'>>() or something like that.

Can you provide some examples of how you're using LeanDocument<>?

@JavaScriptBach
Copy link
Contributor

We pass lean documents around in our codebase and use TypeScript. Therefore, we need a type that represents what the result of calling await FooModel.findOne(...).lean({virtuals: ["id"]}).exec() and LeanDocument used to be that type before it was removed.

We used to define it next to our models like this:

const fooSchemaDef = {...};
export type FooFields = ObtainDocumentType<typeof fooSchemaDef>;
const FooModel = model("Foo", new Schema(fooSchemaDef));
export type FooDocument = FooFields & Document;
export type FooLeanDoc = LeanDocument<FooDocument>;

I agree it's hard to get the types right in TypeScript and to be honest, I've generally found it quite difficult to work with Mongoose's types. However, regardless of the difficulty of implementation, I think the need is still there.

For now I'm trying to just vendor LeanDocument myself and then doing the Mongoose 7 upgrade.

@vkarpov15
Copy link
Collaborator

Instead of doing export type FooDocument = FooFields & Document, I'd recommend export type FooDocument = HydratedDocument<FooFields> where import { HydratedDocument } from 'mongoose';.

One thing we're working on is inferRawDocType in #13900 that may help you (I saw you commented on that issue 😃 ). The idea of inferRawDocType is you would be able to do:

const fooSchema = new Schema(fooSchemaDef);

export type FooDocument = HydratedDocument<FooFields>;
export type FooLeanDoc = inferRawDocType<typeof fooSchema>;

Based on our experience so far, it is much easier to infer the raw doc type (what LeanDocument<> used to return) based on the schema definition rather than the hydrated doc type. The hydrated doc type is more likely to use generics, private fields, external types, and other pitfalls. Whereas schema defs are typically inline POJOs. What do you think about that?

@JavaScriptBach
Copy link
Contributor

@vkarpov15 The problem is that I have no idea how to type Mongoose models correctly in any version, and they are changing in every major version.

As an example illustration, I'm using Mongoose 6 and ObtainDocumentType. I'm trying something like this:

const subDoc = {
  name: { type: String, required: true },
  controls: { type: String, required: true },
};

const testSchema = {
  question: { type: String, required: true },
  subDocArray: { type: [subDoc], required: true },
  subDoc: { type: subDoc, required: true },
}

type TestFields = ObtainDocumentType<typeof testSchema>;
type TestDoc = HydratedDocument<TestFields>;

const TestModel = model<TestDoc>("TestModel", new Schema(testSchema));

But this does not give the correct types. When I do a lean query, the types of subDoc and subDocArray are missing _id.

I think the docs need to clearly state how to type 1) the raw schema definition, 2) lean documents (i.e. they include _id fields on subdocuments and subdocument arrays), and 3) the full document. And ideally all this should be derived from a single schema definition source of truth.

https://mongoosejs.com/docs/typescript/subdocuments.html makes it sound like this is impossible without manually feeding your own types into Mongoose, is that true?

@vkarpov15
Copy link
Collaborator

@JavaScriptBach try the following, seems to compile fine with Mongoose 6.12:

import mongoose from 'mongoose';

const subDoc = {
  name: { type: String, required: true },
  controls: { type: String, required: true },
};

const testSchema = {
  question: { type: String, required: true },
  subDocArray: { type: [new mongoose.Schema(subDoc)], required: true },
  subDoc: { type: new mongoose.Schema(subDoc), required: true },
}

const TestModel = mongoose.model("TestModel", new mongoose.Schema(testSchema));

const doc = new TestModel({});
doc.subDocArray[0]._id;

@JavaScriptBach
Copy link
Contributor

Thanks @vkarpov15 ! I ended up going with a different approach, as all of our schemas are defined using object literals and it would be confusing to tell users to define some fields using schemas, so I ended up writing the following:

import type { IfEquals, Types } from "mongoose";

type GetPathType<T> = T extends Array<infer Item>
  ? // Need to wrap both sides of this `extends` with square brackets to avoid
    // distributing unions, which changes the type.
    // See https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types
    [Item] extends [{ [x: string]: any }]
    ? Array<Item & { _id: Types.ObjectId }>
    : T
  : T extends { [x: string]: any }
  ? T & { _id: Types.ObjectId }
  : T;

// Given a Mongoose document interface, hydrate it with additional types so that it's suitable
// to feed as the first type argument into mongoose.Model.
export type HydratedFields<T extends { [x: string]: any }> = {
  [K in keyof T]: IfEquals<
    T[K],
    NonNullable<T[K]>,
    GetPathType<T[K]>,
    GetPathType<NonNullable<T[K]> | null | undefined>
  >;
} & { id: string };

Taking a step back, I think the larger problem here is that Mongoose is trying to provide types for an API that is not statically typeable, so we've had to write a lot of hacks to get things somewhat working. If Mongoose wants to provide first-class support for TypeScript, then I think we should consider writing a statically typeable API in a future major release.

@vkarpov15
Copy link
Collaborator

We will take a look as to why subdocArray being array of schemas versus array of objects makes a difference, that's a little strange.

@vkarpov15 vkarpov15 reopened this Nov 9, 2023
@vkarpov15 vkarpov15 added this to the 7.6.6 milestone Nov 9, 2023
@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 help wanted labels Nov 9, 2023
vkarpov15 added a commit that referenced this issue Nov 21, 2023
types: allow defining document array using `[{ prop: String }]` syntax
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

Successfully merging a pull request may close this issue.

3 participants