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

Breaking TypeScript Change in find from v8.5.5 to v8.6.0 #14863

Closed
2 tasks done
Towerss opened this issue Sep 3, 2024 · 7 comments · Fixed by #14874
Closed
2 tasks done

Breaking TypeScript Change in find from v8.5.5 to v8.6.0 #14863

Towerss opened this issue Sep 3, 2024 · 7 comments · Fixed by #14874
Labels
discussion If you have any thoughts or comments on this issue, please share them!

Comments

@Towerss
Copy link

Towerss commented Sep 3, 2024

Prerequisites

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

Mongoose version

8.6.0

Node.js version

20.15.1

MongoDB server version

7

Typescript version (if applicable)

5.5.4

Description

Hi Mongoose team,

Mongoose version 8.6.0 introduced a breaking change in the type definitions for the find method. The filter parameter type was changed from FilterQuery to RootFilterQuery, which no longer allows fields or operators that are not explicitly defined in the schema.

This change significantly affects the use of generic utility functions in TypeScript. For example, functions designed to operate across multiple Mongoose models with different schemas (using a generic Model) now fail to compile when using dynamically added fields or common filters that are not explicitly declared in every schema.

Previously, the more permissive FilterQuery allowed such flexibility, enabling code reuse and reducing code duplication across models. The stricter typing of RootFilterQuery now forces developers to either:

  • Refrain from using dynamic fields or filters that are not explicitly defined in the schema.

  • Resort to less type-safe workarounds, such as using type assertions or @ts-ignore, to bypass TypeScript errors.

Steps to Reproduce

  1. Initialize a new Node.js project with TypeScript and install Mongoose version 8.5.5
  2. Define a Mongoose schema and a function that uses find with a filter containing fields that might not be strictly defined in the schema:
import mongoose, { Schema, Document, Model, Types } from 'mongoose';

// Define a simple schema with a limited number of fields
interface ExampleDoc extends Document {
  isActive: boolean;
  isCustom: boolean;
}

const ExampleSchema = new Schema<ExampleDoc>({
  isActive: Boolean,
  isCustom: Boolean
});

const ExampleModel: Model<ExampleDoc> = mongoose.model<ExampleDoc>('Example', ExampleSchema);

async function initializeRelatedParametersHelper<T extends Document>(
  model: Model<T>,
  createdForId: Types.ObjectId,
  createdForType: string,
  trainingZoneId: Types.ObjectId
): Promise<void> {
  // This should work in version 8.5.5 but throws an error in 8.6.0
  const defaults = await model
    .find({
      createdForType: 'system', // This field is not defined in ExampleSchema
      isActive: true,
      isCustom: false,
    })
    .lean()
    .exec();

  for (const defaultParam of defaults) {
    const newParam = new model({
      ...defaultParam,
      createdForId,
      createdForType,
      trainingZoneId,
      isCustom: false,
    });
    await newParam.save();
  }
}
  1. Compile and Run with Mongoose Version 8.5.5. The code should compile without errors.
  2. Upgrade to Mongoose Version 8.6.0
  3. Recompile the TypeScript code and observe the TypeScript error: Object literal may only specify known properties, and 'createdForType' does not exist in type 'RootFilterQuery'.

Expected Behavior

TypeScript should compile without errors, allowing the flexibility to use generic utility functions across different models with dynamic fields or query parameters not strictly defined in the schema.

@isnifer
Copy link

isnifer commented Sep 3, 2024

For example: $expr is not available now inside .find() as RootFilterQuery

@vkarpov15
Copy link
Collaborator

@isnifer We fixed $expr with #14845, will ship that release shortly.

@Towerss in this case, what do you think about just using a type assertion?

  const defaults = await model
    .find({
      createdForType: 'system', // This field is not defined in ExampleSchema
      isActive: true,
      isCustom: false,
    } as RootFilterQuery<T>)
    .lean()
    .exec();

That seems to compile, and #14764 can be very useful for catching issues like #14862, so I think retaining #14764 may have some merit. Another option would be to add some type restrictions on initializeRelatedParametersHelper() as follows:

// Define a simple schema with a limited number of fields
interface ExampleDoc extends Document {
  createdForType: string;
  isActive: boolean;
  isCustom: boolean;
}

const ExampleSchema = new Schema<ExampleDoc>({
  isActive: Boolean,
  isCustom: Boolean
});

const ExampleModel: Model<ExampleDoc> = mongoose.model<ExampleDoc>('Example', ExampleSchema);

async function initializeRelatedParametersHelper<T extends ExampleDoc>(
  model: Model<T>,
  createdForId: Types.ObjectId,
  createdForType: string,
  trainingZoneId: Types.ObjectId
): Promise<void> {

What do you think of these alternatives?

@vkarpov15 vkarpov15 added the discussion If you have any thoughts or comments on this issue, please share them! label Sep 3, 2024
@Towerss
Copy link
Author

Towerss commented Sep 4, 2024

@vkarpov15

Thanks for the suggestions. I understand that the stricter type definitions help with type safety and catching errors early. However, I still see this as a breaking change because it requires developers to update existing code that was previously valid, leading to TypeScript errors and CI/CD pipeline failures until the code is modified. This fits the definition of a breaking change as it breaks backward compatibility.

To adapt to the new types, I had to dynamically check for schema fields and build the filter object accordingly:

const filter: Partial<Record<keyof T, unknown>> = {};

if (model.schema.path('createdForType')) {
  (filter as any)['createdForType'] = 'system';
}
if (model.schema.path('isActive')) {
  (filter as any)['isActive'] = true;
}
if (model.schema.path('isCustom')) {
  (filter as any)['isCustom'] = false;
}

const defaults = await model.find(filter).lean().exec();

While this works, it adds complexity compared to the previous approach.

Regarding the suggestion to use a type assertion like:

const defaults = await model
  .find({
    createdForType: 'system',
    isActive: true,
    isCustom: false,
  } as RootFilterQuery<T>)
  .lean()
  .exec();

This approach is a quick fix but bypasses type safety, which could lead to potential bugs if the fields don’t exist in some schemas. It also reduces the benefits of stricter typing. If you are not careful, using type assertions can introduce bugs. For example, if createdForType truly does not exist in some schemas, the query could fail at runtime or return unexpected results. Also, future developers may not understand why a type assertion was used and could introduce changes that break assumptions, leading to hidden bugs.

I think a more gradual change—such as temporarily allowing both RootFilterQuery and FilterQuery with a deprecation notice—could have made the transition smoother for developers while still moving towards improved type safety in version 9.x

Thanks for considering this feedback.

@arenier
Copy link

arenier commented Sep 4, 2024

I'm a bit surprised of having my pipeline breaking this morning. I feel like it is Breaking Change too.

@nikzanda
Copy link

nikzanda commented Sep 4, 2024

Related issue

@Lordfirespeed
Copy link

Lordfirespeed commented Sep 6, 2024

I'm a bit surprised of having my pipeline breaking this morning. I feel like it is Breaking Change too.

The @types/node tests on DefinitelyTyped are failing because of this change - yes, it's breaking 😂

To be specific: the tests for mongoose-delete run queries on age, which isn't explicitly declared on the model, so raises type errors.

@dmint789
Copy link

dmint789 commented Sep 6, 2024

Would also like to see this reverted

vkarpov15 added a commit that referenced this issue Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants