Skip to content
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

Draft: Created a test for UpgradeTransitiveDependencyVersion that demonstrates the "Gradle visitor used for a Maven project" issue #106

Closed
wants to merge 1 commit into from

Conversation

gjd6640
Copy link

@gjd6640 gjd6640 commented Jul 10, 2024

What's your motivation?

I'm learning to use OpenRewrite. My overall goal is to create tooling to resolve CVEs across many Maven Java projects.

I have been unsuccessfully trying to use "org.openrewrite.java.dependencies.UpgradeTransitiveDependencyVersion" to increase version numbers. Today I found that "org.openrewrite.maven.UpgradeTransitiveDependencyVersion" worked for my specific example but "org.openrewrite.java.dependencies.UpgradeTransitiveDependencyVersion" did nothing. I'm under the impression that the latter is the newer and more flexible recipe.

Anything in particular you'd like reviewers to focus on?

This PR contains a new test class that demonstrates the setup, expected results, and the comments show which recipes worked and which didn't. When I used a debugger to see where things went wrong it looked like the issue was that the gradle isAcceptable call on line 129 of UpgradeTransitiveDepedencyVersion evaluated to true which prevented it from running the maven visitor code on line 132.

                if(gradleUDV.isAcceptable(t, ctx)) {  // This was true
                    t = (SourceFile) gradleUDV.visitNonNull(t, ctx);  // This ran...
                } else if(mavenUTDV.isAcceptable(t, ctx)) {
                    t = (SourceFile) mavenUTDV.visitNonNull(t, ctx);  // This never ran
                }


Ability to help further

Unfortunately, I can't commit the time required to figure out how to get Gradle working and deliver a fix for this. I'm hoping that by creating this test I'm making it easy enough for some else to quickly grok this and code up a fix.

Workarounds

I plan to use the maven-specific recipe mentioned above in the meantime since that one seems to work.

Checklist

I'm not set up to work with Gradle projects so this code is just copy-pasted from where I worked on it in a fork of rewrite-recipe-starter. Hopefully it is clean enough for you to work with.

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek timtebeek added the question Further information is requested label Jul 12, 2024
@timtebeek
Copy link
Contributor

Hi @gjd6640 ; Somehow I didn't get notified of this PR, hence the delayed feedback here. Sorry about that!

You mentioned you're looking to resolve vulnerabilities in transitive dependencies; that's great!

For context: From the use case you've shared I can see the transitive version of snappy-java is 1.1.7.3 in
https://repo1.maven.org/maven2/org/apache/spark/spark-core_2.12/2.4.4/spark-core_2.12-2.4.4.pom

We'll have to explore why indeed that Maven version is working for you, while the delegating Maven/Gradle agnostic version does not work.

org.openrewrite.java.dependencies.DependencyVulnerabilityCheck

Do note that we also have a dedicated recipe to resolve vulnerable dependencies, which leverages the recipes you use here:
https://docs.openrewrite.org/recipes/java/dependencies/dependencyvulnerabilitycheck
One caveat:

This recipe only upgrades to the latest patch version. If a minor or major upgrade is required to reach the fixed version, this recipe will not make any changes.

In addition to bumping .patch versions the recipe also produces a CSV data table in target/ named org.openrewrite.java.dependencies.table.VulnerabilityReport.csv.

@timtebeek
Copy link
Contributor

Thanks @gjd6640 ; I've traced it to this issue, which will then likely resolve the issues you're seeing here.

@gjd6640
Copy link
Author

gjd6640 commented Jul 13, 2024

Nice! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants