diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringMyObject.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringMyObject.java new file mode 100644 index 00000000000..195cbd5fd4e --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringMyObject.java @@ -0,0 +1,14 @@ +package checks.VisibleForTestingProtectedUsageCheck.CheckOtherwiseString; + +public class IssueStringMyObject { + + IssueStringMyObject() { + } + + @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED) + String bar; + + @VisibleForTesting(otherwise = "3") + String foo; + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringService.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringService.java new file mode 100644 index 00000000000..301e85b8966 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringService.java @@ -0,0 +1,13 @@ +package checks.VisibleForTestingProtectedUsageCheck.CheckOtherwiseString; + +public class IssueStringService { + + public String test() { + return new IssueStringMyObject().foo; // Noncompliant {{Remove this usage of "foo", it is annotated with @VisibleForTesting and should not be accessed from production code.}} + } + + public String test2() { + return new IssueStringMyObject().bar; // Noncompliant {{Remove this usage of "bar", it is annotated with @VisibleForTesting and should not be accessed from production code.}} + } + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/VisibleForTesting.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/VisibleForTesting.java new file mode 100644 index 00000000000..70369aa0b06 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/VisibleForTesting.java @@ -0,0 +1,16 @@ +package checks.VisibleForTestingProtectedUsageCheck.CheckOtherwiseString; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.CLASS; + +@Retention(value = CLASS) +@Target(value = {TYPE, METHOD, FIELD}) +public @interface VisibleForTesting { + String otherwise(); + String PROTECTED = "4"; +} diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueMyObject.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueMyObject.java new file mode 100644 index 00000000000..22a81fc0e7e --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueMyObject.java @@ -0,0 +1,17 @@ +package checks.VisibleForTestingProtectedUsageCheck; + +public class IssueMyObject { + + IssueMyObject() { + } + + @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED, othertestcase=1, othertypecase="S") + String bar; + + // androidx.annotation.VisibleForTesting + // @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED) + // where VisibleForTesting.PROTECTED = 4 + @VisibleForTesting(otherwise = 3, othertestcase=1, othertypecase="F") + String foo; + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueService.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueService.java new file mode 100644 index 00000000000..fe72a98c0a1 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/IssueService.java @@ -0,0 +1,13 @@ +package checks.VisibleForTestingProtectedUsageCheck; + +public class IssueService { + + public String test() { + return new IssueMyObject().foo; // Noncompliant {{Remove this usage of "foo", it is annotated with @VisibleForTesting and should not be accessed from production code.}} + } + + public String test2() { + return new IssueMyObject().bar; // Compliant {{bar has valid otherwise = VisibleForTesting.PROTECTED set so can be used here}} + } + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/MyObject.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/MyObject.java new file mode 100644 index 00000000000..4a6c01e830c --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/MyObject.java @@ -0,0 +1,30 @@ +package checks.VisibleForTestingProtectedUsageCheck; + +public class MyObject { + + MyObject() {} + + // androidx.annotation.VisibleForTesting + // @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED) + // where VisibleForTesting.PROTECTED = 4 + @VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F") + String foo; + + @VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F") + int answer() { + return 42; + } + + int answer(int result) { + return result; + } + + @VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F") + class Nested {} +} + +@VisibleForTesting(otherwise = 4, othertestcase=1, othertypecase="F") +class Outer {} + + + diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/Service.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/Service.java new file mode 100644 index 00000000000..33882ba3528 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/Service.java @@ -0,0 +1,45 @@ +package checks.VisibleForTestingProtectedUsageCheck; + +import com.google.common.annotations.VisibleForTesting; + +public class Service { + + public String f(int param) { + + TestOnly testOnly = null; // Compliant, TestOnly class visible if it is private + + MyObject.Nested nested = null; // Noncompliant {{Remove this usage of "Nested", it is annotated with @VisibleForTesting and should not be accessed from production code.}} + + Outer outer = null; // Noncompliant {{Remove this usage of "Outer", it is annotated with @VisibleForTesting and should not be accessed from production code.}} + + String foo = new MyObj().bar; // False negative MyObj and Service are in the same file but if 'bar' is private it wouldn't be visible here) + + String bar = new MyObj().bar; + + return new MyObject().foo; // Already reported in line 21 + + } + + public int g(@Deprecated int param) { + MyObject myObject = new MyObject(); + + myObject.answer(123); // Compliant, no annotation + myObject.answer(param); // Compliant, no annotation + + return myObject.answer(); // Noncompliant {{Remove this usage of "answer", it is annotated with @VisibleForTesting and should not be accessed from production code.}} + } + +} + +class MyObj { + @VisibleForTesting + String bar; +} + + +@VisibleForTesting +class TestOnly { + +} + + diff --git a/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/VisibleForTesting.java b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/VisibleForTesting.java new file mode 100644 index 00000000000..8113f1e1e73 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/VisibleForTestingProtectedUsageCheck/VisibleForTesting.java @@ -0,0 +1,18 @@ +package checks.VisibleForTestingProtectedUsageCheck; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.CLASS; + +@Retention(value = CLASS) +@Target(value = {TYPE, METHOD, FIELD}) +public @interface VisibleForTesting { + int otherwise(); + int othertestcase(); + String othertypecase(); + int PROTECTED = 4; +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/VisibleForTestingUsageCheck.java b/java-checks/src/main/java/org/sonar/java/checks/VisibleForTestingUsageCheck.java index 20d44464699..fb936b8b572 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/VisibleForTestingUsageCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/VisibleForTestingUsageCheck.java @@ -28,6 +28,7 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.SymbolMetadata; import org.sonar.plugins.java.api.semantic.SymbolMetadata.AnnotationInstance; import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.Tree; @@ -70,7 +71,21 @@ private static boolean isMisusedVisibleForTesting(Symbol symbol) { } private static boolean isVisibleForTestingAnnotation(AnnotationInstance annotationInstance) { - return "VisibleForTesting".equals(annotationInstance.symbol().name()); + return "VisibleForTesting".equals(annotationInstance.symbol().name()) + && !isOtherwiseProtected(annotationInstance); + } + + private static boolean isOtherwiseProtected(AnnotationInstance annotationInstance) { + List values = annotationInstance.values(); + for (SymbolMetadata.AnnotationValue value : values) { + // Note: constant to support is androidx.annotation.VisibleForTesting.PROTECTED=4 + if ("otherwise".equals(value.name()) && + value.value() instanceof Integer && + Integer.valueOf(4).equals(value.value())) { + return true; + } + } + return false; } private static boolean inTheSameFile(Symbol symbol) { diff --git a/java-checks/src/test/java/org/sonar/java/checks/VisibleForTestingUsageCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/VisibleForTestingUsageCheckTest.java index 6f4e3acf5dc..c27ff4e44e4 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/VisibleForTestingUsageCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/VisibleForTestingUsageCheckTest.java @@ -34,6 +34,29 @@ void test() { .verifyIssues(); } + @Test + void test_protected() { + // no issues + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/VisibleForTestingProtectedUsageCheck/Service.java")) + .withCheck(new VisibleForTestingUsageCheck()) + .verifyNoIssues(); + // issues + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/VisibleForTestingProtectedUsageCheck/IssueService.java")) + .withCheck(new VisibleForTestingUsageCheck()) + .verifyIssues(); + } + + @Test + void test_protected_string() { + // issues + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/VisibleForTestingProtectedUsageCheck/CheckOtherwiseString/IssueStringService.java")) + .withCheck(new VisibleForTestingUsageCheck()) + .verifyIssues(); + } + @Test void test_no_semantic() { CheckVerifier.newVerifier()