From 8f3259e409fc2d2a8fe21a1e5ebddcd3622664b0 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Wed, 5 Jul 2017 17:44:52 -0400 Subject: [PATCH 1/7] Made 'size' accept empty files, Cromwell-side --- .../main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala | 11 ++++++++++- project/Dependencies.scala | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala index 073bb6a91..263d6abd5 100644 --- a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala +++ b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala @@ -122,11 +122,20 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions override def size(params: Seq[Try[WdlValue]]): Try[WdlFloat] = { def toUnit(wdlValue: Try[WdlValue]) = wdlValue flatMap { unit => Try(MemoryUnit.fromSuffix(unit.valueString)) } + def optionalSafeFileSize(value: WdlValue): Try[Double] = value match { + case f: WdlFile => Try(buildPath(f.valueString).size.toDouble) + case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) map { coerced => buildPath(coerced.asInstanceOf[WdlFile].valueString).size.toDouble } + case WdlOptionalValue(f, Some(o)) if WdlFileType.isCoerceableFrom(f) => optionalSafeFileSize(o) + case WdlOptionalValue(f, None) if WdlFileType.isCoerceableFrom(f) => Success(0d) + case other => Failure(new Exception(s"The 'size' method expects a File argument but got a ${value.wdlType.toWdlString}.")) + } + def fileSize(wdlValue: Try[WdlValue], convertTo: Try[MemoryUnit] = Success(MemoryUnit.Bytes)) = { for { value <- wdlValue unit <- convertTo - } yield MemorySize(buildPath(value.valueString).size.toDouble, MemoryUnit.Bytes).to(unit).amount + fileSize <- optionalSafeFileSize(value) + } yield MemorySize(fileSize, MemoryUnit.Bytes).to(unit).amount } params match { diff --git a/project/Dependencies.scala b/project/Dependencies.scala index eec5ddb6a..e53e19a25 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -2,7 +2,7 @@ import sbt._ object Dependencies { lazy val lenthallV = "0.25" - lazy val wdl4sV = "0.13" + lazy val wdl4sV = "0.14-c3be413-SNAP" lazy val akkaV = "2.4.17" lazy val akkaHttpV = "10.0.9" From 98c8c5a2b422f26ee3d1a07c22b55a2a7a36df7d Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Mon, 10 Jul 2017 10:03:57 -0400 Subject: [PATCH 2/7] PR fixup --- backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala index 263d6abd5..729c9b755 100644 --- a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala +++ b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala @@ -54,7 +54,7 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions fileSize <- fileSize(fileName) _ = if (fileSize > limit) { val errorMsg = s"Use of $fileName failed because the file was too big ($fileSize bytes when only files of up to $limit bytes are permissible" - throw new FileSizeTooBig(errorMsg) + throw FileSizeTooBig(errorMsg) } } yield () @@ -124,7 +124,7 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions def optionalSafeFileSize(value: WdlValue): Try[Double] = value match { case f: WdlFile => Try(buildPath(f.valueString).size.toDouble) - case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) map { coerced => buildPath(coerced.asInstanceOf[WdlFile].valueString).size.toDouble } + case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) flatMap optionalSafeFileSize case WdlOptionalValue(f, Some(o)) if WdlFileType.isCoerceableFrom(f) => optionalSafeFileSize(o) case WdlOptionalValue(f, None) if WdlFileType.isCoerceableFrom(f) => Success(0d) case other => Failure(new Exception(s"The 'size' method expects a File argument but got a ${value.wdlType.toWdlString}.")) From 7c630f05d6ff2f5194f92f5f0b5d77368acacbd0 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Mon, 10 Jul 2017 11:59:07 -0400 Subject: [PATCH 3/7] WDL4S bump --- project/Dependencies.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project/Dependencies.scala b/project/Dependencies.scala index e53e19a25..e74539fc9 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -2,7 +2,7 @@ import sbt._ object Dependencies { lazy val lenthallV = "0.25" - lazy val wdl4sV = "0.14-c3be413-SNAP" + lazy val wdl4sV = "0.14-2fa3ae3-SNAP" lazy val akkaV = "2.4.17" lazy val akkaHttpV = "10.0.9" From 1eb555a9802bee7e9ebf7673f8ce5da2a0dd61ba Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Mon, 10 Jul 2017 12:12:28 -0400 Subject: [PATCH 4/7] WDL4S bump part II --- project/Dependencies.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project/Dependencies.scala b/project/Dependencies.scala index e74539fc9..1f2b8a95c 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -2,7 +2,7 @@ import sbt._ object Dependencies { lazy val lenthallV = "0.25" - lazy val wdl4sV = "0.14-2fa3ae3-SNAP" + lazy val wdl4sV = "0.14-f126e40-SNAP" lazy val akkaV = "2.4.17" lazy val akkaHttpV = "10.0.9" From e396db3699dd9d68860c503f55e2d54149c970ee Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Tue, 11 Jul 2017 07:27:57 -0400 Subject: [PATCH 5/7] Bunch of extra tests for the ReadLikeFunctions --- .../cromwell/backend/wdl/ReadLikeFunctions.scala | 23 +++-- .../backend/wdl/ReadLikeFunctionsSpec.scala | 99 ++++++++++++++++++++++ project/Dependencies.scala | 2 +- 3 files changed, 117 insertions(+), 7 deletions(-) create mode 100644 backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala diff --git a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala index 729c9b755..faa082632 100644 --- a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala +++ b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala @@ -4,7 +4,7 @@ import cromwell.backend.MemorySize import cromwell.core.path.PathFactory import wdl4s.expression.WdlStandardLibraryFunctions import wdl4s.parser.MemoryUnit -import wdl4s.types.{WdlArrayType, WdlFileType, WdlObjectType, WdlStringType} +import wdl4s.types._ import wdl4s.values._ import scala.util.{Failure, Success, Try} @@ -119,17 +119,28 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions override def read_boolean(params: Seq[Try[WdlValue]]): Try[WdlBoolean] = read_string(params) map { s => WdlBoolean(java.lang.Boolean.parseBoolean(s.value.trim.toLowerCase)) } + protected def size(file: WdlFile): Try[Double] = Try(buildPath(file.valueString).size.toDouble) + override def size(params: Seq[Try[WdlValue]]): Try[WdlFloat] = { + // Inner function: get the memory unit from the second (optional) parameter def toUnit(wdlValue: Try[WdlValue]) = wdlValue flatMap { unit => Try(MemoryUnit.fromSuffix(unit.valueString)) } + // Inner function: is this a file type, or an optional containing a file type? + def isOptionalOfFileType(wdlType: WdlType): Boolean = wdlType match { + case WdlFileType => true + case WdlOptionalType(inner) => isOptionalOfFileType(inner) + case _ => false + } + + // Inner function: Get the file size, allowing for unpacking of optionals def optionalSafeFileSize(value: WdlValue): Try[Double] = value match { - case f: WdlFile => Try(buildPath(f.valueString).size.toDouble) - case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) flatMap optionalSafeFileSize - case WdlOptionalValue(f, Some(o)) if WdlFileType.isCoerceableFrom(f) => optionalSafeFileSize(o) - case WdlOptionalValue(f, None) if WdlFileType.isCoerceableFrom(f) => Success(0d) - case other => Failure(new Exception(s"The 'size' method expects a File argument but got a ${value.wdlType.toWdlString}.")) + case f: WdlFile => size(f) + case WdlOptionalValue(_, Some(o)) => optionalSafeFileSize(o) + case WdlOptionalValue(f, None) if isOptionalOfFileType(f) => Success(0d) + case _ => Failure(new Exception(s"The 'size' method expects a File argument but instead got ${value.wdlType.toWdlString}.")) } + // Inner function: get the file size and convert into the requested memory unit def fileSize(wdlValue: Try[WdlValue], convertTo: Try[MemoryUnit] = Success(MemoryUnit.Bytes)) = { for { value <- wdlValue diff --git a/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala b/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala new file mode 100644 index 000000000..fb50b1a6e --- /dev/null +++ b/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala @@ -0,0 +1,99 @@ +package cromwell.backend.wdl + +import cromwell.core.path.PathBuilder +import org.apache.commons.lang3.NotImplementedException +import org.scalatest.{FlatSpec, Matchers} +import wdl4s.expression.PureStandardLibraryFunctionsLike +import wdl4s.types.{WdlFileType, WdlOptionalType, WdlStringType} +import wdl4s.values.{WdlFile, WdlFloat, WdlOptionalValue, WdlSingleFile, WdlString} + +import scala.util.{Failure, Success, Try} + +class ReadLikeFunctionsSpec extends FlatSpec with Matchers { + + behavior of "ReadLikeFunctions.size" + + it should "correctly report a 2048 byte file, in bytes by default" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlSingleFile("blah")))) should be(Success(WdlFloat(2048d))) + } + + it should "correctly report a 2048 byte file, in bytes" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlSingleFile("blah")), Success(WdlString("B")))) should be(Success(WdlFloat(2048d))) + } + + it should "correctly report a 2048 byte file, in KB" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlSingleFile("blah")), Success(WdlString("KB")))) should be(Success(WdlFloat(2.048d))) + } + + it should "correctly report a 2048 byte file, in KiB" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlSingleFile("blah")), Success(WdlString("Ki")))) should be(Success(WdlFloat(2d))) + } + + it should "correctly report the size of a supplied, optional, 2048 byte file" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlOptionalValue(WdlFileType, Some(WdlSingleFile("blah")))))) should be(Success(WdlFloat(2048d))) + } + + it should "correctly report the size of a supplied, optional optional, 2048 byte file" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlOptionalValue(WdlOptionalType(WdlFileType), Some(WdlOptionalValue(WdlFileType, Some(WdlSingleFile("blah")))))))) should be(Success(WdlFloat(2048d))) + } + + it should "correctly report the size of a supplied, optional, 2048 byte file, in MB" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlOptionalValue(WdlFileType, Some(WdlSingleFile("blah")))), Success(WdlString("MB")))) should be(Success(WdlFloat(0.002048d))) + } + + it should "correctly report that an unsupplied optional file is empty" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlOptionalValue(WdlFileType, None)))) should be(Success(WdlFloat(0d))) + } + + it should "correctly report that an unsupplied File?? is empty" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlOptionalValue(WdlOptionalType(WdlFileType), None)))) should be(Success(WdlFloat(0d))) + } + + it should "correctly report that an unsupplied optional file is empty, even in MB" in { + val readLike = new TestReadLikeFunctions(Success(2048d)) + readLike.size(Seq(Success(WdlOptionalValue(WdlFileType, None)), Success(WdlString("MB")))) should be(Success(WdlFloat(0d))) + } + + it should "refuse to report file sizes for Strings" in { + val readLike = new TestReadLikeFunctions(Failure(new Exception("Bad result: WdlStrings shouldn't even be tried for getting file size"))) + val oops = readLike.size(Seq(Success(WdlString("blah")))) + oops match { + case Success(x) => fail(s"Expected a string to not have a file length but instead got $x") + case Failure(e) => e.getMessage should be("The 'size' method expects a File argument but instead got String.") + } + } + + it should "refuse to report file sizes for String?s" in { + val readLike = new TestReadLikeFunctions(Failure(new Exception("Bad result: WdlStrings shouldn't even be tried for getting file size"))) + val oops = readLike.size(Seq(Success(WdlOptionalValue(WdlStringType, None)))) + oops match { + case Success(x) => fail(s"Expected a string to not have a file length but instead got $x") + case Failure(e) => e.getMessage should be("The 'size' method expects a File argument but instead got String?.") + } + } + + it should "pass on underlying size reading errors" in { + val readLike = new TestReadLikeFunctions(Failure(new Exception("'size' inner exception, expect me to be passed on"))) + val oops = readLike.size(Seq(Success(WdlSingleFile("blah")))) + oops match { + case Success(x) => fail(s"The 'size' engine function didn't return the error generated in the inner 'size' method") + case Failure(e) => e.getMessage should be("'size' inner exception, expect me to be passed on") + } + } +} + + +class TestReadLikeFunctions(sizeResult: Try[Double]) extends PureStandardLibraryFunctionsLike with ReadLikeFunctions { + override protected def size(file: WdlFile): Try[Double] = sizeResult + override def pathBuilders: List[PathBuilder] = throw new NotImplementedException("Didn't expect ReadLikefunctionsSpec to need pathBuilders") +} + diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 1f2b8a95c..ae37e4c87 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -2,7 +2,7 @@ import sbt._ object Dependencies { lazy val lenthallV = "0.25" - lazy val wdl4sV = "0.14-f126e40-SNAP" + lazy val wdl4sV = "0.14-828fa39-SNAP" lazy val akkaV = "2.4.17" lazy val akkaHttpV = "10.0.9" From 3c8467ac2fbf1a5200e78cd589cea9158d395d53 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Tue, 18 Jul 2017 15:30:59 -0400 Subject: [PATCH 6/7] Actually, it does need to accept Strings --- backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala | 3 ++- .../src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala index faa082632..e8c2dd8c8 100644 --- a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala +++ b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala @@ -127,7 +127,7 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions // Inner function: is this a file type, or an optional containing a file type? def isOptionalOfFileType(wdlType: WdlType): Boolean = wdlType match { - case WdlFileType => true + case f if WdlFileType.isCoerceableFrom(f) => true case WdlOptionalType(inner) => isOptionalOfFileType(inner) case _ => false } @@ -135,6 +135,7 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions // Inner function: Get the file size, allowing for unpacking of optionals def optionalSafeFileSize(value: WdlValue): Try[Double] = value match { case f: WdlFile => size(f) + case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) flatMap { file => size(file.asInstanceOf[WdlFile]) } case WdlOptionalValue(_, Some(o)) => optionalSafeFileSize(o) case WdlOptionalValue(f, None) if isOptionalOfFileType(f) => Success(0d) case _ => Failure(new Exception(s"The 'size' method expects a File argument but instead got ${value.wdlType.toWdlString}.")) diff --git a/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala b/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala index fb50b1a6e..b7529aa36 100644 --- a/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala +++ b/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala @@ -85,7 +85,7 @@ class ReadLikeFunctionsSpec extends FlatSpec with Matchers { val readLike = new TestReadLikeFunctions(Failure(new Exception("'size' inner exception, expect me to be passed on"))) val oops = readLike.size(Seq(Success(WdlSingleFile("blah")))) oops match { - case Success(x) => fail(s"The 'size' engine function didn't return the error generated in the inner 'size' method") + case Success(_) => fail(s"The 'size' engine function didn't return the error generated in the inner 'size' method") case Failure(e) => e.getMessage should be("'size' inner exception, expect me to be passed on") } } From e25e9786b9bd89d346c5edaff06acbe3765d4530 Mon Sep 17 00:00:00 2001 From: Chris Llanwarne Date: Tue, 18 Jul 2017 17:15:34 -0400 Subject: [PATCH 7/7] Hmm --- backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala | 5 ++--- .../src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala index e8c2dd8c8..506fa7317 100644 --- a/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala +++ b/backend/src/main/scala/cromwell/backend/wdl/ReadLikeFunctions.scala @@ -119,7 +119,7 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions override def read_boolean(params: Seq[Try[WdlValue]]): Try[WdlBoolean] = read_string(params) map { s => WdlBoolean(java.lang.Boolean.parseBoolean(s.value.trim.toLowerCase)) } - protected def size(file: WdlFile): Try[Double] = Try(buildPath(file.valueString).size.toDouble) + protected def size(file: WdlValue): Try[Double] = Try(buildPath(file.valueString).size.toDouble) override def size(params: Seq[Try[WdlValue]]): Try[WdlFloat] = { // Inner function: get the memory unit from the second (optional) parameter @@ -134,8 +134,7 @@ trait ReadLikeFunctions extends PathFactory { this: WdlStandardLibraryFunctions // Inner function: Get the file size, allowing for unpacking of optionals def optionalSafeFileSize(value: WdlValue): Try[Double] = value match { - case f: WdlFile => size(f) - case f if WdlFileType.isCoerceableFrom(f.wdlType) => WdlFileType.coerceRawValue(f) flatMap { file => size(file.asInstanceOf[WdlFile]) } + case f if f.isInstanceOf[WdlFile] || WdlFileType.isCoerceableFrom(f.wdlType) => size(f) case WdlOptionalValue(_, Some(o)) => optionalSafeFileSize(o) case WdlOptionalValue(f, None) if isOptionalOfFileType(f) => Success(0d) case _ => Failure(new Exception(s"The 'size' method expects a File argument but instead got ${value.wdlType.toWdlString}.")) diff --git a/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala b/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala index b7529aa36..10d54ac5c 100644 --- a/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala +++ b/backend/src/test/scala/cromwell/backend/wdl/ReadLikeFunctionsSpec.scala @@ -5,7 +5,7 @@ import org.apache.commons.lang3.NotImplementedException import org.scalatest.{FlatSpec, Matchers} import wdl4s.expression.PureStandardLibraryFunctionsLike import wdl4s.types.{WdlFileType, WdlOptionalType, WdlStringType} -import wdl4s.values.{WdlFile, WdlFloat, WdlOptionalValue, WdlSingleFile, WdlString} +import wdl4s.values.{WdlFloat, WdlOptionalValue, WdlSingleFile, WdlString, WdlValue} import scala.util.{Failure, Success, Try} @@ -93,7 +93,7 @@ class ReadLikeFunctionsSpec extends FlatSpec with Matchers { class TestReadLikeFunctions(sizeResult: Try[Double]) extends PureStandardLibraryFunctionsLike with ReadLikeFunctions { - override protected def size(file: WdlFile): Try[Double] = sizeResult + override protected def size(file: WdlValue): Try[Double] = sizeResult override def pathBuilders: List[PathBuilder] = throw new NotImplementedException("Didn't expect ReadLikefunctionsSpec to need pathBuilders") }