Handle invalid requirement strings when using importlib.metadata (#345)

Resolves #344.

This change also:
- Makes `Distribution.requires()` into a generator for performance (i.e.
avoid building `list[Requirement]` and then iterating `list[Requirement`
and instead do both at the same time) and so that we can handle invalid
requirement exceptions in `PackageDAG.from_pkgs()` for each individual
requirement string
- Modified conftest to avoid adding a extra comma at the end and removed
some older code

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: kemzeb

src/pipdeptree/_models/dag.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import sys
 from collections import defaultdict, deque
 from fnmatch import fnmatch
 from itertools import chain
@@ -11,7 +12,20 @@
     from importlib.metadata import Distribution
 
 
-from .package import DistPackage, ReqPackage
+from .package import DistPackage, InvalidRequirementError, ReqPackage
+
+
+def render_invalid_reqs_text_if_necessary(dist_name_to_invalid_reqs_dict: dict[str, list[str]]) -> None:
+    if not dist_name_to_invalid_reqs_dict:
+        return
+
+    print("Warning!!! Invalid requirement strings found for the following distributions:", file=sys.stderr)  # noqa: T201
+    for dist_name, invalid_reqs in dist_name_to_invalid_reqs_dict.items():
+        print(dist_name, file=sys.stderr)  # noqa: T201
+
+        for invalid_req in invalid_reqs:
+            print(f'  Skipping "{invalid_req}"', file=sys.stderr)  # noqa: T201
+    print("-" * 72, file=sys.stderr)  # noqa: T201
 
 
 class PackageDAG(Mapping[DistPackage, List[ReqPackage]]):
@@ -42,18 +56,30 @@ def from_pkgs(cls, pkgs: list[Distribution]) -> PackageDAG:
         dist_pkgs = [DistPackage(p) for p in pkgs]
         idx = {p.key: p for p in dist_pkgs}
         m: dict[DistPackage, list[ReqPackage]] = {}
+        dist_name_to_invalid_reqs_dict: dict[str, list[str]] = {}
         for p in dist_pkgs:
             reqs = []
-            for r in p.requires():
-                d = idx.get(canonicalize_name(r.name))
-                # Distribution.requires only return the name of requirements in metadata file, which may not be
-                # the same with the capitalized one in pip. We should retain the casing of required package name.
-                # see https://github.com/tox-dev/pipdeptree/issues/242
-                r.name = d.project_name if d is not None else r.name
-                pkg = ReqPackage(r, d)
+            requires_iterator = p.requires()
+            while True:
+                try:
+                    req = next(requires_iterator)
+                except InvalidRequirementError as err:
+                    # We can't work with invalid requirement strings. Let's warn the user about them.
+                    dist_name_to_invalid_reqs_dict.setdefault(p.project_name, []).append(str(err))
+                    continue
+                except StopIteration:
+                    break
+                d = idx.get(canonicalize_name(req.name))
+                # Distribution.requires only returns the name of requirements in the metadata file, which may not be the
+                # same as the name in PyPI. We should try to retain the original package names for requirements.
+                # See https://github.com/tox-dev/pipdeptree/issues/242
+                req.name = d.project_name if d is not None else req.name
+                pkg = ReqPackage(req, d)
                 reqs.append(pkg)
             m[p] = reqs
 
+        render_invalid_reqs_text_if_necessary(dist_name_to_invalid_reqs_dict)
+
         return cls(m)
 
     def __init__(self, m: dict[DistPackage, list[ReqPackage]]) -> None:

src/pipdeptree/_models/package.py

@@ -4,9 +4,9 @@
 from importlib import import_module
 from importlib.metadata import Distribution, PackageNotFoundError, metadata, version
 from inspect import ismodule
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Iterator
 
-from packaging.requirements import Requirement
+from packaging.requirements import InvalidRequirement, Requirement
 from packaging.utils import canonicalize_name
 
 if TYPE_CHECKING:
@@ -15,6 +15,14 @@
 from pipdeptree._adapter import PipBaseDistributionAdapter
 
 
+class InvalidRequirementError(ValueError):
+    """
+    An invalid requirement string was found.
+
+    When raising an exception, this should provide just the problem requirement string.
+    """
+
+
 class Package(ABC):
     """Abstract class for wrappers around objects that pip returns."""
 
@@ -96,17 +104,23 @@ def __init__(self, obj: Distribution, req: ReqPackage | None = None) -> None:
         self._obj = obj
         self.req = req
 
-    def requires(self) -> list[Requirement]:
-        req_list = []
+    def requires(self) -> Iterator[Requirement]:
+        """
+        Return an iterator of the distribution's required dependencies.
+
+        :raises InvalidRequirementError: If the metadata contains invalid requirement strings.
+        """
         for r in self._obj.requires or []:
-            req = Requirement(r)
+            try:
+                req = Requirement(r)
+            except InvalidRequirement:
+                raise InvalidRequirementError(r) from None
             if not req.marker or req.marker.evaluate():
                 # Make sure that we're either dealing with a dependency that has no environment markers or does but
                 # are evaluated True against the existing environment (if it's False, it means they cannot be
                 # installed). "extra" markers are always evaluated False here which is what we want when retrieving
                 # only required dependencies.
-                req_list.append(req)
-        return req_list
+                yield req
 
     @property
     def version(self) -> str:

tests/_models/test_dag.py

@@ -134,3 +134,19 @@ def test_package_dag_from_pkgs_uses_pep503normalize(mock_pkgs: Callable[[MockGra
     c = package_dag.get_children(parent_key)
     assert c[0].dist
     assert c[0].key == "flufl-lock"
+
+
+def test_package_from_pkgs_given_invalid_requirements(
+    mock_pkgs: Callable[[MockGraph], Iterator[Mock]], capfd: pytest.CaptureFixture[str]
+) -> None:
+    graph: dict[tuple[str, str], list[tuple[str, list[tuple[str, str]]]]] = {
+        ("a-package", "1.2.3"): [("BAD**requirement", [(">=", "2.0.0")])],
+    }
+    package_dag = PackageDAG.from_pkgs(list(mock_pkgs(graph)))
+    assert len(package_dag) == 1
+    out, err = capfd.readouterr()
+    assert not out
+    assert err == (
+        "Warning!!! Invalid requirement strings found for the following distributions:\na-package\n  "
+        'Skipping "BAD**requirement>=2.0.0"\n------------------------------------------------------------------------\n'
+    )

tests/_models/test_package.py

@@ -45,7 +45,7 @@ def test_dist_package_requires() -> None:
         requires=["bar", "baz >=2.7.2"],
     )
     dp = DistPackage(foo)
-    reqs = dp.requires()
+    reqs = list(dp.requires())
     assert len(reqs) == 2
 
 
@@ -55,7 +55,7 @@ def test_dist_package_requires_with_environment_markers_that_eval_to_false() ->
         requires=['foo ; sys_platform == "NoexistOS"', "bar >=2.7.2 ; extra == 'testing'"],
     )
     dp = DistPackage(foo)
-    reqs = dp.requires()
+    reqs = list(dp.requires())
     assert len(reqs) == 0
 
 

tests/conftest.py

@@ -2,7 +2,7 @@
 
 from random import shuffle
 from typing import TYPE_CHECKING, Callable, Iterator
-from unittest.mock import MagicMock, Mock
+from unittest.mock import Mock
 
 import pytest
 
@@ -18,16 +18,15 @@ def func(simple_graph: MockGraph) -> Iterator[Mock]:
         for node, children in simple_graph.items():
             nk, nv = node
             m = Mock(metadata={"Name": nk}, version=nv)
-            as_req = MagicMock(specifier=[("==", nv)])
-            as_req.name = nk
-            m.as_requirement = Mock(return_value=as_req)
             reqs = []
             for ck, cv in children:
                 r = ck
                 for item in cv:
                     if item:
                         rs, rv = item
-                        r = r + rs + rv + ","
+                        r = r + rs + rv
+                    if item != cv[-1]:
+                        r += ","
                 reqs.append(r)
             m.requires = reqs
             yield m