[CORDA-3243] - Improve CorDapp loading logic for duplicates (#5551)

client/rpc/src/integration-test/kotlin/net/corda/client/rpc/FlowsExecutionModeRpcTest.kt

@@ -14,7 +14,7 @@ import org.assertj.core.api.Assertions.assertThat
 import org.junit.Before
 import org.junit.Test
 
-class FlowsExecutionModeTests : NodeBasedTest(listOf("net.corda.finance.contracts", CashSchemaV1::class.packageName)) {
+class FlowsExecutionModeTests : NodeBasedTest(emptyList()) {
 
     private val rpcUser = User("user1", "test", permissions = setOf(Permissions.all()))
     private lateinit var node: NodeWithInfo

core/src/main/kotlin/net/corda/core/internal/cordapp/CordappResolver.kt

@@ -15,6 +15,8 @@ object CordappResolver {
     private val logger = loggerFor<CordappResolver>()
     private val cordappClasses: ConcurrentHashMap<String, Set<Cordapp>> = ConcurrentHashMap()
 
+    private val insideInMemoryTest: Boolean by lazy { insideInMemoryTest() }
+
     // TODO Use the StackWalker API once we migrate to Java 9+
     private var cordappResolver: () -> Cordapp? = {
         Exception().stackTrace
@@ -25,9 +27,12 @@ object CordappResolver {
                 ?.single()
     }
 
-    /*
+    /**
      * Associates class names with CorDapps or logs a warning when a CorDapp is already registered for a given class.
      * This could happen when trying to run different versions of the same CorDapp on the same node.
+     *
+     * @throws IllegalStateException when multiple CorDapps are registered for the same contract class,
+     *          since this can lead to undefined behaviour.
      */
     @Synchronized
     fun register(cordapp: Cordapp) {
@@ -39,12 +44,30 @@ object CordappResolver {
 
         notAlreadyRegisteredClasses.forEach { cordappClasses[it] = setOf(cordapp) }
 
-        for ((className, registeredCordapps) in alreadyRegistered) {
+        for ((registeredClassName, registeredCordapps) in alreadyRegistered) {
-            if (registeredCordapps.any { it.jarHash == cordapp.jarHash }) continue
+            val duplicateCordapps = registeredCordapps.filter { it.jarHash == cordapp.jarHash }.toSet()
-            if (className in contractClasses) {
+
-                logger.error("ATTENTION: More than one CorDapp installed on the node for contract $className. Please remove the previous version when upgrading to a new version.")
+            if (duplicateCordapps.isNotEmpty()) {
+                logger.warnOnce("The CorDapp (name: ${cordapp.info.shortName}, file: ${cordapp.name}) " +
+                        "is installed multiple times on the node. The following files correspond to the exact same content: " +
+                        "${duplicateCordapps.map { it.name }}")
+                continue
             }
-            cordappClasses[className] = registeredCordapps + cordapp
+            // During in-memory tests, the spawned nodes share the same CordappResolver, so detected conflicts can be spurious.
+            if (registeredClassName in contractClasses && !insideInMemoryTest) {
+                    throw IllegalStateException("More than one CorDapp installed on the node for contract $registeredClassName. " +
+                            "Please remove the previous version when upgrading to a new version.")
+            }
+
+            cordappClasses[registeredClassName] = registeredCordapps + cordapp
+        }
+    }
+
+    private fun insideInMemoryTest(): Boolean {
+        return Exception().stackTrace.any {
+            it.className.startsWith("net.corda.testing.node.internal.InternalMockNetwork") ||
+            it.className.startsWith("net.corda.testing.node.internal.InProcessNode") ||
+            it.className.startsWith("net.corda.testing.node.MockServices")
         }
     }
 

core/src/test/kotlin/net/corda/core/internal/cordapp/CordappResolverTest.kt

@@ -2,9 +2,11 @@ package net.corda.core.internal.cordapp
 
 import net.corda.core.crypto.SecureHash
 import org.assertj.core.api.Assertions.assertThat
+import org.assertj.core.api.Assertions.assertThatThrownBy
 import org.junit.After
 import org.junit.Before
 import org.junit.Test
+import java.lang.IllegalStateException
 import kotlin.test.assertEquals
 
 class CordappResolverTest {
@@ -49,19 +51,42 @@ class CordappResolverTest {
     }
 
     @Test
-    fun `when different cordapps are registered for the same class, the resolver returns null`() {
+    fun `when different cordapps are registered for the same (non-contract) class, the resolver returns null`() {
         CordappResolver.register(CordappImpl.TEST_INSTANCE.copy(
-                contractClassNames = listOf(javaClass.name),
+                contractClassNames = listOf("ContractClass1"),
                 minimumPlatformVersion = 3,
                 targetPlatformVersion = 222,
                 jarHash = SecureHash.randomSHA256()
         ))
         CordappResolver.register(CordappImpl.TEST_INSTANCE.copy(
-                contractClassNames = listOf(javaClass.name),
+                contractClassNames = listOf("ContractClass2"),
                 minimumPlatformVersion = 2,
                 targetPlatformVersion = 456,
                 jarHash = SecureHash.randomSHA256()
         ))
         assertThat(CordappResolver.currentCordapp).isNull()
     }
+
+    @Test
+    fun `when different cordapps are registered for the same (contract) class, the resolver throws an exception`() {
+        val firstCordapp = CordappImpl.TEST_INSTANCE.copy(
+            contractClassNames = listOf(javaClass.name),
+            minimumPlatformVersion = 3,
+            targetPlatformVersion = 222,
+            jarHash = SecureHash.randomSHA256()
+        )
+        val secondCordapp = CordappImpl.TEST_INSTANCE.copy(
+            contractClassNames = listOf(javaClass.name),
+            minimumPlatformVersion = 2,
+            targetPlatformVersion = 456,
+            jarHash = SecureHash.randomSHA256()
+        )
+
+        CordappResolver.register(firstCordapp)
+        assertThatThrownBy { CordappResolver.register(secondCordapp) }
+                .isInstanceOf(IllegalStateException::class.java)
+                .hasMessageContaining("More than one CorDapp installed on the node for contract ${javaClass.name}. " +
+                        "Please remove the previous version when upgrading to a new version.")
+    }
+
 }

node/src/integration-test/kotlin/net/corda/node/AddressBindingFailureTests.kt

@@ -43,7 +43,8 @@ class AddressBindingFailureTests {
                 driver(DriverParameters(startNodesInProcess = false,
                     notarySpecs = listOf(NotarySpec(notaryName)),
                     notaryCustomOverrides = mapOf("p2pAddress" to address.toString()),
-                        portAllocation = portAllocation)
+                    portAllocation = portAllocation,
+                    cordappsForAllNodes = emptyList())
                 ) {} }.isInstanceOfSatisfying(IllegalStateException::class.java) { error ->
 
                 assertThat(error.message).contains("Unable to start notaries")
@@ -56,7 +57,11 @@ class AddressBindingFailureTests {
         ServerSocket(0).use { socket ->
 
             val address = InetSocketAddress("localhost", socket.localPort).toNetworkHostAndPort()
-            driver(DriverParameters(startNodesInProcess = true, notarySpecs = emptyList(), inMemoryDB = false, portAllocation = portAllocation)) {
+            driver(DriverParameters(startNodesInProcess = true,
+                                         notarySpecs = emptyList(),
+                                         inMemoryDB = false,
+                                         portAllocation = portAllocation,
+                                         cordappsForAllNodes = emptyList())) {
 
                 assertThatThrownBy { startNode(customOverrides = overrides(address)).getOrThrow() }.isInstanceOfSatisfying(AddressBindingException::class.java) { exception ->
                     assertThat(exception.addresses).contains(address).withFailMessage("Expected addresses to contain $address but was ${exception.addresses}.")