Skip to content

feat(NODE-6893): Distinguish between command-level options and index-level options when creating indexes - #5012

Open
seanrmilligan wants to merge 23 commits into
mongodb:mainfrom
seanrmilligan:sean.milligan/createIndex
Open

seanrmilligan wants to merge 23 commits into
mongodb:mainfrom
seanrmilligan:sean.milligan/createIndex

Conversation

@seanrmilligan

@seanrmilligan seanrmilligan commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

Description

Summary of Changes

Differentiates between options for indexes and options for commands in the createIndex and createIndexes API.

More granular changes:

  • Create a createIndex overload with three parameters to differentiate between options for the index and options for the command
  • Add a @deprecated tag to the createIndex overoad with two parameters (the mixed index/command options path), to be removed in a future release.
  • Migrate internal calls to createIndex from the deprecated two-parameter overload to the preferred three-parameter overload.
  • Maintain parity with existing behavior (filtering unknown index options) by way of an allowUnknownIndexOptions toggle.
  • Import interface IndexOptions from the specifications repository
  • Align interface CreateIndexesOptions with 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 command createIndex
  • CreateIndexesOptions -- options for the command createIndexes
  • IndexOptions -- options for an index
  • CreateIndexesOperation -- the command to create indexes, containing all relevant parts including index property names, index options, and command options are fed.
CreateIndexesOperation

The CreateIndexesOperation command object is created on both the createIndex and createIndexes paths. It accepts an array of indexes, where the createIndex path is the special case of an array with only one item. There is no corresponding CreateIndexOperation for creating a single index.

allowUnknownIndexOptions

allowUnknownIndexOptions is inferred transparently on behalf of the consumer of the createIndex API by detecting whether the caller called the two parameter overload (old behavior, set to false) or the three parameter overload (new, set to true). Using the createIndex(<3>) overload is considered as opting into the new passthrough behavior.

allowUnknownIndexOptions cannot be inferred on the createIndexes path because the types on createIndexes already 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:

  1. It presents a more consistent experience for users across drivers.
  2. Where the language allows, users can now send an index option supported by the server before the driver has even added the option to the options type.

Release Highlight

Release notes highlight

  • Adds support for "pass-through" behavior for index options: all index options passed (when opted in) will now be validated by the server rather than the driver. This will become the default behavior in a future release.
  • Adds an overload for createIndex which separates options for the index from options for the command. This new overload also uses pass-through behavior by default.
  • The original createIndex overload, which retains driver-side options validation, has been deprecated and will be removed in a future major release.
  • Adds an opt-in flag to new passthrough behavior to the createIndexes API. This flag will be removed and pass-through will become the default in a future major release.

Double check the following

  • Lint is passing (npm run check:lint)
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@seanrmilligan
seanrmilligan force-pushed the sean.milligan/createIndex branch from 93afa52 to 756f24d Compare August 26, 2026 20:18
@dariakp dariakp changed the title Allow passthrough options on createIndexes feat(NODE-6893): Allow passthrough options on createIndexes Aug 31, 2026
@seanrmilligan
seanrmilligan force-pushed the sean.milligan/createIndex branch from 756f24d to 5304629 Compare September 9, 2026 19:16
Comment thread src/gridfs/upload.ts Outdated
Comment thread src/operations/indexes.ts Outdated
*
* @remarks This option is ignored by the server.
* @see https://www.mongodb.com/docs/manual/reference/command/createIndexes/
* @deprecated 4.2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We removed 4.2 support recently, so we may not need this method at all. Can you verify?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on chat offline, deprecated here means discouraged but not removed. So the option remains until the next major version of the server removes it. At that time, we can remove it too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ticket to v8 epic to remove this field

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought adding a new field that the documentation is non-functional on arrival so that we can create a ticket, schedule it, and remove it in a strict window as a breaking change feels like a silly amount of overhead so I've gone the route of not introducing it here in the first place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread src/operations/indexes.ts Outdated
Comment thread test/integration/index-management/create_indexes_option_validation.test.ts Outdated
Comment thread src/operations/indexes.ts Outdated
Comment thread src/operations/indexes.ts Outdated
Comment thread src/utils.ts Outdated
Comment thread src/operations/indexes.ts Outdated
collectionName: string,
indexes: IndexDescription[],
allowUnknownIndexOptions: boolean,
commandOptions?: CreateIndexesOptions | CreateIndexOptions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional params on a private constructor doesn't buy us anything and is detrimental, suggest making this mandatory. (Detrimental because "I forgot to pass a parameter" and "I don't have anything to specify" become indistinguishable.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done-ish? I can remove the optionality but as the API surface takes optional parameters, I still have to type the constructor as | undefined. This forces the caller to pass the parameter, even if the parameter has a value of undefined.

  createIndex(
    keys: IndexSpecification,
    indexOptions?: IndexOptions,
    commandOptions?: CreateIndexOptions
  ): Promise<string>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your | undefined intuition is correct, but the optionality isn't coming from the createIndex overloads, rather from fromIndexDescriptionArray.

Try this:

  1. drop the first two private constructors of CreateIndexesOperation
  2. make the following change to this constructor: commandOptions: CreateIndexesOptions | CreateIndexOptions | undefined
  3. fromIndexDescriptionArray update commandOptions: CreateIndexesOptions | undefined
  4. fromIndexSpecification overload 1:
    1. indexOptions: IndexOptions | undefined,
    2. commandOptions: CreateIndexOptions | undefined
  5. fromIndexSpecification overload 2
    1. indexOptions: CreateIndexesOptions | IndexOptions | undefined,
    2. commandOptions: CreateIndexOptions | undefined
  6. drop 3rd fromIndexSpecification

Now this will force us to explicitly pass undefined to fromIndexSpecification as necessary, including in tests.
I just tried this locally on top of this PR and it seems to work. Holler if you run into issues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sean to push changes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@seanrmilligan
seanrmilligan force-pushed the sean.milligan/createIndex branch from 5304629 to 991884f Compare September 11, 2026 13:55
@seanrmilligan
seanrmilligan marked this pull request as ready for review September 11, 2026 13:56
@seanrmilligan
seanrmilligan requested a review from a team as a code owner September 11, 2026 13:56
Copilot AI lite review requested due to automatic review settings September 11, 2026 13:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 allowUnknownIndexOptions handling.
  • Introduces IndexOptions and CreateIndexOptions.
  • 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 commandOptions optional, but the implementation uses commandOptions == null to select the legacy allowlist path. A valid two-argument call using the new IndexOptions shape (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 uses allowUnknownIndexOptions. 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 validateOptions setting. Refer to the legacy two-parameter createIndex path 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.comment is unknown), but this new public type narrows it to Document. The string comments used by the added createIndex tests are consequently not typeable through the new overload; use the existing unknown comment type.
  comment?: Document;

src/operations/indexes.ts:479

  • This new public option is documented as enabling comments, but CreateIndexesOperation.buildCommandDocument still emits only commitQuorum from the command options, so comment is 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.

Comment thread src/operations/indexes.ts Outdated
*
* This options is only supported by servers \>= 6.0.
*/
clustered?: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. If clustered is only every present on the response*, why do we need to expose it on a request object.

*Side question: how/when do we return clustered to the user? I'm not finding it yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I adopted it wholesale from the spec's IndexOptions interface (clustered seen here: https://github.com/mongodb/specifications/blob/a9adc41b6eed59fd1197e852a2d8dccc4eaf16fb/source/index-management/index-management.md?plain=1#L806) but I'm not seeing IndexOptions itself used as the return type on any function, so perhaps it's a leftover?

We could bring that and the deprecated 4.2 field to the maintainers and see if it just needs a little cleanup.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to bring this up with the maintainers. My thought is that we should remove clustered and background, but let's see.

For clustered, that information comes through on the Document in this signature:

export type IndexDescriptionInfo = Omit<IndexDescription, 'key' | 'version'> & {
  key: { [key: string]: IndexDirection };
  v?: IndexDescription['version'];
} & Document;          // ← here

So then you can retrieve that data like this:

await db.createCollection('c', {
  clusteredIndex: { key: { _id: 1 }, unique: true, name: 'my_clustered_id' }
});
await db.collection('c').listIndexes().toArray();
// [ { v: 2, key: { _id: 1 }, name: 'my_clustered_id', unique: true, clustered: true } ]

Reading clustered works now, so there's no reason to add it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sean to remove clustered as the listIndexes API does not use the IndexOptions model, and the spec is vague on the type of Cursor. Drive spec conversation separately.

Comment thread src/operations/indexes.ts Outdated
Comment thread src/operations/indexes.ts Outdated
Comment thread src/utils.ts Outdated
Comment thread src/collection.ts
Comment thread test/integration/index-management/create_indexes_option_validation.test.ts Outdated
Comment thread test/integration/index-management/create_indexes_option_validation.test.ts Outdated

@PavelSafronov PavelSafronov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of type additions, but no new types tests. You'll need to add these.

Comment thread src/operations/indexes.ts Outdated
Comment thread src/operations/indexes.ts Outdated
Comment thread src/operations/indexes.ts Outdated
*
* This options is only supported by servers \>= 6.0.
*/
clustered?: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. If clustered is only every present on the response*, why do we need to expose it on a request object.

*Side question: how/when do we return clustered to the user? I'm not finding it yet.

Comment thread src/operations/indexes.ts Outdated
const validProvidedOptions = Object.entries(description).filter(([optionName]) =>
VALID_INDEX_OPTIONS.has(optionName)
const providedOptions = Object.entries(description).filter(
([optionName]) => allowUnknownIndexOptions || VALID_INDEX_OPTIONS.has(optionName)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we need to test optionName !== 'key' here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Filtered out key even when allowUnknownIndexOptions is true because I saw it is re-added by the caller to resolveIndexDescription

Comment thread src/utils.ts Outdated
@tadjik1

tadjik1 commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

2 tests are failing across the variants (looks relevant to changes):

[2026/09/11 16:19:54.080]   2 failing
[2026/09/11 16:19:54.080]   1) createIndex option validation
[2026/09/11 16:19:54.080]        when command options are given (three parameter form)
[2026/09/11 16:19:54.080]          creates an index using a server option the driver does not know about:
[2026/09/11 16:19:54.080]      MongoServerError: Error in specification { prepareUnique: true, key: { e: 1 }, name: "e_1" } :: caused by :: The field 'prepareUnique' is not valid for an index specification. Specification: { prepareUnique: true, key: { e: 1 }, name: "e_1" }
[2026/09/11 16:19:54.080]       at Connection.sendCommand (src/cmap/connection.ts:174:4)
[2026/09/11 16:19:54.080]       at processTicksAndRejections (node:internal/process/task_queues:104:5)
[2026/09/11 16:19:54.080]       at async Connection.command (src/cmap/connection.ts:174:4)
[2026/09/11 16:19:54.080]       at async Server.command (src/sdam/server.ts:68:58)
[2026/09/11 16:19:54.080]       at async executeOperationWithRetries (src/operations/execute_operation.ts:92:6)
[2026/09/11 16:19:54.080]       at async executeOperation (src/operations/execute_operation.ts:24:2576)
[2026/09/11 16:19:54.080]       at async Collection.createIndex (src/collection.ts:151:267)
[2026/09/11 16:19:54.080]       at async Context.<anonymous> (test/integration/index-management/create_indexes_option_validation.test.ts:166:9)
[2026/09/11 16:19:54.080] 
[2026/09/11 16:19:54.080]   2) createIndexes option validation
[2026/09/11 16:19:54.080]        when command options are given
[2026/09/11 16:19:54.080]          creates an index using a server option the driver does not know about:
[2026/09/11 16:19:54.080]      MongoServerError: Error in specification { key: { e: 1 }, name: "e_1", prepareUnique: true } :: caused by :: The field 'prepareUnique' is not valid for an index specification. Specification: { key: { e: 1 }, name: "e_1", prepareUnique: true }
[2026/09/11 16:19:54.080]       at Connection.sendCommand (src/cmap/connection.ts:174:4)
[2026/09/11 16:19:54.080]       at processTicksAndRejections (node:internal/process/task_queues:104:5)
[2026/09/11 16:19:54.080]       at async Connection.command (src/cmap/connection.ts:174:4)
[2026/09/11 16:19:54.080]       at async Server.command (src/sdam/server.ts:68:58)
[2026/09/11 16:19:54.080]       at async executeOperationWithRetries (src/operations/execute_operation.ts:92:6)
[2026/09/11 16:19:54.080]       at async executeOperation (src/operations/execute_operation.ts:24:2576)
[2026/09/11 16:19:54.080]       at async Collection.createIndexes (src/collection.ts:187:170)
[2026/09/11 16:19:54.080]       at async Context.<anonymous> (test/integration/index-management/create_indexes_option_validation.test.ts:357:9)

@seanrmilligan

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/operations/indexes.ts
Comment thread src/operations/indexes.ts
}

// Maps to `IndexOptions` in
// https://github.com/mongodb/specifications/blob/6f64d0ee3ae49edbdb30eb995f3e29549e8cfa6a/source/index-management/index-management.md#common-api-components

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface is missing finestIndexedLevel and coarsestIndexedLevel, which are called out in the AC.

And in a related question, do we have a DRIVERS ticket that updates the specifications repo with these additions as well? I'm having trouble finding these.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the 2dsphere options. The options do not exist anywhere in the spec, so we should create a DRIVERS ticket to document them

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed over Slack, the AC needs to be updated to remove that, to match https://jira.mongodb.org/browse/DRIVERS-3569

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sean to remove the finestIndexedLevel, coarsestIndexedLevel

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a/c requires that they work, not that they be added to the model

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread src/operations/indexes.ts Outdated
@seanrmilligan seanrmilligan changed the title feat(NODE-6893): Allow passthrough options on createIndexes feat(NODE-6893): Distinguish between command-level options and index-level options when creating indexes Sep 17, 2026
@PavelSafronov PavelSafronov self-assigned this Sep 17, 2026
@PavelSafronov PavelSafronov added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 17, 2026

@PavelSafronov PavelSafronov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add release notes to the PR description? We'll definitely need those.

Comment thread src/collection.ts

@addaleax addaleax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/collection.ts Outdated
* @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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be deprecated then?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sean to add TODO(NODE-7868) to allowUnknownIndexOptions parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seanrmilligan

Copy link
Copy Markdown
Contributor Author

Can you also add release notes to the PR description? We'll definitely need those.

Done.

Comment thread src/collection.ts
* @param indexOptions - Optional settings for the index
* @param commandOptions - Optional settings for the `createIndexes` command
*/
createIndex(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two optional parameters here need to be made mandatory, so this definition is distinct from the legacy path.

createIndex(
  keys: IndexSpecification,
  indexOptions: IndexOptions,
  commandOptions: CreateIndexOptions
): Promise<string>;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread src/operations/indexes.ts
/**
* Creates the index in the background, yielding whenever possible.
*
* @deprecated Index options will be removed from this type in a future major release. Pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These @deprecated attributes are no longer necessary, since the legacy 2-param method carries the @deprecated attribute now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec provides for:

  • CreateIndexOptions for createIndex, and
  • CreateIndexesOptions for createIndexes

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 used CreateIndexesOptions instead of CreateIndexOptions (mostly harmless)
  • (b) CreateIndexesOptions contained options for the command and options for the index (bad). It was essentially a union of CreateIndexesOptions and IndexOptions from 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.

Comment thread src/operations/indexes.ts Outdated
collectionName: string,
indexes: IndexDescription[],
allowUnknownIndexOptions: boolean,
commandOptions?: CreateIndexesOptions | CreateIndexOptions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your | undefined intuition is correct, but the optionality isn't coming from the createIndex overloads, rather from fromIndexDescriptionArray.

Try this:

  1. drop the first two private constructors of CreateIndexesOperation
  2. make the following change to this constructor: commandOptions: CreateIndexesOptions | CreateIndexOptions | undefined
  3. fromIndexDescriptionArray update commandOptions: CreateIndexesOptions | undefined
  4. fromIndexSpecification overload 1:
    1. indexOptions: IndexOptions | undefined,
    2. commandOptions: CreateIndexOptions | undefined
  5. fromIndexSpecification overload 2
    1. indexOptions: CreateIndexesOptions | IndexOptions | undefined,
    2. commandOptions: CreateIndexOptions | undefined
  6. drop 3rd fromIndexSpecification

Now this will force us to explicitly pass undefined to fromIndexSpecification as necessary, including in tests.
I just tried this locally on top of this PR and it seems to work. Holler if you run into issues.

@johnmtll johnmtll left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment thread src/db.ts
this,
name,
indexSpec,
/*allowUnknownIndexOptions=*/ false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Can we omit /allowUnknownIndexOptions=/ everywhere? Checking the function definition is probably enough to make it clear what's being provided, and this doesn't seem to be a standard practice across the codebase.

);

context('when an unknown index option is provided', function () {
context('and allowUnknownIndexOptions is unset (default)', function () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: explicit false and unset hit the same code path. Can we drop this one or merge them into the same test?

});
});

describe('allowUnknownIndexOptions (createIndexes passthrough)', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
describe('allowUnknownIndexOptions (createIndexes passthrough)', () => {
describe('CreateIndexesOperation.fromIndexDescriptionArray', () => {

With the added suggestion of maybe putting 'allowUnknownIndexOptions (createIndexPassthrough)' as a nested block

randomOptionThatWillNeverBeAdded: true
});

it('drops unknown options when the flag is unset (default behavior)', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
it('drops unknown options when the flag is unset (default behavior)', () => {
it('drops unknown options when the allowUnknownIndexOptions is false (default behavior when unset)', () => {

expect(output.indexes[0]).to.not.have.property('randomOptionThatWillNeverBeAdded');
});

it('drops unknown options when the flag is set to false', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
it('drops unknown options when the flag is set to false', () => {
it('drops unknown options when allowUnknownIndexOptions is set to false', () => {

});

describe('and command options are passed in the third parameter', function () {
it('keeps a comment out of the index description', async function () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
});

describe('when command options are given (three parameter form)', function () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we wrap all of the subsequent 'it's' in a describe block of its own?

Something like: 'and the command options are an empty object'.

]);
});

describe('and command options are passed in the third parameter', function () {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: position of argument is an implementation detail

Suggested change
describe('and command options are passed in the third parameter', function () {
describe('and the command options have values', function () {

Comment on lines +241 to +261
describe('and a command option is left in the index options', function () {
it('forwards a comment to the server, which rejects it', async function () {
const error = await collection
.createIndex({ l: 1 }, { unique: true, comment: 'a comment' }, {})
.catch(error => error);

expect(sentIndexes()[0]).to.have.property('comment', 'a comment');
expect(error).to.be.instanceOf(MongoServerError);
expect(error.message).to.match(/not valid for an index specification/);
});

it('forwards maxTimeMS to the server, which rejects it', async function () {
const error = await collection
.createIndex({ m: 1 }, { unique: true, maxTimeMS: 1000 }, {})
.catch(error => error);

expect(sentIndexes()[0]).to.have.property('maxTimeMS', 1000);
expect(error).to.be.instanceOf(MongoServerError);
expect(error.message).to.match(/not valid for an index specification/);
});
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we can move these into the suggested block of 'and the command options are an empty object'. I get what's happening here, we're testing outcomes reliant on the second parameter, but the third parameter is supplied, so let's group it along its neighbours. :)

});
});

context('#createIndex', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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

  2. 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

Sean Milligan added 4 commits September 30, 2026 13:16
…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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants