feat(NODE-6893): Distinguish between command-level options and index-level options when creating indexes - #5012
seanrmilligan wants to merge 24 commits into
Conversation
93afa52 to
756f24d
Compare
756f24d to
5304629
Compare
5304629 to
991884f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect overload typing, command-option typing, and serialization of language options.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds opt-in passthrough for unknown index options while separating index and command options for createIndex.
Changes:
- Adds
allowUnknownIndexOptionshandling. - Introduces
IndexOptionsandCreateIndexOptions. - Updates operation construction, exports, and test coverage.
File summaries
| File | Summary |
|---|---|
test/unit/operations/indexes.test.ts |
Tests option filtering and passthrough behavior. |
test/unit/collection.test.ts |
Tests command and index option separation. |
test/integration/index-management/create_indexes_option_validation.test.ts |
Adds integration validation coverage. |
test/integration/index_management.test.ts |
Tests unknown-option handling. |
test/integration/crud/abstract_operation.test.ts |
Updates operation construction tests. |
src/utils.ts |
Resolves inherited command options. |
src/operations/indexes.ts |
Defines option types, filtering, and passthrough logic. |
src/operations/create_collection.ts |
Updates internal index creation. |
src/index.ts |
Exports new public option types. |
src/gridfs/upload.ts |
Documents future option migration. |
src/db.ts |
Updates createIndex operation construction. |
src/collection.ts |
Adds overloads and passthrough support. |
Review details
Suppressed comments (7)
src/collection.ts:739
- This new TODO also has no NODE/DRIVERS ticket identifier, unlike the repository's established TODO convention. Please link the future default-change work to a concrete ticket before merging.
// TODO(seanrmilligan): default this to true and remove the parameter in a future major
// release. Index options live on each index description, so nothing on this path
// contaminates them -- but flipping it turns today's silently dropped unknown option into
// a server error.
src/collection.ts:655
- The passthrough overload leaves
commandOptionsoptional, but the implementation usescommandOptions == nullto select the legacy allowlist path. A valid two-argument call using the newIndexOptionsshape (for example{ defaultLanguage: 'english' }) is therefore accepted by TypeScript and then silently drops the option. Require the third argument for this overload, or use an unambiguous runtime discriminator.
indexOptions?: IndexOptions,
commandOptions?: CreateIndexOptions
src/gridfs/upload.ts:278
- This TODO refers to
validateOptions, but that is not the option controlling this code path; the new API usesallowUnknownIndexOptions. The stale name will mislead anyone implementing the GridFS migration, so update the comment to the actual overload/flag.
// the index option allowlist. When validateOptions defaults to false, move the command
// options into the third parameter.
src/gridfs/upload.ts:387
- This TODO also names the nonexistent
validateOptionssetting. Refer to the legacy two-parametercreateIndexpath instead so the follow-up work is tied to the API that actually controls the behavior.
// TODO(NODE-6893): timeoutMS is a command option; move it into the third parameter when
// validateOptions defaults to false.
src/operations/indexes.ts:479
- A command comment is normally any BSON value (
CommandOperationOptions.commentisunknown), but this new public type narrows it toDocument. The string comments used by the addedcreateIndextests are consequently not typeable through the new overload; use the existingunknowncomment type.
comment?: Document;
src/operations/indexes.ts:479
- This new public option is documented as enabling comments, but
CreateIndexesOperation.buildCommandDocumentstill emits onlycommitQuorumfrom the command options, socommentis silently dropped. The added unit test currently asserts the opposite behavior; either add comment to the command document or remove it from this API until it is supported.
/**
* Enables users to specify an arbitrary comment to help trace the operation through
* the database profiler, currentOp and logs. The default is to not send a value.
*
* @see https://www.mongodb.com/docs/manual/reference/command/createIndexes/
*
* @sinceServerVersion 4.4
*/
comment?: Document;
src/operations/indexes.ts:610
- Repository TODOs consistently carry a NODE/DRIVERS ticket identifier, but this TODO explicitly asks for a future NODE ticket without one. Please attach the follow-up ticket so the planned default change remains traceable.
// TODO(seanrmilligan): Add NODE ticket to set to remove allowUnknownIndexOptions with
// a default behavior of true in a future 8.0.0 release
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
PavelSafronov
left a comment
There was a problem hiding this comment.
There are a number of type additions, but no new types tests. You'll need to add these.
|
2 tests are failing across the variants (looks relevant to changes): |
I suspect this is because the wrong server version was used. Waiting on evergreen to confirm latest push. |
PavelSafronov
left a comment
There was a problem hiding this comment.
Can you also add release notes to the PR description? We'll definitely need those.
addaleax
left a comment
There was a problem hiding this comment.
This looks like something that will eventually require compatibility work in mongosh, let's make sure to give this ticket the appropriate downstream changes marker
| * @param commandOptions - Optional settings for the `createIndexes` command | ||
| * @param allowUnknownIndexOptions - When `true`, index options the driver does not recognise are | ||
| * sent to the server instead of being dropped. Defaults to `false`; this will become the only | ||
| * behaviour in a future major release. |
There was a problem hiding this comment.
Should this also be deprecated then?
There was a problem hiding this comment.
Sean to add TODO(NODE-7868) to allowUnknownIndexOptions parameter.
There was a problem hiding this comment.
Sean to consider declaring this function as a two-parameter, and three-parameter overload. If so, remove optionality for second parameter, making it | undefined. Mark new three parameter overload as deprecated so that 8.0 goes back to only having single two-parameter overload but with new passthrough behavior always on.
There was a problem hiding this comment.
Sean implemented this: createIndexes now has two-parameter and three-parameter overloads, with the three-parameter one deprecated.
Note that the arity is flipped relative to createIndex, but this is correct. The rule we’re applying is deprecate whatever won’t exist in 8.0:
createIndex: the merged options bag goes away, so the two-parameter form is legacy.createIndexes:allowUnknownIndexOptionsgoes away once passthrough is the default (NODE-7868), so the three-parameter form is the temporary one.
Done. |
| /** | ||
| * Creates the index in the background, yielding whenever possible. | ||
| * | ||
| * @deprecated Index options will be removed from this type in a future major release. Pass |
There was a problem hiding this comment.
These @deprecated attributes are no longer necessary, since the legacy 2-param method carries the @deprecated attribute now.
There was a problem hiding this comment.
The spec provides for:
CreateIndexOptionsforcreateIndex, andCreateIndexesOptionsforcreateIndexes
Note that CreateIndexOptions and CreateIndexesOptions have the same set of members so there isn't a functional difference. Where there is a difference is how:
- (a)
createIndex's two parameter overload erroneously usedCreateIndexesOptionsinstead ofCreateIndexOptions(mostly harmless) - (b)
CreateIndexesOptionscontained options for the command and options for the index (bad). It was essentially a union ofCreateIndexesOptionsandIndexOptionsfrom the spec.
Deprecating the two-parameter overload moves us toward using the correct type (CreateIndexOptions singular) in the function signature.
Deprecating the individual fields in CreateIndexesOptions (plural) is also necessary to align createIndexes with the members that the spec provides as options for the command.
There was a problem hiding this comment.
Right you are, I missed that.
For the plural method these field tags are the only signal that index options don't belong in the command input, so we definitely do want them there.
The issue with this approach, though, is that the Pick pulls in all the @deprecated attributes as well. So I think we need the explicit field definitions here after all. I tried this locally with unique and sparse and it worked.
Worth adding a type test for this as well:
expectDeprecated(legacyOptions.unique);
expectNotDeprecated(indexDescription.unique);
johnmtll
left a comment
There was a problem hiding this comment.
Pretty much all nitpicks, feel free to skip as many as you like. Only 2 real requests, and they're just test restructuring.
https://github.com/mongodb/node-mongodb-native/pull/5012/changes#r4031303695 https://github.com/mongodb/node-mongodb-native/pull/5012/changes#r4040901817
Awesome stuff Sean. Thanks!
| }); | ||
|
|
||
| describe('and command options are passed in the third parameter', function () { | ||
| it('keeps a comment out of the index description', async function () { |
There was a problem hiding this comment.
This test seems to ensure that indexOptions are exclusively considered and not command options. I think this does a good job asserting that, but the following two tests ('sends maxTimeMS on the command and not in the index description', 'sends a session on the command and not in the index description') are redundant because they effectively assert the same thing. the session test actually tests a little bit more than just that though, so I suggest supplanting this test, and the following test, with that one.
| }); | ||
| }); | ||
|
|
||
| context('#createIndex', () => { |
There was a problem hiding this comment.
I think these tests are a little hard to read and they escape the scope of the intended unit to test. The intent is just to test how CreateIndexesOperation is constructed. I think there are some places where we breach into the behaviour of fromIndexSpecification and executeOperation but that behaviour is external to collections.createIndex, and is already covered by indexes.test.ts. I'd prefer we reduce the test footprint to focus solely on the unit being tested (createIndex).
If you opt to do 2 tests here, you could do:
-
when commandOptions are supplied: it('enables passthrough and passes index/command options separately')
In here, assert an unknown option is forwarded, check that command opts come only from third param, and indexopts only come from second -
when commandOptions are not supplied: it('disables passthrough and passes an index/command option composite').
In here, assert an unknown option is not forwarded, 2nd parameter opts lands on command and index respectively
…sOperation Collapse the private constructor to a single signature and make the trailing options parameters of the constructor and both static factories mandatory but nullable (`| undefined`). Callers must now pass `undefined` explicitly, so "forgot to pass command options" is a compile error rather than being indistinguishable from "there are no command options". Drops the redundant constructor overloads and the third `fromIndexSpecification` signature, and updates every call site (including tests) to pass `undefined` where no command options apply.
…eateIndex overload Make `indexOptions` and `commandOptions` on the three parameter `createIndex` overload mandatory (still nullable). With both optional, a two argument call that failed the legacy overload on an unknown option fell through to this overload, since `IndexOptions` accepts arbitrary keys. That silently dropped the compile error, hid the deprecation warning, and matched a pass-through signature for a call that takes the legacy path at runtime. Two argument calls can now only match the legacy overload, so the unknown option error is reported again. Restore the `@ts-expect-error` on the two tests that deliberately pass an unknown option on the legacy path.
`IndexOptions` is new in this change, and `background` has been ignored by the server since 4.2 while the driver's minimum supported server is 4.4. Rather than add the field only to deprecate and remove it later, leave it out. GridFS no longer sends `background` when creating its indexes, which has no effect on the server. The released `CreateIndexesOptions.background` is kept for the two parameter path and remains deprecated until v8.
Description
Summary of Changes
Differentiates between options for indexes and options for commands in the
createIndexandcreateIndexesAPI.More granular changes:
createIndexoverload with three parameters to differentiate between options for the index and options for the command@deprecatedtag to thecreateIndexoveroad with two parameters (the mixed index/command options path), to be removed in a future release.createIndexfrom the deprecated two-parameter overload to the preferred three-parameter overload.allowUnknownIndexOptionstoggle.IndexOptionsfrom the specifications repositoryCreateIndexesOptionswith the specifications repository by deprecating options related to indexes and leaving only options related to commands.Notes for Reviewers
Types
There are many similar type names floating around. Some come by design from the spec, and some from the history of the API in this area. Note the differences between:
CreateIndexOptions-- options for the commandcreateIndexCreateIndexesOptions-- options for the commandcreateIndexesIndexOptions-- options for an indexCreateIndexesOperation-- the command to create indexes, containing all relevant parts including index property names, index options, and command options are fed.CreateIndexesOperationThe
CreateIndexesOperationcommand object is created on both thecreateIndexandcreateIndexespaths. It accepts an array of indexes, where thecreateIndexpath is the special case of an array with only one item. There is no correspondingCreateIndexOperationfor creating a single index.allowUnknownIndexOptionsallowUnknownIndexOptionsis inferred transparently on behalf of the consumer of thecreateIndexAPI by detecting whether the caller called the two parameter overload (old behavior, set tofalse) or the three parameter overload (new, set totrue). Using thecreateIndex(<3>)overload is considered as opting into the new passthrough behavior.allowUnknownIndexOptionscannot be inferred on thecreateIndexespath because the types oncreateIndexesalready align with the spec.What is the motivation for this change?
This is in support of achieving "passthrough" behavior where options are validated by the server rather than the driver. Validation of options by the server rather than the driver accomplishes two goals:
Release Highlight
Release notes highlight
createIndexwhich separates options for the index from options for the command. This new overload also uses pass-through behavior by default.createIndexoverload, which retains driver-side options validation, has been deprecated and will be removed in a future major release.createIndexesAPI. This flag will be removed and pass-through will become the default in a future major release.Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript