Hardcode shellquoting on for StringCommandParts, fix a conformance test.

broadinstitute · Jan 17, 2018 · 9c2528a · 9c2528a
1 parent b5eda83
commit 9c2528a
Show file tree

Hide file tree

Showing 6 changed files with 27 additions and 14 deletions.
diff --git a/centaur/src/main/resources/standardTestCases/cwl_resources/cwl_resources.cwl b/centaur/src/main/resources/standardTestCases/cwl_resources/cwl_resources.cwl
@@ -21,7 +21,7 @@ $graph:
         glob: stdout
         loadContents: true
         outputEval: $(self[0].contents.trim())
-  baseCommand: ['curl', '"http://metadata.google.internal/computeMetadata/v1/instance/machine-type"', '-H', '"Metadata-Flavor: Google"']
+  baseCommand: ['curl', 'http://metadata.google.internal/computeMetadata/v1/instance/machine-type', '-H', 'Metadata-Flavor: Google']
   stdout: stdout
 
 
diff --git a/cwl/src/main/scala/cwl/command/StringCommandPart.scala b/cwl/src/main/scala/cwl/command/StringCommandPart.scala
@@ -7,10 +7,23 @@ import wom.callable.RuntimeEnvironment
 import wom.expression.IoFunctionSet
 import wom.graph.LocalName
 import wom.values.WomValue
+import StringCommandPart._
 
 case class StringCommandPart(literal: String) extends CommandPart {
   override def instantiate(inputsMap: Map[LocalName, WomValue],
                            functions: IoFunctionSet,
                            valueMapper: (WomValue) => WomValue,
-                           runtimeEnvironment: RuntimeEnvironment): ErrorOr[List[InstantiatedCommand]] = List(InstantiatedCommand(literal)).validNel
+                           runtimeEnvironment: RuntimeEnvironment): ErrorOr[List[InstantiatedCommand]] =
+    // TODO CWL shellquotes by default, but this shellquotes everything. Only shellquote what should be shellquoted.
+    List(InstantiatedCommand(literal.shellQuote)).validNel
+}
+
+object StringCommandPart {
+
+  implicit class ShellQuoteHelper(val s: String) extends AnyVal {
+    // Escape any double quote characters in the string and surround with double quotes. The CWL spec doesn't
+    // appear to say whether quotes should be of the single or double variety, but the conformance tests clearly
+    // expect double quotes to allow for interpolation of environment variables.
+    def shellQuote: String = '"' + s.replaceAll("\"", "\\\"") + '"'
+  }
 }
diff --git a/cwl/src/test/scala/cwl/CommandLineToolSpec.scala b/cwl/src/test/scala/cwl/CommandLineToolSpec.scala
@@ -79,7 +79,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
       |outputs: []
     """.stripMargin
 
-    validate(tool, List("echo", "helloA"))
+    validate(tool, List("\"echo\"", "helloA"))
   }
 
   it should "order input arguments that are bound to command line" in {
@@ -103,7 +103,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "helloA", "helloB", "helloC"))
+    validate(tool, List("\"echo\"", "helloA", "helloB", "helloC"))
   }
 
   it should "include arguments and sort them" in {
@@ -133,7 +133,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "arg0", "helloA", "arg1", "arg2", "helloB", "helloC"))
+    validate(tool, List("\"echo\"", "\"arg0\"", "helloA", "arg1", "arg2", "helloB", "helloC"))
   }
 
   it should "handle array types" in {
@@ -159,7 +159,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "helloA", "arg2", "-XXX", "-YYY", "helloD0", "-YYY", "helloD1"))
+    validate(tool, List("\"echo\"", "helloA", "arg2", "-XXX", "-YYY", "helloD0", "-YYY", "helloD1"))
   }
 
   it should "handle record schema" in {
@@ -195,7 +195,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "--prefixRecord", "--prefixFa", "helloEfa", "--prefixFb", "0", "helloA", "arg2"))
+    validate(tool, List("\"echo\"", "--prefixRecord", "--prefixFa", "helloEfa", "--prefixFb", "0", "helloA", "arg2"))
   }
 
   it should "handle prefixes properly for primitive types" in {
@@ -223,7 +223,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "--aprefix", "helloA", "--show", "--hprefix", "0", "--iprefix", "ifile"))
+    validate(tool, List("\"echo\"", "--aprefix", "helloA", "--show", "--hprefix", "0", "--iprefix", "ifile"))
   }
 
   it should "handle arrays with item separator" in {
@@ -242,7 +242,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "--array", "helloD0, helloD1"))
+    validate(tool, List("\"echo\"", "--array", "helloD0, helloD1"))
   }
 
   it should "handle arrays with valueFrom" in {
@@ -261,7 +261,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "--array", "hello"))
+    validate(tool, List("\"echo\"", "--array", "hello"))
   }
 
   it should "handle the separate field" in {
@@ -277,7 +277,7 @@ class CommandLineToolSpec extends FlatSpec with Matchers with ParallelTestExecut
                  |outputs: []
                """.stripMargin
 
-    validate(tool, List("echo", "--prefixhelloA"))
+    validate(tool, List("\"echo\"", "--prefixhelloA"))
   }
 
   it should "pull expression libs from all requirements" in {

diff --git a/cwl/src/test/scala/cwl/DirectorySpec.scala b/cwl/src/test/scala/cwl/DirectorySpec.scala
@@ -30,7 +30,7 @@ class DirectorySpec extends FlatSpec with Matchers {
     val command = call.callable.asInstanceOf[CallableTaskDefinition].instantiateCommand(
       defaultCallInputs, PlaceholderIoFunctionSet, identity, runtimeEnvironment
     ).toTry.get
-    command.commandString should be("echo exampledir")
+    command.commandString should be("""   "echo" exampledir   """.trim)
   }
 
 }
diff --git a/cwl/src/test/scala/cwl/FileSpec.scala b/cwl/src/test/scala/cwl/FileSpec.scala
@@ -31,7 +31,7 @@ class FileSpec extends FlatSpec with Matchers {
       defaultCallInputs, PlaceholderIoFunctionSet, identity, runtimeEnvironment
     ).toEither
     val command = commandEither.right.get.commandString
-    command should be("echo example.txt")
+    command should be("""   "echo" example.txt   """.trim)
   }
 
 }
diff --git a/src/bin/travis/resources/conformance_expected_failures.txt b/src/bin/travis/resources/conformance_expected_failures.txt
@@ -106,7 +106,7 @@
 106
 107
 108
-109
+#109
 110
 111
 112