Merge pull request #6542 from cjerdonek/issue-5082-missing-metadata-error

Improve error message if METADATA or PKG-INFO metadata is None

Author: cjerdonek

news/5082.feature

@@ -0,0 +1,2 @@
+Improve the error message when ``METADATA`` or ``PKG-INFO`` is None when
+accessing metadata.

src/pip/_internal/exceptions.py

@@ -9,6 +9,7 @@
 
 if MYPY_CHECK_RUNNING:
     from typing import Optional
+    from pip._vendor.pkg_resources import Distribution
     from pip._internal.req.req_install import InstallRequirement
 
 
@@ -28,6 +29,36 @@ class UninstallationError(PipError):
     """General exception during uninstallation"""
 
 
+class NoneMetadataError(PipError):
+    """
+    Raised when accessing "METADATA" or "PKG-INFO" metadata for a
+    pip._vendor.pkg_resources.Distribution object and
+    `dist.has_metadata('METADATA')` returns True but
+    `dist.get_metadata('METADATA')` returns None (and similarly for
+    "PKG-INFO").
+    """
+
+    def __init__(self, dist, metadata_name):
+        # type: (Distribution, str) -> None
+        """
+        :param dist: A Distribution object.
+        :param metadata_name: The name of the metadata being accessed
+            (can be "METADATA" or "PKG-INFO").
+        """
+        self.dist = dist
+        self.metadata_name = metadata_name
+
+    def __str__(self):
+        # type: () -> str
+        # Use `dist` in the error message because its stringification
+        # includes more information, like the version and location.
+        return (
+            'None {} metadata found for distribution: {}'.format(
+                self.metadata_name, self.dist,
+            )
+        )
+
+
 class DistributionNotFound(InstallationError):
     """Raised when a distribution cannot be found to satisfy a requirement"""
 

src/pip/_internal/utils/packaging.py

@@ -6,6 +6,7 @@
 from pip._vendor import pkg_resources
 from pip._vendor.packaging import specifiers, version
 
+from pip._internal.exceptions import NoneMetadataError
 from pip._internal.utils.misc import display_path
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 
@@ -43,16 +44,27 @@ def check_requires_python(requires_python, version_info):
 
 def get_metadata(dist):
     # type: (Distribution) -> Message
+    """
+    :raises NoneMetadataError: if the distribution reports `has_metadata()`
+        True but `get_metadata()` returns None.
+    """
+    metadata_name = 'METADATA'
     if (isinstance(dist, pkg_resources.DistInfoDistribution) and
-            dist.has_metadata('METADATA')):
-        metadata = dist.get_metadata('METADATA')
+            dist.has_metadata(metadata_name)):
+        metadata = dist.get_metadata(metadata_name)
     elif dist.has_metadata('PKG-INFO'):
-        metadata = dist.get_metadata('PKG-INFO')
+        metadata_name = 'PKG-INFO'
+        metadata = dist.get_metadata(metadata_name)
     else:
         logger.warning("No metadata found in %s", display_path(dist.location))
         metadata = ''
 
+    if metadata is None:
+        raise NoneMetadataError(dist, metadata_name)
+
     feed_parser = FeedParser()
+    # The following line errors out if with a "NoneType" TypeError if
+    # passed metadata=None.
     feed_parser.feed(metadata)
     return feed_parser.close()
 

tests/unit/test_legacy_resolve.py

@@ -1,58 +1,77 @@
 import logging
 
 import pytest
-from mock import patch
+from pip._vendor import pkg_resources
 
-from pip._internal.exceptions import UnsupportedPythonVersion
+from pip._internal.exceptions import (
+    NoneMetadataError, UnsupportedPythonVersion,
+)
 from pip._internal.legacy_resolve import _check_dist_requires_python
+from pip._internal.utils.packaging import get_requires_python
 
 
-class FakeDist(object):
+# We need to inherit from DistInfoDistribution for the `isinstance()`
+# check inside `packaging.get_metadata()` to work.
+class FakeDist(pkg_resources.DistInfoDistribution):
 
-    def __init__(self, project_name):
-        self.project_name = project_name
+    def __init__(self, metadata, metadata_name=None):
+        """
+        :param metadata: The value that dist.get_metadata() should return
+            for the `metadata_name` metadata.
+        :param metadata_name: The name of the metadata to store
+            (can be "METADATA" or "PKG-INFO").  Defaults to "METADATA".
+        """
+        if metadata_name is None:
+            metadata_name = 'METADATA'
 
+        self.project_name = 'my-project'
+        self.metadata_name = metadata_name
+        self.metadata = metadata
 
-@pytest.fixture
-def dist():
-    return FakeDist('my-project')
+    def __str__(self):
+        return '<distribution {!r}>'.format(self.project_name)
+
+    def has_metadata(self, name):
+        return (name == self.metadata_name)
+
+    def get_metadata(self, name):
+        assert name == self.metadata_name
+        return self.metadata
+
+
+def make_fake_dist(requires_python=None, metadata_name=None):
+    metadata = 'Name: test\n'
+    if requires_python is not None:
+        metadata += 'Requires-Python:{}'.format(requires_python)
+
+    return FakeDist(metadata, metadata_name=metadata_name)
 
 
-@patch('pip._internal.legacy_resolve.get_requires_python')
 class TestCheckDistRequiresPython(object):
 
     """
     Test _check_dist_requires_python().
     """
 
-    def test_compatible(self, mock_get_requires, caplog, dist):
+    def test_compatible(self, caplog):
+        """
+        Test a Python version compatible with the dist's Requires-Python.
+        """
         caplog.set_level(logging.DEBUG)
-        mock_get_requires.return_value = '== 3.6.5'
-        _check_dist_requires_python(
-            dist,
-            version_info=(3, 6, 5),
-            ignore_requires_python=False,
-        )
-        assert not len(caplog.records)
+        dist = make_fake_dist('== 3.6.5')
 
-    def test_invalid_specifier(self, mock_get_requires, caplog, dist):
-        caplog.set_level(logging.DEBUG)
-        mock_get_requires.return_value = 'invalid'
         _check_dist_requires_python(
             dist,
             version_info=(3, 6, 5),
             ignore_requires_python=False,
         )
-        assert len(caplog.records) == 1
-        record = caplog.records[0]
-        assert record.levelname == 'WARNING'
-        assert record.message == (
-            "Package 'my-project' has an invalid Requires-Python: "
-            "Invalid specifier: 'invalid'"
-        )
+        assert not len(caplog.records)
 
-    def test_incompatible(self, mock_get_requires, dist):
-        mock_get_requires.return_value = '== 3.6.4'
+    def test_incompatible(self):
+        """
+        Test a Python version incompatible with the dist's Requires-Python.
+        """
+        dist = make_fake_dist('== 3.6.4')
         with pytest.raises(UnsupportedPythonVersion) as exc:
             _check_dist_requires_python(
                 dist,
@@ -64,11 +83,13 @@ def test_incompatible(self, mock_get_requires, dist):
             "3.6.5 not in '== 3.6.4'"
         )
 
-    def test_incompatible_with_ignore_requires(
-        self, mock_get_requires, caplog, dist,
-    ):
+    def test_incompatible_with_ignore_requires(self, caplog):
+        """
+        Test a Python version incompatible with the dist's Requires-Python
+        while passing ignore_requires_python=True.
+        """
         caplog.set_level(logging.DEBUG)
-        mock_get_requires.return_value = '== 3.6.4'
+        dist = make_fake_dist('== 3.6.4')
         _check_dist_requires_python(
             dist,
             version_info=(3, 6, 5),
@@ -81,3 +102,67 @@ def test_incompatible_with_ignore_requires(
             "Ignoring failed Requires-Python check for package 'my-project': "
             "3.6.5 not in '== 3.6.4'"
         )
+
+    def test_none_requires_python(self, caplog):
+        """
+        Test a dist with Requires-Python None.
+        """
+        caplog.set_level(logging.DEBUG)
+        dist = make_fake_dist()
+        # Make sure our test setup is correct.
+        assert get_requires_python(dist) is None
+        assert len(caplog.records) == 0
+
+        # Then there is no exception and no log message.
+        _check_dist_requires_python(
+            dist,
+            version_info=(3, 6, 5),
+            ignore_requires_python=False,
+        )
+        assert len(caplog.records) == 0
+
+    def test_invalid_requires_python(self, caplog):
+        """
+        Test a dist with an invalid Requires-Python.
+        """
+        caplog.set_level(logging.DEBUG)
+        dist = make_fake_dist('invalid')
+        _check_dist_requires_python(
+            dist,
+            version_info=(3, 6, 5),
+            ignore_requires_python=False,
+        )
+        assert len(caplog.records) == 1
+        record = caplog.records[0]
+        assert record.levelname == 'WARNING'
+        assert record.message == (
+            "Package 'my-project' has an invalid Requires-Python: "
+            "Invalid specifier: 'invalid'"
+        )
+
+    @pytest.mark.parametrize('metadata_name', [
+        'METADATA',
+        'PKG-INFO',
+    ])
+    def test_empty_metadata_error(self, caplog, metadata_name):
+        """
+        Test dist.has_metadata() returning True and dist.get_metadata()
+        returning None.
+        """
+        dist = make_fake_dist(metadata_name=metadata_name)
+        dist.metadata = None
+
+        # Make sure our test setup is correct.
+        assert dist.has_metadata(metadata_name)
+        assert dist.get_metadata(metadata_name) is None
+
+        with pytest.raises(NoneMetadataError) as exc:
+            _check_dist_requires_python(
+                dist,
+                version_info=(3, 6, 5),
+                ignore_requires_python=False,
+            )
+        assert str(exc.value) == (
+            "None {} metadata found for distribution: "
+            "<distribution 'my-project'>".format(metadata_name)
+        )