Merge pull request #553 from python/bugfix/534-entry-points-prefer-valid

Prefer valid distributions when collecting entry points

Author: jaraco

importlib_metadata/__init__.py

@@ -27,15 +27,14 @@
 from importlib import import_module
 from importlib.abc import MetaPathFinder
 from itertools import starmap
-from typing import Any
+from typing import Any, TypeVar
 
 from ._collections import FreezableDefaultDict, Pair
 from ._compat import (
     NullFinder,
     install,
     localize,
 )
-from ._context import ExceptionTrap
 from ._functools import apply, compose, method_cache, noop, pass_none, passthrough
 from ._itertools import always_iterable, bucket, unique_everseen
 from ._meta import (
@@ -67,6 +66,9 @@
 ]
 
 
+_D = TypeVar('_D', bound=IDistribution)
+
+
 class PackageNotFoundError(ModuleNotFoundError):
     """The package was not found."""
 
@@ -520,18 +522,14 @@ def discover(
         )
 
     @staticmethod
-    def _prefer_valid(dists: Iterable[Distribution]) -> Iterable[Distribution]:
+    def _prefer_valid(dists: Iterable[_D]) -> Iterable[_D]:
         """
         Prefer (move to the front) distributions that have metadata.
 
         Ref python/importlib_resources#489.
         """
 
-        has_metadata = ExceptionTrap(MetadataNotFound).passes(
-            operator.attrgetter('metadata')
-        )
-
-        buckets = bucket(dists, has_metadata)
+        buckets = bucket(dists, _safe_has_metadata)
         return itertools.chain(buckets[True], buckets[False])
 
     @staticmethod
@@ -566,15 +564,37 @@ def metadata(self) -> PackageMetadata:
         :raises MetadataNotFound: If no metadata file is present.
         """
 
-        text = (
+        text = self._metadata_text()
+        return self._assemble_message(self._ensure_metadata_present(text))
+
+    def _metadata_text(self) -> str | None:
+        """
+        The metadata text, if any.
+
+        Separate from ``metadata`` so that ``_has_metadata`` can name
+        the same files without repeating them and without paying to
+        parse what it finds.
+        """
+        return (
             self.read_text('METADATA')
             or self.read_text('PKG-INFO')
             # This last clause is here to support old egg-info files.  Its
             # effect is to just end up using the PathDistribution's self._path
             # (which points to the egg-info file) attribute unchanged.
             or self.read_text('')
         )
-        return self._assemble_message(self._ensure_metadata_present(text))
+
+    def _has_metadata(self) -> bool:
+        """
+        Whether this distribution supplies metadata.
+
+        Reads the metadata, as the interface offers nothing cheaper:
+        ``locate_file`` resolves against the package root and not the
+        metadata, and every other member loads the metadata or parses
+        it. A subclass able to detect the metadata without reading it
+        should override this (python/importlib_metadata#534).
+        """
+        return bool(self._metadata_text())
 
     @staticmethod
     def _assemble_message(text: str) -> PackageMetadata:
@@ -1078,6 +1098,24 @@ def read_text(self, filename: str | os.PathLike[str]) -> str | None:
 
     read_text.__doc__ = Distribution.read_text.__doc__
 
+    def _has_metadata(self) -> bool:
+        """
+        Detect the metadata file without opening it
+        (python/importlib_metadata#534).
+
+        The last clause is for an old-style egg-info file, whose
+        metadata is the distribution's own path; a metadata directory
+        is not itself metadata. ``SimplePath`` doesn't name
+        ``is_file``, but only these legacy paths reach that clause and
+        they arrive as a ``pathlib.Path`` or a ``zipfile.Path``, both
+        of which supply it.
+        """
+        return (
+            self._path.joinpath('METADATA').exists()
+            or self._path.joinpath('PKG-INFO').exists()
+            or self._path.is_file()  # type: ignore[attr-defined]
+        )
+
     def locate_file(self, path: str | os.PathLike[str]) -> SimplePath:
         return self._path.parent / path
 
@@ -1148,6 +1186,25 @@ def version(distribution_name: str) -> str:
     return distribution(distribution_name).version
 
 
+def _safe_has_metadata(dist: IDistribution) -> bool:
+    """
+    ``dist._has_metadata()``, for a distribution that may not supply it.
+
+    A provider's own ``Distribution`` subclass derives from
+    ``importlib.metadata`` and is passed through unadapted (#486), so
+    it has none; read its metadata as the default would. It supplies
+    no ``_metadata_text`` either, hence the unbound call.
+
+    Such a distribution cannot be asked for ``metadata`` behind a trap
+    instead. The ``MetadataNotFound`` it raises is the one from
+    ``importlib.metadata``, a different class from this package's,
+    which no trap of ours would catch.
+    """
+    with suppress(AttributeError):
+        return dist._has_metadata()  # type: ignore[attr-defined]
+    return bool(Distribution._metadata_text(dist))  # type: ignore[arg-type]
+
+
 _unique = functools.partial(
     unique_everseen,
     key=operator.attrgetter('_normalized_name'),
@@ -1167,7 +1224,8 @@ def entry_points(**params) -> EntryPoints:
     :return: EntryPoints for all installed packages.
     """
     eps = itertools.chain.from_iterable(
-        dist.entry_points for dist in _unique(distributions())
+        dist.entry_points
+        for dist in _unique(Distribution._prefer_valid(distributions()))
     )
     return EntryPoints(eps).select(**params)
 

newsfragments/+foreign-metadata-not-found.bugfix.rst

@@ -0,0 +1,5 @@
+The preference for valid distributions no longer propagates the
+``MetadataNotFound`` of a distribution that a provider derived from
+``importlib.metadata``, whose exception is a different class from this
+package's and so escaped the preference rather than marking the distribution
+invalid.

newsfragments/+has-metadata.feature.rst

@@ -0,0 +1,4 @@
+Detecting whether a distribution supplies metadata no longer reads or parses
+that metadata where the metadata directory can be inspected instead, so
+retrieving a distribution by name, which prefers the valid candidates, is
+substantially faster.

newsfragments/534.bugfix.rst

@@ -0,0 +1,5 @@
+``entry_points()`` now prefers distributions whose metadata is readable, so
+an empty metadata directory left behind by an unclean upgrade no longer
+shadows the valid one of the same name and suppresses its entry points.
+Invalid distributions are deferred to the end of the enumeration rather than
+dropped.

tests/compat/test_localize.py

@@ -119,6 +119,24 @@ def test_conforming_provider_is_not_warned_about(self):
             assert distribution('distinfo-pkg').version == '1.0.0'
         assert not [str(warning.message) for warning in caught]
 
+    def test_dist_without_metadata_is_deferred(self):
+        """
+        A provider's own distribution supplies no detection of its own,
+        so the metadata is sought through ``read_text``. Asking for
+        ``metadata`` instead would raise, as the ``MetadataNotFound``
+        of a distribution deriving from ``importlib.metadata`` is a
+        different class.
+        """
+
+        class Empty(stdlib.Distribution):
+            def read_text(self, filename):
+                return None
+
+            def locate_file(self, path):
+                return None
+
+        assert not importlib_metadata._safe_has_metadata(Empty())
+
 
 class ConformanceCheckTests(unittest.TestCase):
     @unittest.skipIf(

tests/test_main.py

@@ -56,6 +56,25 @@ def test_abc_enforced(self):
         with self.assertRaises(TypeError):
             type('DistributionSubclass', (Distribution,), {})()
 
+    @fixtures.parameterize(
+        dict(text='Name: foo\nVersion: 1.0\n', expected=True),
+        dict(text=None, expected=False),
+    )
+    def test_detects_metadata_by_reading(self, text, expected):
+        """
+        A distribution that isn't path-based takes the default
+        detection, which reads what it cannot otherwise detect.
+        """
+
+        class Provided(Distribution):
+            def read_text(self, filename):
+                return text if filename == 'METADATA' else None
+
+            def locate_file(self, path):
+                return None  # pragma: no cover
+
+        assert Provided()._has_metadata() is expected
+
     @fixtures.parameterize(
         dict(name=None),
         dict(name=''),
@@ -160,6 +179,40 @@ def test_valid_dists_preferred(self):
         dist = Distribution.from_name('foo')
         assert dist.version == "1.0"
 
+    def test_invalid_dists_deferred(self):
+        """
+        Preferring the valid dists defers the invalid ones to the end
+        rather than dropping them.
+        """
+        fixtures.build_files(self.make_pkg('foo-4.0', files={}), self.site_dir)
+        fixtures.build_files(self.make_pkg('foo-4.1'), self.site_dir)
+        dists = list(Distribution._prefer_valid(Distribution.discover(name='foo')))
+        assert len(dists) == 2
+        assert dists[0].version == "1.0"
+
+    def test_entry_points_from_valid_dist(self):
+        """
+        Entry points come from the valid dist even when an invalid one
+        of the same name is discovered first.
+
+        Ref python/importlib_metadata#534.
+        """
+        # as in test_valid_dists_preferred, bracket the valid dist so that
+        # on most file systems it is never naturally first.
+        fixtures.build_files(self.make_pkg('foo-4.0', files={}), self.site_dir)
+        fixtures.build_files(
+            self.make_pkg(
+                'foo-4.1',
+                files={
+                    'METADATA': "VERSION: 1.0",
+                    'entry_points.txt': '[shadowed]\nep = foo:main\n',
+                },
+            ),
+            self.site_dir,
+        )
+        fixtures.build_files(self.make_pkg('foo-4.2', files={}), self.site_dir)
+        assert entry_points(group='shadowed').names == {'ep'}
+
     def test_missing_metadata(self):
         """
         Dists with a missing metadata file should raise ``MetadataNotFound``.