Skip to content

Commit

Permalink
Merge pull request WorksApplications#14 from eller86/detect-implicit-…
Browse files Browse the repository at this point in the history
…length

Detect implicit 'length' and 'nullable' of @column
  • Loading branch information
wliyongfeng committed Mar 20, 2014
2 parents af1fd07 + a193db5 commit ab0d5f7
Show file tree
Hide file tree
Showing 17 changed files with 445 additions and 15 deletions.
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ To use this product, please configure your findbugs-maven-plugin like below.
- added LongColumnNameDetector
- added UnknownNullnessDetector
- added UndocumentedIgnoreDetector
- added ImplicitLengthDetector
- added ImplicitNullnessDetector
- added ColumnDefinitionDetector

## 0.0.1
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
package jp.co.worksap.oss.findbugs.jpa;

import java.util.List;
import java.util.Map;

import javax.annotation.CheckReturnValue;
import javax.annotation.Nonnull;

import org.apache.bcel.classfile.AnnotationEntry;
import org.apache.bcel.classfile.ElementValue;
import org.apache.bcel.classfile.Field;
import org.apache.bcel.classfile.FieldOrMethod;
import org.apache.bcel.generic.Type;

import com.google.common.base.Objects;
import com.google.common.collect.Lists;

import edu.umd.cs.findbugs.BugReporter;
import edu.umd.cs.findbugs.bcel.AnnotationDetector;
import edu.umd.cs.findbugs.internalAnnotations.DottedClassName;

abstract class AbstractColumnDetector extends AnnotationDetector {
private final BugReporter bugReporter;

AbstractColumnDetector(BugReporter bugReporter) {
this.bugReporter = bugReporter;
}

@Nonnull
@CheckReturnValue
protected final BugReporter getBugReporter() {
return bugReporter;
}

@Override
public final void visitAnnotation(@DottedClassName String annotationClass,
Map<String, ElementValue> map, boolean runtimeVisible) {
if (!Objects.equal(annotationClass, "javax.persistence.Column")) {
return;
}

Type columnType = findVisitingColumnType();
verifyColumn(columnType, map);
}

protected boolean isVisitingLob() {
List<FieldOrMethod> targetListToSearch = Lists.newArrayList();
if (visitingField()) {
targetListToSearch.add(getField());
} else if (visitingMethod()) {
targetListToSearch.add(getMethod());
targetListToSearch.add(findFieldInVisitingMethod());
} else {
throw new IllegalStateException("@Column should annotate field or method.");
}

for (FieldOrMethod targetToSearch : targetListToSearch) {
for (AnnotationEntry annotation : targetToSearch.getAnnotationEntries()) {
if (!Objects.equal(annotation.getAnnotationType(), "Ljavax/persistence/Lob;")) {
continue;
}
return true;
}
}

return false;
}

private Type findVisitingColumnType() {
final Type columnType;
if (visitingField()) {
columnType = getField().getType();
} else if (visitingMethod()) {
Field visitingField = findFieldInVisitingMethod();
columnType = visitingField.getType();
} else {
throw new IllegalStateException("@Column should annotate field or method.");
}
return columnType;
}

@Nonnull
private Field findFieldInVisitingMethod() {
String fieldName = VisitedFieldFinder.findFieldWhichisVisitedInVisitingMethod(this);
Field visitingField = null;
for (Field field : getThisClass().getFields()) {
if (Objects.equal(field.getName(), fieldName)) {
visitingField = field;
break;
}
}
if (visitingField == null) {
throw new IllegalStateException("Cannot find field which named as " + fieldName + ".");
}
return visitingField;
}

protected abstract void verifyColumn(Type columnType, Map<String, ElementValue> elements);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package jp.co.worksap.oss.findbugs.jpa;

import java.util.Map;

import org.apache.bcel.classfile.ElementValue;
import org.apache.bcel.generic.Type;

import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugReporter;

public class ImplicitLengthDetector extends AbstractColumnDetector {
/**
* @see http://docs.oracle.com/cd/B28359_01/server.111/b28320/limits001.htm
*/
private static final int MAX_LENGTH_OF_ORACLE_VARCHAR = 4000;
/**
* @see http://www-01.ibm.com/support/knowledgecenter/SSEPEK_10.0.0/com.ibm.db2z10.doc.intro/src/tpc/db2z_stringdatatypes.htm
*/
private static final int MAX_LENGTH_OF_DB2_VARCHAR = 32704;

private static final int MAX_LENGTH_OF_VARCHAR = Math.min(MAX_LENGTH_OF_ORACLE_VARCHAR, MAX_LENGTH_OF_DB2_VARCHAR);

public ImplicitLengthDetector(BugReporter bugReporter) {
super(bugReporter);
}

@Override
protected void verifyColumn(Type columnType,
Map<String, ElementValue> elements) {
if (! isTarget(columnType)) {
return;
}

if (! elements.containsKey("length")) {
BugInstance bug = new BugInstance(this, "IMPLICIT_LENGTH", HIGH_PRIORITY);
if (visitingMethod()) {
bug.addMethod(this);
}
getBugReporter().reportBug(bug);
} else {
ElementValue value = elements.get("length");
int lengthValue = Integer.parseInt(value.stringifyValue());

if (lengthValue <= 0) {
reportIllegalLength(lengthValue);
} else if (MAX_LENGTH_OF_VARCHAR < lengthValue && !isVisitingLob()) {
reportIllegalLength(lengthValue);
}
}
}

private void reportIllegalLength(int lengthValue) {
BugInstance bug = new BugInstance(this, "ILLEGAL_LENGTH", HIGH_PRIORITY);
if (visitingMethod()) {
bug.addMethod(this);
}
getBugReporter().reportBug(bug);
}

/**
* @return true if column type requires length property.
*/
private boolean isTarget(Type columnType) {
return Type.STRING.equals(columnType) || Type.STRINGBUFFER.equals(columnType);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package jp.co.worksap.oss.findbugs.jpa;

import java.util.Map;

import org.apache.bcel.classfile.ElementValue;
import org.apache.bcel.generic.Type;

import edu.umd.cs.findbugs.BugInstance;
import edu.umd.cs.findbugs.BugReporter;

public class ImplicitNullnessDetector extends AbstractColumnDetector {

public ImplicitNullnessDetector(BugReporter bugReporter) {
super(bugReporter);
}

@Override
protected void verifyColumn(Type columnType,
Map<String, ElementValue> elements) {
if (! elements.containsKey("nullable")) {
BugInstance bug = new BugInstance(this, "IMPLICIT_NULLNESS", HIGH_PRIORITY);
if (visitingMethod()) {
bug.addMethod(this);
}
getBugReporter().reportBug(bug);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import org.apache.bcel.classfile.ElementValue;
import org.apache.bcel.classfile.Method;
import org.apache.commons.lang.IllegalClassException;
import org.objectweb.asm.ClassReader;

import com.google.common.base.Objects;

Expand Down Expand Up @@ -49,7 +48,7 @@ public void visitAnnotation(@DottedClassName String annotationClass,
columnName = getFieldName();
} else if (visitingMethod()) {
Method targetMethod = getMethod();
columnName = findFieldWhichisVisitedIn(targetMethod);
columnName = VisitedFieldFinder.findFieldWhichisVisitedInVisitingMethod(this);
if (columnName == null) {
throw new IllegalClassException(String.format(
"Method which is annotated with @Column should access to field, but %s#%s does not access.",
Expand All @@ -62,18 +61,6 @@ public void visitAnnotation(@DottedClassName String annotationClass,
detectLongName(columnName);
}

private String findFieldWhichisVisitedIn(Method targetMethod) {
byte[] classByteCode = getClassContext().getJavaClass().getBytes();
ClassReader reader = new ClassReader(classByteCode);

// note: bcel's #getSignature() method returns String like "(J)V", this is named as "descriptor" in the context of ASM.
// This is the reason why we call `targetMethod.getSignature()` to get value for `targetMethodDescriptor` argument.
VisitedFieldFinder visitedFieldFinder = new VisitedFieldFinder(targetMethod.getName(), targetMethod.getSignature());

reader.accept(visitedFieldFinder, 0);
return visitedFieldFinder.getVisitedFieldName();
}

private void detectLongName(String tableName) {
if (tableName.length() > MAX_TABLE_LENGTH) {
bugReporter.reportBug(new BugInstance(this, "LONG_COLUMN_NAME",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

import org.apache.bcel.classfile.FieldOrMethod;
import org.objectweb.asm.ClassReader;
import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.commons.EmptyVisitor;

import edu.umd.cs.findbugs.bcel.AnnotationDetector;

/**
* <p>Simple ClassVisitor implementation to find visited field in the specified method.</p>
* <p>To create instance, you need to provide name and descriptor to specify the target method.</p>
Expand All @@ -28,7 +32,7 @@ public VisitedFieldFinder(@Nonnull String targetMethodName, @Nonnull String targ

@CheckReturnValue
@Nullable
String getVisitedFieldName() {
private String getVisitedFieldName() {
return visitedFieldName;
}

Expand All @@ -48,4 +52,18 @@ public void visitFieldInsn(int code, String owner, String name, String descripti
this.visitedFieldName = name;
}

@Nullable
@CheckReturnValue
static String findFieldWhichisVisitedInVisitingMethod(AnnotationDetector detector) {
byte[] classByteCode = detector.getClassContext().getJavaClass().getBytes();
ClassReader reader = new ClassReader(classByteCode);

FieldOrMethod targetMethod = detector.getMethod();
// note: bcel's #getSignature() method returns String like "(J)V", this is named as "descriptor" in the context of ASM.
// This is the reason why we call `targetMethod.getSignature()` to get value for `targetMethodDescriptor` argument.
VisitedFieldFinder visitedFieldFinder = new VisitedFieldFinder(targetMethod .getName(), targetMethod.getSignature());

reader.accept(visitedFieldFinder, 0);
return visitedFieldFinder.getVisitedFieldName();
}
}
3 changes: 3 additions & 0 deletions src/main/meta/bugrank.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@
0 BugPattern UNKNOWN_NULLNESS_OF_PARAMETER
0 BugPattern UNKNOWN_NULLNESS_OF_RETURNED_VALUE
0 BugPattern UNDOCUMENTED_IGNORE
0 BugPattern IMPLICIT_NULLNESS
0 BugPattern IMPLICIT_LENGTH
0 BugPattern ILLEGAL_LENGTH
0 BugPattern USE_COLUMN_DEFINITION
12 changes: 12 additions & 0 deletions src/main/meta/findbugs.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@
<BugPattern type="LONG_COLUMN_NAME" abbrev="JPA"
category="CORRECTNESS" />

<Detector class="jp.co.worksap.oss.findbugs.jpa.ImplicitLengthDetector"
speed="fast" hidden="false" />
<BugPattern type="IMPLICIT_LENGTH" abbrev="JPA"
category="BAD_PRACTICE" />
<BugPattern type="ILLEGAL_LENGTH" abbrev="JPA"
category="CORRECTNESS" />

<Detector class="jp.co.worksap.oss.findbugs.jpa.ImplicitNullnessDetector"
speed="fast" hidden="false" />
<BugPattern type="IMPLICIT_NULLNESS" abbrev="JPA"
category="BAD_PRACTICE" />

<Detector class="jp.co.worksap.oss.findbugs.jpa.ColumnDefinitionDetector"
speed="fast" hidden="false" />
<BugPattern type="USE_COLUMN_DEFINITION" abbrev="JPA"
Expand Down
54 changes: 54 additions & 0 deletions src/main/meta/messages.xml
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,60 @@
</Details>
</BugPattern>

<Detector class="jp.co.worksap.oss.findbugs.jpa.ImplicitLengthDetector">
<Details>
The default value of length element (255) might be not enough in some case.
It is good to specify the value of length element clear, it tells that you have considered about it.
</Details>
</Detector>

<BugPattern type="IMPLICIT_LENGTH">
<ShortDescription>Specify length of column, its default (255) might be not enough.
</ShortDescription>
<LongDescription>
Specify length of column, its default (255) might be not enough.
</LongDescription>
<Details>
<![CDATA[
<p>Specify length of column, its default (255) might be not enough.</p>
]]>
</Details>
</BugPattern>

<BugPattern type="ILLEGAL_LENGTH">
<ShortDescription>Specified length of column should be greater than 0, and smaller than 4,000 bytes.
</ShortDescription>
<LongDescription>
Specified length of column ({1}) should be greater than 0, and smaller than 4,000 bytes.
</LongDescription>
<Details>
<![CDATA[
<p>Specified length of column should be greater than 0, and smaller than 4,000 bytes.</p>
<p>Oracle&#39;s maximum varchar size is 4000 bytes, and DB2&#39;s is 32704 bytes.
To keep portability of your product, it is better to shorten length to 4000 bytes, or to use @Lob annotation to use CLOB.</p>
]]>
</Details>
</BugPattern>

<Detector class="jp.co.worksap.oss.findbugs.jpa.ImplicitNullnessDetector">
<Details>
It is good to specify the value of nullable element clear, it tells that you have considered about it.
</Details>
</Detector>

<BugPattern type="IMPLICIT_NULLNESS">
<ShortDescription>It is good to specify the value of nullable element clear, it tells that you have considered about it.
</ShortDescription>
<LongDescription>
It is good to specify the value of nullable element clear, it tells that you have considered about it.
</LongDescription>
<Details>
<![CDATA[
<p>It is good to specify the value of nullable element clear, it tells that you have considered about it.</p>
]]>
</Details>
</BugPattern>

<Detector class="jp.co.worksap.oss.findbugs.jpa.ColumnDefinitionDetector">
<Details>
To ensure that JPA entity does not depend on specific RDBMS,
Expand Down
12 changes: 12 additions & 0 deletions src/test/java/jp/co/worksap/oss/findbugs/jpa/ColumnWithLength.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package jp.co.worksap.oss.findbugs.jpa;

import javax.persistence.Column;

public class ColumnWithLength {
@Column(length = 100)
private String name;

public String getName() {
return name;
}
}
Loading

0 comments on commit ab0d5f7

Please sign in to comment.