Skip to content

Commit

Permalink
AJ-1602: list-jobs API no longer a passthrough to Import Service (#1305)
Browse files Browse the repository at this point in the history
  • Loading branch information
davidangb authored Mar 12, 2024
1 parent c53581f commit 664506a
Show file tree
Hide file tree
Showing 9 changed files with 145 additions and 96 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,14 @@ class EntityService(rawlsDAO: RawlsDAO, importServiceDAO: ImportServiceDAO, goog
importServiceDAO.importJob(workspaceNamespace, workspaceName, importRequest, isUpsert = true)(userInfo)
}

def listJobs(workspaceNamespace: String, workspaceName: String, runningOnly: Boolean, userInfo: UserInfo): Future[List[ImportServiceListResponse]] = {
// get jobs from Import Service
val importServiceJobs = importServiceDAO.listJobs(workspaceNamespace, workspaceName, runningOnly)(userInfo)
// TODO AJ-1602: get jobs from cWDS
// TODO AJ-1602: merge lists before replying
importServiceJobs
}

def getEntitiesWithType(workspaceNamespace: String, workspaceName: String, userInfo: UserInfo): Future[PerRequestMessage] = {
rawlsDAO.getEntityTypes(workspaceNamespace, workspaceName)(userInfo).flatMap { entityTypeResponse =>
val entityTypes = entityTypeResponse.keys.toList
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ import akka.http.scaladsl.model.{HttpEntity, HttpResponse}
import akka.http.scaladsl.model.StatusCodes._
import akka.http.scaladsl.unmarshalling.Unmarshal
import akka.stream.Materializer
import org.broadinstitute.dsde.firecloud.{FireCloudConfig, FireCloudExceptionWithErrorReport}
import org.broadinstitute.dsde.firecloud.model.{ImportServiceRequest, ImportServiceResponse, AsyncImportRequest, AsyncImportResponse, RequestCompleteWithErrorReport, UserInfo}
import org.broadinstitute.dsde.firecloud.FireCloudConfig
import org.broadinstitute.dsde.firecloud.model.{AsyncImportRequest, AsyncImportResponse, ImportServiceListResponse, ImportServiceRequest, ImportServiceResponse, RequestCompleteWithErrorReport, UserInfo}
import org.broadinstitute.dsde.firecloud.service.FireCloudDirectiveUtils
import org.broadinstitute.dsde.firecloud.service.PerRequest.{PerRequestMessage, RequestComplete}
import org.broadinstitute.dsde.firecloud.model.ModelJsonProtocol._
import org.broadinstitute.dsde.firecloud.utils.RestJsonClient
import org.broadinstitute.dsde.rawls.model.{ErrorReport, ErrorReportSource, WorkspaceName}
import org.broadinstitute.dsde.rawls.model.{ErrorReportSource, WorkspaceName}

import spray.json.DefaultJsonProtocol._

import scala.concurrent.{ExecutionContext, Future}
import scala.concurrent.duration._
Expand All @@ -26,6 +28,17 @@ class HttpImportServiceDAO(implicit val system: ActorSystem, implicit val materi
doImport(workspaceNamespace, workspaceName, isUpsert, importRequest)
}

override def listJobs(workspaceNamespace: String, workspaceName: String, runningOnly: Boolean)(implicit userInfo: UserInfo): Future[List[ImportServiceListResponse]] = {
// get jobs from import service
val importServiceUrl = FireCloudDirectiveUtils
.encodeUri(s"${FireCloudConfig.ImportService.server}/$workspaceNamespace/$workspaceName/imports")
.appendedAll(s"?running_only=$runningOnly")

userAuthedRequest(Get(importServiceUrl))(userInfo) flatMap { importServiceResponse =>
Unmarshal(importServiceResponse).to[List[ImportServiceListResponse]]
}
}

private def doImport(workspaceNamespace: String, workspaceName: String, isUpsert: Boolean, importRequest: AsyncImportRequest)(implicit userInfo: UserInfo): Future[PerRequestMessage] = {
// the payload to Import Service sends "path" and filetype.
val importServicePayload: ImportServiceRequest = ImportServiceRequest(path = importRequest.url, filetype = importRequest.filetype, isUpsert = isUpsert, options = importRequest.options)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.broadinstitute.dsde.firecloud.dataaccess

import org.broadinstitute.dsde.firecloud.model.{ImportServiceRequest, AsyncImportRequest, UserInfo}
import org.broadinstitute.dsde.firecloud.model.{AsyncImportRequest, ImportServiceListResponse, ImportServiceRequest, ImportServiceResponse, UserInfo}
import org.broadinstitute.dsde.firecloud.service.PerRequest.PerRequestMessage

import scala.concurrent.Future
Expand All @@ -19,4 +19,9 @@ trait ImportServiceDAO {
isUpsert: Boolean)
(implicit userInfo: UserInfo): Future[PerRequestMessage]

def listJobs(workspaceNamespace: String,
workspaceName: String,
runningOnly: Boolean
)(implicit userInfo: UserInfo): Future[List[ImportServiceListResponse]]

}
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ object ModelJsonProtocol extends WorkspaceJsonSupport with SprayJsonSupport {
implicit val impAsyncImportResponse: RootJsonFormat[AsyncImportResponse] = jsonFormat3(AsyncImportResponse)
implicit val impImportServiceRequest: RootJsonFormat[ImportServiceRequest] = jsonFormat4(ImportServiceRequest)
implicit val impImportServiceResponse: RootJsonFormat[ImportServiceResponse] = jsonFormat3(ImportServiceResponse)
implicit val impImportServiceListResponse: RootJsonFormat[ImportServiceListResponse] = jsonFormat4(ImportServiceListResponse)

implicit val impWorkspaceStorageCostEstimate: RootJsonFormat[WorkspaceStorageCostEstimate] = jsonFormat2(WorkspaceStorageCostEstimate)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ case class ImportServiceResponse(
status: String,
message: Option[String])

case class ImportServiceListResponse(
jobId: String,
status: String,
filetype: String,
message: Option[String])

case class MethodConfigurationId(
name: Option[String] = None,
namespace: Option[String] = None,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.broadinstitute.dsde.firecloud.webservice

import akka.http.scaladsl.model.StatusCodes.{Accepted, OK}

import java.text.SimpleDateFormat
import akka.http.scaladsl.model.Uri.Query
import akka.http.scaladsl.model.headers.OAuth2BearerToken
Expand All @@ -8,6 +10,7 @@ import akka.http.scaladsl.server.Route
import org.broadinstitute.dsde.firecloud.dataaccess.ImportServiceFiletypes.FILETYPE_PFB
import org.broadinstitute.dsde.firecloud.model.ModelJsonProtocol._
import org.broadinstitute.dsde.firecloud.model._
import org.broadinstitute.dsde.firecloud.service.PerRequest.RequestComplete
import org.broadinstitute.dsde.firecloud.service.{FireCloudDirectives, FireCloudRequestBuilding, PermissionReportService, WorkspaceService}
import org.broadinstitute.dsde.firecloud.utils.StandardUserInfoDirectives
import org.broadinstitute.dsde.firecloud.{EntityService, FireCloudConfig}
Expand Down Expand Up @@ -174,9 +177,13 @@ trait WorkspaceApiService extends FireCloudRequestBuilding with FireCloudDirecti
// GET importPFB is deprecated; use GET importJob instead
path(("importPFB" | "importJob")) {
get {
requireUserInfo() { _ =>
extract(_.request.uri.query()) { query =>
passthrough(Uri(encodeUri(s"${FireCloudConfig.ImportService.server}/$workspaceNamespace/$workspaceName/imports")).withQuery(query), HttpMethods.GET)
requireUserInfo() { userInfo =>
parameter(Symbol("running_only").as[Boolean].withDefault(false)) { runningOnly =>
complete {
entityServiceConstructor(FlexibleModelSchema).listJobs(workspaceNamespace, workspaceName, runningOnly, userInfo) map { respBody =>
RequestComplete(OK, respBody)
}
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package org.broadinstitute.dsde.firecloud.dataaccess

import java.util.UUID
import akka.http.scaladsl.model.StatusCodes._
import org.broadinstitute.dsde.firecloud.dataaccess.ImportServiceFiletypes.{FILETYPE_PFB, FILETYPE_RAWLS, FILETYPE_TDR}
import org.broadinstitute.dsde.firecloud.model.{AsyncImportRequest, AsyncImportResponse, UserInfo}
import org.broadinstitute.dsde.firecloud.model.{
AsyncImportRequest, AsyncImportResponse, ImportServiceListResponse,
UserInfo
}
import org.broadinstitute.dsde.firecloud.service.PerRequest
import org.broadinstitute.dsde.firecloud.service.PerRequest.RequestComplete
import org.broadinstitute.dsde.rawls.model.WorkspaceName
Expand All @@ -18,10 +22,13 @@ class MockImportServiceDAO extends ImportServiceDAO {
(implicit userInfo: UserInfo): Future[PerRequest.PerRequestMessage] = {
importRequest.filetype match {
case FILETYPE_PFB | FILETYPE_TDR =>
if(importRequest.url.contains("forbidden")) Future.successful(RequestComplete(Forbidden, "Missing Authorization: Bearer token in header"))
else if(importRequest.url.contains("bad.request")) Future.successful(RequestComplete(BadRequest, "Bad request as reported by import service"))
else if(importRequest.url.contains("its.lawsuit.time")) Future.successful(RequestComplete(UnavailableForLegalReasons, "import service message"))
else if(importRequest.url.contains("good")) Future.successful(RequestComplete(Accepted,
if (importRequest.url.contains("forbidden")) Future.successful(RequestComplete(Forbidden, "Missing " +
"Authorization: Bearer token in header"))
else if (importRequest.url.contains("bad.request")) Future.successful(RequestComplete(BadRequest, "Bad " +
"request as reported by import service"))
else if (importRequest.url.contains("its.lawsuit.time")) Future.successful(RequestComplete
(UnavailableForLegalReasons, "import service message"))
else if (importRequest.url.contains("good")) Future.successful(RequestComplete(Accepted,
AsyncImportResponse(url = importRequest.url,
jobId = UUID.randomUUID().toString,
workspace = WorkspaceName(workspaceNamespace, workspaceName))
Expand All @@ -31,4 +38,10 @@ class MockImportServiceDAO extends ImportServiceDAO {
case _ => ???
}
}

override def listJobs(workspaceNamespace: String, workspaceName: String, runningOnly: Boolean)(implicit
userInfo: UserInfo)
: Future[List[ImportServiceListResponse]] = {
Future.successful(List.empty[ImportServiceListResponse])
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package org.broadinstitute.dsde.firecloud.webservice

import akka.http.scaladsl.model.StatusCodes.OK
import akka.http.scaladsl.server.Route.seal
import org.broadinstitute.dsde.firecloud.dataaccess.ImportServiceDAO
import org.broadinstitute.dsde.firecloud.model.{ImportServiceListResponse, ModelSchema, UserInfo, WithAccessToken}
import org.broadinstitute.dsde.firecloud.service.{BaseServiceSpec, PermissionReportService, WorkspaceService}
import org.broadinstitute.dsde.firecloud.{EntityService, FireCloudConfig}
import org.broadinstitute.dsde.rawls.model._
import org.joda.time.DateTime
import org.mockito.ArgumentMatchers
import org.mockito.ArgumentMatchers.any
import org.mockito.Mockito.{clearInvocations, times, verify, when}
import org.scalatest.BeforeAndAfterEach
import org.scalatestplus.mockito.MockitoSugar

import java.util.UUID
import scala.concurrent.{ExecutionContext, Future}

class WorkspaceApiServiceJobSpec extends BaseServiceSpec with WorkspaceApiService with MockitoSugar with
BeforeAndAfterEach {

// mock for ImportServiceDAO
private val mockitoImportServiceDAO = mock[ImportServiceDAO]

// setup for the WorkspaceApiService routes
override val executionContext: ExecutionContext = ExecutionContext.Implicits.global
override val workspaceServiceConstructor: WithAccessToken => WorkspaceService = WorkspaceService.constructor(app
.copy(importServiceDAO = mockitoImportServiceDAO))
override val permissionReportServiceConstructor: UserInfo => PermissionReportService = PermissionReportService
.constructor(app)
override val entityServiceConstructor: ModelSchema => EntityService = EntityService.constructor(app.copy
(importServiceDAO = mockitoImportServiceDAO))

// dummy data for use in tests below
private val dummyUserId = "1234"
private val workspace = WorkspaceDetails("namespace", "name", "workspace_id", "buckety_bucket", Some
("wf-collection"), DateTime.now(), DateTime.now(), "my_workspace_creator", Some(Map()), //attributes
isLocked = false, //locked
Some(Set.empty), //authorizationDomain
WorkspaceVersions.V2, GoogleProjectId("googleProject"), Some(GoogleProjectNumber("googleProjectNumber")), Some
(RawlsBillingAccountName("billingAccount")), None, None, Option(DateTime.now()), None, None, WorkspaceState.Ready)
private val importList = List(
ImportServiceListResponse(UUID.randomUUID().toString, "running", "filetype1", None),
ImportServiceListResponse(UUID.randomUUID().toString, "error", "filetype2", Some("my error message")),
ImportServiceListResponse(UUID.randomUUID().toString, "success", "filetype3", None)
)

// a few shortcuts for accessing the routes
private final val workspacesRoot = FireCloudConfig.Rawls.authPrefix + FireCloudConfig.Rawls.workspacesPath
private final val pfbImportPath = workspacesRoot + "/%s/%s/importPFB".format(workspace.namespace, workspace.name)
private final val importJobPath = workspacesRoot + "/%s/%s/importJob".format(workspace.namespace, workspace.name)

"WorkspaceService list-jobs API" - {
// test both the importPFB and importJob routes
List(pfbImportPath, importJobPath) foreach { pathUnderTest =>
s"for path $pathUnderTest" - {
// test running_only=true and running_only=false
List(true, false) foreach { runningOnly =>
s"should call ImportServiceDAO.listJobs with running_only=$runningOnly" in {
// reset mock invocation counts and configure its return value
clearInvocations(mockitoImportServiceDAO)
when(mockitoImportServiceDAO.listJobs(any[String], any[String], any[Boolean])(any[UserInfo])).thenReturn(
Future.successful(importList))
// execute the route
(Get(s"$pathUnderTest?running_only=$runningOnly") ~> dummyUserIdHeaders(dummyUserId) ~> seal
(workspaceRoutes)) ~> check {
// route should return 200 OK
status should equal(OK)
// we should have invoked the ImportServiceDAO correctly
verify(mockitoImportServiceDAO, times(1)).listJobs(ArgumentMatchers.eq(workspace.namespace),
ArgumentMatchers.eq(workspace.name), ArgumentMatchers.eq(runningOnly))(any[UserInfo])
}
}
}
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -1195,90 +1195,6 @@ class WorkspaceApiServiceSpec extends BaseServiceSpec with WorkspaceApiService w
}
}

"WorkspaceService importPFB list-jobs tests" - {

List(pfbImportPath, importJobPath) foreach { pathUnderTest =>

s"for path $pathUnderTest" - {

"Successful passthrough should return OK with payload" in {
val responsePayload = JsArray(
JsObject(
("id", JsString(UUID.randomUUID().toString)),
("status", JsString("Running"))
),
JsObject(
("id", JsString(UUID.randomUUID().toString)),
("status", JsString("Error"))
),
JsObject(
("id", JsString(UUID.randomUUID().toString)),
("status", JsString("ImAUnitTest"))
)
)

importServiceServer
.when(request()
.withMethod("GET")
.withPath(s"/${workspace.namespace}/${workspace.name}/imports"))
.respond(org.mockserver.model.HttpResponse.response()
.withStatusCode(OK.intValue)
.withBody(responsePayload.compactPrint)
.withHeader("Content-Type", "application/json"))

(Get(pathUnderTest)
~> dummyUserIdHeaders(dummyUserId)
~> sealRoute(workspaceRoutes)) ~> check {
status should equal(OK)
responseAs[String].parseJson should be (responsePayload) // to address string-formatting issues
}
}

"Passthrough should pass on querystrings" in {
val responsePayload = JsArray(
JsObject(
("id", JsString(UUID.randomUUID().toString)),
("status", JsString("Running"))
)
)

val k1 = UUID.randomUUID().toString
val v1 = UUID.randomUUID().toString
val k2 = UUID.randomUUID().toString
val v2 = UUID.randomUUID().toString

val queryString = s"$k1=$v1&$k2=$v2"

importServiceServer
.when(request()
.withMethod("GET")
.withPath(s"/${workspace.namespace}/${workspace.name}/imports")
.withQueryStringParameters(new Parameter(k1, v1), new Parameter(k2, v2)))
.respond(org.mockserver.model.HttpResponse.response()
.withStatusCode(OK.intValue)
.withBody(responsePayload.compactPrint)
.withHeader("Content-Type", "application/json"))

(Get(s"$pathUnderTest?$queryString")
~> dummyUserIdHeaders(dummyUserId)
~> sealRoute(workspaceRoutes)) ~> check {
status should equal(OK)
responseAs[String].parseJson should be (responsePayload) // to address string-formatting issues
}
}

"Passthrough should not pass unrecognized HTTP verbs" in {
(Delete(s"$pathUnderTest")
~> dummyUserIdHeaders(dummyUserId)
~> sealRoute(workspaceRoutes)) ~> check {
status should equal(MethodNotAllowed)
}
}
}
}

}

"WorkspaceService POST importJob Tests" - {

List(FILETYPE_PFB, FILETYPE_TDR) foreach { filetype =>
Expand Down

0 comments on commit 664506a

Please sign in to comment.