From 912a9e5dc1bc15352d053fa55c02c8a90eb7b3c6 Mon Sep 17 00:00:00 2001 From: Kurt McKee Date: Mon, 25 Nov 2024 11:13:49 -0600 Subject: [PATCH 1/7] Demonstrate python/cpython#127012 This adds an in-memory finder, loader, and traversable implementation, which allows the `Traversable` protocol and concrete methods to be tested. This additional infrastructure demonstrates python/cpython#127012, but also highlights that the `Traversable.joinpath()` concrete method raises `TraversalError` which is not getting caught in several places. --- importlib_resources/tests/test_functional.py | 21 ++-- importlib_resources/tests/test_util.py | 35 +++++++ importlib_resources/tests/util.py | 105 ++++++++++++++++++- 3 files changed, 153 insertions(+), 8 deletions(-) create mode 100644 importlib_resources/tests/test_util.py diff --git a/importlib_resources/tests/test_functional.py b/importlib_resources/tests/test_functional.py index 2285389..d65a52c 100644 --- a/importlib_resources/tests/test_functional.py +++ b/importlib_resources/tests/test_functional.py @@ -7,10 +7,6 @@ from . import util from .compat.py39 import warnings_helper -# Since the functional API forwards to Traversable, we only test -# filesystem resources here -- not zip files, namespace packages etc. -# We do test for two kinds of Anchor, though. - class StringAnchorMixin: anchor01 = 'data01' @@ -27,7 +23,7 @@ def anchor02(self): return importlib.import_module('data02') -class FunctionalAPIBase(util.DiskSetup): +class FunctionalAPIBase: def setUp(self): super().setUp() self.load_fixture('data02') @@ -244,17 +240,28 @@ def test_text_errors(self): ) -class FunctionalAPITest_StringAnchor( +class FunctionalAPITest_StringAnchor_Disk( StringAnchorMixin, FunctionalAPIBase, + util.DiskSetup, unittest.TestCase, ): pass -class FunctionalAPITest_ModuleAnchor( +class FunctionalAPITest_ModuleAnchor_Disk( ModuleAnchorMixin, FunctionalAPIBase, + util.DiskSetup, + unittest.TestCase, +): + pass + + +class FunctionalAPITest_StringAnchor_Memory( + StringAnchorMixin, + FunctionalAPIBase, + util.MemorySetup, unittest.TestCase, ): pass diff --git a/importlib_resources/tests/test_util.py b/importlib_resources/tests/test_util.py new file mode 100644 index 0000000..ba3ba28 --- /dev/null +++ b/importlib_resources/tests/test_util.py @@ -0,0 +1,35 @@ +import unittest + +from .util import MemorySetup, Traversable + + +class TestMemoryTraversableImplementation(unittest.TestCase): + def test_concrete_methods_are_not_overridden(self): + """`MemoryTraversable` must not override `Traversable` concrete methods. + + This test is not an attempt to enforce a particular `Traversable` protocol; + it merely catches changes in the `Traversable` abstract/concrete methods + that have not been mirrored in the `MemoryTraversable` subclass. + """ + + traversable_concrete_methods = { + method + for method, value in Traversable.__dict__.items() + if callable(value) and method not in Traversable.__abstractmethods__ + } + memory_traversable_concrete_methods = { + method + for method, value in MemorySetup.MemoryTraversable.__dict__.items() + if callable(value) and not method.startswith("__") + } + overridden_methods = ( + memory_traversable_concrete_methods & traversable_concrete_methods + ) + + if overridden_methods: + raise AssertionError( + "MemorySetup.MemoryTraversable overrides Traversable concrete methods, " + "which may mask problems in the Traversable protocol. " + "Please remove the following methods in MemoryTraversable: " + + ", ".join(overridden_methods) + ) diff --git a/importlib_resources/tests/util.py b/importlib_resources/tests/util.py index 07d1529..2f709d3 100644 --- a/importlib_resources/tests/util.py +++ b/importlib_resources/tests/util.py @@ -1,5 +1,6 @@ import abc import contextlib +import functools import importlib import io import pathlib @@ -7,7 +8,7 @@ import types from importlib.machinery import ModuleSpec -from ..abc import ResourceReader +from ..abc import ResourceReader, Traversable, TraversableResources from . import _path from . import zip as zip_ from .compat.py39 import import_helper, os_helper @@ -200,5 +201,107 @@ def tree_on_path(self, spec): self.fixtures.enter_context(import_helper.DirsOnSysPath(temp_dir)) +class MemorySetup(ModuleSetup): + """Support loading a module in memory.""" + + MODULE = 'data01' + + def load_fixture(self, module): + self.fixtures.enter_context(self.augment_sys_metapath(module)) + return importlib.import_module(module) + + @contextlib.contextmanager + def augment_sys_metapath(self, module): + finder_instance = self.MemoryFinder(module) + sys.meta_path.append(finder_instance) + yield + sys.meta_path.remove(finder_instance) + + class MemoryFinder(importlib.abc.MetaPathFinder): + def __init__(self, module): + self._module = module + + def find_spec(self, fullname, path, target=None): + if fullname != self._module: + return None + + return importlib.machinery.ModuleSpec( + name=fullname, + loader=MemorySetup.MemoryLoader(self._module), + is_package=True, + ) + + class MemoryLoader(importlib.abc.Loader): + def __init__(self, module): + self._module = module + + def exec_module(self, module): + pass + + def get_resource_reader(self, fullname): + return MemorySetup.MemoryTraversableResources(self._module, fullname) + + class MemoryTraversableResources(TraversableResources): + def __init__(self, module, fullname): + self._module = module + self._fullname = fullname + + def files(self): + return MemorySetup.MemoryTraversable(self._module, self._fullname) + + class MemoryTraversable(Traversable): + """Implement only the abstract methods of `Traversable`. + + Besides `.__init__()`, no other methods may be implemented or overridden. + This is critical for validating the concrete `Traversable` implementations. + """ + + def __init__(self, module, fullname): + self._module = module + self._fullname = fullname + + def iterdir(self): + path = pathlib.PurePosixPath(self._fullname) + directory = functools.reduce(lambda d, p: d[p], path.parts, fixtures) + if not isinstance(directory, dict): + # Filesystem openers raise OSError, and that exception is mirrored here. + raise OSError(f"{self._fullname} is not a directory") + for path in directory: + yield MemorySetup.MemoryTraversable( + self._module, f"{self._fullname}/{path}" + ) + + def is_dir(self) -> bool: + path = pathlib.PurePosixPath(self._fullname) + # Fully traverse the `fixtures` dictionary. + # This should be wrapped in a `try/except KeyError` + # but it is not currently needed, and lowers the code coverage numbers. + directory = functools.reduce(lambda d, p: d[p], path.parts, fixtures) + return isinstance(directory, dict) + + def is_file(self) -> bool: + path = pathlib.PurePosixPath(self._fullname) + directory = functools.reduce(lambda d, p: d[p], path.parts, fixtures) + return not isinstance(directory, dict) + + def open(self, mode='r', encoding=None, errors=None, *_, **__): + path = pathlib.PurePosixPath(self._fullname) + contents = functools.reduce(lambda d, p: d[p], path.parts, fixtures) + if isinstance(contents, dict): + # Filesystem openers raise OSError when attempting to open a directory, + # and that exception is mirrored here. + raise OSError(f"{self._fullname} is a directory") + if isinstance(contents, str): + contents = contents.encode("utf-8") + result = io.BytesIO(contents) + if "b" in mode: + return result + return io.TextIOWrapper(result, encoding=encoding, errors=errors) + + @property + def name(self): + return pathlib.PurePosixPath(self._fullname).name + + class CommonTests(DiskSetup, CommonTestsBase): pass From f10a2e9b2608cdf5006608dd7f2b29d3f0b1cbd2 Mon Sep 17 00:00:00 2001 From: Kurt McKee Date: Mon, 25 Nov 2024 11:22:52 -0600 Subject: [PATCH 2/7] Catch `TraversalError`, raised by `Traversable.joinpath()` Exercising the `Traversable` protocol's concrete methods has highlighted that `.joinpath()` raises `TraversalError`, which needs to be caught in several places. This is primarily resolved within the test suite, but implicates the `is_resource()` function as well. --- importlib_resources/_functional.py | 6 +++++- importlib_resources/tests/test_functional.py | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/importlib_resources/_functional.py b/importlib_resources/_functional.py index 5a3ca0a..b08a5c6 100644 --- a/importlib_resources/_functional.py +++ b/importlib_resources/_functional.py @@ -3,6 +3,7 @@ import warnings from ._common import as_file, files +from .abc import TraversalError _MISSING = object() @@ -41,7 +42,10 @@ def is_resource(anchor, *path_names): Otherwise returns ``False``. """ - return _get_resource(anchor, path_names).is_file() + try: + return _get_resource(anchor, path_names).is_file() + except TraversalError: + return False def contents(anchor, *path_names): diff --git a/importlib_resources/tests/test_functional.py b/importlib_resources/tests/test_functional.py index d65a52c..9eb2d81 100644 --- a/importlib_resources/tests/test_functional.py +++ b/importlib_resources/tests/test_functional.py @@ -72,7 +72,7 @@ def test_read_text(self): # fail with PermissionError rather than IsADirectoryError with self.assertRaises(OSError): resources.read_text(self.anchor01) - with self.assertRaises(OSError): + with self.assertRaises((OSError, resources.abc.TraversalError)): resources.read_text(self.anchor01, 'no-such-file') with self.assertRaises(UnicodeDecodeError): resources.read_text(self.anchor01, 'utf-16.file') @@ -120,7 +120,7 @@ def test_open_text(self): # fail with PermissionError rather than IsADirectoryError with self.assertRaises(OSError): resources.open_text(self.anchor01) - with self.assertRaises(OSError): + with self.assertRaises((OSError, resources.abc.TraversalError)): resources.open_text(self.anchor01, 'no-such-file') with resources.open_text(self.anchor01, 'utf-16.file') as f: with self.assertRaises(UnicodeDecodeError): @@ -188,7 +188,7 @@ def test_contents(self): for path_parts in self._gen_resourcetxt_path_parts(): with ( - self.assertRaises(OSError), + self.assertRaises((OSError, resources.abc.TraversalError)), warnings_helper.check_warnings(( ".*contents.*", DeprecationWarning, From d001110ce83b32ad5fa7bae3932a8db1faccbb8b Mon Sep 17 00:00:00 2001 From: Kurt McKee Date: Mon, 25 Nov 2024 11:29:05 -0600 Subject: [PATCH 3/7] Resolve a `TypeError` lurking in the `read_text()` functional API `importlib_resources.read_text()` calls the `Traversable.read_text()` concrete method with an `errors` argument that doesn't exist in the method signature, resulting in an `TypeError`. This is resolved by adding an `errors` parameter to `Traversable.read_text()`. Fixes python/cpython#127012 --- importlib_resources/abc.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/importlib_resources/abc.py b/importlib_resources/abc.py index 84631c7..befe799 100644 --- a/importlib_resources/abc.py +++ b/importlib_resources/abc.py @@ -93,11 +93,13 @@ def read_bytes(self) -> bytes: with self.open('rb') as strm: return strm.read() - def read_text(self, encoding: Optional[str] = None) -> str: + def read_text( + self, encoding: Optional[str] = None, errors: Optional[str] = None + ) -> str: """ Read contents of self as text """ - with self.open(encoding=encoding) as strm: + with self.open(encoding=encoding, errors=errors) as strm: return strm.read() @abc.abstractmethod From 72d550dfabc4a56fa22ec1e347b2672d546a39a4 Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 3 Jan 2025 11:33:06 -0500 Subject: [PATCH 4/7] Consolidate MemoryTraversable._resolve. --- importlib_resources/tests/util.py | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/importlib_resources/tests/util.py b/importlib_resources/tests/util.py index 2f709d3..0340c15 100644 --- a/importlib_resources/tests/util.py +++ b/importlib_resources/tests/util.py @@ -260,9 +260,18 @@ def __init__(self, module, fullname): self._module = module self._fullname = fullname - def iterdir(self): + def _resolve(self): + """ + Fully traverse the `fixtures` dictionary. + + This should be wrapped in a `try/except KeyError` + but it is not currently needed and lowers the code coverage numbers. + """ path = pathlib.PurePosixPath(self._fullname) - directory = functools.reduce(lambda d, p: d[p], path.parts, fixtures) + return functools.reduce(lambda d, p: d[p], path.parts, fixtures) + + def iterdir(self): + directory = self._resolve() if not isinstance(directory, dict): # Filesystem openers raise OSError, and that exception is mirrored here. raise OSError(f"{self._fullname} is not a directory") @@ -272,21 +281,13 @@ def iterdir(self): ) def is_dir(self) -> bool: - path = pathlib.PurePosixPath(self._fullname) - # Fully traverse the `fixtures` dictionary. - # This should be wrapped in a `try/except KeyError` - # but it is not currently needed, and lowers the code coverage numbers. - directory = functools.reduce(lambda d, p: d[p], path.parts, fixtures) - return isinstance(directory, dict) + return isinstance(self._resolve(), dict) def is_file(self) -> bool: - path = pathlib.PurePosixPath(self._fullname) - directory = functools.reduce(lambda d, p: d[p], path.parts, fixtures) - return not isinstance(directory, dict) + return not self.is_dir() def open(self, mode='r', encoding=None, errors=None, *_, **__): - path = pathlib.PurePosixPath(self._fullname) - contents = functools.reduce(lambda d, p: d[p], path.parts, fixtures) + contents = self._resolve() if isinstance(contents, dict): # Filesystem openers raise OSError when attempting to open a directory, # and that exception is mirrored here. From cf269ce50f496671f3b7fbc5e6292946ecc70e7d Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 3 Jan 2025 11:42:13 -0500 Subject: [PATCH 5/7] Replace unreachable block with simple assertion. Fixes diffcov failure. --- importlib_resources/tests/test_util.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/importlib_resources/tests/test_util.py b/importlib_resources/tests/test_util.py index ba3ba28..de304b6 100644 --- a/importlib_resources/tests/test_util.py +++ b/importlib_resources/tests/test_util.py @@ -26,10 +26,4 @@ def test_concrete_methods_are_not_overridden(self): memory_traversable_concrete_methods & traversable_concrete_methods ) - if overridden_methods: - raise AssertionError( - "MemorySetup.MemoryTraversable overrides Traversable concrete methods, " - "which may mask problems in the Traversable protocol. " - "Please remove the following methods in MemoryTraversable: " - + ", ".join(overridden_methods) - ) + assert not overridden_methods From 9a872e5dbceff32260e8ff19d039236304ee150c Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 3 Jan 2025 11:46:18 -0500 Subject: [PATCH 6/7] Add news fragment. --- newsfragments/321.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/321.bugfix.rst diff --git a/newsfragments/321.bugfix.rst b/newsfragments/321.bugfix.rst new file mode 100644 index 0000000..788cd32 --- /dev/null +++ b/newsfragments/321.bugfix.rst @@ -0,0 +1 @@ +Updated ``Traversable.read_text()`` to reflect the ``errors`` parameter (python/cpython#127012). From 78c4bda73c5d671cbbcfdf1430b6f2da03aeb04f Mon Sep 17 00:00:00 2001 From: "Jason R. Coombs" Date: Fri, 3 Jan 2025 11:54:59 -0500 Subject: [PATCH 7/7] Finalize --- NEWS.rst | 9 +++++++++ newsfragments/321.bugfix.rst | 1 - 2 files changed, 9 insertions(+), 1 deletion(-) delete mode 100644 newsfragments/321.bugfix.rst diff --git a/NEWS.rst b/NEWS.rst index c3612c1..48899f0 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -1,3 +1,12 @@ +v6.5.1 +====== + +Bugfixes +-------- + +- Updated ``Traversable.read_text()`` to reflect the ``errors`` parameter (python/cpython#127012). (#321) + + v6.5.0 ====== diff --git a/newsfragments/321.bugfix.rst b/newsfragments/321.bugfix.rst deleted file mode 100644 index 788cd32..0000000 --- a/newsfragments/321.bugfix.rst +++ /dev/null @@ -1 +0,0 @@ -Updated ``Traversable.read_text()`` to reflect the ``errors`` parameter (python/cpython#127012).