[RW-183] Create WorkspaceService to handle research purpose review. (#…

…239)

* Factor out WorkspaceService (wrapping WorkspaceDao).
* Combine copies of 'get workspace and throw if it doesn not exist'.
* Add methods for research purpose review, and associated tests.
* Incidental fix of string formatting in exception messages (in WS files only).

TODOs:
 * `@Version` to prevent conflicting edits. RW-215
 * Fix remaining string formatting in exceptions. RW-217
 * Remove remaining redundant Java index declarations. RW-220

api/db/changelog/db.changelog-8-ws-index.xml

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<databaseChangeLog
+    xmlns="http://www.liquibase.org/xml/ns/dbchangelog/1.9"
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog/1.9
+                      http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-1.9.xsd">
+  <changeSet author="markfickett" id="changelog-8-ws-index">
+    <createIndex
+        indexName="idx_workspace_rp"
+        tableName="workspace"
+        unique="true">
+      <column name="rp_review_requested"/>
+      <column name="rp_approved"/>
+    </createIndex>
+  </changeSet>
+</databaseChangeLog>

api/db/changelog/db.changelog-master.xml

@@ -15,4 +15,5 @@
   <include file="changelog/db.changelog-5-authority.xml"/>
   <include file="changelog/db.changelog-6.xml"/>
   <include file="changelog/db.changelog-7.xml"/>
+  <include file="changelog/db.changelog-8-ws-index.xml"/>
 </databaseChangeLog>

api/src/main/java/org/pmiops/workbench/Application.java

@@ -2,12 +2,11 @@
 
 import org.springframework.boot.SpringApplication;
 import org.springframework.boot.autoconfigure.SpringBootApplication;
-import org.springframework.context.annotation.ComponentScan;
 
 @SpringBootApplication
 public class Application {
 
   public static void main(String[] args) {
     SpringApplication.run(Application.class, args);
   }
-}
+}

api/src/main/java/org/pmiops/workbench/api/CohortsController.java

@@ -8,7 +8,7 @@
 import java.util.stream.Collectors;
 import javax.inject.Provider;
 import org.pmiops.workbench.db.dao.CohortDao;
-import org.pmiops.workbench.db.dao.WorkspaceDao;
+import org.pmiops.workbench.db.dao.WorkspaceService;
 import org.pmiops.workbench.db.model.User;
 import org.pmiops.workbench.db.model.Workspace;
 import org.pmiops.workbench.exceptions.BadRequestException;
@@ -61,15 +61,18 @@ public Cohort apply(org.pmiops.workbench.db.model.Cohort cohort) {
         }
       };
 
-  private final WorkspaceDao workspaceDao;
+  private final WorkspaceService workspaceService;
   private final CohortDao cohortDao;
   private final Provider<User> userProvider;
   private final Clock clock;
 
   @Autowired
-  CohortsController(WorkspaceDao workspaceDao, CohortDao cohortDao, Provider<User> userProvider,
+  CohortsController(
+      WorkspaceService workspaceService,
+      CohortDao cohortDao,
+      Provider<User> userProvider,
       Clock clock) {
-    this.workspaceDao = workspaceDao;
+    this.workspaceService = workspaceService;
     this.cohortDao = cohortDao;
     this.userProvider = userProvider;
     this.clock = clock;
@@ -78,7 +81,7 @@ public Cohort apply(org.pmiops.workbench.db.model.Cohort cohort) {
   @Override
   public ResponseEntity<Cohort> createCohort(String workspaceNamespace, String workspaceId,
       Cohort cohort) {
-    Workspace workspace = getDbWorkspace(workspaceNamespace, workspaceId);
+    Workspace workspace = workspaceService.getRequired(workspaceNamespace, workspaceId);
     Timestamp now = new Timestamp(clock.instant().toEpochMilli());
     org.pmiops.workbench.db.model.Cohort dbCohort = FROM_CLIENT_COHORT.apply(cohort);
     dbCohort.setCreator(userProvider.get());
@@ -118,7 +121,7 @@ public Cohort apply(org.pmiops.workbench.db.model.Cohort cohort) {
   @Override
   public ResponseEntity<CohortListResponse> getCohortsInWorkspace(String workspaceNamespace,
       String workspaceId) {
-    Workspace workspace = getDbWorkspace(workspaceNamespace, workspaceId);
+    Workspace workspace = workspaceService.getRequired(workspaceNamespace, workspaceId);
     CohortListResponse response = new CohortListResponse();
     List<org.pmiops.workbench.db.model.Cohort> cohorts = workspace.getCohorts();
     if (cohorts != null) {
@@ -151,23 +154,9 @@ public Cohort apply(org.pmiops.workbench.db.model.Cohort cohort) {
     return ResponseEntity.ok(TO_CLIENT_COHORT.apply(dbCohort));
   }
 
-  /**
-   * Gets or creates a workspace with the given namespace and ID.
-   * (In future it will throw NotFoundException if the workspace wasn't created previously.)
-   */
-  private Workspace getDbWorkspace(String workspaceNamespace, String workspaceId) {
-    Workspace workspace = workspaceDao.findByWorkspaceNamespaceAndFirecloudName(workspaceNamespace,
-        workspaceId);
-    if (workspace == null) {
-      throw new NotFoundException("No workspace with name {0}/{1}"
-          .format(workspaceNamespace, workspaceId));
-    }
-    return workspace;
-  }
-
   private org.pmiops.workbench.db.model.Cohort getDbCohort(String workspaceName,
       String workspaceId, String cohortId) {
-    Workspace workspace = getDbWorkspace(workspaceName, workspaceId);
+    Workspace workspace = workspaceService.getRequired(workspaceName, workspaceId);
 
     org.pmiops.workbench.db.model.Cohort cohort =
         cohortDao.findOne(convertCohortId(cohortId));

api/src/main/java/org/pmiops/workbench/api/WorkspacesController.java

@@ -10,23 +10,29 @@
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 import javax.inject.Provider;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.http.ResponseEntity;
+import org.springframework.web.bind.annotation.RestController;
+
+import org.pmiops.workbench.annotations.AuthorityRequired;
 import org.pmiops.workbench.db.dao.CdrVersionDao;
-import org.pmiops.workbench.db.dao.WorkspaceDao;
+import org.pmiops.workbench.db.dao.WorkspaceService;
 import org.pmiops.workbench.db.model.CdrVersion;
 import org.pmiops.workbench.db.model.User;
 import org.pmiops.workbench.db.model.Workspace.FirecloudWorkspaceId;
 import org.pmiops.workbench.exceptions.BadRequestException;
-import org.pmiops.workbench.exceptions.NotFoundException;
 import org.pmiops.workbench.exceptions.ServerErrorException;
 import org.pmiops.workbench.firecloud.FireCloudService;
+import org.pmiops.workbench.model.Authority;
 import org.pmiops.workbench.model.DataAccessLevel;
+import org.pmiops.workbench.model.EmptyResponse;
 import org.pmiops.workbench.model.ResearchPurpose;
+import org.pmiops.workbench.model.ResearchPurposeReviewRequest;
 import org.pmiops.workbench.model.Workspace;
 import org.pmiops.workbench.model.Workspace.DataAccessLevelEnum;
 import org.pmiops.workbench.model.WorkspaceListResponse;
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.http.ResponseEntity;
-import org.springframework.web.bind.annotation.RestController;
+
 
 @RestController
 public class WorkspacesController implements WorkspacesApiDelegate {
@@ -121,16 +127,20 @@ public Workspace apply(org.pmiops.workbench.db.model.Workspace workspace) {
       };
 
 
-  private final WorkspaceDao workspaceDao;
+  private final WorkspaceService workspaceService;
   private final CdrVersionDao cdrVersionDao;
   private final Provider<User> userProvider;
   private final FireCloudService fireCloudService;
   private final Clock clock;
 
   @Autowired
-  WorkspacesController(WorkspaceDao workspaceDao, CdrVersionDao cdrVersionDao,
-      Provider<User> userProvider, FireCloudService fireCloudService, Clock clock) {
-    this.workspaceDao = workspaceDao;
+  WorkspacesController(
+      WorkspaceService workspaceService,
+      CdrVersionDao cdrVersionDao,
+      Provider<User> userProvider,
+      FireCloudService fireCloudService,
+      Clock clock) {
+    this.workspaceService = workspaceService;
     this.cdrVersionDao = cdrVersionDao;
     this.userProvider = userProvider;
     this.fireCloudService = fireCloudService;
@@ -152,13 +162,13 @@ private void setCdrVersionId(Workspace workspace,
       try {
         CdrVersion cdrVersion = cdrVersionDao.findOne(Long.parseLong(workspace.getCdrVersionId()));
         if (cdrVersion == null) {
-          throw new BadRequestException("CDR version with ID {0} not found"
-              .format(workspace.getCdrVersionId()));
+          throw new BadRequestException(
+              String.format("CDR version with ID %s not found", workspace.getCdrVersionId()));
         }
         dbWorkspace.setCdrVersion(cdrVersion);
       } catch (NumberFormatException e) {
-        throw new BadRequestException("Invalid cdr version ID: {0}"
-            .format(workspace.getCdrVersionId()));
+        throw new BadRequestException(String.format(
+            "Invalid cdr version ID: %s", workspace.getCdrVersionId()));
       }
     }
   }
@@ -186,10 +196,11 @@ private FirecloudWorkspaceId generateFirecloudWorkspaceId(String namespace, Stri
     }
     FirecloudWorkspaceId workspaceId = generateFirecloudWorkspaceId(workspace.getNamespace(),
         workspace.getName());
-    org.pmiops.workbench.db.model.Workspace existingWorkspace =
-        getDbWorkspace(workspaceId.getWorkspaceNamespace(), workspaceId.getWorkspaceName());
+    org.pmiops.workbench.db.model.Workspace existingWorkspace = workspaceService.get(
+        workspaceId.getWorkspaceNamespace(), workspaceId.getWorkspaceName());
     if (existingWorkspace != null) {
-      throw new BadRequestException("Workspace {0}/{1} already exists".format(
+      throw new BadRequestException(String.format(
+          "Workspace %s/%s already exists",
           workspaceId.getWorkspaceNamespace(), workspaceId.getWorkspaceName()));
     }
     try {
@@ -211,21 +222,21 @@ private FirecloudWorkspaceId generateFirecloudWorkspaceId(String namespace, Stri
     dbWorkspace.setCreationTime(now);
     dbWorkspace.setLastModifiedTime(now);
     setCdrVersionId(workspace, dbWorkspace);
-    dbWorkspace = workspaceDao.save(dbWorkspace);
+    dbWorkspace = workspaceService.dao.save(dbWorkspace);
     return ResponseEntity.ok(TO_CLIENT_WORKSPACE.apply(dbWorkspace));
   }
 
   @Override
   public ResponseEntity<Void> deleteWorkspace(String workspaceNamespace, String workspaceId) {
-    org.pmiops.workbench.db.model.Workspace dbWorkspace = getDbWorkspaceCheckExists(
+    org.pmiops.workbench.db.model.Workspace dbWorkspace = workspaceService.getRequired(
         workspaceNamespace, workspaceId);
-    workspaceDao.delete(dbWorkspace);
+    workspaceService.dao.delete(dbWorkspace);
     return ResponseEntity.ok(null);
   }
 
   @Override
   public ResponseEntity<Workspace> getWorkspace(String workspaceNamespace, String workspaceId) {
-    org.pmiops.workbench.db.model.Workspace dbWorkspace = getDbWorkspaceCheckExists(
+    org.pmiops.workbench.db.model.Workspace dbWorkspace = workspaceService.getRequired(
         workspaceNamespace, workspaceId);
     return ResponseEntity.ok(TO_CLIENT_WORKSPACE.apply(dbWorkspace));
   }
@@ -239,7 +250,7 @@ private FirecloudWorkspaceId generateFirecloudWorkspaceId(String namespace, Stri
     if (user == null) {
       workspaces = new ArrayList<>();
     } else {
-      workspaces = workspaceDao.findByCreatorOrderByNameAsc(userProvider.get());
+      workspaces = workspaceService.dao.findByCreatorOrderByNameAsc(userProvider.get());
     }
     WorkspaceListResponse response = new WorkspaceListResponse();
     response.setItems(workspaces.stream().map(TO_CLIENT_WORKSPACE).collect(Collectors.toList()));
@@ -249,7 +260,7 @@ private FirecloudWorkspaceId generateFirecloudWorkspaceId(String namespace, Stri
   @Override
   public ResponseEntity<Workspace> updateWorkspace(String workspaceNamespace, String workspaceId,
       Workspace workspace) {
-    org.pmiops.workbench.db.model.Workspace dbWorkspace = getDbWorkspaceCheckExists(
+    org.pmiops.workbench.db.model.Workspace dbWorkspace = workspaceService.getRequired(
         workspaceNamespace, workspaceId);
     if (workspace.getDataAccessLevel() != null) {
       dbWorkspace.setDataAccessLevel(
@@ -266,23 +277,30 @@ private FirecloudWorkspaceId generateFirecloudWorkspaceId(String namespace, Stri
     Timestamp now = new Timestamp(clock.instant().toEpochMilli());
     dbWorkspace.setLastModifiedTime(now);
     // TODO: add version, check it here
-    dbWorkspace = workspaceDao.save(dbWorkspace);
+    dbWorkspace = workspaceService.dao.save(dbWorkspace);
     return ResponseEntity.ok(TO_CLIENT_WORKSPACE.apply(dbWorkspace));
   }
 
-  private org.pmiops.workbench.db.model.Workspace getDbWorkspace(String workspaceNamespace,
-      String firecloudName) {
-    return workspaceDao.findByWorkspaceNamespaceAndFirecloudName(workspaceNamespace, firecloudName);
+  /** Record approval or rejection of research purpose. */
+  @AuthorityRequired({Authority.REVIEW_RESEARCH_PURPOSE})
+  public ResponseEntity<EmptyResponse> reviewWorkspace(
+      String ns, String id, ResearchPurposeReviewRequest review) {
+    workspaceService.setResearchPurposeApproved(ns, id, review.getApproved());
+    return ResponseEntity.ok(new EmptyResponse());
   }
 
-  private org.pmiops.workbench.db.model.Workspace getDbWorkspaceCheckExists(
-      String workspaceNamespace, String workspaceId) {
-    org.pmiops.workbench.db.model.Workspace workspace = getDbWorkspace(workspaceNamespace,
-        workspaceId);
-    if (workspace == null) {
-      throw new NotFoundException("Workspace {0}/{1} not found".format(workspaceNamespace,
-          workspaceId));
-    }
-    return workspace;
+
+  // Note we do not paginate the workspaces list, since we expect few workspaces
+  // to require review.
+  //
+  // We can add pagination in the DAO by returning Slice<Workspace> if we want the method to return
+  // pagination information (e.g. are there more workspaces to get), and Page<Workspace> if we
+  // want the method to return both pagination information and a total count.
+  @AuthorityRequired({Authority.REVIEW_RESEARCH_PURPOSE})
+  public ResponseEntity<WorkspaceListResponse> getWorkspacesForReview() {
+    WorkspaceListResponse response = new WorkspaceListResponse();
+    List<org.pmiops.workbench.db.model.Workspace> workspaces = workspaceService.findForReview();
+    response.setItems(workspaces.stream().map(TO_CLIENT_WORKSPACE).collect(Collectors.toList()));
+    return ResponseEntity.ok(response);
   }
 }

api/src/main/java/org/pmiops/workbench/db/dao/README.md

@@ -1,2 +1,6 @@
 DAOs' query implementations are
 [automatically derived](https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#repositories.query-methods.details).
+
+Services implement custom behavior which can't be auto-generated. Where both a
+Service and a DAO are available, inject the Service and use its public dao field
+where necessary. [Design doc](https://docs.google.com/document/d/1uNf6_5TZxnQt8BP_wWoGxPcGwaIZAhZswW3bXwbswHU).

api/src/main/java/org/pmiops/workbench/db/dao/WorkspaceDao.java

@@ -4,10 +4,19 @@
 import org.pmiops.workbench.db.model.User;
 import org.pmiops.workbench.db.model.Workspace;
 import org.springframework.data.repository.CrudRepository;
+import org.springframework.data.jpa.repository.Query;
 
+
+/**
+ * Declaration of automatic query methods for Workspaces. The methods declared here are
+ * automatically interpreted by Spring Data (see README).
+ *
+ * Use of @Query is discouraged; if desired, define aliases in WorkspaceService.
+ */
 public interface WorkspaceDao extends CrudRepository<Workspace, Long> {
+  Workspace findByWorkspaceNamespaceAndFirecloudName(
+      String workspaceNamespace, String firecloudName);
   List<Workspace> findByWorkspaceNamespace(String workspaceNamespace);
-  Workspace findByWorkspaceNamespaceAndFirecloudName(String workspaceNamespace,
-      String firecloudName);
   List<Workspace> findByCreatorOrderByNameAsc(User creator);
+  List<Workspace> findByApprovedIsNullAndReviewRequestedTrueOrderByTimeRequested();
 }

api/src/main/java/org/pmiops/workbench/db/dao/WorkspaceService.java

@@ -0,0 +1,68 @@
+package org.pmiops.workbench.db.dao;
+
+import java.util.List;
+import java.util.logging.Logger;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+import org.springframework.transaction.annotation.Transactional;
+
+import org.pmiops.workbench.db.model.Workspace;
+import org.pmiops.workbench.exceptions.BadRequestException;
+import org.pmiops.workbench.exceptions.NotFoundException;
+
+
+/**
+ * Workspace manipulation and shared business logic which can't be represented by automatic query
+ * generation in WorkspaceDao, or convenience aliases.
+ *
+ * TODO(RW-215) Add versioning to detect/prevent concurrent edits.
+ */
+@Service
+public class WorkspaceService {
+  private static final Logger log = Logger.getLogger(WorkspaceService.class.getName());
+
+  /**
+   * Clients wishing to use the auto-generated methods from the DAO interface may directly access
+   * it here.
+   */
+  public final WorkspaceDao dao;
+
+  @Autowired
+  public WorkspaceService(WorkspaceDao workspaceDao) {
+    this.dao = workspaceDao;
+  }
+
+  public Workspace get(String ns, String id) {
+    return dao.findByWorkspaceNamespaceAndFirecloudName(ns, id);
+  }
+
+  public Workspace getRequired(String ns, String id) {
+    Workspace workspace = get(ns, id);
+    if (workspace == null) {
+      throw new NotFoundException(String.format("Workspace %s/%s not found.", ns, id));
+    }
+    return workspace;
+  }
+
+  public List<Workspace> findForReview() {
+    return dao.findByApprovedIsNullAndReviewRequestedTrueOrderByTimeRequested();
+  }
+
+  // FIXME @Version instead? Bean instantiation v. @Transactional?
+  //@Transactional
+  public void setResearchPurposeApproved(String ns, String id, boolean approved) {
+    Workspace workspace = getRequired(ns, id);
+    if (workspace.getReviewRequested() == null || !workspace.getReviewRequested()) {
+      throw new BadRequestException(String.format(
+          "No review requested for workspace %s/%s.", ns, id));
+    }
+    if (workspace.getApproved() != null) {
+      throw new BadRequestException(String.format(
+          "Workspace %s/%s already %s.",
+          ns, id, workspace.getApproved() ? "approved" : "rejected"));
+    }
+    workspace.setApproved(approved);
+    dao.save(workspace);
+  }
+}