Skip to content

Commit

Permalink
refactor!: CreateIndexOp returns string, CreateIndexesOp returns array (
Browse files Browse the repository at this point in the history
#2666)

`CreateIndexesOperation` was returning the name of the created index
instead of the result from the server because of the logic ensuring
`CreateIndexOperation` has that behavior.

This PR makes `CreateIndexesOperation` return an *array of index names*
(breaking change), while `CreateIndexOperation` continues to return a
single index name string. This makes the behavior consistent with other
official drivers.

Users in need of the raw result from the server should use `db.command`.

NODE-2602
  • Loading branch information
emadum authored Dec 14, 2020
1 parent 0f2eb49 commit e12c485
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 37 deletions.
7 changes: 5 additions & 2 deletions src/bulk/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1221,7 +1221,10 @@ export abstract class BulkOperationBase {
}

/** An internal helper method. Do not invoke directly. Will be going away in the future */
execute(options?: BulkWriteOptions, callback?: Callback<BulkWriteResult>): Promise<void> | void {
execute(
options?: BulkWriteOptions,
callback?: Callback<BulkWriteResult>
): Promise<BulkWriteResult> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options ?? {};

Expand Down Expand Up @@ -1303,7 +1306,7 @@ Object.defineProperty(BulkOperationBase.prototype, 'length', {
function handleEarlyError(
err?: AnyError,
callback?: Callback<BulkWriteResult>
): Promise<void> | void {
): Promise<BulkWriteResult> | void {
const Promise = PromiseProvider.get();
if (typeof callback === 'function') {
callback(err);
Expand Down
28 changes: 14 additions & 14 deletions src/collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -744,19 +744,19 @@ export class Collection {
* await collection.createIndex(['j', ['k', -1], { l: '2d' }])
* ```
*/
createIndex(indexSpec: IndexSpecification): Promise<Document>;
createIndex(indexSpec: IndexSpecification, callback: Callback<Document>): void;
createIndex(indexSpec: IndexSpecification, options: CreateIndexesOptions): Promise<Document>;
createIndex(indexSpec: IndexSpecification): Promise<string>;
createIndex(indexSpec: IndexSpecification, callback: Callback<string>): void;
createIndex(indexSpec: IndexSpecification, options: CreateIndexesOptions): Promise<string>;
createIndex(
indexSpec: IndexSpecification,
options: CreateIndexesOptions,
callback: Callback<Document>
callback: Callback<string>
): void;
createIndex(
indexSpec: IndexSpecification,
options?: CreateIndexesOptions | Callback<Document>,
callback?: Callback<Document>
): Promise<Document> | void {
options?: CreateIndexesOptions | Callback<string>,
callback?: Callback<string>
): Promise<string> | void {
if (typeof options === 'function') (callback = options), (options = {});

return executeOperation(
Expand Down Expand Up @@ -798,19 +798,19 @@ export class Collection {
* ]);
* ```
*/
createIndexes(indexSpecs: IndexDescription[]): Promise<Document>;
createIndexes(indexSpecs: IndexDescription[], callback: Callback<Document>): void;
createIndexes(indexSpecs: IndexDescription[], options: CreateIndexesOptions): Promise<Document>;
createIndexes(indexSpecs: IndexDescription[]): Promise<string[]>;
createIndexes(indexSpecs: IndexDescription[], callback: Callback<string[]>): void;
createIndexes(indexSpecs: IndexDescription[], options: CreateIndexesOptions): Promise<string[]>;
createIndexes(
indexSpecs: IndexDescription[],
options: CreateIndexesOptions,
callback: Callback<Document>
callback: Callback<string[]>
): void;
createIndexes(
indexSpecs: IndexDescription[],
options?: CreateIndexesOptions | Callback<Document>,
callback?: Callback<Document>
): Promise<Document> | void {
options?: CreateIndexesOptions | Callback<string[]>,
callback?: Callback<string[]>
): Promise<string[]> | void {
if (typeof options === 'function') (callback = options), (options = {});
options = options ? Object.assign({}, options) : {};
if (typeof options.maxTimeMS !== 'number') delete options.maxTimeMS;
Expand Down
14 changes: 7 additions & 7 deletions src/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -579,25 +579,25 @@ export class Db {
* @param options - Optional settings for the command
* @param callback - An optional callback, a Promise will be returned if none is provided
*/
createIndex(name: string, indexSpec: IndexSpecification): Promise<Document>;
createIndex(name: string, indexSpec: IndexSpecification, callback?: Callback<Document>): void;
createIndex(name: string, indexSpec: IndexSpecification): Promise<string>;
createIndex(name: string, indexSpec: IndexSpecification, callback?: Callback<string>): void;
createIndex(
name: string,
indexSpec: IndexSpecification,
options: CreateIndexesOptions
): Promise<Document>;
): Promise<string>;
createIndex(
name: string,
indexSpec: IndexSpecification,
options: CreateIndexesOptions,
callback: Callback<Document>
callback: Callback<string>
): void;
createIndex(
name: string,
indexSpec: IndexSpecification,
options?: CreateIndexesOptions | Callback<Document>,
callback?: Callback<Document>
): Promise<Document> | void {
options?: CreateIndexesOptions | Callback<string>,
callback?: Callback<string>
): Promise<string> | void {
if (typeof options === 'function') (callback = options), (options = {});

return executeOperation(
Expand Down
23 changes: 14 additions & 9 deletions src/operations/indexes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,11 @@ export class IndexesOperation extends AbstractOperation<Document> {
}

/** @internal */
export class CreateIndexesOperation extends CommandOperation<Document> {
export class CreateIndexesOperation<
T extends string | string[] = string[]
> extends CommandOperation<T> {
options: CreateIndexesOptions;
collectionName: string;
onlyReturnNameOfCreatedIndex?: boolean;
indexes: IndexDescription[];

constructor(
Expand All @@ -172,12 +173,9 @@ export class CreateIndexesOperation extends CommandOperation<Document> {
this.collectionName = collectionName;

this.indexes = indexes;
if (indexes.length === 1) {
this.onlyReturnNameOfCreatedIndex = true;
}
}

execute(server: Server, session: ClientSession, callback: Callback<Document>): void {
execute(server: Server, session: ClientSession, callback: Callback<T>): void {
const options = this.options;
const indexes = this.indexes;

Expand Down Expand Up @@ -223,19 +221,20 @@ export class CreateIndexesOperation extends CommandOperation<Document> {
// collation is set on each index, it should not be defined at the root
this.options.collation = undefined;

super.executeCommand(server, session, cmd, (err, result) => {
super.executeCommand(server, session, cmd, err => {
if (err) {
callback(err);
return;
}

callback(undefined, this.onlyReturnNameOfCreatedIndex ? indexes[0].name : result);
const indexNames = indexes.map(index => index.name || '');
callback(undefined, indexNames as T);
});
}
}

/** @internal */
export class CreateIndexOperation extends CreateIndexesOperation {
export class CreateIndexOperation extends CreateIndexesOperation<string> {
constructor(
parent: OperationParent,
collectionName: string,
Expand All @@ -250,6 +249,12 @@ export class CreateIndexOperation extends CreateIndexesOperation {

super(parent, collectionName, [makeIndexSpec(indexSpec, options)], options);
}
execute(server: Server, session: ClientSession, callback: Callback<string>): void {
super.execute(server, session, (err, indexNames) => {
if (err || !indexNames) return callback(err);
return callback(undefined, indexNames[0]);
});
}
}

/** @internal */
Expand Down
40 changes: 35 additions & 5 deletions test/functional/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -831,10 +831,8 @@ describe('Indexes', function () {
}
});

it('should correctly execute createIndexes', {
metadata: {
requires: { mongodb: '>=2.6.0', topology: ['single', 'ssl', 'heap', 'wiredtiger'] }
},
it('should correctly execute createIndexes with multiple indexes', {
metadata: { requires: { mongodb: '>=2.6.0', topology: ['single'] } },

test: function (done) {
var configuration = this.configuration;
Expand All @@ -845,7 +843,7 @@ describe('Indexes', function () {
[{ key: { a: 1 } }, { key: { b: 1 }, name: 'hello1' }],
function (err, r) {
expect(err).to.not.exist;
test.equal(3, r.numIndexesAfter);
expect(r).to.deep.equal(['a_1', 'hello1']);

db.collection('createIndexes')
.listIndexes()
Expand All @@ -868,6 +866,38 @@ describe('Indexes', function () {
}
});

it('should correctly execute createIndexes with one index', {
metadata: { requires: { mongodb: '>=2.6.0', topology: ['single'] } },

test: function (done) {
var configuration = this.configuration;
var client = configuration.newClient(configuration.writeConcernMax(), { maxPoolSize: 1 });
client.connect(function (err, client) {
var db = client.db(configuration.db);
db.collection('createIndexes').createIndexes([{ key: { a: 1 } }], function (err, r) {
expect(err).to.not.exist;
expect(r).to.deep.equal(['a_1']);

db.collection('createIndexes')
.listIndexes()
.toArray(function (err, docs) {
expect(err).to.not.exist;
var keys = {};

for (var i = 0; i < docs.length; i++) {
keys[docs[i].name] = true;
}

test.ok(keys['a_1']);
test.ok(keys['hello1']);

client.close(done);
});
});
});
}
});

it('shouldCorrectlyCreateTextIndex', {
metadata: {
requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] }
Expand Down

0 comments on commit e12c485

Please sign in to comment.