-
Notifications
You must be signed in to change notification settings - Fork 70
v6 #141
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
v6 #141
Conversation
…ete, countDocuments, estimatedDocumentCount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR prepares the mquery library for a v6 release with two main changes: removing the debug dependency for security reasons and upgrading to MongoDB Node driver v6.
- Remove
debugproduction dependency due to security concerns - Upgrade MongoDB Node driver from v5 to v6 in dev dependencies and upgrade Mocha from v9 to v11
- Update test code to handle MongoDB driver v6's new return format for
findOneAndUpdate()operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| package.json | Removes debug dependency, upgrades MongoDB driver to v6 and Mocha to v11 |
| lib/mquery.js | Removes debug import and all debug logging statements throughout the codebase |
| test/index.js | Updates tests to handle MongoDB driver v6's changed return format for findOneAndUpdate operations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…mquery into vkarpov15/remove-old-methods
BREAKING CHANGE: remove count and findOneAndRemove, add findOneAndDelete, countDocuments, estimatedDocumentCount
…AndX set conditions not update, add findOneAndReplace Fix #104 Re: Automattic/mongoose#15363
|
From the looks of our dependent graph, no active open source projects of size, outside mongoose, are using this module. That does not mean there aren't other non-public, internal applications using this (that's how I used this module). Back when I developed this and was actively using it outside mongoose, it was a very convenient way to see what was happening within my apps. There are other tools for debugging but in very locked down production environments, sometimes As for security being a reason to remove |
|
That's a fair concern re: internal applications. I'm open to using Node's debuglog instead if that helps. It looks like debuglog behaves almost exactly the same as debug, is that correct? Re: "I see the risk as the same as any other module on npm, including mongoose itself.", I don't entirely agree. Because debug is so widely used (6th most depended on package on npm), it is more likely to be targeted. I see debug integration as more of a nice-to-have for this package, and I imagine users are more likely to write debugging logic in their own code, but I could be wrong. |
Co-authored-by: Aaron Heckmann <aaron.heckmann+github@gmail.com>
|
I don't feel strongly. I'm +1 on whatever direction you feel is best. |
Co-authored-by: Aaron Heckmann <aaron.heckmann+github@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| denied.findOneAndReplace = function(self) { | ||
| const keys = Object.keys(denied.findOneAndUpdate); | ||
| let err; | ||
|
|
||
| keys.every(function(option) { | ||
| if (self.options[option]) { | ||
| err = option; | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
|
|
||
| return err; | ||
| }; |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The findOneAndReplace permission validation function references denied.findOneAndUpdate keys instead of its own keys. This means it won't have any denied options defined (limit, skip, batchSize, tailable), and the validation will always pass.
You should either:
- Add the denied options for
findOneAndReplaceafter the function definition (similar tofindOneAndUpdate), or - Reference
denied.findOneAndReplacekeys instead ofdenied.findOneAndUpdatekeys on line 57.
Suggested fix:
denied.findOneAndReplace = function(self) {
const keys = Object.keys(denied.findOneAndReplace);
// ... rest of function
};
// Add denied options after the function
denied.findOneAndReplace.limit =
denied.findOneAndReplace.skip =
denied.findOneAndReplace.batchSize =
denied.findOneAndReplace.tailable = true;| * | ||
| * #### Available options | ||
| * | ||
| * - `new`: bool - true to return the modified document rather than the original. defaults to true |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation mentions the new option, but MongoDB Node driver v6 uses returnDocument: 'after' instead of new: true. The documentation should be updated to reflect the correct option name for MongoDB driver v6.
Suggested change:
* - `returnDocument`: string - 'before' to return the original document, 'after' to return the modified document. defaults to 'before'| * - `new`: bool - true to return the modified document rather than the original. defaults to true | |
| * - `returnDocument`: string - 'before' to return the original document, 'after' to return the modified document. defaults to 'before' |
| * | ||
| * #### Available options | ||
| * | ||
| * - `new`: bool - true to return the modified document rather than the original. defaults to true |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation mentions the new option, but MongoDB Node driver v6 uses returnDocument: 'after' instead of new: true. The documentation should be updated to reflect the correct option name for MongoDB driver v6.
Suggested change:
* - `returnDocument`: string - 'before' to return the original document, 'after' to return the modified document. defaults to 'before'| * - `new`: bool - true to return the modified document rather than the original. defaults to true | |
| * - `returnDocument`: string - 'before' to return the original document, 'after' to return the modified document. defaults to 'before' |
| }; | ||
|
|
||
| /** | ||
| * Executes a `count` Query |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Executes a count Query" but this should be "Executes an estimatedDocumentCount Query" to match the actual method name.
| * Executes a `count` Query | |
| * Executes an `estimatedDocumentCount` Query |
|
|
||
| /** | ||
| * Exectues the query as a count() operation. | ||
| * Executes the query as a countDocuments() operation. |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "Exectues" - should be "Executes".
| * await query.findOneAndReplace(replacement) // returns Query | ||
| * query.findOneAndReplace(replacement) // returns Query |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states that calling findOneAndReplace(replacement) with a single argument will set the replacement document. However, according to the new behavior described in the PR (point #5), a single argument should set the query filter (conditions), not the replacement. This is inconsistent with the implementation where the single argument sets conditions when calling Query.canMerge(criteria).
The example should be:
* await query.findOneAndReplace(conditions) // returns Query
* query.findOneAndReplace(conditions) // returns Query| * await query.findOneAndReplace(replacement) // returns Query | |
| * query.findOneAndReplace(replacement) // returns Query | |
| * await query.findOneAndReplace(conditions) // returns Query | |
| * query.findOneAndReplace(conditions) // returns Query |
| * @param {Object|Query} [query] | ||
| * @param {Object} [replacement] | ||
| * @param {Object} [options] | ||
| * @see mongodb http://www.mongodb.org/display/DOCS/findAndModify+Command |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link references the old MongoDB documentation for findAndModify command. Since this is a v6 release targeting MongoDB Node driver v6, the link should be updated to point to current MongoDB documentation.
| * @see mongodb http://www.mongodb.org/display/DOCS/findAndModify+Command | |
| * @see mongodb https://www.mongodb.com/docs/manual/reference/method/db.collection.findOneAndReplace/ |
| const conds = this._conditions; | ||
| const options = this._optionsForExec(); | ||
| debug('estimatedDocumentCount', this._collection.collectionName, conds, options); | ||
| return this._collection.estimatedDocumentCount(conds, options); |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _estimatedDocumentCount method is passing conds (conditions) to estimatedDocumentCount(), but MongoDB's estimatedDocumentCount() doesn't accept a filter parameter - it estimates the count of all documents in the collection. Passing conditions to it may cause an error or be ignored.
According to MongoDB documentation, estimatedDocumentCount() only accepts options, not a filter. Consider removing the conds parameter:
return this._collection.estimatedDocumentCount(options);| return this._collection.estimatedDocumentCount(conds, options); | |
| return this._collection.estimatedDocumentCount(options); |
| * estimatedDocumentCount(match, options) | ||
| */ | ||
| async estimatedDocumentCount(match, options) { | ||
| return this.collection.estimatedDocumentCount(match, options); |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The estimatedDocumentCount method is passing match parameter to MongoDB's estimatedDocumentCount(), but this method doesn't accept a filter/match parameter - it only accepts options. According to MongoDB documentation, estimatedDocumentCount() estimates the count of all documents in the collection based on collection metadata, and cannot filter by conditions.
The signature should be:
async estimatedDocumentCount(options) {
return this.collection.estimatedDocumentCount(options);
}And the comment should be updated to:
/**
* estimatedDocumentCount(options)
*/| * estimatedDocumentCount(match, options) | |
| */ | |
| async estimatedDocumentCount(match, options) { | |
| return this.collection.estimatedDocumentCount(match, options); | |
| * estimatedDocumentCount(options) | |
| */ | |
| async estimatedDocumentCount(options) { | |
| return this.collection.estimatedDocumentCount(options); |
Summary
Branch for v6 release, changes:
debugas prod dependency due to the recent security issue. I don't think it is worth the risk to Mongoose to have debug as a direct dependency of mquery, especially since Mongoose itself doesn't use debug.includeResultMetadatabecause MongoDB driver v6 returns just the document fromfindOneAndUpdate()by default.updateOne(v)is equivalent toupdateOne(null, v)is surprising and not very idiomatic these days. Devs tend to think more in terms of TypeScript's optional arguments rules, and chaining APIs are less popular.Examples