From c4b183bc6d6083dad0754e42de19e96a867ff8ed Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Sun, 21 May 2023 23:55:40 +0200 Subject: [PATCH] `InteractiveOption`: Fix validation being skipped if `!` provided The `InteractiveOption` reserves the exclamation point `!` as a special character in order to "skip" the option and not define it. This is necessary for options that are _not_ required but that do specify a defualt. Without this character, it is impossible to _not_ set the default as, if the user doesn't specify anything specific and simply presses enter, the default is taken, even if the user did not want to specify any specific value. The problem with the implementation, however, is that when `!` was provided, the option would return `None` as the value, bypassing any validation that might be defined. This would make it possible to bypass the validation of a required option. The solution is to, when `!` is provided for an interactive option, it is translated to `None` and is then processed as normal, validating it as any other value. --- .../cmdline/params/options/commands/computer.py | 4 ++-- aiida/cmdline/params/options/commands/setup.py | 4 ---- aiida/cmdline/params/options/interactive.py | 5 ++++- tests/cmdline/params/options/test_interactive.py | 16 ++++++++++++++++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/aiida/cmdline/params/options/commands/computer.py b/aiida/cmdline/params/options/commands/computer.py index 5d419a9d69..7e1cfc0859 100644 --- a/aiida/cmdline/params/options/commands/computer.py +++ b/aiida/cmdline/params/options/commands/computer.py @@ -126,7 +126,7 @@ def should_call_default_memory_per_machine(ctx): # pylint: disable=invalid-name prompt='Default number of CPUs per machine', cls=InteractiveOption, prompt_fn=should_call_default_mpiprocs_per_machine, - required_fn=False, + required=False, type=click.INT, help='The default number of MPI processes that should be executed per machine (node), if not otherwise specified.' 'Use 0 to specify no default value.', @@ -137,7 +137,7 @@ def should_call_default_memory_per_machine(ctx): # pylint: disable=invalid-name prompt='Default amount of memory per machine (kB).', cls=InteractiveOption, prompt_fn=should_call_default_memory_per_machine, - required_fn=False, + required=False, type=click.INT, help='The default amount of RAM (kB) that should be allocated per machine (node), if not otherwise specified.' ) diff --git a/aiida/cmdline/params/options/commands/setup.py b/aiida/cmdline/params/options/commands/setup.py index ab086b0e60..8372cb2006 100644 --- a/aiida/cmdline/params/options/commands/setup.py +++ b/aiida/cmdline/params/options/commands/setup.py @@ -177,7 +177,6 @@ def get_quicksetup_password(ctx, param, value): # pylint: disable=unused-argume SETUP_USER_EMAIL = options.USER_EMAIL.clone( prompt='Email Address (for sharing data)', default=get_config_option('autofill.user.email'), - required_fn=lambda x: get_config_option('autofill.user.email') is None, required=True, cls=options.interactive.InteractiveOption ) @@ -185,7 +184,6 @@ def get_quicksetup_password(ctx, param, value): # pylint: disable=unused-argume SETUP_USER_FIRST_NAME = options.USER_FIRST_NAME.clone( prompt='First name', default=get_config_option('autofill.user.first_name'), - required_fn=lambda x: get_config_option('autofill.user.first_name') is None, required=True, cls=options.interactive.InteractiveOption ) @@ -193,7 +191,6 @@ def get_quicksetup_password(ctx, param, value): # pylint: disable=unused-argume SETUP_USER_LAST_NAME = options.USER_LAST_NAME.clone( prompt='Last name', default=get_config_option('autofill.user.last_name'), - required_fn=lambda x: get_config_option('autofill.user.last_name') is None, required=True, cls=options.interactive.InteractiveOption ) @@ -201,7 +198,6 @@ def get_quicksetup_password(ctx, param, value): # pylint: disable=unused-argume SETUP_USER_INSTITUTION = options.USER_INSTITUTION.clone( prompt='Institution', default=get_config_option('autofill.user.institution'), - required_fn=lambda x: get_config_option('autofill.user.institution') is None, required=True, cls=options.interactive.InteractiveOption ) diff --git a/aiida/cmdline/params/options/interactive.py b/aiida/cmdline/params/options/interactive.py index 7c78c532d0..d4b5e42b23 100644 --- a/aiida/cmdline/params/options/interactive.py +++ b/aiida/cmdline/params/options/interactive.py @@ -115,7 +115,10 @@ def process_value(self, ctx: click.Context, value: t.Any) -> t.Any: return self.prompt_for_value(ctx) if value == self.CHARACTER_IGNORE_DEFAULT and source is click.core.ParameterSource.PROMPT: - return None + # This means the user does not want to set a specific value for this option, so the ``!`` character is + # translated to ``None`` and processed as normal. If the option is required, a validation error will be + # raised further down below, forcing the user to specify a valid value. + value = None try: return super().process_value(ctx, value) diff --git a/tests/cmdline/params/options/test_interactive.py b/tests/cmdline/params/options/test_interactive.py index 1e17ab3f1c..eaa3a7167f 100644 --- a/tests/cmdline/params/options/test_interactive.py +++ b/tests/cmdline/params/options/test_interactive.py @@ -485,6 +485,22 @@ def test_not_required_interactive_default(run_cli_command): assert expected in result.output +def test_interactive_ignore_default_not_required_option(run_cli_command): + """Test that if an option is not required ``!`` is accepted and is translated to ``None``.""" + cmd = create_command(required=False) + result = run_cli_command(cmd, user_input='!\n') + expected = 'Opt: !\nNone\n' + assert expected in result.output + + +def test_interactive_ignore_default_required_option(run_cli_command): + """Test that if an option is required, ``!`` is translated to ``None`` and so is not accepted.""" + cmd = create_command(required=True) + result = run_cli_command(cmd, user_input='!\nvalue\n') + expected = 'Opt: !\nError: Missing parameter: opt\nOpt: value\nvalue\n' + assert expected in result.output + + def test_get_help_message(): """Test the :meth:`aiida.cmdline.params.options.interactive.InteractiveOption.get_help_message`.""" option = InteractiveOption('-s', type=click.STRING)