diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e89bd247..0edc95b5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ TODO add summary * `testbenches`: Add testbenches for local dependencies (PR #565). * `testbenches`: Refactor testbenches helper functions to uniformize them (PR #565). + +## BUG FIXES + +* `__merge__`: Handle invalid yaml during merging (PR #570). There was not enough error handling during this operation. Switched to the more advanced `Convert.textToJson` helper method. # Viash 0.8.0-RC5 (2023-10-11): Fix run workflow diff --git a/src/main/scala/io/viash/exceptions/ConfigException.scala b/src/main/scala/io/viash/exceptions/ConfigException.scala index 98d837979..e053e097e 100644 --- a/src/main/scala/io/viash/exceptions/ConfigException.scala +++ b/src/main/scala/io/viash/exceptions/ConfigException.scala @@ -43,4 +43,9 @@ case class ConfigParserSubTypeException(tpe: String, validTypes: List[String], j case class ConfigParserValidationException(tpe: String, json: String) extends Exception { val shortType = tpe.split("\\.").last override def getMessage(): String = s"Invalid data fields for $shortType.\n$json" +} + +case class ConfigParserMergeException(uri: String, innerMessage: String, json: String) extends AbstractConfigException { + val e: Throwable = null + override def getMessage(): String = json } \ No newline at end of file diff --git a/src/main/scala/io/viash/helpers/DependencyResolver.scala b/src/main/scala/io/viash/helpers/DependencyResolver.scala index b177be112..fe56f439b 100644 --- a/src/main/scala/io/viash/helpers/DependencyResolver.scala +++ b/src/main/scala/io/viash/helpers/DependencyResolver.scala @@ -28,13 +28,13 @@ import java.nio.file.Files import java.io.IOException import java.io.UncheckedIOException import io.viash.helpers.{IO, Logging} -import io.circe.yaml.parser import io.circe.Json import io.viash.config.Config._ import io.viash.ViashNamespace import io.viash.functionality.dependencies.Dependency import io.viash.functionality.resources.NextflowScript import io.viash.exceptions.MissingDependencyException +import io.viash.helpers.circe.Convert object DependencyResolver extends Logging { @@ -227,7 +227,7 @@ object DependencyResolver extends Logging { // The yaml file in the target folder should be final // We're also assuming that the file will be proper yaml and an actual viash config file val yamlText = IO.read(IO.uri(configPath)) - val json = parser.parse(yamlText).toOption.get + val json = Convert.textToJson(yamlText, configPath) def getFunctionalityName(json: Json): Option[String] = { json.hcursor.downField("functionality").downField("name").as[String].toOption @@ -256,7 +256,7 @@ object DependencyResolver extends Logging { def getSparseDependencyInfo(configPath: String): List[String] = { try { val yamlText = IO.read(IO.uri(configPath)) - val json = parser.parse(yamlText).toOption.get + val json = Convert.textToJson(yamlText, configPath) val dependencies = json.hcursor.downField("functionality").downField("dependencies").focus.flatMap(_.asArray).get dependencies.flatMap(_.hcursor.downField("writtenPath").as[String].toOption).toList diff --git a/src/main/scala/io/viash/helpers/circe/RichJson.scala b/src/main/scala/io/viash/helpers/circe/RichJson.scala index 01f31b991..39a0b6fb2 100644 --- a/src/main/scala/io/viash/helpers/circe/RichJson.scala +++ b/src/main/scala/io/viash/helpers/circe/RichJson.scala @@ -26,6 +26,7 @@ import io.circe.{Json, Printer => JsonPrinter} import io.circe.yaml.{Printer => YamlPrinter} import io.viash.helpers.IO +import io.viash.exceptions._ class RichJson(json: Json) { /** @@ -150,6 +151,9 @@ class RichJson(json: Json) { List(y.asString.get) } else { // TODO: add decent error message instead of simply .get + if (y.asArray.get.filter(!_.isString).nonEmpty) { + throw new ConfigParserMergeException(uri.toString, "invalid merge tag type. Must be a String or Array of Strings", y.toString()) + } y.asArray.get.map(_.asString.get).toList } @@ -197,8 +201,7 @@ class RichJson(json: Json) { val str = IO.read(newURI) // parse as yaml - // TODO: add decent error message instead of simply .get - val newJson1 = io.circe.yaml.parser.parse(str).toOption.get + val newJson1 = Convert.textToJson(str, newURI.toString) // recurse through new json as well val newJson2 = newJson1.inherit(newURI, projectDir = projectDir, stripInherits = stripInherits) @@ -212,8 +215,8 @@ class RichJson(json: Json) { // return combined object jsMerged.asObject.get - case Some(_) => - throw new RuntimeException("Invalid merge tag type. Must be a String or Array.") + case Some(j) => + throw new ConfigParserMergeException(uri.toString, "invalid merge tag type. Must be a String or Array of Strings", j.toString()) case None => obj1 } val obj3 = obj2.mapValues(x => x.inherit(uri, projectDir = projectDir, stripInherits = stripInherits)) diff --git a/src/test/scala/io/viash/helpers/circe/RichJsonTest.scala b/src/test/scala/io/viash/helpers/circe/RichJsonTest.scala index 5aefc796d..e3f358265 100644 --- a/src/test/scala/io/viash/helpers/circe/RichJsonTest.scala +++ b/src/test/scala/io/viash/helpers/circe/RichJsonTest.scala @@ -6,6 +6,8 @@ import io.circe._ import io.circe.yaml.parser import io.viash.helpers.{IO, Logger} import java.nio.file.Files +import io.viash.exceptions._ +import java.io.FileNotFoundException class RichJsonTest extends AnyFunSuite with BeforeAndAfterAll { Logger.UseColorOverride.value = Some(false) @@ -265,6 +267,67 @@ class RichJsonTest extends AnyFunSuite with BeforeAndAfterAll { assert(jsonOut == jsonExpected) } + test("check exception when merging invalid yaml") { + val json1 = parser.parse(""" + |__merge__: not_existing.yaml + |a: [1, 2] + |""".stripMargin + ).getOrElse(Json.Null) + + val caught = intercept[FileNotFoundException] { + json1.inherit(temporaryFolder.toUri(), projectDir = None) + } + + assert(caught.getMessage().contains("not_existing.yaml")) + } + + test("check exception when __merge__ isn't a string") { + val json1 = parser.parse(""" + |__merge__: 5 + |a: [1, 2] + |""".stripMargin + ).getOrElse(Json.Null) + + val caught = intercept[ConfigParserMergeException] { + json1.inherit(temporaryFolder.toUri(), projectDir = None) + } + + assert(caught.innerMessage.contains("invalid merge tag type")) + assert(caught.getMessage().contains("5")) + } + + test("check exception when __merge__ is an array but contains something else than a string") { + val json1 = parser.parse(""" + |__merge__: [5, foo.yaml] + |a: [1, 2] + |""".stripMargin + ).getOrElse(Json.Null) + + val caught = intercept[ConfigParserMergeException] { + json1.inherit(temporaryFolder.toUri(), projectDir = None) + } + + assert(caught.innerMessage.contains("invalid merge tag type")) + assert(caught.getMessage().contains("5")) + } + + test("inherit with invalid yaml") { + // write json to file + IO.write("one: foo\n/wqwqws", temporaryFolder.resolve("invalid.yaml")) + + val json1 = parser.parse(""" + |__merge__: invalid.yaml + |a: [1, 2] + |""".stripMargin + ).getOrElse(Json.Null) + + val caught = intercept[ConfigYamlException] { + json1.inherit(temporaryFolder.toUri(), projectDir = None) + } + + assert(caught.innerMessage.contains("invalid Yaml structure")) + } + override def afterAll(): Unit = { IO.deleteRecursively(temporaryFolder) }