From f29b96465872a2524a291fa0034c5290a8c1445d Mon Sep 17 00:00:00 2001 From: Hendrik Cannoodt Date: Mon, 22 Apr 2024 14:07:39 +0200 Subject: [PATCH 1/4] Verify that component namespace and name combinations are unique --- CHANGELOG.md | 2 + src/main/scala/io/viash/config/Config.scala | 10 +++ .../e2e/ns_build/MainNSBuildNativeSuite.scala | 90 +++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c358f255e..527dd88d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ * `viash test` and `viash ns test`: Add a hidden `--deterministic_working directory` argument to use a fixed directory path (PR #683). +* `component names`: Verify that component namespace and name combinations are unique (PR #...). + ## BUG FIXES * `NextflowPlatform`: Fix publishing state for output arguments with `multiple: true` (#638, PR #639). diff --git a/src/main/scala/io/viash/config/Config.scala b/src/main/scala/io/viash/config/Config.scala index 3a474c73b..9da7f5455 100644 --- a/src/main/scala/io/viash/config/Config.scala +++ b/src/main/scala/io/viash/config/Config.scala @@ -365,6 +365,16 @@ object Config extends Logging { val allConfigs = allConfigsWithStdOutErr.collect({ case Left((config, _, _)) if config.functionality.isEnabled => config }) + // Verify that all configs except the disabled ones are unique + // Even configs disabled by the query should be unique as they might be picked up as a dependency + // Only allowed exception are configs where the status is set to disabled + val uniqueConfigs = allConfigs.groupBy(config => config.functionality.namespace.map(_ + "/").getOrElse("") + config.functionality.name) + val duplicateConfigs = uniqueConfigs.filter(_._2.size > 1) + if (duplicateConfigs.nonEmpty) { + val duplicateNames = duplicateConfigs.keys.mkString(", ") + throw new RuntimeException(s"Duplicate component name${ if (duplicateConfigs.size == 1) "" else "s" } found: $duplicateNames") + } + (filteredConfigs, allConfigs) } } diff --git a/src/test/scala/io/viash/e2e/ns_build/MainNSBuildNativeSuite.scala b/src/test/scala/io/viash/e2e/ns_build/MainNSBuildNativeSuite.scala index 269d7ffcd..8c3eabdf1 100644 --- a/src/test/scala/io/viash/e2e/ns_build/MainNSBuildNativeSuite.scala +++ b/src/test/scala/io/viash/e2e/ns_build/MainNSBuildNativeSuite.scala @@ -109,6 +109,96 @@ class MainNSBuildNativeSuite extends AnyFunSuite with BeforeAndAfterAll{ } } + test("Check uniqueness of component names, same name, different namespace") { + val compStr = + """functionality: + | name: comp + | namespace: %s + |""".stripMargin + val tempSrcDir = IO.makeTemp("viash_ns_build_check_uniqueness_src") + IO.write(compStr.format("ns1"), tempSrcDir.resolve("config1.vsh.yaml")) + IO.write(compStr.format("ns2"), tempSrcDir.resolve("config2.vsh.yaml")) + + val tempTargetDir = IO.makeTemp("viash_ns_build_check_uniqueness_target") + + val (stdout, stderr, exitCode) = TestHelper.testMainWithStdErr( + "ns", "build", + "-s", tempSrcDir.toString(), + "-t", tempTargetDir.toString() + ) + + assert(exitCode == 0) + assert(stderr.contains("All 2 configs built successfully")) + } + + test("Check uniqueness of component names, different name, same namespace") { + val compStr = + """functionality: + | name: %s + | namespace: ns + |""".stripMargin + val tempSrcDir = IO.makeTemp("viash_ns_build_check_uniqueness_src") + IO.write(compStr.format("comp1"), tempSrcDir.resolve("config1.vsh.yaml")) + IO.write(compStr.format("comp2"), tempSrcDir.resolve("config2.vsh.yaml")) + + val tempTargetDir = IO.makeTemp("viash_ns_build_check_uniqueness_target") + + val (stdout, stderr, exitCode) = TestHelper.testMainWithStdErr( + "ns", "build", + "-s", tempSrcDir.toString(), + "-t", tempTargetDir.toString() + ) + + assert(exitCode == 0) + assert(stderr.contains("All 2 configs built successfully")) + } + + test("Check uniqueness of component names, same name, same namespace") { + val compStr = + """functionality: + | name: %s + | namespace: ns + |""".stripMargin + val tempSrcDir = IO.makeTemp("viash_ns_build_check_uniqueness_src") + IO.write(compStr.format("comp"), tempSrcDir.resolve("config1.vsh.yaml")) + IO.write(compStr.format("comp"), tempSrcDir.resolve("config2.vsh.yaml")) + + val tempTargetDir = IO.makeTemp("viash_ns_build_check_uniqueness_target") + + val testOutput = TestHelper.testMainException2[RuntimeException]( + "ns", "build", + "-s", tempSrcDir.toString(), + "-t", tempTargetDir.toString() + ) + + assert(!testOutput.error.contains("All 2 configs built successfully")) + assert(testOutput.exceptionText.contains("Duplicate component name found: ns/comp")) + } + + test("Check uniqueness of component names, same name, same namespace - multiple duplicates") { + val compStr = + """functionality: + | name: %s + | namespace: ns + |""".stripMargin + val tempSrcDir = IO.makeTemp("viash_ns_build_check_uniqueness_src") + IO.write(compStr.format("comp1"), tempSrcDir.resolve("config1.vsh.yaml")) + IO.write(compStr.format("comp1"), tempSrcDir.resolve("config2.vsh.yaml")) + IO.write(compStr.format("comp2"), tempSrcDir.resolve("config3.vsh.yaml")) + IO.write(compStr.format("comp2"), tempSrcDir.resolve("config4.vsh.yaml")) + + val tempTargetDir = IO.makeTemp("viash_ns_build_check_uniqueness_target") + + val testOutput = TestHelper.testMainException2[RuntimeException]( + "ns", "build", + "-s", tempSrcDir.toString(), + "-t", tempTargetDir.toString() + ) + + assert(!testOutput.error.contains("All 2 configs built successfully")) + assert(testOutput.exceptionText.contains("Duplicate component names found: ns/comp1, ns/comp2")) + } + override def afterAll(): Unit = { IO.deleteRecursively(temporaryFolder) } From abd0e6c7230617d6a471607d13850e0ad06f531f Mon Sep 17 00:00:00 2001 From: Hendrik Cannoodt Date: Mon, 22 Apr 2024 14:08:43 +0200 Subject: [PATCH 2/4] add PR# --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 527dd88d7..40cc2741b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ * `viash test` and `viash ns test`: Add a hidden `--deterministic_working directory` argument to use a fixed directory path (PR #683). -* `component names`: Verify that component namespace and name combinations are unique (PR #...). +* `component names`: Verify that component namespace and name combinations are unique (PR #685). ## BUG FIXES From 80dab64182c8f8b5703be663e433f013a336d13b Mon Sep 17 00:00:00 2001 From: Hendrik Cannoodt Date: Tue, 23 Apr 2024 16:44:57 +0200 Subject: [PATCH 3/4] Use Namespace.targetOutputPath to determine component name uniqueness Add wrapper to call targetOutputPath with config instead individual namespace and name fields --- src/main/scala/io/viash/Main.scala | 3 +-- src/main/scala/io/viash/ViashNamespace.scala | 5 ++++- src/main/scala/io/viash/config/Config.scala | 2 +- src/main/scala/io/viash/helpers/DependencyResolver.scala | 2 +- .../scala/io/viash/platforms/nextflow/NextflowHelper.scala | 2 +- src/main/scala/io/viash/wrapper/BashWrapper.scala | 2 +- 6 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/main/scala/io/viash/Main.scala b/src/main/scala/io/viash/Main.scala index ca6121632..01ccb2420 100644 --- a/src/main/scala/io/viash/Main.scala +++ b/src/main/scala/io/viash/Main.scala @@ -400,8 +400,7 @@ object Main extends Logging { Some(ViashNamespace.targetOutputPath( targetDir = td, platformId = pl, - namespace = config.functionality.namespace, - functionalityName = config.functionality.name + config = config )) case _ => None } diff --git a/src/main/scala/io/viash/ViashNamespace.scala b/src/main/scala/io/viash/ViashNamespace.scala index 052130f9b..da930ccff 100644 --- a/src/main/scala/io/viash/ViashNamespace.scala +++ b/src/main/scala/io/viash/ViashNamespace.scala @@ -51,6 +51,9 @@ object ViashNamespace extends Logging { list.foreach(f) } + def targetOutputPath(targetDir: String, platformId: String, config: Config): String = + targetOutputPath(targetDir, platformId, config.functionality.namespace, config.functionality.name) + def targetOutputPath( targetDir: String, platformId: String, @@ -86,7 +89,7 @@ object ViashNamespace extends Logging { if (flatten) { target } else { - targetOutputPath(target, platformId, ns, funName) + targetOutputPath(target, platformId, conf) } val nsStr = ns.map(" (" + _ + ")").getOrElse("") infoOut(s"Exporting $funName$nsStr =$platformId=> $out") diff --git a/src/main/scala/io/viash/config/Config.scala b/src/main/scala/io/viash/config/Config.scala index 9da7f5455..5c36302d6 100644 --- a/src/main/scala/io/viash/config/Config.scala +++ b/src/main/scala/io/viash/config/Config.scala @@ -368,7 +368,7 @@ object Config extends Logging { // Verify that all configs except the disabled ones are unique // Even configs disabled by the query should be unique as they might be picked up as a dependency // Only allowed exception are configs where the status is set to disabled - val uniqueConfigs = allConfigs.groupBy(config => config.functionality.namespace.map(_ + "/").getOrElse("") + config.functionality.name) + val uniqueConfigs = allConfigs.groupBy(c => Namespace.targetOutputPath("", "", c)) val duplicateConfigs = uniqueConfigs.filter(_._2.size > 1) if (duplicateConfigs.nonEmpty) { val duplicateNames = duplicateConfigs.keys.mkString(", ") diff --git a/src/main/scala/io/viash/helpers/DependencyResolver.scala b/src/main/scala/io/viash/helpers/DependencyResolver.scala index 24659f18c..389b740a2 100644 --- a/src/main/scala/io/viash/helpers/DependencyResolver.scala +++ b/src/main/scala/io/viash/helpers/DependencyResolver.scala @@ -156,7 +156,7 @@ object DependencyResolver { case "nextflow" => "main.nf" case _ => c.functionality.name } - Paths.get(ViashNamespace.targetOutputPath("", pid, c.functionality.namespace, c.functionality.name), executableName).toString() + Paths.get(ViashNamespace.targetOutputPath("", pid, c), executableName).toString() } val info = c.info.get.copy( executable = executable diff --git a/src/main/scala/io/viash/platforms/nextflow/NextflowHelper.scala b/src/main/scala/io/viash/platforms/nextflow/NextflowHelper.scala index d025c2a8a..f7ce66e1e 100644 --- a/src/main/scala/io/viash/platforms/nextflow/NextflowHelper.scala +++ b/src/main/scala/io/viash/platforms/nextflow/NextflowHelper.scala @@ -221,7 +221,7 @@ object NextflowHelper { def renderDependencies(config: Config): String = { // TODO ideally we'd already have 'thisPath' precalculated but until that day, calculate it here - val thisPath = Paths.get(ViashNamespace.targetOutputPath("", "invalid_platform_name", config.functionality.namespace, config.functionality.name)) + val thisPath = Paths.get(ViashNamespace.targetOutputPath("", "invalid_platform_name", config)) val depStrs = config.functionality.dependencies.map{ dep => NextflowHelper.renderInclude(dep, thisPath) diff --git a/src/main/scala/io/viash/wrapper/BashWrapper.scala b/src/main/scala/io/viash/wrapper/BashWrapper.scala index 2f34207d4..7a5f24c7d 100644 --- a/src/main/scala/io/viash/wrapper/BashWrapper.scala +++ b/src/main/scala/io/viash/wrapper/BashWrapper.scala @@ -231,7 +231,7 @@ object BashWrapper { val localDependenciesStrings = localDependencies.map{ d => // relativize the path of the main component to the local dependency // TODO ideally we'd already have 'thisPath' precalculated but until that day, calculate it here - val thisPath = ViashNamespace.targetOutputPath("", "invalid_platform_name", config.functionality.namespace, config.functionality.name) + val thisPath = ViashNamespace.targetOutputPath("", "invalid_platform_name", config) val relativePath = Paths.get(thisPath).relativize(Paths.get(d.configInfo.getOrElse("executable", ""))) s"${d.VIASH_DEP}=\"$$VIASH_META_RESOURCES_DIR/$relativePath\"" } From 964b989fdbdd83ee058620a83fc8ffb8b5331a99 Mon Sep 17 00:00:00 2001 From: Hendrik Cannoodt Date: Tue, 23 Apr 2024 17:14:29 +0200 Subject: [PATCH 4/4] fix incorrect namespace, sort duplicate names and strip leading slashes --- src/main/scala/io/viash/config/Config.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/scala/io/viash/config/Config.scala b/src/main/scala/io/viash/config/Config.scala index 5c36302d6..39dc8640e 100644 --- a/src/main/scala/io/viash/config/Config.scala +++ b/src/main/scala/io/viash/config/Config.scala @@ -24,6 +24,7 @@ import io.viash.helpers.{Git, GitInfo, IO, Logging} import io.viash.helpers.circe._ import io.viash.helpers.status._ import io.viash.helpers.Yaml +import io.viash.ViashNamespace.targetOutputPath import java.net.URI import io.viash.functionality.resources._ @@ -368,10 +369,10 @@ object Config extends Logging { // Verify that all configs except the disabled ones are unique // Even configs disabled by the query should be unique as they might be picked up as a dependency // Only allowed exception are configs where the status is set to disabled - val uniqueConfigs = allConfigs.groupBy(c => Namespace.targetOutputPath("", "", c)) + val uniqueConfigs = allConfigs.groupBy(c => targetOutputPath("", "", c)) val duplicateConfigs = uniqueConfigs.filter(_._2.size > 1) if (duplicateConfigs.nonEmpty) { - val duplicateNames = duplicateConfigs.keys.mkString(", ") + val duplicateNames = duplicateConfigs.keys.map(_.dropWhile(_ == '/')).toSeq.sorted.mkString(", ") throw new RuntimeException(s"Duplicate component name${ if (duplicateConfigs.size == 1) "" else "s" } found: $duplicateNames") }