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 updateMany from v8.5.5 to v8.6.0 #14862

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

Breaking TypeScript Change in updateMany from v8.5.5 to v8.6.0 #14862

Towerss opened this issue Sep 3, 2024 · 4 comments · Fixed by #14874
Labels
help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary

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,

In version 8.6.0, the type definition for the filter parameter in updateMany was changed from FilterQuery to RootFilterQuery. This change restricts the use of certain MongoDB operators (such as $pull) that were allowed in 8.5.5.

This change causes TypeScript errors in existing codebases that rely on the old type definition, making it a breaking change. According to Semantic Versioning, this should have warranted a major version bump rather than a minor version change.

I think this change is a breaking change for existing codebases that use MongoDB operators in the filter parameter of updateMany and suggest that it may require a major version bump according to SemVer principles.

This is my prod code snippet, which works with version 8.5.5:

			await GuardianModel.updateMany(
				{ $pull: { wards: { $in: [userId] } } },
				{ session },
			);

with version 8.6.0, I need to add something to replace the filter to keep Typescript happy:

			await GuardianModel.updateMany(
				{},
				// or undefined
				// or { _id: { $exists: true } }
				{ $pull: { wards: { $in: [userId] } } },
				{ session },
			);

This workaround does feel like a hack and is not intuitive, especially when your goal is to search all documents or apply a filter based on nested values.

Suggested Actions:

  • Revert the type change or consider bumping to a new major version.

  • Clearly document this change in the release notes to warn users.

Steps to Reproduce

  1. Initialize a new Node.js project with TypeScript.
  2. Install Mongoose version 8.5.5 first to demonstrate the working scenario, and then 8.6.0 to show the issue.
  3. Create a TypeScript file (index.ts) and define a simple Mongoose schema.
import mongoose, { Schema, Document } from 'mongoose';

// Define a simple Guardian schema
interface Guardian extends Document {
  wards: Schema.Types.ObjectId[];
}

const GuardianSchema = new Schema<Guardian>({
  wards: { type: [Schema.Types.ObjectId] }
});

const GuardianModel = mongoose.model<Guardian>('Guardian', GuardianSchema);

// Sample function that demonstrates the issue
async function testUpdateMany(userId: string) {
  // This should work in version 8.5.5 but throws an error in 8.6.0
  await GuardianModel.updateMany(
    { $pull: { wards: { $in: [userId] } } }, // Filter with $pull operator
    {},
    {}
  );
}
  1. Compile and Run the Example with Version 8.5.5
  2. Upgrade to Mongoose Version 8.6.0
  3. Run TypeScript compilation and observe the TypeScript error: Object literal may only specify known properties, and '$pull' does not exist in type 'RootFilterQuery'.

Expected Behavior

The TypeScript code should compile without errors as it did in version 8.5.5, allowing MongoDB update operators like $pull in the filter parameter of updateMany.

Actual Behavior: The TypeScript compiler throws an error in version 8.6.0 due to the stricter type definition RootFilterQuery.

@vkarpov15
Copy link
Collaborator

This seems like expected behavior - $pull isn't supported in the filter operator, nor are any update operators in general. So the following code will never update anything unless you happen to have a document with a $pull property

  await GuardianModel.updateMany(
    { $pull: { wards: { $in: [userId] } } }, // Filter with $pull operator
    {},
    {}
  );

Even worse, with the session property as the 2nd argument, the following code says "find all documents with a $pull property that is exactly equal to { wards: ... } and set a session property on it"

			await GuardianModel.updateMany(
				{ $pull: { wards: { $in: [userId] } } },
				{ session },
			);

So I think this is a case where the new TypeScript code is doing its job, catching code that is dangerously wrong.

@vkarpov15 vkarpov15 added the help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary label Sep 3, 2024
@Towerss
Copy link
Author

Towerss commented Sep 3, 2024

@vkarpov15

Thank you for the clarification! I understand that $pull and other update operators are not valid in the filter parameter and that TypeScript is correctly catching this mistake.

My intention was to create a reusable generic function to update documents across different models, but I realize now that I misunderstood the proper use of filter and update operators in this context. The stricter type definitions do help prevent mistakes like this.

I agree that it is beneficial to have TypeScript catch these kinds of errors early on, even if it does make the transition from older versions a bit challenging.

However, I do think it would be helpful to include a note in the documentation or release notes explaining this change and why it's now enforced. This would help developers understand the reasoning behind the stricter type checking, especially if they are upgrading from an older version where such issues might not have been caught by TypeScript.

Thanks again for your response, and I appreciate the work being done to make Mongoose safer and more robust!

@vkarpov15
Copy link
Collaborator

Where's a good place to add a note that you would have seen? We do have a note in the changelog from #14764, do you have any suggestions for other places to highlight this change?

@Towerss
Copy link
Author

Towerss commented Sep 3, 2024

@vkarpov15

Thank you for considering my feedback on where to add a note about the type changes!

Suggestions for Improving Documentation

To help developers notice this change, here are a few suggestions:

  • Changelog and Release Notes: Add a dedicated "Breaking Changes" or "Important TypeScript Changes" section to the changelog. This would make it easier for developers to spot crucial updates quickly.

  • Documentation for Affected Methods: A "TypeScript Note" or "Important Note" directly in the documentation pages for methods like updateMany and find could clarify the new restrictions, why they were introduced, and provide examples of correct usage.

  • Migration Guide: Consider adding a section in a Migration Guide to outline "TypeScript Changes" for developers upgrading from older versions. This could include common pitfalls and advice for updating code.

  • TypeScript Usage Documentation: Expanding any TypeScript-specific documentation with detailed explanations of the type changes would also be helpful.

Regarding the Breaking Change

I understand that this change improves type safety and helps catch errors early, which is beneficial. However, I still believe that this qualifies as a breaking change because it requires developers to update their existing code to adhere to the new types.

In my case, the stricter types caused TypeScript errors in code that was previously valid, and the CI/CD pipeline began rejecting pull requests until the code was updated. This behavior fits the definition of a breaking change, as it breaks backward compatibility and necessitates modifications to existing codebases.

A more gradual approach to introducing this change could have been:

  • Using a Union Type: Allowing both RootFilterQuery and FilterQuery temporarily would let developers transition more smoothly without immediate breakages.

  • Providing a Deprecation Notice: A note in the documentation or changelog indicating that the older type would be deprecated in a future major version would have given developers time to prepare for the change.

Something like:

    /** Creates a `updateMany` query: updates all documents that match `filter` with `update`. */
    /**TypeScript Note**: As of version 8.6.0, the `filter` parameter in `updateMany` now uses the stricter `RootFilterQuery` type. This means that only schema-defined fields or valid MongoDB filter operators are allowed. Update operators like `$pull`, `$set`, `$inc`, etc., are not valid in the `filter` parameter. This change improves type safety by catching potential errors at compile time. For more details, see [#14764](https://github.com/Automattic/mongoose/pull/14764). */
    updateMany<ResultDoc = THydratedDocumentType>(
      filter?: RootFilterQuery<TRawDocType> | FilterQuery<TRawDocType>,
      update?: UpdateQuery<TRawDocType> | UpdateWithAggregationPipeline,
      options?: (mongodb.UpdateOptions & MongooseUpdateQueryOptions<TRawDocType>) | null
    ): QueryWithHelpers<UpdateWriteOpResult, ResultDoc, TQueryHelpers, TRawDocType, 'updateMany', TInstanceMethods>;

This approach would have balanced the benefits of improved type safety with a smoother developer experience.

Thanks again for your consideration and for the great work on Mongoose!

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants