Skip to content

CLI: Fail command if --config file contains unknown key #5939

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

Merged
Merged
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
131 changes: 123 additions & 8 deletions aiida/cmdline/params/options/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,19 @@
"""
.. py:module::config
:synopsis: Convenience class for configuration file option

The functions :func:`configuration_callback` and :func:`configuration_option` were directly taken from the repository
https://github.com/phha/click_config_file/blob/7b93a20b4c79458987fac116418859f30a16d82a/click_config_file.py with a
minor modification to ``configuration_callback`` to add a check for unknown parameters in the configuration file and
the default provider is changed to :func:`yaml_config_file_provider`.
"""
import click_config_file
from __future__ import annotations

import functools
import os
import typing as t

import click
import yaml

from .overridable import OverridableOption
Expand All @@ -25,9 +36,115 @@ def yaml_config_file_provider(handle, cmd_name): # pylint: disable=unused-argum
return yaml.safe_load(handle)


class ConfigFileOption(OverridableOption):
def configuration_callback(
cmd_name: str | None,
option_name: str,
config_file_name: str,
saved_callback: t.Callable[..., t.Any] | None,
provider: t.Callable[..., t.Any],
implicit: bool,
ctx: click.Context,
param: click.Parameter,
value: t.Any,
):
"""Callback for reading the config file.

Also takes care of calling user specified custom callback afterwards.

:param cmd_name: The command name. This is used to determine the configuration directory.
:param option_name: The name of the option. This is used for error messages.
:param config_file_name: The name of the configuration file.
:param saved_callback: User-specified callback to be called later.
:param provider: A callable that parses the configuration file and returns a dictionary of the configuration
parameters. Will be called as ``provider(file_path, cmd_name)``. Default: ``yaml_config_file_provider``.
:param implicit: Whether a implicit value should be applied if no configuration option value was provided.
:param ctx: ``click`` context.
:param param: ``click`` parameters.
:param value: Value passed to the parameter.
"""
ctx.default_map = ctx.default_map or {}
cmd_name = cmd_name or ctx.info_name

if implicit:
default_value = os.path.join(click.get_app_dir(cmd_name), config_file_name) # type: ignore[arg-type]
param.default = default_value
value = value or default_value

if value:
try:
config = provider(value, cmd_name)
except Exception as exception:
raise click.BadOptionUsage(option_name, f'Error reading configuration file: {exception}', ctx)

valid_params = [param.name for param in ctx.command.params if param.name != option_name]
specified_params = list(config.keys())
unknown_params = set(specified_params).difference(set(valid_params))

if unknown_params:
raise click.BadParameter(
f'Invalid configuration file, the following keys are not supported: {unknown_params}', ctx, param
Copy link
Contributor

Choose a reason for hiding this comment

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

A nitpick:
Is BadParameter the best error class to use here?
I feel like the parameter to the option --config is the path or URL.
That said, the other click error classes are no better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am open to suggestions for better exceptions but I couldn't find one. Note that for the exception to be properly handled, it needs to be a click exception. I think it makes sense to say there is a problem with the --config option, because its value (the content of the file) is incorrect. Since the problem is likely that it contains unsupported keys, we couldn't have it be coupled to any other option of the command, as they don't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm convinced!

)

ctx.default_map.update(config)

return saved_callback(ctx, param, value) if saved_callback else value


def configuration_option(*param_decls, **attrs):
"""Adds configuration file support to a click application.

This will create an option of type ``click.File`` expecting the path to a configuration file. When specified, it
overwrites the default values for all other click arguments or options with the corresponding value from the
configuration file. The default name of the option is ``--config``. By default, the configuration will be read from
a configuration directory as determined by ``click.get_app_dir``. This decorator accepts the same arguments as
``click.option`` and ``click.Path``. In addition, the following keyword arguments are available:

:param cmd_name: str
The command name. This is used to determine the configuration directory. Default: ``ctx.info_name``.
:param config_file_name: str
The name of the configuration file. Default: ``config``.
:param implicit: bool
If ``True`` then implicitly create a value for the configuration option using the above parameters. If a
configuration file exists in this path it will be applied even if no configuration option was suppplied as a
CLI argument or environment variable. If ``False`` only apply a configuration file that has been explicitely
specified. Default: ``False``.
:param provider: callable
A callable that parses the configuration file and returns a dictionary of the configuration parameters. Will be
called as ``provider(file_path, cmd_name)``. Default: ``yaml_config_file_provider``.
"""
Wrapper around click_config_file.configuration_option that increases reusability.
param_decls = param_decls or ('--config',)
option_name = param_decls[0]

def decorator(func):
attrs.setdefault('is_eager', True)
attrs.setdefault('help', 'Read configuration from FILE.')
attrs.setdefault('expose_value', False)
implicit = attrs.pop('implicit', True)
cmd_name = attrs.pop('cmd_name', None)
config_file_name = attrs.pop('config_file_name', 'config')
provider = attrs.pop('provider', yaml_config_file_provider)
path_default_params = {
'exists': False,
'file_okay': True,
'dir_okay': False,
'writable': False,
'readable': True,
'resolve_path': False
}
path_params = {k: attrs.pop(k, v) for k, v in path_default_params.items()}
attrs['type'] = attrs.get('type', click.Path(**path_params))
saved_callback = attrs.pop('callback', None)
partial_callback = functools.partial(
configuration_callback, cmd_name, option_name, config_file_name, saved_callback, provider, implicit
)
attrs['callback'] = partial_callback
return click.option(*param_decls, **attrs)(func)

return decorator


class ConfigFileOption(OverridableOption):
"""Reusable option that reads a configuration file containing values for other command parameters.

Example::

Expand All @@ -49,8 +166,7 @@ def computer_setup(computer_name):
"""

def __init__(self, *args, **kwargs):
"""
Store the default args and kwargs.
"""Store the default args and kwargs.

:param args: default arguments to be used for the option
:param kwargs: default keyword arguments to be used that can be overridden in the call
Expand All @@ -59,8 +175,7 @@ def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

def __call__(self, **kwargs):
"""
Override the stored kwargs, (ignoring args as we do not allow option name changes) and return the option.
"""Override the stored kwargs, (ignoring args as we do not allow option name changes) and return the option.

:param kwargs: keyword arguments that will override those set in the construction
:return: click_config_file.configuration_option constructed with args and kwargs defined during construction
Expand All @@ -69,4 +184,4 @@ def __call__(self, **kwargs):
kw_copy = self.kwargs.copy()
kw_copy.update(kwargs)

return click_config_file.configuration_option(*self.args, **kw_copy)
return configuration_option(*self.args, **kw_copy)
1 change: 0 additions & 1 deletion environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ dependencies:
- archive-path~=0.4.2
- aio-pika~=6.6
- circus~=0.18.0
- click-config-file~=0.6.0
- click-spinner~=0.1.8
- click~=8.1
- disk-objectstore~=0.6.0
Expand Down
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ dependencies = [
"archive-path~=0.4.2",
"aio-pika~=6.6",
"circus~=0.18.0",
"click-config-file~=0.6.0",
"click-spinner~=0.1.8",
"click~=8.1",
"disk-objectstore~=0.6.0",
Expand Down
1 change: 0 additions & 1 deletion requirements/requirements-py-3.10.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ cffi==1.15.0
charset-normalizer==2.0.8
circus==0.18.0
click==8.1.0
click-config-file==0.6.0
click-spinner==0.1.10
configobj==5.0.6
coverage==6.5.0
Expand Down
1 change: 0 additions & 1 deletion requirements/requirements-py-3.11.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ cffi==1.15.1
charset-normalizer==2.1.1
circus==0.18.0
click==8.1.3
click-config-file==0.6.0
click-spinner==0.1.10
configobj==5.0.6
contourpy==1.0.6
Expand Down
1 change: 0 additions & 1 deletion requirements/requirements-py-3.8.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ cffi==1.15.0
charset-normalizer==2.0.8
circus==0.18.0
click==8.1.0
click-config-file==0.6.0
click-spinner==0.1.10
configobj==5.0.6
coverage==6.5.0
Expand Down
1 change: 0 additions & 1 deletion requirements/requirements-py-3.9.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ cffi==1.15.0
charset-normalizer==2.0.8
circus==0.18.0
click==8.1.0
click-config-file==0.6.0
click-spinner==0.1.10
configobj==5.0.6
coverage==6.5.0
Expand Down
63 changes: 63 additions & 0 deletions tests/cmdline/params/options/test_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# -*- coding: utf-8 -*-
###########################################################################
# Copyright (c), The AiiDA team. All rights reserved. #
# This file is part of the AiiDA code. #
# #
# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core #
# For further information on the license, see the LICENSE.txt file #
# For further information please visit http://www.aiida.net #
###########################################################################
# pylint: disable=redefined-outer-name
"""Unit tests for the :class:`aiida.cmdline.params.options.config.ConfigOption`."""
import functools
import tempfile
import textwrap

import click
import pytest

from aiida.cmdline.params.options import CONFIG_FILE


@pytest.fixture
def run_cli_command(run_cli_command):
"""Override the ``run_cli_command`` fixture to always run with ``use_subprocess=False`` for tests in this module."""
return functools.partial(run_cli_command, use_subprocess=False)


@click.command()
@click.option('--integer', type=int)
@click.option('--boolean', type=bool)
@CONFIG_FILE()
def cmd(integer, boolean):
"""Test command for :class:`aiida.cmdline.params.options.config.ConfigOption`."""
click.echo(f'Integer: {integer}')
click.echo(f'Boolean: {boolean}')


def test_valid(run_cli_command):
"""Test the option for a valid configuration file."""
with tempfile.NamedTemporaryFile('w+') as handle:
handle.write(textwrap.dedent("""
integer: 1
boolean: false
"""))
handle.flush()

result = run_cli_command(cmd, ['--config', handle.name])
assert 'Integer: 1' in result.output_lines[0]
assert 'Boolean: False' in result.output_lines[1]


def test_invalid_unknown_keys(run_cli_command):
"""Test the option for an invalid configuration file containing unknown keys."""
with tempfile.NamedTemporaryFile('w+') as handle:
handle.write(textwrap.dedent("""
integer: 1
unknown: 2.0
"""))
handle.flush()

result = run_cli_command(cmd, ['--config', handle.name], raises=True)
assert "Error: Invalid value for '--config': Invalid configuration file" in result.stderr
assert "the following keys are not supported: {'unknown'}" in result.stderr