NodeVaultService._queryBy does not preserve requested order from Sort

[adamturski]

In summary, in NodeVaultService._queryBy() method, SQL Query results list elements are mapped to StateRefs and put in Set. Then the Set is used to populate List which is returned in a Vault.Page. This means that if we used any sort on a page, then this can be lost due to mapping to Set.
More simple example what is happening here is when we query vault we translate:
List->Set->List
and this is how we can loose the order.

Line in which problem occurs:

corda/node/src/main/kotlin/net/corda/node/services/vault/NodeVaultService.kt

Line 738 in 149ccdb

statesAndRefs.addAll(uncheckedCast(servicesForResolution.loadStates(stateRefs)))

Code snippet:

    private fun <T : ContractState> _queryBy(criteria: QueryCriteria, paging_: PageSpecification, sorting: Sort, contractStateType: Class<out T>, skipPagingChecks: Boolean): Vault.Page<T> {
...
// Results from DB are in desired order
val results = query.resultList
...
val statesAndRefs: MutableList<StateAndRef<T>> = mutableListOf()
val statesMeta: MutableList<Vault.StateMetadata> = mutableListOf()
val otherResults: MutableList<Any> = mutableListOf()
val stateRefs = mutableSetOf<StateRef>()

// Iterate through results - order is preserved for now
results.asSequence()
                    .forEachIndexed { index, result ->
...
// stateRefs is a Set
stateRefs.add(stateRef) // stateRef is created using Tuple from result item
...
if (stateRefs.isNotEmpty())
// ISSUE is caused by below line, because we populate statesAndRefs using Set meaning that in the end statesAndRefs can loose correct order and additionally loadStates method will modify additionaly the Set - it will group items by tx
                statesAndRefs.addAll(uncheckedCast(servicesForResolution.loadStates(stateRefs)))
...
// Page is returned with statesAndRefs as list
            Vault.Page(states = statesAndRefs, statesMetadata = statesMeta, stateTypes = criteriaParser.stateTypes, totalStatesAvailable = totalStates, otherResults = otherResults)

override fun loadStates(stateRefs: Set<StateRef>): Set<StateAndRef<ContractState>> {
        return stateRefs.groupBy { it.txhash }.flatMap {
            val stx = validatedTransactions.getTransaction(it.key) ?: throw TransactionResolutionException(it.key)
            val baseTx = stx.resolveBaseTransaction(this)
            it.value.map { ref -> StateAndRef(baseTx.outputs[ref.index], ref) }
        }.toSet()
    }

Example case:
TradeState {
linearId: UUID,
status: String,
our_state: Boolean
}

SQL Query results:

1. 33c40565-06b2-4010-b78e-479d0f796860
   AA0E62E56670A1D58A37434B1378424B02DD0AD284B4623FF0FEB2F0CA8C77F0(1)
2. 166dc450-594e-492b-931c-be3a877beae4
   C4566AEFCEF53AE98101D6AEEE9F82E05052A2F8A1B56BE793F39F91A5278568(1)
3. 259bd904-b0cd-481b-932e-e2f1d41dbb4d
   AA0E62E56670A1D58A37434B1378424B02DD0AD284B4623FF0FEB2F0CA8C77F0(0)

Vault.Page - StateAndRefs:

1. 33c40565-06b2-4010-b78e-479d0f796860
   AA0E62E56670A1D58A37434B1378424B02DD0AD284B4623FF0FEB2F0CA8C77F0(1)
2. 259bd904-b0cd-481b-932e-e2f1d41dbb4d
   AA0E62E56670A1D58A37434B1378424B02DD0AD284B4623FF0FEB2F0CA8C77F0(0)
3. 166dc450-594e-492b-931c-be3a877beae4
   C4566AEFCEF53AE98101D6AEEE9F82E05052A2F8A1B56BE793F39F91A5278568(1)

2 last elements were swapped, which is wrong as SQL results were correctly sorted.

SQL query extracted from logs:

select
    tradeState.linear_id as linear_id,
    tradeState.status as status,
    tradeState.our_state as our_state
    vaultschem0_.output_index as output_i1_40_0_,
    vaultschem0_.transaction_id as transact2_40_0_,
    tradeState.output_index as output_i1_35_1_,
    tradeState.transaction_id as transact2_35_1_,
    vaultschem0_.constraint_data as constrai3_40_0_,
    vaultschem0_.constraint_type as constrai4_40_0_,
    vaultschem0_.consumed_timestamp as consumed5_40_0_,
    vaultschem0_.contract_state_class_name as contract6_40_0_,
    vaultschem0_.lock_id as lock_id7_40_0_,
    vaultschem0_.lock_timestamp as lock_tim8_40_0_,
    vaultschem0_.notary_name as notary_n9_40_0_,
    vaultschem0_.recorded_timestamp as recorde10_40_0_,
    vaultschem0_.relevancy_status as relevan11_40_0_,
    vaultschem0_.state_status as state_s12_40_0_
from vault_states vaultschem0_
    cross join trade_state tradeState
where
        vaultschem0_.state_status=0
  and (
        vaultschem0_.contract_state_class_name in (
        'com.test.TradeState'
        )
    )
  and vaultschem0_.relevancy_status=0
  and (
    vaultschem0_.output_index=tradeState.output_index
            and vaultschem0_.transaction_id=tradeState.transaction_id
        or vaultschem0_.output_index=tradeState.output_index
                    and vaultschem0_.transaction_id=tradeState.transaction_id
                    and vaultschem0_.output_index=tradeState.output_index
                    and vaultschem0_.transaction_id=tradeState.transaction_id
                    and (
                   tradeState.status in (
                   'NEW'
                   )
               )
    )
  and vaultschem0_.output_index=tradeState.output_index
  and vaultschem0_.transaction_id=tradeState.transaction_id
order by
    tradeState.status desc,
    tradeState.our_state desc,
    vaultschem0_.output_index,
    vaultschem0_.transaction_id asc
limit 10

From the _queryBy logic it seems that sorting should work randomly, but in reality it works most of the time, except when in returned results there are states from the same transaction.

[adamturski]

I see that Kotlin Sets preserve the order meaning that here the issue is not related to a mapping from List to Set, but to the fact that here:
https://github.com/corda/corda/blob/4.9.2/node/src/main/kotlin/net/corda/node/internal/ServicesForResolutionImpl.kt#L35
we group items by txHash meaning that we can change order how records were retrieved from DB.

Most probably we have to do not invoke groupBy operation on an original Set.