Skip to content

Commit

Permalink
Check for inner class call was invalid. Added some optimizations in v…
Browse files Browse the repository at this point in the history 
…isibility check.
  • Loading branch information
Frank Jakop committed Mar 10, 2015
1 parent bdc014d commit 6869281
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 114 deletions.
37 changes: 9 additions & 28 deletions src/main/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ public void sawOpcode(final int opcode) {
final XField invokedField = XFactory.createReferencedXField(this);
final ClassDescriptor currentClass = getClassDescriptor();
if (hasToCheck(invokedField, currentClass)) {
checkField(invokedField);
checkAnnotations(invokedField.getAnnotations(), true);
}
}
if (isInvoking(opcode)) {
final XMethod invokedMethod = XFactory.createReferencedXMethod(this);
final ClassDescriptor currentClass = getClassDescriptor();
if (hasToCheck(invokedMethod, currentClass)) {
checkMethod(invokedMethod);
checkAnnotations(invokedMethod.getAnnotations(), true);
}
}
}
Expand All @@ -70,38 +70,19 @@ public void sawOpcode(final int opcode) {
* @param currentClass
* @return true, if the member is called within the same package
*/
private boolean hasToCheck(final @Nonnull ClassMember member, final @Nonnull ClassDescriptor currentClass) {
if (currentClass.getClassName().startsWith(member.getClassDescriptor().getClassName())) {
@VisibleForTesting
boolean hasToCheck(final @Nonnull ClassMember member, final @Nonnull ClassDescriptor currentClass) {
final boolean isDefaultVisible = isDefaultVisible(member);
final boolean isSameClass = currentClass.equals(member.getClassDescriptor());
final boolean isInnerClass = currentClass.getClassName().startsWith(member.getClassDescriptor().getClassName() + "$");
if (!isDefaultVisible || isSameClass || isInnerClass) {
log.debug("No check: " + member.getName() + " is called within the same class or inner 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;
}

@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
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.");
}
}

/**
* checks a collection of annotations for VisibleForTesting
* @param annotations from method or field
Expand All @@ -124,7 +105,7 @@ void checkAnnotations(final Collection<AnnotationValue> annotations, final boole
* @return true if visibility of specified AccessibleEntity, implemented by method or field, is package-private.
*/
@VisibleForTesting
boolean checkVisibility(final @Nonnull AccessibleEntity entry) {
boolean isDefaultVisible(final @Nonnull AccessibleEntity entry) {
return !(entry.isPrivate() || entry.isProtected() || entry.isPublic());
}

Expand Down
171 changes: 85 additions & 86 deletions src/test/java/jp/co/worksap/oss/findbugs/guava/UnexpectedAccessDetectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.any;
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;
Expand All @@ -20,8 +19,9 @@
import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugReporter;
import edu.umd.cs.findbugs.ba.AccessibleEntity;
import edu.umd.cs.findbugs.ba.XField;
import edu.umd.cs.findbugs.ba.ClassMember;
import edu.umd.cs.findbugs.ba.XMethod;
import edu.umd.cs.findbugs.classfile.ClassDescriptor;
import edu.umd.cs.findbugs.classfile.analysis.AnnotationValue;

/**
Expand All @@ -40,106 +40,91 @@ public void initMocks() {
MockitoAnnotations.initMocks(this);
}

/**
* Checks if the visibility is checked and the annotations are loaded.
*/
@Test
public void checkMethod() throws Exception {
final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);
final XMethod invokedMethod = mock(XMethod.class);

when(invokedMethod.isPrivate()).thenReturn(true);
when(invokedMethod.isProtected()).thenReturn(false);
when(invokedMethod.isPublic()).thenReturn(false);

detector.checkMethod(invokedMethod);

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();
}

/**
* Checks if the visibility is checked and the annotations are loaded.
*/
@Test
public void checkField() throws Exception {
final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);
final XField invokedField = mock(XField.class);

when(invokedField.isPrivate()).thenReturn(true);
when(invokedField.isProtected()).thenReturn(false);
when(invokedField.isPublic()).thenReturn(false);

detector.checkField(invokedField);

when(invokedField.isPrivate()).thenReturn(false);
when(invokedField.isProtected()).thenReturn(true);
when(invokedField.isPublic()).thenReturn(false);

detector.checkField(invokedField);

when(invokedField.isPrivate()).thenReturn(false);
when(invokedField.isProtected()).thenReturn(false);
when(invokedField.isPublic()).thenReturn(true);

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();
}

/**
* Checks, if detecting of 'default'-Visibility works
*/
@Test
public void testCheckVisibility() {
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);
public void testIsDefaultVisible() {
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(
bugReporter);
final AccessibleEntity privateMethod = mock(AccessibleEntity.class);
when(privateMethod.isPrivate()).thenReturn(true);
assertFalse(detector.checkVisibility(privateMethod));
assertFalse(detector.isDefaultVisible(privateMethod));

final AccessibleEntity protectedMethod = mock(AccessibleEntity.class);
when(protectedMethod.isProtected()).thenReturn(true);
assertFalse(detector.checkVisibility(protectedMethod));
assertFalse(detector.isDefaultVisible(protectedMethod));

final AccessibleEntity publicMethod = mock(AccessibleEntity.class);
when(publicMethod.isPublic()).thenReturn(true);
assertFalse(detector.checkVisibility(publicMethod));
assertFalse(detector.isDefaultVisible(publicMethod));

final AccessibleEntity defaultMethod = mock(AccessibleEntity.class);
assertTrue(detector.checkVisibility(defaultMethod));
assertTrue(detector.isDefaultVisible(defaultMethod));
}

@Test
public void testHasToCheck_Visibility() throws Exception {
final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);

final ClassDescriptor currentClass = new TestClassDescriptor("a/b/F");
final ClassDescriptor memberClass = new TestClassDescriptor("a/b/C");

ClassMember entity = mock(ClassMember.class);
when(entity.getClassDescriptor()).thenReturn(memberClass);

when(entity.isPrivate()).thenReturn(true);
assertFalse(detector.hasToCheck(entity, currentClass));

entity = mock(ClassMember.class);
when(entity.isProtected()).thenReturn(true);
when(entity.getClassDescriptor()).thenReturn(memberClass);
assertFalse(detector.hasToCheck(entity, currentClass));

entity = mock(ClassMember.class);
when(entity.isPublic()).thenReturn(true);
when(entity.getClassDescriptor()).thenReturn(memberClass);
assertFalse(detector.hasToCheck(entity, currentClass));

entity = mock(ClassMember.class);
when(entity.getClassDescriptor()).thenReturn(memberClass);
assertTrue(detector.hasToCheck(entity, currentClass));
}

@Test
public void testHasToCheck_CallFromClass() throws Exception {
final UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);

ClassDescriptor currentClass = new TestClassDescriptor("a/b/C");
final ClassDescriptor memberClass = new TestClassDescriptor("a/b/C");

final ClassMember entity = mock(ClassMember.class);
when(entity.getClassDescriptor()).thenReturn(memberClass);

// same class
assertFalse(detector.hasToCheck(entity, currentClass));

// inner class of same class
currentClass= new TestClassDescriptor("a/b/C$1");
assertFalse(detector.hasToCheck(entity, currentClass));

currentClass= new TestClassDescriptor("a/b/Cxy");
// different class in same package, name starts with member class
assertTrue(detector.hasToCheck(entity, currentClass));

// different class in same package
currentClass= new TestClassDescriptor("a/b/F");
assertTrue(detector.hasToCheck(entity, currentClass));

}

/**
* Checks, if detecting of the annotation works.
*/
@Test
public void testCheckAnnotated() {
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(
bugReporter);
final XMethod method = mock(XMethod.class);

detector.checkAnnotations(method.getAnnotations(), false);
Expand All @@ -148,15 +133,19 @@ public void testCheckAnnotated() {

final AnnotationValue someAnnotationEntry = mock(AnnotationValue.class);
when(someAnnotationEntry.toString()).thenReturn("some other annotaion");
when(method.getAnnotations()).thenReturn(Arrays.asList(someAnnotationEntry));
when(method.getAnnotations()).thenReturn(
Arrays.asList(someAnnotationEntry));

detector.checkAnnotations(method.getAnnotations(), false);

verifyZeroInteractions(bugReporter);

final AnnotationValue visibleForTestingAnnotationEntry = mock(AnnotationValue.class);
when(visibleForTestingAnnotationEntry.toString()).thenReturn(UnexpectedAccessDetector.VISIBLE_FOR_TESTING_ANNOTATION);
when(method.getAnnotations()).thenReturn(Arrays.asList(someAnnotationEntry, visibleForTestingAnnotationEntry));
when(visibleForTestingAnnotationEntry.toString()).thenReturn(
UnexpectedAccessDetector.VISIBLE_FOR_TESTING_ANNOTATION);
when(method.getAnnotations()).thenReturn(
Arrays.asList(someAnnotationEntry,
visibleForTestingAnnotationEntry));

detector.checkAnnotations(method.getAnnotations(), false);

Expand All @@ -168,7 +157,8 @@ public void testCheckAnnotated() {
*/
@Test
public void testIsInvoking() {
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(
bugReporter);
assertTrue(detector.isInvoking(Constants.INVOKESPECIAL));
assertTrue(detector.isInvoking(Constants.INVOKEINTERFACE));
assertTrue(detector.isInvoking(Constants.INVOKESTATIC));
Expand All @@ -180,8 +170,17 @@ public void testIsInvoking() {
*/
@Test
public void testIsWorkingOnAField() {
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(bugReporter);
UnexpectedAccessDetector detector = new UnexpectedAccessDetector(
bugReporter);
assertTrue(detector.isWorkingOnAField(Constants.PUTFIELD));
assertTrue(detector.isWorkingOnAField(Constants.GETFIELD));
}

class TestClassDescriptor extends ClassDescriptor {
private static final long serialVersionUID = 1L;

public TestClassDescriptor(final String classname) {
super(classname);
}
}
}

0 comments on commit 6869281

Please sign in to comment.