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.

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

util: support 'option.required' in parseArgs #44565

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -2464,6 +2464,17 @@ added:
When `strict` set to `true`, thrown by [`util.parseArgs()`][] if an argument
is not configured in `options`.

<a id="ERR_PARSE_ARGS_REQUIRED_OPTION"></a>

### `ERR_PARSE_ARGS_REQUIRED_OPTION`

<!-- YAML
added: REPLACEME
-->

When `required` set to `true`, thrown by [`util.parseArgs()`][] if an option
is not provided in `args`.

<a id="ERR_PERFORMANCE_INVALID_TIMESTAMP"></a>

### `ERR_PERFORMANCE_INVALID_TIMESTAMP`
Expand Down
10 changes: 8 additions & 2 deletions doc/api/util.md
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,9 @@ added:
- v18.3.0
- v16.17.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/44565
description: support `required` field in option.
- version:
- v18.7.0
- v16.17.0
Expand All @@ -1053,6 +1056,7 @@ changes:
times. If `true`, all values will be collected in an array. If
`false`, values for the option are last-wins. **Default:** `false`.
* `short` {string} A single character alias for the option.
* `required` {boolean} Whether this option is required. **Default:** `false`.
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the default depend on strict?

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not sure what it should be🤔
But in my real code, I just check if it is a truely value.

Copy link
Member

Choose a reason for hiding this comment

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

No, I think the behaviour is independent of strict. Options which take a value generate an error in strict, but there is a difference between an option with a required option-argument (i.e. type='string') and a required option as proposed here.

* `strict` {boolean} Should an error be thrown when unknown arguments
are encountered, or when arguments are passed that do not match the
`type` configured in `options`.
Expand Down Expand Up @@ -1085,7 +1089,8 @@ const options = {
short: 'f'
},
bar: {
type: 'string'
type: 'string',
required: true
Copy link
Member

Choose a reason for hiding this comment

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

I suggest not including required in the example, as it is a less common configuration. The majority of programs will not use required, and there is some small potential for confusion between options expecting an argument, and required options.

}
};
const {
Expand All @@ -1105,7 +1110,8 @@ const options = {
short: 'f'
},
bar: {
type: 'string'
type: 'string',
required: true
}
};
const {
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1511,6 +1511,7 @@ E('ERR_PACKAGE_PATH_NOT_EXPORTED', (pkgPath, subpath, base = undefined) => {
pkgPath}package.json${base ? ` imported from ${base}` : ''}`;
}, Error);
E('ERR_PARSE_ARGS_INVALID_OPTION_VALUE', '%s', TypeError);
E('ERR_PARSE_ARGS_REQUIRED_OPTION', "Required option '%s'.", TypeError);
E('ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL', "Unexpected argument '%s'. This " +
'command does not take positional arguments', TypeError);
E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => {
Expand Down
11 changes: 11 additions & 0 deletions lib/internal/util/parse_args/parse_args.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const {
ERR_PARSE_ARGS_INVALID_OPTION_VALUE,
ERR_PARSE_ARGS_UNKNOWN_OPTION,
ERR_PARSE_ARGS_UNEXPECTED_POSITIONAL,
ERR_PARSE_ARGS_REQUIRED_OPTION,
},
} = require('internal/errors');

Expand Down Expand Up @@ -336,6 +337,16 @@ const parseArgs = (config = kEmptyObject) => {
}
});

// Phase 3: check if some options are required
ArrayPrototypeForEach(
ObjectEntries(options),
({ 0: longOption, 1: optionConfig }) => {
if (optionConfig.required && result.values[longOption] === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

The options came from the user, and is subject to possible prototype pollution. There is a helper routine objectGetOwn to do this more robustly:

function objectGetOwn(obj, prop) {

(The result was made internally with null prototype so can be used more conventionally with safety.)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, even more paranoid and robust is optionsGetOwn:

function optionsGetOwn(options, longOption, prop) {

throw new ERR_PARSE_ARGS_REQUIRED_OPTION(longOption);
}
}
);

return result;
};

Expand Down
75 changes: 75 additions & 0 deletions test/parallel/test-parse-args.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -823,3 +823,78 @@ test('tokens: strict:false with -- --', () => {
const { tokens } = parseArgs({ strict: false, args, tokens: true });
assert.deepStrictEqual(tokens, expectedTokens);
});

test('strict: required option', () => {
const args = ['--foo'];
parseArgs({
args,
options: {
foo: {
type: 'boolean',
required: true
}
}
});
});

test('required option', () => {
const args = ['--foo', '--goo'];
parseArgs({
strict: false,
args,
options: {
foo: {
type: 'boolean',
required: true
}
}
});
});

test('strict: false required option fail', () => {
const args = [];
assert.throws(() => {
parseArgs({
strict: false,
args,
options: {
foo: {
type: 'boolean',
required: true
}
}
});
}, {
code: 'ERR_PARSE_ARGS_REQUIRED_OPTION'
});
});

test('strict: no input but has required option', () => {
const args = [];
assert.throws(() => {
parseArgs({
args,
options: {
foo: {
type: 'boolean',
required: true
}
}
});
}, {
code: 'ERR_PARSE_ARGS_REQUIRED_OPTION'
});
});

test('strict: no input and no required option', () => {
const args = [];
parseArgs({
args,
options: {
foo: {
type: 'boolean',
required: false
}
}
});
});