Skip to content

Commit abc0317

Browse files
rose-mrose-m
authored
fix(shell-api): improve invalid input errors MONGOSH-600 (#742)
1 parent 8b2b4b3 commit abc0317

14 files changed

+238
-166
lines changed

‎packages/shell-api/src/bulk.ts‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
} from '@mongosh/service-provider-core';
1313
import { asPrintable } from './enums';
1414
import { blockedByDriverMetadata } from './error-codes';
15-
import { assertArgsDefined } from './helpers';
15+
import { assertArgsDefinedType } from './helpers';
1616
import { BulkWriteResult } from './result';
1717
import type Collection from './collection';
1818

@@ -49,7 +49,7 @@ export class BulkFindOp extends ShellApiClass {
4949

5050
@returnType('BulkFindOp')
5151
hint(hintDoc: Document): BulkFindOp {
52-
assertArgsDefined(hintDoc);
52+
assertArgsDefinedType([hintDoc], [true], 'BulkFindOp.hint');
5353
this._hint = hintDoc;
5454
return this;
5555
}
@@ -71,7 +71,7 @@ export class BulkFindOp extends ShellApiClass {
7171
@returnType('Bulk')
7272
replaceOne(replacement: Document): Bulk {
7373
this._parentBulk._batchCounts.nUpdateOps++;
74-
assertArgsDefined(replacement);
74+
assertArgsDefinedType([replacement], [true], 'BulkFindOp.replacement');
7575
const op = { ...replacement };
7676
if (this._hint) {
7777
op.hint = this._hint;
@@ -83,7 +83,7 @@ export class BulkFindOp extends ShellApiClass {
8383
@returnType('Bulk')
8484
updateOne(update: Document): Bulk {
8585
this._parentBulk._batchCounts.nUpdateOps++;
86-
assertArgsDefined(update);
86+
assertArgsDefinedType([update], [true], 'BulkFindOp.update');
8787
const op = { ...update };
8888
if (this._hint) {
8989
op.hint = this._hint;
@@ -98,7 +98,7 @@ export class BulkFindOp extends ShellApiClass {
9898
@returnType('Bulk')
9999
update(update: Document): Bulk {
100100
this._parentBulk._batchCounts.nUpdateOps++;
101-
assertArgsDefined(update);
101+
assertArgsDefinedType([update], [true], 'BulkFindOp.update');
102102
const op = { ...update };
103103
if (this._hint) {
104104
op.hint = this._hint;
@@ -112,7 +112,6 @@ export class BulkFindOp extends ShellApiClass {
112112

113113
@returnType('Bulk')
114114
upsert(): BulkFindOp {
115-
assertArgsDefined();
116115
this._serviceProviderBulkFindOp.upsert();
117116
return this;
118117
}
@@ -186,14 +185,14 @@ export default class Bulk extends ShellApiClass {
186185

187186
@returnType('BulkFindOp')
188187
find(query: Document): BulkFindOp {
189-
assertArgsDefined(query);
188+
assertArgsDefinedType([query], [true], 'Bulk.find');
190189
return new BulkFindOp(this._serviceProviderBulkOp.find(query), this);
191190
}
192191

193192
@returnType('Bulk')
194193
insert(document: Document): Bulk {
195194
this._batchCounts.nInsertOps++;
196-
assertArgsDefined(document);
195+
assertArgsDefinedType([document], [true], 'Bulk.insert');
197196
this._serviceProviderBulkOp.insert(document);
198197
return this;
199198
}

‎packages/shell-api/src/collection.ts‎

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ import {
1515
import { ADMIN_DB, asPrintable, namespaceInfo, ServerVersions, Topologies } from './enums';
1616
import {
1717
adaptAggregateOptions,
18-
assertArgsDefined,
19-
assertArgsType,
2018
assertKeysDefined,
2119
dataFormat,
2220
validateExplainableVerbosity,
@@ -29,7 +27,8 @@ import {
2927
processMapReduceOptions,
3028
setHideIndex,
3129
maybeMarkAsExplainOutput,
32-
markAsExplainOutput
30+
markAsExplainOutput,
31+
assertArgsDefinedType
3332
} from './helpers';
3433
import {
3534
AnyBulkWriteOperation,
@@ -287,7 +286,7 @@ export default class Collection extends ShellApiClass {
287286
*/
288287
@returnsPromise
289288
async deleteMany(filter: Document, options: DeleteOptions = {}): Promise<DeleteResult | Document> {
290-
assertArgsDefined(filter);
289+
assertArgsDefinedType([filter], [true], 'Collection.deleteMany');
291290
this._emitCollectionApiCall('deleteMany', { filter, options });
292291

293292
const result = await this._mongo._serviceProvider.deleteMany(
@@ -320,7 +319,7 @@ export default class Collection extends ShellApiClass {
320319
*/
321320
@returnsPromise
322321
async deleteOne(filter: Document, options: DeleteOptions = {}): Promise<DeleteResult | Document> {
323-
assertArgsDefined(filter);
322+
assertArgsDefinedType([filter], [true], 'Collection.deleteOne');
324323
this._emitCollectionApiCall('deleteOne', { filter, options });
325324

326325
const result = await this._mongo._serviceProvider.deleteOne(
@@ -409,7 +408,7 @@ export default class Collection extends ShellApiClass {
409408

410409
@returnsPromise
411410
async findAndModify(options: FindAndModifyMethodShellOptions): Promise<Document> {
412-
assertArgsDefined(options);
411+
assertArgsDefinedType([options], [true], 'Collection.findAndModify');
413412
assertKeysDefined(options, ['query']);
414413
this._emitCollectionApiCall(
415414
'findAndModify',
@@ -463,8 +462,7 @@ export default class Collection extends ShellApiClass {
463462
newName: string,
464463
dropTarget?: boolean
465464
): Promise<Document> {
466-
assertArgsDefined(newName);
467-
assertArgsType([newName], ['string']);
465+
assertArgsDefinedType([newName], ['string'], 'Collection.renameCollection');
468466
this._emitCollectionApiCall('renameCollection', { newName, dropTarget });
469467

470468
try {
@@ -505,7 +503,7 @@ export default class Collection extends ShellApiClass {
505503
@returnType('Document')
506504
@serverVersions(['3.2.0', ServerVersions.latest])
507505
async findOneAndDelete(filter: Document, options: FindAndModifyOptions = {}): Promise<Document> {
508-
assertArgsDefined(filter);
506+
assertArgsDefinedType([filter], [true], 'Collection.findOneAndDelete');
509507
this._emitCollectionApiCall('findOneAndDelete', { filter, options });
510508
const result = await this._mongo._serviceProvider.findOneAndDelete(
511509
this._database._name,
@@ -538,7 +536,7 @@ export default class Collection extends ShellApiClass {
538536
@returnType('Document')
539537
@serverVersions(['3.2.0', ServerVersions.latest])
540538
async findOneAndReplace(filter: Document, replacement: Document, options: FindAndModifyShellOptions = {}): Promise<Document> {
541-
assertArgsDefined(filter);
539+
assertArgsDefinedType([filter], [true], 'Collection.findOneAndReplace');
542540
const findOneAndReplaceOptions = processFindAndModifyOptions({
543541
...this._database._baseOptions,
544542
...options
@@ -576,7 +574,7 @@ export default class Collection extends ShellApiClass {
576574
@returnType('Document')
577575
@serverVersions(['3.2.0', ServerVersions.latest])
578576
async findOneAndUpdate(filter: Document, update: Document | Document[], options: FindAndModifyShellOptions = {}): Promise<Document> {
579-
assertArgsDefined(filter);
577+
assertArgsDefinedType([filter], [true], 'Collection.findOneAndUpdate');
580578
const findOneAndUpdateOptions = processFindAndModifyOptions({
581579
...this._database._baseOptions,
582580
...options
@@ -616,7 +614,7 @@ export default class Collection extends ShellApiClass {
616614
'Collection.insert() is deprecated. Use insertOne, insertMany, or bulkWrite.',
617615
this._mongo._internalState.context.print
618616
);
619-
assertArgsDefined(docs);
617+
assertArgsDefinedType([docs], [true], 'Collection.insert');
620618
// When inserting documents into MongoDB that do not contain the _id field,
621619
// one will be added to each of the documents missing it by the Node driver,
622620
// mutating the document. To prevent this behaviour we pass not the original document,
@@ -654,7 +652,7 @@ export default class Collection extends ShellApiClass {
654652
@returnsPromise
655653
@serverVersions(['3.2.0', ServerVersions.latest])
656654
async insertMany(docs: Document[], options: BulkWriteOptions = {}): Promise<InsertManyResult> {
657-
assertArgsDefined(docs);
655+
assertArgsDefinedType([docs], [true], 'Collection.insertMany');
658656
const docsToInsert: Document[] = Array.isArray(docs) ? docs.map((doc) => ({ ...doc })) : docs;
659657

660658
this._emitCollectionApiCall('insertMany', { options });
@@ -687,7 +685,7 @@ export default class Collection extends ShellApiClass {
687685
@returnsPromise
688686
@serverVersions(['3.2.0', ServerVersions.latest])
689687
async insertOne(doc: Document, options: InsertOneOptions = {}): Promise<InsertOneResult> {
690-
assertArgsDefined(doc);
688+
assertArgsDefinedType([doc], [true], 'Collection.insertOne');
691689

692690
this._emitCollectionApiCall('insertOne', { options });
693691
const result = await this._mongo._serviceProvider.insertOne(
@@ -734,7 +732,7 @@ export default class Collection extends ShellApiClass {
734732
'Collection.remove() is deprecated. Use deleteOne, deleteMany, findOneAndDelete, or bulkWrite.',
735733
this._mongo._internalState.context.print
736734
);
737-
assertArgsDefined(query);
735+
assertArgsDefinedType([query], [true], 'Collection.remove');
738736
const removeOptions = processRemoveOptions(options);
739737
const method = removeOptions.justOne ? 'deleteOne' : 'deleteMany';
740738
delete removeOptions.justOne;
@@ -781,7 +779,7 @@ export default class Collection extends ShellApiClass {
781779
@returnsPromise
782780
@serverVersions(['3.2.0', ServerVersions.latest])
783781
async replaceOne(filter: Document, replacement: Document, options: ReplaceOptions = {}): Promise<UpdateResult> {
784-
assertArgsDefined(filter);
782+
assertArgsDefinedType([filter], [true], 'Collection.replaceOne');
785783

786784
this._emitCollectionApiCall('replaceOne', { filter, options });
787785
const result = await this._mongo._serviceProvider.replaceOne(
@@ -808,7 +806,7 @@ export default class Collection extends ShellApiClass {
808806
'Collection.update() is deprecated. Use updateOne, updateMany, or bulkWrite.',
809807
this._mongo._internalState.context.print
810808
);
811-
assertArgsDefined(update);
809+
assertArgsDefinedType([filter, update], [true, true], 'Collection.update');
812810
this._emitCollectionApiCall('update', { filter, options });
813811
let result;
814812

@@ -857,7 +855,7 @@ export default class Collection extends ShellApiClass {
857855
@returnsPromise
858856
@serverVersions(['3.2.0', ServerVersions.latest])
859857
async updateMany(filter: Document, update: Document, options: UpdateOptions = {}): Promise<UpdateResult | Document> {
860-
assertArgsDefined(filter);
858+
assertArgsDefinedType([filter], [true], 'Collection.updateMany');
861859
this._emitCollectionApiCall('updateMany', { filter, options });
862860
const result = await this._mongo._serviceProvider.updateMany(
863861
this._database._name,
@@ -899,7 +897,7 @@ export default class Collection extends ShellApiClass {
899897
update: Document,
900898
options: UpdateOptions = {}
901899
): Promise<UpdateResult | Document> {
902-
assertArgsDefined(filter);
900+
assertArgsDefinedType([filter], [true], 'Collection.updateOne');
903901
this._emitCollectionApiCall('updateOne', { filter, options });
904902
const result = await this._mongo._serviceProvider.updateOne(
905903
this._database._name,
@@ -956,7 +954,7 @@ export default class Collection extends ShellApiClass {
956954
keyPatterns: Document[],
957955
options: CreateIndexesOptions = {}
958956
): Promise<string[]> {
959-
assertArgsDefined(keyPatterns);
957+
assertArgsDefinedType([keyPatterns], [true], 'Collection.createIndexes');
960958
if (typeof options !== 'object' || Array.isArray(options)) {
961959
throw new MongoshInvalidInputError(
962960
'The "options" argument must be an object.',
@@ -990,7 +988,7 @@ export default class Collection extends ShellApiClass {
990988
keys: Document,
991989
options: CreateIndexesOptions = {}
992990
): Promise<string> {
993-
assertArgsDefined(keys);
991+
assertArgsDefinedType([keys], [true], 'Collection.createIndex');
994992
if (typeof options !== 'object' || Array.isArray(options)) {
995993
throw new MongoshInvalidInputError(
996994
'The "options" argument must be an object.',
@@ -1025,7 +1023,7 @@ export default class Collection extends ShellApiClass {
10251023
keys: Document,
10261024
options: CreateIndexesOptions = {}
10271025
): Promise<Document> {
1028-
assertArgsDefined(keys);
1026+
assertArgsDefinedType([keys], [true], 'Collection.ensureIndex');
10291027
if (typeof options !== 'object' || Array.isArray(options)) {
10301028
throw new MongoshInvalidInputError(
10311029
'The "options" argument must be an object.',
@@ -1145,7 +1143,7 @@ export default class Collection extends ShellApiClass {
11451143
*/
11461144
@returnsPromise
11471145
async dropIndex(index: string|Document): Promise<Document> {
1148-
assertArgsDefined(index);
1146+
assertArgsDefinedType([index], [true], 'Collection.dropIndex');
11491147
this._emitCollectionApiCall('dropIndex', { index });
11501148
if (index === '*') {
11511149
throw new MongoshInvalidInputError(
@@ -1315,7 +1313,7 @@ export default class Collection extends ShellApiClass {
13151313

13161314
@returnsPromise
13171315
async runCommand(commandName: string, options?: RunCommandOptions): Promise<Document> {
1318-
assertArgsType([commandName], ['string']);
1316+
assertArgsDefinedType([commandName], ['string'], 'Collection.runCommand');
13191317

13201318
if (options && commandName in options) {
13211319
throw new MongoshInvalidInputError(
@@ -1475,7 +1473,7 @@ export default class Collection extends ShellApiClass {
14751473

14761474
@returnsPromise
14771475
async mapReduce(map: Function | string, reduce: Function | string, optionsOrOutString: MapReduceShellOptions): Promise<Document> {
1478-
assertArgsDefined(map, reduce, optionsOrOutString);
1476+
assertArgsDefinedType([map, reduce, optionsOrOutString], [true, true, true], 'Collection.mapReduce');
14791477
this._emitCollectionApiCall('mapReduce', { map, reduce, out: optionsOrOutString });
14801478

14811479
let cmd = {

‎packages/shell-api/src/database.spec.ts‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,17 @@ describe('Database', () => {
182182
);
183183
});
184184

185+
it('transforms a string argument into the command document', async() => {
186+
await database.runCommand('isMaster');
187+
188+
expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith(
189+
database._name,
190+
{
191+
isMaster: 1
192+
}
193+
);
194+
});
195+
185196
it('returns whatever serviceProvider.runCommand returns', async() => {
186197
const expectedResult = { ok: 1 };
187198
serviceProvider.runCommandWithCheck.resolves(expectedResult);
@@ -210,6 +221,17 @@ describe('Database', () => {
210221
);
211222
});
212223

224+
it('transforms a string argument into the command document', async() => {
225+
await database.adminCommand('command');
226+
227+
expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith(
228+
'admin',
229+
{
230+
command: 1
231+
}
232+
);
233+
});
234+
213235
it('returns whatever serviceProvider.runCommand returns', async() => {
214236
const expectedResult = { ok: 1 };
215237
serviceProvider.runCommandWithCheck.resolves(expectedResult);
@@ -2392,11 +2414,26 @@ describe('Database', () => {
23922414
getCollectionInfos: { m: 'listCollections' },
23932415
aggregate: { m: 'aggregateDb' },
23942416
dropDatabase: { m: 'dropDatabase', i: 1 },
2395-
createCollection: { m: 'createCollection' },
2396-
createView: { m: 'createCollection' },
2417+
createCollection: { m: 'createCollection', a: ['coll'] },
2418+
createView: { m: 'createCollection', a: ['coll', 'source', []] },
2419+
changeUserPassword: { a: ['username', 'pass'] },
23972420
createUser: { a: [{ user: 'a', pwd: 'p', roles: [] }] },
2421+
updateUser: { a: ['username', { roles: [] }] },
23982422
createRole: { a: [{ role: 'a', privileges: [], roles: [] }] },
2399-
setLogLevel: { a: ['a'] }
2423+
updateRole: { a: ['role', {}] },
2424+
getUser: { a: ['username'] },
2425+
getRole: { a: ['rolename'] },
2426+
dropUser: { a: ['username'] },
2427+
dropRole: { a: ['role'] },
2428+
grantRolesToUser: { a: ['username', []] },
2429+
revokeRolesFromUser: { a: ['username', []] },
2430+
grantRolesToRole: { a: ['rolename', []] },
2431+
revokeRolesFromRole: { a: ['rolename', []] },
2432+
grantPrivilegesToRole: { a: ['rolename', []] },
2433+
revokePrivilegesFromRole: { a: ['rolename', []] },
2434+
setLogLevel: { a: [1] },
2435+
setProfilingLevel: { a: [1] },
2436+
killOp: { a: [1] }
24002437
};
24012438
const ignore = [
24022439
'auth',

0 commit comments

Comments
 (0)