Repository: gluesql/gluesql
Pull request #184 (2021-03-20 13:12:55)

Add validation to CREATE & ALTER TABLE
Now CREATE TABLE, ALTER TABLE and DROP codes are grouped by the module named "alter".

Apply 3 validations to CREATE TABLE and ALTER TABLE ADD COLUMN.
- data type (`INTEGER | FLOAT | BOOLEAN | TEXT`)
- column option (`NULL | NOT NULL | DEFAULT | UNIQUE`)
- `FLOAT & UNIQUE` is not allowed.


Patch:
diff --git a/src/executor/alter/alter_table.rs b/src/executor/alter/alter_table.rs
new file mode 100644
index 00000000..5c19033e
--- /dev/null
+++ b/src/executor/alter/alter_table.rs
@@ -0,0 +1,65 @@
+use {
+    super::{validate, AlterError},
+    crate::{
+        data::get_name,
+        result::MutResult,
+        store::{AlterTable, Store, StoreMut},
+    },
+    sqlparser::ast::{AlterTableOperation, ObjectName},
+    std::fmt::Debug,
+};
+
+macro_rules! try_into {
+    ($storage: expr, $expr: expr) => {
+        match $expr {
+            Err(e) => {
+                return Err(($storage, e.into()));
+            }
+            Ok(v) => v,
+        }
+    };
+}
+
+pub async fn alter_table<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>(
+    storage: U,
+    name: &ObjectName,
+    operation: &AlterTableOperation,
+) -> MutResult<U, ()> {
+    let table_name = try_into!(storage, get_name(name));
+
+    match operation {
+        AlterTableOperation::RenameTable {
+            table_name: new_table_name,
+        } => {
+            let new_table_name = try_into!(storage, get_name(new_table_name));
+
+            storage.rename_schema(table_name, new_table_name).await
+        }
+        AlterTableOperation::RenameColumn {
+            old_column_name,
+            new_column_name,
+        } => {
+            storage
+                .rename_column(table_name, &old_column_name.value, &new_column_name.value)
+                .await
+        }
+        AlterTableOperation::AddColumn { column_def } => {
+            try_into!(storage, validate(column_def));
+
+            storage.add_column(table_name, column_def).await
+        }
+        AlterTableOperation::DropColumn {
+            column_name,
+            if_exists,
+            ..
+        } => {
+            storage
+                .drop_column(table_name, &column_name.value, *if_exists)
+                .await
+        }
+        _ => Err((
+            storage,
+            AlterError::UnsupportedAlterTableOperation(operation.to_string()).into(),
+        )),
+    }
+}
diff --git a/src/executor/alter/create_table.rs b/src/executor/alter/create_table.rs
new file mode 100644
index 00000000..c6cf3fb7
--- /dev/null
+++ b/src/executor/alter/create_table.rs
@@ -0,0 +1,44 @@
+use {
+    super::{validate, AlterError},
+    crate::{
+        data::{get_name, Schema},
+        result::MutResult,
+        store::{AlterTable, Store, StoreMut},
+    },
+    sqlparser::ast::{ColumnDef, ObjectName},
+    std::fmt::Debug,
+};
+
+pub async fn create_table<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>(
+    storage: U,
+    name: &ObjectName,
+    column_defs: &[ColumnDef],
+    if_not_exists: bool,
+) -> MutResult<U, ()> {
+    let schema = (|| async {
+        let schema = Schema {
+            table_name: get_name(name)?.to_string(),
+            column_defs: column_defs.to_vec(),
+        };
+
+        for column_def in &schema.column_defs {
+            validate(column_def)?;
+        }
+
+        if !if_not_exists && storage.fetch_schema(&schema.table_name).await?.is_some() {
+            return Err(AlterError::TableAlreadyExists(schema.table_name.to_owned()).into());
+        }
+
+        Ok(schema)
+    })()
+    .await;
+
+    let schema = match schema {
+        Ok(s) => s,
+        Err(e) => {
+            return Err((storage, e));
+        }
+    };
+
+    storage.insert_schema(&schema).await
+}
diff --git a/src/executor/alter/drop.rs b/src/executor/alter/drop.rs
new file mode 100644
index 00000000..cfb9f380
--- /dev/null
+++ b/src/executor/alter/drop.rs
@@ -0,0 +1,50 @@
+use {
+    super::AlterError,
+    crate::{
+        data::get_name,
+        result::MutResult,
+        store::{AlterTable, Store, StoreMut},
+    },
+    futures::stream::{self, TryStreamExt},
+    sqlparser::ast::{ObjectName, ObjectType},
+    std::fmt::Debug,
+};
+
+pub async fn drop<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>(
+    storage: U,
+    object_type: &ObjectType,
+    names: &[ObjectName],
+    if_exists: bool,
+) -> MutResult<U, ()> {
+    if object_type != &ObjectType::Table {
+        return Err((
+            storage,
+            AlterError::DropTypeNotSupported(object_type.to_string()).into(),
+        ));
+    }
+
+    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
new file mode 100644
index 00000000..ab039c51
--- /dev/null
+++ b/src/executor/alter/error.rs
@@ -0,0 +1,30 @@
+use {serde::Serialize, std::fmt::Debug, thiserror::Error};
+
+#[derive(Error, Serialize, Debug, PartialEq)]
+pub enum AlterError {
+    // CREATE TABLE
+    #[error("table already exists: {0}")]
+    TableAlreadyExists(String),
+
+    // ALTER TABLE
+    #[cfg(feature = "alter-table")]
+    #[error("unsupported alter table operation: {0}")]
+    UnsupportedAlterTableOperation(String),
+
+    // DROP
+    #[error("drop type not supported: {0}")]
+    DropTypeNotSupported(String),
+
+    #[error("table does not exist: {0}")]
+    TableNotFound(String),
+
+    // validate column def
+    #[error("unsupported data type: {0}")]
+    UnsupportedDataType(String),
+
+    #[error("unsupported column option: {0}")]
+    UnsupportedColumnOption(String),
+
+    #[error("column '{0}' of data type '{1}' is unsupported for unique constraint")]
+    UnsupportedDataTypeForUniqueColumn(String, String),
+}
diff --git a/src/executor/alter/mod.rs b/src/executor/alter/mod.rs
new file mode 100644
index 00000000..1b694910
--- /dev/null
+++ b/src/executor/alter/mod.rs
@@ -0,0 +1,14 @@
+#[cfg(feature = "alter-table")]
+mod alter_table;
+mod create_table;
+mod drop;
+mod error;
+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;
diff --git a/src/executor/alter/validate.rs b/src/executor/alter/validate.rs
new file mode 100644
index 00000000..1890bcda
--- /dev/null
+++ b/src/executor/alter/validate.rs
@@ -0,0 +1,50 @@
+use {
+    super::AlterError,
+    crate::result::Result,
+    sqlparser::ast::{ColumnDef, ColumnOption, ColumnOptionDef, DataType},
+};
+
+pub fn validate(column_def: &ColumnDef) -> Result<()> {
+    let ColumnDef {
+        data_type,
+        options,
+        name,
+        ..
+    } = column_def;
+
+    // data type
+    if !matches!(
+        data_type,
+        DataType::Boolean | DataType::Int | DataType::Float(_) | DataType::Text
+    ) {
+        return Err(AlterError::UnsupportedDataType(data_type.to_string()).into());
+    }
+
+    // column option
+    if let Some(option) = options.iter().find(|ColumnOptionDef { option, .. }| {
+        !matches!(
+            option,
+            ColumnOption::Null
+                | ColumnOption::NotNull
+                | ColumnOption::Default(_)
+                | ColumnOption::Unique { .. }
+        )
+    }) {
+        return Err(AlterError::UnsupportedColumnOption(option.to_string()).into());
+    }
+
+    // unique + data type
+    if matches!(data_type, DataType::Float(_))
+        && options
+            .iter()
+            .any(|ColumnOptionDef { option, .. }| matches!(option, ColumnOption::Unique { .. }))
+    {
+        return Err(AlterError::UnsupportedDataTypeForUniqueColumn(
+            name.to_string(),
+            data_type.to_string(),
+        )
+        .into());
+    }
+
+    Ok(())
+}
diff --git a/src/executor/create_table.rs b/src/executor/create_table.rs
deleted file mode 100644
index e9b9cbfb..00000000
--- a/src/executor/create_table.rs
+++ /dev/null
@@ -1,58 +0,0 @@
-use serde::Serialize;
-use sqlparser::ast::{ColumnDef, ColumnOption, DataType};
-use std::fmt::Debug;
-use thiserror::Error as ThisError;
-
-use crate::data::Schema;
-use crate::result::Result;
-use crate::store::Store;
-
-#[derive(Debug, PartialEq, Serialize, ThisError)]
-pub enum CreateTableError {
-    #[error("table already exists")]
-    TableAlreadyExists,
-
-    #[error("column '{0}' of data type '{1}' is unsupported for unique constraint")]
-    UnsupportedDataTypeForUniqueColumn(String, String),
-}
-
-pub async fn validate_table<T: 'static + Debug>(
-    storage: &impl Store<T>,
-    schema: &Schema,
-    if_not_exists: bool,
-) -> Result<()> {
-    validate_column_unique_option(&schema.column_defs)?;
-    validate_table_if_not_exists(storage, &schema.table_name, if_not_exists).await
-}
-
-fn validate_column_unique_option(column_defs: &[ColumnDef]) -> Result<()> {
-    let found = column_defs.iter().find(|col| match col.data_type {
-        DataType::Float(_) => col
-            .options
-            .iter()
-            .any(|opt| matches!(opt.option, ColumnOption::Unique { .. })),
-        _ => false,
-    });
-
-    if let Some(col) = found {
-        return Err(CreateTableError::UnsupportedDataTypeForUniqueColumn(
-            col.name.to_string(),
-            col.data_type.to_string(),
-        )
-        .into());
-    }
-
-    Ok(())
-}
-
-async fn validate_table_if_not_exists<'a, T: 'static + Debug>(
-    storage: &impl Store<T>,
-    table_name: &str,
-    if_not_exists: bool,
-) -> Result<()> {
-    if if_not_exists || storage.fetch_schema(table_name).await?.is_none() {
-        return Ok(());
-    }
-
-    Err(CreateTableError::TableAlreadyExists.into())
-}
diff --git a/src/executor/execute.rs b/src/executor/execute.rs
index 4fe21e92..decb25c8 100644
--- a/src/executor/execute.rs
+++ b/src/executor/execute.rs
@@ -1,19 +1,19 @@
-use futures::stream::{self, TryStreamExt};
+use futures::stream::TryStreamExt;
 use serde::Serialize;
 use std::fmt::Debug;
 use std::rc::Rc;
 use thiserror::Error as ThisError;
 
-#[cfg(feature = "alter-table")]
-use sqlparser::ast::AlterTableOperation;
-use sqlparser::ast::{ObjectType, SetExpr, Statement, Values};
+use sqlparser::ast::{SetExpr, Statement, Values};
 
 use crate::data::{get_name, Row, Schema};
 use crate::parse_sql::Query;
 use crate::result::{MutResult, Result};
 use crate::store::{AlterTable, Store, StoreMut};
 
-use super::create_table::validate_table;
+#[cfg(feature = "alter-table")]
+use super::alter::alter_table;
+use super::alter::{create_table, drop};
 use super::fetch::{fetch, fetch_columns};
 use super::filter::Filter;
 use super::select::{select, select_with_labels};
@@ -25,16 +25,9 @@ pub enum ExecuteError {
     #[error("query not supported")]
     QueryNotSupported,
 
-    #[error("drop type not supported")]
-    DropTypeNotSupported,
-
     #[error("unsupported insert value type: {0}")]
     UnreachableUnsupportedInsertValueType(String),
 
-    #[cfg(feature = "alter-table")]
-    #[error("unsupported alter table operation: {0}")]
-    UnsupportedAlterTableOperation(String),
-
     #[error("table does not exist")]
     TableNotExists,
 }
@@ -70,18 +63,6 @@ pub async fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>
         }};
     }
 
-    #[cfg(feature = "alter-table")]
-    macro_rules! try_into {
-        ($storage: expr, $expr: expr) => {
-            match $expr {
-                Err(e) => {
-                    return Err(($storage, e.into()));
-                }
-                Ok(v) => v,
-            }
-        };
-    }
-
     let Query(query) = query;
 
     match query {
@@ -92,91 +73,21 @@ pub async fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>
             columns,
             if_not_exists,
             ..
-        } => {
-            let schema = try_block!(storage, {
-                let schema = Schema {
-                    table_name: get_name(name)?.to_string(),
-                    column_defs: columns.clone(),
-                };
-
-                validate_table(&storage, &schema, *if_not_exists).await?;
-                Ok(schema)
-            });
-
-            storage
-                .insert_schema(&schema)
-                .await
-                .map(|(storage, _)| (storage, Payload::Create))
-        }
+        } => create_table(storage, name, columns, *if_not_exists)
+            .await
+            .map(|(storage, _)| (storage, Payload::Create)),
         Statement::Drop {
             object_type,
             names,
             if_exists,
             ..
-        } => {
-            if object_type != &ObjectType::Table {
-                return Err((storage, ExecuteError::DropTypeNotSupported.into()));
-            }
-
-            stream::iter(names.iter().map(Ok))
-                .try_fold(storage, |storage, table_name| async move {
-                    let schema = try_block!(storage, {
-                        let table_name = get_name(table_name)?;
-                        let schema = storage.fetch_schema(table_name).await?;
-
-                        if !if_exists {
-                            schema.ok_or(ExecuteError::TableNotExists)?;
-                        }
-                        Ok(table_name)
-                    });
-                    storage
-                        .delete_schema(schema)
-                        .await
-                        .map(|(storage, _)| storage)
-                })
-                .await
-                .map(|storage| (storage, Payload::DropTable))
-        }
+        } => drop(storage, object_type, names, *if_exists)
+            .await
+            .map(|(storage, _)| (storage, Payload::DropTable)),
         #[cfg(feature = "alter-table")]
-        Statement::AlterTable { name, operation } => {
-            let table_name = try_into!(storage, get_name(name));
-
-            let result = match operation {
-                AlterTableOperation::RenameTable {
-                    table_name: new_table_name,
-                } => {
-                    let new_table_name = try_into!(storage, get_name(new_table_name));
-
-                    storage.rename_schema(table_name, new_table_name).await
-                }
-                AlterTableOperation::RenameColumn {
-                    old_column_name,
-                    new_column_name,
-                } => {
-                    storage
-                        .rename_column(table_name, &old_column_name.value, &new_column_name.value)
-                        .await
-                }
-                AlterTableOperation::AddColumn { column_def } => {
-                    storage.add_column(table_name, column_def).await
-                }
-                AlterTableOperation::DropColumn {
-                    column_name,
-                    if_exists,
-                    ..
-                } => {
-                    storage
-                        .drop_column(table_name, &column_name.value, *if_exists)
-                        .await
-                }
-                _ => Err((
-                    storage,
-                    ExecuteError::UnsupportedAlterTableOperation(operation.to_string()).into(),
-                )),
-            };
-
-            result.map(|(storage, _)| (storage, Payload::AlterTable))
-        }
+        Statement::AlterTable { name, operation } => alter_table(storage, name, operation)
+            .await
+            .map(|(storage, _)| (storage, Payload::AlterTable)),
 
         //-- Rows
         Statement::Insert {
diff --git a/src/executor/mod.rs b/src/executor/mod.rs
index 5d969da0..64696d5e 100644
--- a/src/executor/mod.rs
+++ b/src/executor/mod.rs
@@ -1,7 +1,7 @@
 mod aggregate;
+mod alter;
 mod blend;
 mod context;
-mod create_table;
 mod evaluate;
 mod execute;
 mod fetch;
@@ -13,8 +13,8 @@ mod update;
 mod validate;
 
 pub use aggregate::{AggregateError, GroupKey};
+pub use alter::AlterError;
 pub use blend::BlendError;
-pub use create_table::CreateTableError;
 pub use evaluate::EvaluateError;
 pub use execute::{execute, ExecuteError, Payload};
 pub use fetch::FetchError;
diff --git a/src/result.rs b/src/result.rs
index f44a307a..23fd0eb5 100644
--- a/src/result.rs
+++ b/src/result.rs
@@ -2,7 +2,7 @@ use {
     crate::{
         data::{LiteralError, RowError, TableError, ValueError},
         executor::{
-            AggregateError, BlendError, CreateTableError, EvaluateError, ExecuteError, FetchError,
+            AggregateError, AlterError, BlendError, EvaluateError, ExecuteError, FetchError,
             FilterError, JoinError, LimitError, SelectError, UpdateError, ValidateError,
         },
     },
@@ -26,6 +26,8 @@ pub enum Error {
     #[error(transparent)]
     Execute(#[from] ExecuteError),
     #[error(transparent)]
+    Alter(#[from] AlterError),
+    #[error(transparent)]
     Fetch(#[from] FetchError),
     #[error(transparent)]
     Evaluate(#[from] EvaluateError),
@@ -53,8 +55,6 @@ pub enum Error {
     Value(#[from] ValueError),
     #[error(transparent)]
     Literal(#[from] LiteralError),
-    #[error(transparent)]
-    CreateTable(#[from] CreateTableError),
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -68,6 +68,7 @@ impl PartialEq for Error {
             #[cfg(feature = "alter-table")]
             (AlterTable(e), AlterTable(e2)) => e == e2,
             (Execute(e), Execute(e2)) => e == e2,
+            (Alter(e), Alter(e2)) => e == e2,
             (Fetch(e), Fetch(e2)) => e == e2,
             (Evaluate(e), Evaluate(e2)) => e == e2,
             (Select(e), Select(e2)) => e == e2,
@@ -82,7 +83,6 @@ impl PartialEq for Error {
             (Validate(e), Validate(e2)) => e == e2,
             (Value(e), Value(e2)) => e == e2,
             (Literal(e), Literal(e2)) => e == e2,
-            (CreateTable(e), CreateTable(e2)) => e == e2,
             _ => false,
         }
     }


Test patch:
diff --git a/src/tests/alter_table.rs b/src/tests/alter_table.rs
index 6b46b424..f5abc722 100644
--- a/src/tests/alter_table.rs
+++ b/src/tests/alter_table.rs
@@ -82,6 +82,22 @@ test_case!(add_drop, async move {
             "ALTER TABLE Foo ADD COLUMN something INTEGER DEFAULT (SELECT id FROM Bar LIMIT 1)",
             Err(LiteralError::UnsupportedExpr("(SELECT id FROM Bar LIMIT 1)".to_owned()).into()),
         ),
+        (
+            "ALTER TABLE Foo ADD COLUMN something SOMEWHAT",
+            Err(AlterError::UnsupportedDataType("SOMEWHAT".to_owned()).into()),
+        ),
+        (
+            "ALTER TABLE Foo ADD COLUMN something INTEGER CHECK (true)",
+            Err(AlterError::UnsupportedColumnOption("CHECK (true)".to_owned()).into()),
+        ),
+        (
+            "ALTER TABLE Foo ADD COLUMN something FLOAT UNIQUE",
+            Err(AlterError::UnsupportedDataTypeForUniqueColumn(
+                "something".to_owned(),
+                "FLOAT".to_owned(),
+            )
+            .into()),
+        ),
         (
             "ALTER TABLE Foo DROP COLUMN IF EXISTS something;",
             Ok(Payload::AlterTable),
diff --git a/src/tests/create_table.rs b/src/tests/create_table.rs
index f14413db..3bfb2cec 100644
--- a/src/tests/create_table.rs
+++ b/src/tests/create_table.rs
@@ -18,7 +18,7 @@ test_case!(create_table, async move {
             num INTEGER,
             name TEXT
         )"#,
-            Err(CreateTableError::TableAlreadyExists.into()),
+            Err(AlterError::TableAlreadyExists("CreateTable1".to_owned()).into()),
         ),
         (
             r#"
@@ -38,13 +38,21 @@ test_case!(create_table, async move {
         )"#,
             Ok(Payload::Create),
         ),
+        (
+            "CREATE TABLE Gluery (id SOMEWHAT);",
+            Err(AlterError::UnsupportedDataType("SOMEWHAT".to_owned()).into()),
+        ),
+        (
+            "CREATE TABLE Gluery (id INTEGER CHECK (true));",
+            Err(AlterError::UnsupportedColumnOption("CHECK (true)".to_owned()).into()),
+        ),
         (
             r#"
         CREATE TABLE CreateTable3 (
             id INTEGER,
             ratio FLOAT UNIQUE
         )"#,
-            Err(CreateTableError::UnsupportedDataTypeForUniqueColumn(
+            Err(AlterError::UnsupportedDataTypeForUniqueColumn(
                 "ratio".to_owned(),
                 "FLOAT".to_owned(),
             )
diff --git a/src/tests/drop_table.rs b/src/tests/drop_table.rs
index 177d1ef1..4aae7e3d 100644
--- a/src/tests/drop_table.rs
+++ b/src/tests/drop_table.rs
@@ -30,7 +30,7 @@ CREATE TABLE DropTable (
         ("DROP TABLE DropTable;", Ok(Payload::DropTable)),
         (
             "DROP TABLE DropTable;",
-            Err(ExecuteError::TableNotExists.into()),
+            Err(AlterError::TableNotFound("DropTable".to_owned()).into()),
         ),
         (
             r#"
@@ -54,7 +54,7 @@ CREATE TABLE DropTable (
         ),
         (
             "DROP VIEW DropTable;",
-            Err(ExecuteError::DropTypeNotSupported.into()),
+            Err(AlterError::DropTypeNotSupported("VIEW".to_owned()).into()),
         ),
     ];
 
diff --git a/src/tests/error.rs b/src/tests/error.rs
index 3bf81971..110ebabd 100644
--- a/src/tests/error.rs
+++ b/src/tests/error.rs
@@ -6,6 +6,14 @@ test_case!(error, async move {
 
     let test_cases = vec![
         (ExecuteError::QueryNotSupported.into(), "COMMIT;"),
+        (
+            ExecuteError::TableNotExists.into(),
+            "INSERT INTO Nothing VALUES (1);",
+        ),
+        (
+            ExecuteError::TableNotExists.into(),
+            "UPDATE Nothing SET a = 1;",
+        ),
         (
             FetchError::TableNotFound("Nothing".to_owned()).into(),
             "SELECT * FROM Nothing;",
@@ -52,7 +60,7 @@ test_case!(error, async move {
         ),
         #[cfg(feature = "alter-table")]
         (
-            ExecuteError::UnsupportedAlterTableOperation(
+            AlterError::UnsupportedAlterTableOperation(
                 r#"ADD CONSTRAINT "hey" PRIMARY KEY (asdf)"#.to_owned(),
             )
             .into(),