Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify that component namespace and name combinations are unique #685

Merged
merged 5 commits into from
Apr 24, 2024
Merged

Verify that component namespace and name combinations are unique #685

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f29b964
Verify that component namespace and name combinations are unique
Grifs Apr 22, 2024
abd0e6c
add PR#
Grifs Apr 22, 2024
afdb974
Merge branch 'develop_0_8' into feature/check_component_name_uniqueness
Grifs Apr 23, 2024
80dab64
Use Namespace.targetOutputPath to determine component name uniqueness
Grifs Apr 23, 2024
964b989
fix incorrect namespace, sort duplicate names and strip leading slashes
Grifs Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 #685).

## BUG FIXES

* `NextflowPlatform`: Fix publishing state for output arguments with `multiple: true` (#638, PR #639).
Expand Down
10 changes: 10 additions & 0 deletions src/main/scala/io/viash/config/Config.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could also add organisation to this id. Wdyt?

Copy link
Collaborator Author

@Grifs Grifs Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the organisation isn't used in the output path, so collisions could still happen then.
However, this reminds me that I wanted to look into using ViashNamespace.targetOutputPath(...) as the key generator - as that is what determines what would result in collisions and means that once we have more flexibility in the folder name strategy as previously discussed, we're halfway there in solving that too.

Wdyt? ping pong

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see -- good point.

Should we even consider the use-case where one might use multiple organisations in one project -- perhaps not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one I see most likely to happen is a project that sets the organisation but with one or more components where the organisation is not set. In that regard it's still 2 different values for the organisation.
In itself it doesn't really hurt much but it's probably an error.

If we want a check for that, I'd suggest displaying a warning and adding it to Viash 0.9 (or later).

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)
}
}
90 changes: 90 additions & 0 deletions src/test/scala/io/viash/e2e/ns_build/MainNSBuildNativeSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Loading