Boeg/gradle dependency constraint trait #5604
Conversation
|
ATM I have problems determining the versions denoted in variables. In other Traits, we rely on the Resolution result, but the constraints are not necessarily resolved in the same version or at all. |
There was a problem hiding this comment.
I still have some more testing to do, but wanted to drop an early feedback while I was thinking about it.
ATM I have problems determining the versions denoted in variables. In other Traits, we rely on the Resolution result, but the constraints are not necessarily resolved in the same version or at all.
To get better at this, we probably need to capture some more information via the marker. Another facet of complexity is that version can be something that isn't a standard version, but a version range as well.
Additional aspects that come to mind that would be useful on such a trait as this would be then because and possibly some logic with the version ranges (this part being down the road for sure).
For now though, I think it's reasonable to capture the information that we have available and can achieve 100% accuracy for. As you'll notice in the new GradlePlugin trait, I had to accept that some information just wasn't yet available in the marker, but the position and identification of the plugin is still valid and very useful. I think the same practice applies here in that we know it's a constraint, but if we can't reliably capture some bit of information then that's fine for phase 1. We can improve our capturing of information with future PRs.
| @@ -26,4 +26,8 @@ public static GradleDependency.Matcher gradleDependency() { | |||
| public static JvmTestSuite.Matcher jvmTestSuite() { | |||
| return new JvmTestSuite.Matcher(); | |||
| } | |||
|
|
|||
| public static GradleDependencyConstraint.Matcher gradleDependencyConstraint() { | |||
There was a problem hiding this comment.
What do you think about dropping the Gradle and gradle from these names?
IMO, there is a lot of the Gradle name being placed within the package, class name, and method names already and it's unlikely that you'd need to have more than one build tool in a single import list.
There was a problem hiding this comment.
I would keep some of them.
Imagine writing a recipe for dealing with Framework upgrade or analysis in multiple repositories, one would like to implement the same logic for maven and Gradle to be prepared for both.
If we drop the prefix, at least one of them has to be fully qualified. Same for the type names of GradleDependency.Matcher and MavenDependency.Matcher.
import static org.openrewrite.gradle.trait.Traits.gradleDependency;
import static org.openrewrite.maven.trait.Traits.mavenDependency;
// ..
mavenDependency().groupId("").artifactId("").asVisitor(d -> null);
gradleDependency().groupId("").artifactId("").asVisitor(d -> null);I think there is more to it if we take the other languages into account.
There was a problem hiding this comment.
Another solution would be to get rid of the Traits class. It does not contribute a lot as
Traits.gradleDependencyConstraint() vs. new GradleDependencyConstraint.Matcher() is not that much of a difference plus it would resolve all naming clashes. What do you think?
There was a problem hiding this comment.
Part of me wonders how often you'd actually have both because in the cases where we deal with multiple build tools those tend to be in rewrite-java-dependencies and utilize the recipes directly. But the point is fair for the very common ones for sure.
So if we expand this out for a moment, what could we technically call a dependency constraint on the Maven/Python/Node side? Would there ever be a direct parallel?
I don't think I mind too much honestly, but it is a thought that I was asking myself directly when creating the JvmTestSuite (couldn't think of a parallel that would ever make sense) and GradlePlugin (definitely has a direct parallel with Maven at a minumum) traits.
There was a problem hiding this comment.
Regarind the usage of both, also in custom analysis recipes and rewrite-java-security.
I can check at our end if the language engineers that currently work on Python/JS/C# are aware of anything similar.
We will omit the Traits class in a first step, so don't habe to discuss about the method name any more :D
|
@shanman190
you mean that we are accepting that the version for |
|
Essentially. Allow current state to have missing information where it's not reliably available. Longer term, the goal would to be to gather the information directly from the Gradle project, record it on the marker, then once we've determined the call site look it up from the marker itself negating the need for the variable lookup in the first place. |
What's changed?
Add a trait to act upon Gradle Dependency Constraints.
What's your motivation?
Expand trait coverage and enable easy implementation of Informal Marker in DependencyVulnerabilityCheck
Anything in particular you'd like reviewers to focus on?
Anyone you would like to review specifically?
Took most from @shanman190 ;)
Have you considered any alternatives or workarounds?
Any additional context
Checklist