Merge branch 'develop' into jd_WX-1409_java17

CHANGELOG.md

@@ -7,6 +7,10 @@
 As of this version, a distribution of Java 17 is required to run Cromwell. Cromwell is developed, tested, and
 containerized using [Eclipse Temurin](https://adoptium.net/temurin/releases/?version=17).
 
+### Fixed Optional and String Concatenation Bug
+
+As outlined in the [WDL Spec](https://github.com/openwdl/wdl/blob/main/versions/1.0/SPEC.md#prepending-a-string-to-an-optional-parameter), concatenating a string with an empty optional now correctly evaluates to the empty string.
+
 ### Improved status reporting behavior
 
 When Cromwell restarts during a workflow that is failing, it no longer reports pending tasks as a reason for that failure. 

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

@@ -0,0 +1,20 @@
+# We expect that when combining a string with an empty option, nothing is output.
+# We expect that when combining a string with a present option, the two are concatenated.
+# https://github.com/openwdl/wdl/blob/main/versions/1.0/SPEC.md#prepending-a-string-to-an-optional-parameter
+
+name: prepend_string_option
+testFormat: workflowsuccess
+
+files {
+  workflow: prepend_string_option/prepend_string_option.wdl
+}
+
+metadata {
+  workflowName: prepend_string_option
+  status: Succeeded
+
+  "outputs.prepend_string_option.neither_provided": ".maf"
+  "outputs.prepend_string_option.first_provided": "outPrefix.maf"
+  "outputs.prepend_string_option.second_provided": ".outSuffix.maf"
+  "outputs.prepend_string_option.both_provided": "outPrefix.outSuffix.maf"
+}

centaur/src/main/resources/standardTestCases/prepend_string_option/prepend_string_option.wdl

@@ -0,0 +1,33 @@
+version 1.0
+
+task prepend_string_option_task {
+    input {
+        String? out_prefix
+        String? out_suffix
+    }
+
+    command {
+        touch ${out_prefix}${'.' + out_suffix}.maf
+    }
+
+    output {
+        File outFile = "${out_prefix}${'.' + out_suffix}.maf"
+    }
+
+    runtime {
+        docker : "ubuntu:latest"
+    }
+}
+
+workflow prepend_string_option {
+    call prepend_string_option_task as both {input: out_prefix = "outPrefix", out_suffix = "outSuffix"}
+    call prepend_string_option_task as first {input: out_prefix = "outPrefix"}
+    call prepend_string_option_task as second {input: out_suffix = "outSuffix"}
+    call prepend_string_option_task as neither
+    output {
+        String both_provided = basename(both.outFile)
+        String first_provided = basename(first.outFile)
+        String second_provided = basename(second.outFile)
+        String neither_provided = basename(neither.outFile)
+    }
+}

wdl/transforms/draft3/src/main/scala/wdl/draft3/transforms/linking/expression/files/package.scala

@@ -53,6 +53,9 @@ package object files {
         case a: IndexAccess =>
           a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)
 
+        case a: StringExpression =>
+          a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)
+
         // Unary operators:
         case a: UnaryNegation =>
           a.predictFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)(fileEvaluator, valueEvaluator)

wdl/transforms/draft3/src/test/scala/wdl/draft3/transforms/expression/UnaryAndBinaryOperatorsEvaluatorSpec.scala

@@ -12,8 +12,8 @@ import wdl.model.draft3.graph.expression.EvaluatedValue
 import wdl.model.draft3.graph.expression.TypeEvaluator.ops._
 import wdl.model.draft3.graph.expression.ValueEvaluator.ops._
 import wom.expression.NoIoFunctionSet
-import wom.types.{WomBooleanType, WomIntegerType, WomType}
-import wom.values.{WomBoolean, WomInteger, WomValue}
+import wom.types.{WomBooleanType, WomIntegerType, WomStringType, WomType}
+import wom.values.{WomBoolean, WomInteger, WomOptionalValue, WomString, WomValue}
 
 /**
   * Checks that the draft3 value evaluators for ExpressionElements are wired to forward values through to the appropriate
@@ -68,4 +68,22 @@ class UnaryAndBinaryOperatorsEvaluatorSpec extends AnyFlatSpec with CromwellTime
       expression.evaluateType(Map.empty, Map.empty) shouldBeValid expectedType
     }
   }
+
+  it should "correctly evaluate the value of optional and string concatenation" in {
+    // Test name, input expression, output value, output type.
+    val prefixLiteral = PrimitiveLiteralExpressionElement(WomString("myPrefix"))
+    val suffixLiteral = IdentifierLookup("suffixOptional")
+    val inputsWithUndefinedOptional = Map("suffixOptional" -> WomOptionalValue(WomStringType, None))
+    val inputsWithDefinedOptional =
+      Map("suffixOptional" -> WomOptionalValue(WomStringType, Option(WomString("myOptionalValue"))))
+    val addExpression: ExpressionElement = Add(prefixLiteral, suffixLiteral)
+    addExpression.evaluateValue(inputsWithUndefinedOptional, NoIoFunctionSet, None) shouldBeValid EvaluatedValue(
+      WomOptionalValue(WomStringType, None),
+      Seq.empty
+    )
+    addExpression.evaluateValue(inputsWithDefinedOptional, NoIoFunctionSet, None) shouldBeValid EvaluatedValue(
+      WomString("myPrefixmyOptionalValue"),
+      Seq.empty
+    )
+  }
 }

wdl/transforms/draft3/src/test/scala/wdl/draft3/transforms/linking/expression/files/FileEvaluatorSpec.scala

@@ -6,7 +6,7 @@ import org.scalatest.flatspec.AnyFlatSpec
 import org.scalatest.matchers.should.Matchers
 import wdl.draft3.transforms.linking.expression.values.expressionEvaluator
 import wdl.model.draft3.elements.ExpressionElement
-import wdl.model.draft3.elements.ExpressionElement.{ObjectLiteral, StringLiteral}
+import wdl.model.draft3.elements.ExpressionElement._
 import wdl.model.draft3.graph.expression.FileEvaluator.ops._
 import wom.expression.NoIoFunctionSet
 import wom.types.{WomCompositeType, WomSingleFileType}
@@ -26,4 +26,20 @@ class FileEvaluatorSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matche
     )
     evaluatedFiles shouldBeValid Set(WomSingleFile("moo.txt"))
   }
+
+  it should "find files in a string expression" in {
+    val filePiece: StringPiece = StringLiteral("moo.txt")
+    val anotherFilePiece: StringPiece = StringLiteral("boo.txt")
+    val notAFilePiece: StringPiece = NewlineEscape
+    val anotherNotAFilePiece: StringPiece = BackslashEscape
+
+    val expressionElement: ExpressionElement =
+      StringExpression(Seq(filePiece, anotherFilePiece, notAFilePiece, anotherNotAFilePiece))
+
+    val evaluatedFiles = expressionElement.predictFilesNeededToEvaluate(inputs = Map.empty,
+                                                                        ioFunctionSet = NoIoFunctionSet,
+                                                                        coerceTo = WomSingleFileType
+    )
+    evaluatedFiles shouldBeValid Set(WomSingleFile("moo.txt"), WomSingleFile("boo.txt"))
+  }
 }

wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/files/LiteralEvaluators.scala

@@ -1,14 +1,16 @@
 package wdl.transforms.base.linking.expression.files
 
+import cats.data.NonEmptyList
+import cats.data.Validated.{Invalid, Valid}
+import cats.instances.list._
 import cats.syntax.apply._
 import cats.syntax.traverse._
 import cats.syntax.validated._
-import cats.instances.list._
 import common.validation.ErrorOr.ErrorOr
 import wdl.model.draft3.elements.ExpressionElement
 import wdl.model.draft3.elements.ExpressionElement._
-import wdl.model.draft3.graph.expression.{FileEvaluator, ValueEvaluator}
 import wdl.model.draft3.graph.expression.FileEvaluator.ops._
+import wdl.model.draft3.graph.expression.{FileEvaluator, ValueEvaluator}
 import wom.expression.IoFunctionSet
 import wom.types.{WomCompositeType, WomSingleFileType, WomType}
 import wom.values.{WomFile, WomSingleFile, WomValue}
@@ -107,4 +109,24 @@ object LiteralEvaluators {
        a.right.evaluateFilesNeededToEvaluate(inputs, ioFunctionSet, coerceTo)
       ) mapN { _ ++ _ }
   }
+
+  implicit val stringExpressionEvaluator: FileEvaluator[StringExpression] = new FileEvaluator[StringExpression] {
+    override def predictFilesNeededToEvaluate(a: StringExpression,
+                                              inputs: Map[String, WomValue],
+                                              ioFunctionSet: IoFunctionSet,
+                                              coerceTo: WomType
+    )(implicit
+      fileEvaluator: FileEvaluator[ExpressionElement],
+      valueEvaluator: ValueEvaluator[ExpressionElement]
+    ): ErrorOr[Set[WomFile]] = {
+      val predictedFiles: Seq[ErrorOr[Set[WomFile]]] = a.pieces.map {
+        case literal: StringLiteral =>
+          stringLiteralEvaluator.predictFilesNeededToEvaluate(literal, inputs, ioFunctionSet, coerceTo)
+        case _ => Valid(Set.empty[WomFile])
+      }
+      val sets: Set[WomFile] = predictedFiles.collect { case Valid(set) => set }.foldLeft(Set[WomFile]())(_ ++ _)
+      val errors: Seq[NonEmptyList[String]] = predictedFiles.collect { case Invalid(error) => error }
+      errors.headOption.map(head => head.invalid).getOrElse(sets.validNel)
+    }
+  }
 }

wdl/transforms/new-base/src/main/scala/wdl/transforms/base/linking/expression/values/BinaryOperatorEvaluators.scala

@@ -50,10 +50,16 @@ object BinaryOperatorEvaluators {
           a.right.evaluateValue(inputs, ioFunctionSet, forCommandInstantiationOptions) flatMap { right =>
             val rawResult = op(left.value, right.value)
 
-            // Allow unsupplied optionals, but only if we're instantiating a command:
+            // Operations with empty optionals generally fail.
+            // However, if we're instantiating a command or prepending a string, return an empty option of string.
+            // https://github.com/openwdl/wdl/blob/main/versions/1.0/SPEC.md#prepending-a-string-to-an-optional-parameter
             val handleOptionals = rawResult.recover {
               case OptionalNotSuppliedException(_) if forCommandInstantiationOptions.isDefined =>
                 WomOptionalValue(WomStringType, None)
+              case OptionalNotSuppliedException(_) =>
+                left.value.womType match {
+                  case WomStringType => WomOptionalValue(WomStringType, None)
+                }
             }
 
             handleOptionals.toErrorOr map { newValue =>