Skip to content

Commit

Permalink
FindBugsParser is using string matching to try to parse XML, which is
Browse files Browse the repository at this point in the history 
failing because SourceLine elements may, or may not self-terminate.

Convert to use a StAX parser, which is likely more performant since it
does not need to construct regexes and lists to store the results in, but
instead will do a forwards-only stream read.

Signed-off-by: Nigel Magnay <[email protected]>
  • Loading branch information
@magnayn
magnayn committed Apr 7, 2016
1 parent eeb2a62 commit c6679a5
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 29 deletions.
92 changes: 63 additions & 29 deletions src/main/java/se/bjurr/violations/lib/parsers/FindbugsParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
import static se.bjurr.violations.lib.reports.Reporter.FINDBUGS;

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.List;
import java.util.Map;

Expand All @@ -21,6 +23,11 @@
import com.google.common.io.Files;
import com.google.common.io.Resources;

import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.XMLStreamReader;
import javax.xml.stream.events.XMLEvent;

public class FindbugsParser extends ViolationsParser {

/**
Expand All @@ -37,50 +44,77 @@ public static void setFindbugsMessagesXml(String findbugsMessagesXml) {
public List<Violation> parseFile(File file) throws Exception {
List<Violation> violations = newArrayList();
Map<String, String> messagesPerType = getMessagesPerType();
String string = Files.toString(file, UTF_8);
List<String> bugInstances = getChunks(string, "<BugInstance", "</BugInstance>");
for (String bugInstanceChunk : bugInstances) {
String type = getAttribute(bugInstanceChunk, "type");
Integer rank = getIntegerAttribute(bugInstanceChunk, "rank");

try(InputStream input = new FileInputStream(file)) {

XMLInputFactory factory = XMLInputFactory.newInstance();
XMLStreamReader xmlr = factory.createXMLStreamReader(input);

while (xmlr.hasNext()) {
int eventType = xmlr.next();
if (eventType == XMLEvent.START_ELEMENT) {
if (xmlr.getLocalName().equals("BugInstance")) {
parseBugInstance(xmlr, violations, messagesPerType);
}
}
}
}
return violations;
}

private void parseBugInstance(XMLStreamReader xmlr, List<Violation> violations, Map<String, String> messagesPerType) throws XMLStreamException {
String type = getAttribute(xmlr,"type");
Integer rank = getIntegerAttribute(xmlr,"rank");
String message = messagesPerType.get(type);
if (message == null) {
message = type;
}
SEVERITY severity = toSeverity(rank);

List<String> sourceLineChunks = getChunks(bugInstanceChunk, "<SourceLine", "/>");
List<Violation> candidates = newArrayList();
for (String sourceLineChunk : sourceLineChunks) {
Optional<Integer> startLine = findIntegerAttribute(sourceLineChunk, "start");
Optional<Integer> endLine = findIntegerAttribute(sourceLineChunk, "end");
if (!startLine.isPresent() || !endLine.isPresent()) {
continue;
List<Violation> candidates = newArrayList();

while(xmlr.hasNext()) {
int eventType = xmlr.next();
if( eventType == XMLEvent.START_ELEMENT) {
if( xmlr.getLocalName().equals("SourceLine")) {
Optional<Integer> startLine = findIntegerAttribute(xmlr, "start");
Optional<Integer> endLine = findIntegerAttribute(xmlr, "end");
if (!startLine.isPresent() || !endLine.isPresent()) {
continue;
}
String filename = getAttribute(xmlr, "sourcepath");
String classname = getAttribute(xmlr, "classname");
candidates.add(//
violationBuilder()//
.setReporter(FINDBUGS)//
.setMessage(message)//
.setFile(filename)//
.setStartLine(startLine.get())//
.setEndLine(endLine.get())//
.setRule(type)//
.setSeverity(severity)//
.setSource(classname)//
.setSpecific(FINDBUGS_SPECIFIC_RANK, rank)//
.build()//
);
}
String filename = getAttribute(sourceLineChunk, "sourcepath");
String classname = getAttribute(sourceLineChunk, "classname");
candidates.add(//
violationBuilder()//
.setReporter(FINDBUGS)//
.setMessage(message)//
.setFile(filename)//
.setStartLine(startLine.get())//
.setEndLine(endLine.get())//
.setRule(type)//
.setSeverity(severity)//
.setSource(classname)//
.setSpecific(FINDBUGS_SPECIFIC_RANK, rank)//
.build()//
);
}
if( eventType == XMLEvent.END_ELEMENT) {
if( xmlr.getLocalName().equals("BugInstance") ) {
// End of the bug instance.
break;
}
}
}

if (!candidates.isEmpty()) {
/**
* Last one is the most specific, first 2 may be class and method when the
* third is source line.
*/
violations.add(candidates.get(candidates.size() - 1));
}
}
return violations;

}

/**
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/se/bjurr/violations/lib/parsers/ViolationsParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

import com.google.common.base.Optional;

import javax.xml.stream.XMLStreamReader;

public abstract class ViolationsParser {

public static Optional<String> findAttribute(String in, String attribute) {
Expand All @@ -37,6 +39,10 @@ public static Integer getIntegerAttribute(String in, String attribute) {
return parseInt(getAttribute(in, attribute));
}

public static Integer getIntegerAttribute(XMLStreamReader in, String attribute) {
return parseInt(getAttribute(in, attribute));
}

public static String getAttribute(String in, String attribute) {
Optional<String> foundOpt = findAttribute(in, attribute);
if (foundOpt.isPresent()) {
Expand All @@ -45,13 +51,28 @@ public static String getAttribute(String in, String attribute) {
throw new RuntimeException("\"" + attribute + "\" not found in \"" + in + "\"");
}

public static String getAttribute(XMLStreamReader in, String attribute) {
String foundOpt = in.getAttributeValue("", attribute);
if( foundOpt == null)
throw new RuntimeException("\"" + attribute + "\" not found in \"" + in + "\"");
return foundOpt;
}

public static Optional<Integer> findIntegerAttribute(String in, String attribute) {
if (findAttribute(in, attribute).isPresent()) {
return of(parseInt(getAttribute(in, attribute)));
}
return absent();
}

public static Optional<Integer> findIntegerAttribute(XMLStreamReader in, String attribute) {
String attr = in.getAttributeValue("",attribute);
if( attr == null )
return Optional.absent();
else
return Optional.of( Integer.parseInt(attr) );
}

public static List<String> getChunks(String in, String includingStart, String includingEnd) {
Pattern pattern = Pattern.compile("(" + includingStart + ".+?" + includingEnd + ")", DOTALL);
Matcher matcher = pattern.matcher(in);
Expand Down
13 changes: 13 additions & 0 deletions src/test/java/se/bjurr/violations/lib/FindbugsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,17 @@ public void testThatViolationsCanBeParsed() {
assertThat(actual.get(0).getSpecifics().get(FINDBUGS_SPECIFIC_RANK))//
.isEqualTo("7");
}

@Test
public void testMavenGeneratedFindbugs() {
String rootFolder = getRootFolder();
List<Violation> maven = violationsReporterApi() //
.withPattern(".*/findbugs/fromMaven.xml$") //
.inFolder(rootFolder) //
.findAll(FINDBUGS) //
.violations();

assertThat(maven)//
.hasSize(1);
}
}
Loading

0 comments on commit c6679a5

Please sign in to comment.