Skip to content

Commit df311e9

Browse files
pdelagravepdelagrave
committed
added more tests
implemented more
1 parent 8f6ddbf commit df311e9

File tree

2 files changed

+81
-12
lines changed

2 files changed

+81
-12
lines changed

‎src/main/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionals.java‎

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import lombok.Value;
2020
import org.jspecify.annotations.Nullable;
2121
import org.openrewrite.*;
22-
import org.openrewrite.internal.ListUtils;
2322
import org.openrewrite.internal.StringUtils;
2423
import org.openrewrite.java.*;
2524
import org.openrewrite.java.search.UsesMethod;
@@ -29,7 +28,6 @@
2928
import java.util.*;
3029
import java.util.function.Function;
3130
import java.util.stream.Collectors;
32-
import java.util.stream.Stream;
3331

3432
import static java.util.Collections.emptyList;
3533
import static org.openrewrite.Preconditions.or;
@@ -118,18 +116,70 @@ private boolean isInIfStatementWithLogLevelCheck(Cursor cursor, J.MethodInvocati
118116
}
119117

120118
private boolean isAnyArgumentExpensive(J.MethodInvocation m) {
121-
return !m
119+
return !areAllArgumentsCheap(m);
120+
}
121+
122+
private static boolean areAllArgumentsCheap(J.MethodInvocation m) {
123+
return m
122124
.getArguments()
123125
.stream()
124126
.allMatch(
125127
arg ->
126-
(arg instanceof J.MethodInvocation && isSimpleGetter(arg)) ||
127-
arg instanceof J.Literal
128+
(arg instanceof J.MethodInvocation && isSimpleGetter((J.MethodInvocation) arg)) ||
129+
arg instanceof J.Literal ||
130+
arg instanceof J.Identifier ||
131+
(arg instanceof J.Binary && isOnlyLiterals((J.Binary) arg))
132+
);
133+
}
134+
135+
private static boolean isSimpleGetter(J.MethodInvocation mi) {
136+
return (
137+
(mi.getSimpleName().startsWith("get") && mi.getSimpleName().length() > 3) ||
138+
(mi.getSimpleName().startsWith("is") && mi.getSimpleName().length() > 2)
139+
) &&
140+
mi.getMethodType().getParameterNames().isEmpty() &&
141+
(
142+
(mi.getSelect() == null || mi.getSelect() instanceof J.Identifier) &&
143+
!mi.getMethodType().hasFlags(Flag.Static)
144+
);
145+
}
146+
147+
private static boolean isOnlyLiterals(J.Binary binary) {
148+
return isLiteralOrBinary(binary.getLeft()) &&
149+
isLiteralOrBinary(binary.getRight());
150+
}
151+
152+
private static boolean isLiteralOrBinary(J expression) {
153+
if (expression instanceof J.Literal || isSimpleBooleanGetter(expression) || isBooleanIdentifier(expression)) {
154+
return true;
155+
} else if (expression instanceof J.Binary) {
156+
return isOnlyLiterals((J.Binary) expression);
157+
} else {
158+
return false;
159+
}
128160
}
129161

130-
private boolean isSimpleGetter(Expression arg) {
162+
private static boolean isSimpleBooleanGetter(J expression) {
163+
if (expression instanceof J.MethodInvocation) {
164+
J.MethodInvocation mi = (J.MethodInvocation) expression;
165+
if (isSimpleGetter(mi) && mi.getMethodType() != null) {
166+
JavaType returnType = mi.getMethodType().getReturnType();
167+
if (returnType == JavaType.Primitive.Boolean || (returnType instanceof JavaType.FullyQualified && "java.lang.Boolean".equals(((JavaType.FullyQualified) returnType).getFullyQualifiedName()))) {
168+
return true;
169+
}
170+
}
171+
}
131172
return false;
132173
}
174+
private static boolean isBooleanIdentifier(J expression) {
175+
if (expression instanceof J.Identifier) {
176+
J.Identifier identifier = (J.Identifier) expression;
177+
JavaType type = identifier.getType();
178+
return type != null && (type == JavaType.Primitive.Boolean || (type instanceof JavaType.FullyQualified && "java.lang.Boolean".equals(((JavaType.FullyQualified) type).getFullyQualifiedName())));
179+
}
180+
return false;
181+
}
182+
133183
}
134184

135185
@EqualsAndHashCode(callSuper = false)

‎src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsTest.java‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -595,7 +595,10 @@ private static Stream<Arguments> wrapWhenExpensiveArgument() {
595595
Arguments.of("getClass().getName()"), // getter on a method invocation expression
596596
Arguments.of("optional.get()"), // not a getter
597597
Arguments.of("A.getExpensive()"), // static getter likely to use external resources or allocate things
598-
Arguments.of("getExpensive()") // static getter likely to use external resources or allocate things
598+
Arguments.of("getExpensive()"), // static getter likely to use external resources or allocate things
599+
Arguments.of("342 + input"), // allocating a new string
600+
Arguments.of("\"foo\" + getClass()"), // allocating a new string
601+
Arguments.of("true && isSomething(1)")
599602
);
600603
}
601604

@@ -611,7 +614,7 @@ void wrapWhenExpensiveArgument(String logArgument) {
611614
import org.slf4j.Logger;
612615
613616
class A {
614-
void method(Logger log, String input, Optional<String> optional) {
617+
void method(Logger log, String input, Optional<String> optional, boolean boolVariable) {
615618
log.info("{}", %s);
616619
}
617620
@@ -622,6 +625,10 @@ String notAGetter() {
622625
static String getExpensive() {
623626
return "expensive";
624627
}
628+
629+
boolean isSomething(int i) {
630+
return true;
631+
}
625632
}
626633
""", logArgument),
627634
String.format("""
@@ -630,7 +637,7 @@ static String getExpensive() {
630637
import org.slf4j.Logger;
631638
632639
class A {
633-
void method(Logger log, String input, Optional<String> optional) {
640+
void method(Logger log, String input, Optional<String> optional, boolean boolVariable) {
634641
if (log.isInfoEnabled()) {
635642
log.info("{}", %s);
636643
}
@@ -643,6 +650,10 @@ String notAGetter() {
643650
static String getExpensive() {
644651
return "expensive";
645652
}
653+
654+
boolean isSomething(int i) {
655+
return true;
656+
}
646657
}
647658
""", logArgument)
648659
)
@@ -657,8 +668,12 @@ private static Stream<Arguments> dontWrapWhenCheapArgument() {
657668
Arguments.of("log.getName()"), // a getter
658669
Arguments.of("34 + 78"), // literal
659670
Arguments.of("8344"), // literal
660-
Arguments.of("\"like, literally!\"") // literal
661-
// add boolean expression composed of variables/contants only (and a negative test of it too)
671+
Arguments.of("\"like, literally!\""), // literal
672+
Arguments.of("\"one\" + \"two\" + \"three\""), // compile time literal
673+
Arguments.of("\"one\" + 1"), // compile time literal
674+
Arguments.of("true && false"), // boolean literal
675+
Arguments.of("true && isSomething()"), // boolean literal and boolean getter
676+
Arguments.of("true && boolVariable || isSomething()") // boolean literal and boolean variable
662677
);
663678
}
664679

@@ -672,9 +687,13 @@ void dontWrapWhenCheapArgument(String logArgument) {
672687
import org.slf4j.Logger;
673688
674689
class A {
675-
void method(Logger log, String input) {
690+
void method(Logger log, String input, boolean boolVariable) {
676691
log.info("{}", %s);
677692
}
693+
694+
boolean isSomething() {
695+
return true;
696+
}
678697
}
679698
""", logArgument)
680699
)

0 commit comments

Comments
 (0)