Repository: tox-dev/pipdeptree (python)

Issue:
Error with a certain version constraint format
### What pipdeptree version are you using?

2.17.0

### Are you running pipdeptree in a virtual environment?

- [ ] Yes

### Describe the problem

Thank you for developing a great library!

We use `pipdeptree` in our CIs, and it fails in v2.17.0 but passes in v2.16.2.
https://github.com/optuna/optuna-integration/pull/109
```
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/requirements.py", line 35, in __init__
    parsed = _parse_requirement(requirement_string)
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/_parser.py", line 64, in parse_requirement
    return _parse_requirement(Tokenizer(source, rules=DEFAULT_RULES))
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/_parser.py", line 82, in _parse_requirement
    url, specifier, marker = _parse_requirement_details(tokenizer)
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/_parser.py", line 120, in _parse_requirement_details
    specifier = _parse_specifier(tokenizer)
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/_parser.py", line 210, in _parse_specifier
    with tokenizer.enclosing_tokens(
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/contextlib.py", line 142, in __exit__
    next(self.gen)
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/_tokenizer.py", line 187, in enclosing_tokens
    self.raise_syntax_error(
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/_tokenizer.py", line 165, in raise_syntax_error
    raise ParserSyntaxError(
packaging._tokenizer.ParserSyntaxError: Expected matching RIGHT_PARENTHESIS for LEFT_PARENTHESIS, after version specifier
    scikit-image (<0.[19](https://github.com/optuna/optuna-integration/actions/runs/8562392755/job/23465560814#step:7:20).0>=0.16.1) ; extra == 'all'
                 ~~~~~~~~^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.10.14/x64/bin/pipdeptree", line 8, in <module>
    sys.exit(main())
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pipdeptree/__main__.py", line 24, in main
    tree = PackageDAG.from_pkgs(pkgs)
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pipdeptree/_models/dag.py", line 47, in from_pkgs
    for r in p.requires():
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/pipdeptree/_models/package.py", line 102, in requires
    req = Requirement(r)
  File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/packaging/requirements.py", line 37, in __init__
    raise InvalidRequirement(str(e)) from e
packaging.requirements.InvalidRequirement: Expected matching RIGHT_PARENTHESIS for LEFT_PARENTHESIS, after version specifier
    scikit-image (<0.19.0>=0.16.1) ; extra == 'all'
                 ~~~~~~~~^
```

I guess that the cause of this error is the incorrect format of `catalyst`.
https://github.com/catalyst-team/catalyst/blob/v22.04/requirements/requirements-cv.txt#L3
```
scikit-image<0.19.0>=0.16.1
```

Pull request description:
Handle invalid requirement strings when using importlib.metadata
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

Patch:
diff --git a/src/pipdeptree/_models/dag.py b/src/pipdeptree/_models/dag.py
index 80f77fa..1410aeb 100644
--- a/src/pipdeptree/_models/dag.py
+++ b/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 @@ if TYPE_CHECKING:
     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 @@ class PackageDAG(Mapping[DistPackage, List[ReqPackage]]):
         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:
diff --git a/src/pipdeptree/_models/package.py b/src/pipdeptree/_models/package.py
index 370a6cd..39a9096 100644
--- a/src/pipdeptree/_models/package.py
+++ b/src/pipdeptree/_models/package.py
@@ -4,9 +4,9 @@ from abc import ABC, abstractmethod
 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 @@ if TYPE_CHECKING:
 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 @@ class DistPackage(Package):
         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:


Test patch:
diff --git a/tests/_models/test_dag.py b/tests/_models/test_dag.py
index b696669..eb909c1 100644
--- a/tests/_models/test_dag.py
+++ b/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'
+    )
diff --git a/tests/_models/test_package.py b/tests/_models/test_package.py
index d0fb28e..9b0434f 100644
--- a/tests/_models/test_package.py
+++ b/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
 
 
diff --git a/tests/conftest.py b/tests/conftest.py
index 0c00092..1323c68 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2,7 +2,7 @@ from __future__ import annotations
 
 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 mock_pkgs() -> Callable[[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