ChangePackage recipe does not handle package names with single element

somethingvague commented 5 months ago

What version of OpenRewrite are you using?

I am using

OpenRewrite v8.23.3
Maven/Gradle plugin gradlew

How are you running OpenRewrite?

Initially observed the issue when running in a custom Bazel setup but reproduced on an OpenRewrite fork off the main branch with a new test.

Fork Example Test

What is the smallest, simplest way to reproduce the problem?

./gradlew -q :rewrite-java-test:test --tests org.openrewrite.java.ChangePackageBugTest

> Configure project :
Inferred project: rewrite, version: 0.1.0-SNAPSHOT

> Task :rewrite-java-test:test FAILED

ChangePackageBugTest > updateVariableType() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "A.java":
    diff --git a/A.java b/A.java
    index 4f96f86..ef55fc0 100644
    --- a/A.java
    +++ b/A.java
    @@ -1,4 +1,4 @@ 
    -import org.openrewrite.somepkg.Test;
    +import somepkg.Test;

     public class A {
         Test a;
    ] 
    expected: 
      "import org.openrewrite.somepkg.Test;

      public class A {
          Test a;
      }"
     but was: 
      "import somepkg.Test;

      public class A {
          Test a;
      }"
        at java.base@17.0.4.1/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base@17.0.4.1/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base@17.0.4.1/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base@17.0.4.1/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.openrewrite.test.RewriteTest.assertContentEquals(RewriteTest.java:617)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:508)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:133)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:128)
        at app//org.openrewrite.java.ChangePackageBugTest.updateVariableType(ChangePackageBugTest.java:73)

ChangePackageBugTest > renameImport() FAILED
    java.lang.AssertionError: Recipe was expected to make a change but made no changes.
        at org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:118)
        at org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:97)
        at org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:41)
        at org.openrewrite.Recipe.run(Recipe.java:340)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:375)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:133)
        at org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:128)
        at org.openrewrite.java.ChangePackageBugTest.renameImport(ChangePackageBugTest.java:48)

3 tests completed, 2 failed

The test itself attempts to change a package from somepkg to org.openrewrite.somepkg but imports are not updated.

The issue stems from visitFieldAccess in the ChangePackage recipe which invokes isFullyQualifiedClassReference on the FieldAccess, passing in the old package name. This method expects there to be a . in the value passed in. It's not clear to me what the fix is since isFullyQualifiedClassReference is expecting a class reference, not a package name.

Are you interested in [contributing a fix to OpenRewrite

Sure, once I'm more comfortable with the codebase.

somethingvague commented 5 months ago

This unpleasant diff appears to fix the issue (all tests pass). Unsure what the idiomatic ways to solve the underlying issues are https://github.com/openrewrite/rewrite/commit/4561fa4787742081557595858ee9be2e667e39de

Edit: is it unsuitable because it also changes imports for subpackages

timtebeek commented 5 months ago

Thanks for the detailed report! Indeed perhaps a too simplistic assumption that there would always be a dot in a package. I'll try to fit in a more detailed look next week, although you're welcome to iterate on a better solution if you feel there's more to it. :)

somethingvague commented 5 months ago

Thanks for picking this up. I have an even worse diff which relies on the JavaType not being Unknown i.e. somepkg.RealClass is a known type but somepkg.other (as part of a package) is not. This works in our setup but it does not work in the tests since somepkg.RealClass is not on the class path and is therefore Unknown.

Off topic, but we still cannot use the recipe with a fix for this issue because we do not want to move files - they have already been moved to a sane place in our monorepo. How would you feel about a new option to not change the file paths? Happy to contribute if you think that'd be useful for others too.

timtebeek commented 5 months ago

Off topic, but we still cannot use the recipe with a fix for this issue because we do not want to move files - they have already been moved to a sane place in our monorepo. How would you feel about a new option to not change the file paths? Happy to contribute if you think that'd be useful for others too.

Thanks for the offer to contribute such an addition; I've not come across such a requirement before, which makes me hesitant to adopt and maintain that going forward. Instead, I propose to explore running ChangePackage as you would normally, potentially followed by Rename a file. You can bundle that into a recipe that you use internally, and package that with a fork of this template: https://github.com/moderneinc/rewrite-recipe-starter

openrewrite / rewrite