From 78f03b797094683e943ed4253df298a1ce1c54f1 Mon Sep 17 00:00:00 2001 From: Arun Babu Neelicattu Date: Sat, 11 Jan 2025 23:23:28 +0100 Subject: [PATCH 1/3] fix(application): handle global options correctly Previously, the application implementation did not handle global options correctly when positioned after command options. This lead to incorrect cached working directory values prior to switching application context. Resolves: #9978 --- src/poetry/console/application.py | 62 +++++++++++----- .../test_application_global_options.py | 70 +++++++++++++++++++ 2 files changed, 113 insertions(+), 19 deletions(-) create mode 100644 tests/console/test_application_global_options.py diff --git a/src/poetry/console/application.py b/src/poetry/console/application.py index dc17735770b..1dfe64eadd4 100644 --- a/src/poetry/console/application.py +++ b/src/poetry/console/application.py @@ -4,7 +4,6 @@ import re from contextlib import suppress -from functools import cached_property from importlib import import_module from pathlib import Path from typing import TYPE_CHECKING @@ -25,6 +24,7 @@ if TYPE_CHECKING: from collections.abc import Callable + from typing import Any from cleo.events.event import Event from cleo.io.inputs.argv_input import ArgvInput @@ -103,6 +103,8 @@ def __init__(self) -> None: self._disable_plugins = False self._disable_cache = False self._plugins_loaded = False + self._working_directory = Path.cwd() + self._project_directory = self._working_directory dispatcher = EventDispatcher() dispatcher.add_listener(COMMAND, self.register_command_loggers) @@ -156,21 +158,6 @@ def _default_definition(self) -> Definition: return definition - @cached_property - def _project_directory(self) -> Path: - if self._io and self._io.input.option("project"): - with directory(self._working_directory): - return Path(self._io.input.option("project")).absolute() - - return self._working_directory - - @cached_property - def _working_directory(self) -> Path: - if self._io and self._io.input.option("directory"): - return Path(self._io.input.option("directory")).absolute() - - return Path.cwd() - @property def poetry(self) -> Poetry: from poetry.factory import Factory @@ -227,9 +214,6 @@ def create_io( return io def _run(self, io: IO) -> int: - self._disable_plugins = io.input.parameter_option("--no-plugins") - self._disable_cache = io.input.has_parameter_option("--no-cache") - self._load_plugins(io) with directory(self._working_directory): @@ -237,6 +221,44 @@ def _run(self, io: IO) -> int: return exit_code + def _option_get_value(self, io: IO, name: str, default: Any) -> Any: + option = self.definition.option(name) + + if option is None: + return default + + values = [f"--{option.name}"] + + if option.shortcut: + values.append(f"-{option.shortcut}") + + if not io.input.has_parameter_option(values): + return default + + if option.is_flag(): + return True + + return io.input.parameter_option(values=values, default=default) + + def _configure_custom_application_options(self, io: IO) -> None: + self._disable_plugins = self._option_get_value( + io, "no-plugins", self._disable_plugins + ) + self._disable_cache = self._option_get_value( + io, "no-cache", self._disable_cache + ) + self._working_directory = self._project_directory = Path( + self._option_get_value(io, "directory", Path.cwd()) + ) + + self._project_directory = Path( + self._option_get_value(io, "project", self._working_directory) + ) + + if self._project_directory != self._working_directory: + with directory(self._working_directory): + self._project_directory = self._project_directory.absolute() + def _configure_io(self, io: IO) -> None: # We need to check if the command being run # is the "run" command. @@ -273,6 +295,8 @@ def _configure_io(self, io: IO) -> None: super()._configure_io(io) + self._configure_custom_application_options(io) + def register_command_loggers( self, event: Event, event_name: str, _: EventDispatcher ) -> None: diff --git a/tests/console/test_application_global_options.py b/tests/console/test_application_global_options.py new file mode 100644 index 00000000000..3fadc8db4a1 --- /dev/null +++ b/tests/console/test_application_global_options.py @@ -0,0 +1,70 @@ +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +from cleo.testers.application_tester import ApplicationTester + +from poetry.console.application import Application + + +if TYPE_CHECKING: + from tests.types import FixtureCopier + + +NO_PYPROJECT_TOML_ERROR = "Poetry could not find a pyproject.toml file in" + + +@pytest.fixture +def project_source_directory(fixture_copier: FixtureCopier) -> Path: + return fixture_copier("up_to_date_lock") + + +@pytest.fixture +def tester() -> ApplicationTester: + return ApplicationTester(Application()) + + +def test_application_global_option_ensure_error_when_context_invalid( + tester: ApplicationTester, +) -> None: + # command fails due to lack of pyproject.toml file in cwd + tester.execute("show --only main") + assert tester.status_code != 0 + + stderr = tester.io.fetch_error() + assert NO_PYPROJECT_TOML_ERROR in stderr + + +@pytest.mark.parametrize("parameter", ["-C", "--directory", "-P", "--project"]) +@pytest.mark.parametrize( + "command_args", + [ + "{option} show --only main", + "show {option} --only main", + "show --only main {option}", + ], +) +def test_application_global_option_position_does_not_matter( + parameter: str, + command_args: str, + tester: ApplicationTester, + project_source_directory: Path, +) -> None: + cwd = Path.cwd() + assert cwd != project_source_directory + + option = f"{parameter} {project_source_directory.as_posix()}" + tester.execute(command_args.format(option=option)) + assert tester.status_code == 0 + + stdout = tester.io.fetch_output() + stderr = tester.io.fetch_error() + + assert NO_PYPROJECT_TOML_ERROR not in stderr + assert NO_PYPROJECT_TOML_ERROR not in stdout + + assert "certifi" in stdout + assert len(stdout.splitlines()) == 8 From f37ba9420d470f43122bcef23bffc663d2c6d842 Mon Sep 17 00:00:00 2001 From: Arun Babu Neelicattu Date: Sun, 12 Jan 2025 00:07:29 +0100 Subject: [PATCH 2/3] chore(test): refactor app context tests --- tests/console/commands/test_version.py | 108 ------------------ .../test_application_global_options.py | 81 +++++++++++++ 2 files changed, 81 insertions(+), 108 deletions(-) diff --git a/tests/console/commands/test_version.py b/tests/console/commands/test_version.py index 4df35505c99..5741cf3d9f5 100644 --- a/tests/console/commands/test_version.py +++ b/tests/console/commands/test_version.py @@ -1,22 +1,14 @@ from __future__ import annotations -import os -import textwrap - -from pathlib import Path from typing import TYPE_CHECKING import pytest -from cleo.testers.application_tester import ApplicationTester - -from poetry.console.application import Application from poetry.console.commands.version import VersionCommand if TYPE_CHECKING: from cleo.testers.command_tester import CommandTester - from pytest_mock import MockerFixture from poetry.poetry import Poetry from tests.types import CommandTesterFactory @@ -140,103 +132,3 @@ def test_dry_run(tester: CommandTester) -> None: new_pyproject = tester.command.poetry.file.path.read_text(encoding="utf-8") assert tester.io.fetch_output() == "Bumping version from 1.2.3 to 2.0.0\n" assert old_pyproject == new_pyproject - - -def test_version_with_project_parameter( - fixture_dir: FixtureDirGetter, mocker: MockerFixture -) -> None: - app = Application() - tester = ApplicationTester(app) - - orig_version_command = VersionCommand.handle - - def mock_handle(command: VersionCommand) -> int: - exit_code = orig_version_command(command) - - command.io.write_line(f"ProjectPath: {command.poetry.pyproject_path.parent}") - command.io.write_line(f"WorkingDirectory: {os.getcwd()}") - - return exit_code - - mocker.patch("poetry.console.commands.version.VersionCommand.handle", mock_handle) - - source_dir = fixture_dir("scripts") - tester.execute(f"--project {source_dir} version") - - output = tester.io.fetch_output() - expected = textwrap.dedent(f"""\ - scripts 0.1.0 - ProjectPath: {source_dir} - WorkingDirectory: {os.getcwd()} - """) - - assert source_dir != Path(os.getcwd()) - assert output == expected - - -def test_version_with_directory_parameter( - fixture_dir: FixtureDirGetter, mocker: MockerFixture -) -> None: - app = Application() - tester = ApplicationTester(app) - - orig_version_command = VersionCommand.handle - - def mock_handle(command: VersionCommand) -> int: - exit_code = orig_version_command(command) - - command.io.write_line(f"ProjectPath: {command.poetry.pyproject_path.parent}") - command.io.write_line(f"WorkingDirectory: {os.getcwd()}") - - return exit_code - - mocker.patch("poetry.console.commands.version.VersionCommand.handle", mock_handle) - - source_dir = fixture_dir("scripts") - tester.execute(f"--directory {source_dir} version") - - output = tester.io.fetch_output() - expected = textwrap.dedent(f"""\ - scripts 0.1.0 - ProjectPath: {source_dir} - WorkingDirectory: {source_dir} - """) - - assert source_dir != Path(os.getcwd()) - assert output == expected - - -def test_version_with_directory_and_project_parameter( - fixture_dir: FixtureDirGetter, mocker: MockerFixture -) -> None: - app = Application() - tester = ApplicationTester(app) - - orig_version_command = VersionCommand.handle - - def mock_handle(command: VersionCommand) -> int: - exit_code = orig_version_command(command) - - command.io.write_line(f"ProjectPath: {command.poetry.pyproject_path.parent}") - command.io.write_line(f"WorkingDirectory: {os.getcwd()}") - - return exit_code - - mocker.patch("poetry.console.commands.version.VersionCommand.handle", mock_handle) - - source_dir = fixture_dir("scripts") - working_directory = source_dir.parent - project_path = "./scripts" - - tester.execute(f"--directory {working_directory} --project {project_path} version") - - output = tester.io.fetch_output() - - expected = textwrap.dedent(f"""\ - scripts 0.1.0 - ProjectPath: {source_dir} - WorkingDirectory: {working_directory} - """) - - assert source_dir != working_directory - assert output == expected diff --git a/tests/console/test_application_global_options.py b/tests/console/test_application_global_options.py index 3fadc8db4a1..70f9d143a91 100644 --- a/tests/console/test_application_global_options.py +++ b/tests/console/test_application_global_options.py @@ -1,5 +1,7 @@ from __future__ import annotations +import textwrap + from pathlib import Path from typing import TYPE_CHECKING @@ -8,9 +10,14 @@ from cleo.testers.application_tester import ApplicationTester from poetry.console.application import Application +from poetry.console.commands.version import VersionCommand +from tests.helpers import switch_working_directory if TYPE_CHECKING: + from pytest import TempPathFactory + from pytest_mock import MockerFixture + from tests.types import FixtureCopier @@ -27,6 +34,21 @@ def tester() -> ApplicationTester: return ApplicationTester(Application()) +@pytest.fixture +def with_mocked_version_command(mocker: MockerFixture) -> None: + orig_version_command = VersionCommand.handle + + def mock_handle(command: VersionCommand) -> int: + exit_code = orig_version_command(command) + + command.io.write_line(f"ProjectPath: {command.poetry.pyproject_path.parent}") + command.io.write_line(f"WorkingDirectory: {Path.cwd()}") + + return exit_code + + mocker.patch("poetry.console.commands.version.VersionCommand.handle", mock_handle) + + def test_application_global_option_ensure_error_when_context_invalid( tester: ApplicationTester, ) -> None: @@ -68,3 +90,62 @@ def test_application_global_option_position_does_not_matter( assert "certifi" in stdout assert len(stdout.splitlines()) == 8 + + +@pytest.mark.parametrize("parameter", ["project", "directory"]) +def test_application_with_context_parameters( + parameter: str, + tester: ApplicationTester, + project_source_directory: Path, + with_mocked_version_command: None, +) -> None: + # ensure pre-conditions are met + assert project_source_directory != Path.cwd() + + is_directory_param = parameter == "directory" + + tester.execute(f"--{parameter} {project_source_directory} version") + assert tester.io.fetch_error() == "" + assert tester.status_code == 0 + + output = tester.io.fetch_output() + assert output == textwrap.dedent(f"""\ + foobar 0.1.0 + ProjectPath: {project_source_directory} + WorkingDirectory: {project_source_directory if is_directory_param else Path.cwd()} + """) + + +def test_application_with_relative_project_parameter( + tester: ApplicationTester, + project_source_directory: Path, + with_mocked_version_command: None, + tmp_path_factory: TempPathFactory, +) -> None: + # ensure pre-conditions are met + cwd = Path.cwd() + assert project_source_directory.is_relative_to(cwd) + + # construct relative path + relative_source_directory = project_source_directory.relative_to(cwd) + assert relative_source_directory.as_posix() != project_source_directory.as_posix() + assert not relative_source_directory.is_absolute() + + # we expect application run to be executed within current cwd but project to be a subdirectory + args = f"--directory '{cwd}' --project {relative_source_directory} version" + + # we switch cwd to a new temporary directory unrelated to the project directory + new_working_dir = tmp_path_factory.mktemp("unrelated-working-directory") + with switch_working_directory(new_working_dir): + assert Path.cwd() == new_working_dir + + tester.execute(args) + assert tester.io.fetch_error() == "" + assert tester.status_code == 0 + + output = tester.io.fetch_output() + assert output == textwrap.dedent(f"""\ + foobar 0.1.0 + ProjectPath: {project_source_directory} + WorkingDirectory: {cwd} + """) From 54c37313a13bb2f450b79efe13d69f11a6ff05b9 Mon Sep 17 00:00:00 2001 From: Arun Babu Neelicattu Date: Sun, 12 Jan 2025 19:47:49 +0100 Subject: [PATCH 3/3] fix(application): check context options preflight This change ensures that context options like --project and --directory are validated prior to executing the application to prevent runtime exceptions. --- src/poetry/console/application.py | 42 +++++++++++----- src/poetry/utils/helpers.py | 12 +++++ .../test_application_global_options.py | 28 +++++++++++ tests/utils/test_helpers.py | 48 ++++++++++++++++++- 4 files changed, 115 insertions(+), 15 deletions(-) diff --git a/src/poetry/console/application.py b/src/poetry/console/application.py index 1dfe64eadd4..89f78bf0819 100644 --- a/src/poetry/console/application.py +++ b/src/poetry/console/application.py @@ -20,6 +20,7 @@ from poetry.console.command_loader import CommandLoader from poetry.console.commands.command import Command from poetry.utils.helpers import directory +from poetry.utils.helpers import ensure_path if TYPE_CHECKING: @@ -104,7 +105,7 @@ def __init__(self) -> None: self._disable_cache = False self._plugins_loaded = False self._working_directory = Path.cwd() - self._project_directory = self._working_directory + self._project_directory: Path | None = None dispatcher = EventDispatcher() dispatcher.add_listener(COMMAND, self.register_command_loggers) @@ -158,6 +159,10 @@ def _default_definition(self) -> Definition: return definition + @property + def project_directory(self) -> Path: + return self._project_directory or self._working_directory + @property def poetry(self) -> Poetry: from poetry.factory import Factory @@ -166,7 +171,7 @@ def poetry(self) -> Poetry: return self._poetry self._poetry = Factory().create_poetry( - cwd=self._project_directory, + cwd=self.project_directory, io=self._io, disable_plugins=self._disable_plugins, disable_cache=self._disable_cache, @@ -214,6 +219,12 @@ def create_io( return io def _run(self, io: IO) -> int: + # we do this here and not inside the _configure_io implementation in order + # to ensure the users are not exposed to a stack trace for providing invalid values to + # the options --directory or --project, configuring the options here allow cleo to trap and + # display the error cleanly unless the user uses verbose or debug + self._configure_custom_application_options(io) + self._load_plugins(io) with directory(self._working_directory): @@ -247,17 +258,24 @@ def _configure_custom_application_options(self, io: IO) -> None: self._disable_cache = self._option_get_value( io, "no-cache", self._disable_cache ) - self._working_directory = self._project_directory = Path( - self._option_get_value(io, "directory", Path.cwd()) - ) - self._project_directory = Path( - self._option_get_value(io, "project", self._working_directory) + # we use ensure_path for the directories to make sure these are valid paths + # this will raise an exception if the path is invalid + self._working_directory = ensure_path( + self._option_get_value(io, "directory", Path.cwd()), is_directory=True ) - if self._project_directory != self._working_directory: - with directory(self._working_directory): - self._project_directory = self._project_directory.absolute() + self._project_directory = self._option_get_value(io, "project", None) + if self._project_directory is not None: + self._project_directory = Path(self._project_directory) + self._project_directory = ensure_path( + self._project_directory + if self._project_directory.is_absolute() + else self._working_directory.joinpath(self._project_directory).resolve( + strict=False + ), + is_directory=True, + ) def _configure_io(self, io: IO) -> None: # We need to check if the command being run @@ -295,8 +313,6 @@ def _configure_io(self, io: IO) -> None: super()._configure_io(io) - self._configure_custom_application_options(io) - def register_command_loggers( self, event: Event, event_name: str, _: EventDispatcher ) -> None: @@ -418,7 +434,7 @@ def _load_plugins(self, io: IO) -> None: from poetry.plugins.application_plugin import ApplicationPlugin from poetry.plugins.plugin_manager import PluginManager - PluginManager.add_project_plugin_path(self._project_directory) + PluginManager.add_project_plugin_path(self.project_directory) manager = PluginManager(ApplicationPlugin.group) manager.load_plugins() manager.activate(self) diff --git a/src/poetry/utils/helpers.py b/src/poetry/utils/helpers.py index 365e47fef1f..c5facf140f6 100644 --- a/src/poetry/utils/helpers.py +++ b/src/poetry/utils/helpers.py @@ -258,6 +258,18 @@ def paths_csv(paths: list[Path]) -> str: return ", ".join(f'"{c!s}"' for c in paths) +def ensure_path(path: str | Path, is_directory: bool = False) -> Path: + if isinstance(path, str): + path = Path(path) + + if path.exists() and path.is_dir() == is_directory: + return path + + raise ValueError( + f"Specified path '{path}' is not a valid {'directory' if is_directory else 'file'}." + ) + + def is_dir_writable(path: Path, create: bool = False) -> bool: try: if not path.exists(): diff --git a/tests/console/test_application_global_options.py b/tests/console/test_application_global_options.py index 70f9d143a91..49ea006eedd 100644 --- a/tests/console/test_application_global_options.py +++ b/tests/console/test_application_global_options.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import textwrap from pathlib import Path @@ -92,6 +93,33 @@ def test_application_global_option_position_does_not_matter( assert len(stdout.splitlines()) == 8 +@pytest.mark.parametrize("parameter", ["-C", "--directory", "-P", "--project"]) +@pytest.mark.parametrize( + "invalid_source_directory", + [ + "/invalid/path", # non-existent path + __file__, # not a directory + ], +) +def test_application_global_option_context_is_validated( + parameter: str, + tester: ApplicationTester, + invalid_source_directory: str, +) -> None: + option = f"{parameter} '{invalid_source_directory}'" + tester.execute(f"show {option}") + assert tester.status_code != 0 + + stdout = tester.io.fetch_output() + assert stdout == "" + + stderr = tester.io.fetch_error() + assert re.match( + r"\nSpecified path '(.+)?' is not a valid directory.\n", + stderr, + ) + + @pytest.mark.parametrize("parameter", ["project", "directory"]) def test_application_with_context_parameters( parameter: str, diff --git a/tests/utils/test_helpers.py b/tests/utils/test_helpers.py index 462cfb636df..35e796328c3 100644 --- a/tests/utils/test_helpers.py +++ b/tests/utils/test_helpers.py @@ -3,6 +3,7 @@ import base64 import re +from pathlib import Path from typing import TYPE_CHECKING from typing import Any @@ -14,13 +15,12 @@ from poetry.utils.helpers import Downloader from poetry.utils.helpers import HTTPRangeRequestSupportedError from poetry.utils.helpers import download_file +from poetry.utils.helpers import ensure_path from poetry.utils.helpers import get_file_hash from poetry.utils.helpers import get_highest_priority_hash_type if TYPE_CHECKING: - from pathlib import Path - from httpretty import httpretty from httpretty.core import HTTPrettyRequest @@ -299,3 +299,47 @@ def test_downloader_uses_authenticator_by_default( request = http.last_request() basic_auth = base64.b64encode(b"bar:baz").decode() assert request.headers["Authorization"] == f"Basic {basic_auth}" + + +def test_ensure_path_converts_string(tmp_path: Path) -> None: + assert tmp_path.exists() + assert ensure_path(path=tmp_path.as_posix(), is_directory=True) == tmp_path + + +def test_ensure_path_does_not_convert_path(tmp_path: Path) -> None: + assert tmp_path.exists() + assert Path(tmp_path.as_posix()) is not tmp_path + + result = ensure_path(path=tmp_path, is_directory=True) + + assert result == tmp_path + assert result is tmp_path + + +def test_ensure_path_is_directory_parameter(tmp_path: Path) -> None: + with pytest.raises(ValueError): + ensure_path(path=tmp_path, is_directory=False) + + assert ensure_path(path=tmp_path, is_directory=True) is tmp_path + + +def test_ensure_path_file(tmp_path: Path) -> None: + path = tmp_path.joinpath("some_file.txt") + assert not path.exists() + + with pytest.raises(ValueError): + ensure_path(path=path, is_directory=False) + + path.write_text("foobar") + assert ensure_path(path=path, is_directory=False) is path + + +def test_ensure_path_directory(tmp_path: Path) -> None: + path = tmp_path.joinpath("foobar") + assert not path.exists() + + with pytest.raises(ValueError): + ensure_path(path=path, is_directory=True) + + path.mkdir() + assert ensure_path(path=path, is_directory=True) is path