Cli backwards compatibility testing (#3733)

* first pass at infrastructure around cli compatibility

* add example unit test

* inspect enum types

* add a basic unit test to verify behaviour of the cli checker

* revert root build.gradle

.idea/compiler.xml

@@ -185,6 +185,8 @@
       <module name="source-example-code_integrationTest" target="1.8" />
       <module name="source-example-code_main" target="1.8" />
       <module name="source-example-code_test" target="1.8" />
+      <module name="test-cli_main" target="1.8" />
+      <module name="test-cli_test" target="1.8" />
       <module name="test-common_main" target="1.8" />
       <module name="test-common_test" target="1.8" />
       <module name="test-utils_integrationTest" target="1.8" />

settings.gradle

@@ -24,11 +24,12 @@ include 'experimental:kryo-hook'
 include 'experimental:corda-utils'
 include 'jdk8u-deterministic'
 include 'test-common'
+include 'test-cli'
 include 'test-utils'
 include 'smoke-test-utils'
 include 'node-driver'
 // Avoid making 'testing' a project, and allow build.gradle files to refer to these by their simple names:
-['test-common', 'test-utils', 'smoke-test-utils', 'node-driver'].each {
+['test-common', 'test-utils', 'test-cli', 'smoke-test-utils', 'node-driver'].each {
     project(":$it").projectDir = new File("$settingsDir/testing/$it")
 }
 include 'tools:explorer'

testing/test-cli/build.gradle

@@ -0,0 +1,17 @@
+apply plugin: 'java'
+apply plugin: 'kotlin'
+
+dependencies {
+    compile group: 'info.picocli', name: 'picocli', version: '3.0.1'
+    compile "org.jetbrains.kotlin:kotlin-reflect:$kotlin_version"
+    compile group: "com.fasterxml.jackson.dataformat", name: "jackson-dataformat-yaml", version: "2.9.0"
+    compile group: "com.fasterxml.jackson.core", name: "jackson-databind", version: "2.9.0"
+    compile "com.fasterxml.jackson.module:jackson-module-kotlin:2.9.+"
+    compile "junit:junit:$junit_version"
+
+}
+compileKotlin {
+    kotlinOptions {
+        languageVersion = "1.2"
+    }
+}

testing/test-cli/src/main/kotlin/net/corda/testing/CliBackwardsCompatibleTest.kt

@@ -0,0 +1,19 @@
+package net.corda.testing
+
+import junit.framework.AssertionFailedError
+
+open class CliBackwardsCompatibleTest {
+
+
+    fun checkBackwardsCompatibility(clazz: Class<*>) {
+        val checker = CommandLineCompatibilityChecker()
+        val checkResults = checker.checkCommandLineIsBackwardsCompatible(clazz)
+
+        if (checkResults.isNotEmpty()) {
+            val exceptionMessage= checkResults.map { it.message }.joinToString(separator = "\n")
+            throw AssertionFailedError("Command line is not backwards compatible:\n$exceptionMessage")
+        }
+    }
+
+
+}

testing/test-cli/src/main/kotlin/net/corda/testing/GenerateCommandLineCompat.kt

@@ -0,0 +1,188 @@
+package net.corda.testing
+
+import com.fasterxml.jackson.core.type.TypeReference
+import com.fasterxml.jackson.databind.ObjectMapper
+import com.fasterxml.jackson.dataformat.yaml.YAMLFactory
+import com.fasterxml.jackson.module.kotlin.registerKotlinModule
+import picocli.CommandLine
+import java.io.InputStream
+import java.util.*
+import kotlin.collections.ArrayList
+
+
+class CommandLineCompatibilityChecker {
+
+    fun topoSort(commandLine: CommandLine): List<CommandDescription> {
+        val toVisit = Stack<CommandLine>()
+        toVisit.push(commandLine)
+        val sorted: MutableList<CommandLine> = ArrayList();
+        while (toVisit.isNotEmpty()) {
+            val visiting = toVisit.pop()
+            sorted.add(visiting)
+            visiting.subcommands.values.sortedBy { it.commandName }.forEach {
+                toVisit.push(it)
+            }
+        }
+        return buildDescriptors(sorted)
+    }
+
+    private fun buildDescriptors(result: MutableList<CommandLine>): List<CommandDescription> {
+        return result.map { ::parseToDescription.invoke(it) }
+    }
+
+    internal fun parseToDescription(it: CommandLine): CommandDescription {
+        val commandSpec = it.commandSpec
+        val options = commandSpec.options().filterNot { it.usageHelp() || it.versionHelp() }
+                .map { hit -> hit.names().map { it to hit } }
+                .flatMap { it }
+                .sortedBy { it.first }
+                .map {
+                    val type = it.second.type()
+                    ParameterDescription(it.first, type.componentType?.canonicalName
+                            ?: type.canonicalName, it.second.required(), isMultiple(type), determineAcceptableOptions(type))
+                }
+
+        val positionals = commandSpec.positionalParameters().sortedBy { it.index() }.map {
+            val type = it.type()
+            ParameterDescription(it.index().toString(), type.componentType?.canonicalName
+                    ?: type.canonicalName, it.required(), isMultiple(type))
+        }
+        return CommandDescription(it.commandName, positionals, options)
+    }
+
+    private fun determineAcceptableOptions(type: Class<*>?): List<String> {
+        return if (type?.isEnum == true) {
+            type.enumConstants.map { it.toString() }
+        } else {
+            emptyList()
+        }
+    }
+
+    fun isMultiple(clazz: Class<*>): Boolean {
+        return Iterable::class.java.isAssignableFrom(clazz) || Array<Any>::class.java.isAssignableFrom(clazz)
+    }
+
+    fun printCommandDescription(commandLine: CommandLine) {
+        val objectMapper = ObjectMapper(YAMLFactory()).registerKotlinModule()
+        val results = topoSort(commandLine)
+        println(objectMapper.writeValueAsString(results))
+    }
+
+    fun readCommandDescription(inputStream: InputStream): List<CommandDescription> {
+        val objectMapper = ObjectMapper(YAMLFactory()).registerKotlinModule()
+        return objectMapper.readValue<List<CommandDescription>>(inputStream, object : TypeReference<List<CommandDescription>>() {});
+    }
+
+    fun checkAllCommandsArePresent(old: List<CommandDescription>, new: List<CommandDescription>): List<CliBackwardsCompatibilityValidationCheck> {
+        val oldSet = old.map { it.commandName }.toSet()
+        val newSet = new.map { it.commandName }.toSet()
+        val newIsSuperSetOfOld = newSet.containsAll(oldSet)
+        return if (!newIsSuperSetOfOld) {
+            oldSet.filterNot { newSet.contains(it) }.map {
+                CommandsChangedError("SubCommand: $it has been removed from the CLI")
+            }
+        } else {
+            emptyList()
+        }
+    }
+
+    fun checkAllOptionsArePresent(old: CommandDescription, new: CommandDescription): List<CliBackwardsCompatibilityValidationCheck> {
+        if (old.commandName != new.commandName) {
+            throw IllegalArgumentException("Commands must match (${old.commandName} != ${new.commandName})")
+        }
+        val oldSet = old.params.map { it.parameterName }.toSet()
+        val newSet = new.params.map { it.parameterName }.toSet()
+
+        val newIsSuperSetOfOld = newSet.containsAll(oldSet)
+
+        return if (!newIsSuperSetOfOld) {
+            oldSet.filterNot { newSet.contains(it) }.map {
+                OptionsChangedError("Parameter: $it has been removed from subcommand: ${old.commandName}")
+            }
+        } else {
+            emptyList()
+        }
+    }
+
+    fun checkAllPositionalCharactersArePresent(old: CommandDescription, new: CommandDescription): List<CliBackwardsCompatibilityValidationCheck> {
+        if (old.commandName != new.commandName) {
+            throw IllegalArgumentException("Commands must match (${old.commandName} != ${new.commandName})")
+        }
+        val oldSet = old.positionalParams.sortedBy { it.parameterName }.toSet()
+        val newSet = new.positionalParams.sortedBy { it.parameterName}.toSet()
+        val newIsSuperSetOfOld = newSet.containsAll(oldSet)
+        return if (!newIsSuperSetOfOld) {
+            oldSet.filterNot { newSet.contains(it) }.map {
+                PositionalArgumentsChangedError("Positional Parameter [ ${it.parameterName} ] has been removed from subcommand: ${old.commandName}")
+            }
+        } else {
+            emptyList()
+        }
+    }
+
+    fun checkAllParamsAreOfTheSameType(old: CommandDescription, new: CommandDescription): List<CliBackwardsCompatibilityValidationCheck> {
+
+        val oldMap = old.params.map { it.parameterName to it.parameterType }.toMap()
+        val newMap = new.params.map { it.parameterName to it.parameterType }.toMap()
+
+        val changedTypes = oldMap.filter { newMap[it.key] != null && newMap[it.key] != it.value }.map {
+            TypesChangedError("Parameter [ ${it.key} has changed from type: ${it.value} to ${newMap[it.key]}")
+        }
+        val oldAcceptableTypes = old.params.map { it.parameterName to it.acceptableValues }.toMap()
+        val newAcceptableTypes = new.params.map { it.parameterName to it.acceptableValues }.toMap()
+        val potentiallyChanged = oldAcceptableTypes.filter { newAcceptableTypes[it.key] != null && newAcceptableTypes[it.key]!!.toSet() != it.value.toSet() }
+        val missingEnumErrors = potentiallyChanged.map {
+            val oldEnums = it.value
+            val newEnums = newAcceptableTypes[it.key]!!
+            if (!newEnums.containsAll(oldEnums)) {
+                val toPrint = oldEnums.toMutableSet()
+                toPrint.removeAll(newAcceptableTypes[it.key]!!)
+                EnumOptionsChangedError(it.key + " on command ${old.commandName} previously accepted: $oldEnums, and now is missing $toPrint}")
+            } else {
+                null
+            }
+        }.filterNotNull()
+        return changedTypes + missingEnumErrors
+
+    }
+
+    fun checkCommandLineIsBackwardsCompatible(commandLineToCheck: Class<*>): List<CliBackwardsCompatibilityValidationCheck> {
+        val commandLineToCheckName = commandLineToCheck.canonicalName
+        val instance = commandLineToCheck.newInstance()
+        val resourceAsStream = this.javaClass.classLoader.getResourceAsStream("$commandLineToCheckName.yml")
+                ?: throw IllegalStateException("no Descriptor for $commandLineToCheckName found on classpath")
+        val old = readCommandDescription(resourceAsStream)
+        val new = topoSort(CommandLine(instance))
+        return checkCommandLineIsBackwardsCompatible(old, new)
+    }
+
+
+    fun checkBackwardsCompatibility(old: CommandLine, new: CommandLine): List<CliBackwardsCompatibilityValidationCheck> {
+        val topoSortOld= topoSort(old)
+        val topoSortNew= topoSort(new)
+        return checkCommandLineIsBackwardsCompatible(topoSortOld, topoSortNew)
+    }
+
+    private fun checkCommandLineIsBackwardsCompatible(old: List<CommandDescription>, new: List<CommandDescription>): List<CliBackwardsCompatibilityValidationCheck> {
+        val results = ArrayList<CliBackwardsCompatibilityValidationCheck>()
+        results += checkAllCommandsArePresent(old, new)
+        for (oldCommand in old) {
+            new.find { it.commandName == oldCommand.commandName }?.let { newCommand ->
+                results += checkAllOptionsArePresent(oldCommand, newCommand)
+                results += checkAllParamsAreOfTheSameType(oldCommand, newCommand)
+                results += checkAllPositionalCharactersArePresent(oldCommand, newCommand)
+            }
+        }
+
+        return results
+    }
+}
+
+open class CliBackwardsCompatibilityValidationCheck(val message: String)
+class OptionsChangedError(error: String) : CliBackwardsCompatibilityValidationCheck(error)
+class TypesChangedError(error: String) : CliBackwardsCompatibilityValidationCheck(error)
+class EnumOptionsChangedError(error: String) : CliBackwardsCompatibilityValidationCheck(error)
+class CommandsChangedError(error: String) : CliBackwardsCompatibilityValidationCheck(error)
+class PositionalArgumentsChangedError(error: String) : CliBackwardsCompatibilityValidationCheck(error)
+data class CommandDescription(val commandName: String, val positionalParams: List<ParameterDescription>, val params: List<ParameterDescription>)
+data class ParameterDescription(val parameterName: String, val parameterType: String, val required: Boolean, val multiParam: Boolean, val acceptableValues: List<String> = emptyList())

testing/test-cli/src/test/kotlin/net/corda/testing/CommandLineCompatibilityCheckerTest.kt

@@ -0,0 +1,106 @@
+package net.corda.testing
+
+import org.hamcrest.CoreMatchers.*
+import org.junit.Assert
+import org.junit.Test
+import picocli.CommandLine
+import java.util.regex.Pattern
+
+class CommandLineCompatibilityCheckerTest {
+
+    enum class AllOptions {
+        YES, NO, MAYBZ
+    }
+
+    enum class BinaryOptions {
+        YES, NO
+    }
+
+
+    @Test
+    fun `should detect missing parameter`() {
+        val value1 = object {
+            @CommandLine.Option(names = arrayOf("-d", "--directory"), description = arrayOf("the directory to run in"))
+            var baseDirectory: String? = null
+        }
+        val value2 = object {
+            @CommandLine.Option(names = arrayOf("--directory"), description = arrayOf("the directory to run in"))
+            var baseDirectory: String? = null
+        }
+        val breaks = CommandLineCompatibilityChecker().checkBackwardsCompatibility(CommandLine(value1), CommandLine(value2))
+        Assert.assertThat(breaks.size, `is`(1))
+        Assert.assertThat(breaks.first(), `is`(instanceOf(OptionsChangedError::class.java)))
+    }
+
+
+    @Test
+    fun `should detect changes in positional parameters`() {
+        val value1 = object {
+            @CommandLine.Parameters(index = "0")
+            var baseDirectory: String? = null
+            @CommandLine.Parameters(index = "1")
+            var depth: Pattern? = null
+        }
+        val value2 = object {
+            @CommandLine.Parameters(index = "1")
+            var baseDirectory: String? = null
+            @CommandLine.Parameters(index = "0")
+            var depth: Int? = null
+        }
+        val breaks = CommandLineCompatibilityChecker().checkBackwardsCompatibility(CommandLine(value1), CommandLine(value2))
+        Assert.assertThat(breaks.size, `is`(2))
+        Assert.assertThat(breaks.first(), `is`(instanceOf(PositionalArgumentsChangedError::class.java)))
+    }
+
+    @Test
+    fun `should detect removal of a subcommand`() {
+        @CommandLine.Command(subcommands = [ListCommand::class, StatusCommand::class])
+        class Dummy
+
+        @CommandLine.Command(subcommands = [ListCommand::class])
+        class Dummy2
+
+        val breaks = CommandLineCompatibilityChecker().checkBackwardsCompatibility(CommandLine(Dummy()), CommandLine(Dummy2()))
+        Assert.assertThat(breaks.size, `is`(1))
+        Assert.assertThat(breaks.first(), `is`(instanceOf(CommandsChangedError::class.java)))
+    }
+
+    @Test
+    fun `should detect change of parameter type`() {
+        val value1 = object {
+            @CommandLine.Option(names = ["--directory"], description = ["the directory to run in"])
+            var baseDirectory: String? = null
+        }
+        val value2 = object {
+            @CommandLine.Option(names = ["--directory"], description = ["the directory to run in"])
+            var baseDirectory: Pattern? = null
+        }
+
+        val breaks = CommandLineCompatibilityChecker().checkBackwardsCompatibility(CommandLine(value1), CommandLine(value2))
+        Assert.assertThat(breaks.size, `is`(1))
+        Assert.assertThat(breaks.first(), `is`(instanceOf(TypesChangedError::class.java)))
+    }
+
+    @Test
+    fun `should detect change of enum options`() {
+        val value1 = object {
+            @CommandLine.Option(names = ["--directory"], description = ["the directory to run in"])
+            var baseDirectory: AllOptions? = null
+        }
+        val value2 = object {
+            @CommandLine.Option(names = ["--directory"], description = ["the directory to run in"])
+            var baseDirectory: BinaryOptions? = null
+        }
+
+        val breaks = CommandLineCompatibilityChecker().checkBackwardsCompatibility(CommandLine(value1), CommandLine(value2))
+        Assert.assertThat(breaks.filter { it is EnumOptionsChangedError }.size, `is`(1))
+        Assert.assertThat(breaks.first { it is EnumOptionsChangedError }.message, containsString(AllOptions.MAYBZ.name))
+    }
+
+    @CommandLine.Command(name = "status")
+    class StatusCommand
+
+    @CommandLine.Command(name = "ls")
+    class ListCommand
+
+}

testing/test-cli/src/test/resources/net.corda.testing.Dummy.yml

@@ -0,0 +1,77 @@
+- commandName: "<main class>"
+  positionalParams:
+  - parameterName: "0"
+    parameterType: "java.net.InetAddress"
+    required: true
+    multiParam: false
+    acceptableValues: []
+  - parameterName: "1"
+    parameterType: "int"
+    required: true
+    multiParam: false
+    acceptableValues: []
+  params:
+  - parameterName: "--directory"
+    parameterType: "java.lang.String"
+    required: false
+    multiParam: false
+    acceptableValues: []
+  - parameterName: "-d"
+    parameterType: "java.lang.String"
+    required: false
+    multiParam: false
+    acceptableValues: []
+- commandName: "status"
+  positionalParams: []
+  params:
+  - parameterName: "--pattern"
+    parameterType: "java.lang.String"
+    required: false
+    multiParam: true
+    acceptableValues: []
+  - parameterName: "--style"
+    parameterType: "net.corda.testing.DummyEnum"
+    required: false
+    multiParam: false
+    acceptableValues:
+    - "FULL"
+    - "DIR"
+    - "FILE"
+    - "DISK"
+  - parameterName: "-p"
+    parameterType: "java.lang.String"
+    required: false
+    multiParam: true
+    acceptableValues: []
+  - parameterName: "-s"
+    parameterType: "net.corda.testing.DummyEnum"
+    required: false
+    multiParam: false
+    acceptableValues:
+    - "FULL"
+    - "DIR"
+    - "FILE"
+    - "DISK"
+- commandName: "ls"
+  positionalParams:
+  - parameterName: "0"
+    parameterType: "java.lang.String"
+    required: true
+    multiParam: false
+    acceptableValues: []
+  - parameterName: "1"
+    parameterType: "int"
+    required: true
+    multiParam: false
+    acceptableValues: []
+  params:
+  - parameterName: "--depth"
+    parameterType: "java.lang.Integer"
+    required: false
+    multiParam: false
+    acceptableValues: []
+  - parameterName: "-d"
+    parameterType: "java.lang.Integer"
+    required: false
+    multiParam: false
+    acceptableValues: []