From 5d9ef41c317101ef9f0002578b72a41a1b3d564c Mon Sep 17 00:00:00 2001 From: Sayan Nandan Date: Sat, 26 Feb 2022 07:42:07 -0800 Subject: [PATCH] Fix length checks Also improve panic messages in `dbtest` tests. --- server/src/actions/get.rs | 2 +- server/src/actions/lists/lget.rs | 2 +- server/src/actions/lists/lmod.rs | 2 +- server/src/actions/lists/mod.rs | 2 +- server/src/corestore/mod.rs | 19 ++++++++++++++++++- server/src/corestore/table.rs | 2 +- server/src/queryengine/ddl.rs | 8 ++++---- server/src/queryengine/mod.rs | 29 +++++++++++++---------------- sky-macros/src/lib.rs | 19 +++++++++++++++++++ 9 files changed, 59 insertions(+), 26 deletions(-) diff --git a/server/src/actions/get.rs b/server/src/actions/get.rs index ae80822d..9396405d 100644 --- a/server/src/actions/get.rs +++ b/server/src/actions/get.rs @@ -34,7 +34,7 @@ use crate::util::compiler; action!( /// Run a `GET` query fn get(handle: &crate::corestore::Corestore, con: &mut T, mut act: ActionIter<'a>) { - ensure_length(act.len(), |len| len == 0)?; + ensure_length(act.len(), |len| len == 1)?; let kve = handle.get_table_with::()?; unsafe { match kve.get_cloned_with_tsymbol(act.next_unchecked()) { diff --git a/server/src/actions/lists/lget.rs b/server/src/actions/lists/lget.rs index e17b61fb..5f8df99a 100644 --- a/server/src/actions/lists/lget.rs +++ b/server/src/actions/lists/lget.rs @@ -67,7 +67,7 @@ action! { /// - `LGET LAST` will return the last item /// if it exists fn lget(handle: &Corestore, con: &mut T, mut act: ActionIter<'a>) { - ensure_length(act.len(), |len| len > 1)?; + ensure_length(act.len(), |len| len != 0)?; let listmap = handle.get_table_with::()?; // get the list name let listname = unsafe { act.next_unchecked() }; diff --git a/server/src/actions/lists/lmod.rs b/server/src/actions/lists/lmod.rs index e2debdf5..222a547e 100644 --- a/server/src/actions/lists/lmod.rs +++ b/server/src/actions/lists/lmod.rs @@ -46,7 +46,7 @@ action! { /// - `LMOD remove ` /// - `LMOD clear` fn lmod(handle: &Corestore, con: &mut T, mut act: ActionIter<'a>) { - ensure_length(act.len(), |len| len > 2)?; + ensure_length(act.len(), |len| len > 1)?; let listmap = handle.get_table_with::()?; // get the list name let listname = unsafe { act.next_unchecked() }; diff --git a/server/src/actions/lists/mod.rs b/server/src/actions/lists/mod.rs index e3dcb23c..4a5b1064 100644 --- a/server/src/actions/lists/mod.rs +++ b/server/src/actions/lists/mod.rs @@ -46,7 +46,7 @@ action! { /// Handle an `LSET` query for the list model /// Syntax: `LSET ` fn lset(handle: &Corestore, con: &mut T, mut act: ActionIter<'a>) { - ensure_length(act.len(), |len| len > 1)?; + ensure_length(act.len(), |len| len > 0)?; let listmap = handle.get_table_with::()?; let listname = unsafe { act.next_unchecked_bytes() }; let list = listmap.kve_inner_ref(); diff --git a/server/src/corestore/mod.rs b/server/src/corestore/mod.rs index 9f88c9b3..3148b30e 100644 --- a/server/src/corestore/mod.rs +++ b/server/src/corestore/mod.rs @@ -41,6 +41,7 @@ use crate::storage::sengine::SnapshotEngine; use crate::util::Unwrappable; use crate::IoResult; use core::borrow::Borrow; +use core::fmt; use core::hash::Hash; pub use htable::Data; use std::sync::Arc; @@ -67,13 +68,29 @@ pub type OwnedEntityGroup = OptionTuple; /// A raw borrowed entity (not the struct, but in a tuple form) type BorrowedEntityGroupRaw<'a> = OptionTuple<&'a [u8]>; -#[derive(Debug, PartialEq)] +#[derive(PartialEq)] /// An entity group borrowed from a byte slice pub struct BorrowedEntityGroup<'a> { va: Option<&'a [u8]>, vb: Option<&'a [u8]>, } +impl<'a> fmt::Debug for BorrowedEntityGroup<'a> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fn write_if_some(v: Option<&'_ [u8]>) -> String { + if let Some(v) = v { + format!("{:?}", String::from_utf8_lossy(&v)) + } else { + "None".to_owned() + } + } + f.debug_struct("BorrowedEntityGroup") + .field("va", &write_if_some(self.va)) + .field("vb", &write_if_some(self.vb)) + .finish() + } +} + impl<'a> BorrowedEntityGroup<'a> { pub unsafe fn into_owned(self) -> OwnedEntityGroup { match self { diff --git a/server/src/corestore/table.rs b/server/src/corestore/table.rs index 9136caec..16ff5da0 100644 --- a/server/src/corestore/table.rs +++ b/server/src/corestore/table.rs @@ -48,7 +48,7 @@ pub trait DescribeTable { None => util::err(groups::WRONG_MODEL), } } - _ => util::err(groups::DEFAULT_UNSET), + None => util::err(groups::DEFAULT_UNSET), } } } diff --git a/server/src/queryengine/ddl.rs b/server/src/queryengine/ddl.rs index a880080c..65981a36 100644 --- a/server/src/queryengine/ddl.rs +++ b/server/src/queryengine/ddl.rs @@ -43,7 +43,7 @@ action! { /// like queries fn create(handle: &Corestore, con: &'a mut T, mut act: ActionIter<'a>) { // minlength is 2 (create has already been checked) - ensure_length(act.len(), |size| size > 2)?; + ensure_length(act.len(), |size| size > 1)?; let mut create_what = unsafe { act.next().unsafe_unwrap() }.to_vec(); create_what.make_ascii_uppercase(); match create_what.as_ref() { @@ -61,7 +61,7 @@ action! { /// like queries fn ddl_drop(handle: &Corestore, con: &'a mut T, mut act: ActionIter<'a>) { // minlength is 2 (create has already been checked) - ensure_length(act.len(), |size| size > 2)?; + ensure_length(act.len(), |size| size > 1)?; let mut create_what = unsafe { act.next().unsafe_unwrap() }.to_vec(); create_what.make_ascii_uppercase(); match create_what.as_ref() { @@ -75,9 +75,9 @@ action! { Ok(()) } - /// We should have ` (args)` + /// We should have ` (args) properties` fn create_table(handle: &Corestore, con: &'a mut T, mut act: ActionIter<'a>) { - ensure_length(act.len(), |size| size > 2 && size < 3)?; + ensure_length(act.len(), |size| size > 1 && size < 4)?; let (table_entity, model_code) = parser::parse_table_args(&mut act)?; let is_volatile = match act.next() { Some(maybe_volatile) => { diff --git a/server/src/queryengine/mod.rs b/server/src/queryengine/mod.rs index 1f8b2b82..68debfb6 100644 --- a/server/src/queryengine/mod.rs +++ b/server/src/queryengine/mod.rs @@ -71,23 +71,20 @@ macro_rules! gen_constants_and_matches { macro_rules! swap_entity { ($con:expr, $handle:expr, $entity:expr) => { - match parser::get_query_entity(&$entity) { - Ok(e) => match $handle.swap_entity(e) { - Ok(()) => $con.write_response(responses::groups::OKAY).await?, - Err(DdlError::ObjectNotFound) => { - $con.write_response(responses::groups::CONTAINER_NOT_FOUND) - .await? - } - Err(DdlError::DefaultNotFound) => { - $con.write_response(responses::groups::DEFAULT_UNSET) - .await? - } - Err(_) => unsafe { - // we know Corestore::swap_entity doesn't return anything else - impossible!() - }, + match $handle.swap_entity(parser::get_query_entity(&$entity)?) { + Ok(()) => $con.write_response(responses::groups::OKAY).await?, + Err(DdlError::ObjectNotFound) => { + $con.write_response(responses::groups::CONTAINER_NOT_FOUND) + .await? + } + Err(DdlError::DefaultNotFound) => { + $con.write_response(responses::groups::DEFAULT_UNSET) + .await? + } + Err(_) => unsafe { + // we know Corestore::swap_entity doesn't return anything else + impossible!() }, - Err(e) => $con.write_response(e).await?, } }; } diff --git a/sky-macros/src/lib.rs b/sky-macros/src/lib.rs index e23d7157..4e7636f5 100644 --- a/sky-macros/src/lib.rs +++ b/sky-macros/src/lib.rs @@ -84,10 +84,23 @@ fn parse_dbtest( con.run_simple_query( &skytable::query!("create", "keyspace", "testsuite") ).await.unwrap(); + if !( + __create_ks == skytable::Element::RespCode(skytable::RespCode::Okay) || + __create_ks == skytable::Element::RespCode( + skytable::RespCode::ErrorString( + skytable::error::errorstring::ERR_ALREADY_EXISTS.to_owned() + ) + ) + ) { + panic!("Failed to create keyspace: {:?}", __create_ks); + } let __switch_ks = con.run_simple_query( &skytable::query!("use", "testsuite") ).await.unwrap(); + if (__switch_ks != skytable::Element::RespCode(skytable::RespCode::Okay)) { + panic!("Failed to switch keyspace: {:?}", __switch_ks); + } let __create_tbl = con.run_simple_query( &skytable::query!( @@ -98,6 +111,9 @@ fn parse_dbtest( "volatile" ) ).await.unwrap(); + assert_eq!( + __create_tbl, skytable::Element::RespCode(skytable::RespCode::Okay), "Failed to create table" + ); let mut __concat_entity = std::string::String::new(); __concat_entity.push_str("testsuite:"); __concat_entity.push_str(&#rand_string); @@ -106,6 +122,9 @@ fn parse_dbtest( con.run_simple_query( &skytable::query!("use", __concat_entity) ).await.unwrap(); + assert_eq!( + __switch_entity, skytable::Element::RespCode(skytable::RespCode::Okay), "Failed to switch" + ); let mut query = skytable::Query::new(); #body {