Use regionMatches in isFullyQualifiedClassReference causes wrong results

[ckcd]

What version of OpenRewrite are you using?

I am using the latest rewrite 8.41.2

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

mvn org.openrewrite.maven:rewrite-maven-plugin:5.46.2:run ...

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

Below is a very simple test case for using ChangeType to change import from: org.codehaus.jackson.annotate.JsonIgnoreProperties to com.fasterxml.jackson.annotation.JsonIgnoreProperties.

But in latest version it will failed: it remove the irrelevant import org.codehaus.jackson.annotate.JsonIgnore wrongly.

public class ChangeTypeWithAnnoTest implements RewriteTest {
    @Override
    public void defaults(RecipeSpec spec) {
        spec.recipe(new ChangeType("org.codehaus.jackson.annotate.JsonIgnoreProperties", "com.fasterxml.jackson.annotation.JsonIgnoreProperties", false))
          .parser(JavaParser.fromJavaVersion().classpath("jackson-core", "jackson-core-asl"));
    }

    @Test
    void testRecipe() {
        // language=java
        rewriteRun(
          spec -> spec.typeValidationOptions(TypeValidation.none()),
          java(
            """
             import org.codehaus.jackson.annotate.JsonIgnoreProperties;
             import org.codehaus.jackson.annotate.JsonIgnore;

             @JsonIgnoreProperties(ignoreUnknown = true)
             public class myClass {
                 @JsonIgnore
                 public boolean isDirty() {
                     return false;
                 }
             }
        """, """
             import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
             import org.codehaus.jackson.annotate.JsonIgnore;

             @JsonIgnoreProperties(ignoreUnknown = true)
             public class myClass {
                 @JsonIgnore
                 public boolean isDirty() {
                     return false;
                 }
             }
            """)
        );
    }
}

As for the reason, I did some troubleshooting, and seems it's because in this commit: e536ed2 we change the behavior of isFullyQualifiedClassReference

The details:
During the running of this testing recipe, at some point it will call isFullyQualifiedClassReference to check J.FieldAccess(org.codehaus.jackson.annotate.JsonIgnore) and String(org.codehaus.jackson.annotate.JsonIgnoreProperties), just like below picture shows. Obviously the correct result for this call should be false since they are two different class.

• In previous logic, it will return false correctly, because in old code in will goto return false in if (!fieldAccess.getName().getSimpleName().equals(className.substring(className.lastIndexOf('.') + 1)))
• But, in latest code, it will return true wrongly, because it cannot return false in theif (!simpleName.regionMatches(0, className, dotIndex + 1, simpleName.length())) part because the regionMatches will return true in this case (JsonIgnore and JsonIgnoreProperties has the same prefix).

So the root cause is, why we use regionMatches here? Personally, I think the old code is correct and enough.

@knutwannheden @timtebeek @pstreef Please help check this, that's a real serious bug for us.

What did you expect to see?

The expect behavior is it should only replace the JsonIgnoreProperties related import.

 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import org.codehaus.jackson.annotate.JsonIgnore;

 @JsonIgnoreProperties(ignoreUnknown = true)
 public class myClass {
     @JsonIgnore
     public boolean isDirty() {
         return false;
     }
 }

What did you see instead?

But it remove another import org.codehaus.jackson.annotate.JsonIgnore wrongly.

 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;

 @JsonIgnoreProperties(ignoreUnknown = true)
 public class myClass {
     @JsonIgnore
     public boolean isDirty() {
         return false;
     }
 }

[timtebeek]

hi @ckcd ; I don't know much about the specifics there, so I'm hoping Knut can chime in with at more detailed answer.

I do wonder about the ChangeType example that you've given; are you aware there's also a rewrite-jackson module containing that codehaus to fasterxml migration recipe? It might help to look through the implementation there; perhaps it's already handled differently there.

[ckcd]

@timtebeek Thanks for your reply!
Yes we find the rewrite-jackson repo and we are trying it, it should be helpful. But in this issue I just use the simple test to show the bug, and it's not only for specific codehaus/fastxml class i think, it should be a common issue.

[knutwannheden]

Let me take a look and provide a fix.