rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

Added a new unit test to check the use of variables in the plugins section in a Gradle file

Open lnnery opened this issue 2 years ago • 12 comments

What's changed?

Added a new unit test to check the use of variables in the plugins section in a Gradle file. There was an error when using variables in this section, and that was what led to the creation of the new test.

Have you considered any alternatives or workarounds?

Yes, if the variables are removed from the plugins section everything works, however, this is not a good approach as it requires changing the Gradle file.

Checklist

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

lnnery avatar Jan 10 '24 10:01 lnnery

Thanks for adding this test @lnnery ! Helpful indeed to track support for this use case too. In parallel there's also some efforts underway on the use of gradle.properties for dependency in

  • https://github.com/openrewrite/rewrite/pull/3665

You might be interested to follow those as well, and the plans there to create a ScanningRecipe to handle gradle.properties files, as well as the associated Slack thread.

timtebeek avatar Jan 10 '24 10:01 timtebeek

Sharing the failure here for easier visibility

FindPluginsTest > findPluginWithVariable() STANDARD_OUT
    doesntmatter: 3: [Static type checking] - The variable [jfrogBintrayVersion] is undeclared.

     @ line 3, column 39.

           id 'com.jfrog.bintray' version "${jfrogBintrayVersion}"

                                             ^


    1 error



FindPluginsTest > findPluginWithVariable() FAILED
    java.lang.ClassCastException: class org.openrewrite.groovy.tree.G$GString cannot be cast to class org.openrewrite.java.tree.J$Literal (org.openrewrite.groovy.tree.G$GString and org.openrewrite.java.tree.J$Literal are in unnamed module of loader 'app')
        at org.openrewrite.gradle.search.FindPlugins.lambda$find$0(FindPlugins.java:95)
        at org.openrewrite.gradle.search.FindPlugins$$Lambda$1101/0x00007f65d892abc0.apply(Unknown Source)
        at java.base/java.util.stream.ReferencePipeline$7$1.accept(ReferencePipeline.java:273)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
        at org.openrewrite.gradle.search.FindPlugins.find(FindPlugins.java:99)
        at org.openrewrite.gradle.search.FindPluginsTest.lambda$findPluginWithVariable$4(FindPluginsTest.java:76)
        at org.openrewrite.gradle.search.FindPluginsTest$$Lambda$1090/0x00007f65d891ca10.accept0(Unknown Source)
        at org.openrewrite.internal.ThrowingConsumer.accept(ThrowingConsumer.java:26)
        at org.openrewrite.test.SourceSpec.lambda$beforeRecipe$4(SourceSpec.java:153)
        at org.openrewrite.test.SourceSpec$$Lambda$1091/0x00007f65d891cc68.apply0(Unknown Source)
        at org.openrewrite.test.internal.ThrowingUnaryOperator.apply(ThrowingUnaryOperator.java:28)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:323)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:127)
        at org.openrewrite.gradle.search.FindPluginsTest.findPluginWithVariable(FindPluginsTest.java:62)

timtebeek avatar Jan 10 '24 16:01 timtebeek

You might want to keep an eye out on this PR as well, which adds similar support to UpgradePluginVersion:

  • https://github.com/openrewrite/rewrite/pull/3933

timtebeek avatar Jan 19 '24 12:01 timtebeek

The above PR https://github.com/openrewrite/rewrite/pull/3933 has now been merged, and the same approach can likely be replicated here to allow finding plugins which use properties in their version number.

timtebeek avatar Jan 22 '24 18:01 timtebeek

Still the same failure here for FindPlugins; there's been a lot of changes recently to recipes affecting Gradle to support properties files, but I suspect FindPlugins was missed so far. We can likely copy the same approach here though to get this going.

timtebeek avatar Feb 13 '24 18:02 timtebeek

Hi @ajohnsonz, adding you here as requested by @timtebeek :)

lnnery avatar Feb 13 '24 18:02 lnnery

Ah yes, thanks @lnnery ! Figured @ajohnsonz might have some insights to share as to what's needed to make FindPlugins also work with plugin versions defined in a properties file.

timtebeek avatar Feb 13 '24 18:02 timtebeek

We might need to be mindful that if FindPlugins is currently used as a precondition, then we can not simply convert it into a scanning recipe, as scanning recipes are not supported as preconditions.

timtebeek avatar Feb 13 '24 18:02 timtebeek

Hi @lnnery @timtebeek - you can certainly take a look at UpgradePluginVersion which has now been upgraded to work with plugins whose version is in gradle.properties - I'm not very familiar with "FindPlugin" but I assume that perhaps UpgradePluginVersion is doing both a find and a replace, so could copy some of the code out of there and remove the "replace" part?

It really depends if you care about finding the plugin and also knowing the version, or if you just care about finding the groupId and artifactId - if the latter, a simple if instanceof GString block before the downcast would at least prevent that exception. If the former I'm not really sure how that would be achieved if it can't support ScanningRecipe 🤔

ajohnsonz avatar Feb 14 '24 10:02 ajohnsonz

Thanks for chiming in! Looking at how FindPlugins is used in AddPluginVisitor it might make most sense indeed to add that instanceof for now. https://github.com/search?q=org%3Aopenrewrite+FindPlugins&type=code

timtebeek avatar Feb 14 '24 11:02 timtebeek

I've pushed up a minimal change to prevent that cast exception; figured easier to iterate if there's something to critique. This now returns the variable name, which I don't think we have to resolve to be able to satisfy FindPlugins responsibility. It seems we only use this from AddPluginVisitor, where we check for the existence of the plugin, not whatever version is used there already. Good enough?

timtebeek avatar Feb 14 '24 12:02 timtebeek