chore(shell-api): add test and type updates for sort #2520
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds test coverage and type updates for the sort option in MongoDB's updateOne and replaceOne operations. The changes ensure that the shell API properly supports sorting to determine which document gets updated when multiple documents match the filter criteria.
- Adds comprehensive integration tests for
updateOneandreplaceOnewith sort options - Updates TypeScript type definitions to include the
sortparameter - Updates JSDoc comments to document the new sort option
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/shell-api/src/integration.spec.ts | Adds test cases verifying sort functionality for updateOne and replaceOne operations |
| packages/shell-api/src/collection.ts | Updates type signature and documentation for updateOne method to include sort option |
| packages/service-provider-core/src/writable.ts | Updates interface type definition to include sort option in updateOne method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gagik
added
the
no-title-validation
gagik
changed the title
gagik
changed the title
| @@ -319,7 +319,7 @@ export default interface Writable { | |||
| collection: string, | |||
| filter: Document, | |||
| update: Document, | |||
| options?: UpdateOptions, | |||
| options?: UpdateOptions & { sort?: Document }, | |||
There was a problem hiding this comment.
I was about to say "shouldn't that just be added to UpdateOptions?" but then realised it is also used for updateMany. Wondering if we should have a type next to UpdateOptions wherever it is defined that's type UpdateOneOptions = UpdateOptions & { sort?: Document } rather.
There was a problem hiding this comment.
But also shouldn't sort already be on an underlying driver type?
There was a problem hiding this comment.
this is some weirdness from the driver: #2511 (comment)
There was a problem hiding this comment.
I'm wondering if we should at least make an UpdateOneOptions on our end so this is contained once, but that's not blocking.
There was a problem hiding this comment.
for now I'd match what the driver does so we don't end up with our type system too deeply, also have some precedent for this (admittedly that was also done by me I believe..)
mongosh/packages/shell-api/src/database.ts
Line 452 in a845602
| @@ -930,7 +930,7 @@ export class Collection< | |||
| * @param {Object} filter - The filter. | |||
| * @param {Object} replacement - The replacement document for matches. | |||
| * @param {Object} options - The replace options. | |||
| * <upsert, writeConcern, collation, hint> | |||
| * <upsert, writeConcern, collation, hint, sort> | |||
There was a problem hiding this comment.
Shouldn't sort be added to the options that replaceOne takes? Or is it already on there? I suppose there's a test so somehow it has to be working.
Followed up from the driver update, tests to ensure the new sort option works.