fix(manifest_ingest): scope Maven pom.xml dependency parsing correctly#2020
Open
kimdzhekhon wants to merge 1 commit into
Open
fix(manifest_ingest): scope Maven pom.xml dependency parsing correctly#2020kimdzhekhon wants to merge 1 commit into
kimdzhekhon wants to merge 1 commit into
Conversation
_parse_pom() had two accuracy bugs found by code review:
1. root.findall(".//dependencies/dependency") searches the whole tree,
so it also picked up <dependencyManagement> entries (version pins,
not real dependencies) and <build><plugins><plugin> dependencies as
if they were project dependencies, producing false depends_on edges.
Scoped to root-level <dependencies> plus <profiles><profile>
dependencies, which are the only legitimate dependency sources.
2. A child module's <groupId>/<version> are commonly omitted and
inherited from <parent>. The old code left them unset in that case,
so the node id became a bare artifactId instead of "group:artifact",
silently breaking depends_on edges from any other pom that references
the dependency by its full coordinates. Falls back to
parent/groupId and parent/version when the top-level element is
absent.
Found via codex-assisted code review, verified against the actual code
before delegating the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found via code review, not tied to an existing issue. Two accuracy bugs in
_parse_pom():dependencyManagement/plugin dependencies leaking in as real deps.root.findall(".//dependencies/dependency")searches the entire tree, so it also matched<dependencyManagement><dependencies>entries (version pins, not actual dependencies) and<build><plugins><plugin><dependencies>(build-tool-only deps), producing falsedepends_onedges. Scoped to root-level<dependencies>plus<profiles><profile><dependencies>— the only legitimate sources of a project's actual dependencies.groupId/versionparent inheritance. A child module commonly omits<groupId>/<version>and inherits them from<parent>. The old code left them unset in that case, so the node id became a bare artifactId instead ofgroup:artifact— silently breakingdepends_onedges from any other pom referencing the dependency by full coordinates. Falls back toparent/groupId/parent/versionwhen the top-level element is absent.Test plan
tests/test_manifest_ingest.py: top-level deps only picked up;dependencyManagement/plugin deps excluded;groupIdinherited fromparent;versioninherited fromparentuv run pytest tests/test_manifest_ingest.py -q -k pom— 6 passeduv run pytest tests/ -q— full suite: 3416 passed (5 pre-existing failures unrelated to this change, missing optionalopenaidependency in this environment)