-
Notifications
You must be signed in to change notification settings - Fork 385
Adds prompting on invalid option sets in Zod-enabled commands #7061
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -297,6 +297,66 @@ class MockCommandWithSchemaAndBoolRequiredOption extends AnonymousCommand { | |
| } | ||
| } | ||
|
|
||
| const refinedSchemaOptions = z.strictObject({ | ||
| ...globalOptionsZod.shape, | ||
| authType: z.string().optional(), | ||
| userName: z.string().optional(), | ||
| password: z.string().optional(), | ||
| certificateFile: z.string().optional(), | ||
| certificateBase64Encoded: z.string().optional() | ||
| }); | ||
|
|
||
| class MockCommandWithRefinedSchema extends AnonymousCommand { | ||
| public get name(): string { | ||
| return 'cli mock schema refined'; | ||
| } | ||
| public get description(): string { | ||
| return 'Mock command with refined schema'; | ||
| } | ||
| public get schema(): z.ZodType { | ||
| return refinedSchemaOptions; | ||
| } | ||
| public getRefinedSchema(schema: typeof refinedSchemaOptions): z.ZodObject<any> | undefined { | ||
| return schema | ||
| .refine(options => options.authType !== 'password' || options.userName, { | ||
| error: 'Username is required when using password authentication.', | ||
| path: ['userName'], | ||
| params: { | ||
| customCode: 'required' | ||
| } | ||
| }) | ||
| .refine(options => options.authType !== 'password' || options.password, { | ||
| error: 'Password is required when using password authentication.', | ||
| path: ['password'], | ||
| params: { | ||
| customCode: 'required' | ||
| } | ||
| }) | ||
| .refine(options => options.authType !== 'certificate' || !(options.certificateFile && options.certificateBase64Encoded), { | ||
| error: 'Specify either certificateFile or certificateBase64Encoded, but not both.', | ||
| path: ['certificateBase64Encoded'], | ||
| params: { | ||
| customCode: 'optionSet', | ||
| options: ['certificateFile', 'certificateBase64Encoded'] | ||
| } | ||
| }) | ||
| .refine(options => options.authType !== 'certificate' || options.certificateFile || options.certificateBase64Encoded, { | ||
| error: 'Specify either certificateFile or certificateBase64Encoded.', | ||
| path: ['certificateFile'], | ||
| params: { | ||
| customCode: 'optionSet', | ||
| options: ['certificateFile', 'certificateBase64Encoded'] | ||
| } | ||
| }) | ||
| .refine(options => options.authType !== 'invalid' || false, { | ||
| error: 'Invalid authentication type.', | ||
| path: ['authType'] | ||
| }); | ||
| } | ||
| public async commandAction(): Promise<void> { | ||
| } | ||
| } | ||
|
|
||
| describe('cli', () => { | ||
| let rootFolder: string; | ||
| let cliLogStub: sinon.SinonStub; | ||
|
|
@@ -313,6 +373,7 @@ describe('cli', () => { | |
| let mockCommandWithSchema: Command; | ||
| let mockCommandWithSchemaAndRequiredOptions: Command; | ||
| let mockCommandWithSchemaAndBoolRequiredOption: Command; | ||
| let mockCommandWithRefinedSchema: Command; | ||
| let log: string[] = []; | ||
| let mockCommandWithBooleanRewrite: Command; | ||
|
|
||
|
|
@@ -337,6 +398,7 @@ describe('cli', () => { | |
| mockCommandWithSchema = new MockCommandWithSchema(); | ||
| mockCommandWithSchemaAndRequiredOptions = new MockCommandWithSchemaAndRequiredOptions(); | ||
| mockCommandWithSchemaAndBoolRequiredOption = new MockCommandWithSchemaAndBoolRequiredOption(); | ||
| mockCommandWithRefinedSchema = new MockCommandWithRefinedSchema(); | ||
| mockCommandWithOptionSets = new MockCommandWithOptionSets(); | ||
| mockCommandActionSpy = sinon.spy(mockCommand, 'action'); | ||
|
|
||
|
|
@@ -359,6 +421,7 @@ describe('cli', () => { | |
| cli.getCommandInfo(mockCommandWithSchema, 'cli-schema-mock.js', 'help.mdx'), | ||
| cli.getCommandInfo(mockCommandWithSchemaAndRequiredOptions, 'cli-schema-mock.js', 'help.mdx'), | ||
| cli.getCommandInfo(mockCommandWithSchemaAndBoolRequiredOption, 'cli-schema-mock.js', 'help.mdx'), | ||
| cli.getCommandInfo(mockCommandWithRefinedSchema, 'cli-schema-refined-mock.js', 'help.mdx'), | ||
| cli.getCommandInfo(cliCompletionUpdateCommand, 'cli/commands/completion/completion-clink-update.js', 'cli/completion/completion-clink-update.mdx'), | ||
| cli.getCommandInfo(mockCommandWithBooleanRewrite, 'cli-boolean-rewrite-mock.js', 'help.mdx') | ||
| ]; | ||
|
|
@@ -395,6 +458,7 @@ describe('cli', () => { | |
| cli.loadAllCommandsInfo, | ||
| cli.getConfig().get, | ||
| cli.loadCommandFromFile, | ||
| cli.promptForValue, | ||
| browserUtil.open | ||
| ]); | ||
| }); | ||
|
|
@@ -1154,6 +1218,94 @@ describe('cli', () => { | |
| }); | ||
| }); | ||
|
|
||
| it(`prompts for missing required options from refined schema when prompting enabled`, async () => { | ||
| cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock schema refined'); | ||
| const promptInputStub: sinon.SinonStub = sinon.stub(prompt, 'forInput') | ||
| .onFirstCall().resolves('[email protected]') | ||
| .onSecondCall().resolves('pass@word1'); | ||
| sinon.stub(cli, 'getSettingWithDefaultValue').callsFake((settingName, defaultValue) => { | ||
| if (settingName === settingsNames.prompt) { | ||
| return true; | ||
| } | ||
| return defaultValue; | ||
| }); | ||
| const executeCommandSpy = sinon.spy(cli, 'executeCommand'); | ||
|
|
||
| await cli.execute(['cli', 'mock', 'schema', 'refined', '--authType', 'password']); | ||
| assert(cliErrorStub.calledWith('🌶️ Provide values for the following parameters:')); | ||
| assert.strictEqual(promptInputStub.callCount, 2); | ||
| assert(executeCommandSpy.called); | ||
| }); | ||
|
|
||
| it(`prompts for option set selection from refined schema when prompting enabled`, async () => { | ||
| cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock schema refined'); | ||
| const promptSelectionStub: sinon.SinonStub = sinon.stub(prompt, 'forSelection').resolves('certificateFile'); | ||
| const promptInputStub: sinon.SinonStub = sinon.stub(prompt, 'forInput').resolves('/path/to/cert.pem'); | ||
| sinon.stub(cli, 'getSettingWithDefaultValue').callsFake((settingName, defaultValue) => { | ||
| if (settingName === settingsNames.prompt) { | ||
| return true; | ||
| } | ||
| return defaultValue; | ||
| }); | ||
| const executeCommandSpy = sinon.spy(cli, 'executeCommand'); | ||
|
|
||
| await cli.execute(['cli', 'mock', 'schema', 'refined', '--authType', 'certificate']); | ||
| assert(cliErrorStub.calledWith('🌶️ Please specify one of the following options:')); | ||
| assert(promptSelectionStub.calledOnce); | ||
| assert.deepStrictEqual(promptSelectionStub.firstCall.args[0].choices, [ | ||
| { name: 'certificateFile', value: 'certificateFile' }, | ||
| { name: 'certificateBase64Encoded', value: 'certificateBase64Encoded' } | ||
| ]); | ||
| assert(promptInputStub.calledOnce); | ||
| assert(executeCommandSpy.called); | ||
| }); | ||
|
|
||
| it(`exits with error for non-required/non-optionSet errors in refined schema when prompting enabled`, (done) => { | ||
| cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock schema refined'); | ||
| sinon.stub(cli, 'getSettingWithDefaultValue').callsFake((settingName, defaultValue) => { | ||
| if (settingName === settingsNames.prompt) { | ||
| return true; | ||
| } | ||
| return defaultValue; | ||
| }); | ||
| const executeCommandSpy = sinon.spy(cli, 'executeCommand'); | ||
|
|
||
| cli | ||
| .execute(['cli', 'mock', 'schema', 'refined', '--authType', 'invalid']) | ||
| .then(_ => done('Promise fulfilled while error expected'), _ => { | ||
| try { | ||
| assert(executeCommandSpy.notCalled); | ||
| done(); | ||
| } | ||
| catch (e) { | ||
| done(e); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| it(`exits with proper error when prompting disabled and refined schema validation fails`, (done) => { | ||
| cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock schema refined'); | ||
| sinon.stub(cli, 'getSettingWithDefaultValue').callsFake((settingName, defaultValue) => { | ||
| if (settingName === settingsNames.prompt) { | ||
| return false; | ||
| } | ||
| return defaultValue; | ||
| }); | ||
| const executeCommandSpy = sinon.spy(cli, 'executeCommand'); | ||
|
|
||
| cli | ||
| .execute(['cli', 'mock', 'schema', 'refined', '--authType', 'password']) | ||
| .then(_ => done('Promise fulfilled while error expected'), _ => { | ||
| try { | ||
| assert(executeCommandSpy.notCalled); | ||
| done(); | ||
| } | ||
| catch (e) { | ||
| done(e); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| it(`executes command when validation passed`, async () => { | ||
| cli.commandToExecute = cli.commands.find(c => c.name === 'cli mock'); | ||
|
|
||
|
|
@@ -1725,7 +1877,7 @@ describe('cli', () => { | |
| await cli.loadCommandFromArgs(['spo', 'site', 'list']); | ||
| cli.printAvailableCommands(); | ||
|
|
||
| assert(cliLogStub.calledWith(' cli * 11 commands')); | ||
| assert(cliLogStub.calledWith(' cli * 12 commands')); | ||
| }); | ||
|
|
||
| it(`prints commands from the specified group`, async () => { | ||
|
|
@@ -1738,7 +1890,7 @@ describe('cli', () => { | |
| }; | ||
| cli.printAvailableCommands(); | ||
|
|
||
| assert(cliLogStub.calledWith(' cli mock * 8 commands')); | ||
| assert(cliLogStub.calledWith(' cli mock * 9 commands')); | ||
| }); | ||
|
|
||
| it(`prints commands from the root group when the specified string doesn't match any group`, async () => { | ||
|
|
@@ -1751,7 +1903,7 @@ describe('cli', () => { | |
| }; | ||
| cli.printAvailableCommands(); | ||
|
|
||
| assert(cliLogStub.calledWith(' cli * 11 commands')); | ||
| assert(cliLogStub.calledWith(' cli * 12 commands')); | ||
| }); | ||
|
|
||
| it(`runs properly when context file not found`, async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ import os from 'os'; | |||||||||||||||||||||||||||||||||||||||
| import path from 'path'; | ||||||||||||||||||||||||||||||||||||||||
| import { fileURLToPath, pathToFileURL } from 'url'; | ||||||||||||||||||||||||||||||||||||||||
| import yargs from 'yargs-parser'; | ||||||||||||||||||||||||||||||||||||||||
| import { ZodError } from 'zod'; | ||||||||||||||||||||||||||||||||||||||||
| import z, { ZodError } from 'zod'; | ||||||||||||||||||||||||||||||||||||||||
| import Command, { CommandArgs, CommandError } from '../Command.js'; | ||||||||||||||||||||||||||||||||||||||||
| import GlobalOptions from '../GlobalOptions.js'; | ||||||||||||||||||||||||||||||||||||||||
| import config from '../config.js'; | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -186,15 +186,35 @@ async function execute(rawArgs: string[]): Promise<void> { | |||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||
| const hasNonRequiredErrors = result.error.issues.some(i => i.code !== 'invalid_type'); | ||||||||||||||||||||||||||||||||||||||||
| const shouldPrompt = cli.getSettingWithDefaultValue<boolean>(settingsNames.prompt, true); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (hasNonRequiredErrors === false && | ||||||||||||||||||||||||||||||||||||||||
| shouldPrompt) { | ||||||||||||||||||||||||||||||||||||||||
| if (!shouldPrompt) { | ||||||||||||||||||||||||||||||||||||||||
| result.error.issues.forEach(e => { | ||||||||||||||||||||||||||||||||||||||||
| if (e.code === 'invalid_type' && | ||||||||||||||||||||||||||||||||||||||||
| e.input === undefined) { | ||||||||||||||||||||||||||||||||||||||||
| (e.message as any) = `Required option not specified`; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(result.error, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const missingRequiredValuesErrors: z.core.$ZodIssue[] = result.error.issues | ||||||||||||||||||||||||||||||||||||||||
| .filter(e => (e.code === 'invalid_type' && e.input === undefined) || | ||||||||||||||||||||||||||||||||||||||||
| (e.code === 'custom' && e.params?.customCode === 'required')); | ||||||||||||||||||||||||||||||||||||||||
| const optionSetErrors: z.core.$ZodIssueCustom[] = result.error.issues | ||||||||||||||||||||||||||||||||||||||||
| .filter(e => e.code === 'custom' && e.params?.customCode === 'optionSet') as z.core.$ZodIssueCustom[]; | ||||||||||||||||||||||||||||||||||||||||
| const otherErrors: z.core.$ZodIssue[] = result.error.issues | ||||||||||||||||||||||||||||||||||||||||
| .filter(e => !missingRequiredValuesErrors.includes(e) && !optionSetErrors.includes(e as z.core.$ZodIssueCustom)); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (otherErrors.some(e => e)) { | ||||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(result.error, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
Jwaegebaert marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (missingRequiredValuesErrors.some(e => e)) { | ||||||||||||||||||||||||||||||||||||||||
| await cli.error('🌶️ Provide values for the following parameters:'); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| for (const issue of result.error.issues) { | ||||||||||||||||||||||||||||||||||||||||
| const optionName = issue.path.join('.'); | ||||||||||||||||||||||||||||||||||||||||
| for (const error of missingRequiredValuesErrors) { | ||||||||||||||||||||||||||||||||||||||||
| const optionName = error.path.join('.'); | ||||||||||||||||||||||||||||||||||||||||
| const optionInfo = cli.commandToExecute.options.find(o => o.name === optionName); | ||||||||||||||||||||||||||||||||||||||||
| const answer = await cli.promptForValue(optionInfo!); | ||||||||||||||||||||||||||||||||||||||||
| // coerce the answer to the correct type | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -206,15 +226,14 @@ async function execute(rawArgs: string[]): Promise<void> { | |||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(e.message, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| else { | ||||||||||||||||||||||||||||||||||||||||
| result.error.issues.forEach(i => { | ||||||||||||||||||||||||||||||||||||||||
| if (i.code === 'invalid_type' && | ||||||||||||||||||||||||||||||||||||||||
| i.input === undefined) { | ||||||||||||||||||||||||||||||||||||||||
| (i.message as any) = `Required option not specified`; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| return cli.closeWithError(result.error, cli.optionsFromArgs, true); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (optionSetErrors.some(e => e)) { | ||||||||||||||||||||||||||||||||||||||||
| for (const error of optionSetErrors) { | ||||||||||||||||||||||||||||||||||||||||
| await promptForOptionSetNameAndValue(cli.optionsFromArgs, error.params?.options); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1057,6 +1076,16 @@ function shouldTrimOutput(output: string | undefined): boolean { | |||||||||||||||||||||||||||||||||||||||
| return output === 'text'; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| async function promptForOptionSetNameAndValue(args: CommandArgs, options: string[]): Promise<void> { | ||||||||||||||||||||||||||||||||||||||||
| await cli.error(`🌶️ Please specify one of the following options:`); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const selectedOptionName = await prompt.forSelection<string>({ message: `Option to use:`, choices: options.map((choice: any) => { return { name: choice, value: choice }; }) }); | ||||||||||||||||||||||||||||||||||||||||
| const optionValue = await prompt.forInput({ message: `${selectedOptionName}:` }); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| args.options[selectedOptionName] = optionValue; | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| args.options[selectedOptionName] = optionValue; | |
| let coercedValue: unknown = optionValue; | |
| // Basic type coercion to align with how CLI arguments are typically parsed | |
| if (typeof optionValue === 'string') { | |
| const trimmed = optionValue.trim(); | |
| if (trimmed === 'true') { | |
| coercedValue = true; | |
| } | |
| else if (trimmed === 'false') { | |
| coercedValue = false; | |
| } | |
| else if (trimmed !== '' && !isNaN(Number(trimmed))) { | |
| coercedValue = Number(trimmed); | |
| } | |
| } | |
| args.options[selectedOptionName] = coercedValue as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I actually managed to reproduce the type coercion bug. Here's what happens with m365 pp environment get:
- You get prompted to pick an option > select default
- Type something as the value > CLI crashes with Error: The value 'test' for option '--default' is not a valid boolean
- Even if you type true, it loops back and prompts again for the default as if it's still missing
The value gets stored as a raw string ("true") in args.options, but the Zod schema expects a proper boolean true. So validation fails again, this time as a missingRequiredValuesErrors error, which re-prompts you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit nitpicking but I think
.length > 0would be a bit cleaner in this scenarioThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a performance penalty for using length over some. Typically, when you use length, the runtime needs to calculate the full length of a collection. In comparison, when you use some, the runtime uses the iterator and stops as soon as the condition is met, which would be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, it’s a field the engine keeps up to date when the array changes, so checking
otherErrors.length > 0is basically just a quick property read.