[HWORKS-956] Sub-resources should not do work outside of the endpoint.

hopsworks-UT/src/test/java/io/hops/hopsworks/TestSubResources.java

@@ -0,0 +1,146 @@
+/*
+ * This file is part of Hopsworks
+ * Copyright (C) 2024, Hopsworks AB. All rights reserved
+ *
+ * Hopsworks is free software: you can redistribute it and/or modify it under the terms of
+ * the GNU Affero General Public License as published by the Free Software Foundation,
+ * either version 3 of the License, or (at your option) any later version.
+ *
+ * Hopsworks is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
+ * without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License along with this program.
+ * If not, see <https://www.gnu.org/licenses/>.
+ */
+package io.hops.hopsworks;
+
+import io.hops.hopsworks.common.dataset.util.DatasetPath;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.reflections.Reflections;
+import org.reflections.scanners.SubTypesScanner;
+import org.reflections.scanners.TypeAnnotationsScanner;
+
+import javax.enterprise.context.RequestScoped;
+import javax.persistence.Entity;
+import javax.ws.rs.Path;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * This test will ensure that sub resources do not contain entity class properties.
+ * 1. Sub resources are request scoped and so should not cache anything.
+ * 2. If an entity class is passed to a sub resource that means it needs to be fetched from database before the sub
+ * resource is called. This will bypass any filters that are set to intercept methods on the sub resource.
+ * <p>
+ * So if we have a project entity in a sub resource, and we pass it from a parent resource the project entity will be
+ * fetched from database before the user, that is making the request, is authenticated and verified if he/she has access
+ * to the project.
+ */
+public class TestSubResources {
+  Set<Class<?>> subResourceClasses = new HashSet<>();
+  Set<Class<?>> resourceClasses = new HashSet<>();
+  Set<Class<?>> entityClasses = new HashSet<>();
+
+  @Before
+  public void beforeTest() {
+    Reflections reflections = new Reflections("io.hops.hopsworks", new SubTypesScanner(false),
+      new TypeAnnotationsScanner());
+    for (Class<?> c : reflections.getTypesAnnotatedWith(Path.class, true)) {
+      if (c.getName().startsWith("io.hops.hopsworks.api")) {
+        resourceClasses.add(c);
+      }
+    }
+    for (Class<?> c : reflections.getTypesAnnotatedWith(RequestScoped.class, true)) {
+      if (c.getName().startsWith("io.hops.hopsworks.api")) {
+        subResourceClasses.add(c);
+      }
+    }
+
+    for (Class<?> c : reflections.getTypesAnnotatedWith(Entity.class, true)) {
+      if (c.getName().startsWith("io.hops.hopsworks.persistence.entity")) {
+        entityClasses.add(c);
+      }
+    }
+
+  }
+
+  @Test
+  public void testIfClassesAreFound() {
+    System.out.println("SubResources: " + subResourceClasses.size());
+    assertFalse("Sub Resource classes should not be empty.", subResourceClasses.isEmpty());
+    System.out.println("Entity classes: " + entityClasses.size());
+    assertFalse("Entity classes should not be empty.", entityClasses.isEmpty());
+  }
+
+  @Test
+  public void testSubResourceAreFinal() {
+    for (Class<?> c : resourceClasses) {
+      if (!Modifier.isFinal(c.getModifiers())) {
+        System.out.println("Api Resources should be final. Inheriting a resource class will force the swagger " +
+          "documentation to be too general. Offending classes " + c.getName());
+      }
+//      assertTrue("Api Resources should be final. Inheriting a resource class will force the swagger " +
+//          "documentation to be too general. Offending classes " + c.getName(),
+//        Modifier.isFinal(c.getModifiers()));
+    }
+    for (Class<?> c : subResourceClasses) {
+      if (!Modifier.isFinal(c.getModifiers())) {
+        System.out.println("Api Resources should be final. Inheriting a resource class will force the swagger " +
+          "documentation to be too general. Offending classes " + c.getName());
+      }
+//      assertTrue("Api Resources should be final. Inheriting a resource class will force the swagger " +
+//          "documentation to be too general. Offending classes " + c.getName(),
+//        Modifier.isFinal(c.getModifiers()));
+    }
+  }
+
+  @Test
+  public void testEntityClassesInFields() {
+    for (Class<?> c : subResourceClasses) {
+      Set<Class<?>> entityClassesInFields = containsEntity(c);
+      if (!entityClassesInFields.isEmpty()) {
+        System.out.println("Class " + c.getName() + " contains entity class fields. ");
+        System.out.println(entityClassesInFields);
+      }
+      assertTrue("SubResources should not contain entity classes fields that query the database.",
+        entityClassesInFields.isEmpty());
+    }
+  }
+
+  @Test
+  public void testDatasetPathClassesInFields() {
+    for (Class<?> c : subResourceClasses) {
+      if (containsClass(c, DatasetPath.class)) {
+        System.out.println("Class " + c.getName() + " contains DatasetPath in fields. ");
+        Assert.fail("SubResources should not contain any Class that queries the database. Found DatasetPath");
+      }
+    }
+  }
+
+  private Set<Class<?>> containsEntity(Class<?> c) {
+    Set<Class<?>> entityClassesInFields = new HashSet<>();
+    for (Field field : c.getDeclaredFields()) {
+      if (entityClasses.contains(field.getType())) {
+        entityClassesInFields.add(field.getType());
+      }
+    }
+    return entityClassesInFields;
+  }
+
+  private boolean containsClass(Class<?> c, Class<?> c1) {
+    for (Field field : c.getDeclaredFields()) {
+      if (field.getType().equals(c1)) {
+        return true;
+      }
+    }
+    return false;
+  }
+}

hopsworks-api/src/main/java/io/hops/hopsworks/api/activities/ProjectActivitiesResource.java

@@ -17,14 +17,14 @@
 
 import io.hops.hopsworks.api.filter.AllowedProjectRoles;
 import io.hops.hopsworks.api.filter.Audience;
+import io.hops.hopsworks.api.project.ProjectSubResource;
 import io.hops.hopsworks.api.util.Pagination;
 import io.hops.hopsworks.common.api.ResourceRequest;
-import io.hops.hopsworks.common.dao.project.ProjectFacade;
+import io.hops.hopsworks.common.project.ProjectController;
 import io.hops.hopsworks.exceptions.ActivitiesException;
 import io.hops.hopsworks.exceptions.ProjectException;
 import io.hops.hopsworks.jwt.annotation.JWTRequired;
 import io.hops.hopsworks.persistence.entity.project.Project;
-import io.hops.hopsworks.restutils.RESTCodes;
 import io.swagger.annotations.Api;
 import io.swagger.annotations.ApiOperation;
 
@@ -42,53 +42,25 @@
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.SecurityContext;
 import javax.ws.rs.core.UriInfo;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 
 @Api(value = "Project Activities Resource")
 @RequestScoped
 @TransactionAttribute(TransactionAttributeType.NEVER)
-public class ProjectActivitiesResource {
+public class ProjectActivitiesResource extends ProjectSubResource {
 
   private static final Logger LOGGER = Logger.getLogger(ProjectActivitiesResource.class.getName());
   @EJB
   private ActivitiesBuilder activitiesBuilder;
   @EJB
-  private ProjectFacade projectFacade;
-
-  private Integer projectId;
-  private String projectName;
+  private ProjectController projectController;
 
   public ProjectActivitiesResource() {
   }
 
-  public void setProjectId(Integer projectId) {
-    this.projectId = projectId;
-  }
-
-  public void setProjectName(String projectName) {
-    this.projectName = projectName;
-  }
-
-  private Project getProjectById() throws ProjectException {
-    Project project = projectFacade.find(this.projectId);
-    if (project == null) {
-      throw new ProjectException(RESTCodes.ProjectErrorCode.PROJECT_NOT_FOUND, Level.FINE, "projectId: " + projectId);
-    }
-    return project;
-  }
-
-  private Project getProjectByName() throws ProjectException {
-    Project project = projectFacade.findByName(this.projectName);
-    if (project == null) {
-      throw new ProjectException(RESTCodes.ProjectErrorCode.PROJECT_NOT_FOUND, Level.FINE, "projectName: " + 
-          projectName);
-    }
-    return project;
-  }
-
-  private Project getProject() throws ProjectException {
-    return this.projectId != null ? getProjectById() : getProjectByName();
+  @Override
+  protected ProjectController getProjectController() {
+    return projectController;
   }
 
   @GET

hopsworks-api/src/main/java/io/hops/hopsworks/api/airflow/AirflowService.java

@@ -19,12 +19,13 @@
 import io.hops.hopsworks.api.filter.AllowedProjectRoles;
 import io.hops.hopsworks.api.filter.Audience;
 import io.hops.hopsworks.api.jwt.JWTHelper;
+import io.hops.hopsworks.api.project.ProjectSubResource;
 import io.hops.hopsworks.common.airflow.AirflowController;
 import io.hops.hopsworks.common.airflow.AirflowDagDTO;
-import io.hops.hopsworks.common.dao.project.ProjectFacade;
+import io.hops.hopsworks.common.project.ProjectController;
 import io.hops.hopsworks.exceptions.AirflowException;
+import io.hops.hopsworks.exceptions.ProjectException;
 import io.hops.hopsworks.jwt.annotation.JWTRequired;
-import io.hops.hopsworks.persistence.entity.project.Project;
 import io.hops.hopsworks.persistence.entity.user.Users;
 import io.swagger.annotations.Api;
 import io.swagger.annotations.ApiOperation;
@@ -45,31 +46,20 @@
 @RequestScoped
 @TransactionAttribute(TransactionAttributeType.NEVER)
 @Api(value = "Airflow related endpoints")
-public class AirflowService {
+public class AirflowService extends ProjectSubResource {
   @EJB
-  private ProjectFacade projectFacade;
+  private ProjectController projectController;
   @EJB
   private JWTHelper jwtHelper;
   @EJB
   private AirflowController airflowController;
-  private Integer projectId;
-  // No @EJB annotation for Project, it's injected explicitly in ProjectService.
-  private Project project;
-
-
-  // Audience for Airflow JWTs
-  private static final String[] JWT_AUDIENCE = new String[]{Audience.API};
 
   public AirflowService() {
   }
 
-  public void setProjectId(Integer projectId) {
-    this.projectId = projectId;
-    this.project = this.projectFacade.find(projectId);
-  }
-
-  public Integer getProjectId() {
-    return projectId;
+  @Override
+  protected ProjectController getProjectController() {
+    return projectController;
   }
 
   @POST
@@ -80,9 +70,9 @@ public Integer getProjectId() {
   @ApiOperation(value = "Generate an Airflow Python DAG file from a DAG definition")
   public Response composeDAG(AirflowDagDTO dagDefinition,
                              @Context HttpServletRequest req,
-                             @Context SecurityContext sc) throws AirflowException {
+                             @Context SecurityContext sc) throws AirflowException, ProjectException {
     Users user = jwtHelper.getUserPrincipal(sc);
-    airflowController.composeDAG(project, user, dagDefinition);
+    airflowController.composeDAG(getProject(), user, dagDefinition);
     return Response.ok().build();
   }
-}
+}

hopsworks-api/src/main/java/io/hops/hopsworks/api/alert/AlertResource.java

@@ -27,13 +27,13 @@
 import io.hops.hopsworks.api.alert.silence.SilenceResource;
 import io.hops.hopsworks.api.filter.AllowedProjectRoles;
 import io.hops.hopsworks.api.filter.Audience;
+import io.hops.hopsworks.api.project.ProjectSubResource;
 import io.hops.hopsworks.api.util.Pagination;
 import io.hops.hopsworks.common.api.ResourceRequest;
 import io.hops.hopsworks.common.project.ProjectController;
 import io.hops.hopsworks.exceptions.AlertException;
 import io.hops.hopsworks.exceptions.ProjectException;
 import io.hops.hopsworks.jwt.annotation.JWTRequired;
-import io.hops.hopsworks.persistence.entity.project.Project;
 import io.hops.hopsworks.restutils.RESTCodes;
 import io.swagger.annotations.Api;
 import io.swagger.annotations.ApiOperation;
@@ -64,7 +64,7 @@
 @RequestScoped
 @Api(value = "Alert Resource")
 @TransactionAttribute(TransactionAttributeType.NEVER)
-public class AlertResource {
+public class AlertResource extends ProjectSubResource {
 
   private static final Logger LOGGER = Logger.getLogger(AlertResource.class.getName());
 
@@ -81,24 +81,9 @@ public class AlertResource {
   @Inject
   private ReceiverResource receiverResource;
 
-  private Integer projectId;
-  private String projectName;
-
-  public void setProjectId(Integer projectId) {
-    this.projectId = projectId;
-  }
-
-  public void setProjectName(String projectName) {
-    this.projectName = projectName;
-  }
-
-  private Project getProject() throws ProjectException {
-    if (this.projectId != null) {
-      return projectController.findProjectById(this.projectId);
-    } else if (this.projectName != null) {
-      return projectController.findProjectByName(this.projectName);
-    }
-    throw new ProjectException(RESTCodes.ProjectErrorCode.PROJECT_NOT_FOUND, Level.FINE);
+  @Override
+  protected ProjectController getProjectController() {
+    return projectController;
   }
 
   @GET
@@ -169,19 +154,19 @@ public Response createAlerts(PostableAlertDTOs alerts, @Context UriInfo uriInfo,
 
   @Path("silences")
   public SilenceResource silence() {
-    this.silenceResource.setProjectId(this.projectId);
+    this.silenceResource.setProjectId(getProjectId());
     return silenceResource;
   }
 
   @Path("receivers")
   public ReceiverResource receiver() {
-    this.receiverResource.setProjectId(this.projectId);
+    this.receiverResource.setProjectId(getProjectId());
     return receiverResource;
   }
 
   @Path("routes")
   public RouteResource route() {
-    this.routeResource.setProjectId(this.projectId);
+    this.routeResource.setProjectId(getProjectId());
     return routeResource;
   }
 }