Allow describing wdl with zipped imports.

kshakir · kshakir · commit 1d1c815461c8 · 2023-05-09T10:25:11.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -65,6 +65,14 @@ For more information, see the [Cromwell 79 release notes](https://github.com/bro
 
 * Cromwell now attempts to translate `disks` attributes [written for GCP](https://cromwell.readthedocs.io/en/stable/RuntimeAttributes/#disks) into valid `disk` attributes for TES. For information on supported conversions, refer to the [TES documentation](https://cromwell.readthedocs.io/en/stable/backends/TES/).
 
+### `/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.
+
 ### Bug Fixes
 
 * Reference disks are only mounted if configured in the workflow options.
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
@@ -228,8 +228,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
@@ -43,7 +43,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
@@ -261,7 +261,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,7 +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
@@ -642,6 +642,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
@@ -45,6 +45,7 @@ trait WomtoolRouteSupport extends WebServiceUtils {
     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,
@@ -55,7 +56,7 @@ trait WomtoolRouteSupport extends WebServiceUtils {
       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
@@ -670,7 +670,12 @@ 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 = WorkflowDescription(valid = true, errors = readBack, validWorkflow = true))
diff --git a/engine/src/test/scala/cromwell/webservice/routes/WomtoolRouteSupportSpec.scala b/engine/src/test/scala/cromwell/webservice/routes/WomtoolRouteSupportSpec.scala
@@ -35,6 +35,11 @@ class WomtoolRouteSupportSpec extends AsyncFlatSpec with ScalatestRouteTest with
     val workflowInputs = Multipart.FormData.BodyPart("workflowInputs", HttpEntity(MediaTypes.`application/json`, "{\"a\":\"is for apple\"}"))
     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"))
@@ -82,7 +87,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
           )
@@ -104,7 +110,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
           )
@@ -126,7 +133,40 @@ 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,7 +1,9 @@
 package cromwell.services.womtool
 
 import cats.data.Validated.{Invalid, Valid}
-import cromwell.core.WorkflowSourceFilesCollection
+import cats.syntax.traverse._
+import common.validation.ErrorOr.ErrorOr
+import cromwell.core.{WorkflowId, WorkflowSourceFilesCollection}
 import cromwell.languages.util.ImportResolver.HttpResolver
 import cromwell.languages.util.{ImportResolver, LanguageFactoryUtil}
 import cromwell.languages.{LanguageFactory, ValidatedWomNamespace}
@@ -14,9 +16,22 @@ import wom.expression.NoIoFunctionSet
 object Describer {
 
   def describeWorkflow(wsfc: WorkflowSourceFilesCollection): DescribeResult = {
+    val zipResolverErrorOr: ErrorOr[Option[ImportResolver.ImportResolver]] =
+      wsfc.importsZipFileOption.map(ImportResolver.zippedImportResolver(_, WorkflowId.randomId())).sequence
 
-    val initialResolvers = List(HttpResolver(None, Map.empty))
+    val initialResolversErrorOr: ErrorOr[List[ImportResolver.ImportResolver]] =
+      zipResolverErrorOr map { zipResolverOption =>
+        zipResolverOption.toList ++ List(HttpResolver(None, Map.empty))
+      }
+
+    initialResolversErrorOr match {
+      case Valid(initialResolvers) => describeWorkflow(wsfc, initialResolvers)
+      case Invalid(errors) => DescribeFailure(errors.toList.mkString(", "))
+    }
+  }
 
+  def describeWorkflow(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])) =>
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" : "Biscayne"
+  },
+  "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
`@@ -43,7 +43,8 @@ final case class Workflow private(testName: String,`
`43`	`43`	`workflowUrl = data.workflowUrl,`
`44`	`44`	`workflowType = data.workflowType,`
`45`	`45`	`workflowTypeVersion = data.workflowTypeVersion,`
`46`		`- inputsJson = data.inputs.map(_.unsafeRunSync())`
	`46`	`+ inputsJson = data.inputs.map(_.unsafeRunSync()),`
	`47`	`+ zippedImports = data.zippedImports,`
`47`	`48`	`)`
`48`	`49`
`49`	`50`	`def secondRun: Workflow = {`
Original file line number	Diff line number	Diff line change
`@@ -261,7 +261,13 @@ object CromwellClient {`
`261`	`261`	Multipart.FormData.BodyPart(name, HttpEntity(MediaTypes.`application/json`, ByteString(source)))
`262`	`262`	`}`
`263`	`263`
`264`		`- val multipartFormData = Multipart.FormData(sourceBodyParts.toSeq : _*)`
	`264`	`+ val zipBodyParts = Map(`
	`265`	`+ "workflowDependencies" -> describeRequest.zippedImports`
	`266`	`+ ) collect {`
	`267`	+ case (name, Some(file)) => Multipart.FormData.BodyPart.fromPath(name, MediaTypes.`application/zip`, file.path)
	`268`	`+ }`
	`269`	`+`
	`270`	`+ val multipartFormData = Multipart.FormData((sourceBodyParts ++ zipBodyParts).toSeq : _*)`
`265`	`271`	`multipartFormData.toEntity()`
`266`	`272`	`}`
`267`	`273`