pallets / click

Python composable command line interface toolkit
https://click.palletsprojects.com
BSD 3-Clause "New" or "Revised" License
15.62k stars 1.4k forks source link

click.Path(resolve_path=True) crash on non-Path #2742

Closed msftcangoblowm closed 3 months ago

msftcangoblowm commented 3 months ago

This is a working minimal unittest module demonstrating the issue. tl;dr; EPILOG_CHOKE contains the bug report

mkdir integration
touch integration/test_upstream_click_issue.py

Uncomment the assert False to see the locals Copy+paste into integration/test_upstream_click_issue.py

python -m unittest integration.test_upstream_click_issue --locals

(The paths in the traceback has been obfusicated)

integration/test_upstream_click_issue.py

import sys
import traceback
import unittest
from pathlib import Path

import click
from click.testing import CliRunner

help_path = "Provide a Path. relative or absolute"

EPILOG_CHOKE = """
Description:

click.Path(resolve_path=True) assumes can resolve the user supplied input.
A float is not a Path, so cannot be resolved. Results in a TypeError and the
entrypoint crashing with a traceback.

End users should never be presented with a traceback. When click is
dealing with these issues the normal exit code is 2

Reproduce issue:

python -m unittest integration.test_upstream_click_issue

Expected behavior:

Do not make assumptions regarding the user input type. Handler should
accept typing.Any

- Ideally and gracefully, use the default as a fallback. No exit code

- Otherwise exit code 2

Why'd this happen?:

Python typing encourages parameters ideally to accept typing.Any
Unless already handled elsewhere, assuming parameters have particular
type(s) is a recipe for disaster. Should always check

Environment:

click: 8.1.7
Python: py39
OS: GNU/Linux
distro: Void Linux

Output:

  File ".venv/lib/python3.9/site-packages/click/testing.py", line 408, in invoke
    return_value = cli.main(args=args or (), prog_name=prog_name, **extra)
  File ".venv/lib/python3.9/site-packages/click/core.py", line 1077, in main
    with self.make_context(prog_name, args, **extra) as ctx:
  File ".venv/lib/python3.9/site-packages/click/core.py", line 943, in make_context
    self.parse_args(ctx, args)
  File ".venv/lib/python3.9/site-packages/click/core.py", line 1408, in parse_args
    value, args = param.handle_parse_result(ctx, opts, args)
  File ".venv/lib/python3.9/site-packages/click/core.py", line 2400, in handle_parse_result
    value = self.process_value(ctx, value)
  File ".venv/lib/python3.9/site-packages/click/core.py", line 2356, in process_value
    value = self.type_cast_value(ctx, value)
  File ".venv/lib/python3.9/site-packages/click/core.py", line 2344, in type_cast_value
    return convert(value)
  File ".venv/lib/python3.9/site-packages/click/core.py", line 2316, in convert
    return self.type(value, param=self, ctx=ctx)
  File ".venv/lib/python3.9/site-packages/click/types.py", line 83, in __call__
    return self.convert(value, param, ctx)
  File ".venv/lib/python3.9/site-packages/click/types.py", line 869, in convert
    rv = os.fsdecode(pathlib.Path(rv).resolve())
  File "~/.pyenv/versions/3.9.16/lib/python3.9/pathlib.py", line 1082, in __new__
    self = cls._from_parts(args, init=False)
  File "~/.pyenv/versions/3.9.16/lib/python3.9/pathlib.py", line 707, in _from_parts
    drv, root, parts = self._parse_args(args)
  File "~/.pyenv/versions/3.9.16/lib/python3.9/pathlib.py", line 691, in _parse_args
    a = os.fspath(a)
F
======================================================================
FAIL: test_choke_on_this (integration.test_upstream_click_issue.TestChokeOnNonPath)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "integration/test_upstream_click_issue.py", line 86, in test_choke_on_this
    assert False
    cmd = ('--path', 0.1234)
    exit_code = 1
    result = <Result TypeError('expected str, bytes or os.PathLike object, not float')>
    runner = <click.testing.CliRunner object at 0x7fdb2060c4c0>
    self = <integration.test_upstream_click_issue.TestChokeOnNonPath testMethod=test_choke_on_this>
    str_tb = ...
    tb = <traceback object at 0x7fdb20b753c0>
AssertionError

----------------------------------------------------------------------
Ran 1 test in 0.003s

FAILED (failures=1)

"""

@click.group(
    context_settings={"help_option_names": ["-h", "--help"]},
)
@click.version_option(version="0.0.1")
def main():
    """Command-line. Prints usage"""

@main.command(
    "choke",
    context_settings={"ignore_unknown_options": True},
    epilog=EPILOG_CHOKE,
)
@click.option(
    "-p",
    "--path",
    "path",
    default=Path.cwd(),
    type=click.Path(exists=False, file_okay=False, dir_okay=True, resolve_path=True),
    help=help_path,
)
def choke_on_non_path(path):
    # When a relative path, gets resolved, but returns as str rather than Path
    if isinstance(path, str):
        path = Path(path)

class TestChokeOnNonPath(unittest.TestCase):
    def test_choke_on_this(self):
        """"""
        runner = CliRunner()
        cmd = (
            "--path",
            0.1234,
        )
        # result = <Result TypeError('expected str, bytes or os.PathLike object, not float')>
        result = runner.invoke(choke_on_non_path, cmd)

        # An uncatchable (within the click command, choke_on_non_path) TypeError
        # is equivalent to click crashing
        assert result.exception.__class__ == TypeError

        tb = result.exc_info[2]
        # str_tb = traceback.format_tb(tb)
        traceback.print_tb(tb)

        exit_code = result.exit_code
        # ideally use the default, Path.cwd() and exit code == 0
        assert exit_code != 2  # expecting 2
        assert exit_code == 1  # meaningless exit code

        # uncomment to see the locals
        # assert False
        pass

if __name__ == "__main__":  # pragma: no cover
    """
    .. code-block:: shell

       python -m unittest integration.test_upstream_click_issue --locals

    """
    unittest.main(tb_locals=True)
davidism commented 3 months ago

End users should never be presented with a traceback.

You're using the test client. You still need to pass strings, just like the command line would pass. End users wouldn't see this.

Do not make assumptions regarding the user input type. Handler should accept typing.Any

It sounds like you may have confused python typing with click param type converters.

msftcangoblowm commented 3 months ago

From a shell, the click entrypoint receives strings. Didn't realize this. Actually had to confirm couldn't pass in a float value.

Thru the test client, was not aware should only be passing in str, the same as the shell

Thank you for your quick response and clear explaination