Fix UpgradePluginVersion to handle plugins with 'apply false'#6269
Conversation
The previous implementation only checked the immediate parent of the version() method call for the plugins() method. This failed when other method calls (like 'apply false') were chained between version() and plugins(). This change walks up the entire method invocation chain to find the plugins() method, correctly handling patterns like: id(...) version "x.y.z" apply false Added test coverage for both Kotlin DSL and Groovy DSL with apply false.
POSIX standard requires text files to end with a newline character.
|
This recipe predates several new enhancements, one of which I think could excellently be deployed here and that's the In it's current form, it'd help to find the plugin declaration while also handling several more cases. Furthermore, like we've done with other traits could be updated to handle the basic changes that one may want to apply to a plugin declaration. If you feel up to this @flex-gwanbin , then I can help coach you through various aspects of the above. Alternatively, if that feels a bit too much Tim or I can jump in and apply this improvement. |
…ching Replace manual VERSION_MATCHER, isPluginVersion(), and kotlin special-casing with the GradlePlugin trait which already understands all plugin declaration forms. Also fix the trait to unwrap J.Parentheses when extracting version literals and walk up chained method invocations in withinPlugins().
|
Thanks both! I've gone ahead and made the changes to use the new trait here ; would appreciate another quick round of review from @shanman190 before we merge. |
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); | ||
| if (!isPluginVersion(getCursor())) { | ||
|
|
||
| if (!"version".equals(m.getSimpleName())) { |
There was a problem hiding this comment.
Are we needing to stop short here for some reason? The trait will select the outer most element of the plugin chain (apply -> version -> plugin).
Address PR review feedback: - Remove "version" method name filter in scanner; let trait match any plugin chain element - Use trait's built-in pluginIdPattern filtering instead of custom matchesPluginId, translating "kotlin" shorthand to "org.jetbrains.kotlin.*" - Keep "version" check in visitor only where we modify arguments
| private String effectivePluginIdPattern() { | ||
| // Translate the short "kotlin" DSL name to the full plugin ID pattern | ||
| return "kotlin".equals(pluginIdPattern) ? "org.jetbrains.kotlin.*" : pluginIdPattern; | ||
| } |
There was a problem hiding this comment.
I don't think this is necessary or the places where we are doing this aren't correct.
In particular, the trait expands out the Kotlin plugin alias to its full plugin id automatically -- kotlin("jvm") -> org.jetbrains.kotlin.jvm. Since the pluginIdPattern is matching on the effective plugin id, then it stands to reason that if an UpgradePluginVersion is trying to target all Kotlin plugins it should instead be doing a pattern of org.jetbrains.kotlin.* as shown. kotlin standalone doesn't really mean anything from a Gradle standpoint.
The trait already expands kotlin("jvm") to org.jetbrains.kotlin.jvm,
so tests should use org.jetbrains.kotlin.* as the pattern rather than
relying on a "kotlin" shorthand translation.
shanman190
left a comment
shanman190
left a comment
There was a problem hiding this comment.
Further improvements that we could make, but are not needed right now:
- Enable the trait to change the plugin id
- Enable the trait to change the plugin version
- Enable the trait to change the plugin applicability (ie. add or remove
apply false)
github-project-automation
Bot
moved this from In Progress
to Ready to Review
in OpenRewrite
3 participants
Changes
isPluginVersion()to walk up the entire method invocation chain to find theplugins()methodapply falsewas betweenversion()andplugins()id(...) version "x.y.z" apply falseTesting
Added test coverage for both DSLs:
kotlinDslWithApplyFalse: Tests Kotlin DSL with apply falsegroovyDslWithApplyFalse: Tests Groovy DSL with apply falseBefore
plugins { id("com.github.johnrengelman.shadow") version "7.1.0" apply false }No upgrade happened ❌
After
plugins { id("com.github.johnrengelman.shadow") version "7.1.2" apply false }Successfully upgraded ✅
Root Cause
The method invocation chain with
apply falseis:The previous implementation only checked one level up from
version(), findingapply()instead ofplugins().Technical Details
Changed in
UpgradePluginVersion.java:116-141:plugins()or reaching the rootapply()