A detector to ensure that implementation (class in src/main/java) doesn't call @@ -29,7 +30,10 @@ * @see com.google.common.annotations.VisibleForTesting */ public class UnexpectedAccessDetector extends BytecodeScanningDetector { - private static final String VISIBLE_FOR_TESTING_ANNOTATION_TYPE = "Lcom/google/common/annotations/VisibleForTesting;"; + + private static final String GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING = "GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING"; + @VisibleForTesting + static final String VISIBLE_FOR_TESTING_ANNOTATION = "com/google/common/annotations/VisibleForTesting:{}"; @Nonnull private final BugReporter bugReporter; @@ -44,81 +48,84 @@ public UnexpectedAccessDetector(final @Nonnull BugReporter bugReporter) { } @Override - public void sawOpcode(int opcode) { - if (!isInvoking(opcode)) { - return; + public void sawOpcode(final int opcode) { + if (isWorkingOnAField(opcode)) { + final XField invokedField = XFactory.createReferencedXField(this); + final ClassDescriptor currentClass = getClassDescriptor(); + if (hasToCheck(invokedField, currentClass)) { + checkField(invokedField); + } } - - final ClassDescriptor currentClass = getClassDescriptor(); - final ClassDescriptor invokedClass = getClassDescriptorOperand(); - if (currentClass.equals(invokedClass)) { - // no need to check, because method is called by owner - } else if (!currentClass.getPackageName().equals(invokedClass.getPackageName())) { - // no need to check, because method is called by class in other package - } else { - final MethodDescriptor invokedMethod = getMethodDescriptorOperand(); - try { - verifyVisibility(invokedClass, invokedMethod, true); - } catch (ClassNotFoundException e) { - final String message = String.format("Detector could not find %s, you should add this class into CLASSPATH", invokedClass.getDottedClassName()); - bugReporter.logError(message, e); + if (isInvoking(opcode)) { + final XMethod invokedMethod = XFactory.createReferencedXMethod(this); + final ClassDescriptor currentClass = getClassDescriptor(); + if (hasToCheck(invokedMethod, currentClass)) { + checkMethod(invokedMethod); } } } /** - *
Report if specified method is package-private and annotated by {@code @VisibleForTesting}.
+ * @param member + * @param currentClass + * @return true, if the member is called within the same package */ - @VisibleForTesting - void verifyVisibility(final @Nonnull ClassDescriptor invokedClass, final @Nonnull MethodDescriptor invokedMethod, boolean reportCaller) throws ClassNotFoundException { - final JavaClass bcelClass = Repository.getRepository().loadClass(invokedClass.getDottedClassName()); - final Method bcelMethod = findMethod(bcelClass, invokedMethod); - - if (isIllegalAccesDetected(bcelMethod)) { - final BugInstance bug = new BugInstance(this, "GUAVA_UNEXPECTED_ACCESS_TO_VISIBLE_FOR_TESTING", HIGH_PRIORITY); - if (reportCaller) { - bug.addCalledMethod(this).addClassAndMethod(this).addSourceLine(this); - } - bugReporter.reportBug(bug); + private boolean hasToCheck(final @Nonnull ClassMember member, final @Nonnull ClassDescriptor currentClass) { + if (currentClass.equals(member.getClassDescriptor())) { + log.debug("No check: " + member.getName() + " is called within the same class: " + currentClass.getClassName()); + // no need to check, because method is called by owner + return false; + } else if (!member.getPackageName().equals(currentClass.getPackageName())) { + log.debug("No check: " + member.getName() + " is called from outside the its package. Member package: " // + + member.getPackageName() + ". Caller package: " + currentClass.getPackageName()); + // no need to check, because method is called by class in other package + return false; } + return true; } - private boolean isIllegalAccesDetected(final @Nullable Method bcelMethod) { - return bcelMethod != null && checkVisibility(bcelMethod) && checkAnnotated(bcelMethod); + @VisibleForTesting + void checkMethod(final @Nonnull XMethod invokedMethod) { + if (checkVisibility(invokedMethod)) { + checkAnnotations(invokedMethod.getAnnotations(), true); + } else { + log.debug("No check: Method " + invokedMethod.getName() + " it not package visible."); + } } @VisibleForTesting - Method findMethod(final @Nonnull JavaClass bcelClass, final @Nonnull MethodDescriptor invokedMethod) { - for (Method bcelMethod : bcelClass.getMethods()) { - MethodDescriptor methodDescriptor = BCELUtil.getMethodDescriptor(bcelClass, bcelMethod); - if (methodDescriptor.equals(invokedMethod)) { - return bcelMethod; - } + void checkField(final @Nonnull XField invokedField) { + if (checkVisibility(invokedField)) { + checkAnnotations(invokedField.getAnnotations(), true); + } else { + log.debug("No check: Field " + invokedField.getName() + " it not package visible."); } - log.warn("Method not found: " + invokedMethod); - return null; } /** - * @return true if visibility of specified method is package-private. + * checks a collection of annotations for VisibleForTesting + * @param annotations from method or field + * @param reportCaller should report be added to {@link BugInstance}, for testing */ @VisibleForTesting - boolean checkVisibility(final @Nonnull Method bcelMethod) { - return !(bcelMethod.isPrivate() || bcelMethod.isProtected() || bcelMethod.isPublic()); + void checkAnnotations(final CollectionYou must not access to package-private method or field which is annotated by @VisibleForTesting.
This annotation means that visibility was widened only for test code, so your implementation code shouldn't access to method which is annotated by this annotation.
]]> diff --git a/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java b/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java index 10b246f..f012bbe 100644 --- a/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java +++ b/src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java @@ -1,45 +1,34 @@ package jp.co.worksap.oss.findbugs.guava; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyByte; -import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; +import java.util.Arrays; + import org.apache.bcel.Constants; -import org.apache.bcel.classfile.AnnotationEntry; -import org.apache.bcel.classfile.Attribute; -import org.apache.bcel.classfile.Constant; -import org.apache.bcel.classfile.ConstantPool; -import org.apache.bcel.classfile.ConstantUtf8; -import org.apache.bcel.classfile.JavaClass; -import org.apache.bcel.classfile.Method; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; -import org.objectweb.asm.Opcodes; - -import com.google.common.annotations.VisibleForTesting; import edu.umd.cs.findbugs.BugInstance; import edu.umd.cs.findbugs.BugReporter; -import edu.umd.cs.findbugs.classfile.ClassDescriptor; -import edu.umd.cs.findbugs.classfile.DescriptorFactory; -import edu.umd.cs.findbugs.classfile.MethodDescriptor; - +import edu.umd.cs.findbugs.ba.AccessibleEntity; +import edu.umd.cs.findbugs.ba.XField; +import edu.umd.cs.findbugs.ba.XMethod; +import edu.umd.cs.findbugs.classfile.analysis.AnnotationValue; /** * @author tolina GmbH - * */ public class UnexpectedAccessDetectorTest { - @Mock private BugReporter bugReporter; @@ -52,59 +41,75 @@ public void initMocks() { } /** - * Tests reporting a Bug when: - * - called Method has 'default'-visibility - * - called Method is annotated with {@link VisibleForTesting} - * @throws Exception + * Checks if the visibility is checked and the annotations are loaded. */ @Test - public void testVerifyVisibility() throws Exception { - UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - ClassDescriptor invokedClassDescriptor; - MethodDescriptor invokedMethodDescriptor; + public void checkMethod() throws Exception { + final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); + final XMethod invokedMethod = mock(XMethod.class); - invokedClassDescriptor = DescriptorFactory.createClassDescriptor(Object.class); - invokedMethodDescriptor = DescriptorFactory.instance().getMethodDescriptor("java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false); - detector.verifyVisibility(invokedClassDescriptor, invokedMethodDescriptor, false); + when(invokedMethod.isPrivate()).thenReturn(true); + when(invokedMethod.isProtected()).thenReturn(false); + when(invokedMethod.isPublic()).thenReturn(false); - invokedClassDescriptor = DescriptorFactory.createClassDescriptor(TestClass.class); - invokedMethodDescriptor = DescriptorFactory.instance().getMethodDescriptor("jp/co/worksap/oss/findbugs/guava/TestClass", "test", "()V", false); + detector.checkMethod(invokedMethod); - detector.verifyVisibility(invokedClassDescriptor, invokedMethodDescriptor, false); - verify(bugReporter).reportBug(any(BugInstance.class)); + when(invokedMethod.isPrivate()).thenReturn(false); + when(invokedMethod.isProtected()).thenReturn(true); + when(invokedMethod.isPublic()).thenReturn(false); + + detector.checkMethod(invokedMethod); + + when(invokedMethod.isPrivate()).thenReturn(false); + when(invokedMethod.isProtected()).thenReturn(false); + when(invokedMethod.isPublic()).thenReturn(true); + + detector.checkMethod(invokedMethod); + + when(invokedMethod.isPrivate()).thenReturn(false); + when(invokedMethod.isProtected()).thenReturn(false); + when(invokedMethod.isPublic()).thenReturn(false); + + detector.checkMethod(invokedMethod); + + // This method should only be invoked when it is package protected + verify(invokedMethod, times(1)).getAnnotations(); } /** - * Tests searching for {@link Method}s of a {@link JavaClass} + * Checks if the visibility is checked and the annotations are loaded. */ @Test - public void testFindMethod() { - UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - ConstantPool constant_pool = mock(ConstantPool.class); - JavaClass bcelClass = mock(JavaClass.class); + public void checkField() throws Exception { + final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); + final XField invokedField = mock(XField.class); - Method testMethod = new Method(); - int name_index = 0; - int signature_index = 1; - testMethod.setNameIndex(name_index); - testMethod.setSignatureIndex(signature_index); - testMethod.setConstantPool(constant_pool); - Constant nameConstant = new ConstantUtf8("equals"); - Constant signatureConstant = new ConstantUtf8("(Ljava/lang/Object;)Z"); + when(invokedField.isPrivate()).thenReturn(true); + when(invokedField.isProtected()).thenReturn(false); + when(invokedField.isPublic()).thenReturn(false); - when(constant_pool.getConstant(eq(name_index), anyByte())).thenReturn(nameConstant); - when(constant_pool.getConstant(eq(signature_index), anyByte())).thenReturn(signatureConstant); + detector.checkField(invokedField); - Method[] methods = { testMethod }; - when(bcelClass.getMethods()).thenReturn(methods); - when(bcelClass.getClassName()).thenReturn("java.lang.Object"); + when(invokedField.isPrivate()).thenReturn(false); + when(invokedField.isProtected()).thenReturn(true); + when(invokedField.isPublic()).thenReturn(false); - MethodDescriptor invokedMethod = new MethodDescriptor("java/lang/Object", "equals", "(Ljava/lang/Object;)Z", false); // mock(MethodDescriptor.class); + detector.checkField(invokedField); - Method method = detector.findMethod(bcelClass, invokedMethod); + when(invokedField.isPrivate()).thenReturn(false); + when(invokedField.isProtected()).thenReturn(false); + when(invokedField.isPublic()).thenReturn(true); - assertNotNull(method); + detector.checkField(invokedField); + when(invokedField.isPrivate()).thenReturn(false); + when(invokedField.isProtected()).thenReturn(false); + when(invokedField.isPublic()).thenReturn(false); + + detector.checkField(invokedField); + + // This method should only be invoked when it is package protected + verify(invokedField, times(1)).getAnnotations(); } /** @@ -113,44 +118,49 @@ public void testFindMethod() { @Test public void testCheckVisibility() { UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - Method privateMethod = new Method(); - privateMethod.setModifiers(Opcodes.ACC_PRIVATE); + final AccessibleEntity privateMethod = mock(AccessibleEntity.class); + when(privateMethod.isPrivate()).thenReturn(true); assertFalse(detector.checkVisibility(privateMethod)); - Method protectedMethod = new Method(); - protectedMethod.setModifiers(Opcodes.ACC_PROTECTED); + final AccessibleEntity protectedMethod = mock(AccessibleEntity.class); + when(protectedMethod.isProtected()).thenReturn(true); assertFalse(detector.checkVisibility(protectedMethod)); - Method publicMethod = new Method(); - publicMethod.setModifiers(Opcodes.ACC_PUBLIC); + final AccessibleEntity publicMethod = mock(AccessibleEntity.class); + when(publicMethod.isPublic()).thenReturn(true); assertFalse(detector.checkVisibility(publicMethod)); - Method defaultVisibilityMethod = new Method(); - assertTrue(detector.checkVisibility(defaultVisibilityMethod)); + final AccessibleEntity defaultMethod = mock(AccessibleEntity.class); + assertTrue(detector.checkVisibility(defaultMethod)); } /** - * Checks, if detecting of annotated Methods works + * Checks, if detecting of the annotation works. */ @Test public void testCheckAnnotated() { UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); - Method method = new Method(); - method.setAttributes(new Attribute[] {}); + final XMethod method = mock(XMethod.class); + + detector.checkAnnotations(method.getAnnotations(), false); - assertFalse(detector.checkAnnotated(method)); + verifyZeroInteractions(bugReporter); - AnnotationEntry someAnnotationEntry = mock(AnnotationEntry.class); - when(someAnnotationEntry.getAnnotationType()).thenReturn("Lorg/junit/Test;"); - method.addAnnotationEntry(someAnnotationEntry); + final AnnotationValue someAnnotationEntry = mock(AnnotationValue.class); + when(someAnnotationEntry.toString()).thenReturn("some other annotaion"); + when(method.getAnnotations()).thenReturn(Arrays.asList(someAnnotationEntry)); - assertFalse(detector.checkAnnotated(method)); + detector.checkAnnotations(method.getAnnotations(), false); - AnnotationEntry visibleForTestingAnnotationEntry = mock(AnnotationEntry.class); - when(visibleForTestingAnnotationEntry.getAnnotationType()).thenReturn("Lcom/google/common/annotations/VisibleForTesting;"); - method.addAnnotationEntry(visibleForTestingAnnotationEntry); + verifyZeroInteractions(bugReporter); - assertTrue(detector.checkAnnotated(method)); + final AnnotationValue visibleForTestingAnnotationEntry = mock(AnnotationValue.class); + when(visibleForTestingAnnotationEntry.toString()).thenReturn(UnexpectedAccessDetector.VISIBLE_FOR_TESTING_ANNOTATION); + when(method.getAnnotations()).thenReturn(Arrays.asList(someAnnotationEntry, visibleForTestingAnnotationEntry)); + + detector.checkAnnotations(method.getAnnotations(), false); + + verify(bugReporter).reportBug(any(BugInstance.class)); } /** @@ -163,7 +173,15 @@ public void testIsInvoking() { assertTrue(detector.isInvoking(Constants.INVOKEINTERFACE)); assertTrue(detector.isInvoking(Constants.INVOKESTATIC)); assertTrue(detector.isInvoking(Constants.INVOKEVIRTUAL)); - } + /** + * Checks, if 'field' OP-Codes are detected + */ + @Test + public void testIsWorkingOnAField() { + UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter); + assertTrue(detector.isWorkingOnAField(Constants.PUTFIELD)); + assertTrue(detector.isWorkingOnAField(Constants.GETFIELD)); + } }