fix(shell-api): improve invalid input errors MONGOSH-600 #742
Conversation
There was a problem hiding this comment.
Overall looks good to me! Definitely improves the experience of using shell. I noticed that there is ReplicaSet.add, and global connect and load methods in shell-api that are still using assertArgsDefined, should we also switch them to your new validation method?
If you don't mind, I think I will also open a ticket for the future to enhance this with some automated code generation as we discussed yesterday. I think this would be better for us in the long run if such validation is added to shell-api public interface (semi-)automatically
EDIT: Oh, also wanted to say that I haven't noticed any new tests added and I thought that maybe we can add just a few that check the error message is generated correctly, what do you think?
packages/shell-api/src/database.ts
Outdated
| @@ -234,7 +233,7 @@ export default class Database extends ShellApiClass { | |||
| */ | |||
| @returnsPromise | |||
| async runCommand(cmd: Document): Promise<Document> { | |||
| assertArgsDefined(cmd); | |||
| assertArgsDefinedType([cmd], [true], 'Database.runCommand'); | |||
There was a problem hiding this comment.
Not sure if we want to do something about it in the scope of this ticket, but we don't have any validation if input is a document and this leads to some very weird results when I'm providing non document input:
There was a problem hiding this comment.
Yeah for these cases I did not start to change even more but I tried to just "replace" where I could immediately infer the logic. But in these cases it would probably make sense to check for typeof cmd === 'object'.
|
Huh I didn't see the other usages then... I'll cover those, too. And yep, sounds good to add a unit test for the And yes, absolutely - ticket for the auto-generation would be awesome!! |
| describe('assertArgsDefinedType', () => { | ||
| it('allows to specify an argument must be defined', () => { | ||
| try { | ||
| assertArgsDefinedType([1, undefined], [true, true], 'helper.test'); | ||
| } catch (e) { | ||
| expect(e.message).to.contain('Missing required argument at position 1'); | ||
| expect(e.message).to.contain('helper.test'); | ||
| return; | ||
| } | ||
| expect.fail('Expected error'); | ||
| }); | ||
| it('allows to specify a single argument type', () => { | ||
| [null, 2, {}].forEach(value => { | ||
| try { | ||
| assertArgsDefinedType([1, value], [true, 'string'], 'helper.test'); | ||
| } catch (e) { | ||
| expect(e.message).to.contain('Argument at position 1 must be of type string'); | ||
| expect(e.message).to.contain('helper.test'); | ||
| return; | ||
| } | ||
| expect.fail('Expected error'); | ||
| }); | ||
| expect(() => assertArgsDefinedType([1, 'test'], [true, 'string'])).to.not.throw; | ||
| }); | ||
| it('allows to specify multiple argument types', () => { | ||
| [null, {}].forEach(value => { | ||
| try { | ||
| assertArgsDefinedType([1, value], [true, ['number', 'string']]); | ||
| } catch (e) { | ||
| return expect(e.message).to.contain('Argument at position 1 must be of type number or string'); | ||
| } | ||
| expect.fail('Expected error'); | ||
| }); | ||
| expect(() => assertArgsDefinedType([1, 'test'], [true, ['number', 'string']])).to.not.throw; | ||
| expect(() => assertArgsDefinedType([1, 2], [true, ['number', 'string']])).to.not.throw; | ||
| }); | ||
| it('allows to specify an optional argument with type', () => { | ||
| expect(() => assertArgsDefinedType([1, undefined], [true, [undefined, 'string']])).to.not.throw; | ||
| expect(() => assertArgsDefinedType([1, 'test'], [true, [undefined, 'string']])).to.not.throw; | ||
| try { | ||
| assertArgsDefinedType([1, 2], [true, [undefined, 'string']]); | ||
| } catch (e) { | ||
| return expect(e.message).to.contain('Argument at position 1 must be of type string'); | ||
| } | ||
| expect.fail('Expected error'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Thanks for adding those, so easy to assert how this new method works at a glance now!
rose-m
force-pushed
the
MONGOSH-600-improve-shell-api-invalid-input-errors
branch
from
673b1f7
to
a36b37d
Compare
Adds the affected method name to the InvalidInputErrors of the Shell API.