Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ChangeType and ChangePackage to work with SourceFileWithReference #4648

Merged
merged 24 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Contributor

@knutwannheden knutwannheden Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider renaming this to something more generic like SymbolReference or just Reference and then have a "kind" property, which would probably be an enum like Kind with constants for Type and Namespace (or Package) for now.

That "name" property will probably also have to be complemented by a structured "symbol" property at some point (so that the caller can query a method reference for name, declaring type, and parameters). But this can wait, I think.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree but I also feel like this is a creep of the scope, could we log it somewhere as future improvement?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It would just be good if we could get the API finalized before people try to start using it.

Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
*/
package org.openrewrite.trait;

import org.openrewrite.Incubating;
import org.openrewrite.SourceFile;
import org.openrewrite.Tree;
import org.openrewrite.*;

import java.util.Set;

Expand All @@ -26,6 +24,14 @@ public interface TypeReference extends Trait<Tree> {

String getName();

default boolean supportsRename() {
return false;
}

default TreeVisitor<Tree, ExecutionContext> renameTo(String name) {
throw new UnsupportedOperationException();
}
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved

default boolean matches(Matcher matcher) {
return matcher.matchesName(getName());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.xml.Assertions.xml;

@SuppressWarnings("ConstantConditions")
class ChangePackageTest implements RewriteTest {
Expand Down Expand Up @@ -1702,4 +1703,26 @@ public enum MyEnum {
)
);
}

@Test
void changePackageInSpringXml() {
rewriteRun(
spec -> spec.recipe(new ChangePackage("test.type", "test.test.type", true)),
xml(
"""
<?xml version="1.0" encoding="UTF-8"?>
<beans xsi:schemaLocation="www.springframework.org/schema/beans">
<bean id="abc" class="test.type.A"/>
</beans>
""",
"""
<?xml version="1.0" encoding="UTF-8"?>
<beans xsi:schemaLocation="www.springframework.org/schema/beans">
<bean id="abc" class="test.test.type.A"/>
</beans>
"""
)
);

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.xml.Assertions.xml;

@SuppressWarnings("ConstantConditions")
class ChangeTypeTest implements RewriteTest {
Expand Down Expand Up @@ -2019,4 +2020,26 @@ class Letters {
);
}

@Test
void changeTypeInSpringXml() {
rewriteRun(
spec -> spec.recipe(new ChangeType("test.type.A", "test.type.B", true)),
xml(
"""
<?xml version="1.0" encoding="UTF-8"?>
<beans xsi:schemaLocation="www.springframework.org/schema/beans">
<bean id="abc" class="test.type.A"/>
</beans>
""",
"""
<?xml version="1.0" encoding="UTF-8"?>
<beans xsi:schemaLocation="www.springframework.org/schema/beans">
<bean id="abc" class="test.type.B"/>
</beans>
"""
)
);

}

}
71 changes: 64 additions & 7 deletions rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@
import org.openrewrite.internal.ListUtils;
import org.openrewrite.java.tree.*;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.trait.TypeReference;

import java.nio.file.Paths;
import java.util.HashMap;
import java.util.IdentityHashMap;
import java.util.Map;

import static java.util.Objects.requireNonNull;

/**
* A recipe that will rename a package name in package statements, imports, and fully-qualified types (see: NOTE).
* <p>
Expand Down Expand Up @@ -83,11 +83,11 @@ public Validated<Object> validate() {

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
JavaIsoVisitor<ExecutionContext> condition = new JavaIsoVisitor<ExecutionContext>() {
TreeVisitor<?, ExecutionContext> condition = new TreeVisitor<Tree, ExecutionContext>() {
@Override
public @Nullable J preVisit(J tree, ExecutionContext ctx) {
public @Nullable Tree preVisit(Tree tree, ExecutionContext ctx) {
if (tree instanceof JavaSourceFile) {
JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree);
JavaSourceFile cu = (JavaSourceFile) tree;
if (cu.getPackageDeclaration() != null) {
String original = cu.getPackageDeclaration().getExpression()
.printTrimmed(getCursor()).replaceAll("\\s", "");
Expand All @@ -113,14 +113,52 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
}
stopAfterPreVisit();
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
}
if (tree instanceof SourceFileWithTypeReferences) {
SourceFileWithTypeReferences cu = (SourceFileWithTypeReferences) tree;
boolean recursive = Boolean.TRUE.equals(ChangePackage.this.recursive);
String recursivePackageNamePrefix = oldPackageName + ".";
for (TypeReference ref : cu.getTypeReferences().getTypeReferences()) {
if (ref.getName().equals(oldPackageName) || recursive && ref.getName().startsWith(recursivePackageNamePrefix)) {
Copy link
Contributor

@knutwannheden knutwannheden Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally I think we should only be using the matches() and renameTo() methods instead of the getName() method, but coming up with a good API for those two methods feels pretty difficult, so maybe this will have to wait. I was thinking something along the lines of Java's Matcher for regex:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One example I keep thinking about here is when we have META-INF/services files as we have them for the TypeReference.Provider interface. In this case the name of a nested type looks like foo.Bar$Baz. Now ChangeType can't possibly know that it needs to call renameTo("foo.Bar$Baz") and not renameTo("foo.Bar.Baz"). The latter is probably correct for some other config files. Currently, the ChangeType visitor would have to make sure to always pass in names like "foo.Bar$Baz" and then the TypeReference implementation would have to know if it should apply a heuristic (although Bar$Baz could even be the name of the top-level class, but that would be highly unlikely). The question is just what should the API look like to cater for this?

return SearchResult.found(cu);
knutwannheden marked this conversation as resolved.
Show resolved Hide resolved
}
}
stopAfterPreVisit();
}
return super.preVisit(tree, ctx);
}
};

return Preconditions.check(condition, new ChangePackageVisitor());
return Preconditions.check(condition, new TreeVisitor<Tree, ExecutionContext>() {
@Override
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
return sourceFile instanceof JavaSourceFile || sourceFile instanceof SourceFileWithTypeReferences;
}

@Override
public @Nullable Tree preVisit(@Nullable Tree tree, ExecutionContext ctx) {
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
if (tree instanceof JavaSourceFile) {
return new JavaChangePackageVisitor().visit(tree, ctx);
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
} else if (tree instanceof SourceFileWithTypeReferences) {
SourceFileWithTypeReferences sourceFile = (SourceFileWithTypeReferences) tree;
SourceFileWithTypeReferences.TypeReferences typeReferences = sourceFile.getTypeReferences();
boolean recursive = Boolean.TRUE.equals(ChangePackage.this.recursive);
String recursivePackageNamePrefix = oldPackageName + ".";
Map<Tree, TypeReference> matches = new HashMap<>();
for (TypeReference ref : typeReferences.getTypeReferences()) {
if (ref.supportsRename()) {
if (ref.getName().equals(oldPackageName) || recursive && ref.getName().startsWith(recursivePackageNamePrefix)) {
matches.put(tree, ref);
}
}
}
return new TypeReferenceChangePackageVisitor(matches, oldPackageName, newPackageName).visit(tree, ctx, getCursor().getParent());
}
return tree;
}
});
}

private class ChangePackageVisitor extends JavaVisitor<ExecutionContext> {
private class JavaChangePackageVisitor extends JavaVisitor<ExecutionContext> {
private static final String RENAME_TO_KEY = "renameTo";
private static final String RENAME_FROM_KEY = "renameFrom";

Expand Down Expand Up @@ -349,4 +387,23 @@ private boolean isTargetRecursivePackageName(String packageName) {
}

}

@Value
@EqualsAndHashCode(callSuper = false)
private static class TypeReferenceChangePackageVisitor extends TreeVisitor<Tree, ExecutionContext> {
Map<Tree, TypeReference> matches;
String oldPackageName;
String newPackageName;

@Override
public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
Tree tree1 = super.visit(tree, ctx);
TypeReference ref = matches.get(tree);
if (ref != null) {
return ref.renameTo(ref.getName().replace(oldPackageName, newPackageName)).visit(tree, ctx, getCursor().getParent());
}
return tree1;
}
}

}
54 changes: 48 additions & 6 deletions rewrite-java/src/main/java/org/openrewrite/java/ChangeType.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.openrewrite.java.tree.*;
import org.openrewrite.marker.Markers;
import org.openrewrite.marker.SearchResult;
import org.openrewrite.trait.TypeReference;

import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -81,22 +82,45 @@ public String getDescription() {
public TreeVisitor<?, ExecutionContext> getVisitor() {
TreeVisitor<?, ExecutionContext> condition = new TreeVisitor<Tree, ExecutionContext>() {
@Override
public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
public @Nullable Tree preVisit(@Nullable Tree tree, ExecutionContext ctx) {
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
if (tree instanceof JavaSourceFile) {
JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree);
JavaSourceFile cu = (JavaSourceFile) tree;
if (!Boolean.TRUE.equals(ignoreDefinition) && containsClassDefinition(cu, oldFullyQualifiedTypeName)) {
return SearchResult.found(cu);
}
return new UsesType<>(oldFullyQualifiedTypeName, true).visitNonNull(cu, ctx);
}
if (tree instanceof SourceFileWithTypeReferences) {
SourceFileWithTypeReferences cu = (SourceFileWithTypeReferences) tree;
return new UsesType<>(oldFullyQualifiedTypeName, true).visitNonNull(cu, ctx);
}
return tree;
}
};

return Preconditions.check(condition, new ChangeTypeVisitor(oldFullyQualifiedTypeName, newFullyQualifiedTypeName, ignoreDefinition));
return Preconditions.check(condition, new TreeVisitor<Tree, ExecutionContext>() {
@Override
public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) {
return sourceFile instanceof JavaSourceFile || sourceFile instanceof SourceFileWithTypeReferences;
}

@Override
public @Nullable Tree preVisit(@Nullable Tree tree, ExecutionContext ctx) {
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
if (tree instanceof JavaSourceFile) {
return new JavaChangeTypeVisitor(oldFullyQualifiedTypeName, newFullyQualifiedTypeName, ignoreDefinition).visit(tree, ctx);
} else if (tree instanceof SourceFileWithTypeReferences) {
SourceFileWithTypeReferences sourceFile = (SourceFileWithTypeReferences) tree;
SourceFileWithTypeReferences.TypeReferences typeReferences = sourceFile.getTypeReferences();
TypeMatcher matcher = new TypeMatcher(oldFullyQualifiedTypeName);
Set<TypeReference> matches = new HashSet<>(typeReferences.findMatches(matcher));
return new TypeReferenceChangeTypeVisitor(matches, newFullyQualifiedTypeName).visit(tree, ctx, getCursor().getParent());
}
return tree;
}
});
}

private static class ChangeTypeVisitor extends JavaVisitor<ExecutionContext> {
private static class JavaChangeTypeVisitor extends JavaVisitor<ExecutionContext> {
private final JavaType.Class originalType;
private final JavaType targetType;

Expand All @@ -109,7 +133,7 @@ private static class ChangeTypeVisitor extends JavaVisitor<ExecutionContext> {
private final Map<JavaType, JavaType> oldNameToChangedType = new IdentityHashMap<>();
private final Set<String> topLevelClassnames = new HashSet<>();

private ChangeTypeVisitor(String oldFullyQualifiedTypeName, String newFullyQualifiedTypeName, @Nullable Boolean ignoreDefinition) {
private JavaChangeTypeVisitor(String oldFullyQualifiedTypeName, String newFullyQualifiedTypeName, @Nullable Boolean ignoreDefinition) {
this.originalType = JavaType.ShallowClass.build(oldFullyQualifiedTypeName);
this.targetType = JavaType.buildType(newFullyQualifiedTypeName);
this.ignoreDefinition = ignoreDefinition;
Expand Down Expand Up @@ -527,6 +551,24 @@ private boolean hasNoConflictingImport(@Nullable JavaSourceFile sf) {
}
}

@Value
@EqualsAndHashCode(callSuper = false)
private static class TypeReferenceChangeTypeVisitor extends TreeVisitor<Tree, ExecutionContext> {
Set<TypeReference> matches;
String newFullyQualifiedName;

@Override
public @Nullable Tree preVisit(@Nullable Tree tree, ExecutionContext ctx) {
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
Tree tree1 = super.visit(tree, ctx);
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
for (TypeReference ref : matches) {
if (ref.getTree().equals(tree) && ref.supportsRename()) {
return ref.renameTo(newFullyQualifiedName).visit(tree, ctx, getCursor().getParent());
}
}
return tree1;
}
}

private static class ChangeClassDefinition extends JavaIsoVisitor<ExecutionContext> {
private final JavaType.Class originalType;
private final JavaType.Class targetType;
Expand Down Expand Up @@ -579,7 +621,7 @@ private boolean updatePath(JavaSourceFile sf, String oldPath, String newPath) {
}

@Override
public J.@Nullable Package visitPackage(J.Package pkg, ExecutionContext ctx) {
public J.@Nullable Package visitPackage(J.Package pkg, ExecutionContext ctx) {
Boolean updatePackage = getCursor().pollNearestMessage("UPDATE_PACKAGE");
if (updatePackage != null && updatePackage) {
String original = pkg.getExpression().printTrimmed(getCursor()).replaceAll("\\s", "");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@

import lombok.Value;
import org.jspecify.annotations.Nullable;
import org.openrewrite.Cursor;
import org.openrewrite.SourceFile;
import org.openrewrite.Tree;
import org.openrewrite.*;
import org.openrewrite.trait.SimpleTraitMatcher;
import org.openrewrite.trait.TypeReference;
import org.openrewrite.xml.XPathMatcher;
Expand Down Expand Up @@ -52,6 +50,27 @@ public String getName() {
throw new IllegalArgumentException("getTree() must be an Xml.Attribute or Xml.Tag: " + getTree().getClass());
}

@Override
public boolean supportsRename() {
return true;
}

@Override
public TreeVisitor<Tree, ExecutionContext> renameTo(String name) {
return new TreeVisitor<Tree, ExecutionContext>() {
@Override
public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
if (tree instanceof Xml.Attribute) {
return ((Xml.Attribute) tree).withValue(((Xml.Attribute) tree).getValue().withValue(name));
}
if (tree instanceof Xml.Tag) {
return ((Xml.Tag) tree).withValue(name);
}
return super.visit(tree, ctx);
}
};
}

static class Matcher extends SimpleTraitMatcher<SpringTypeReference> {
private final Pattern typeReference = Pattern.compile("(?:[a-zA-Z_][a-zA-Z0-9_]*\\.)+[A-Z*][a-zA-Z0-9_]*(?:<[a-zA-Z0-9_,?<> ]*>)?");
Laurens-W marked this conversation as resolved.
Show resolved Hide resolved
private final XPathMatcher classXPath = new XPathMatcher("//@class");
Expand Down
Loading