Repository: gluesql/gluesql
Pull request #231 (2021-06-07 06:38:08)

Add index expression validator
`CREATE INDEX` now validate index expression before it calls `Storage -> create_index` storage API.

Not only `expr`s which are not supported, but also `expr`s without identifier used are also not accepted.

e.g.
`CREATE INDEX test ON TableA (10 + 20)` -> error!
`(10 + 20)` is just a literal without using any identifier in the table (TableA).



Patch:
diff --git a/src/executor/alter/alter_table.rs b/src/executor/alter/alter_table.rs
index 7d548015..a52f11fe 100644
--- a/src/executor/alter/alter_table.rs
+++ b/src/executor/alter/alter_table.rs
@@ -1,3 +1,5 @@
+#![cfg(feature = "alter-table")]
+
 use {
     super::validate,
     crate::{
diff --git a/src/executor/alter/drop.rs b/src/executor/alter/drop.rs
deleted file mode 100644
index 8dfb7f84..00000000
--- a/src/executor/alter/drop.rs
+++ /dev/null
@@ -1,42 +0,0 @@
-use {
-    super::AlterError,
-    crate::{
-        ast::ObjectName,
-        data::get_name,
-        result::MutResult,
-        store::{GStore, GStoreMut},
-    },
-    futures::stream::{self, TryStreamExt},
-    std::fmt::Debug,
-};
-
-pub async fn drop<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
-    storage: U,
-    names: &[ObjectName],
-    if_exists: bool,
-) -> MutResult<U, ()> {
-    stream::iter(names.iter().map(Ok))
-        .try_fold((storage, ()), |(storage, _), table_name| async move {
-            let schema = (|| async {
-                let table_name = get_name(table_name)?;
-                let schema = storage.fetch_schema(table_name).await?;
-
-                if !if_exists {
-                    schema.ok_or_else(|| AlterError::TableNotFound(table_name.to_owned()))?;
-                }
-
-                Ok(table_name)
-            })()
-            .await;
-
-            let schema = match schema {
-                Ok(s) => s,
-                Err(e) => {
-                    return Err((storage, e));
-                }
-            };
-
-            storage.delete_schema(schema).await
-        })
-        .await
-}
diff --git a/src/executor/alter/error.rs b/src/executor/alter/error.rs
index 79e186e0..33bd577b 100644
--- a/src/executor/alter/error.rs
+++ b/src/executor/alter/error.rs
@@ -6,11 +6,18 @@ pub enum AlterError {
     #[error("table already exists: {0}")]
     TableAlreadyExists(String),
 
-    // DROP
+    // CREATE INDEX, DROP TABLE
     #[error("table does not exist: {0}")]
     TableNotFound(String),
 
     // validate column def
     #[error("column '{0}' of data type '{1}' is unsupported for unique constraint")]
     UnsupportedDataTypeForUniqueColumn(String, String),
+
+    // validate index expr
+    #[error("unsupported index expr: {0}")]
+    UnsupportedIndexExpr(String),
+
+    #[error("identifier not found: {0}")]
+    IdentifierNotFound(String),
 }
diff --git a/src/executor/alter/index.rs b/src/executor/alter/index.rs
new file mode 100644
index 00000000..63192909
--- /dev/null
+++ b/src/executor/alter/index.rs
@@ -0,0 +1,92 @@
+#![cfg(feature = "index")]
+
+use {
+    super::AlterError,
+    crate::{
+        ast::{ColumnDef, Expr, ObjectName},
+        data::{get_name, Schema},
+        result::MutResult,
+        store::{GStore, GStoreMut},
+    },
+    std::fmt::Debug,
+};
+
+pub async fn create_index<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
+    storage: U,
+    table_name: &ObjectName,
+    index_name: &ObjectName,
+    expr: &Expr,
+) -> MutResult<U, ()> {
+    let names = (|| async {
+        let table_name = get_name(table_name)?;
+        let index_name = get_name(index_name)?;
+        let Schema { column_defs, .. } = storage
+            .fetch_schema(table_name)
+            .await?
+            .ok_or_else(|| AlterError::TableNotFound(table_name.to_owned()))?;
+        let columns = column_defs
+            .into_iter()
+            .map(|ColumnDef { name, .. }| name)
+            .collect::<Vec<_>>();
+
+        let (valid, has_ident) = validate_index_expr(&columns, expr);
+        if !valid {
+            return Err(AlterError::UnsupportedIndexExpr(format!("{:#?}", expr)).into());
+        } else if !has_ident {
+            return Err(AlterError::IdentifierNotFound(format!("{:#?}", expr)).into());
+        }
+
+        Ok((table_name, index_name))
+    })()
+    .await;
+
+    let (table_name, index_name) = match names {
+        Ok(s) => s,
+        Err(e) => {
+            return Err((storage, e));
+        }
+    };
+
+    storage.create_index(table_name, index_name, expr).await
+}
+
+fn validate_index_expr(columns: &[String], expr: &Expr) -> (bool, bool) {
+    let validate = |expr| validate_index_expr(columns, expr);
+
+    match expr {
+        Expr::Identifier(ident) => (columns.iter().any(|column| column == ident), true),
+        Expr::Literal(_) | Expr::TypedString { .. } => (true, false),
+        Expr::Nested(expr) => validate(expr),
+        Expr::BinaryOp { left, right, .. } => {
+            let (valid_l, has_ident_l) = validate(left);
+            let (valid_r, has_ident_r) = validate(right);
+
+            (valid_l && valid_r, has_ident_l || has_ident_r)
+        }
+        Expr::UnaryOp { expr, .. } => validate(expr),
+        Expr::Cast { expr, .. } => validate(expr),
+        _ => (false, false),
+    }
+}
+
+pub async fn drop_index<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
+    storage: U,
+    table_name: &ObjectName,
+    index_name: &ObjectName,
+) -> MutResult<U, ()> {
+    let names = (|| {
+        let table_name = get_name(table_name)?;
+        let index_name = get_name(index_name)?;
+
+        Ok((table_name, index_name))
+    })();
+
+    let (table_name, index_name) = match names {
+        Ok(s) => s,
+        Err(e) => {
+            return Err((storage, e));
+        }
+    };
+
+    storage.drop_index(table_name, index_name).await
+}
diff --git a/src/executor/alter/mod.rs b/src/executor/alter/mod.rs
index 1b694910..7f799fc9 100644
--- a/src/executor/alter/mod.rs
+++ b/src/executor/alter/mod.rs
@@ -1,14 +1,14 @@
-#[cfg(feature = "alter-table")]
 mod alter_table;
-mod create_table;
-mod drop;
 mod error;
+mod index;
+mod table;
 mod validate;
 
 use validate::validate;
 
 #[cfg(feature = "alter-table")]
 pub use alter_table::alter_table;
-pub use create_table::create_table;
-pub use drop::drop;
 pub use error::AlterError;
+#[cfg(feature = "index")]
+pub use index::{create_index, drop_index};
+pub use table::{create_table, drop_table};
diff --git a/src/executor/alter/create_table.rs b/src/executor/alter/table.rs
similarity index 58%
rename from src/executor/alter/create_table.rs
rename to src/executor/alter/table.rs
index 1812f4c3..0d35e31c 100644
--- a/src/executor/alter/create_table.rs
+++ b/src/executor/alter/table.rs
@@ -6,6 +6,7 @@ use {
         result::MutResult,
         store::{GStore, GStoreMut},
     },
+    futures::stream::{self, TryStreamExt},
     std::fmt::Debug,
 };
 
@@ -52,3 +53,34 @@ pub async fn create_table<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
         Ok((storage, ()))
     }
 }
+
+pub async fn drop_table<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
+    storage: U,
+    table_names: &[ObjectName],
+    if_exists: bool,
+) -> MutResult<U, ()> {
+    stream::iter(table_names.iter().map(Ok))
+        .try_fold((storage, ()), |(storage, _), table_name| async move {
+            let schema = (|| async {
+                let table_name = get_name(table_name)?;
+                let schema = storage.fetch_schema(table_name).await?;
+
+                if !if_exists {
+                    schema.ok_or_else(|| AlterError::TableNotFound(table_name.to_owned()))?;
+                }
+
+                Ok(table_name)
+            })()
+            .await;
+
+            let schema = match schema {
+                Ok(s) => s,
+                Err(e) => {
+                    return Err((storage, e));
+                }
+            };
+
+            storage.delete_schema(schema).await
+        })
+        .await
+}
diff --git a/src/executor/evaluate/stateless.rs b/src/executor/evaluate/stateless.rs
index 050b4ac1..d53260a4 100644
--- a/src/executor/evaluate/stateless.rs
+++ b/src/executor/evaluate/stateless.rs
@@ -105,6 +105,7 @@ pub fn evaluate_stateless<'a>(
         Expr::Wildcard | Expr::QualifiedWildcard(_) => {
             Err(EvaluateError::UnreachableWildcardExpr.into())
         }
+        // TODO: Add test - UnsupportedStatelessExpr error
         _ => Err(EvaluateError::UnsupportedStatelessExpr(format!("{:#?}", expr)).into()),
     }
 }
diff --git a/src/executor/execute.rs b/src/executor/execute.rs
index 1293d9f4..b53588a0 100644
--- a/src/executor/execute.rs
+++ b/src/executor/execute.rs
@@ -1,6 +1,6 @@
 use {
     super::{
-        alter::{create_table, drop},
+        alter::{create_table, drop_table},
         fetch::{fetch, fetch_columns},
         select::{select, select_with_labels},
         update::Update,
@@ -21,6 +21,9 @@ use {
 #[cfg(feature = "alter-table")]
 use super::alter::alter_table;
 
+#[cfg(feature = "index")]
+use super::alter::{create_index, drop_index};
+
 #[derive(ThisError, Serialize, Debug, PartialEq)]
 pub enum ExecuteError {
     #[error("table not found: {0}")]
@@ -77,7 +80,7 @@ pub async fn execute<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
             .map(|(storage, _)| (storage, Payload::Create)),
         Statement::DropTable {
             names, if_exists, ..
-        } => drop(storage, names, *if_exists)
+        } => drop_table(storage, names, *if_exists)
             .await
             .map(|(storage, _)| (storage, Payload::DropTable)),
         #[cfg(feature = "alter-table")]
@@ -89,33 +92,13 @@ pub async fn execute<T: 'static + Debug, U: GStore<T> + GStoreMut<T>>(
             name,
             table_name,
             column,
-        } => {
-            let (table_name, index_name) = try_block!(storage, {
-                let index_name = get_name(name)?;
-                let table_name = get_name(table_name)?;
-
-                Ok((table_name, index_name))
-            });
-
-            storage
-                .create_index(table_name, index_name, column)
-                .await
-                .map(|(storage, _)| (storage, Payload::CreateIndex))
-        }
+        } => create_index(storage, table_name, name, column)
+            .await
+            .map(|(storage, _)| (storage, Payload::CreateIndex)),
         #[cfg(feature = "index")]
-        Statement::DropIndex { name, table_name } => {
-            let (table_name, index_name) = try_block!(storage, {
-                let index_name = get_name(name)?;
-                let table_name = get_name(table_name)?;
-
-                Ok((table_name, index_name))
-            });
-
-            storage
-                .drop_index(table_name, index_name)
-                .await
-                .map(|(storage, _)| (storage, Payload::DropIndex))
-        }
+        Statement::DropIndex { name, table_name } => drop_index(storage, table_name, name)
+            .await
+            .map(|(storage, _)| (storage, Payload::DropIndex)),
         //-- Rows
         Statement::Insert {
             table_name,
diff --git a/src/plan.rs b/src/plan.rs
index d296234e..cd6dbe83 100644
--- a/src/plan.rs
+++ b/src/plan.rs
@@ -305,6 +305,10 @@ fn search_index_op(
             index_value_expr: *left,
             selection: None,
         }
+    } else if let Expr::Nested(left) = *left {
+        search_index_op(indexes, index_op, left, right)
+    } else if let Expr::Nested(right) = *right {
+        search_index_op(indexes, index_op, left, right)
     } else {
         Searched::NotFound(Expr::BinaryOp {
             left,
diff --git a/src/storages/sled_storage/index_mut.rs b/src/storages/sled_storage/index_mut.rs
index ebbf5bba..c58b7495 100644
--- a/src/storages/sled_storage/index_mut.rs
+++ b/src/storages/sled_storage/index_mut.rs
@@ -70,7 +70,7 @@ impl IndexMut<IVec> for SledStorage {
             ..
         } = try_into!(
             self,
-            schema.ok_or_else(|| IndexError::TableNotFound(table_name.to_owned()))
+            schema.ok_or_else(|| IndexError::ConflictTableNotFound(table_name.to_owned()))
         );
 
         if indexes.iter().any(|(name, _)| name == index_name) {
diff --git a/src/storages/sled_storage/index_sync.rs b/src/storages/sled_storage/index_sync.rs
index e55b08f2..e627169a 100644
--- a/src/storages/sled_storage/index_sync.rs
+++ b/src/storages/sled_storage/index_sync.rs
@@ -49,7 +49,7 @@ impl<'a> IndexSync<'a> {
             column_defs,
             indexes,
             ..
-        } = schema.ok_or_else(|| IndexError::TableNotFound(table_name.to_owned()))?;
+        } = schema.ok_or_else(|| IndexError::ConflictTableNotFound(table_name.to_owned()))?;
 
         let columns = column_defs
             .into_iter()
diff --git a/src/store/index.rs b/src/store/index.rs
index 32da01fc..fae57eaf 100644
--- a/src/store/index.rs
+++ b/src/store/index.rs
@@ -22,6 +22,9 @@ pub enum IndexError {
     #[error("index name does not exist: {0}")]
     IndexNameDoesNotExist(String),
 
+    #[error("conflict - table not found: {0}")]
+    ConflictTableNotFound(String),
+
     #[error("conflict - update failed - index value")]
     ConflictOnEmptyIndexValueUpdate,
 


Test patch:
diff --git a/src/tests/alter_table.rs b/src/tests/alter/alter_table.rs
similarity index 97%
rename from src/tests/alter_table.rs
rename to src/tests/alter/alter_table.rs
index b0a905d6..b043b7de 100644
--- a/src/tests/alter_table.rs
+++ b/src/tests/alter/alter_table.rs
@@ -1,10 +1,10 @@
-//! Tests in this file are only executed when feature = "alter-table" is enabled.
+#![cfg(feature = "alter-table")]
 
 use crate::*;
 use ast::*;
 use Value::*;
 
-test_case!(rename, async move {
+test_case!(alter_table_rename, async move {
     let test_cases = vec![
         ("CREATE TABLE Foo (id INTEGER);", Ok(Payload::Create)),
         (
@@ -34,7 +34,7 @@ test_case!(rename, async move {
     }
 });
 
-test_case!(add_drop, async move {
+test_case!(alter_table_add_drop, async move {
     let test_cases = vec![
         ("CREATE TABLE Foo (id INTEGER);", Ok(Payload::Create)),
         ("INSERT INTO Foo VALUES (1), (2);", Ok(Payload::Insert(2))),
diff --git a/src/tests/create_table.rs b/src/tests/alter/create_table.rs
similarity index 100%
rename from src/tests/create_table.rs
rename to src/tests/alter/create_table.rs
diff --git a/src/tests/drop_table.rs b/src/tests/alter/drop_table.rs
similarity index 100%
rename from src/tests/drop_table.rs
rename to src/tests/alter/drop_table.rs
diff --git a/src/tests/alter/mod.rs b/src/tests/alter/mod.rs
new file mode 100644
index 00000000..0fb834ab
--- /dev/null
+++ b/src/tests/alter/mod.rs
@@ -0,0 +1,8 @@
+mod alter_table;
+mod create_table;
+mod drop_table;
+
+#[cfg(feature = "alter-table")]
+pub use alter_table::{alter_table_add_drop, alter_table_rename};
+pub use create_table::create_table;
+pub use drop_table::drop_table;
diff --git a/src/tests/index/basic.rs b/src/tests/index/basic.rs
index 7d42da49..32a98043 100644
--- a/src/tests/index/basic.rs
+++ b/src/tests/index/basic.rs
@@ -272,7 +272,7 @@ CREATE TABLE Test (
     );
 
     test!(
-        Err(EvaluateError::UnsupportedStatelessExpr(format!(
+        Err(AlterError::UnsupportedIndexExpr(format!(
             "{:#?}",
             ast::Expr::CompoundIdentifier(vec!["a".to_owned(), "b".to_owned()])
         ))
@@ -281,10 +281,15 @@ CREATE TABLE Test (
     );
 
     test!(
-        Err(IndexError::TableNotFound("Abc".to_owned()).into()),
+        Err(AlterError::TableNotFound("Abc".to_owned()).into()),
         "CREATE INDEX idx_wow ON Abc (name)"
     );
 
+    test!(
+        Err(IndexError::TableNotFound("NoNameTable".to_owned()).into()),
+        "DROP INDEX NoNameTable.idx_id"
+    );
+
     test!(
         Err(IndexError::IndexNameAlreadyExists("idx_name".to_owned()).into()),
         "CREATE INDEX idx_name ON Test (name || id)"
diff --git a/src/tests/index/expr.rs b/src/tests/index/expr.rs
new file mode 100644
index 00000000..13b2f504
--- /dev/null
+++ b/src/tests/index/expr.rs
@@ -0,0 +1,142 @@
+use crate::*;
+
+test_case!(expr, async move {
+    run!(
+        r#"
+CREATE TABLE Test (
+    id INTEGER,
+    num INTEGER,
+    name TEXT
+)"#
+    );
+
+    run!(
+        r#"
+        INSERT INTO Test
+            (id, num, name)
+        VALUES
+            (1, 2, "Hello");
+    "#
+    );
+
+    use {
+        ast::{AstLiteral, Expr, IndexOperator::*},
+        Value::*,
+    };
+
+    test!(Ok(Payload::CreateIndex), "CREATE INDEX idx_id ON Test (id)");
+
+    test!(
+        Ok(Payload::CreateIndex),
+        "CREATE INDEX idx_typed_string ON Test ((id))"
+    );
+
+    test!(
+        Ok(Payload::CreateIndex),
+        "CREATE INDEX idx_binary_op ON Test (num || name);"
+    );
+
+    test!(
+        Ok(Payload::CreateIndex),
+        "CREATE INDEX idx_unary_op ON Test (-num);"
+    );
+
+    test!(
+        Ok(Payload::CreateIndex),
+        "CREATE INDEX idx_cast ON Test (CAST(id AS TEXT));"
+    );
+
+    test!(
+        Err(AlterError::IdentifierNotFound(format!(
+            "{:#?}",
+            Expr::Literal(AstLiteral::Number("100".to_owned()))
+        ))
+        .into()),
+        "CREATE INDEX idx_literal ON Test (100)"
+    );
+
+    test!(
+        Ok(Payload::Insert(1)),
+        r#"INSERT INTO Test VALUES (4, 7, "Well");"#
+    );
+
+    test!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            1     2     "Hello".to_owned();
+            4     7     "Well".to_owned()
+        )),
+        "SELECT id, num, name FROM Test"
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            1     2     "Hello".to_owned()
+        )),
+        idx!(idx_id, LtEq, "1"),
+        "SELECT id, num, name FROM Test WHERE id <= 1"
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            1     2     "Hello".to_owned()
+        )),
+        idx!(idx_id, LtEq, "(1)"),
+        "SELECT id, num, name FROM Test WHERE id <= (1)"
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            1     2     "Hello".to_owned()
+        )),
+        idx!(idx_binary_op, Eq, r#""2Hello""#),
+        r#"SELECT id, num, name FROM Test WHERE num || name = "2Hello""#
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            1     2     "Hello".to_owned()
+        )),
+        idx!(idx_binary_op, Eq, r#""2Hello""#),
+        r#"SELECT id, num, name FROM Test WHERE (num || name) = "2Hello""#
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            4     7     "Well".to_owned()
+        )),
+        idx!(idx_binary_op, Eq, r#""7Well""#),
+        r#"SELECT id, num, name FROM Test WHERE "7Well" = (num || name)"#
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            4     7     "Well".to_owned()
+        )),
+        idx!(idx_unary_op, Lt, "-2"),
+        "SELECT id, num, name FROM Test WHERE -num < -2"
+    );
+
+    test_idx!(
+        Ok(select!(
+            id  | num | name
+            I64 | I64 | Str;
+            4     7     "Well".to_owned()
+        )),
+        idx!(idx_cast, Eq, r#""4""#),
+        r#"SELECT id, num, name FROM Test WHERE CAST(id AS TEXT) = "4""#
+    );
+});
diff --git a/src/tests/index/mod.rs b/src/tests/index/mod.rs
index 218fb761..ce9da5ef 100644
--- a/src/tests/index/mod.rs
+++ b/src/tests/index/mod.rs
@@ -1,9 +1,13 @@
+#![cfg(feature = "index")]
+
 mod and;
 mod basic;
+mod expr;
 mod null;
 mod value;
 
 pub use and::and;
 pub use basic::basic;
+pub use expr::expr;
 pub use null::null;
 pub use value::value;
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index e9d17ae7..2f6dd23c 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -1,18 +1,14 @@
 pub mod aggregate;
-#[cfg(feature = "alter-table")]
-pub mod alter_table;
+pub mod alter;
 pub mod arithmetic;
 pub mod basic;
 pub mod blend;
 pub mod concat;
-pub mod create_table;
 pub mod data_type;
 pub mod default;
-pub mod drop_table;
 pub mod error;
 pub mod filter;
 pub mod function;
-#[cfg(feature = "index")]
 pub mod index;
 pub mod join;
 pub mod migrate;
@@ -50,9 +46,9 @@ macro_rules! generate_tests {
         glue!(arithmetic_blend, arithmetic::blend);
         glue!(concat, concat::concat);
         glue!(blend, blend::blend);
-        glue!(create_table, create_table::create_table);
+        glue!(create_table, alter::create_table);
+        glue!(drop_table, alter::drop_table);
         glue!(default, default::default);
-        glue!(drop_table, drop_table::drop_table);
         glue!(error, error::error);
         glue!(filter, filter::filter);
         glue!(function_upper_lower, function::upper_lower::upper_lower);
@@ -81,6 +77,7 @@ macro_rules! generate_tests {
                 glue!(index_basic, index::basic);
                 glue!(index_and, index::and);
                 glue!(index_null, index::null);
+                glue!(index_expr, index::expr);
                 glue!(index_value, index::value);
             };
         }
@@ -88,8 +85,8 @@ macro_rules! generate_tests {
         #[cfg(feature = "alter-table")]
         macro_rules! glue_alter_table {
             () => {
-                glue!(alter_table_rename, alter_table::rename);
-                glue!(alter_table_add_drop, alter_table::add_drop);
+                glue!(alter_table_rename, alter::alter_table_rename);
+                glue!(alter_table_add_drop, alter::alter_table_add_drop);
             };
         }