performance: cache validation result to avoid unnecessary file system access

Author: radoering

src/poetry/core/packages/directory_dependency.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import functools
-import logging
 
 from typing import TYPE_CHECKING
 from typing import Iterable
@@ -14,8 +13,6 @@
 if TYPE_CHECKING:
     from pathlib import Path
 
-logger = logging.getLogger(__name__)
-
 
 class DirectoryDependency(PathDependency):
     def __init__(
@@ -46,28 +43,22 @@ def __init__(
     def develop(self) -> bool:
         return self._develop
 
-    def validate(self, *, raise_error: bool) -> bool:
-        if not super().validate(raise_error=raise_error):
-            return False
+    def _validate(self) -> str:
+        message = super()._validate()
+        if message:
+            return message
 
-        message = ""
         if self._full_path.is_file():
-            message = (
+            return (
                 f"{self._full_path} for {self.pretty_name} is a file,"
                 " expected a directory"
             )
-        elif not is_python_project(self._full_path):
-            message = (
+        if not is_python_project(self._full_path):
+            return (
                 f"Directory {self._full_path} for {self.pretty_name} does not seem"
                 " to be a Python package"
             )
-
-        if message:
-            if raise_error:
-                raise ValueError(message)
-            logger.warning(message)
-            return False
-        return True
+        return ""
 
     def _supports_poetry(self) -> bool:
         return PyProjectTOML(self._full_path / "pyproject.toml").is_poetry_project()

src/poetry/core/packages/file_dependency.py

@@ -2,7 +2,6 @@
 
 import hashlib
 import io
-import logging
 import warnings
 
 from typing import TYPE_CHECKING
@@ -14,8 +13,6 @@
 if TYPE_CHECKING:
     from pathlib import Path
 
-logger = logging.getLogger(__name__)
-
 
 class FileDependency(PathDependency):
     def __init__(
@@ -37,20 +34,17 @@ def __init__(
             extras=extras,
         )
 
-    def validate(self, *, raise_error: bool) -> bool:
-        if not super().validate(raise_error=raise_error):
-            return False
+    def _validate(self) -> str:
+        message = super()._validate()
+        if message:
+            return message
 
         if self._full_path.is_dir():
-            message = (
+            return (
                 f"{self._full_path} for {self.pretty_name} is a directory,"
                 " expected a file"
             )
-            if raise_error:
-                raise ValueError(message)
-            logger.warning(message)
-            return False
-        return True
+        return ""
 
     def hash(self, hash_name: str = "sha256") -> str:
         warnings.warn(

src/poetry/core/packages/path_dependency.py

@@ -45,6 +45,8 @@ def __init__(
             source_url=self._full_path.as_posix(),
             extras=extras,
         )
+        # cache validation result to avoid unnecessary file system access
+        self._validation_error = self._validate()
         self.validate(raise_error=False)
 
     @property
@@ -66,13 +68,12 @@ def is_directory(self) -> bool:
         return self._source_type == "directory"
 
     def validate(self, *, raise_error: bool) -> bool:
-        if not self._full_path.exists():
-            message = f"Path {self._full_path} for {self.pretty_name} does not exist"
-            if raise_error:
-                raise ValueError(message)
-            logger.warning(message)
-            return False
-        return True
+        if not self._validation_error:
+            return True
+        if raise_error:
+            raise ValueError(self._validation_error)
+        logger.warning(self._validation_error)
+        return False
 
     @property
     def base_pep_508_name(self) -> str:
@@ -86,3 +87,8 @@ def base_pep_508_name(self) -> str:
         requirement += f" @ {path}"
 
         return requirement
+
+    def _validate(self) -> str:
+        if not self._full_path.exists():
+            return f"Path {self._full_path} for {self.pretty_name} does not exist"
+        return ""

tests/packages/test_directory_dependency.py

@@ -12,12 +12,18 @@
 
 if TYPE_CHECKING:
     from _pytest.logging import LogCaptureFixture
+    from pytest_mock import MockerFixture
+
 
 DIST_PATH = Path(__file__).parent.parent / "fixtures" / "distributions"
 SAMPLE_PROJECT = Path(__file__).parent.parent / "fixtures" / "sample_project"
 
 
-def test_directory_dependency_does_not_exist(caplog: LogCaptureFixture) -> None:
+def test_directory_dependency_does_not_exist(
+    caplog: LogCaptureFixture, mocker: MockerFixture
+) -> None:
+    mock_exists = mocker.patch.object(Path, "exists")
+    mock_exists.return_value = False
     dep = DirectoryDependency("demo", DIST_PATH / "invalid")
     assert len(caplog.records) == 1
     record = caplog.records[0]
@@ -27,8 +33,14 @@ def test_directory_dependency_does_not_exist(caplog: LogCaptureFixture) -> None:
     with pytest.raises(ValueError, match="does not exist"):
         dep.validate(raise_error=True)
 
+    mock_exists.assert_called_once()
+
 
-def test_directory_dependency_is_file(caplog: LogCaptureFixture) -> None:
+def test_directory_dependency_is_file(
+    caplog: LogCaptureFixture, mocker: MockerFixture
+) -> None:
+    mock_is_file = mocker.patch.object(Path, "is_file")
+    mock_is_file.return_value = True
     dep = DirectoryDependency("demo", DIST_PATH / "demo-0.1.0.tar.gz")
     assert len(caplog.records) == 1
     record = caplog.records[0]
@@ -38,6 +50,8 @@ def test_directory_dependency_is_file(caplog: LogCaptureFixture) -> None:
     with pytest.raises(ValueError, match="is a file"):
         dep.validate(raise_error=True)
 
+    mock_is_file.assert_called_once()
+
 
 def test_directory_dependency_is_not_a_python_project(
     caplog: LogCaptureFixture,

tests/packages/test_file_dependency.py

@@ -21,7 +21,11 @@
 TEST_FILE = "demo-0.1.0.tar.gz"
 
 
-def test_file_dependency_does_not_exist(caplog: LogCaptureFixture) -> None:
+def test_file_dependency_does_not_exist(
+    caplog: LogCaptureFixture, mocker: MockerFixture
+) -> None:
+    mock_exists = mocker.patch.object(Path, "exists")
+    mock_exists.return_value = False
     dep = FileDependency("demo", DIST_PATH / "demo-0.2.0.tar.gz")
     assert len(caplog.records) == 1
     record = caplog.records[0]
@@ -31,8 +35,14 @@ def test_file_dependency_does_not_exist(caplog: LogCaptureFixture) -> None:
     with pytest.raises(ValueError, match="does not exist"):
         dep.validate(raise_error=True)
 
+    mock_exists.assert_called_once()
+
 
-def test_file_dependency_is_directory(caplog: LogCaptureFixture) -> None:
+def test_file_dependency_is_directory(
+    caplog: LogCaptureFixture, mocker: MockerFixture
+) -> None:
+    mock_is_directory = mocker.patch.object(Path, "is_dir")
+    mock_is_directory.return_value = True
     dep = FileDependency("demo", DIST_PATH)
     assert len(caplog.records) == 1
     record = caplog.records[0]
@@ -42,6 +52,8 @@ def test_file_dependency_is_directory(caplog: LogCaptureFixture) -> None:
     with pytest.raises(ValueError, match="is a directory"):
         dep.validate(raise_error=True)
 
+    mock_is_directory.assert_called_once()
+
 
 def test_default_hash() -> None:
     with pytest.warns(DeprecationWarning):