Handle exception when __merge__ uses an invalid yaml (#570)

* Handle exception when __merge__ uses an invalid yaml

Throw a clean exception instead of a stacktrace
Handle same issue during dependency resolution for reading config yamls

* Add the PR #

* Improve exception text when the __merge__ type is invalid

Create a new exception type inheriting AbstractConfigException

* Add some unit tests

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
 

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
 }

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

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))

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)
   }