From 054cc2609fb8e4203b07615258bcdaf8f07be6db Mon Sep 17 00:00:00 2001 From: Lohachov Mykhailo Date: Wed, 18 Dec 2024 15:52:51 +0900 Subject: [PATCH] fix: remove sorting Signed-off-by: Lohachov Mykhailo --- crates/iroha/tests/pagination.rs | 3 +- crates/iroha/tests/sorting.rs | 346 ------------------ crates/iroha_core/src/query/store.rs | 4 +- .../src/smartcontracts/isi/query.rs | 148 +------- .../iroha_data_model/src/query/builder/mod.rs | 12 +- .../iroha_data_model/src/query/parameters.rs | 33 +- crates/iroha_schema_gen/src/lib.rs | 2 - .../query_assets_and_save_cursor/src/lib.rs | 1 - 8 files changed, 20 insertions(+), 529 deletions(-) delete mode 100644 crates/iroha/tests/sorting.rs diff --git a/crates/iroha/tests/pagination.rs b/crates/iroha/tests/pagination.rs index fe710d21411..1ca44529164 100644 --- a/crates/iroha/tests/pagination.rs +++ b/crates/iroha/tests/pagination.rs @@ -51,7 +51,7 @@ fn fetch_size_should_work() -> Result<()> { // use the lower-level API to inspect the batch size use iroha::data_model::query::{ builder::QueryExecutor as _, - parameters::{FetchSize, QueryParams, Sorting}, + parameters::{FetchSize, QueryParams}, QueryWithFilter, QueryWithParams, }; @@ -69,7 +69,6 @@ fn fetch_size_should_work() -> Result<()> { .into(), QueryParams::new( Pagination::new(Some(nonzero!(7_u64)), 1), - Sorting::default(), FetchSize::new(Some(nonzero!(3_u64))), ), ); diff --git a/crates/iroha/tests/sorting.rs b/crates/iroha/tests/sorting.rs deleted file mode 100644 index c27c1d5e1d2..00000000000 --- a/crates/iroha/tests/sorting.rs +++ /dev/null @@ -1,346 +0,0 @@ -use std::collections::HashSet; - -use eyre::{Result, WrapErr as _}; -use iroha::{ - client::QueryResult, - crypto::KeyPair, - data_model::{account::Account, name::Name, prelude::*}, -}; -use iroha_test_network::*; -use iroha_test_samples::ALICE_ID; -use nonzero_ext::nonzero; -use rand::{seq::SliceRandom, thread_rng}; - -#[test] -#[allow(clippy::cast_possible_truncation)] -fn correct_pagination_assets_after_creating_new_one() { - // FIXME transaction is rejected for more than a certain number of instructions - const N_ASSETS: usize = 9; - // 0 < pagination.start < missing_idx < pagination.end < N_ASSETS - let missing_indices = vec![N_ASSETS / 2]; - let pagination = Pagination::new(Some(nonzero!(N_ASSETS as u64 / 3)), N_ASSETS as u64 / 3); - let xor_filter = - CompoundPredicate::::build(|asset| asset.id.definition.name.starts_with("xor")); - - let sort_by_metadata_key = "sort".parse::().expect("Valid"); - let sorting = Sorting::by_metadata_key(sort_by_metadata_key.clone()); - let account_id = ALICE_ID.clone(); - - let (network, _rt) = NetworkBuilder::new().start_blocking().unwrap(); - let test_client = network.client(); - - let mut tester_assets = vec![]; - let mut register_asset_definitions = vec![]; - let mut register_assets = vec![]; - - let mut missing_tester_assets = vec![]; - let mut missing_register_asset_definitions = vec![]; - let mut missing_register_assets = vec![]; - - for i in 0..N_ASSETS { - let asset_definition_id = format!("xor{i}#wonderland") - .parse::() - .expect("Valid"); - let asset_definition = AssetDefinition::store(asset_definition_id.clone()); - let mut asset_metadata = Metadata::default(); - asset_metadata.insert(sort_by_metadata_key.clone(), i as u32); - let asset = Asset::new( - AssetId::new(asset_definition_id, account_id.clone()), - AssetValue::Store(asset_metadata), - ); - - if missing_indices.contains(&i) { - missing_tester_assets.push(asset.clone()); - missing_register_asset_definitions.push(Register::asset_definition(asset_definition)); - missing_register_assets.push(Register::asset(asset)); - } else { - tester_assets.push(asset.clone()); - register_asset_definitions.push(Register::asset_definition(asset_definition)); - register_assets.push(Register::asset(asset)); - } - } - register_asset_definitions.shuffle(&mut thread_rng()); - register_assets.shuffle(&mut thread_rng()); - - test_client - .submit_all_blocking(register_asset_definitions) - .expect("Valid"); - test_client - .submit_all_blocking(register_assets) - .expect("Valid"); - - let queried_assets = test_client - .query(FindAssets::new()) - .filter(xor_filter.clone()) - .with_pagination(pagination) - .with_sorting(sorting.clone()) - .execute_all() - .expect("Valid"); - - tester_assets - .iter() - .skip(N_ASSETS / 3) - .take(N_ASSETS / 3) - .zip(queried_assets) - .for_each(|(tester, queried)| assert_eq!(*tester, queried)); - - for (i, missing_idx) in missing_indices.into_iter().enumerate() { - tester_assets.insert(missing_idx, missing_tester_assets[i].clone()); - } - test_client - .submit_all_blocking(missing_register_asset_definitions) - .expect("Valid"); - test_client - .submit_all_blocking(missing_register_assets) - .expect("Valid"); - - let queried_assets = test_client - .query(FindAssets::new()) - .filter(xor_filter) - .with_pagination(pagination) - .with_sorting(sorting) - .execute_all() - .expect("Valid"); - - tester_assets - .iter() - .skip(N_ASSETS / 3) - .take(N_ASSETS / 3) - .zip(queried_assets) - .for_each(|(tester, queried)| assert_eq!(*tester, queried)); -} - -#[test] -#[allow(clippy::too_many_lines)] -fn correct_sorting_of_entities() { - let (network, _rt) = NetworkBuilder::new().start_blocking().unwrap(); - let test_client = network.client(); - - let sort_by_metadata_key = "test_sort".parse::().expect("Valid"); - - // Test sorting asset definitions - - let mut asset_definitions = vec![]; - let mut metadata_of_assets = vec![]; - let mut instructions = vec![]; - let n = 10_u32; - for i in 0..n { - let asset_definition_id = format!("xor_{i}#wonderland") - .parse::() - .expect("Valid"); - let mut asset_metadata = Metadata::default(); - asset_metadata.insert(sort_by_metadata_key.clone(), n - i - 1); - let asset_definition = AssetDefinition::numeric(asset_definition_id.clone()) - .with_metadata(asset_metadata.clone()); - - metadata_of_assets.push(asset_metadata); - asset_definitions.push(asset_definition_id); - - let create_asset_definition = Register::asset_definition(asset_definition); - instructions.push(create_asset_definition); - } - - test_client - .submit_all_blocking(instructions) - .expect("Valid"); - - let res = test_client - .query(FindAssetsDefinitions::new()) - .with_sorting(Sorting::by_metadata_key(sort_by_metadata_key.clone())) - .filter_with(|asset_definition| asset_definition.id.name.starts_with("xor_")) - .execute_all() - .expect("Valid"); - - assert!(res - .iter() - .map(Identifiable::id) - .eq(asset_definitions.iter().rev())); - assert!(res - .iter() - .map(AssetDefinition::metadata) - .eq(metadata_of_assets.iter().rev())); - - // Test sorting accounts - - let domain_name = "_neverland"; - let domain_id: DomainId = domain_name.parse().unwrap(); - test_client - .submit_blocking(Register::domain(Domain::new(domain_id.clone()))) - .expect("should be committed"); - - let mut accounts = vec![]; - let mut metadata_of_accounts = vec![]; - let mut instructions = vec![]; - - let n = 10u32; - let mut public_keys = (0..n) - .map(|_| KeyPair::random().into_parts().0) - .collect::>(); - public_keys.sort_unstable(); - for i in 0..n { - let account_id = AccountId::new(domain_id.clone(), public_keys[i as usize].clone()); - let mut account_metadata = Metadata::default(); - account_metadata.insert(sort_by_metadata_key.clone(), n - i - 1); - let account = Account::new(account_id.clone()).with_metadata(account_metadata.clone()); - - accounts.push(account_id); - metadata_of_accounts.push(account_metadata); - - let create_account = Register::account(account); - instructions.push(create_account); - } - - test_client - .submit_all_blocking(instructions) - .expect("Valid"); - - let res = test_client - .query(FindAccounts::new()) - .with_sorting(Sorting::by_metadata_key(sort_by_metadata_key.clone())) - .filter_with(|account| account.id.domain.eq(domain_id)) - .execute_all() - .expect("Valid"); - - assert!(res.iter().map(Identifiable::id).eq(accounts.iter().rev())); - assert!(res - .iter() - .map(Account::metadata) - .eq(metadata_of_accounts.iter().rev())); - - // Test sorting domains - - let mut domains = vec![]; - let mut metadata_of_domains = vec![]; - let mut instructions = vec![]; - let n = 10u32; - for i in 0..n { - let domain_id = format!("neverland{i}").parse::().expect("Valid"); - let mut domain_metadata = Metadata::default(); - domain_metadata.insert(sort_by_metadata_key.clone(), n - i - 1); - let domain = Domain::new(domain_id.clone()).with_metadata(domain_metadata.clone()); - - domains.push(domain_id); - metadata_of_domains.push(domain_metadata); - - let create_account = Register::domain(domain); - instructions.push(create_account); - } - - test_client - .submit_all_blocking(instructions) - .expect("Valid"); - - let res = test_client - .query(FindDomains::new()) - .with_sorting(Sorting::by_metadata_key(sort_by_metadata_key.clone())) - .filter_with(|domain| domain.id.name.starts_with("neverland")) - .execute_all() - .expect("Valid"); - - assert!(res.iter().map(Identifiable::id).eq(domains.iter().rev())); - assert!(res - .iter() - .map(Domain::metadata) - .eq(metadata_of_domains.iter().rev())); - - // Naive test sorting of domains - let input = [(0_i32, 1_u32), (2, 0), (1, 2)]; - let mut domains = vec![]; - let mut metadata_of_domains = vec![]; - let mut instructions = vec![]; - for (idx, val) in input { - let domain_id = format!("neverland_{idx}") - .parse::() - .expect("Valid"); - let mut domain_metadata = Metadata::default(); - domain_metadata.insert(sort_by_metadata_key.clone(), val); - let domain = Domain::new(domain_id.clone()).with_metadata(domain_metadata.clone()); - - domains.push(domain_id); - metadata_of_domains.push(domain_metadata); - - let create_account = Register::domain(domain); - instructions.push(create_account); - } - test_client - .submit_all_blocking(instructions) - .expect("Valid"); - - let res = test_client - .query(FindDomains::new()) - .with_sorting(Sorting::by_metadata_key(sort_by_metadata_key)) - .filter_with(|domain| domain.id.name.starts_with("neverland_")) - .execute() - .expect("Valid") - .collect::>>() - .expect("Valid"); - - assert_eq!(res[0].id(), &domains[1]); - assert_eq!(res[1].id(), &domains[0]); - assert_eq!(res[2].id(), &domains[2]); - assert_eq!(res[0].metadata(), &metadata_of_domains[1]); - assert_eq!(res[1].metadata(), &metadata_of_domains[0]); - assert_eq!(res[2].metadata(), &metadata_of_domains[2]); -} - -#[test] -fn sort_only_elements_which_have_sorting_key() -> Result<()> { - const TEST_DOMAIN: &str = "neverland"; - - let (network, _rt) = NetworkBuilder::new().start_blocking().unwrap(); - let test_client = network.client(); - - let domain_id: DomainId = TEST_DOMAIN.parse().unwrap(); - test_client - .submit_blocking(Register::domain(Domain::new(domain_id.clone()))) - .expect("should be committed"); - - let sort_by_metadata_key = "test_sort".parse::().expect("Valid"); - - let mut accounts_a = vec![]; - let mut accounts_b = vec![]; - let mut instructions = vec![]; - - let mut skip_set = HashSet::new(); - skip_set.insert(4); - skip_set.insert(7); - - let n = 10u32; - let mut public_keys = (0..n) - .map(|_| KeyPair::random().into_parts().0) - .collect::>(); - public_keys.sort_unstable(); - for i in 0..n { - let account_id = AccountId::new(domain_id.clone(), public_keys[i as usize].clone()); - let account = if skip_set.contains(&i) { - let account = Account::new(account_id.clone()); - accounts_b.push(account_id); - account - } else { - let mut account_metadata = Metadata::default(); - account_metadata.insert(sort_by_metadata_key.clone(), n - i - 1); - let account = Account::new(account_id.clone()).with_metadata(account_metadata); - accounts_a.push(account_id); - account - }; - - let create_account = Register::account(account); - instructions.push(create_account); - } - - test_client - .submit_all_blocking(instructions) - .wrap_err("Failed to register accounts")?; - - let res = test_client - .query(FindAccounts::new()) - .with_sorting(Sorting::by_metadata_key(sort_by_metadata_key)) - .filter_with(|account| account.id.domain.eq(domain_id)) - .execute_all() - .wrap_err("Failed to submit request")?; - - let accounts = accounts_a.iter().rev().chain(accounts_b.iter()); - assert!(res.iter().map(Identifiable::id).eq(accounts)); - - Ok(()) -} diff --git a/crates/iroha_core/src/query/store.rs b/crates/iroha_core/src/query/store.rs index 98078ecac0b..721202d9dab 100644 --- a/crates/iroha_core/src/query/store.rs +++ b/crates/iroha_core/src/query/store.rs @@ -279,7 +279,7 @@ mod tests { use iroha_data_model::{ permission::Permission, prelude::SelectorTuple, - query::parameters::{FetchSize, Pagination, QueryParams, Sorting}, + query::parameters::{FetchSize, Pagination, QueryParams}, }; use iroha_primitives::json::Json; use iroha_test_samples::ALICE_ID; @@ -297,11 +297,9 @@ mod tests { let fetch_size = FetchSize { fetch_size: Some(nonzero!(1_u64)), }; - let sorting = Sorting::default(); let query_params = QueryParams { pagination, - sorting, fetch_size, }; diff --git a/crates/iroha_core/src/smartcontracts/isi/query.rs b/crates/iroha_core/src/smartcontracts/isi/query.rs index d2e4929aa2f..7b588a2d823 100644 --- a/crates/iroha_core/src/smartcontracts/isi/query.rs +++ b/crates/iroha_core/src/smartcontracts/isi/query.rs @@ -1,6 +1,5 @@ //! Query functionality. The common error type is also defined here, //! alongside functions for converting them into HTTP responses. -use std::cmp::Ordering; use eyre::Result; use iroha_data_model::{ @@ -9,8 +8,8 @@ use iroha_data_model::{ dsl::{EvaluateSelector, HasProjection, SelectorMarker}, error::QueryExecutionFail as Error, parameters::QueryParams, - CommittedTransaction, QueryBox, QueryOutputBatchBox, QueryRequest, - QueryRequestWithAuthority, QueryResponse, SingularQueryBox, SingularQueryOutputBox, + QueryBox, QueryOutputBatchBox, QueryRequest, QueryRequestWithAuthority, QueryResponse, + SingularQueryBox, SingularQueryOutputBox, }, }; @@ -21,96 +20,7 @@ use crate::{ state::{StateReadOnly, WorldReadOnly}, }; -/// Allows to generalize retrieving the metadata key for all the query output types -pub trait SortableQueryOutput { - /// Get the sorting key for the output, from metadata - /// - /// If the type doesn't have metadata or metadata key doesn't exist - return None - fn get_metadata_sorting_key(&self, key: &Name) -> Option; -} - -impl SortableQueryOutput for Account { - fn get_metadata_sorting_key(&self, key: &Name) -> Option { - self.metadata.get(key).cloned() - } -} - -impl SortableQueryOutput for Domain { - fn get_metadata_sorting_key(&self, key: &Name) -> Option { - self.metadata.get(key).cloned() - } -} - -impl SortableQueryOutput for AssetDefinition { - fn get_metadata_sorting_key(&self, key: &Name) -> Option { - self.metadata.get(key).cloned() - } -} - -impl SortableQueryOutput for Asset { - fn get_metadata_sorting_key(&self, key: &Name) -> Option { - match &self.value { - AssetValue::Numeric(_) => None, - AssetValue::Store(metadata) => metadata.get(key).cloned(), - } - } -} - -impl SortableQueryOutput for Role { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for RoleId { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for CommittedTransaction { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for PeerId { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for Permission { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for Trigger { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for TriggerId { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for iroha_data_model::block::SignedBlock { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -impl SortableQueryOutput for iroha_data_model::block::BlockHeader { - fn get_metadata_sorting_key(&self, _key: &Name) -> Option { - None - } -} - -/// Applies sorting and pagination to the query output and wraps it into a type-erasing batching iterator. +/// Applies pagination to the query output and wraps it into a type-erasing batching iterator. /// /// # Errors /// @@ -120,12 +30,11 @@ pub fn apply_query_postprocessing( selector: SelectorTuple, &QueryParams { pagination, - ref sorting, fetch_size, }: &QueryParams, ) -> Result where - I: Iterator, + I: Iterator, I::Item: HasProjection + 'static, >::Projection: EvaluateSelector + Send + Sync, QueryOutputBatchBox: From>, @@ -138,43 +47,18 @@ where return Err(Error::FetchSizeTooBig); } - // sort & paginate, erase the iterator with QueryBatchedErasedIterator - let output = if let Some(key) = &sorting.sort_by_metadata_key { - // if sorting was requested, we need to retrieve all the results first - let mut pairs: Vec<(Option, I::Item)> = iter - .map(|value| { - let key = value.get_metadata_sorting_key(key); - (key, value) - }) - .collect(); - pairs.sort_by( - |(left_key, _), (right_key, _)| match (left_key, right_key) { - (Some(l), Some(r)) => l.cmp(r), - (Some(_), None) => Ordering::Less, - (None, Some(_)) => Ordering::Greater, - (None, None) => Ordering::Equal, - }, - ); - - ErasedQueryIterator::new( - pairs.into_iter().map(|(_, val)| val).paginate(pagination), - selector, - fetch_size, - ) - } else { - // FP: this collect is very deliberate - #[allow(clippy::needless_collect)] - let output = iter - .paginate(pagination) - // it should theoretically be possible to not collect the results into a vec and build the response lazily - // but: - // - the iterator is bound to the 'state lifetime and this lifetime should somehow be erased - // - for small queries this might not be efficient - // TODO: investigate this - .collect::>(); - - ErasedQueryIterator::new(output.into_iter(), selector, fetch_size) - }; + // FP: this collect is very deliberate + #[allow(clippy::needless_collect)] + let output = iter + .paginate(pagination) + // it should theoretically be possible to not collect the results into a vec and build the response lazily + // but: + // - the iterator is bound to the 'state lifetime and this lifetime should somehow be erased + // - for small queries this might not be efficient + // TODO: investigate this + .collect::>(); + + let output = ErasedQueryIterator::new(output.into_iter(), selector, fetch_size); Ok(output) } diff --git a/crates/iroha_data_model/src/query/builder/mod.rs b/crates/iroha_data_model/src/query/builder/mod.rs index 5a29abb4ba1..91d54f70e3c 100644 --- a/crates/iroha_data_model/src/query/builder/mod.rs +++ b/crates/iroha_data_model/src/query/builder/mod.rs @@ -18,7 +18,7 @@ use crate::query::{ BaseProjector, CompoundPredicate, HasPrototype, IntoSelectorTuple, PredicateMarker, SelectorMarker, SelectorTuple, }, - parameters::{FetchSize, Pagination, QueryParams, Sorting}, + parameters::{FetchSize, Pagination, QueryParams}, Query, QueryBox, QueryOutputBatchBoxTuple, QueryWithFilter, QueryWithParams, SingularQueryBox, SingularQueryOutputBox, }; @@ -108,7 +108,6 @@ where filter: CompoundPredicate, selector: SelectorTuple, pagination: Pagination, - sorting: Sorting, fetch_size: FetchSize, // NOTE: T is a phantom type used to denote the selected tuple in `selector` phantom: PhantomData, @@ -126,7 +125,6 @@ where filter: CompoundPredicate::PASS, selector: SelectorTuple::default(), pagination: Pagination::default(), - sorting: Sorting::default(), fetch_size: FetchSize::default(), phantom: PhantomData, } @@ -196,18 +194,11 @@ where filter: self.filter, selector: new_selector, pagination: self.pagination, - sorting: self.sorting, fetch_size: self.fetch_size, phantom: PhantomData, } } - /// Sort the results according to the specified sorting. - #[must_use] - pub fn with_sorting(self, sorting: Sorting) -> Self { - Self { sorting, ..self } - } - /// Only return part of the results specified by the pagination. #[must_use] pub fn with_pagination(self, pagination: Pagination) -> Self { @@ -243,7 +234,6 @@ where query: boxed, params: QueryParams { pagination: self.pagination, - sorting: self.sorting, fetch_size: self.fetch_size, }, }; diff --git a/crates/iroha_data_model/src/query/parameters.rs b/crates/iroha_data_model/src/query/parameters.rs index 249759cd0cd..e2d36c7107f 100644 --- a/crates/iroha_data_model/src/query/parameters.rs +++ b/crates/iroha_data_model/src/query/parameters.rs @@ -12,8 +12,6 @@ use iroha_version::{Decode, Encode}; use nonzero_ext::nonzero; use serde::{Deserialize, Serialize}; -use crate::name::Name; - /// Default value for `fetch_size` parameter in queries. pub const DEFAULT_FETCH_SIZE: NonZeroU64 = nonzero!(100_u64); /// Max value for `fetch_size` parameter in queries. @@ -69,25 +67,6 @@ mod model { pub offset: u64, } - /// Struct for sorting requests - #[derive( - Debug, - Clone, - Default, - PartialEq, - Eq, - Decode, - Encode, - Deserialize, - Serialize, - IntoSchema, - Constructor, - )] - pub struct Sorting { - /// Sort query result using [`Name`] of the key in [`Asset`]'s metadata. - pub sort_by_metadata_key: Option, - } - /// Structure for query fetch size parameter encoding/decoding #[derive( Debug, @@ -126,21 +105,11 @@ mod model { )] pub struct QueryParams { pub pagination: Pagination, - pub sorting: Sorting, pub fetch_size: FetchSize, } } -impl Sorting { - /// Creates a sorting by [`Name`] of the key. - pub fn by_metadata_key(key: Name) -> Self { - Self { - sort_by_metadata_key: Some(key), - } - } -} - pub mod prelude { //! Prelude: re-export most commonly used traits, structs and macros from this module. - pub use super::{FetchSize, Pagination, Sorting}; + pub use super::{FetchSize, Pagination}; } diff --git a/crates/iroha_schema_gen/src/lib.rs b/crates/iroha_schema_gen/src/lib.rs index cf176edf1c7..9d504afce6f 100644 --- a/crates/iroha_schema_gen/src/lib.rs +++ b/crates/iroha_schema_gen/src/lib.rs @@ -350,7 +350,6 @@ types!( Option>, Option>, Option, - Option, Option, Option, Option>, @@ -488,7 +487,6 @@ types!( SocketAddrHost, SocketAddrV4, SocketAddrV6, - Sorting, String, StringPredicateAtom, SumeragiParameter, diff --git a/wasm/samples/query_assets_and_save_cursor/src/lib.rs b/wasm/samples/query_assets_and_save_cursor/src/lib.rs index 576b46e9948..46faa4921f2 100644 --- a/wasm/samples/query_assets_and_save_cursor/src/lib.rs +++ b/wasm/samples/query_assets_and_save_cursor/src/lib.rs @@ -39,7 +39,6 @@ fn main(host: Iroha, context: Context) { ) .into(), QueryParams::new( - Default::default(), Default::default(), FetchSize::new(Some(nonzero!(1_u64))), ),