openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.03k stars 300 forks source link

`ChangeStaticFielToMethod` does not correctly handle statically imported inner class. #1626

Closed yeikel closed 2 years ago

yeikel commented 2 years ago

In my codebase, I have many usages of

Constants.OK (returns the constant 200) and I'd like to replace it with io.netty.handler.codec.http.HttpResponseStatus.200.code() as I have the library in my classpath and there is no need to redefine the same constant

Is this something possible with the current recipes? Or do we need a new one?

Similar issues : https://github.com/openrewrite/rewrite/issues/1622

Also https://github.com/openrewrite/rewrite/blob/main/rewrite-java/src/main/java/org/openrewrite/java/ReplaceConstant.java

tkvangorder commented 2 years ago

I think you can probably get away with using ChangeStaticFieldToMethod for this use case. I would give that a try first.

tkvangorder commented 2 years ago

I will leave this open for the time being until you get a chance to try the existing recipe.

tkvangorder commented 2 years ago

@yeikel Did you get a chance to try this out?

yeikel commented 2 years ago

@yeikel Did you get a chance to try this out?

I tried

recipeList:
  - org.openrewrite.java.ChangeStaticFieldToMethod:
      oldClassName: com.Constants
      oldFieldName: SUCCESS_CODE
      newMethodName: io.netty.handler.codec.http.HttpResponseStatus.OK.codeAsText()
Constants { 

public static final String SUCCESS_CODE = "200"

}

io.netty.handler.codec.http.HttpResponseStatus.OK.codeAsText() returns "200" as well

After applying the recipe, I did not see any diff. I noticed the following warning in the logs but I am not sure what that means for me

[WARNING] class org.openrewrite.java.tree.J$NewArray cannot be cast to class org.openrewrite.java.tree.J$Block (org.openrewrite.java.tree.J$NewArray and org.openrewrite.java.tree.J$Block are in unnamed module of loader org.codehaus.plexus.classworlds.realm.ClassRealm @ff7ea29)
tkvangorder commented 2 years ago

@yeikel I have added a test that is working for your use case, you need to separate the new method class and new method name as follows (and drop the parens):

recipeList:
  - org.openrewrite.java.ChangeStaticFieldToMethod:
      oldClassName: com.Constants
      oldFieldName: SUCCESS_CODE
      newClassName: io.netty.handler.codec.http.HttpResponseStatus.OK
      newMethodName: codeAsText

See this test, it is similar but has a different package for Constants: https://github.com/openrewrite/rewrite/commit/54d1621332266ffab4a6d23dd9d1ed30c1e7efb3#diff-ebc8cfce68834516fa21a67358e3c2216c5247ed87990d358cea425331a14e8cR213-R270

tkvangorder commented 2 years ago

Let me know if this works for you. Thanks!

yeikel commented 2 years ago

Thank you

That worked, but I am also seeing this in my logs :

[WARNING] Expected a template that would generate exactly one statement to replace one statement, but generated 0. Template: {OK.codeAsText();}

tkvangorder commented 2 years ago

I suspect there is an edge case in your code that is resulting in an invalid template. If you search for any remaining instances of com.Constants.SUCCESS_CODE we might get a hint at where this is happening.

Thanks for all your input!

yeikel commented 2 years ago

Actually, the transformation worked (in terms of changes) but it is not compiling

It added the import

import io.netty.handler.codec.http.HttpResponseStatus.OK;

And called OK.codeAsText()

But OK is a static variable so it had to be imported statically for it to work :

public static final HttpResponseStatus OK = newStatus(200, "OK");

import static io.netty.handler.codec.http.HttpResponseStatus.OK

yeikel commented 2 years ago

Alternatively, it could be called HttpResponseStatus.OK.codeAsText() to avoid the static import

tkvangorder commented 2 years ago

Added the ability to specify the target (inside the newClass) on which the method will be invoked:

recipeList:
  - org.openrewrite.java.ChangeStaticFieldToMethod:
      oldClassName: com.Constants
      oldFieldName: SUCCESS_CODE
      newClassName: io.netty.handler.codec.http.HttpResponseStatus
      newTarget: OK
      newMethodName: codeAsText

This will result in the import of the class (as before) and then add the target in each replacement:

https://github.com/openrewrite/rewrite/blob/main/rewrite-test/src/main/kotlin/org/openrewrite/java/ChangeStaticFieldToMethodTest.kt#L252-L273

yeikel commented 2 years ago

Added the ability to specify the target (inside the newClass) on which the method will be invoked:

recipeList:
  - org.openrewrite.java.ChangeStaticFieldToMethod:
      oldClassName: com.Constants
      oldFieldName: SUCCESS_CODE
      newClassName: io.netty.handler.codec.http.HttpResponseStatus
      newTarget: OK
      newMethodName: codeAsText

This will result in the import of the class (as before) and then add the target in each replacement:

https://github.com/openrewrite/rewrite/blob/main/rewrite-test/src/main/kotlin/org/openrewrite/java/ChangeStaticFieldToMethodTest.kt#L252-L273

Thank you. I'll give it a try as soon as it's released