[HWORKS-991] add spotbugs (#1486)

.github/workflows/hopsworks-tests.yml

@@ -0,0 +1,42 @@
+name: pre-commit tests
+
+on:
+  pull_request_review:
+    types: [submitted]
+
+jobs:
+  hopsworks-unit-tests:
+    name: Hopsworks Unit Tests
+    runs-on: ubuntu-22.04
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      - name: Set up JDK 8
+        uses: actions/setup-java@v3
+        with:
+          java-version: '8'
+          distribution: 'adopt'
+          cache: 'maven'
+
+      - name: Run unit tests
+        run: mvn --batch-mode test
+
+  hopsworks-vulnerability-checker:
+    name: Hopsworks Vulnerability Checker
+    runs-on: ubuntu-22.04
+
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v3
+
+      - name: Set up JDK 8
+        uses: actions/setup-java@v3
+        with:
+          java-version: '8'
+          distribution: 'adopt'
+          cache: 'maven'
+
+      - name: Run vulnerability checker
+        run: mvn clean install -Powasp-dependency-check,spot-bugs -DskipTests

alerting/pom.xml

@@ -66,5 +66,9 @@
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>
     </dependency>
+    <dependency>
+      <groupId>com.github.spotbugs</groupId>
+      <artifactId>spotbugs-annotations</artifactId>
+    </dependency>
   </dependencies>
 </project>

alerting/src/main/java/io/hops/hopsworks/alerting/api/util/Settings.java

@@ -22,6 +22,7 @@
 import com.logicalclocks.servicediscoverclient.resolvers.Type;
 import com.logicalclocks.servicediscoverclient.service.Service;
 import com.logicalclocks.servicediscoverclient.service.ServiceQuery;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 import javax.ws.rs.core.UriBuilder;
 import java.net.InetAddress;
@@ -38,6 +39,7 @@
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
+@SuppressFBWarnings(justification="Used in a singleton", value="AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION")
 public class Settings {
   private final static Logger LOGGER = Logger.getLogger(Settings.class.getName());
   public static final String MANAGEMENT_API_HEALTH = "/-/healthy";

alerting/src/main/java/io/hops/hopsworks/alerting/config/AlertManagerConfigController.java

@@ -22,6 +22,7 @@
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
 import com.fasterxml.jackson.dataformat.yaml.YAMLGenerator;
 import com.google.common.base.Strings;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import io.hops.hopsworks.alerting.api.AlertManagerClient;
 import io.hops.hopsworks.alerting.config.dto.AlertManagerConfig;
 import io.hops.hopsworks.alerting.exceptions.AlertManagerConfigFileNotFoundException;
@@ -33,6 +34,7 @@
 import java.io.File;
 import java.io.IOException;
 
+@SuppressFBWarnings(justification="Used only as default", value="DMI_HARDCODED_ABSOLUTE_FILENAME")
 public class AlertManagerConfigController {
   private static final String CONFIG_FILE_PATH = "/srv/hops/alertmanager/alertmanager/alertmanager.yml";
 

alerting/src/main/java/io/hops/hopsworks/alerting/config/dto/Responder.java

@@ -28,10 +28,13 @@ public class Responder {
   public Responder() {
   }
 
-  public Responder(String id, String name, String username) {
+  private Responder(String id, String name, String username) {
     this.id = id;
     this.name = name;
     this.username = username;
+  }
+
+  public static Responder getInstance(String id, String name, String username) {
     String errorMsg = "Exactly one of id, name or username fields should be defined.";
     if (!Strings.isNullOrEmpty(id) && (!Strings.isNullOrEmpty(name) || !Strings.isNullOrEmpty(username))) {
       throw new IllegalStateException(errorMsg);
@@ -42,6 +45,7 @@ public Responder(String id, String name, String username) {
     if (!Strings.isNullOrEmpty(username) && (!Strings.isNullOrEmpty(name) || !Strings.isNullOrEmpty(id))) {
       throw new IllegalStateException(errorMsg);
     }
+    return new Responder(id, name, username);
   }
 
   public String getId() {

hopsworks-alert/src/main/java/io/hops/hopsworks/alert/AMClient.java

@@ -128,7 +128,7 @@ void tryBuildClient() {
   }
 
   void createRetryTimer() {
-    long duration = Constants.RETRY_SECONDS * 1000;
+    long duration = Constants.RETRY_SECONDS * 1000L;
     if (count > Constants.NUM_RETRIES) {
       duration *= Constants.NUM_RETRIES;
     } else {

hopsworks-api/src/main/java/io/hops/hopsworks/api/admin/dto/MaterializerStateResponse.java

@@ -98,7 +98,7 @@ public void setMaterialKeyLocks(Map<String, Boolean> materialKeyLocks) {
     this.materialKeyLocks = materialKeyLocks;
   }
 
-  public static class CryptoMaterial {
+  public static class CryptoMaterial implements Serializable {
     private String user;
     private String path;
     private Integer references;

hopsworks-api/src/main/java/io/hops/hopsworks/api/dataset/tags/InodeTagUri.java

@@ -24,7 +24,7 @@
 import javax.ws.rs.core.UriInfo;
 
 public class InodeTagUri implements TagUri {
-  private static UriInfo uriInfo;
+  private final UriInfo uriInfo;
 
   public InodeTagUri(UriInfo uriInfo) {
     this.uriInfo = uriInfo;

hopsworks-api/src/main/java/io/hops/hopsworks/api/filter/ApiKeyFilter.java

@@ -16,6 +16,7 @@
 
 package io.hops.hopsworks.api.filter;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import io.hops.hopsworks.api.auth.Configuration;
 import io.hops.hopsworks.api.auth.UserStatusValidator;
 import io.hops.hopsworks.api.auth.UserUtilities;
@@ -41,6 +42,7 @@
 @Provider
 @ApiKeyRequired
 @Priority(Priorities.AUTHENTICATION - 1)
+@SuppressFBWarnings(justification = "Should be renamed", value = "NM_SAME_SIMPLE_NAME_AS_SUPERCLASS")
 public class ApiKeyFilter extends io.hops.hopsworks.api.auth.key.ApiKeyFilter {
   @EJB
   private UserStatusValidator userStatusValidator;

hopsworks-api/src/main/java/io/hops/hopsworks/api/jwt/JWTHelper.java

@@ -54,7 +54,6 @@
 import javax.ws.rs.core.SecurityContext;
 import java.io.IOException;
 import java.security.NoSuchAlgorithmException;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;
@@ -78,10 +77,7 @@
 public class JWTHelper {
   private static final Logger LOGGER = Logger.getLogger(JWTHelper.class.getName());
 
-  public static final List<String> SERVICE_RENEW_JWT_AUDIENCE = new ArrayList<>(1);
-  static {
-    SERVICE_RENEW_JWT_AUDIENCE.add(Audience.SERVICES);
-  }
+  public static final List<String> SERVICE_RENEW_JWT_AUDIENCE = Collections.singletonList(Audience.SERVICES);
 
   @EJB
   private JWTController jwtController;

hopsworks-api/src/main/java/io/hops/hopsworks/api/jwt/JWTRequestDTO.java

@@ -22,6 +22,7 @@
 import javax.xml.bind.annotation.XmlAccessorType;
 import javax.xml.bind.annotation.XmlRootElement;
 import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter;
+import java.util.Arrays;
 import java.util.Date;
 
 @XmlRootElement
@@ -125,9 +126,9 @@ public void setExpLeeway(int expLeeway) {
 
   @Override
   public String toString() {
-    return "JWTRequestDTO{" + "subject=" + subject + ", audiences=" + audiences + ", keyName=" + keyName
-        + ", expiresAt=" + expiresAt + ", notBefore=" + nbf + ", renewable=" + renewable + ", expLeeway="
-        + expLeeway + '}';
+    return "JWTRequestDTO{" + "subject=" + subject + ", audiences=" + Arrays.toString(audiences) + ", keyName=" +
+      keyName + ", expiresAt=" + expiresAt + ", notBefore=" + nbf + ", renewable=" + renewable + ", expLeeway="
+      + expLeeway + '}';
   }
 
 }

hopsworks-api/src/main/java/io/hops/hopsworks/api/project/util/PathValidator.java

@@ -123,7 +123,7 @@ private void buildFullPath(Project project, String path, DsPath dsPath) throws D
     while (path.startsWith("/")) {
       path = path.substring(1);
     }
-    String[] pathComponents = path.split(File.separator);
+    String[] pathComponents = path.split("/");
 
     String dsName = pathComponents[0];
     boolean shared = false;

hopsworks-api/src/main/java/io/hops/hopsworks/api/proxy/ProxyServlet.java

@@ -437,7 +437,7 @@ protected void consumeQuietly(HttpEntity entity) {
    * I use an HttpClient HeaderGroup class instead of Set<String> because this
    * approach does case insensitive lookup faster.
    */
-  protected static HeaderGroup hopByHopHeaders;
+  protected static final HeaderGroup hopByHopHeaders;
 
   static {
     hopByHopHeaders = new HeaderGroup();

hopsworks-api/src/main/java/io/hops/hopsworks/api/python/library/LibraryDTO.java

@@ -15,6 +15,7 @@
  */
 package io.hops.hopsworks.api.python.library;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import io.hops.hopsworks.api.python.command.CommandDTO;
 import io.hops.hopsworks.common.api.RestDTO;
 import io.hops.hopsworks.common.python.library.PackageSource;
@@ -103,6 +104,8 @@ public void setPackageSource(PackageSource packageSource) {
   }
 
   @Override
+  @SuppressFBWarnings(justification = "Can be compared to PythonDep",
+    value = "EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS")
   public boolean equals(Object o) {
     if (o instanceof LibraryDTO) {
       LibraryDTO pd = (LibraryDTO) o;
@@ -116,13 +119,11 @@ public boolean equals(Object o) {
     }
     if (o instanceof PythonDep) {
       PythonDep pd = (PythonDep) o;
-      if (pd.getRepoUrl().compareToIgnoreCase(this.channel) == 0
-          && pd.getInstallType().name().equalsIgnoreCase(this.getPackageSource().name())
-          && pd.getDependency().compareToIgnoreCase(this.library) == 0
-          && pd.getVersion().compareToIgnoreCase(this.version) == 0
-          && Boolean.toString(pd.isPreinstalled()).compareToIgnoreCase(this.preinstalled) == 0) {
-        return true;
-      }
+      return pd.getRepoUrl().compareToIgnoreCase(this.channel) == 0
+        && pd.getInstallType().name().equalsIgnoreCase(this.getPackageSource().name())
+        && pd.getDependency().compareToIgnoreCase(this.library) == 0
+        && pd.getVersion().compareToIgnoreCase(this.version) == 0
+        && Boolean.toString(pd.isPreinstalled()).compareToIgnoreCase(this.preinstalled) == 0;
     }
     return false;
   }

hopsworks-api/src/main/java/io/hops/hopsworks/api/user/apiKey/ApiKeyResource.java

@@ -15,6 +15,7 @@
  */
 package io.hops.hopsworks.api.user.apiKey;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import io.hops.hopsworks.api.auth.key.ApiKeyUtilities;
 import io.hops.hopsworks.api.filter.AllowedProjectRoles;
 import io.hops.hopsworks.api.filter.Audience;
@@ -230,6 +231,9 @@ public Response checkSession(@Context SecurityContext sc,
   // For a strange reason the Set of user supplied ApiScope(s) is marshalled
   // to String even though it's a Set of ApiScope. We need to explicitly convert
   // them to ApiScope
+  @SuppressFBWarnings(justification = "For a strange reason the Set of user supplied ApiScope(s) is marshalled to " +
+    "String even though it's a Set of ApiScope. We need to explicitly convert them to ApiScope",
+    value = "BC_IMPOSSIBLE_CAST")
   private Set<ApiScope> validateScopes(Users user, Set<ApiScope> scopes) throws ApiKeyException {
     Set<ApiScope> validScopes = getScopesForUser(user);
     Set<ApiScope> validatedScopes = new HashSet<>(scopes.size());

hopsworks-api/src/main/java/io/hops/hopsworks/api/util/UploadService.java

@@ -38,12 +38,11 @@
  */
 package io.hops.hopsworks.api.util;
 
+import io.hops.hopsworks.api.auth.key.ApiKeyRequired;
 import io.hops.hopsworks.api.filter.AllowedProjectRoles;
 import io.hops.hopsworks.api.filter.Audience;
-import io.hops.hopsworks.api.auth.key.ApiKeyRequired;
 import io.hops.hopsworks.api.jwt.JWTHelper;
 import io.hops.hopsworks.common.dao.project.ProjectFacade;
-import io.hops.hopsworks.common.dataset.DatasetController;
 import io.hops.hopsworks.common.dataset.util.DatasetHelper;
 import io.hops.hopsworks.common.dataset.util.DatasetPath;
 import io.hops.hopsworks.common.hdfs.HdfsUsersController;
@@ -75,7 +74,6 @@
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.SecurityContext;
-import java.io.IOException;
 import java.io.InputStream;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -86,8 +84,6 @@ public class UploadService {
 
   private static final Logger LOGGER = Logger.getLogger(UploadService.class.getName());
 
-  @EJB
-  private DatasetController datasetController;
   @EJB
   private JWTHelper jWTHelper;
   @EJB
@@ -177,8 +173,7 @@ public Response testMethod(@QueryParam("flowChunkNumber") String flowChunkNumber
       @QueryParam("flowRelativePath") String flowRelativePath,
       @QueryParam("flowTotalChunks") String flowTotalChunks,
       @QueryParam("flowTotalSize") String flowTotalSize,
-      @Context HttpServletRequest request, @Context SecurityContext sc) throws IOException, DatasetException,
-      ProjectException {
+      @Context HttpServletRequest request, @Context SecurityContext sc) throws DatasetException, ProjectException {
     configureUploader(sc);
     RESTApiJsonResponse json = new RESTApiJsonResponse();
     FlowInfo flowInfo = new FlowInfo(

hopsworks-common/src/main/java/io/hops/hopsworks/common/agent/AgentController.java

@@ -32,6 +32,7 @@
 import javax.ejb.Stateless;
 import javax.ejb.TransactionAttribute;
 import javax.ejb.TransactionAttributeType;
+import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.Date;
@@ -240,7 +241,7 @@ public ServiceStatus getStatus() {
     }
   }
 
-  private static class CommandsComparator<T> implements Comparator<T> {
+  private static class CommandsComparator<T> implements Comparator<T>, Serializable {
 
     @Override
     public int compare(T t, T t1) {

hopsworks-common/src/main/java/io/hops/hopsworks/common/alert/AlertController.java

@@ -330,10 +330,10 @@ public PostableAlert getPostableFeatureMonitorAlert(Project project, FeatureStor
         .withFeatureMonitorConfig(fmConfigName, fmResultId)
         .withSummary(summary)
         .withDescription(description);
-    if (resourceName.equals(ResourceRequest.Name.FEATUREGROUPS)) {
+    if (resourceName.equals(ResourceRequest.Name.FEATUREGROUPS) && featureStoreAlert instanceof FeatureGroupAlert) {
       builder.withFeatureGroupName(((FeatureGroupAlert) featureStoreAlert).getFeatureGroup().getName())
         .withFeatureGroupVersion(((FeatureGroupAlert) featureStoreAlert).getFeatureGroup().getVersion());
-    } else if (resourceName.equals(ResourceRequest.Name.FEATUREVIEW)) {
+    } else if (resourceName.equals(ResourceRequest.Name.FEATUREVIEW) && featureStoreAlert instanceof FeatureViewAlert) {
       builder.withFeatureViewVersion(((FeatureViewAlert) featureStoreAlert).getFeatureView().getVersion())
         .withFeatureViewName(((FeatureViewAlert) featureStoreAlert).getFeatureView().getName());
     }

hopsworks-common/src/main/java/io/hops/hopsworks/common/arrowflight/ArrowFlightController.java

@@ -124,7 +124,7 @@ private FlightClient initFlightClient(Project project, Users user)
       // register client certificates
       ArrowFlightCredentialDTO arrowFlightCredentials = new ArrowFlightCredentialDTO(accessCredentialsDTO);
       flightClient.doAction(new Action("register-client-certificates",
-              objectMapper.writeValueAsString(arrowFlightCredentials).getBytes()))
+              objectMapper.writeValueAsString(arrowFlightCredentials).getBytes(StandardCharsets.UTF_8)))
           .hasNext();
 
       return flightClient;

hopsworks-common/src/main/java/io/hops/hopsworks/common/dao/AbstractFacade.java

@@ -39,10 +39,13 @@
 
 package io.hops.hopsworks.common.dao;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /*
  * Only for compatibility purposes
  */
 @Deprecated
+@SuppressFBWarnings(justification = "Should be removed", value = "NM_SAME_SIMPLE_NAME_AS_SUPERCLASS")
 public abstract class AbstractFacade<T> extends io.hops.hopsworks.persistence.entity.util.AbstractFacade<T> {
 
   public AbstractFacade(Class<T> entityClass) {

hopsworks-common/src/main/java/io/hops/hopsworks/common/dao/hdfs/HdfsLeDescriptorsFacade.java

@@ -44,14 +44,15 @@
 import javax.persistence.EntityManager;
 import javax.persistence.NoResultException;
 import javax.persistence.PersistenceContext;
+
 import io.hops.hopsworks.common.dao.AbstractFacade;
 import io.hops.hopsworks.persistence.entity.hdfs.HdfsLeDescriptors;
 
 import java.util.Random;
 
 @Stateless
 public class HdfsLeDescriptorsFacade extends AbstractFacade<HdfsLeDescriptors> {
-
+  private static final Random RANDOM = new Random();
   @PersistenceContext(unitName = "kthfsPU")
   private EntityManager em;
 
@@ -78,7 +79,7 @@ public String getRPCEndpoint() {
         return null;
       } else {
         // Get a random NN Address
-        int randomNNIndex = new Random().nextInt(hdfsLeDescriptorsList.size());
+        int randomNNIndex = RANDOM.nextInt(hdfsLeDescriptorsList.size());
         HdfsLeDescriptors randomNN = hdfsLeDescriptorsList.get(randomNNIndex);
         String rpcAddresses = randomNN.getRpcAddresses();
         rpcAddresses = rpcAddresses.trim();

hopsworks-common/src/main/java/io/hops/hopsworks/common/dao/tensorflow/config/TensorBoardProcessMgr.java

@@ -133,7 +133,7 @@ public TensorBoardDTO startTensorBoard(Project project, Users user, HdfsUsers hd
 
     String anacondaEnvironmentPath = settings.getAnacondaProjectDir();
     int retries = 3;
-    while(retries > 0) {
+    while(retries >= 0) {
       try {
         if(retries == 0) {
           throw new IOException("Failed to start TensorBoard for project=" + project.getName() + ", user="