-
Notifications
You must be signed in to change notification settings - Fork 79
(fix)gradle: ignore (c) lines in the report (IDETECT-4760) #1602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a parsing issue in the Gradle dependency report configuration parser where constraint lines (marked with "(c)") were incorrectly treated as actual dependencies, leading to an inaccurate dependency graph.
Key Changes:
- Added filtering logic to skip constraint lines ending with "(c)" in the dependency report
- Ensures only actual resolved dependencies are parsed into the dependency graph
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| children.add(parser.parseLine(line, metadata)); | ||
| if (!line.trim().endsWith("(c)")) { | ||
| children.add(parser.parseLine(line, metadata)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it at all possible for a dependency constraint to not be elsewhere in the tree? Based on what Gradle says “(c) - A dependency constraint, not a dependency. The dependency affected by the constraint occurs elsewhere in the tree.“ it sounds like the answer is no but I wonder if we should add a fallback ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is responsible for checking every line irrespective of the level hence it's added here as (c) can occur in any level.
shantyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding a test case for a tree that has a transitive dependency constraint + rich version. @devmehtabd is more familiar with rich versions but it might be relevant.
...egration/detectable/detectables/gradle/inspection/parse/GradleReportConfigurationParser.java
Outdated
Show resolved
Hide resolved
We already have an integration test for gradle rich versions, so if that test passes, we should be good. But I suggest adding some functional or unit tests for the same. |
Root Cause
The GradleReportConfigurationParser was incorrectly parsing lines from the Gradle dependency report that represent constraints (indicated by a (c) suffix) and treating them as actual dependencies. This resulted in an inaccurate dependency graph where version constraints were mistakenly included as project dependencies. The actual resolved dependencies are listed elsewhere in the dependency tree, so these constraint lines should be ignored.
The Fix