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.

Sign up for GitHub

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

fix(NODE-6029): update types for collection listing indexes #4072

Merged
merged 7 commits into from
Apr 11, 2024
Merged

fix(NODE-6029): update types for collection listing indexes #4072

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bada1ec
fix(NODE-6029): update types for collection listing collection indexes
prenaissance Apr 8, 2024
e8f4ab1
fix(NODE-6029): fix review comments
prenaissance Apr 9, 2024
1ba6e8d
test(NODE-6029): add type tests for Db.prototype.indexInformation
prenaissance Apr 9, 2024
d859a6a
test(NODE-6029): add comment
prenaissance Apr 9, 2024
1f18df2
fix(NODE-6029): remove unused type
prenaissance Apr 9, 2024
cce822e
style(NODE-6029): explicitly declare return types
prenaissance Apr 9, 2024
6f8450d
fix(NODE-6029): fix non-literal index options return type
prenaissance Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions src/collection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ import type {
CreateIndexesOptions,
DropIndexesOptions,
IndexDescription,
IndexDirection,
IndexDescriptionCompact,
IndexDescriptionInfo,
IndexInformationOptions,
IndexSpecification,
ListIndexesOptions
Expand Down Expand Up @@ -121,6 +122,13 @@ export interface CollectionPrivate {
writeConcern?: WriteConcern;
}

/** @public */
export type IndexesInformation<TFull extends boolean = boolean> = TFull extends true
? IndexDescriptionInfo[]
: TFull extends false
? IndexDescriptionCompact
: IndexDescriptionInfo[] | IndexDescriptionCompact;

baileympearson marked this conversation as resolved.
Show resolved Hide resolved
/**
* The **Collection** class is an internal class that embodies a MongoDB collection
* allowing for insert/find/update/delete and other command operation on that MongoDB collection.
Expand Down Expand Up @@ -693,8 +701,13 @@ export class Collection<TSchema extends Document = Document> {
*
* @param options - Optional settings for the command
*/
async indexInformation(options?: IndexInformationOptions): Promise<Document> {
return await this.indexes({ ...options, full: options?.full ?? false });
async indexInformation<TFull extends boolean = false>(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's preferrable to avoid computed types as the return types from our APIs, since they don't provided users with information about the return type of a function until a user 1) reads the source of the computed type or 2) writes code using the API and checks the computed return type. Could we accomplish the same here using overloads and removing all TFull generics? Something like:

  indexInformation(
    options: IndexInformationOptions & { full: true }
  ): Promise<IndexDescriptionInfo[]>;
  indexInformation(
    options: IndexInformationOptions & { full: false }
  ): Promise<IndexDescriptionCompact>;
  indexInformation(
    options: IndexInformationOptions & { full: boolean }
  ): Promise<IndexDescriptionCompact | IndexDescriptionInfo[]>;

This has the additional benefits of

  • better documentation, because our tooling will generate a doc entry for each overload
  • clearly defined overloads with existing IDE tooling
  • each overload can be documented originally (if desired).

for example - here's what VSCode could hypothetically display if we used overloads:

Screenshot 2024-04-09 at 12 23 01 PM Screenshot 2024-04-09 at 12 22 55 PM

options?: IndexInformationOptions<TFull>
): Promise<IndexesInformation<TFull>> {
return (await this.indexes({
...options,
full: options?.full ?? false
})) as IndexesInformation<TFull>;
}

/**
Expand Down Expand Up @@ -798,20 +811,20 @@ export class Collection<TSchema extends Document = Document> {
*
* @param options - Optional settings for the command
*/
async indexes(options?: IndexInformationOptions): Promise<Document[]> {
const indexes = await this.listIndexes(options).toArray();
async indexes<TFull extends boolean = true>(
options?: IndexInformationOptions<TFull>
): Promise<IndexesInformation<TFull>> {
const indexes: IndexDescriptionInfo[] = await this.listIndexes(options).toArray();
const full = options?.full ?? true;
if (full) {
return indexes;
return indexes as IndexesInformation<TFull>;
}

const object: Record<
string,
Array<[name: string, direction: IndexDirection]>
> = Object.fromEntries(indexes.map(({ name, key }) => [name, Object.entries(key)]));
const object: IndexDescriptionCompact = Object.fromEntries(
indexes.map(({ name, key }) => [name, Object.entries(key)])
);

// @ts-expect-error TODO(NODE-6029): fix return type of `indexes()` and `indexInformation()`
return object;
return object as IndexesInformation<TFull>;
}

/**
Expand Down
5 changes: 4 additions & 1 deletion src/db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,10 @@ export class Db {
* @param name - The name of the collection.
* @param options - Optional settings for the command
*/
async indexInformation(name: string, options?: IndexInformationOptions): Promise<Document> {
async indexInformation<TFull extends boolean = false>(
name: string,
options?: IndexInformationOptions<TFull>
) {
return await this.collection(name).indexInformation(resolveOptions(this, options));
}

Expand Down
9 changes: 8 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,12 @@ export type { StreamDescription, StreamDescriptionOptions } from './cmap/stream_
export type { CompressorName } from './cmap/wire_protocol/compression';
export type { JSTypeOf, OnDemandDocument } from './cmap/wire_protocol/on_demand/document';
export type { MongoDBResponse, MongoDBResponseConstructor } from './cmap/wire_protocol/responses';
export type { CollectionOptions, CollectionPrivate, ModifyResult } from './collection';
export type {
CollectionOptions,
CollectionPrivate,
IndexesInformation,
ModifyResult
} from './collection';
export type {
COMMAND_FAILED,
COMMAND_STARTED,
Expand Down Expand Up @@ -471,6 +476,8 @@ export type {
CreateIndexesOptions,
DropIndexesOptions,
IndexDescription,
IndexDescriptionCompact,
IndexDescriptionInfo,
IndexDirection,
IndexSpecification,
ListIndexesOptions
Expand Down
16 changes: 14 additions & 2 deletions src/operations/indexes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export type IndexSpecification = OneOrMore<
>;

/** @public */
export interface IndexInformationOptions extends ListIndexesOptions {
export interface IndexInformationOptions<TFull extends boolean> extends ListIndexesOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider this a breaking change, since existing users' code that makes use of IndexInformationOptions would break because they wouldn't be providing a generic argument. We could fix this by providing a default, but I think overloads are preferable (see my comment on Collection.indexInformation()).

/**
* When `true`, an array of index descriptions is returned.
* When `false`, the driver returns an object that with keys corresponding to index names with values
Expand All @@ -91,7 +91,7 @@ export interface IndexInformationOptions extends ListIndexesOptions {
* }
* ```
*/
full?: boolean;
full?: TFull;
}

/** @public */
Expand Down Expand Up @@ -214,6 +214,18 @@ function resolveIndexDescription(
);
}

/**
* @public
* The index information returned by the listIndexes command. https://www.mongodb.com/docs/manual/reference/command/listIndexes/#mongodb-dbcommand-dbcmd.listIndexes
*/
export type IndexDescriptionInfo = Omit<IndexDescription, 'key' | 'version'> & {
key: { [key: string]: IndexDirection };
v?: IndexDescription['version'];
};

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally try to keep our type definitions for server responses flexible so that if the server modifies a response in a future server version the driver is still compatible with it. Could we & Document here? I think that will provide type safety on known keys but will allow unknown keys without a Typescript error.

/** @public */
export type IndexDescriptionCompact = Record<string, [name: string, direction: IndexDirection][]>;

/**
* @internal
*
Expand Down
65 changes: 59 additions & 6 deletions test/types/indexes_test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,69 @@
import { expectType } from 'tsd';
import { expectAssignable, expectType } from 'tsd';

import { type Document, MongoClient } from '../../src';
import { MongoClient } from '../../src';
import { type IndexDescriptionCompact, type IndexDescriptionInfo } from '../mongodb';

const client = new MongoClient('');
const db = client.db('test');
const collection = db.collection('test.find');

// Promise variant testing
expectType<Promise<Document[]>>(collection.indexes());
expectType<Promise<Document[]>>(collection.indexes({}));
const exampleFullIndexes: IndexDescriptionInfo[] = [
{ v: 2, key: { _id: 1 }, name: '_id_' },
{ v: 2, key: { listingName: 'hashed' }, name: 'listingName_hashed' },
{
v: 2,
key: { 'auctionDetails.listingId': 1 },
name: 'auctionDetails_listingId_1',
unique: true
}
];
const exampleCompactIndexes: IndexDescriptionCompact = {
_id_: [['_id', 1]],
listingName_hashed: [['listingName', 'hashed']],
auctionDetails_listingId_1: [['auctionDetails.listingId', 1]]
};

const ambiguousFullness = Math.random() > 0.5;

// test Collection.prototype.indexes

const defaultIndexes = await collection.indexes();
const emptyOptionsIndexes = await collection.indexes({});
const fullIndexes = await collection.indexes({ full: true });
const notFullIndexes = await collection.indexes({ full: false });
const ambiguousIndexes = await collection.indexes({ full: ambiguousFullness });

expectAssignable<typeof fullIndexes>(exampleFullIndexes);
expectAssignable<typeof ambiguousIndexes>(exampleFullIndexes);
expectAssignable<typeof ambiguousIndexes>(exampleCompactIndexes);
expectAssignable<typeof notFullIndexes>(exampleCompactIndexes);

expectType<IndexDescriptionInfo[]>(defaultIndexes);
expectType<IndexDescriptionInfo[]>(emptyOptionsIndexes);
expectType<IndexDescriptionInfo[]>(fullIndexes);
expectType<IndexDescriptionCompact>(notFullIndexes);
expectType<IndexDescriptionInfo[] | IndexDescriptionCompact>(ambiguousIndexes);

// test Collection.prototype.indexInformation

const defaultIndexInfo = await collection.indexInformation();
const emptyOptionsIndexInfo = await collection.indexInformation({});
const fullIndexInfo = await collection.indexInformation({ full: true });
const notFullIndexInfo = await collection.indexInformation({ full: false });
const ambiguousIndexInfo = await collection.indexInformation({ full: ambiguousFullness });

expectAssignable<typeof fullIndexInfo>(exampleFullIndexes);
expectAssignable<typeof ambiguousIndexInfo>(exampleFullIndexes);
expectAssignable<typeof ambiguousIndexInfo>(exampleCompactIndexes);
expectAssignable<typeof notFullIndexInfo>(exampleCompactIndexes);

expectType<IndexDescriptionCompact>(defaultIndexInfo);
expectType<IndexDescriptionCompact>(emptyOptionsIndexInfo);
expectType<IndexDescriptionInfo[]>(fullIndexInfo);
expectType<IndexDescriptionCompact>(notFullIndexInfo);
expectType<IndexDescriptionInfo[] | IndexDescriptionCompact>(ambiguousIndexInfo);

// Explicit check for iterable result
for (const index of await collection.indexes()) {
expectType<Document>(index);
expectType<IndexDescriptionInfo>(index);
}