Skip to content

Commit

Permalink
Merge b56f557 into 0e0c067
Browse files Browse the repository at this point in the history
  • Loading branch information
@aednichols
aednichols committed Oct 20, 2017
2 parents 0e0c067 + b56f557 commit 32d202b
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 56 deletions.
10 changes: 5 additions & 5 deletions build.sbt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ organization := "org.broadinstitute"

scalaVersion := "2.11.11"

val sprayV = "1.3.2"
val sprayV = "1.3.4"

val wdl4sV = "0.13"
val wdl4sV = "0.15-814d203"

val artifactory = "https://broadinstitute.jfrog.io/broadinstitute/"

Expand All @@ -24,7 +24,7 @@ libraryDependencies ++= Seq(
"org.apache.commons" % "commons-compress" % "1.4.1", // upgrading a transitive dependency to avoid security warnings
"org.apache.httpcomponents" % "httpclient" % "4.5.3", // upgrading a transitive dependency to avoid security warnings
"ch.qos.logback" % "logback-classic" % "1.2.2",
"com.github.swagger-spray" %% "swagger-spray" % "0.8.0",
"com.github.swagger-spray" %% "swagger-spray" % "0.8.0" excludeAll ExclusionRule(organization = "io.spray"),
"com.github.simplyscala" %% "scalatest-embedmongo" % "0.2.2",
"com.google.api-client" % "google-api-client" % "1.20.0" excludeAll ExclusionRule(organization = "com.google.guava"),
"com.google.apis" % "google-api-services-admin-directory" % "directory_v1-rev53-1.20.0" excludeAll ExclusionRule(organization = "com.google.guava"),
Expand All @@ -37,8 +37,8 @@ libraryDependencies ++= Seq(
"com.zaxxer" % "HikariCP" % "2.3.9",
"io.spray" %% "spray-can" % sprayV,
"io.spray" %% "spray-client" % sprayV,
"io.spray" %% "spray-json" % "1.3.1", // NB: Not at sprayV. 1.3.2 does not exist.
"io.spray" %% "spray-routing" % sprayV,
"io.spray" %% "spray-json" % "1.3.3", // NB: Not at sprayV. 1.3.4 does not exist.
"io.spray" %% "spray-routing-shapeless23" % sprayV,
"mysql" % "mysql-connector-java" % "5.1.42",
"org.broadinstitute" %% "wdl4s" % wdl4sV,
"org.broadinstitute.dsde.vault" %% "vault-common" % "0.1-15-bf74315",
Expand Down
37 changes: 12 additions & 25 deletions src/main/scala/org/broadinstitute/dsde/agora/server/business/AgoraBusiness.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ import org.bson.types.ObjectId
import slick.dbio.DBIO
import spray.http.StatusCodes
import spray.json._
import wdl4s.{AstTools, WdlNamespace, WdlNamespaceWithWorkflow}
import wdl4s.parser.WdlParser.{AstList, SyntaxError}
import wdl4s.parser.WdlParser.{SyntaxError}
import wdl4s.wdl.{WdlNamespace, WdlNamespaceWithWorkflow}
import wdl4s.wdl.WdlNamespace.httpResolver

import scala.concurrent.{ExecutionContext, Future}
import scala.util.{Failure, Success, Try}
Expand Down Expand Up @@ -100,31 +101,17 @@ class AgoraBusiness(permissionsDataSource: PermissionsDataSource)(implicit ec: E
}
}

//Workaround for https://github.com/broadinstitute/wdl4s/issues/103
//Checks to see if the WDL has imports, since loading a WDL that contains imports without specifying a resolver
//barfs in an unintuitive way.
private def checkWdlHasNoImports(wdl: String): Try[Unit] = {
Try { //AST loader might syntax error if the wdl is crap
val ast = AstTools.getAst(wdl, "string")
ast.getAttribute("imports").asInstanceOf[AstList].isEmpty
} match {
case Failure(x) => Failure(ValidationException(s"WDL parsing failure. ${x.getMessage}", x)) //ast didn't parse nicely
case Success(true) => Success() //no imports, WDL is fine
case Success(false) => Failure(ValidationException("WDL imports not yet supported."))
}
}

private def checkValidPayload[T](agoraEntity: AgoraEntity, username: String)(op: => ReadWriteAction[T]): ReadWriteAction[T] = {
val payload = agoraEntity.payload.get

val payloadOK = agoraEntity.entityType match {
case Some(AgoraEntityType.Task) =>
checkWdlHasNoImports(payload) flatMap ( _ => WdlNamespace.loadUsingSource(payload, None, Option(Seq())) )
WdlNamespace.loadUsingSource(payload, None, Option(Seq(httpResolver(_)))) // TODO: we may not need this change if we don't need Tasks
// NOTE: Still not validating existence of docker images.
// namespace.tasks.foreach { validateDockerImage }

case Some(AgoraEntityType.Workflow) =>
checkWdlHasNoImports(payload) flatMap ( _ => WdlNamespaceWithWorkflow.load(payload, Seq()) )
WdlNamespaceWithWorkflow.load(payload, Seq(httpResolver(_)))
// NOTE: Still not validating existence of docker images.
//namespace.tasks.foreach { validateDockerImage }

Expand Down Expand Up @@ -401,8 +388,8 @@ class AgoraBusiness(permissionsDataSource: PermissionsDataSource)(implicit ec: E
}

// copied from org.broadinstitute.dsde.rawls.jobexec.MethodConfigResolver
def parseWDL(wdl: String): Try[wdl4s.Workflow] = {
val parsed: Try[WdlNamespaceWithWorkflow] = WdlNamespaceWithWorkflow.load(wdl, Seq()).recoverWith { case t: SyntaxError =>
def parseWDL(wdl: String): Try[wdl4s.wdl.WdlWorkflow] = {
val parsed: Try[WdlNamespaceWithWorkflow] = WdlNamespaceWithWorkflow.load(wdl, Seq(httpResolver(_))).recoverWith { case t: SyntaxError =>
Failure(AgoraException("Failed to parse WDL: " + t.getMessage))
}
parsed map( _.workflow )
Expand Down Expand Up @@ -473,11 +460,11 @@ class AgoraBusiness(permissionsDataSource: PermissionsDataSource)(implicit ec: E
AgoraDao.createAgoraDao(AgoraEntityType.MethodTypes).findSingle(queryMethod)
}

/**
* Parses out user/image:tag from a docker string.
*
* @param imageId docker imageId string. Looks like ubuntu:latest ggrant/joust:latest
*/
// /**
// * Parses out user/image:tag from a docker string.
// *
// * @param imageId docker imageId string. Looks like ubuntu:latest ggrant/joust:latest
// */
// private def parseDockerString(imageId: String) : Option[DockerImageReference] = {
// if (imageId.startsWith("gcr.io")) {
// None
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/org/broadinstitute/dsde/agora/server/ga4gh/ModelSupport.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package org.broadinstitute.dsde.agora.server.ga4gh
import org.broadinstitute.dsde.agora.server.AgoraConfig
import org.broadinstitute.dsde.agora.server.ga4gh.Models.{Metadata, Tool, ToolClass, ToolDescriptor, ToolDescriptorType, ToolId, ToolVersion}
import org.broadinstitute.dsde.agora.server.model.{AgoraEntity, AgoraEntityType, MethodDefinition}
import wdl4s.WdlNamespaceWithWorkflow
import wdl4s.wdl.WdlNamespaceWithWorkflow

import scala.util.Success

Expand Down
29 changes: 14 additions & 15 deletions src/test/scala/org/broadinstitute/dsde/agora/server/AgoraTestData.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,20 @@ object AgoraTestData {
|}
|
| """.stripMargin)

val testAgoraEntity = new AgoraEntity(
namespace = namespace3,
name = name1,
synopsis = synopsis1,
documentation = documentation1,
owner = owner1,
payload = payload1,
entityType = Option(AgoraEntityType.Workflow)
)

// TODO: import a method from the local server, not dev!
val payloadReferencingExternalMethod = Option( """
|import "methods://broad.wc.1"
|import "https://firecloud-orchestration.dsde-dev.broadinstitute.org/ga4gh/v1/tools/anichols:cnv_common_tasks/versions/4/plain-WDL/descriptor" as CNVTasks
|
|task grep {
| String pattern
Expand All @@ -137,9 +149,6 @@ object AgoraTestData {
| input: file_name = f
| }
| }
| call wc {
| input: files = grep.out
| }
|}
| """.stripMargin)
val badPayload = Option("task test {")
Expand All @@ -161,7 +170,7 @@ object AgoraTestData {
|
| """.stripMargin)
val badPayloadNonExistentImport = Option( """
|import "methods://broad.non_existent_grep.1"
|import "broad.non_existent_grep.1"
|import "broad.wc.1"
|
|workflow scatter_gather_grep_wc {
Expand Down Expand Up @@ -520,16 +529,6 @@ object AgoraTestData {
entityType = Option(AgoraEntityType.Task)
)

val testAgoraEntity = new AgoraEntity(
namespace = namespace3,
name = name1,
synopsis = synopsis1,
documentation = documentation1,
owner = owner1,
payload = payload1,
entityType = Option(AgoraEntityType.Workflow)
)

val testMethodWithSnapshot1 = new AgoraEntity(
namespace = namespace3,
name = name1,
Expand Down
28 changes: 18 additions & 10 deletions src/test/scala/org/broadinstitute/dsde/agora/server/webservice/AgoraImportSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,25 @@ class AgoraImportSpec extends ApiServiceSpec with FlatSpecLike{
"MethodsService" should "return a 400 when posting a WDL with an invalid import statement" in {
Post(ApiUtil.Methods.withLeadingVersion, testBadAgoraEntityInvalidWdlImportFormat) ~>
methodsService.postRoute ~> check {
assert(status == BadRequest)
assert(status == InternalServerError)
assert(body.asString contains "Failed to import workflow invalid_syntax_for_tool.:")
}
}

"MethodsService" should "return a 400 when posting a WDL with an import statement that references a non-existent method" in {
Post(ApiUtil.Methods.withLeadingVersion, testBadAgoraEntityNonExistentWdlImportFormat) ~>
methodsService.postRoute ~> check {
assert(status == BadRequest)
assert(status == InternalServerError)
assert(body.asString contains "Failed to import workflow broad.non_existent_grep.1.:")
}
}

"MethodsService" should "return 400 when posting a WDL that contains an import, because imports are no longer allowed" in {
"MethodsService" should "return 201 when posting a valid WDL that contains an import" in {
Post(ApiUtil.Methods.withLeadingVersion, testEntityWorkflowWithExistentWdlImport) ~>
methodsService.postRoute ~> check {
assert(status == BadRequest)
assert(status == Created)
assert(body.asString contains "\"name\": \"testMethod1\"")
assert(body.asString contains "\"createDate\"")
}
}

Expand All @@ -67,24 +71,28 @@ class AgoraImportSpec extends ApiServiceSpec with FlatSpecLike{
}
}

"MethodsService" should "return a 400 when copying a method and specifying a WDL with an invalid import statement" in {
"MethodsService" should "return a 500 when copying a method and specifying a WDL with an invalid import statement" in {
Post(copyUrl, copyPayload(testBadAgoraEntityInvalidWdlImportFormat.payload)) ~>
methodsService.querySingleRoute ~> check {
assert(status == BadRequest)
assert(status == InternalServerError)
assert(body.asString contains "Failed to import workflow invalid_syntax_for_tool.:")
}
}

"MethodsService" should "return a 400 when copying a method and specifying a WDL with an import statement that references a non-existent method" in {
"MethodsService" should "return a 500 when copying a method and specifying a WDL with an import statement that references a non-existent method" in {
Post(copyUrl, copyPayload(testBadAgoraEntityNonExistentWdlImportFormat.payload)) ~>
methodsService.querySingleRoute ~> check {
assert(status == BadRequest)
assert(status == InternalServerError)
assert(body.asString contains "Failed to import workflow broad.non_existent_grep.1.:")
}
}

"MethodsService" should "return 400 when copying a method and specifying a WDL that contains an import, because imports are no longer allowed" in {
"MethodsService" should "return 200 when copying a method and specifying a valid WDL that contains an import" in {
Post(copyUrl, copyPayload(testEntityWorkflowWithExistentWdlImport.payload)) ~>
methodsService.querySingleRoute ~> check {
assert(status == BadRequest)
assert(status == OK)
assert(body.asString contains "\"name\": \"testMethod1\"")
assert(body.asString contains "\"createDate\"")
}
}

Expand Down

0 comments on commit 32d202b

Please sign in to comment.