Allow describing wdl with zipped imports.

kshakir · kshakir · commit a8c64c33ddc1 · 2024-05-10T22:53:25.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,14 @@
 Cromwell now allows opting into configured soft links on shared file systems such as HPC environments. More details can
 be found [here](https://cromwell.readthedocs.io/en/stable/backends/HPC/#optional-docker-soft-links).
 
+### `/describe` endpoint support for `workflowDependencies`
+
+Previously it was not possible to describe a `workflowSource` that required `workflowDependencies` as the `/describe`
+endpoint did not allow specifying the additional zip file.
+
+Now the endpoint will allow the user to upload the `workflowDependencies` zip file, will validate the top level
+workflow plus the dependencies, then return the appropriate response including the defined workflow inputs and outputs.
+
 ## 87 Release Notes
 
 ### `upgrade` command removed from Womtool
diff --git a/CromIAM/src/main/resources/swagger/cromiam.yaml b/CromIAM/src/main/resources/swagger/cromiam.yaml
@@ -838,6 +838,11 @@ paths:
           required: false
           type: file
           in: formData
+        - name: workflowDependencies
+          description: ZIP file containing workflow source files that are used to resolve local imports. This zip bundle will be unpacked in a sandbox accessible to this workflow.
+          required: false
+          type: file
+          in: formData
         - $ref: '#/parameters/workflowTypeParam'
         - $ref: '#/parameters/workflowTypeVersionParam'
       responses:
diff --git a/centaur/src/main/scala/centaur/test/Test.scala b/centaur/src/main/scala/centaur/test/Test.scala
@@ -244,8 +244,7 @@ object Operations extends StrictLogging {
       }
 
       override def run: IO[Unit] =
-        // We can't describe workflows based on zipped imports, so don't try:
-        if (workflow.skipDescribeEndpointValidation || workflow.data.zippedImports.nonEmpty) {
+        if (workflow.skipDescribeEndpointValidation) {
           IO.pure(())
         } else {
           checkDescriptionInner(0)
diff --git a/centaur/src/main/scala/centaur/test/workflow/Workflow.scala b/centaur/src/main/scala/centaur/test/workflow/Workflow.scala
@@ -45,7 +45,8 @@ final case class Workflow private (testName: String,
     workflowUrl = data.workflowUrl,
     workflowType = data.workflowType,
     workflowTypeVersion = data.workflowTypeVersion,
-    inputsJson = data.inputs.map(_.unsafeRunSync())
+    inputsJson = data.inputs.map(_.unsafeRunSync()),
+    zippedImports = data.zippedImports
   )
 
   def secondRun: Workflow =
diff --git a/cromwellApiClient/src/main/scala/cromwell/api/CromwellClient.scala b/cromwellApiClient/src/main/scala/cromwell/api/CromwellClient.scala
@@ -266,7 +266,13 @@ object CromwellClient {
       Multipart.FormData.BodyPart(name, HttpEntity(MediaTypes.`application/json`, ByteString(source)))
     }
 
-    val multipartFormData = Multipart.FormData(sourceBodyParts.toSeq: _*)
+    val zipBodyParts = Map(
+      "workflowDependencies" -> describeRequest.zippedImports
+    ) collect { case (name, Some(file)) =>
+      Multipart.FormData.BodyPart.fromPath(name, MediaTypes.`application/zip`, file.path)
+    }
+
+    val multipartFormData = Multipart.FormData((sourceBodyParts ++ zipBodyParts).toSeq: _*)
     multipartFormData.toEntity()
   }
 
diff --git a/cromwellApiClient/src/main/scala/cromwell/api/model/WorkflowDescribeRequest.scala b/cromwellApiClient/src/main/scala/cromwell/api/model/WorkflowDescribeRequest.scala
@@ -1,8 +1,11 @@
 package cromwell.api.model
 
+import better.files.File
+
 final case class WorkflowDescribeRequest(workflowSource: Option[String],
                                          workflowUrl: Option[String],
                                          workflowType: Option[String],
                                          workflowTypeVersion: Option[String],
-                                         inputsJson: Option[String]
+                                         inputsJson: Option[String],
+                                         zippedImports: Option[File]
 )
diff --git a/docs/api/RESTAPI.md b/docs/api/RESTAPI.md
diff --git a/engine/src/main/resources/swagger/cromwell.yaml b/engine/src/main/resources/swagger/cromwell.yaml
@@ -662,6 +662,11 @@ paths:
           required: false
           type: file
           in: formData
+        - name: workflowDependencies
+          description: ZIP file containing workflow source files that are used to resolve local imports. This zip bundle will be unpacked in a sandbox accessible to this workflow.
+          required: false
+          type: file
+          in: formData
         - $ref: '#/parameters/workflowTypeParam'
         - $ref: '#/parameters/workflowTypeVersionParam'
       responses:
diff --git a/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala b/engine/src/main/scala/cromwell/webservice/routes/WomtoolRouteSupport.scala
@@ -60,6 +60,7 @@ trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport
     val workflowInputs = data.get("workflowInputs").map(_.utf8String)
     val workflowType = data.get("workflowType").map(_.utf8String)
     val workflowVersion = data.get("workflowTypeVersion").map(_.utf8String)
+    val workflowDependencies = data.get("workflowDependencies").map(_.toArray)
 
     val wsfc = WorkflowSourceFilesCollection(
       workflowSource,
@@ -70,7 +71,7 @@ trait WomtoolRouteSupport extends WebServiceUtils with GithubAuthVendingSupport
       workflowInputs.getOrElse(""),
       workflowOptions = WorkflowOptions.empty,
       labelsJson = "",
-      importsFile = None,
+      importsFile = workflowDependencies,
       workflowOnHold = false,
       warnings = Seq.empty,
       requestedWorkflowId = None
diff --git a/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala b/engine/src/test/scala/cromwell/webservice/routes/CromwellApiServiceSpec.scala
@@ -761,7 +761,9 @@ object CromwellApiServiceSpec {
               s"[reading back DescribeRequest contents] workflow url: ${sourceFiles.workflowUrl}",
               s"[reading back DescribeRequest contents] inputs: ${sourceFiles.inputsJson}",
               s"[reading back DescribeRequest contents] type: ${sourceFiles.workflowType}",
-              s"[reading back DescribeRequest contents] version: ${sourceFiles.workflowTypeVersion}"
+              s"[reading back DescribeRequest contents] version: ${sourceFiles.workflowTypeVersion}",
+              s"[reading back DescribeRequest contents] dependencies: ${sourceFiles.importsZipFileOption
+                  .map(bytes => bytes.map(b => "0x%02X".format(b)).mkString("[", ", ", "]"))}"
             )
 
             sender() ! DescribeSuccess(description =
diff --git a/engine/src/test/scala/cromwell/webservice/routes/WomtoolRouteSupportSpec.scala b/engine/src/test/scala/cromwell/webservice/routes/WomtoolRouteSupportSpec.scala
@@ -46,6 +46,11 @@ class WomtoolRouteSupportSpec extends AsyncFlatSpec with ScalatestRouteTest with
     val workflowType = Multipart.FormData.BodyPart("workflowType", HttpEntity(ContentTypes.`text/plain(UTF-8)`, "WDL"))
     val workflowVersion =
       Multipart.FormData.BodyPart("workflowTypeVersion", HttpEntity(ContentTypes.`text/plain(UTF-8)`, "1.0"))
+    val workflowDependencies =
+      Multipart.FormData.BodyPart(
+        "workflowDependencies",
+        HttpEntity(MediaTypes.`application/zip`, Array[Byte](0x0a, 0x0b, 0x0c))
+      )
 
     val workflowSourceTriggerDescribeFailure =
       Multipart.FormData.BodyPart("workflowSource", HttpEntity(ContentTypes.`text/plain(UTF-8)`, "fail to describe"))
@@ -95,7 +100,8 @@ class WomtoolRouteSupportSpec extends AsyncFlatSpec with ScalatestRouteTest with
               "[reading back DescribeRequest contents] workflow url: None",
               "[reading back DescribeRequest contents] inputs: ",
               "[reading back DescribeRequest contents] type: None",
-              "[reading back DescribeRequest contents] version: None"
+              "[reading back DescribeRequest contents] version: None",
+              "[reading back DescribeRequest contents] dependencies: None"
             ),
             validWorkflow = true
           )
@@ -118,7 +124,8 @@ class WomtoolRouteSupportSpec extends AsyncFlatSpec with ScalatestRouteTest with
               "[reading back DescribeRequest contents] workflow url: Some(https://raw.githubusercontent.com/broadinstitute/cromwell/develop/womtool/src/test/resources/validate/wdl_draft3/valid/callable_imports/my_workflow.wdl)",
               "[reading back DescribeRequest contents] inputs: ",
               "[reading back DescribeRequest contents] type: None",
-              "[reading back DescribeRequest contents] version: None"
+              "[reading back DescribeRequest contents] version: None",
+              "[reading back DescribeRequest contents] dependencies: None"
             ),
             validWorkflow = true
           )
@@ -146,7 +153,43 @@ class WomtoolRouteSupportSpec extends AsyncFlatSpec with ScalatestRouteTest with
               "[reading back DescribeRequest contents] workflow url: None",
               "[reading back DescribeRequest contents] inputs: {\"a\":\"is for apple\"}",
               "[reading back DescribeRequest contents] type: Some(WDL)",
-              "[reading back DescribeRequest contents] version: Some(1.0)"
+              "[reading back DescribeRequest contents] version: Some(1.0)",
+              "[reading back DescribeRequest contents] dependencies: None"
+            ),
+            validWorkflow = true
+          )
+        }(responseAs[WorkflowDescription])
+      }
+  }
+
+  it should "include inputs, workflow type, workflow version, and workflow dependencies in the WorkflowSourceFilesCollection" in {
+    Post(
+      s"/womtool/$version/describe",
+      Multipart
+        .FormData(
+          BodyParts.workflowSource,
+          BodyParts.workflowInputs,
+          BodyParts.workflowType,
+          BodyParts.workflowVersion,
+          BodyParts.workflowDependencies
+        )
+        .toEntity()
+    ) ~>
+      akkaHttpService.womtoolRoutes ~>
+      check {
+        status should be(StatusCodes.OK)
+
+        assertResult {
+          WorkflowDescription(
+            valid = true,
+            errors = List(
+              "this is fake data from the mock SR actor",
+              "[reading back DescribeRequest contents] workflow hashcode: Some(580529622)",
+              "[reading back DescribeRequest contents] workflow url: None",
+              "[reading back DescribeRequest contents] inputs: {\"a\":\"is for apple\"}",
+              "[reading back DescribeRequest contents] type: Some(WDL)",
+              "[reading back DescribeRequest contents] version: Some(1.0)",
+              "[reading back DescribeRequest contents] dependencies: Some([0x0A, 0x0B, 0x0C])"
             ),
             validWorkflow = true
           )
diff --git a/services/src/main/scala/cromwell/services/womtool/Describer.scala b/services/src/main/scala/cromwell/services/womtool/Describer.scala
@@ -1,8 +1,10 @@
 package cromwell.services.womtool
 
 import cats.data.Validated.{Invalid, Valid}
-import cromwell.core.WorkflowSourceFilesCollection
-import cromwell.languages.util.ImportResolver.{HttpResolver, ImportAuthProvider, ImportResolver}
+import cats.syntax.traverse._
+import common.validation.ErrorOr.ErrorOr
+import cromwell.core.{WorkflowId, WorkflowSourceFilesCollection}
+import cromwell.languages.util.ImportResolver.{HttpResolver, ImportAuthProvider}
 import cromwell.languages.util.{ImportResolver, LanguageFactoryUtil}
 import cromwell.languages.{LanguageFactory, ValidatedWomNamespace}
 import cromwell.services.womtool.WomtoolServiceMessages.{DescribeFailure, DescribeResult, DescribeSuccess}
@@ -14,9 +16,23 @@ import wom.expression.NoIoFunctionSet
 object Describer {
 
   def describeWorkflow(wsfc: WorkflowSourceFilesCollection, authProviders: List[ImportAuthProvider]): DescribeResult = {
+    val zipResolverErrorOr: ErrorOr[Option[ImportResolver.ImportResolver]] =
+      wsfc.importsZipFileOption.map(ImportResolver.zippedImportResolver(_, WorkflowId.randomId())).sequence
 
-    val initialResolvers: List[ImportResolver] = List(HttpResolver(None, Map.empty, authProviders))
+    val initialResolversErrorOr: ErrorOr[List[ImportResolver.ImportResolver]] =
+      zipResolverErrorOr map { zipResolverOption =>
+        zipResolverOption.toList ++ List(HttpResolver(None, Map.empty, authProviders))
+      }
 
+    initialResolversErrorOr match {
+      case Valid(initialResolvers) => describeWorkflowWithResolvers(wsfc, initialResolvers)
+      case Invalid(errors) => DescribeFailure(errors.toList.mkString(", "))
+    }
+  }
+
+  private def describeWorkflowWithResolvers(wsfc: WorkflowSourceFilesCollection,
+                                            initialResolvers: List[ImportResolver.ImportResolver]
+  ): DescribeResult =
     // The HTTP resolver is used to pull down workflows submitted by URL
     LanguageFactoryUtil.findWorkflowSource(wsfc.workflowSource, wsfc.workflowUrl, initialResolvers) match {
       case Right((workflowSource: WorkflowSource, importResolvers: List[ImportResolver.ImportResolver])) =>
@@ -40,7 +56,6 @@ object Describer {
           reason = errors.toList.mkString(", ")
         )
     }
-  }
 
   // By this point there are no "out of band" errors that can occur (i.e. those that would indicate a BadRequest, versus just showing up in the `errors` list)
   private def describeWorkflowInner(factory: LanguageFactory,
diff --git a/services/src/test/resources/describe/wdl_1_0/relative_imports/description.json b/services/src/test/resources/describe/wdl_1_0/relative_imports/description.json
@@ -0,0 +1,33 @@
+{
+  "valid" : true,
+  "errors" : [
+  ],
+  "validWorkflow" : true,
+  "name" : "relative_imports",
+  "inputs" : [
+  ],
+  "outputs" : [
+    {
+      "name" : "result",
+      "valueType" : {
+        "typeName" : "Int"
+      },
+      "typeDisplayName" : "Int"
+    }
+  ],
+  "images" : [
+  ],
+  "submittedDescriptorType" : {
+    "descriptorType" : "WDL",
+    "descriptorTypeVersion" : "Cascades"
+  },
+  "importedDescriptorTypes" : [
+  ],
+  "meta" : {
+
+  },
+  "parameterMeta" : {
+
+  },
+  "isRunnableWorkflow" : true
+}
diff --git a/services/src/test/resources/describe/wdl_1_0/relative_imports/workflow.wdl b/services/src/test/resources/describe/wdl_1_0/relative_imports/workflow.wdl
@@ -0,0 +1,11 @@
+version development
+
+import "sub_wfs/foo.wdl"
+
+workflow relative_imports {
+  call foo.foo_wf
+
+  output {
+    Int result = foo_wf.unpacked
+  }
+}
diff --git a/services/src/test/resources/describe/wdl_1_0/relative_imports/workflowDependencies/structs/my_struct.wdl b/services/src/test/resources/describe/wdl_1_0/relative_imports/workflowDependencies/structs/my_struct.wdl
@@ -0,0 +1,5 @@
+version development
+
+struct MyStruct {
+  Int a
+}
diff --git a/services/src/test/resources/describe/wdl_1_0/relative_imports/workflowDependencies/sub_wfs/foo.wdl b/services/src/test/resources/describe/wdl_1_0/relative_imports/workflowDependencies/sub_wfs/foo.wdl
@@ -0,0 +1,11 @@
+version development
+
+import "../structs/my_struct.wdl"
+import "tasks/add5.wdl" as a5
+
+workflow foo_wf {
+  call a5.add5 { input: x = object { a: 100 } }
+  output {
+    Int unpacked = add5.five_added.a
+  }
+}
diff --git a/services/src/test/resources/describe/wdl_1_0/relative_imports/workflowDependencies/sub_wfs/tasks/add5.wdl b/services/src/test/resources/describe/wdl_1_0/relative_imports/workflowDependencies/sub_wfs/tasks/add5.wdl
@@ -0,0 +1,18 @@
+version development
+
+import "../../structs/my_struct.wdl"
+
+task add5 {
+  input {
+    MyStruct x
+  }
+  command <<<
+    echo $((5 + ~{x.a}))
+  >>>
+  output {
+    MyStruct five_added = object { a: read_int(stdout()) }
+  }
+  runtime {
+    docker: "ubuntu:latest"
+  }
+}
diff --git a/services/src/test/scala/cromwell/services/womtool/DescriberSpec.scala b/services/src/test/scala/cromwell/services/womtool/DescriberSpec.scala
@@ -2,7 +2,7 @@ package cromwell.services.womtool
 
 import common.assertion.CromwellTimeoutSpec
 import cromwell.core.path._
-import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection, WorkflowSourceFilesWithoutImports}
+import cromwell.core.{WorkflowOptions, WorkflowSourceFilesCollection}
 import cromwell.languages.config.{CromwellLanguages, LanguageConfiguration}
 import cromwell.services.womtool.DescriberSpec._
 import cromwell.services.womtool.WomtoolServiceMessages.DescribeSuccess
@@ -37,26 +37,37 @@ class DescriberSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers {
         val workflowType = Try(caseDirectory.resolve("workflowType").contentAsString.stripLineEnd).toOption
         val workflowTypeVersion =
           Try(caseDirectory.resolve("workflowTypeVersion").contentAsString.stripLineEnd).toOption
+        val importsFile =
+          Try(caseDirectory.resolve("workflowDependencies")).filter(_.exists).map(_.zip()).toOption
 
-        val interimWsfc = WorkflowSourceFilesWithoutImports(
-          workflowSource = None,
-          workflowUrl = None,
+        val workflowSource = testCase match {
+          case FileAndDescription(file, _) => Option(file)
+          case _ => None
+        }
+
+        val workflowUrl = testCase match {
+          case UrlAndDescription(url, _) => Option(url)
+          case _ => None
+        }
+
+        val wsfc = WorkflowSourceFilesCollection(
+          workflowSource = workflowSource,
+          workflowUrl = workflowUrl,
           workflowRoot = None,
           workflowType = workflowType,
           workflowTypeVersion = workflowTypeVersion,
           inputsJson = "",
           workflowOptions = WorkflowOptions.empty,
+          importsFile = importsFile.map(_.byteArray),
+          workflowOnHold = false,
           labelsJson = "",
           warnings = Seq.empty,
           requestedWorkflowId = None
         )
 
-        val wsfc = testCase match {
-          case FileAndDescription(file, _) => interimWsfc.copy(workflowSource = Option(file))
-          case UrlAndDescription(url, _) => interimWsfc.copy(workflowUrl = Option(url))
-        }
-
         check(wsfc, parse(testCase.expectedDescription).toOption.get)
+
+        importsFile.map(_.delete(swallowIOExceptions = true))
       }
     }
   }

Original file line number	Diff line number	Diff line change
`@@ -244,8 +244,7 @@ object Operations extends StrictLogging {`
`244`	`244`	`}`
`245`	`245`
`246`	`246`	`override def run: IO[Unit] =`
`247`		`- // We can't describe workflows based on zipped imports, so don't try:`
`248`		`- if (workflow.skipDescribeEndpointValidation \|\| workflow.data.zippedImports.nonEmpty) {`
	`247`	`+ if (workflow.skipDescribeEndpointValidation) {`
`249`	`248`	`IO.pure(())`
`250`	`249`	`} else {`
`251`	`250`	`checkDescriptionInner(0)`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,8 @@ final case class Workflow private (testName: String,`
`45`	`45`	`workflowUrl = data.workflowUrl,`
`46`	`46`	`workflowType = data.workflowType,`
`47`	`47`	`workflowTypeVersion = data.workflowTypeVersion,`
`48`		`- inputsJson = data.inputs.map(_.unsafeRunSync())`
	`48`	`+ inputsJson = data.inputs.map(_.unsafeRunSync()),`
	`49`	`+ zippedImports = data.zippedImports`
`49`	`50`	`)`
`50`	`51`
`51`	`52`	`def secondRun: Workflow =`
Original file line number	Diff line number	Diff line change
`@@ -266,7 +266,13 @@ object CromwellClient {`
`266`	`266`	Multipart.FormData.BodyPart(name, HttpEntity(MediaTypes.`application/json`, ByteString(source)))
`267`	`267`	`}`
`268`	`268`
`269`		`- val multipartFormData = Multipart.FormData(sourceBodyParts.toSeq: _*)`
	`269`	`+ val zipBodyParts = Map(`
	`270`	`+ "workflowDependencies" -> describeRequest.zippedImports`
	`271`	`+ ) collect { case (name, Some(file)) =>`
	`272`	+ Multipart.FormData.BodyPart.fromPath(name, MediaTypes.`application/zip`, file.path)
	`273`	`+ }`
	`274`	`+`
	`275`	`+ val multipartFormData = Multipart.FormData((sourceBodyParts ++ zipBodyParts).toSeq: _*)`
`270`	`276`	`multipartFormData.toEntity()`
`271`	`277`	`}`
`272`	`278`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +version development
++
 +struct MyStruct {
 +  Int a
 +}