Skip to content

Commit b4d6cb6

Browse files
NapalysNapalys
authored
Merge pull request #20178 from Napalys/java/visible-for-testing-abuse
Java: Added new query `java/visible-for-testing-abuse`
2 parents bafe22c + 6132900 commit b4d6cb6

File tree

14 files changed

+417
-0
lines changed

14 files changed

+417
-0
lines changed

‎java/ql/integration-tests/java/query-suite/java-code-quality-extended.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ ql/java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalR
7373
ql/java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql
7474
ql/java/ql/src/Violations of Best Practice/Implementation Hiding/AbstractToConcreteCollection.ql
7575
ql/java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql
76+
ql/java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
7677
ql/java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql
7778
ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingMethodNames.ql
7879
ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

‎java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ ql/java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalR
7171
ql/java/ql/src/Violations of Best Practice/Exception Handling/NumberFormatException.ql
7272
ql/java/ql/src/Violations of Best Practice/Implementation Hiding/AbstractToConcreteCollection.ql
7373
ql/java/ql/src/Violations of Best Practice/Implementation Hiding/ExposeRepresentation.ql
74+
ql/java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
7475
ql/java/ql/src/Violations of Best Practice/Naming Conventions/AmbiguousOuterSuper.ql
7576
ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingMethodNames.ql
7677
ql/java/ql/src/Violations of Best Practice/Naming Conventions/ConfusingOverloading.ql

‎java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
## Overview
2+
3+
Accessing class members annotated with `@VisibleForTesting` from production code goes against the intention of the annotation and may indicate programmer error.
4+
5+
The `@VisibleForTesting` annotation serves to increase visibility of methods, fields or classes for the purposes of testing. Accessing these annotated elements in production code (not test code) abuses the intention of the annotation.
6+
7+
## Recommendation
8+
9+
Only access methods, fields or classes annotated with `@VisibleForTesting` from test code. If the visibility of the methods, fields or classes should generally be relaxed, use Java language access modifiers.
10+
11+
## Example
12+
13+
```java
14+
public class Annotated {
15+
@VisibleForTesting static int f() { return 42; }
16+
}
17+
18+
/* src/test/java/Test.java */
19+
int i = Annotated.f(); // COMPLIANT
20+
21+
/* src/main/Source.java */
22+
int i = Annotated.f(); // NON_COMPLIANT
23+
```
24+
25+
## Implementation notes
26+
27+
This rule alerts on any implementation of the annotation `VisibleForTesting`, regardless of where it is provided from.
28+
29+
The rule also uses the following logic to determine what an abuse of the annotation is:
30+
31+
1. If a public or protected member/type is annotated with `@VisibleForTesting`, it's assumed that package-private access is enough for production code. Therefore the rule alerts when a public or protected member/type annotated with `@VisibleForTesting` is used outside of its declaring package.
32+
2. If a package-private member/type is annotated with `@VisibleForTesting`, it's assumed that private access is enough for production code. Therefore the rule alerts when a package-private member/type annotated with `@VisibleForTesting` is used outside its declaring class.
33+
34+
## References
35+
- Javadoc: [AssertJ VisibleForTesting](https://javadoc.io/doc/org.assertj/assertj-core/latest/org.assertj.core/org/assertj/core/util/VisibleForTesting.html).
36+
- Javadoc: [JetBrains VisibleForTesting](https://javadoc.io/doc/org.jetbrains/annotations/22.0.0/org/jetbrains/annotations/VisibleForTesting.html).

‎java/ql/src/Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
/**
2+
* @id java/visible-for-testing-abuse
3+
* @name Use of VisibleForTesting in production code
4+
* @description Accessing methods, fields or classes annotated with `@VisibleForTesting` from
5+
* production code goes against the intention of the annotation and may indicate
6+
* programmer error.
7+
* @kind problem
8+
* @precision high
9+
* @problem.severity warning
10+
* @tags quality
11+
* maintainability
12+
* readability
13+
*/
14+
15+
import java
16+
17+
/**
18+
* Holds if a `Callable` is within the same type hierarchy as `RefType`
19+
* (including through lambdas, inner classes, and outer classes).
20+
*/
21+
predicate isWithinType(Callable c, RefType t) {
22+
// Either the callable is in the target type, or they share a common enclosing type
23+
c.getDeclaringType().getEnclosingType*() = t.getEnclosingType*()
24+
}
25+
26+
/**
27+
* Holds if `e` is within the same package as `t`.
28+
*/
29+
predicate isWithinPackage(Expr e, RefType t) {
30+
e.getCompilationUnit().getPackage() = t.getPackage()
31+
}
32+
33+
/**
34+
* Holds if a callable or any of its enclosing callables is annotated with @VisibleForTesting.
35+
*/
36+
predicate isWithinVisibleForTestingContext(Callable c) {
37+
c.getAnAnnotation().getType().hasName("VisibleForTesting")
38+
or
39+
isWithinVisibleForTestingContext(c.getEnclosingCallable())
40+
}
41+
42+
/**
43+
* Holds if `e` is within a test method context, including lambda expressions
44+
* within test methods and nested lambdas.
45+
*/
46+
private predicate isWithinTest(Expr e) {
47+
e.getEnclosingCallable() instanceof LikelyTestMethod
48+
or
49+
exists(Method lambda, LambdaExpr lambdaExpr |
50+
lambda = lambdaExpr.asMethod() and
51+
lambda.getEnclosingCallable*() instanceof LikelyTestMethod and
52+
e.getEnclosingCallable() = lambda
53+
)
54+
}
55+
56+
from Annotatable annotated, Expr e
57+
where
58+
annotated.getAnAnnotation().getType().hasName("VisibleForTesting") and
59+
(
60+
// field access
61+
e =
62+
any(FieldAccess v |
63+
v.getField() = annotated and
64+
// depending on the visibility of the field, using the annotation to abuse the visibility may/may not be occurring
65+
(
66+
// if its package protected report when its used outside its class because it should have been private (class only permitted)
67+
v.getField().isPackageProtected() and
68+
not isWithinType(v.getEnclosingCallable(), v.getField().getDeclaringType())
69+
or
70+
// if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
71+
(v.getField().isPublic() or v.getField().isProtected()) and
72+
not isWithinPackage(v, v.getField().getDeclaringType())
73+
)
74+
)
75+
or
76+
// method access
77+
e =
78+
any(MethodCall c |
79+
c.getMethod() = annotated and
80+
// depending on the visibility of the method, using the annotation to abuse the visibility may/may not be occurring
81+
(
82+
// if its package protected report when its used outside its class because it should have been private (class only permitted)
83+
c.getMethod().isPackageProtected() and
84+
not isWithinType(c.getEnclosingCallable(), c.getMethod().getDeclaringType())
85+
or
86+
// if public or protected report when its used outside its package because package protected should have been enough (package only permitted)
87+
(c.getMethod().isPublic() or c.getMethod().isProtected()) and
88+
not isWithinPackage(c, c.getMethod().getDeclaringType())
89+
)
90+
)
91+
or
92+
// Class instantiation - report if used outside appropriate scope
93+
e =
94+
any(ClassInstanceExpr c |
95+
c.getConstructedType() = annotated and
96+
(
97+
c.getConstructedType().isPublic() and not isWithinPackage(c, c.getConstructedType())
98+
or
99+
c.getConstructedType().hasNoModifier() and
100+
c.getConstructedType() instanceof NestedClass and
101+
not isWithinType(c.getEnclosingCallable(), c.getConstructedType())
102+
)
103+
)
104+
) and
105+
// not in a test where use is appropriate
106+
not isWithinTest(e) and
107+
// not when the accessing method or any enclosing method is @VisibleForTesting (test-to-test communication)
108+
not isWithinVisibleForTestingContext(e.getEnclosingCallable()) and
109+
// not when used in annotation contexts
110+
not e.getParent*() instanceof Annotation
111+
select e, "Access of $@ annotated with VisibleForTesting found in production code.", annotated,
112+
"element"

‎java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.expected

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
| packageone/SourcePackage.java:9:21:9:32 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
2+
| packageone/SourcePackage.java:10:21:10:32 | Annotated.m2 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:11:26:11:27 | m2 | element |
3+
| packageone/SourcePackage.java:16:18:16:36 | fPublic(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:26:23:26:29 | fPublic | element |
4+
| packageone/SourcePackage.java:17:18:17:39 | fProtected(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:31:26:31:35 | fProtected | element |
5+
| packageone/SourcePackage.java:25:31:25:42 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
6+
| packageone/SourcePackage.java:26:31:26:42 | Annotated.m2 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:11:26:11:27 | m2 | element |
7+
| packageone/SourcePackage.java:29:28:29:46 | fPublic(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:26:23:26:29 | fPublic | element |
8+
| packageone/SourcePackage.java:30:28:30:49 | fProtected(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:31:26:31:35 | fProtected | element |
9+
| packageone/SourcePackage.java:34:23:34:34 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
10+
| packageone/SourcePackage.java:35:30:35:41 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
11+
| packageone/SourcePackage.java:36:31:36:42 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
12+
| packageone/SourcePackage.java:37:33:37:44 | Annotated.m1 | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:9:29:9:30 | m1 | element |
13+
| packagetwo/Source.java:8:20:8:30 | Annotated.m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
14+
| packagetwo/Source.java:14:17:14:29 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |
15+
| packagetwo/Source.java:20:28:20:47 | new AnnotatedClass(...) | Access of $@ annotated with VisibleForTesting found in production code. | packageone/AnnotatedClass.java:4:14:4:27 | AnnotatedClass | element |
16+
| packagetwo/Source.java:24:30:24:40 | Annotated.m | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:7:19:7:19 | m | element |
17+
| packagetwo/Source.java:28:27:28:39 | f(...) | Access of $@ annotated with VisibleForTesting found in production code. | packagetwo/Annotated.java:16:16:16:16 | f | element |

‎java/ql/test/query-tests/VisibleForTestingAbuse/VisibleForTestingAbuse.qlref

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Violations of Best Practice/Implementation Hiding/VisibleForTestingAbuse.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎java/ql/test/query-tests/VisibleForTestingAbuse/packageone/AnnotatedClass.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
package packageone;
2+
3+
@VisibleForTesting
4+
public class AnnotatedClass {
5+
public AnnotatedClass() {}
6+
}

‎java/ql/test/query-tests/VisibleForTestingAbuse/packageone/SourcePackage.java

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
package packageone;
2+
3+
import packagetwo.Annotated;
4+
5+
public class SourcePackage extends Annotated {
6+
void f() {
7+
// Fields - cross-package access (only accessible ones)
8+
// String s = Annotated.m; // Cannot access package-private from different package
9+
String s1 = Annotated.m1; // $ Alert
10+
String s2 = Annotated.m2; // $ Alert
11+
// String s3 = Annotated.m3; // Cannot access private field
12+
13+
// Methods - cross-package access (only accessible ones)
14+
// int i = Annotated.f(); // Cannot access package-private from different package
15+
// int i1 = Annotated.fPrivate(); // Cannot access private method
16+
int i2 = Annotated.fPublic(); // $ Alert
17+
int i3 = Annotated.fProtected(); // $ Alert
18+
19+
// Same package class
20+
AnnotatedClass a = new AnnotatedClass(); // COMPLIANT - same package
21+
22+
// Lambda usage - cross-package (only accessible members)
23+
Runnable lambda = () -> {
24+
// String lambdaS = Annotated.m; // Cannot access package-private
25+
String lambdaS1 = Annotated.m1; // $ Alert
26+
String lambdaS2 = Annotated.m2; // $ Alert
27+
28+
// int lambdaI = Annotated.f(); // Cannot access package-private
29+
int lambdaI2 = Annotated.fPublic(); // $ Alert
30+
int lambdaI3 = Annotated.fProtected(); // $ Alert
31+
};
32+
lambda.run();
33+
}
34+
String myField1 = Annotated.m1; // $ Alert
35+
public String myField2 = Annotated.m1; // $ Alert
36+
private String myField3 = Annotated.m1; // $ Alert
37+
protected String myField4 = Annotated.m1; // $ Alert
38+
}

‎java/ql/test/query-tests/VisibleForTestingAbuse/packageone/SourcePackage1.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package packageone;
2+
3+
import packagetwo.Annotated;
4+
5+
public class SourcePackage1 extends Annotated {
6+
@VisibleForTesting
7+
public void f() {
8+
9+
String s1 = Annotated.m1;
10+
String s2 = Annotated.m2;
11+
12+
int i2 = Annotated.fPublic();
13+
int i3 = Annotated.fProtected();
14+
15+
Runnable lambda = () -> {
16+
String lambdaS1 = Annotated.m1;
17+
String lambdaS2 = Annotated.m2;
18+
int lambdaI2 = Annotated.fPublic();
19+
int lambdaI3 = Annotated.fProtected();
20+
};
21+
}
22+
}

‎java/ql/test/query-tests/VisibleForTestingAbuse/packageone/VisibleForTesting.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
package packageone;
2+
3+
public @interface VisibleForTesting {
4+
}

0 commit comments

Comments
 (0)