Automatic Quality Assurance checks

The GeoServer builds on Github Actions and https://build.geoserver.org/ apply PMD and Error Prone checks on the code base and will fail the build in case of rule violation.

In case you want to just run the build with the full checks locally, use the following command:

mvn clean install -Dqa

Add extra parameters as you see fit, like -T1C -nsu to speed up the build:

mvn install -Prelease -Dqa -T1C -nsu -fae

Flags documented below can be used to shut off individual QA checks when trouble shooting.

PMD checks

The PMD checks are based on source code analysis for common errors, we have configured PMD to check for common mistakes and bad practices such as accidentally including debug System.out.println() statements in your commit.

<artifactId>maven-pmd-plugin</artifactId>
<version>3.14.0</version>
<executions>
  <execution>
    <goals>
      <goal>check</goal>
    </goals>
  </execution>
</executions>
<dependencies>
  <dependency>
    <groupId>net.sourceforge.pmd</groupId>
    <artifactId>pmd-core</artifactId>
    <version>${pmd.version}</version>
  </dependency>
  <dependency>
    <groupId>net.sourceforge.pmd</groupId>
    <artifactId>pmd-java</artifactId>
    <version>${pmd.version}</version>
  </dependency>
  <dependency>
    <groupId>net.sourceforge.pmd</groupId>
    <artifactId>pmd-javascript</artifactId>
    <version>${pmd.version}</version>
  </dependency>
  <dependency>
    <groupId>net.sourceforge.pmd</groupId>
    <artifactId>pmd-jsp</artifactId>
    <version>${pmd.version}</version>
  </dependency>
</dependencies>
<configuration>
  <rulesets>
    <ruleset>${geoserverBaseDir}/../build/qa/pmd-ruleset.xml</ruleset>
    <ruleset>${geoserverBaseDir}/../build/qa/pmd-junit-ruleset.xml</ruleset>
  </rulesets>
  <failurePriority>3</failurePriority>
  <minimumPriority>3</minimumPriority>
  <verbose>true</verbose>
  <printFailingErrors>true</printFailingErrors>
  <includeTests>true</includeTests>
  <sourceEncoding>UTF-8</sourceEncoding>
  <outputEncoding>UTF-8</outputEncoding>
  <excludeRoots>
    <excludeRoot>target/generated-sources</excludeRoot>
  </excludeRoots>
</configuration>

Rules are configured in our build build/qa/pmd-ruleset.xml:

  <rule ref="category/java/bestpractices.xml/AvoidUsingHardCodedIP"/>
  <rule ref="category/java/bestpractices.xml/CheckResultSet"/>
  <rule ref="category/java/bestpractices.xml/UnusedLocalVariable"/>
  <rule ref="category/java/bestpractices.xml/UnusedPrivateField"/>
  <rule ref="category/java/bestpractices.xml/UnusedPrivateMethod"/>
  <rule ref="category/java/bestpractices.xml/SystemPrintln"/>
  <rule ref="category/java/bestpractices.xml/ForLoopCanBeForeach" />
  <rule ref="category/java/bestpractices.xml/UseTryWithResources" >
    <description>
        Use try-with-resources to avoid memory / resources leaks.
    </description>
    <properties>
      <!-- The rule reports false positives for closeable arguments that are used             -->
      <!-- closed within the method body. This suppression tries to avoid false positives for -->
      <!-- this case, until we can switch to Java 9 and just use try(variable) {...} without  -->
      <!-- the need to instatiate the variable in the try                                     -->
      <property name="violationSuppressXPath">
        <value>
          <![CDATA[
              ./FinallyStatement//Name[
                substring-after(@Image, '.') = 'close' or
                substring-after(@Image, '.') = 'closeQuietly'
              ]
              [ pmd-java:typeIs('java.lang.AutoCloseable') and
                fn:substring-before(@Image, '.') = ancestor::MethodDeclaration/MethodDeclarator//VariableDeclaratorId/@Name
              ]"/>
            ]]>
        </value>
      </property>
    </properties>
  </rule>
  <rule ref="category/java/bestpractices.xml/ReplaceHashtableWithMap" />
  <rule ref="category/java/bestpractices.xml/ReplaceVectorWithList" />
  <rule ref="category/java/bestpractices.xml/AvoidPrintStackTrace" />
  <rule ref="category/java/bestpractices.xml/MissingOverride" />
  <rule ref="category/java/bestpractices.xml/UseStandardCharsets" />

  <rule ref="category/java/performance.xml/UseArrayListInsteadOfVector" />

  <rule ref="category/java/codestyle.xml/ExtendsObject"/>
  <rule ref="category/java/codestyle.xml/ForLoopShouldBeWhileLoop"/>
  <rule ref="category/java/codestyle.xml/UnnecessaryFullyQualifiedName"/>
  <rule ref="category/java/codestyle.xml/UseDiamondOperator" />
  <rule ref="category/java/codestyle.xml/UnnecessaryReturn"/>
  <rule ref="category/java/codestyle.xml/UselessParentheses"/>
  <rule ref="category/java/codestyle.xml/UselessQualifiedThis"/>
  <rule ref="category/java/codestyle.xml/UnnecessaryCast" />
  <rule ref="category/java/codestyle.xml/IdenticalCatchBranches" />
  <rule ref="category/java/codestyle.xml/UseShortArrayInitializer" />
  <rule ref="category/java/codestyle.xml/UnnecessaryImport" />
  <rule ref="category/java/errorprone.xml/AvoidDecimalLiteralsInBigDecimalConstructor"/>
  <rule ref="category/java/errorprone.xml/AvoidMultipleUnaryOperators"/>
  <rule ref="category/java/errorprone.xml/AvoidUsingOctalValues"/>
  <rule ref="category/java/errorprone.xml/BrokenNullCheck"/>
  <rule ref="category/java/errorprone.xml/CheckSkipResult"/>
  <rule ref="category/java/errorprone.xml/ClassCastExceptionWithToArray"/>
  <rule ref="category/java/errorprone.xml/DontUseFloatTypeForLoopIndices"/>
  <rule ref="category/java/errorprone.xml/EmptyFinallyBlock"/>
  <rule ref="category/java/errorprone.xml/EmptyIfStmt"/>
  <rule ref="category/java/errorprone.xml/EmptyInitializer"/>
  <rule ref="category/java/errorprone.xml/EmptyStatementBlock"/>
  <rule ref="category/java/errorprone.xml/EmptyStatementNotInLoop"/>
  <rule ref="category/java/errorprone.xml/EmptySwitchStatements"/>
  <rule ref="category/java/errorprone.xml/EmptySynchronizedBlock"/>
  <rule ref="category/java/errorprone.xml/EmptyTryBlock"/>
  <rule ref="category/java/errorprone.xml/EmptyWhileStmt"/>
  <rule ref="category/java/errorprone.xml/JumbledIncrementer"/>
  <rule ref="category/java/errorprone.xml/MisplacedNullCheck"/>
  <rule ref="category/java/errorprone.xml/OverrideBothEqualsAndHashcode"/>
  <rule ref="category/java/errorprone.xml/ReturnFromFinallyBlock"/>
  <rule ref="category/java/errorprone.xml/UnconditionalIfStatement"/>
  <rule ref="category/java/errorprone.xml/UnnecessaryConversionTemporary"/>
  <rule ref="category/java/errorprone.xml/UnusedNullCheckInEquals"/>
  <rule ref="category/java/errorprone.xml/UselessOperationOnImmutable"/>
  <rule ref="category/java/errorprone.xml/CloseResource">
    <properties>
        <!-- When calling the store to close, PMD wants the full prefix before the call to -->
        <!-- the method to match, so let's try to use common var names for store ...       -->
        <property name="closeTargets" value="releaseConnection,store.releaseConnection,closeQuietly,closeConnection,closeSafe,store.closeSafe,dataStore.closeSafe,getDataStore().closeSafe,close,closeResultSet,closeStmt,closeFinally,JDBCUtils.close,closeFileSystem"/>
        <property name="allowedResourceTypes" value="java.io.ByteArrayOutputStream|java.io.ByteArrayInputStream|java.io.StringWriter|java.io.CharArrayWriter|java.util.stream.Stream|java.util.stream.IntStream|java.util.stream.LongStream|java.util.stream.DoubleStream|java.io.StringReader|org.apache.commons.vfs2.FileSystemManager" />
    </properties>
  </rule>
  <rule ref="category/java/multithreading.xml/AvoidThreadGroup"/>
  <rule ref="category/java/multithreading.xml/DontCallThreadRun"/>
  <rule ref="category/java/multithreading.xml/DoubleCheckedLocking"/>
  <rule ref="category/java/performance.xml/BigIntegerInstantiation"/>
  <rule ref="category/java/bestpractices.xml/PrimitiveWrapperInstantiation"/>
  <rule ref="category/java/performance.xml/StringInstantiation"/>
  <rule name="wildcards" language="java" message="No Wildcard Imports" class="net.sourceforge.pmd.lang.rule.XPathRule">
    <description>
      Don't use wildcard imports
    </description>
    <priority>3</priority>
    <properties>
      <property name="version" value="2.0"/>
      <property name="xpath">
        <value><![CDATA[
          //ImportDeclaration[@ImportedName=@PackageName]
          ]]></value>
      </property>
    </properties>
  </rule>
  <rule ref="category/java/bestpractices.xml/UseCollectionIsEmpty" />
  <rule ref="category/java/design.xml/CognitiveComplexity">
    <properties>
      <property name="reportLevel" value="130"/>
    </properties>
  </rule>

In order to activate the PMD checks, use the -Ppmd profile:

mvn verify -Ppmd

Or run pmd:check (requires use of initialize to locate geoserverBaseDir/build/qa/pmd-ruleset.xml):

mvn initialize pmd:check -Ppmd

PMD will fail the build in case of violation, reporting the specific errors before the build error message, and a reference to a XML file with the same information after it (example taken from GeoTools):

7322 [INFO] --- maven-pmd-plugin:3.11.0:check (default) @ gt-main ---
17336 [INFO] PMD Failure: org.geotools.data.DataStoreAdaptor:98 Rule:SystemPrintln Priority:2 System.out.println is used.
17336 [INFO] PMD Failure: org.geotools.data.DataStoreAdaptor:98 Rule:SystemPrintln Priority:2 System.out.println is used.
17337 [INFO] ------------------------------------------------------------------------
17337 [INFO] BUILD FAILURE
17337 [INFO] ------------------------------------------------------------------------
17338 [INFO] Total time:  16.727 s
17338 [INFO] Finished at: 2018-12-29T11:34:33+01:00
17338 [INFO] ------------------------------------------------------------------------
17340 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-pmd-plugin:3.11.0:check (default) on project gt-main: You have 1 PMD violation. For more details see:       /home/yourUser/devel/git-gt/modules/library/main/target/pmd.xml -> [Help 1]
17340 [ERROR]

In case of parallel build, the specific error messages will be in the body of the build output, while the XML file reference will be at the end, search for PMD Failure in the build logs to find the specific code issues.

If you do have a PMD failure it is worth checking the pmd website which offers quite clear suggestions:

PMD false positive suppression

Occasionally PMD will report a false positive failure, for those it’s possible to annotate the method or the class in question with a SuppressWarnings using PMD.<RuleName, e.g. if the above error was actually a legit use of System.out.println it could have been annotated with:

@SuppressWarnings("PMD.SystemPrintln")
public void methodDoingPrintln(...) {

PMD CloseResource checks

PMD can check for Closeable that are not getting property closed by the code, and report about it. PMD by default only checks for SQL related closeables, like “Connection,ResultSet,Statement”, but it can be instructed to check for more by configuration (do check the PMD configuration in build/qa/pmd-ruleset.xml.

The check is a bit fragile, in that there are multiple ways to close an object between direct calls, utilities and delegate methods. The configuration lists the type of methods, and the eventual prefix, that will be used to perform the close, for example:

<rule ref="category/java/errorprone.xml/CloseResource" >
    <properties>
        <property name="closeTargets" value="releaseConnection,store.releaseConnection,closeQuietly,closeConnection,closeSafe,store.closeSafe,dataStore.closeSafe,getDataStore().closeSafe,close,closeResultSet,closeStmt"/>
    </properties>
</rule>

For closing delegates that use an instance object instead of a class static method, the variable name is included in the prefix, so some uninformity in variable names is required.

Error Prone

The Error Prone checker runs a compiler plugin.

In order to activate the Error Prone checks, use the “-Perrorprone”:

<id>errorprone</id>
<activation>
  <property>
    <name>qa</name>
  </property>
</activation>
<build>
  <plugins>
    <plugin>
      <artifactId>maven-compiler-plugin</artifactId>
      <configuration>
        <fork>false</fork>
        <compilerArgs>
          <arg>-XDcompilePolicy=simple</arg>
          <arg>-Xplugin:ErrorProne -XepExcludedPaths:\Q${project.build.directory}\E.* ${errorProneFlags}</arg>
          <arg>-Xlint:${lint}</arg>
          <arg>-Werror</arg>
          <arg>-Xmaxwarns</arg>
          <arg>1000</arg>
        </compilerArgs>
        <annotationProcessorPaths>
          <path>
            <groupId>com.google.errorprone</groupId>
            <artifactId>error_prone_core</artifactId>
            <version>${errorProne.version}</version>
          </path>
        </annotationProcessorPaths>
      </configuration>
    </plugin>
  </plugins>
</build>

Any failure to comply with the “Error Prone” rules will show up as a compile error in the build output, e.g. (example taken from GeoTools):

9476 [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project gt-coverage: Compilation failure
9476 [ERROR] /home/user/devel/git-gt/modules/library/coverage/src/main/java/org/geotools/image/ImageWorker.java:[380,39] error: [IdentityBinaryExpression] A binary expression where both operands are the same is usually incorrect; the value of this expression is equivalent to `255`.
9477 [ERROR]     (see https://errorprone.info/bugpattern/IdentityBinaryExpression)
9477 [ERROR]
9477 [ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project gt-coverage: Compilation failure
/home/user/devel/git-gt/modules/library/coverage/src/main/java/org/geotools/image/ImageWorker.java:[380,39] error: [IdentityBinaryExpression] A binary expression where both operands are the same is usually incorrect; the value of this expression is equivalent to `255`.
(see https://errorprone.info/bugpattern/IdentityBinaryExpression)

In case Error Prone is reporting an invalid error, the method or class in question can be annotated with SuppressWarnings with the name of the rule, e.g., to get rid of the above the following annotation could be used:

@SuppressWarnings("IdentityBinaryExpression")

Spotbugs

The Spotbugs checker runs as a post-compile bytecode analyzer.

<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs-maven-plugin</artifactId>
<version>4.7.3.4</version>
<executions>
  <execution>
    <goals>
      <goal>check</goal>
    </goals>
  </execution>
</executions>
<configuration>
  <effort>More</effort>
  <!--threshold>High</threshold-->
  <xmlOutput>true</xmlOutput>
  <maxRank>15</maxRank>
  <excludeFilterFile>${geoserverBaseDir}/../build/qa/spotbugs-exclude.xml</excludeFilterFile>
</configuration>

Any failure to comply with the rules will show up as a compile error, e.g.:

33630 [ERROR] page could be null and is guaranteed to be dereferenced in org.geotools.swing.wizard.JWizard.setCurrentPanel(String) [org.geotools.swing.wizard.JWizard, org.geotools.swing.wizard.JWizard, org.geotools.swing.wizard.JWizard, org.geotools.swing.wizard.JWizard] Dereferenced at JWizard.java:[line 278]Dereferenced at JWizard.java:[line 269]Null value at JWizard.java:[line 254]Known null at JWizard.java:[line 255] NP_GUARANTEED_DEREF

It is also possible to run the spotbugs:gui goal to have a Swing based issue explorer, e.g.:

mvn spotbugs:gui -Pspotbugs -f wms

In case an invalid report is given, an annotation on the class/method/variable can be added to ignore it:

@SuppressFBWarnings("NP_GUARANTEED_DEREF")

or if it’s a general one that should be ignored, the build/qa/spotbugs-exclude.xml file can be modified.

<!-- This file specifies a spotbugs filter for excluding reports that
     should not be considered errors.

     The format of this file is documented at:

       https://spotbugs.readthedocs.io/en/latest/filter.html

     When possible, please specify the full names of the bug codes,
     using the pattern attribute, to make it clearer what reports are
     being suppressed.  You can find a listing of codes at:

       https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html
  -->
<FindBugsFilter>
    <!-- Won't use prepared statements, very bad performance for geospatial use cases -->
    <Match>
        <Bug pattern="SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE"/>
    </Match>
    <Match>
        <Bug pattern="SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING"/>
    </Match>
    <!-- Returns moslty false positives -->
    <Match>
        <Bug pattern="FE_FLOATING_POINT_EQUALITY"/>
    </Match>
    <!-- Too many cases where we have a semi-legit usage of same name -->
    <Match>
        <Bug pattern="NM_SAME_SIMPLE_NAME_AS_SUPERCLASS"/>
    </Match>
    <!-- False positives, might want to revisit later -->
    <Match>
       <Bug pattern="EC_UNRELATED_TYPES_USING_POINTER_EQUALITY"/>
    </Match>
    <!-- Annoying but not actually a bug per se, might want to revisit later -->
    <Match>
       <Bug pattern="MF_CLASS_MASKS_FIELD"/>
    </Match>
    <!-- False positives -->
    <Match>
      <Bug pattern="UWF_UNWRITTEN_FIELD"/>
    </Match>
    <!-- Too many false positives -->
    <Match>
      <Bug pattern="CN_IMPLEMENTS_CLONE_BUT_NOT_CLONEABLE"/>
    </Match>
    <!-- Too many false positives -->
    <Match>
      <Bug pattern="NP_NULL_ON_SOME_PATH_MIGHT_BE_INFEASIBLE"/>
    </Match>
    <!-- Too many false positives -->
    <Match>
      <Bug pattern="NP_BOOLEAN_RETURN_NULL"/>
    </Match>
    <!-- False positives under Java 11, see https://github.com/spotbugs/spotbugs/issues/878 
         and https://github.com/spotbugs/spotbugs/issues/756 -->
    <Match>
      <Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
    </Match>
</FindBugsFilter>

Spotless

Spotless is used as a fast way to check that the google-java-format is being applied to the codebase.

<groupId>com.diffplug.spotless</groupId>
<artifactId>spotless-maven-plugin</artifactId>
<version>2.22.1</version>
<executions>
  <execution>
    <phase>validate</phase>
    <goals>
      <goal>${spotless.action}</goal>
    </goals>
  </execution>
</executions>
<configuration>
  <java>
    <googleJavaFormat>
      <version>1.7</version>
      <style>AOSP</style>
    </googleJavaFormat>
  </java>
  <upToDateChecking>
    <enabled>true</enabled>
    <indexFile>${project.basedir}/.spotless-index</indexFile>
  </upToDateChecking>
</configuration>

This has been setup for incremental checking, with hidden .spotless-index files used determine when files were last checked.

To run the plugin directly:

mvn spotless:apply

When using check any failure to comply with the rules will show up as a compiler error in the build output.

mvn spotless:check

When verifying spotless.action is used to choose apply or check (defaults to apply):

mvn verify -Dqa -Dspotless.action=check

Property spotless.apply.skip is used to skip spotless plugin when running qa build:

mvn clean install -Dqa -Dspotless.apply.skip=true

Sortpom

Sortpom is used to keep the pom.xml files formatting consistent:

<groupId>com.github.ekryd.sortpom</groupId>
<artifactId>sortpom-maven-plugin</artifactId>
<version>2.15.0</version>
<executions>
  <execution>
    <phase>verify</phase>
    <goals>
      <goal>${pom.fmt.action}</goal>
    </goals>
  </execution>
</executions>
<configuration>
  <skip>${pom.fmt.skip}</skip>
  <keepBlankLines>true</keepBlankLines>
  <spaceBeforeCloseEmptyElement>false</spaceBeforeCloseEmptyElement>
  <createBackupFile>false</createBackupFile>
  <verifyFail>stop</verifyFail>
  <verifyFailOn>strict</verifyFailOn>
</configuration>

The plugin is attached to verification phase to sort pom.xml files.

To run the plugin directly:

mvn sortpom:sort

Verification checks if (ignoring whitespace changes) is the current pom.xml in the correct order:

mvn sortpom:verify

Property pom.fmt.action is used to choose sort or verify (defaults to sort):

mvn verify -Dqa -Dpom.fmt.action=verify

Property pom.fmt.skip used to skip sortpom plugin when running qa build (defaults to spotless.apply.skip setting):

mvn clean install -Dqa -Dpom.fmt.skip=true

Checkstyle

Spotless is already in use to keep the code formatted, so maven checkstyle plugin is used mainly to verify javadocs errors and presence of copyright headers, which none of the other tools can cover.

<artifactId>maven-checkstyle-plugin</artifactId>
<version>3.1.1</version>
<executions>
  <execution>
    <goals>
      <goal>check</goal>
    </goals>
  </execution>
</executions>
<configuration>
  <encoding>UTF-8</encoding>
  <logViolationsToConsole>true</logViolationsToConsole>
  <!-- ignore generated classes, e.g., javacc ones -->
  <sourceDirectories>${project.build.sourceDirectory}</sourceDirectories>
  <skip>${checkstyle.skip}</skip>
  <configLocation>${geoserverBaseDir}/../build/qa/checkstyle.xml</configLocation>
</configuration>

The checkstyle ruleset checks the following:

<module name="Checker">
  <property name="severity" value="error"/>
  <module name="TreeWalker">
    <module name="JavadocMethod">
      <property name="allowMissingParamTags" value="true"/>
      <property name="allowMissingReturnTag" value="true"/>
    </module>
    <module name="ArrayTypeStyle"/>
  </module>
  <module name="RegexpHeader">
    <property name="multiLines" value="2,3"/> <!-- Allows to have other copyrights, see line 2 and 3 of the regexp -->
    <property name="header" value="^/\* (Copyright\s*)?\(c\) \d{4}(\s*-\s*\d{4})? Open Source Geospatial Foundation - all rights reserved$\n^
\* (Copyright\s*)?\(c\).*$\n^
\*$\n^
\* This code is licensed under the GPL 2.0 license, available at the root$\n^
\* application directory.$\n^
\*"/>
    <property name="fileExtensions" value="java"/>
  </module>
  <module name="RegexpSingleline">
    <property name="format" value="^\s{60}"/>
    <property name="message" value="Excessive nesting found. Please try to factor out the deeply nested code in a separate method or class"/>
    <property name="minimum" value="0"/>
    <property name="maximum" value="1"/>
    <property name="fileExtensions" value="java"/>
  </module>
</module>

To run the plugin directly:

mvn initialize checkstyle:check
Previous: Eclipse Guide
Next: Programming Guide