Automating Parts of the Peer Review – Part 1! Adding SpotBugs Maven Plugin for Static Code Analysis

TIP: References Quick List

Introduction

Static code analysis can help find bugs and/or style issues that were overlooked during unit testing before the code gets to to the peer review process. Alternatively, if working on a code base by yourself and peer reviews are not an option for you, then doing so could help you uncover issues in your code that otherwise may not have occurred to you.

This can help new or junior engineers learn of issues that they may not have previously considered, without having to receive peer feedback in a code review or a bug in production. Even senior engineers may make mistakes that could be caught during static code analysis. By catching certain types of issues ahead of the actual peer review, it may help the review either:

Focus on higher level concepts / concerns, such as how the updated code will fit into the larger module / component, or
Proceed through the peer review process more quickly, if reviewers only need to review the code once before approving, with the author already having caught and corrected common issues ahead of the review.

Example Checks

Bad Practice Category

A bad practice issue may or may not be a direct bug. Either way, it does something that is generally discouraged, although team agreement/adoption may vary. A few examples of bad practice category checks are:

Correctness Category

In the correctness category, the check is pointing out a likely bug. A few examples of correctness checks are:

Experimental Category

The experimental category may include new approaches to existing checks, new checks that are being vetted, or it may include test-only checks that aren’t used in production. Teams may choose to exclude this category if they wish to wait for more stable implementations. A few examples:

Internationalization (I18N) Category

Internationalization deals with supporting different languages or locales. A few examples are:

Malicious Code Vulnerability Category

The checks in this category indicate the source code may be vulnerable to malicious code. For example:

Multithreaded Correctness Category

This category of checks attempts to find issues when multiple threads are in use, causing difficult to diagnose race conditions, etc. For example:

Performance Category

Findings in this category may be for functionally correct, but inefficient code. For example:

Security Category

Input handling or other security related concerns. For example:

Style Category

The issues caught by checks in this category deal with style concerns. These may not result in functional bugs, but rather, may help in maintenance and/or usability. For example:

Include Filter File

Let’s start by setting up a filter file to indicate what checks should be included in the plugin’s execution. We are going to add all of the currently available categories:

Create a new build_config directory in IntelliJ by right clicking on java101 project -> New -> Directory -> build_config. Then, right click on the new build_config directory -> New -> File -> spotbugs-include.xml to add the new file. Let’s add the following to the new file to enable checks in all currently available categories. Note: The contents of this file may vary based upon desires of different teams.

<?xml version="1.0" encoding="UTF-8"?>
<FindBugsFilter
        xmlns="https://github.com/spotbugs/filter/3.0.0"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">

    <!--
    https://spotbugs.readthedocs.io/en/latest/filter.html
    https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html
    https://spotbugs.readthedocs.io/en/latest/effort.html
    -->

    <Match>
        <Bug category="BAD_PRACTICE"/>
    </Match>
    <Match>
        <Bug category="CORRECTNESS"/>
    </Match>
    <!--<Match>
        <Bug category="EXPERIMENTAL"/>
    </Match>-->
    <Match>
        <Bug category="I18N"/>
    </Match>
    <Match>
        <Bug category="MALICIOUS_CODE"/>
    </Match>
    <Match>
        <Bug category="MT_CORRECTNESS"/>
    </Match>
    <!--<Match>
        <Bug category="NOISE"/>
    </Match>-->
    <Match>
        <Bug category="PERFORMANCE"/>
    </Match>
    <Match>
        <Bug category="SECURITY"/>
    </Match>
    <Match>
        <Bug category="STYLE"/>
    </Match>
</FindBugsFilter>

Exclude Filter File

Next, let’s describe the scenarios where we want to disable certain check(s). In IntelliJ, right click on the build_config directory -> New -> File -> spotbugs-exclude.xml to add the new file. Next, add the initial contents for the file, such as:

<?xml version="1.0" encoding="UTF-8"?>
<FindBugsFilter
        xmlns="https://github.com/spotbugs/filter/3.0.0"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">

    <!--
    https://spotbugs.readthedocs.io/en/latest/filter.html
    https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html
    https://spotbugs.readthedocs.io/en/latest/effort.html
    -->

    <!-- If adding this quality gate to an existing project, you may need to add exclusions like the following:-->
    <!--
    <Match>
        <Class name="com.foobar.ClassNotToBeAnalyzed" />
        <Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON" />
    </Match>
    -->

    <Match>
        <!-- Permanent: Allow Test classes to have nested inner classes that aren't static. -->
        <Class name="~.*\.*Test" />
        <Bug pattern="SIC_INNER_SHOULD_BE_STATIC" />
    </Match>

    <Match>
        <!-- Permanent: Allow Test classes to call methods with @NonNull with null value. -->
        <Class name="~.*\.*Test" />
        <Bug pattern="NP_NONNULL_PARAM_VIOLATION" />
    </Match>

    <!--
    If adding a new quality gate to an existing project, you may need to introduce additional exclusions.
    For each, recommend adding a comment indicating whether the exclusion is permanent or temporary and, if
    temporary, a ticket to track the removal of the exclusion.
    -->
</FindBugsFilter>

The pom.xml Updates

Now, we can add the configuration to the pom.xml to enable the new plugin.

Plugin Management – Versioning

By adding the version information to the project/build/pluginManagement/plugins section of the pom.xml, we have one location where we maintain the plugin version – for this build and any Maven sub-modules.

<plugin>
    <!-- https://spotbugs.github.io/spotbugs-maven-plugin/usage.html -->
    <groupId>com.github.spotbugs</groupId>
    <artifactId>spotbugs-maven-plugin</artifactId>
    <version>4.7.2.1</version>
</plugin>

New Properties

Since this walk-through is aiming to include all levels analysis effort & priority in both the quality gate and the report, the values we plan to use in both scenarios are equivalent. Therefore, let’s extract those out into the project/properties section of the pom.xml for this walk-through. However, if you wanted to report upon lower priority or higher analysis effort checks, but only break the build for others, then this could instead be included in the corresponding build or report plugin directly.

<!--
Since these properties are needed for both build and reporting spotbugs configuration,
defining as properties for easier maintenance.
-->
<spotbugs.effort>Max</spotbugs.effort>
<spotbugs.excludeFilterFile>build_config/spotbugs-exclude.xml</spotbugs.excludeFilterFile>
<spotbugs.failThreshold>Low</spotbugs.failThreshold>
<spotbugs.includeFilterFile>build_config/spotbugs-include.xml</spotbugs.includeFilterFile>
<spotbugs.includeTests>true</spotbugs.includeTests>

Build Quality Gate

By adding the following to the project/build/plugins section of the pom.xml, we will enforce that any issues encountered will cause the build to fail, ensuring that a successful build equates to passing the quality gate. By including the nested plugin, we are including security related validations, as described in Using the SpotBugs Maven Plugin: Integrate Find Security Bugs into spotbugs-maven-plugin.

<plugin>
    <groupId>com.github.spotbugs</groupId>
    <artifactId>spotbugs-maven-plugin</artifactId>
    <configuration>
        <!-- see also properties configured with spotbugs prefix -->
        <plugins>
            <plugin>
                <!-- https://find-sec-bugs.github.io/bugs.htm -->
                <groupId>com.h3xstream.findsecbugs</groupId>
                <artifactId>findsecbugs-plugin</artifactId>
                <version>${findsecbugs.plugin.version}</version>
            </plugin>
        </plugins>
    </configuration>
    <executions>
        <execution>
            <id>static-code-analysis</id>
            <goals>
                <goal>check</goal>
            </goals>
        </execution>
    </executions>
</plugin>

Maven Site Report

By adding the following to the project/reporting/plugins section of the pom.xml, we will see a new report generated when we execute the mvn site command.

<plugin>
    <!-- https://spotbugs.github.io/spotbugs-maven-plugin/usage.html -->
    <groupId>com.github.spotbugs</groupId>
    <artifactId>spotbugs-maven-plugin</artifactId>
    <configuration>
        <!-- see also properties configured with spotbugs prefix -->
        <plugins>
            <plugin>
                <!-- https://find-sec-bugs.github.io/bugs.htm -->
                <groupId>com.h3xstream.findsecbugs</groupId>
                <artifactId>findsecbugs-plugin</artifactId>
                <version>${findsecbugs.plugin.version}</version>
            </plugin>
        </plugins>
    </configuration>
</plugin>

Running the Maven Build

Site Generation

If we run the mvn site install command, then java101/target/site/spotbugs.html should now be generated and the build should have succeeded. By putting site before install, we are requesting that the site be generated, even if the quality gate fails during the regular build process. In IntelliJ by Right clicking on java101/target/site/spotbugs.html -> Open In -> Browser -> Your favorite browser here, we can view the report, which should be empty, as it is using the same settings as the quality gate.

Configuring a Failure Scenario

If we open codingchica.java101.App and add temporarily add the following:

// TODO Delete this method - do not commit
@Override
public boolean equals(Object o) {
  App app = (App) o;
  return name == app.name;
}

Then by rerunning the Maven build command (AKA mvn site install) we can test if the build breaker / quality gate indeed causes the build to fail if an issue is encountered.

At this point, if we refresh the page in the browser for java101/target/site/spotbugs.html, we should see 4 bugs reported, with links provided to see additional information about the validation, as well as links to the line numbers where the issue was found in the source code.

Similarly, the Maven build should fail with output like the following:

[ERROR] Medium: Equals method for codingchica.java101.App assumes the argument is of type App [codingchica.java101.App] At App.java:[line 84] BC_EQUALS_METHOD_SHOULD_WORK_FOR_ALL_OBJECTS
[ERROR] Medium: Comparison of String objects using == or != in codingchica.java101.App.equals(Object) [codingchica.java101.App] At App.java:[line 85] ES_COMPARING_STRINGS_WITH_EQ
[ERROR] High: codingchica.java101.App defines equals and uses Object.hashCode() [codingchica.java101.App] At App.java:[lines 84-85] HE_EQUALS_USE_HASHCODE
[ERROR] Medium: codingchica.java101.App.equals(Object) does not check for null argument [codingchica.java101.App] At App.java:[lines 84-85] NP_EQUALS_SHOULD_HANDLE_NULL_ARGUMENT

Removing Failure Scenario

Now that we have confirmed that issues in the spotbugs analysis will cause the build to fail (the quality gate is working), let’s remove the code we temporarily added in the prior section. Running mvn site install should once again result in a successful build.

Adding to an Existing Project

Were we to add the SpotBugs plugin quality gate to an existing project, we would likely have existing issues that may not all be possible to address immediately. However, we can massage the output of the build into temporary exclusions, so that we do not break the build at the new gate’s introduction:

Pattern	Is Regular Expression?	Replacement	Comment
[ERROR]	No		Remove log level prefixes from output.
\[lines? [\d-]+\]	Yes		Remove line information that may interfere with upcoming regular expression.
.*\[	Yes	<Match> <!– TODO Temporary: To avoid breaking the build at introduction –> <Class name=”	Prefix the class name with XML information needed in the filter.
\].*:	Yes	” /> <Bug pattern=”	Provide the XML snippet between the class name and the bug name. Make sure there is a space after the colon in the pattern.
(\w)$	Yes	$1″ /> </Match>	Only match the lines where the bug pattern is not yet wrapped in closing quotes and XML elements and add those suffixes to make the XML valid. Repeat the last word character in the output, so we still match the bug name.

Additional Considerations

I would suggest capturing Maven build time before and after the new plugin is added, so we can see how much additional time is added to the build. This will vary by project and code base size. If too much time is being added to the build by the introduction of a new quality gate, the team may elect to change the settings, such as reducing the amount of acceptable effort level the plugin is allowed to expend during the analysis.

Summary

We can both reduce the work needed by our peer reviewers and potentially improve the code quality by adding a static code analysis quality gate to the Maven build, such as the spotbugs-maven-plugin. Doing so may save time during manual peer review efforts or more costly bug-fixes once code is deployed to dev, test, performance or production environments. Teams can configure the spotbugs-maven-plugin using filter files to customize the analysis effort and checks that should be performed on source code. The plugin can also be added to existing builds, with some quick regular expression-based find and replace logic to massage errors into temporary exclusions and avoid breaking the build for existing classes as the new quality gate is added.

Coding Chica