Stop resolving requirements one-by-one

[tetsuo-cpp]

I suspect that dependency resolution might be subtly wrong. Throughout the codebase, we follow an idiom where we have interfaces that have a function like foo to process a single item and then foo_all which returns an iterator.

We also apply this idiom in dependency resolution and end up processing requirements one by one: https://github.com/pypa/pip-audit/blob/main/pip_audit/_dependency_source/resolvelib/resolvelib.py#L77. This doesn't seem right to me because if two packages share the same sub-dependency, both packages requirement for that sub-dependency are going to be used to figure out which version to pick. I think that we need to feed the full list of requirements into the resolver for this it work correctly.

CC: @woodruffw @di

[woodruffw]

Yeah, that sounds suspect. Let's get a test case and see how our resolution behaves vs. pip's.

[tetsuo-cpp]

Yeah, this does seem wrong. I've got an example here:

(env) tetsuo@Alexs-MacBook-Pro pip-audit % cat dependency-resolution-test.txt
jinja2<=2.10
flask==1.0
(env) tetsuo@Alexs-MacBook-Pro pip-audit % pip-audit -v -r dependency-resolution-test.txt
DEBUG:pip_audit._cli:parsed arguments: Namespace(local=False, requirements=[<_io.TextIOWrapper name='dependency-resolution-test.txt' mode='r' encoding='UTF-8'>], project_path=None, format=<OutputFormatChoice.Columns: 'columns'>, vulnerability_service=<VulnerabilityServiceChoice.Pypi: 'pypi'>, dry_run=False, strict=False, desc=<VulnerabilityDescriptionChoice.Auto: 'auto'>, cache_dir=None, progress_spinner=<ProgressSpinnerChoice.On: 'on'>, timeout=15, paths=[], verbose=1, fix=False, require_hashes=False, index_url='https://pypi.org/simple/', extra_index_urls=[], skip_editable=False, no_deps=False, output=PosixPath('stdout'), ignore_vulns=[])
DEBUG:pip_audit._cli:Auditing jinja2 (2.10)
DEBUG:pip_audit._cli:Auditing markupsafe (2.1.1)
DEBUG:pip_audit._cli:Auditing flask (1.0)
DEBUG:pip_audit._cli:Auditing itsdangerous (2.1.2)
DEBUG:pip_audit._cli:Auditing jinja2 (3.1.2)
DEBUG:pip_audit._cli:Auditing click (8.1.3)
DEBUG:pip_audit._cli:Auditing werkzeug (2.2.2)
Found 2 known vulnerabilities in 1 package
Name   Version ID             Fix Versions
------ ------- -------------- ------------
jinja2 2.10    PYSEC-2021-66  2.11.3
jinja2 2.10    PYSEC-2019-217 2.10.1

I've used flask==1.0 which requires jinja2>=2.10. Then I've manually specified jinja2<=2.10. So we should choose audit Jinja2 2.10. Instead what happens is we audit Jinja2 twice. Once for the requirement that we've manually pinned (2.10), and another for Flask which resolves to 3.1.2 without considering the other requirement.

If I try to pip install from this same file, it resolves as we'd expect:

(dependency-resolution-env) tetsuo@Alexs-MacBook-Pro pip-audit % pip install -r dependency-resolution-test.txt
Collecting jinja2<=2.10
  Using cached Jinja2-2.10-py2.py3-none-any.whl (126 kB)
Collecting flask==1.0
  Using cached Flask-1.0-py2.py3-none-any.whl (97 kB)
Collecting Werkzeug>=0.14
  Using cached Werkzeug-2.2.2-py3-none-any.whl (232 kB)
Collecting itsdangerous>=0.24
  Using cached itsdangerous-2.1.2-py3-none-any.whl (15 kB)
Collecting click>=5.1
  Using cached click-8.1.3-py3-none-any.whl (96 kB)
Collecting MarkupSafe>=0.23
  Using cached MarkupSafe-2.1.1-cp39-cp39-macosx_10_9_universal2.whl (17 kB)
Installing collected packages: MarkupSafe, itsdangerous, click, Werkzeug, jinja2, flask
Successfully installed MarkupSafe-2.1.1 Werkzeug-2.2.2 click-8.1.3 flask-1.0 itsdangerous-2.1.2 jinja2-2.10
(dependency-resolution-env) tetsuo@Alexs-MacBook-Pro pip-audit % pip list
Package      Version
------------ -------
click        8.1.3
Flask        1.0
itsdangerous 2.1.2
Jinja2       2.10
MarkupSafe   2.1.1
pip          22.3.1
setuptools   65.6.3
Werkzeug     2.2.2
wheel        0.38.4

So I can confirm we've got a bug here. The good news is that this should be pretty easy to fix. The "streaming" design of this API was more important before we added the AuditState mechanism since it made things look more responsive. However, now that we provide CLI feedback via AuditState, this shouldn't be much of a loss. I'll make a PR shortly.

[woodruffw]

So I can confirm we've got a bug here. The good news is that this should be pretty easy to fix. The "streaming" design of this API was more important before we added the AuditState mechanism since it made things look more responsive. However, now that we provide CLI feedback via AuditState, this shouldn't be much of a loss. I'll make a PR shortly.

Nice work! And yeah, agreed -- the iterator-style APIs have outlived their primary function now that we plumb the CLI callback through the codebase (and it's all async now as well, thanks to rich).