TypeScript errors with .lean() after upgrading from Mongoose 8.7.1 to 8.7.2

Towerss commented 1 month ago

Prerequisites

[x] I have written a descriptive issue title
[x] I have searched existing issues to ensure the bug has not already been reported

Mongoose version

8.7.2

Node.js version

20.15.1

MongoDB server version

7.0.14

Typescript version (if applicable)

5.6.3

Description

Hi Mongoose team,

After upgrading Mongoose from version 8.7.1 to 8.7.2, I'm encountering TypeScript errors when running queries that use the .lean() method. The issue seems to be related to changes in the type utility chain, specifically in how FlattenMaps and BufferToBinary are applied.

Here’s the TypeScript error:

The types of 'options.session' are incompatible between these types.
Type 'Binary | null | undefined' is not assignable to type 'Buffer<ArrayBufferLike> | undefined'.
Type 'null' is not assignable to type 'Buffer<ArrayBufferLike> | undefined'.ts(2322)

Code that previously ran without issues, such as:

const blogs: Blog[] = await BlogModel.find({ status: BlogStatus.Published })
  .select('slug language')
  .lean()
  .exec();

or

const account: Account | null = await AccountModel.findById(accountId)
  .select('-hashedPassword -mfa -oauthId')
  .lean()
  .exec();

is now throwing the above TypeScript error.

Previously, the type chain in mongoose\types\query.d.ts was:

Require_id<FlattenMaps<BufferToBinary<RawDocType>>>

in the code:

  type GetLeanResultType<RawDocType, ResultType, QueryOp> = QueryOp extends QueryOpThatReturnsDocument
    ? (ResultType extends any[] ? Require_id<FlattenMaps<BufferToBinary<RawDocType>>>[] : Require_id<FlattenMaps<BufferToBinary<RawDocType>>>)
    : ResultType;

But after the update, the order of FlattenMaps and BufferToBinary has been swapped to:

Require_id<BufferToBinary<FlattenMaps<RawDocType>>>

like:

  type GetLeanResultType<RawDocType, ResultType, QueryOp> = QueryOp extends QueryOpThatReturnsDocument
    ? (ResultType extends any[] ? Require_id<BufferToBinary<FlattenMaps<RawDocType>>>[] : Require_id<BufferToBinary<FlattenMaps<RawDocType>>>)
    : ResultType;

This change appears to have introduced stricter typing or structural mismatches in queries that return lean documents, particularly when using .lean() or .lean() without explicit typing. The flipping of FlattenMaps and BufferToBinary seems to cause issues when dealing with Map or Buffer fields in the schema. I think this is breaking change, and should be reverted.

Steps to Reproduce

Use Mongoose 8.7.1 and run a similar query like:

const blogs: Blog[] = await BlogModel.find({ status: BlogStatus.Published })
  .select('slug language')
  .lean()
  .exec();

or

const account: Account | null = await AccountModel.findById(accountId)
    .select('-hashedPassword -mfa -oauthId')
    .lean()
    .exec();

This will run without issues.

Upgrade to Mongoose 8.7.2
Run the same queries, and then TypeScript will show an error.

Expected Behavior

Expected Behavior:

Queries using .lean() should return plain objects that conform to the expected TypeScript interface (e.g., Blog[] in the case of a Blog model) without any TypeScript errors, as it worked in version 8.7.1.

As a workaround, I’ve had to explicitly type the result of .lean(), but this was not required in previous versions:

const blogs: Blog[] = await BlogModel.find({ status: BlogStatus.Published })
  .select('slug language')
  .lean<Blog>()
  .exec();

y-nk commented 1 month ago

EDIT: I've opened a separate issue to avoid polluting this one https://github.com/Automattic/mongoose/issues/14976

richardsimko commented 4 weeks ago

Seeing the same thing, probably related to #14902 and #14967

Here is a fully reproducible example which worked in 8.7.1:

import { Schema, model } from 'mongoose';

import type { Document, ObjectId } from 'mongoose';

export interface Foo {
  _id: ObjectId;
  bar: string;
}
export type FooDocument = Foo & Document<ObjectId>;

const schema = new Schema<Foo>(
  {
    bar: { type: String, required: true },
  },
  { collection: 'foo', timestamps: true },
);

export const FooModel = model<Foo>('foo', schema);

const test: FooDocument[] = await FooModel.find({ bar: 'asd' }).lean();

core/models/test.ts:20:7 - error TS2322: Type '({ _id: { auto: (turnOn: boolean) => ObjectId; defaultOptions: { [x: string]: any; }; OptionsConstructor: { [x: string]: any; type?: any; alias?: string | string[]; validate?: Function | ... 6 more ... | readonly (Function | ... 4 more ... | { ...; }[])[]; ... 28 more ...; maxlength?: number | ... 1 more ... | reado...' is not assignable to type 'FooDocument[]'.
  Type '{ _id: { auto: (turnOn: boolean) => ObjectId; defaultOptions: { [x: string]: any; }; OptionsConstructor: { [x: string]: any; type?: any; alias?: string | string[]; validate?: Function | ... 6 more ... | readonly (Function | ... 4 more ... | { ...; }[])[]; ... 28 more ...; maxlength?: number | ... 1 more ... | readon...' is not assignable to type 'FooDocument'.
    Type '{ _id: { auto: (turnOn: boolean) => ObjectId; defaultOptions: { [x: string]: any; }; OptionsConstructor: { [x: string]: any; type?: any; alias?: string | string[]; validate?: Function | ... 6 more ... | readonly (Function | ... 4 more ... | { ...; }[])[]; ... 28 more ...; maxlength?: number | ... 1 more ... | readon...' is not assignable to type 'Foo'.
      The types of '_id.OptionsConstructor.index' are incompatible between these types.
        Type 'boolean | IndexDirection | { expires?: string | number; weights?: { [x: string]: number; }; background?: boolean; unique?: boolean; name?: string; partialFilterExpression?: { ...; }; ... 38 more ...; explain?: ExplainVerbosityLike; }' is not assignable to type 'boolean | IndexDirection | IndexOptions'.
          Type '{ expires?: string | number; weights?: { [x: string]: number; }; background?: boolean; unique?: boolean; name?: string; partialFilterExpression?: { [x: string]: any; }; sparse?: boolean; expireAfterSeconds?: number; ... 36 more ...; explain?: ExplainVerbosityLike; }' is not assignable to type 'boolean | IndexDirection | IndexOptions'.
            Type '{ expires?: string | number; weights?: { [x: string]: number; }; background?: boolean; unique?: boolean; name?: string; partialFilterExpression?: { [x: string]: any; }; sparse?: boolean; expireAfterSeconds?: number; ... 36 more ...; explain?: ExplainVerbosityLike; }' is not assignable to type 'IndexOptions'.
              The types of 'session.clientOptions.autoEncryption.keyVaultClient.options.session' are incompatible between these types.
                Type 'Binary' is missing the following properties from type 'Buffer': slice, subarray, equals, compare, and 93 more.
20 const test: FooDocument[] = await FooModel.find({ bar: 'asd' }).lean();
         ~~~~

richardsimko commented 2 weeks ago

Still present in 8.7.3. @hasezoey Any chance we can get a revert of #14967 until this is fixed as that seems to be the culprit?

hasezoey commented 2 weeks ago

@vkarpov15 what do you think about this?

to my knowledge Binary should work like a buffer, but it seems like mongodb(bson) does not extend buffer (or bufferlike) with it

vkarpov15 commented 2 weeks ago

Binary is slightly different from Buffer. The error message makes it seem like Mongoose is converting some internal type's Buffer properties into Binary when it shouldn't. I'm investigating, but haven't found an explanation yet.

vkarpov15 commented 2 weeks ago

Ok I found what the issue is, your schema definition should be the following:

import type { Document, Types } from 'mongoose';

export interface Foo {
  _id: Types.ObjectId;
  bar: string;
}
export type FooDocument = Foo & Document<Types.ObjectId>;

mongoose.ObjectId is the Mongoose ObjectId SchemaType, which describes a path configuration in a schema. At runtime, your document's _id will be of type mongoose.Types.ObjectId, not mongoose.ObjectId. We should improve it so that _id: mongoose.ObjectId doesn't compile at all, that's a bug.

vkarpov15 commented 2 weeks ago

There doesn't seem to be a good way to make it so that _id: ObjectId as opposed to _id: Types.ObjectId fails to compile with a cleaner error message. But either way, the fix is to replace:

export interface Foo {
  _id: ObjectId;
  bar: string;
}

with:

export interface Foo {
  _id: Types.ObjectId;
  bar: string;
}

because mongoose.ObjectId is not the runtime ObjectId type.

nadavhalfon commented 2 weeks ago

I can say the issue is still present, when using lean our project typescript breaks, I don't think it's right that this issue was closed, the new version just breaks an entire project

Towerss commented 2 weeks ago

Hi @vkarpov15,

Thank you for your time and effort in addressing this issue. I appreciate the attention given to the example provided by @richardsimko. However, I wanted to clarify that the core TypeScript bug I originally reported still persists and remains unaddressed.

The main issue arises after upgrading from Mongoose 8.7.1 to 8.7.2, where TypeScript errors occur when using the .lean() method in queries that previously worked without any issues. This seems to be related to the changes in the type utility chain, specifically the swapping of FlattenMaps and BufferToBinary in mongoose/types/query.d.ts. I believe this Typescript but was introduced with changes merged a couple of weeks ago to try and fix #14902.

Previously, the type chain was:

Require_id<FlattenMaps<BufferToBinary<RawDocType>>>

But in version 8.7.2, it has changed to:

Require_id<BufferToBinary<FlattenMaps<RawDocType>>>

This alteration appears to introduce stricter typing or structural mismatches when dealing with lean documents, especially without explicit typing in .lean<T>() or .lean(). Consequently, TypeScript now throws errors in situations where it didn't before, affecting existing codebases that adhere to proper typing conventions.

While the other user's example may have had typing issues, the fundamental problem is not isolated to that case. The issue impacts any code that utilizes .lean() in a similar fashion.

Could we please revisit this matter? I believe the change introduced in version 8.7.2 has inadvertently caused a breaking change in the TypeScript typings. It would be greatly appreciated if the issue could remain open until a resolution is found.

Thank you for your understanding and for your continued efforts to improve Mongoose.

nadavhalfon commented 2 weeks ago

Hi @vkarpov15,

Thank you for your time and effort in addressing this issue. I appreciate the attention given to the example provided by @richardsimko. However, I wanted to clarify that the core TypeScript bug I originally reported still persists and remains unaddressed.

The main issue arises after upgrading from Mongoose 8.7.1 to 8.7.2, where TypeScript errors occur when using the .lean() method in queries that previously worked without any issues. This seems to be related to the changes in the type utility chain, specifically the swapping of FlattenMaps and BufferToBinary in mongoose/types/query.d.ts. I believe this Typescript but was introduced with changes merged a couple of weeks ago to try and fix #14902.

Previously, the type chain was:

Require_id<FlattenMaps<BufferToBinary<RawDocType>>>

But in version 8.7.2, it has changed to:

Require_id<BufferToBinary<FlattenMaps<RawDocType>>>

This alteration appears to introduce stricter typing or structural mismatches when dealing with lean documents, especially without explicit typing in .lean<T>() or .lean(). Consequently, TypeScript now throws errors in situations where it didn't before, affecting existing codebases that adhere to proper typing conventions.

While the other user's example may have had typing issues, the fundamental problem is not isolated to that case. The issue impacts any code that utilizes .lean() in a similar fashion.

Could we please revisit this matter? I believe the change introduced in version 8.7.2 has inadvertently caused a breaking change in the TypeScript typings. It would be greatly appreciated if the issue could remain open until a resolution is found.

Thank you for your understanding and for your continued efforts to improve Mongoose.

Thanks you for addressing this case in a very detailed answer, I apologize for my attitude earlier 😃 and of course very appreciate the Mongoose team work 🙏🏻

richardsimko commented 2 weeks ago

@vkarpov15 As others have pointed out this issue isn't actually solved by your recommended solution and should be reopened. Here is the same repro with your suggested change which still throws an error.

import { Schema, model } from 'mongoose';

import type { Document, Types } from 'mongoose';

export interface Foo {
  _id: Types.ObjectId;
  bar: string;
}
export type FooDocument = Foo & Document<Types.ObjectId>;

const schema = new Schema<Foo>(
  {
    bar: { type: String, required: true },
  },
  { collection: 'foo', timestamps: true },
);

export const FooModel = model<Foo>('foo', schema);

const test: FooDocument[] = await FooModel.find({ bar: 'asd' }).lean();

I'm again asking for a #14967 to be reverted until a stable solution has been found, as others have mentioned this is a breaking change that was released as a patch version and should be rolled back.

hasezoey commented 2 weeks ago

@Towerss

(original issue description)

Previously, the type chain in mongoose\types\query.d.ts was: Require_id<FlattenMaps<BufferToBinary<RawDocType>>>

(later comment)

This seems to be related to the changes in the type utility chain, specifically the swapping of FlattenMaps and BufferToBinary in mongoose/types/query.d.ts Previously, the type chain was: Require_id<FlattenMaps<BufferToBinary<RawDocType>>> But in version 8.7.2, it has changed to:

just wanna point out that #14967 completely added the new BufferToBinary type

This is how the lean type was in tag 8.7.1: https://github.com/Automattic/mongoose/blob/02c5efd7ee19eca0f0e5744719b36e19cba0f58d/types/query.d.ts#L213-L215

I believe the change introduced in version 8.7.2 has inadvertently caused a breaking change in the TypeScript typings

to my knowledge in mongoose any type change can be in any version, though it had mostly stabilized since 7.x (speaking from a typegoose upgrade perspective)

i also wanna point out that the examples from this comment and this comment want to assign a lean type to a document type. A lean type does not have anything from type mongoose.Document anymore (aside from the extra properties like _id and __v, etc).

A fixed version could look like:

/* eslint-disable @typescript-eslint/no-unused-vars */
// NodeJS: 22.8.0
// Typescript 5.3.3
import * as mongoose from 'mongoose'; // mongoose@8.8.0

interface Foo {
  _id: mongoose.Types.ObjectId;
  bar: string;
}
type FooDocument = Foo & mongoose.Document<mongoose.Types.ObjectId>;
type FooLean = mongoose.GetLeanResultType<Foo, Foo, unknown>;

const schema = new mongoose.Schema<Foo>(
  {
    bar: { type: String, required: true },
  },
  { collection: 'foo', timestamps: true }
);

const FooModel = mongoose.model<Foo>('foo', schema);

async function main() {
  const test1: FooLean[] = await FooModel.find({ bar: 'asd' }).lean();
  const test2: Foo[] = await FooModel.find({ bar: 'asd' }).lean();
  // const test3: FooDocument[] = await FooModel.find({ bar: 'asd' }).lean();
}

main();

(Reproduction repository & branch)

If the issue still persists when removing the Document from the types, please provide new / updated reproduction code (and error output for quicker look-over)

Towerss commented 2 weeks ago

Hi @hasezoey,

Thank you for joining the conversation and for your valuable insights. You correctly pointed out that BufferToBinary was introduced in version 8.7.2. I apologise for any confusion regarding references to changes in the type utility chain; I was referencing the code I was testing while tracing the root cause of the typing issue.

I believe we are now addressing two separate issues:

Breaking Changes in TypeScript Types

I still consider the recent changes in the TypeScript types to be a breaking change. The necessity to refactor existing code to comply with the new types suggests that the changes are not backward-compatible. While I acknowledge that these types of changes may offer improvements or are necessary corrections, the core concern is that such changes should not be introduced in a minor or patch release according to Semantic Versioning (SemVer) principles.

Some might argue that types do not affect the runtime behaviour of JavaScript code and that changing types is merely a correction, especially if the previous types were incorrect or incomplete. However, TypeScript types are more than just annotations; they define the contract between the library and its consumers. When a package exposes types, those types become part of its public API. Altering them can significantly impact how consumers interact with the library, potentially causing type errors or necessitating code changes.

Allowing non-backward-compatible type changes without a major version bump undermines the purpose of SemVer. Consumers rely on version numbers to gauge the impact of updates. Unexpected breaking changes can lead to integration issues and erode trust in the package's stability.

Your code example revealed that my team will need to undertake significant refactoring of our model definitions to accommodate the use of plain JavaScript objects returned from using .lean(), while also adhering to the conventions introduced in Mongoose 7+. In our case, this involves refactoring numerous models and their respective usages. This underscores the impact of the typing changes and supports the view that they constitute a breaking change.

I understand that maintaining detailed documentation for every type of change can be resource-intensive, especially in fast-moving projects. However, when types are integrated and maintained within the repository, they should be treated with the same level of importance as the code itself. Documentation should reflect type changes, providing guidance on how to adapt to new requirements. This transparency helps consumers smoothly transition between versions.

Typing Models and Interfaces for .lean() and Hydrated Documents

As mentioned above, your code example indicates that we need to refactor our models and their usage to comply with the typing conventions introduced in Mongoose 7+, where extending Document in TypeScript interfaces is discouraged. However, I am cautious about using GetLeanResultType during refactoring. The reason is that I could not find any mention of it in the Mongoose documentation. Unless one is a Mongoose expert, it is unlikely that developers are aware of this utility type.

For example, searching the documentation for GetLeanResultType yields no results:

My understanding is that after Mongoose 7, we need separate types or interfaces to work with .lean() results and hydrated Mongoose documents. To avoid using GetLeanResultType and manually extending Document like type FooDocument = Foo & mongoose.Document;, we are planning to use an approach similar to the following:

import mongoose, { Schema, Types, HydratedDocument } from 'mongoose'; // mongoose@8.8.0

interface Foo {
  _id: Types.ObjectId;
  bar: string;
}

const schema = new Schema<Foo>(
  {
    bar: { type: String, required: true },
  },
  { collection: 'foo', timestamps: true }
);

const FooModel = mongoose.model('Foo', schema);

// Define the hydrated document type
type FooDocument = HydratedDocument<Foo>;

// Example usage

  // For standard queries returning Mongoose documents (with methods)
  const fooDocs: FooDocument[] = await FooModel.find();

  // For lean queries returning plain JavaScript objects
  const foos: Foo[] = await FooModel.find().lean();

  // If you need to find one document
  const fooDoc: FooDocument | null = await FooModel.findById(someId);

  // For a lean query returning a single object
  const foo: Foo | null = await FooModel.findById(someId).lean();

This approach allows us to work with both lean results and hydrated documents without relying on undocumented or potentially unstable types like GetLeanResultType. It also aligns with the practice of avoiding direct extension of Documents in our interfaces.

A developer might also consider the user of InferSchemaType to automatically infer the document type from a schema definition, removing the need to create separate Typescript types/interfaces like interface Foo as mentioned avobe:

const fooSchema = new Schema({
...
});

type Foo = InferSchemaType<typeof fooSchema>;
// no need for interface Foo{...}

Possible Temporary Workaround

If reverting the type changes is not an option and the new types are to be kept, we are considering using a temporary workaround by directly typing .lean() where Foo still extends Document, even though we understand this is not ideal and undermines the benefits of type safety. This approach would allow us to keep our project moving forward while we allocate time for the necessary refactoring to properly handle .lean() results and hydrated documents. We suspect that other teams might resort to similar measures under time constraints.

I appreciate your efforts in improving Mongoose and understand the challenges in balancing progress with stability. I hope that this feedback helps in considering the impact of typing changes on the developer community and prompts a discussion on how to better manage such changes in the future.

richardsimko commented 2 weeks ago

@hasezoey Thanks, that did indeed resolve the issue I had.

@Towerss I created #15017 and will try to work on it myself in the coming days/weeks to try and improve TS testing as we've also experienced what you describe with a lot of breaking TS changes being shipped in non-major releases.

hasezoey commented 2 weeks ago

While I acknowledge that these types of changes may offer improvements or are necessary corrections, the core concern is that such changes should not be introduced in a minor or patch release according to Semantic Versioning (SemVer) principles.

I totally agree to that, but from my knowledge types in mongoose are "second class" and can be changed at any time, though from experience it has been more stable since 7.x (coming from a typegoose upgrade standpoint). Also for this one specific change, it did not require any changing to existing tests, hence likely being seen as "non-breaking".

For any more discussion on the general compatibility of types, please refer to #15017.

However, I am cautious about using GetLeanResultType during refactoring. The reason is that I could not find any mention of it in the Mongoose documentation. Unless one is a Mongoose expert, it is unlikely that developers are aware of this utility type.

I just used GetLeanResultType as a example as it is the return type of .lean().

@vkarpov15 what do you think about reverting the mention change?

Guessing that now new minor version (from 8.7 to 8.8) has come out, i think it is unlikely to get a revert or a new 8.7 version with a revert.

Towerss commented 2 weeks ago

Thank you, @hasezoey, for your reply.

The tests were also updated in the same PR to match the inclusion of the new type: https://github.com/Automattic/mongoose/pull/14967/commits/8825b76be5e901e57676b415cfdee2e7df123164 @ test/types/schema.test.ts, and you were one of the reviewers/approvers.

If the tests had not been updated along the type changes, the PR would not have passed the deployment tests, I guess, as I do not know about Mongoose's CI/CD implementation.

We determined we will stick to version 8.7.1 while we fix the issues your code example and the type changes helped us uncover.

hasezoey commented 2 weeks ago

The tests were also updated in the same PR to match the inclusion of the new type: https://github.com/Automattic/mongoose/commit/8825b76be5e901e57676b415cfdee2e7df123164 @ test/types/schema.test.ts, and you were one of the reviewers/approvers.

Do you mean this change? as otherwise there are only new test cases. To me this looked like a type correction only (ie match runtime behavior).

vkarpov15 commented 2 weeks ago

const test: FooDocument[] = await FooModel.find({ bar: 'asd' }).lean(); should be const test: Foo[] = await FooModel.find({ bar: 'asd' }).lean();. The whole point of lean() is that it returns the raw Foo document, without any Mongoose functionality.

"what do you think about reverting the mention change?"

I think we should not revert this change. The line between a bug fix and a breaking change can get blurry, especially when users rely on incorrect behavior. However, in this case, I think the TypeScript types are doing their job by highlighting code that is objectively incorrect. If you're expecting _id to be of type mongoose.ObjectId or expecting lean() to return a hydrated Mongoose document, then you're going to run into runtime errors.

You can always work around this issue using lean()'s generic param: const test: FooDocument[] = await FooModel.find({ bar: 'asd' }).lean<FooDocument[]>(), although I would only recommend that as a temporary workaround.

Towerss commented 2 weeks ago

@hasezoey, the modifications in schema.test.ts seem directly related to supporting the new type definitions. If those tests had not been updated, they likely would have failed, indicating a shift in how consumers should interact with Mongoose. This reinforces the idea that the changes are significant.

@vkarpov15, while I understand that these updates may provide useful improvements, I hope that future changes, even if they enhance current functionality, will adhere to Semantic Versioning best practices. This will ensure a smoother experience for all users and maintain trust in the library's stability. If such changes continue to occur, we will likely keep facing issues like this. I reported something similar two months ago: https://github.com/Automattic/mongoose/issues/14863, and it seems I’m not alone, as @richardsimko mentioned.

Thank you, guys, for the discussion! I learned a lot about Mongoose along the way.

Automattic / mongoose