CORDA-2475 Adjust attachments query logic to return correct results (#4612)

* CORDA-2475 Adjust attachments query logic to return correct results sets for signed/unsigned jars.

* Updates following PR review feedback by RP.

core/src/main/kotlin/net/corda/core/node/services/AttachmentStorage.kt

@@ -1,10 +1,9 @@
 package net.corda.core.node.services
 
-import net.corda.core.CordaInternal
 import net.corda.core.DoNotImplement
 import net.corda.core.contracts.Attachment
-import net.corda.core.contracts.ContractAttachment
 import net.corda.core.crypto.SecureHash
+import net.corda.core.internal.cordapp.CordappImpl.Companion.DEFAULT_CORDAPP_VERSION
 import net.corda.core.node.services.vault.AttachmentQueryCriteria
 import net.corda.core.node.services.vault.AttachmentSort
 import java.io.IOException
@@ -79,22 +78,15 @@ interface AttachmentStorage {
     }
 
     /**
-     * Find the Attachment Id of the contract attachment with the highest version for a given contract class name
+     * Find the Attachment Id(s) of the contract attachments with the highest version for a given contract class name
-     * from trusted upload sources.  If both a signed and unsigned attachment exist, prefer the signed one.
+     * from trusted upload sources.
+     * Return highest version of both signed and unsigned attachment ids (signed first, unsigned second), otherwise return a
+     * single signed or unsigned version id, or an empty list if none meet the criteria.
      *
      * @param contractClassName The fully qualified name of the contract class.
      * @param minContractVersion The minimum contract version that should be returned.
-     * @return the [AttachmentId] of the contract, or null if none meet the criteria.
+     * @return the [AttachmentId]s of the contract attachments (signed always first in list), or an empty list if none meet the criteria.
      */
-    fun getContractAttachmentWithHighestContractVersion(contractClassName: String, minContractVersion: Int): AttachmentId?
+    fun getLatestContractAttachments(contractClassName: String, minContractVersion: Int = DEFAULT_CORDAPP_VERSION): List<AttachmentId>
-
-    /**
-     * Find the Attachment Ids of the contract attachments for a given contract class name
-     * from trusted upload sources.
-     *
-     * @param contractClassName The fully qualified name of the contract class.
-     * @return the [AttachmentId]s of the contract attachments, or an empty set if none meet the criteria.
-     */
-    fun getContractAttachments(contractClassName: String): Set<AttachmentId>
 }
 

core/src/main/kotlin/net/corda/core/transactions/TransactionBuilder.kt

@@ -288,7 +288,7 @@ open class TransactionBuilder(
         val outputHashConstraints = outputStates?.filter { it.constraint is HashAttachmentConstraint } ?: emptyList()
         val outputSignatureConstraints = outputStates?.filter { it.constraint is SignatureAttachmentConstraint } ?: emptyList()
         if (inputsHashConstraints.isNotEmpty() && (outputHashConstraints.isNotEmpty() || outputSignatureConstraints.isNotEmpty())) {
-            val attachmentIds = services.attachments.getContractAttachments(contractClassName)
+            val attachmentIds = services.attachments.getLatestContractAttachments(contractClassName)
             // only switchover if we have both signed and unsigned attachments for the given contract class name
             if (attachmentIds.isNotEmpty() && attachmentIds.size == 2) {
                 val attachmentsToUse = attachmentIds.map {
@@ -465,7 +465,7 @@ open class TransactionBuilder(
         require(isReference || constraints.none { it is HashAttachmentConstraint })
 
         val minimumRequiredContractClassVersion = stateRefs?.map { services.loadContractAttachment(it).contractVersion }?.max() ?: DEFAULT_CORDAPP_VERSION
-        return services.attachments.getContractAttachmentWithHighestContractVersion(contractClassName, minimumRequiredContractClassVersion)
+        return services.attachments.getLatestContractAttachments(contractClassName, minimumRequiredContractClassVersion).firstOrNull()
                 ?: throw MissingContractAttachments(states, minimumRequiredContractClassVersion)
     }
 

core/src/test/kotlin/net/corda/core/transactions/TransactionBuilderTest.kt

@@ -59,8 +59,8 @@ class TransactionBuilderTest {
         doReturn(setOf(DummyContract.PROGRAM_ID)).whenever(attachment).allContracts
         doReturn("app").whenever(attachment).uploader
         doReturn(emptyList<Party>()).whenever(attachment).signerKeys
-        doReturn(contractAttachmentId).whenever(attachmentStorage)
+        doReturn(listOf(contractAttachmentId)).whenever(attachmentStorage)
-                .getContractAttachmentWithHighestContractVersion("net.corda.testing.contracts.DummyContract", DEFAULT_CORDAPP_VERSION)
+                .getLatestContractAttachments("net.corda.testing.contracts.DummyContract")
     }
 
     @Test
@@ -146,8 +146,8 @@ class TransactionBuilderTest {
 
         doReturn(attachments).whenever(services).attachments
         doReturn(signedAttachment).whenever(attachments).openAttachment(contractAttachmentId)
-        doReturn(contractAttachmentId).whenever(attachments)
+        doReturn(listOf(contractAttachmentId)).whenever(attachments)
-                .getContractAttachmentWithHighestContractVersion("net.corda.testing.contracts.DummyContract", DEFAULT_CORDAPP_VERSION)
+                .getLatestContractAttachments("net.corda.testing.contracts.DummyContract")
 
         val outputState = TransactionState(data = DummyState(), contract = DummyContract.PROGRAM_ID, notary = notary)
         val builder = TransactionBuilder()

node-api/src/test/kotlin/net/corda/nodeapi/internal/AttachmentsClassLoaderStaticContractTests.kt

@@ -9,7 +9,6 @@ import net.corda.core.crypto.SecureHash
 import net.corda.core.identity.AbstractParty
 import net.corda.core.identity.CordaX500Name
 import net.corda.core.identity.Party
-import net.corda.core.internal.cordapp.CordappImpl.Companion.DEFAULT_CORDAPP_VERSION
 import net.corda.core.node.ServicesForResolution
 import net.corda.core.node.services.AttachmentStorage
 import net.corda.core.node.services.NetworkParametersService
@@ -90,8 +89,8 @@ class AttachmentsClassLoaderStaticContractTests {
         doReturn("app").whenever(attachment).uploader
         doReturn(emptyList<Party>()).whenever(attachment).signerKeys
         val contractAttachmentId = SecureHash.randomSHA256()
-        doReturn(contractAttachmentId).whenever(attachmentStorage)
+        doReturn(listOf(contractAttachmentId)).whenever(attachmentStorage)
-                .getContractAttachmentWithHighestContractVersion(AttachmentDummyContract.ATTACHMENT_PROGRAM_ID, DEFAULT_CORDAPP_VERSION)
+                .getLatestContractAttachments(AttachmentDummyContract.ATTACHMENT_PROGRAM_ID)
     }
 
     @Test

node/src/main/kotlin/net/corda/node/services/persistence/NodeAttachmentService.kt

@@ -497,14 +497,14 @@ class NodeAttachmentService(
         return it.key to AttachmentIds(signed.singleOrNull(), unsigned.firstOrNull())
     }
 
-    override fun getContractAttachmentWithHighestContractVersion(contractClassName: String, minContractVersion: Int): AttachmentId? {
+    override fun getLatestContractAttachments(contractClassName: String, minContractVersion: Int): List<AttachmentId> {
         val versions: NavigableMap<Version, AttachmentIds> = getContractAttachmentVersions(contractClassName)
-        val newestAttachmentIds = versions.tailMap(minContractVersion, true).lastEntry()?.value
+        val newestAttachmentIds = versions.tailMap(minContractVersion, true)
-        return newestAttachmentIds?.toList()?.first()
+        val newestSignedAttachment = newestAttachmentIds.values.map { it.signed }.lastOrNull { it != null }
-    }
+        val newestUnsignedAttachment = newestAttachmentIds.values.map { it.unsigned }.lastOrNull { it != null }
-
+        return if (newestSignedAttachment != null || newestUnsignedAttachment != null)
-    override fun getContractAttachments(contractClassName: String): Set<AttachmentId> {
+            AttachmentIds(newestSignedAttachment, newestUnsignedAttachment).toList()
-        val versions: NavigableMap<Version, AttachmentIds> = getContractAttachmentVersions(contractClassName)
+        else
-        return versions.values.flatMap { it.toList() }.toSet()
+            emptyList()
     }
 }

node/src/test/kotlin/net/corda/node/services/persistence/NodeAttachmentServiceTest.kt

@@ -13,6 +13,7 @@ import net.corda.core.flows.FlowLogic
 import net.corda.core.internal.*
 import net.corda.core.internal.cordapp.CordappImpl.Companion.DEFAULT_CORDAPP_VERSION
 import net.corda.core.node.ServicesForResolution
+import net.corda.core.node.services.AttachmentId
 import net.corda.core.node.services.vault.AttachmentQueryCriteria.AttachmentsQueryCriteria
 import net.corda.core.node.services.vault.AttachmentSort
 import net.corda.core.node.services.vault.Builder
@@ -585,6 +586,108 @@ class NodeAttachmentServiceTest {
         }
     }
 
+    @Test
+    fun `retrieve latest versions of unsigned and signed contracts - both exist at same version`() {
+        SelfCleaningDir().use { file ->
+            val contractJar = makeTestContractJar(file.path, "com.example.MyContract")
+            val (signedContractJar, publicKey) = makeTestSignedContractJar(file.path, "com.example.MyContract")
+            val contractJarV2 = makeTestContractJar(file.path,"com.example.MyContract", version = 2)
+            val (signedContractJarV2, _) = makeTestSignedContractJar(file.path,"com.example.MyContract", version = 2)
+
+            contractJar.read { storage.privilegedImportAttachment(it, "app", "contract.jar") }
+            signedContractJar.read { storage.privilegedImportAttachment(it, "app", "contract-signed.jar") }
+            var attachmentIdV2Unsigned: AttachmentId? = null
+            contractJarV2.read { attachmentIdV2Unsigned = storage.privilegedImportAttachment(it, "app", "contract-V2.jar") }
+            var attachmentIdV2Signed: AttachmentId? = null
+            signedContractJarV2.read { attachmentIdV2Signed = storage.privilegedImportAttachment(it, "app", "contract-signed-V2.jar") }
+
+            val latestAttachments = storage.getLatestContractAttachments("com.example.MyContract")
+            assertEquals(2, latestAttachments.size)
+            assertEquals(attachmentIdV2Signed, latestAttachments[0])
+            assertEquals(attachmentIdV2Unsigned, latestAttachments[1])
+        }
+    }
+
+    @Test
+    fun `retrieve latest versions of unsigned and signed contracts - signed is later version than unsigned`() {
+        SelfCleaningDir().use { file ->
+            val contractJar = makeTestContractJar(file.path, "com.example.MyContract")
+            val (signedContractJar, publicKey) = makeTestSignedContractJar(file.path, "com.example.MyContract")
+            val contractJarV2 = makeTestContractJar(file.path,"com.example.MyContract", version = 2)
+
+            contractJar.read { storage.privilegedImportAttachment(it, "app", "contract.jar") }
+            var attachmentIdV1Signed: AttachmentId? = null
+            signedContractJar.read { attachmentIdV1Signed = storage.privilegedImportAttachment(it, "app", "contract-signed.jar") }
+            var attachmentIdV2Unsigned: AttachmentId? = null
+            contractJarV2.read { attachmentIdV2Unsigned = storage.privilegedImportAttachment(it, "app", "contract-V2.jar") }
+
+            val latestAttachments = storage.getLatestContractAttachments("com.example.MyContract")
+            assertEquals(2, latestAttachments.size)
+            assertEquals(attachmentIdV1Signed, latestAttachments[0])
+            assertEquals(attachmentIdV2Unsigned, latestAttachments[1])
+        }
+    }
+
+    @Test
+    fun `retrieve latest versions of unsigned and signed contracts - unsigned is later version than signed`() {
+        SelfCleaningDir().use { file ->
+            val contractJar = makeTestContractJar(file.path, "com.example.MyContract")
+            val (signedContractJar, publicKey) = makeTestSignedContractJar(file.path, "com.example.MyContract")
+            val contractJarV2 = makeTestContractJar(file.path,"com.example.MyContract", version = 2)
+
+            contractJar.read { storage.privilegedImportAttachment(it, "app", "contract.jar") }
+            var attachmentIdV1Signed: AttachmentId? = null
+            signedContractJar.read { attachmentIdV1Signed = storage.privilegedImportAttachment(it, "app", "contract-signed.jar") }
+            var attachmentIdV2Unsigned: AttachmentId? = null
+            contractJarV2.read { attachmentIdV2Unsigned = storage.privilegedImportAttachment(it, "app", "contract-V2.jar") }
+
+            val latestAttachments = storage.getLatestContractAttachments("com.example.MyContract")
+            assertEquals(2, latestAttachments.size)
+            assertEquals(attachmentIdV1Signed, latestAttachments[0])
+            assertEquals(attachmentIdV2Unsigned, latestAttachments[1])
+        }
+    }
+
+    @Test
+    fun `retrieve latest versions of unsigned and signed contracts - only signed contracts exist in store`() {
+        SelfCleaningDir().use { file ->
+            val (signedContractJar, publicKey) = makeTestSignedContractJar(file.path, "com.example.MyContract")
+            val (signedContractJarV2, _) = makeTestSignedContractJar(file.path,"com.example.MyContract", version = 2)
+
+            signedContractJar.read { storage.privilegedImportAttachment(it, "app", "contract-signed.jar") }
+            var attachmentIdV2Signed: AttachmentId? = null
+            signedContractJarV2.read { attachmentIdV2Signed = storage.privilegedImportAttachment(it, "app", "contract-signed-V2.jar") }
+
+            val latestAttachments = storage.getLatestContractAttachments("com.example.MyContract")
+            assertEquals(1, latestAttachments.size)
+            assertEquals(attachmentIdV2Signed, latestAttachments[0])
+        }
+    }
+
+    @Test
+    fun `retrieve latest versions of unsigned and signed contracts - only unsigned contracts exist in store`() {
+        SelfCleaningDir().use { file ->
+            val contractJar = makeTestContractJar(file.path, "com.example.MyContract")
+            val contractJarV2 = makeTestContractJar(file.path,"com.example.MyContract", version = 2)
+
+            contractJar.read { storage.privilegedImportAttachment(it, "app", "contract.jar") }
+            var attachmentIdV2Unsigned: AttachmentId? = null
+            contractJarV2.read { attachmentIdV2Unsigned = storage.privilegedImportAttachment(it, "app", "contract-V2.jar") }
+
+            val latestAttachments = storage.getLatestContractAttachments("com.example.MyContract")
+            assertEquals(1, latestAttachments.size)
+            assertEquals(attachmentIdV2Unsigned, latestAttachments[0])
+        }
+    }
+
+    @Test
+    fun `retrieve latest versions of unsigned and signed contracts - none exist in store`() {
+        SelfCleaningDir().use { _ ->
+            val latestAttachments = storage.getLatestContractAttachments("com.example.MyContract")
+            assertEquals(0, latestAttachments.size)
+        }
+    }
+
     // Not the real FetchAttachmentsFlow!
     private class FetchAttachmentsFlow : FlowLogic<Unit>() {
         @Suspendable

testing/test-utils/src/main/kotlin/net/corda/testing/services/MockAttachmentStorage.kt

@@ -44,7 +44,7 @@ class MockAttachmentStorage : AttachmentStorage, SingletonSerializeAsToken() {
 
     override fun openAttachment(id: SecureHash): Attachment? = files[id]?.first
 
-    override fun queryAttachments(criteria: AttachmentQueryCriteria, sorting: AttachmentSort?): List<SecureHash> {
+    override fun queryAttachments(criteria: AttachmentQueryCriteria, sorting: AttachmentSort?): List<AttachmentId> {
         criteria as AttachmentQueryCriteria.AttachmentsQueryCriteria
         val contractClassNames =
                 if (criteria.contractClassNamesCondition is ColumnPredicate.EqualityComparison)
@@ -113,15 +113,10 @@ class MockAttachmentStorage : AttachmentStorage, SingletonSerializeAsToken() {
         return sha256
     }
 
-    override fun getContractAttachmentWithHighestContractVersion(contractClassName: String, minContractVersion: Int): AttachmentId? {
+    override fun getLatestContractAttachments(contractClassName: String, minContractVersion: Int): List<AttachmentId> {
         val attachmentQueryCriteria = AttachmentQueryCriteria.AttachmentsQueryCriteria(contractClassNamesCondition = Builder.equal(listOf(contractClassName)),
                 versionCondition = Builder.greaterThanOrEqual(minContractVersion), uploaderCondition = Builder.`in`(TRUSTED_UPLOADERS))
         val attachmentSort = AttachmentSort(listOf(AttachmentSort.AttachmentSortColumn(AttachmentSort.AttachmentSortAttribute.VERSION, Sort.Direction.DESC)))
-        return queryAttachments(attachmentQueryCriteria, attachmentSort).firstOrNull()
+        return queryAttachments(attachmentQueryCriteria, attachmentSort)
-    }
-
-    override fun getContractAttachments(contractClassName: String): Set<AttachmentId> {
-        val attachmentQueryCriteria = AttachmentQueryCriteria.AttachmentsQueryCriteria(contractClassNamesCondition = Builder.equal(listOf(contractClassName)))
-        return queryAttachments(attachmentQueryCriteria).toSet()
     }
 }