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.

Sign up for GitHub

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

Validate parsed config against CLI options #1910

Merged
merged 6 commits into from
Jul 16, 2023
Merged

Validate parsed config against CLI options #1910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e875e48
Validate parsed config against click command params
atugushev Jul 13, 2023
e3f649c
Fix tests
atugushev Jul 13, 2023
f68b744
Add tests
atugushev Jul 13, 2023
a6526e2
Cleanup
atugushev Jul 13, 2023
278d8df
Update docstring
atugushev Jul 15, 2023
b2cb863
Merge branch 'main' into validate-config
atugushev Jul 16, 2023
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
42 changes: 42 additions & 0 deletions piptools/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import collections
import copy
import difflib
import itertools
import json
import os
Expand Down Expand Up @@ -561,6 +562,7 @@ def override_defaults_from_config_file(

config = parse_config_file(config_file)
if config:
_validate_config(ctx, config)
_assign_config_to_cli_context(ctx, config)

return config_file
Expand All @@ -576,6 +578,45 @@ def _assign_config_to_cli_context(
click_context.default_map.update(cli_config_mapping)


def _validate_config(
click_context: click.Context,
config: dict[str, Any],
) -> None:
"""
Validate parsed config against click command params.

:raises click.NoSuchOption: if config contains unknown keys.
:raises click.BadOptionUsage: if config contains invalid values.
"""
cli_params = {
param.name: param
for param in click_context.command.params
if param.name is not None
}

for key, value in config.items():
# Validate unknown keys
if key not in cli_params:
possibilities = difflib.get_close_matches(key, cli_params.keys())
raise click.NoSuchOption(
option_name=key,
message=f"No such config key {key!r}.",
possibilities=possibilities,
ctx=click_context,
)

# Validate invalid values
param = cli_params[key]
try:
param.type.convert(value=value, param=param, ctx=click_context)
except Exception as e:
raise click.BadOptionUsage(
option_name=key,
message=f"Invalid value for config key {key!r}: {value!r}.",
ctx=click_context,
) from e
Comment on lines +597 to +617
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for key, value in config.items():
# Validate unknown keys
if key not in cli_params:
possibilities = difflib.get_close_matches(key, cli_params.keys())
raise click.NoSuchOption(
option_name=key,
message=f"No such config key {key!r}.",
possibilities=possibilities,
ctx=click_context,
)
# Validate invalid values
param = cli_params[key]
try:
param.type.convert(value=value, param=param, ctx=click_context)
except Exception as e:
raise click.BadOptionUsage(
option_name=key,
message=f"Invalid value for config key {key!r}: {value!r}.",
ctx=click_context,
) from e
swap_keys = False
new_items = {}
old_items = {}
for key, value in config.items():
# Validate unknown keys
if key not in cli_params:
# Replace boolean flag contexts like ``no_annotate`` with their equivalents
if key.startswith("no_"):
old_items[key] = value
key = key[3:]
value = not value
swap_keys = True
new_items[key] = value
else:
possibilities = difflib.get_close_matches(key, cli_params.keys())
raise click.NoSuchOption(
option_name=key,
message=f"No such config key {key!r}.",
possibilities=possibilities,
ctx=click_context,
)
# Validate invalid values
param = cli_params[key]
try:
param.type.convert(value=value, param=param, ctx=click_context)
except Exception as e:
raise click.BadOptionUsage(
option_name=key,
message=f"Invalid value for config key {key!r}: {value!r}.",
ctx=click_context,
) from e
if swap_keys:
for key, value in old_items.items():
del config[key]
for key, value in new_items.items():
config[key] = value

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense indeed. I would prefer to implement this in the parse_config_file() function though. The _validate_config() function does not intend a config mutation.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that function, you might be able to implement it like this:

    swap_items = False
    new_items = {}
    old_items = {}

    for key, value in piptools_config.items(): 
        # Replace boolean flag contexts like ``no_annotate`` with their equivalents
        if key.startswith("no_"):
            swap_items = True
            old_items[key] = value
            if isinstance(value, bool):
                value = not value
            new_items[key[3:]] = value

    if swap_items:
        for key, value in old_items.items():
            del piptools_config[key]
        for key, value in new_items.items():
            piptools_config[key] = value


    return piptools_config

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you like to address this in a following up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thanks 👍🏻

Copy link
Member Author

@atugushev atugushev Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As a reminder to myself: I think we can refactor parse_config_file to parse options using the technique from the following snippet.

Snippet 
(Pdb++) opts = {
    opt: option 
    for option in ctx.command.params
    for opt in itertools.chain(option.opts, option.secondary_opts)
}

(Pdb++) pp opts
{'--all-extras': <Option all_extras>,
 '--allow-unsafe': <Option allow_unsafe>,
 '--annotate': <Option annotate>,
 '--no-annotate': <Option annotate >,
 ...
}
 
(Pdb++) opts['--pip-args'].name
'pip_args_str'

(Pdb++) opts['--annotate'].name
'annotate'

(Pdb++) opts['--no-annotate'].name
'annotate'

(Pdb++) opts['--extra'].name
'extras'

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! I guess this would enable us to rationalise the function get_click_dest_for_option() away. Would you like me to address that too, while I'm at it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, feel free to!



def select_config_file(src_files: tuple[str, ...]) -> Path | None:
"""
Returns the config file to use for defaults given ``src_files`` provided.
Expand Down Expand Up @@ -614,6 +655,7 @@ def select_config_file(src_files: tuple[str, ...]) -> Path | None:
"upgrade_package": "upgrade_packages",
"resolver": "resolver_name",
"user": "user_only",
"pip_args": "pip_args_str",
}


Expand Down
28 changes: 28 additions & 0 deletions tests/test_cli_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2989,3 +2989,31 @@ def test_no_config_option_overrides_config_with_defaults(

assert out.exit_code == 0
assert "Dry-run, so nothing updated" not in out.stderr


def test_raise_error_on_unknown_config_option(
pip_conf, runner, tmp_path, make_config_file
):
config_file = make_config_file("unknown-option", True)

req_in = tmp_path / "requirements.in"
req_in.touch()

out = runner.invoke(cli, [req_in.as_posix(), "--config", config_file.as_posix()])

assert out.exit_code == 2
assert "No such config key 'unknown_option'" in out.stderr


def test_raise_error_on_invalid_config_option(
pip_conf, runner, tmp_path, make_config_file
):
config_file = make_config_file("dry-run", ["invalid", "value"])

req_in = tmp_path / "requirements.in"
req_in.touch()

out = runner.invoke(cli, [req_in.as_posix(), "--config", config_file.as_posix()])

assert out.exit_code == 2
assert "Invalid value for config key 'dry_run': ['invalid', 'value']" in out.stderr
26 changes: 26 additions & 0 deletions tests/test_cli_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,3 +398,29 @@ def test_no_config_option_overrides_config_with_defaults(run, runner, make_confi

assert out.exit_code == 0
assert "Would install:" not in out.stdout


@mock.patch("piptools.sync.run")
def test_raise_error_on_unknown_config_option(run, runner, tmp_path, make_config_file):
config_file = make_config_file("unknown-option", True)

with open(sync.DEFAULT_REQUIREMENTS_FILE, "w") as reqs_txt:
reqs_txt.write("six==1.10.0")

out = runner.invoke(cli, ["--config", config_file.as_posix()])

assert out.exit_code == 2
assert "No such config key 'unknown_option'" in out.stderr


@mock.patch("piptools.sync.run")
def test_raise_error_on_invalid_config_option(run, runner, tmp_path, make_config_file):
config_file = make_config_file("dry-run", ["invalid", "value"])

with open(sync.DEFAULT_REQUIREMENTS_FILE, "w") as reqs_txt:
reqs_txt.write("six==1.10.0")

out = runner.invoke(cli, ["--config", config_file.as_posix()])

assert out.exit_code == 2
assert "Invalid value for config key 'dry_run': ['invalid', 'value']" in out.stderr
5 changes: 0 additions & 5 deletions tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,21 +573,16 @@ def test_get_sys_path_for_python_executable():
@pytest.mark.parametrize(
("pyproject_param", "new_default"),
(
# From sync
("ask", True),
Copy link
Member Author

@atugushev atugushev Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FYI: I have removed the options for pip-sync (which led to test failures) because we only test the pip-compile CLI here. We could add a separate test for pip-sync with its own options, but it seems redundant to me.

("dry-run", True),
("find-links", ["changed"]),
("extra-index-url", ["changed"]),
("trusted-host", ["changed"]),
("no-index", True),
("python-executable", "changed"),
("verbose", True),
("quiet", True),
("user", True),
("cert", "changed"),
("client-cert", "changed"),
("pip-args", "changed"),
# From compile, unless also in sync
("pre", True),
("rebuild", True),
("extras", ["changed"]),
Expand Down