Fix CWL requirement nesting.

centaur/src/main/resources/standardTestCases/three_step.test

@@ -16,5 +16,5 @@ metadata {
   "calls.cgrep.executionStatus": Done
   "calls.ps.runtimeAttributes.docker": "ubuntu:bionic"
   "calls.wc.runtimeAttributes.docker": "ubuntu:latest"
-  "calls.cgrep.runtimeAttributes.docker": "ubuntu:latest"
+  "calls.cgrep.runtimeAttributes.docker": "debian:jessie"
 }

centaur/src/main/resources/standardTestCases/three_step/three_step.cwl

@@ -1,6 +1,7 @@
 cwlVersion: v1.0
 class: Workflow
 id: three_step
+# Workflow-level DockerRequirement
 hints:
   DockerRequirement:
     dockerPull: "ubuntu:latest"
@@ -28,6 +29,7 @@ steps:
       type: File
     class: CommandLineTool
     requirements:
+      # Command line tool-level DockerRequirement
       # Check it: this DockerRequirement does not use the `class` formatting but should still parse
       # thanks to the magic of SALAD.
       DockerRequirement:
@@ -42,6 +44,10 @@ steps:
     source: "#ps/ps-stdOut"
   out:
   - id: cgrep-count
+  # Workflow step-level DockerRequirement
+  requirements:
+    DockerRequirement:
+      dockerPull: "debian:jessie"
   run:
     inputs:
     - id: pattern

cwl/src/main/scala/cwl/CommandLineTool.scala

@@ -49,25 +49,30 @@ case class CommandLineTool private(
   private [cwl] implicit val explicitWorkflowName = ParentName(id)
   private val inputNames = this.inputs.map(i => FullyQualifiedName(i.id).id).toSet
 
+  // Circe can't create bidirectional links between workflow steps and runs (including `CommandLineTool`s) so this
+  // ugly var is here to link back to a possible parent workflow step. This is needed to navigate upward for finding
+  // requirements in the containment hierarchy. There isn't always a containing workflow step so this is an `Option`.
+  private[cwl] var parentWorkflowStep: Option[WorkflowStep] = None
+
   /** Builds an `Executable` directly from a `CommandLineTool` CWL with no parent workflow. */
   def womExecutable(validator: RequirementsValidator, inputFile: Option[String] = None): Checked[Executable] = {
-    val taskDefinition = buildTaskDefinition(parentWorkflow = None, validator)
+    val taskDefinition = buildTaskDefinition(validator)
     CwlExecutableValidation.buildWomExecutable(taskDefinition, inputFile)
   }
 
-  private def validateRequirementsAndHints(parentWorkflow: Option[Workflow], validator: RequirementsValidator): ErrorOr[List[Requirement]] = {
+  private def validateRequirementsAndHints(validator: RequirementsValidator): ErrorOr[List[Requirement]] = {
     import cats.instances.list._
     import cats.syntax.traverse._
 
-    val allRequirements = requirements.toList.flatten ++ parentWorkflow.toList.flatMap(_.allRequirements)
+    val allRequirements = requirements.toList.flatten ++ parentWorkflowStep.toList.flatMap(_.allRequirements)
     // All requirements must validate or this fails.
     val errorOrValidatedRequirements: ErrorOr[List[Requirement]] = allRequirements traverse validator
 
     errorOrValidatedRequirements map { validRequirements =>
       // Only Requirement hints, everything else is thrown out.
       // TODO CWL don't throw them out but pass them back to the caller to do with as the caller pleases.
       val hintRequirements = hints.toList.flatten.flatMap { _.select[Requirement] }
-      val parentHintRequirements = parentWorkflow.toList.flatMap(_.allHints)
+      val parentHintRequirements = parentWorkflowStep.toList.flatMap(_.allHints)
 
       // Throw out invalid Requirement hints.
       // TODO CWL pass invalid hints back to the caller to do with as the caller pleases.
@@ -80,8 +85,8 @@ case class CommandLineTool private(
     requirement.fold(RequirementToAttributeMap).apply(inputNames)
   }
 
-  def buildTaskDefinition(parentWorkflow: Option[Workflow], validator: RequirementsValidator): ErrorOr[CallableTaskDefinition] = {
-    validateRequirementsAndHints(parentWorkflow, validator) map { requirementsAndHints =>
+  def buildTaskDefinition(validator: RequirementsValidator): ErrorOr[CallableTaskDefinition] = {
+    validateRequirementsAndHints(validator) map { requirementsAndHints =>
       val id = this.id
 
       val commandTemplate: Seq[CommandPart] = baseCommand.toSeq.flatMap(_.fold(BaseCommandToCommandParts)) ++

cwl/src/main/scala/cwl/Workflow.scala

@@ -30,21 +30,27 @@ case class Workflow private(
                      steps: Array[WorkflowStep],
                      requirements: Option[Array[Requirement]],
                      hints: Option[Array[Hint]]) {
+
+  steps.foreach { _.parentWorkflow = this }
+
   /** Builds an `Executable` from a `Workflow` CWL with no parent `Workflow` */
   def womExecutable(validator: RequirementsValidator, inputFile: Option[String] = None): Checked[Executable] = {
     CwlExecutableValidation.buildWomExecutable(womDefinition(validator), inputFile)
   }
 
-  private[cwl] var parentWorkflow: Option[Workflow] = None
+  // Circe can't create bidirectional links between workflow steps and runs (including `Workflow`s) so this
+  // ugly var is here to link back to a possible parent workflow step. This is needed to navigate upward for finding
+  // requirements in the containment hierarchy. There isn't always a containing workflow step so this is an `Option`.
+  private[cwl] var parentWorkflowStep: Option[WorkflowStep] = None
 
-  val allRequirements: List[Requirement] = requirements.toList.flatten ++ parentWorkflow.toList.flatMap { _.allRequirements }
+  val allRequirements: List[Requirement] = requirements.toList.flatten ++ parentWorkflowStep.toList.flatMap { _.allRequirements }
 
   val allHints: List[Requirement] = {
     // Just ignore any hint that isn't a Requirement.
     val requirementHints = hints.toList.flatten.flatMap { _.select[Requirement] }
-    requirementHints ++ parentWorkflow.toList.flatMap { _.allHints }
+    requirementHints ++ parentWorkflowStep.toList.flatMap { _.allHints }
   }
-  
+
   private [cwl] implicit val explicitWorkflowName = ParentName(id)
 
   val fileNames: List[String] = steps.toList.flatMap(_.run.select[String].toList)
@@ -140,12 +146,11 @@ case class Workflow private(
     } yield ret
   }
 
-  def womDefinition(validator: RequirementsValidator, parentWorkflow: Option[Workflow] = None): Checked[WorkflowDefinition] = {
+  def womDefinition(validator: RequirementsValidator): Checked[WorkflowDefinition] = {
     val name: String = Paths.get(id).getFileName.toString
     val meta: Map[String, String] = Map.empty
     val paramMeta: Map[String, String] = Map.empty
     val declarations: List[(String, WomExpression)] = List.empty
-    this.parentWorkflow = parentWorkflow
 
     womGraph(name, validator).map(graph =>
       WorkflowDefinition(

cwl/src/main/scala/cwl/WorkflowStep.scala

@@ -42,9 +42,25 @@ case class WorkflowStep(
                          scatter: ScatterVariables = None,
                          scatterMethod: Option[ScatterMethod] = None) {
 
+  run.select[Workflow].foreach(_.parentWorkflowStep = Option(this))
+  run.select[CommandLineTool].foreach(_.parentWorkflowStep = Option(this))
+
   // We're scattering if scatter is defined, and if it's a list of variables the list needs to be non empty
   private val isScattered: Boolean = scatter exists { _.select[Array[String]].forall(_.nonEmpty) }
 
+  // Circe can't create bidirectional links between workflows and workflow steps so this ugly var is here to link
+  // back to the parent workflow. This is needed to navigate upward for finding requirements in the containment
+  // hierarchy. There is always a workflow containing a workflow step so this is not an `Option`.
+  private[cwl] var parentWorkflow: Workflow = _
+
+  lazy val allRequirements: List[Requirement] = requirements.toList.flatten ++ parentWorkflow.allRequirements
+
+  lazy val allHints: List[Requirement] = {
+    // Just ignore any hint that isn't a Requirement.
+    val requirementHints = hints.toList.flatten.flatMap { _.select[Requirement] }
+    requirementHints ++ parentWorkflow.allHints
+  }
+
   def typedOutputs: WomTypeMap = run.fold(RunOutputsToTypeMap)
 
   def fileName: Option[String] = run.select[String]
@@ -74,8 +90,8 @@ case class WorkflowStep(
     if (haveWeSeenThisStep) Right(knownNodes)
     else {
       val callable: Checked[Callable] = run match {
-        case Run.CommandLineTool(clt) => clt.buildTaskDefinition(Option(workflow), validator).toEither
-        case Run.Workflow(wf) => wf.womDefinition(validator, parentWorkflow = Option(workflow))
+        case Run.CommandLineTool(clt) => clt.buildTaskDefinition(validator).toEither
+        case Run.Workflow(wf) => wf.womDefinition(validator)
         // TODO CWL (obviously):
         case Run.ExpressionTool(_) => throw new Exception("ExpressionTool is not supported as a workflow step yet")
       }

cwl/src/test/scala/cwl/CwlWorkflowWomSpec.scala

@@ -36,7 +36,7 @@ class CwlWorkflowWomSpec extends FlatSpec with Matchers with TableDrivenProperty
               map(_.select[CommandLineTool].get).
               value.
               unsafeRunSync
-      taskDef <- clt.buildTaskDefinition(None, _.validNel).toEither
+      taskDef <- clt.buildTaskDefinition(_.validNel).toEither
     } yield validateWom(taskDef)).leftMap(e => throw new RuntimeException(s"error! $e"))
   }