Skip to content

Commit

Permalink
Improve handling of interfaces in LombokValueToRecord (#452)
Browse files Browse the repository at this point in the history 
* fix: exclude only conflicting interfaces

this also considers methods inherited by super-interfaces.

* Fix indentation of text blocks in tests

* Apply formatter to LombokValueToRecord.java

* Slight polish

---------

Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@holgpar @timtebeek
holgpar and timtebeek authored Apr 24, 2024
1 parent f1d17f5 commit dfbee69
Show file tree
Hide file tree
Showing 2 changed files with 148 additions and 57 deletions.
63 changes: 47 additions & 16 deletions src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,36 +99,67 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Ex
return cd;
}

assert cd.getType() != null : "Class type must not be null";
assert cd.getType() != null : "Class type must not be null"; // Checked in isRelevantClass
Set<String> memberVariableNames = getMemberVariableNames(memberVariables);
if (implementsConflictingInterfaces(cd, memberVariableNames)) {
return cd;
}

acc.putIfAbsent(
cd.getType().getFullyQualifiedName(),
getMemberVariableNames(memberVariables));
memberVariableNames);

return cd;
}

private boolean isRelevantClass(J.ClassDeclaration classDeclaration) {
List<J.Annotation> allAnnotations = classDeclaration.getAllAnnotations();
return classDeclaration.getType() != null
&& !J.ClassDeclaration.Kind.Type.Record.equals(classDeclaration.getKind())
&& classDeclaration.getAllAnnotations().stream()
.allMatch(ann -> LOMBOK_VALUE_MATCHER.matches(ann) && (ann.getArguments() == null || ann.getArguments().isEmpty()))
&& !hasGenericTypeParameter(classDeclaration)
&& classDeclaration.getBody().getStatements().stream().allMatch(this::isRecordCompatibleField)
&& !hasIncompatibleModifier(classDeclaration)
&& !implementsInterfaces(classDeclaration);
&& !J.ClassDeclaration.Kind.Type.Record.equals(classDeclaration.getKind())
&& !allAnnotations.isEmpty()
&& allAnnotations.stream().allMatch(ann -> LOMBOK_VALUE_MATCHER.matches(ann) && (ann.getArguments() == null || ann.getArguments().isEmpty()))
&& !hasGenericTypeParameter(classDeclaration)
&& classDeclaration.getBody().getStatements().stream().allMatch(this::isRecordCompatibleField)
&& !hasIncompatibleModifier(classDeclaration);
}

/**
* If the class target class implements an interface, transforming it to a record will not work in general,
* because the record access methods do not have the "get" prefix.
*
* @param classDeclaration
* @return
* @return true if the class implements an interface with a getter method based on a member variable
*/
private boolean implementsInterfaces(J.ClassDeclaration classDeclaration) {
private boolean implementsConflictingInterfaces(J.ClassDeclaration classDeclaration, Set<String> memberVariableNames) {
List<TypeTree> classDeclarationImplements = classDeclaration.getImplements();
return !(classDeclarationImplements == null || classDeclarationImplements.isEmpty());
if (classDeclarationImplements == null) {
return false;
}
return classDeclarationImplements.stream().anyMatch(implemented -> {
JavaType type = implemented.getType();
if (type instanceof JavaType.FullyQualified) {
return isConflictingInterface((JavaType.FullyQualified) type, memberVariableNames);
} else {
return false;
}
});
}

private static boolean isConflictingInterface(JavaType.FullyQualified implemented, Set<String> memberVariableNames) {
boolean hasConflictingMethod = implemented.getMethods().stream()
.map(JavaType.Method::getName)
.map(LombokValueToRecordVisitor::getterMethodNameToFluentMethodName)
.anyMatch(memberVariableNames::contains);
if (hasConflictingMethod) {
return true;
}
List<JavaType.FullyQualified> superInterfaces = implemented.getInterfaces();
if (superInterfaces != null) {
return superInterfaces.stream().anyMatch(i -> isConflictingInterface(i, memberVariableNames));
}
return false;
}

private boolean hasGenericTypeParameter(J.ClassDeclaration classDeclaration) {
List<J.TypeParameter> typeParameters = classDeclaration.getTypeParameters();
return typeParameters != null && !typeParameters.isEmpty();
Expand Down Expand Up @@ -182,10 +213,10 @@ private static class LombokValueToRecordVisitor extends JavaIsoVisitor<Execution
private static final String TO_STRING_MEMBER_DELIMITER = "\", \" +\n";
private static final String STANDARD_GETTER_PREFIX = "get";

private final Boolean useExactToString;
private final @Nullable Boolean useExactToString;
private final Map<String, Set<String>> recordTypeToMembers;

public LombokValueToRecordVisitor(Boolean useExactToString, Map<String, Set<String>> recordTypeToMembers) {
public LombokValueToRecordVisitor(@Nullable Boolean useExactToString, Map<String, Set<String>> recordTypeToMembers) {
this.useExactToString = useExactToString;
this.recordTypeToMembers = recordTypeToMembers;
}
Expand Down Expand Up @@ -220,8 +251,8 @@ private boolean isMethodInvocationOnRecordTypeClassMember(J.MethodInvocation met
String classFqn = classType.getFullyQualifiedName();

return recordTypeToMembers.containsKey(classFqn)
&& methodName.startsWith(STANDARD_GETTER_PREFIX)
&& recordTypeToMembers.get(classFqn).contains(getterMethodNameToFluentMethodName(methodName));
&& methodName.startsWith(STANDARD_GETTER_PREFIX)
&& recordTypeToMembers.get(classFqn).contains(getterMethodNameToFluentMethodName(methodName));
}

private static boolean isClassExpression(@Nullable Expression expression) {
Expand Down
142 changes: 101 additions & 41 deletions src/test/java/org/openrewrite/java/migrate/lombok/LombokValueToRecordTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,18 @@ void valueAnnotatedClassWithUseExactOptionKeepsLombokToString() {
java(
"""
import lombok.Value;
@Value
public class Test {
String field1;
String field2;
}
""",
"""
public record Test(
String field1,
String field2) {
@Override
public String toString() {
Expand All @@ -84,17 +84,17 @@ void convertOnlyValueAnnotatedClassWithoutDefaultValuesToRecord() {
java(
"""
package example;
import lombok.Value;
@Value
public class A {
String test;
}
""",
"""
package example;
public record A(
String test) {
}
Expand All @@ -103,31 +103,31 @@ public record A(
java(
"""
package example;
public class UserOfA {
private final A record;
public UserOfA() {
this.record = new A("some value");
}
public String getRecordValue() {
return record.getTest();
}
}
""",
"""
package example;
public class UserOfA {
private final A record;
public UserOfA() {
this.record = new A("some value");
}
public String getRecordValue() {
return record.test();
}
Expand All @@ -148,7 +148,7 @@ void onlyRemoveAnnotationFromRecords() {
import lombok.ToString;
import lombok.Value;
@Value
public class A {
String test;
Expand All @@ -162,7 +162,7 @@ public class B {
""",
"""
package example;
import lombok.ToString;
import lombok.Value;
Expand Down Expand Up @@ -209,6 +209,38 @@ record B(
"""
)
);


}

@Test
void interfaceIsImplementedThatDoesNotDefineFieldGetter() {
//language=java
rewriteRun(
s -> s.typeValidationOptions(TypeValidation.none()),
java(
"""
package example;
import lombok.Value;
import java.io.Serializable;
@Value
public class A implements Serializable {
String test;
}
""",
"""
package example;
import java.io.Serializable;
public record A(
String test) implements Serializable {
}
"""
)
);
}

@Nested
Expand All @@ -220,11 +252,11 @@ void classWithExplicitConstructor() {
java(
"""
import lombok.Value;
@Value
public class A {
String test;
public A() {
this.test = "test";
}
Expand All @@ -243,10 +275,10 @@ void classWithFieldAnnotations() {
"""
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Value;
@Value
public class A {
@JsonProperty
String test;
}
Expand All @@ -262,11 +294,11 @@ void classWithExplicitMethods() {
java(
"""
import lombok.Value;
@Value
public class A {
String test;
public String getTest() {
return test;
}
Expand All @@ -283,7 +315,7 @@ void genericClass() {
java(
"""
import lombok.Value;
@Value
public class A<T extends Object> {
T test;
Expand All @@ -301,7 +333,7 @@ void nonJava17Class() {
java(
"""
import lombok.Value;
@Value
public class A {
String test;
Expand All @@ -321,7 +353,7 @@ void classWithMultipleLombokAnnotations() {
"""
import lombok.Value;
import lombok.experimental.Accessors;
@Value
@Accessors(fluent = true)
public class A {
Expand Down Expand Up @@ -352,7 +384,7 @@ void classWithStaticField() {
java(
"""
import lombok.Value;
@Value
public class A {
static String disqualifyingField;
Expand Down Expand Up @@ -406,24 +438,52 @@ public class A {
}

@Test
void classImplementingInterfaces() {
void classImplementingConflictingInterface() {
//language=java
rewriteRun(
java(
"""
package example;
import lombok.Value;
interface I {
String getTest();
}
@Value
public class A implements I {
String test;
}
""")
"""
package example;
import lombok.Value;
interface I {
String getTest();
}
@Value
public class A implements I {
String test;
}
"""
)
);

}

@Test
void classImplementingConflictingInterfaceWithInheritance() {
//language=java
rewriteRun(
java(
"""
package example;
import lombok.Value;
interface I {
String getTest();
}
interface J extends I {
}
@Value
public class A implements J {
String test;
}
"""
)
);

}
Expand Down

0 comments on commit dfbee69

Please sign in to comment.