Spotbugs, increasing rank to 15 and lowering threshold to default

build/qa/spotbugs-exclude.xml

@@ -12,13 +12,48 @@
        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>

doc/en/developer/source/qa-guide/index.rst

@@ -5,14 +5,9 @@ The GeoServer builds on Travis and `https://build.geoserver.org/ <https://build.
 `PMD <https://pmd.github.io/>`_ and `Error Prone <https://errorprone.info/>`_ 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
-if you are using a JDK 8::
+In case you want to just run the build with the full checks locally, use the following command::
 
-    mvn clean install -Ppmd,errorprone8 -Dall
-
-or the following if using JDK 11::
-
-    mvn clean install -Ppmd,errorprone -Dall
+    mvn clean install -Dqa -Dall
 
 Add extra parameters as you see fit, like ``-T1C -nsu`` to speed up the build, or ``-Dfmt.skip=true -DskipTests``
 to avoid running tests and code formatting.
@@ -44,13 +39,37 @@ error message, and a reference to a XML file with the same information after it
 In case of parallel build, the specific error messages will be in the body of the build, while the
 XML file reference wil be at end end, just search for "PMD Failure" in the build logs to find the specific code issues.
 
+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
 -----------
 
@@ -74,4 +93,36 @@ Any failure to comply with the "Error Prone" rules will show up as a compile err
 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")
+   @SuppressWarnings("IdentityBinaryExpression")
+
+Spotbugs
+--------
+
+The `Spotbugs <https://spotbugs.github.io/>`_ checker runs as a post-compile bytecode analyzer.
+
+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 ``${geoserverBaseDir}/build/qa/spotbugs-exclude.xml`` file can be modified.
+
+Checkstyle
+----------
+
+Google Format is already in use to keep the code formatted, so Checkstyle is used mainly to verify javadocs errors
+and presence of copyright headers, which none of the other tools can cover.
+
+Any failure to comply with the rules will show up as a compiler error in the build output, e.g.::
+
+        14610 [INFO] --- maven-checkstyle-plugin:3.0.0:check (default) @ gt-jdbc ---
+        15563 [INFO] There is 1 error reported by Checkstyle 6.18 with /home/aaime/devel/git-gs/build/qa/checkstyle.xml ruleset.
+        15572 [ERROR] wms/main/java/org/geoserver/wms/map/RenderedImageMapOutputFormat.java:[325,8] (javadoc) JavadocMethod: Unused @param tag for 'foobar'.
+

src/extension/control-flow/src/main/java/org/geoserver/flow/controller/IpFlowController.java

@@ -70,7 +70,7 @@ public boolean requestIncoming(Request request, long timeout) {
         // generate a unique queue id for this client if none was found
         if (queue == null) {
             // beware of multiple concurrent requests...
-            synchronized (queues) {
+            synchronized (this) {
                 queue = queues.get(incomingIp);
                 if (queue == null) {
                     queue = new TimedBlockingQueue(queueSize, true);

src/extension/control-flow/src/main/java/org/geoserver/flow/controller/RateFlowController.java

@@ -191,7 +191,7 @@ public boolean requestIncoming(Request request, long timeout) {
         if (counters.size() > COUNTERS_CLEANUP_THRESHOLD
                 && (elapsed > (timeInterval) || (elapsed > 10000))) {
             int cleanupCount = 0;
-            synchronized (counters) {
+            synchronized (this) {
                 for (Map.Entry<String, Counter> entry : counters.entrySet()) {
                     Counter c = entry.getValue();
                     long timePeriodId = c.getTimePeriodId();

src/extension/control-flow/src/main/java/org/geoserver/flow/controller/UserConcurrentFlowController.java

@@ -125,7 +125,7 @@ public boolean requestIncoming(Request request, long timeout) {
         if ((queues.size() > maxQueues && (now - lastCleanup) > (maxAge / 10))
                 || (now - lastCleanup) > maxAge) {
             int cleanupCount = 0;
-            synchronized (queues) {
+            synchronized (this) {
                 for (String key : queues.keySet()) {
                     TimedBlockingQueue tbq = queues.get(key);
                     if (now - tbq.lastModified > maxAge && tbq.size() == 0) {

src/extension/csw/core/src/main/java/org/geoserver/csw/response/AbstractRecordTransformer.java

@@ -10,6 +10,7 @@
 import net.opengis.cat.csw20.ElementSetType;
 import net.opengis.cat.csw20.GetRecordByIdType;
 import net.opengis.cat.csw20.RequestBaseType;
+import org.apache.commons.lang3.StringUtils;
 import org.geoserver.csw.records.CSWRecordDescriptor;
 import org.geoserver.platform.ServiceException;
 import org.geotools.csw.CSW;
@@ -58,7 +59,10 @@ public void encode(Object o) throws IllegalArgumentException {
                 String prefix = (String) declaredPrefixes.nextElement();
                 if (!"xml".equalsIgnoreCase(prefix)) {
                     String uri = ns.getURI(prefix);
-                    addAttribute(attributes, prefix == "" ? "xmlns" : "xmlns:" + prefix, uri);
+                    addAttribute(
+                            attributes,
+                            StringUtils.isBlank(prefix) ? "xmlns" : "xmlns:" + prefix,
+                            uri);
                 }
             }
             addAttribute(attributes, "xmlns:xsi", "http://www.w3.org/2001/XMLSchema-instance");

src/extension/importer/core/src/main/java/org/geoserver/importer/Dates.java

@@ -5,7 +5,6 @@
  */
 package org.geoserver.importer;
 
-import com.google.common.base.Predicate;
 import com.google.common.collect.Collections2;
 import java.text.ParseException;
 import java.util.Arrays;
@@ -51,15 +50,7 @@ public class Dates {
     public static Collection<DatePattern> patterns(boolean strict) {
         Collection<DatePattern> patterns = PATTERNS;
         if (!strict) {
-            patterns =
-                    Collections2.filter(
-                            patterns,
-                            new Predicate<DatePattern>() {
-                                @Override
-                                public boolean apply(DatePattern input) {
-                                    return !input.isStrict();
-                                }
-                            });
+            patterns = Collections2.filter(patterns, input -> input != null && !input.isStrict());
         }
         return patterns;
     }

src/extension/importer/core/src/main/java/org/geoserver/importer/Directory.java

@@ -158,54 +158,57 @@ public boolean accept(File dir, String name) {
             Set<File> all = new LinkedHashSet<File>(Arrays.asList(fileList));
 
             // scan all the files looking for spatial ones
-            for (File f : dir.listFiles()) {
-                if (f.isHidden()) {
-                    all.remove(f);
-                    continue;
-                }
-                if (f.isDirectory()) {
-                    if (!recursive && !f.equals(file)) {
-                        // skip it
+            File[] files = dir.listFiles();
+            if (files != null) {
+                for (File f : files) {
+                    if (f.isHidden()) {
+                        all.remove(f);
                         continue;
                     }
-                    // @hacky - ignore __MACOSX
-                    // this could probably be dealt with in a better way elsewhere
-                    // like by having Directory ignore the contents since they
-                    // are all hidden files anyway
-                    if (!"__MACOSX".equals(f.getName())) {
-                        Directory d = new Directory(f);
-                        d.prepare(m);
-
-                        files.add(d);
+                    if (f.isDirectory()) {
+                        if (!recursive && !f.equals(file)) {
+                            // skip it
+                            continue;
+                        }
+                        // @hacky - ignore __MACOSX
+                        // this could probably be dealt with in a better way elsewhere
+                        // like by having Directory ignore the contents since they
+                        // are all hidden files anyway
+                        if (!"__MACOSX".equals(f.getName())) {
+                            Directory d = new Directory(f);
+                            d.prepare(m);
+
+                            this.files.add(d);
+                        }
+                        // q.push(f);
+                        continue;
                     }
-                    // q.push(f);
-                    continue;
-                }
 
-                // special case for .aux files, they are metadata but get picked up as readable
-                // by the erdas imagine reader...just ignore them for now
-                if ("aux".equalsIgnoreCase(FilenameUtils.getExtension(f.getName()))) {
-                    continue;
-                }
+                    // special case for .aux files, they are metadata but get picked up as readable
+                    // by the erdas imagine reader...just ignore them for now
+                    if ("aux".equalsIgnoreCase(FilenameUtils.getExtension(f.getName()))) {
+                        continue;
+                    }
 
-                // determine if this is a spatial format or not
-                DataFormat format = DataFormat.lookup(f);
+                    // determine if this is a spatial format or not
+                    DataFormat format = DataFormat.lookup(f);
 
-                if (format != null) {
-                    SpatialFile sf = newSpatialFile(f, format);
+                    if (format != null) {
+                        SpatialFile sf = newSpatialFile(f, format);
 
-                    // gather up the related files
-                    sf.prepare(m);
+                        // gather up the related files
+                        sf.prepare(m);
 
-                    files.add(sf);
+                        this.files.add(sf);
 
-                    all.removeAll(sf.allFiles());
+                        all.removeAll(sf.allFiles());
+                    }
                 }
             }
 
             // take any left overs and add them as unspatial/unrecognized
             for (File f : all) {
-                files.add(new ASpatialFile(f));
+                this.files.add(new ASpatialFile(f));
             }
         }
 

src/extension/importer/core/src/main/java/org/geoserver/importer/SpatialFile.java

@@ -100,29 +100,32 @@ public String apply(@Nullable StyleHandler input) {
         // getBaseName only gets the LAST extension so beware for .shp.aux.xml stuff
         final String baseName = getBaseName(file.getName());
 
-        for (File f : file.getParentFile().listFiles()) {
-            if (f.equals(file)) {
-                continue;
-            }
+        File[] files = file.getParentFile().listFiles();
+        if (files != null) {
+            for (File f : files) {
+                if (f.equals(file)) {
+                    continue;
+                }
 
-            if (!f.getName().startsWith(baseName)) {
-                continue;
-            }
+                if (!f.getName().startsWith(baseName)) {
+                    continue;
+                }
 
-            if (!f.isFile()) {
-                continue;
-            }
+                if (!f.isFile()) {
+                    continue;
+                }
 
-            String ext = f.getName().substring(baseName.length());
-            // once the basename is stripped, extension(s) should be present
-            if (ext.charAt(0) == '.') {
-                if (".prj".equalsIgnoreCase(ext)) {
-                    prjFile = f;
-                } else if (styleFile == null && styleExtensions.contains(ext.substring(1))) {
-                    // TODO: deal with multiple style files? for now we just grab the first
-                    styleFile = f;
-                } else {
-                    suppFiles.add(f);
+                String ext = f.getName().substring(baseName.length());
+                // once the basename is stripped, extension(s) should be present
+                if (ext.charAt(0) == '.') {
+                    if (".prj".equalsIgnoreCase(ext)) {
+                        prjFile = f;
+                    } else if (styleFile == null && styleExtensions.contains(ext.substring(1))) {
+                        // TODO: deal with multiple style files? for now we just grab the first
+                        styleFile = f;
+                    } else {
+                        suppFiles.add(f);
+                    }
                 }
             }
         }
@@ -205,7 +208,7 @@ public boolean equals(Object obj) {
         return true;
     }
 
-    private Object readResolve() {
+    protected Object readResolve() {
         suppFiles = suppFiles == null ? new ArrayList<File>() : suppFiles;
         return this;
     }