Repository: gluesql/gluesql
Pull request #183 (2021-03-19 15:16:29)

Fix type validation to check all possible branches from INSERT & UPDATE
Issues
* Three different points were doing type validation: `Row::new`, `value::from_expr` and `validate::validate_types`.
* There were several missing points, no null check for validation or missing type check.

I decided to merge into `Value` rather than `validate` because `TryFromLiteral` also does type validation but it was not detachable to add to `validate` module.
Previous `validate_types` wasn't able to work with some cases, e.g. `Update` -> update requires type validation only using `Value` and `DataType` but `validate_types` runs based on `Row`.

I also divided `validate` into `Value::validate_type` and `Value::validate_null`.
If the codes make `Value` using `Value::from_expr` or `Value::try_from_literal`, those also check types so only null validation is required.
The other cases requires validation of both null and types.



Patch:
diff --git a/src/data/mod.rs b/src/data/mod.rs
index b67cd2c8..f0d107e1 100644
--- a/src/data/mod.rs
+++ b/src/data/mod.rs
@@ -1,6 +1,6 @@
 mod literal;
 mod row;
-mod schema;
+pub mod schema;
 mod table;
 pub mod value;
 
diff --git a/src/data/row.rs b/src/data/row.rs
index ea4c7848..afc9b9ab 100644
--- a/src/data/row.rs
+++ b/src/data/row.rs
@@ -2,9 +2,9 @@ use serde::{Deserialize, Serialize};
 use std::fmt::Debug;
 use thiserror::Error;
 
-use sqlparser::ast::{ColumnDef, ColumnOption, ColumnOptionDef, Expr, Ident};
+use sqlparser::ast::{ColumnDef, Expr, Ident};
 
-use crate::data::Value;
+use crate::data::{schema::ColumnDefExt, Value};
 use crate::result::Result;
 
 #[derive(Error, Serialize, Debug, PartialEq)]
@@ -47,10 +47,7 @@ impl Row {
             .enumerate()
             .map(|(i, column_def)| {
                 let ColumnDef {
-                    name,
-                    data_type,
-                    options,
-                    ..
+                    name, data_type, ..
                 } = column_def;
                 let name = name.to_string();
 
@@ -59,14 +56,7 @@ impl Row {
                     _ => columns.iter().position(|target| target.value == name),
                 };
 
-                let default = options
-                    .iter()
-                    .filter_map(|ColumnOptionDef { option, .. }| match option {
-                        ColumnOption::Default(expr) => Some(expr),
-                        _ => None,
-                    })
-                    .next();
-
+                let default = column_def.get_default();
                 let expr = match (i, default) {
                     (Some(i), _) => values
                         .get(i)
@@ -74,14 +64,32 @@ impl Row {
                     (None, Some(expr)) => Ok(expr),
                     (None, _) => Err(RowError::LackOfRequiredColumn(name.clone())),
                 }?;
-
-                let nullable = options
-                    .iter()
-                    .any(|ColumnOptionDef { option, .. }| option == &ColumnOption::Null);
+                let nullable = column_def.is_nullable();
 
                 Value::from_expr(&data_type, nullable, expr)
             })
             .collect::<Result<_>>()
             .map(Self)
     }
+
+    pub fn validate(&self, column_defs: &[ColumnDef]) -> Result<()> {
+        let items = column_defs
+            .iter()
+            .enumerate()
+            .filter_map(|(index, column_def)| {
+                let value = self.get_value(index);
+
+                value.map(|v| (v, column_def))
+            });
+
+        for (value, column_def) in items {
+            let ColumnDef { data_type, .. } = column_def;
+            let nullable = column_def.is_nullable();
+
+            value.validate_type(data_type)?;
+            value.validate_null(nullable)?;
+        }
+
+        Ok(())
+    }
 }
diff --git a/src/data/schema.rs b/src/data/schema.rs
index 42e6dfdf..deee494c 100644
--- a/src/data/schema.rs
+++ b/src/data/schema.rs
@@ -1,8 +1,34 @@
-use serde::{Deserialize, Serialize};
-use sqlparser::ast::ColumnDef;
+use {
+    serde::{Deserialize, Serialize},
+    sqlparser::ast::{ColumnDef, ColumnOption, ColumnOptionDef, Expr},
+};
 
 #[derive(Clone, Serialize, Deserialize)]
 pub struct Schema {
     pub table_name: String,
     pub column_defs: Vec<ColumnDef>,
 }
+
+pub trait ColumnDefExt {
+    fn is_nullable(&self) -> bool;
+
+    fn get_default(&self) -> Option<&Expr>;
+}
+
+impl ColumnDefExt for ColumnDef {
+    fn is_nullable(&self) -> bool {
+        self.options
+            .iter()
+            .any(|ColumnOptionDef { option, .. }| option == &ColumnOption::Null)
+    }
+
+    fn get_default(&self) -> Option<&Expr> {
+        self.options
+            .iter()
+            .filter_map(|ColumnOptionDef { option, .. }| match option {
+                ColumnOption::Default(expr) => Some(expr),
+                _ => None,
+            })
+            .next()
+    }
+}
diff --git a/src/data/value/error.rs b/src/data/value/error.rs
index 4d47c7e9..3ddd6c94 100644
--- a/src/data/value/error.rs
+++ b/src/data/value/error.rs
@@ -5,6 +5,12 @@ pub enum ValueError {
     #[error("literal: {literal} is incompatible with data type: {data_type}")]
     IncompatibleLiteralForDataType { data_type: String, literal: String },
 
+    #[error("incompatible data type, data type: {data_type}, value: {value}")]
+    IncompatibleDataType { data_type: String, value: String },
+
+    #[error("null value on not null field")]
+    NullValueOnNotNullField,
+
     #[error("failed to parse number")]
     FailedToParseNumber,
 
@@ -20,9 +26,6 @@ pub enum ValueError {
     #[error("divide on non numeric value")]
     DivideOnNonNumeric,
 
-    #[error("null value on not null field")]
-    NullValueOnNotNullField,
-
     #[error("floating numbers cannot be grouped by")]
     FloatCannotBeGroupedBy,
 
diff --git a/src/data/value/mod.rs b/src/data/value/mod.rs
index 4353fbd9..fe1b49fb 100644
--- a/src/data/value/mod.rs
+++ b/src/data/value/mod.rs
@@ -69,15 +69,13 @@ impl Value {
         let literal = Literal::try_from(expr)?;
         let value = Value::try_from_literal(&data_type, &literal)?;
 
-        if !nullable && matches!(value, Value::Null) {
-            return Err(ValueError::NullValueOnNotNullField.into());
-        }
+        value.validate_null(nullable)?;
 
         Ok(value)
     }
 
-    pub fn is_same_as_data_type(&self, data_type: &DataType) -> bool {
-        matches!(
+    pub fn validate_type(&self, data_type: &DataType) -> Result<()> {
+        let valid = matches!(
             (data_type, self),
             (DataType::Boolean, Value::Bool(_))
                 | (DataType::Int, Value::I64(_))
@@ -87,7 +85,25 @@ impl Value {
                 | (DataType::Int, Value::Null)
                 | (DataType::Float(_), Value::Null)
                 | (DataType::Text, Value::Null)
-        )
+        );
+
+        if !valid {
+            return Err(ValueError::IncompatibleDataType {
+                data_type: data_type.to_string(),
+                value: format!("{:?}", self),
+            }
+            .into());
+        }
+
+        Ok(())
+    }
+
+    pub fn validate_null(&self, nullable: bool) -> Result<()> {
+        if !nullable && matches!(self, Value::Null) {
+            return Err(ValueError::NullValueOnNotNullField.into());
+        }
+
+        Ok(())
     }
 
     pub fn cast(&self, data_type: &DataType) -> Result<Self> {
diff --git a/src/executor/execute.rs b/src/executor/execute.rs
index 5ae451d1..4fe21e92 100644
--- a/src/executor/execute.rs
+++ b/src/executor/execute.rs
@@ -18,7 +18,7 @@ use super::fetch::{fetch, fetch_columns};
 use super::filter::Filter;
 use super::select::{select, select_with_labels};
 use super::update::Update;
-use super::validate::{validate_rows, ColumnValidation};
+use super::validate::{validate_unique, ColumnValidation};
 
 #[derive(ThisError, Serialize, Debug, PartialEq)]
 pub enum ExecuteError {
@@ -191,6 +191,8 @@ pub async fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>
                     .fetch_schema(table_name)
                     .await?
                     .ok_or(ExecuteError::TableNotExists)?;
+                let column_defs = Rc::from(column_defs);
+                let column_validation = ColumnValidation::All(Rc::clone(&column_defs));
 
                 let rows = match &source.body {
                     SetExpr::Values(Values(values_list)) => values_list
@@ -209,6 +211,15 @@ pub async fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>
 
                         select(&storage, &query(), None)
                             .await?
+                            .and_then(|row| {
+                                let column_defs = Rc::clone(&column_defs);
+
+                                async move {
+                                    row.validate(&column_defs)?;
+
+                                    Ok(row)
+                                }
+                            })
                             .try_collect::<Vec<_>>()
                             .await?
                     }
@@ -220,14 +231,7 @@ pub async fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>
                     }
                 };
 
-                let column_defs = Rc::from(column_defs);
-                validate_rows(
-                    &storage,
-                    &table_name,
-                    ColumnValidation::All(Rc::clone(&column_defs)),
-                    rows.iter(),
-                )
-                .await?;
+                validate_unique(&storage, &table_name, column_validation, rows.iter()).await?;
 
                 Ok((rows, table_name))
             });
@@ -268,17 +272,16 @@ pub async fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>
                     .try_collect::<Vec<_>>()
                     .await?;
 
-                let all_column_defs = Rc::from(column_defs);
-                validate_rows(
+                let column_validation =
+                    ColumnValidation::SpecifiedColumns(Rc::from(column_defs), columns_to_update);
+                validate_unique(
                     &storage,
                     &table_name,
-                    ColumnValidation::SpecifiedColumns(
-                        Rc::clone(&all_column_defs),
-                        columns_to_update,
-                    ),
+                    column_validation,
                     rows.iter().map(|r| &r.1),
                 )
                 .await?;
+
                 Ok(rows)
             });
             let num_rows = rows.len();
diff --git a/src/executor/update.rs b/src/executor/update.rs
index d06bed9c..fbabec16 100644
--- a/src/executor/update.rs
+++ b/src/executor/update.rs
@@ -9,7 +9,7 @@ use sqlparser::ast::{Assignment, ColumnDef, Ident};
 use super::context::FilterContext;
 use super::evaluate::{evaluate, Evaluated};
 use crate::data::value::TryFromLiteral;
-use crate::data::{Row, Value};
+use crate::data::{schema::ColumnDefExt, Row, Value};
 use crate::result::Result;
 use crate::store::Store;
 
@@ -69,12 +69,19 @@ impl<'a, T: 'static + Debug> Update<'a, T> {
             Some(assignment) => {
                 let Assignment { value, .. } = &assignment;
                 let ColumnDef { data_type, .. } = column_def;
+                let nullable = column_def.is_nullable();
 
-                match evaluate(self.storage, context, None, value, false).await? {
-                    Evaluated::Literal(v) => Value::try_from_literal(data_type, &v),
-                    Evaluated::Value(v) => Ok(v.into_owned()),
-                }
-                .map(Some)
+                let value = match evaluate(self.storage, context, None, value, false).await? {
+                    Evaluated::Literal(v) => Value::try_from_literal(data_type, &v)?,
+                    Evaluated::Value(v) => {
+                        v.validate_type(data_type)?;
+                        v.into_owned()
+                    }
+                };
+
+                value.validate_null(nullable)?;
+
+                Ok(Some(value))
             }
         }
     }
diff --git a/src/executor/validate/unique.rs b/src/executor/validate.rs
similarity index 91%
rename from src/executor/validate/unique.rs
rename to src/executor/validate.rs
index e00fa076..bfc4abd3 100644
--- a/src/executor/validate/unique.rs
+++ b/src/executor/validate.rs
@@ -1,5 +1,4 @@
 use {
-    super::{ColumnValidation, ValidateError},
     crate::{
         data::{Row, Value},
         result::Result,
@@ -8,10 +7,26 @@ use {
     },
     boolinator::Boolinator,
     im_rc::HashSet,
+    serde::Serialize,
     sqlparser::ast::{ColumnDef, ColumnOption, Ident},
     std::{convert::TryInto, fmt::Debug, rc::Rc},
+    thiserror::Error as ThisError,
 };
 
+#[derive(ThisError, Debug, PartialEq, Serialize)]
+pub enum ValidateError {
+    #[error("conflict! storage row has no column on index {0}")]
+    ConflictOnStorageColumnIndex(usize),
+
+    #[error("duplicate entry '{0}' for unique column '{1}'")]
+    DuplicateEntryOnUniqueField(String, String),
+}
+
+pub enum ColumnValidation {
+    All(Rc<[ColumnDef]>),
+    SpecifiedColumns(Rc<[ColumnDef]>, Vec<Ident>),
+}
+
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub enum UniqueKey {
     Bool(bool),
diff --git a/src/executor/validate/mod.rs b/src/executor/validate/mod.rs
deleted file mode 100644
index ef7cbf3e..00000000
--- a/src/executor/validate/mod.rs
+++ /dev/null
@@ -1,76 +0,0 @@
-mod unique;
-
-use {
-    crate::{data::Row, result::Result, store::Store},
-    serde::Serialize,
-    sqlparser::ast::{ColumnDef, Ident},
-    std::{fmt::Debug, rc::Rc},
-    thiserror::Error as ThisError,
-    unique::validate_unique,
-};
-
-pub use unique::UniqueKey;
-
-#[derive(ThisError, Debug, PartialEq, Serialize)]
-pub enum ValidateError {
-    #[error("conflict! storage row has no column on index {0}")]
-    ConflictOnStorageColumnIndex(usize),
-
-    #[error("duplicate entry '{0}' for unique column '{1}'")]
-    DuplicateEntryOnUniqueField(String, String),
-
-    #[error("incompatible type attempted, value: {attempted_value} used for typed column: {column_name} ({column_type})")]
-    IncompatibleTypeOnTypedField {
-        attempted_value: String,
-        column_name: String,
-        column_type: String,
-    },
-}
-
-#[derive(Clone)]
-pub enum ColumnValidation {
-    All(Rc<[ColumnDef]>),
-    SpecifiedColumns(Rc<[ColumnDef]>, Vec<Ident>),
-}
-
-pub async fn validate_rows<T: 'static + Debug>(
-    storage: &impl Store<T>,
-    table_name: &str,
-    column_validation: ColumnValidation,
-    row_iter: impl Iterator<Item = &Row> + Clone,
-) -> Result<()> {
-    validate_unique(
-        storage,
-        table_name,
-        column_validation.clone(),
-        row_iter.clone(),
-    )
-    .await?;
-    validate_types(column_validation.clone(), row_iter.clone())?;
-    Ok(())
-}
-
-fn validate_types<'a>(
-    column_validation: ColumnValidation,
-    row_iter: impl Iterator<Item = &'a Row> + Clone,
-) -> Result<()> {
-    let columns = match column_validation {
-        ColumnValidation::All(columns) => columns,
-        ColumnValidation::SpecifiedColumns(columns, ..) => columns,
-    };
-    for row in row_iter.clone() {
-        for (index, column) in columns.iter().enumerate() {
-            if let Some(row_data) = row.get_value(index) {
-                if !row_data.is_same_as_data_type(&column.data_type) {
-                    return Err(ValidateError::IncompatibleTypeOnTypedField {
-                        attempted_value: format!("{:?}", row_data),
-                        column_name: column.name.value.to_string(),
-                        column_type: column.data_type.to_string(),
-                    }
-                    .into());
-                }
-            }
-        }
-    }
-    Ok(())
-}
diff --git a/src/storages/sled_storage/alter_table.rs b/src/storages/sled_storage/alter_table.rs
index 399b68de..1615be04 100644
--- a/src/storages/sled_storage/alter_table.rs
+++ b/src/storages/sled_storage/alter_table.rs
@@ -5,11 +5,11 @@ use boolinator::Boolinator;
 use std::iter::once;
 use std::str;
 
-use sqlparser::ast::{ColumnDef, ColumnOption, ColumnOptionDef, Ident};
+use sqlparser::ast::{ColumnDef, Ident};
 
 use super::{error::err_into, fetch_schema, SledStorage};
 use crate::utils::Vector;
-use crate::{AlterTable, AlterTableError, MutResult, Row, Schema, Value};
+use crate::{schema::ColumnDefExt, AlterTable, AlterTableError, MutResult, Row, Schema, Value};
 
 macro_rules! try_self {
     ($self: expr, $expr: expr) => {
@@ -140,24 +140,11 @@ impl AlterTable for SledStorage {
             ));
         }
 
-        let ColumnDef {
-            options, data_type, ..
-        } = column_def;
-
-        let nullable = options
-            .iter()
-            .any(|ColumnOptionDef { option, .. }| option == &ColumnOption::Null);
-        let default = options
-            .iter()
-            .filter_map(|ColumnOptionDef { option, .. }| match option {
-                ColumnOption::Default(expr) => Some(expr),
-                _ => None,
-            })
-            .map(|expr| Value::from_expr(&data_type, nullable, expr))
-            .next();
-
+        let ColumnDef { data_type, .. } = column_def;
+        let nullable = column_def.is_nullable();
+        let default = column_def.get_default();
         let value = match (default, nullable) {
-            (Some(value), _) => try_self!(self, value),
+            (Some(expr), _) => try_self!(self, Value::from_expr(&data_type, nullable, expr)),
             (None, true) => Value::Null,
             (None, false) => {
                 return Err((


Test patch:
diff --git a/src/tests/validate/types.rs b/src/tests/validate/types.rs
index c91d7d03..7bdee65d 100644
--- a/src/tests/validate/types.rs
+++ b/src/tests/validate/types.rs
@@ -1,24 +1,63 @@
-use {crate::*, std::borrow::Cow};
+use {crate::*, sqlparser::ast::DataType, std::borrow::Cow};
 
 test_case!(types, async move {
-    run!("CREATE TABLE TableB (id BOOL);");
-    run!("CREATE TABLE TableC (uid INTEGER UNIQUE);");
-    run!("INSERT INTO TableC (uid) VALUES (1);");
-    test!(
-        Err(ValidateError::IncompatibleTypeOnTypedField {
-            attempted_value: format!("{:?}", Value::I64(1)),
-            column_name: "id".to_owned(),
-            column_type: "BOOL".to_owned(),
-        }
-        .into()),
-        "INSERT INTO TableB SELECT uid FROM TableC;"
-    );
-    test!(
-        Err(ValueError::IncompatibleLiteralForDataType {
-            data_type: "INT".to_owned(),
-            literal: format!("{:?}", data::Literal::Text(Cow::Owned("A".to_owned()))),
-        }
-        .into()),
-        "INSERT INTO TableC (uid) VALUES (\"A\")"
-    );
+    run!("CREATE TABLE TableB (id BOOLEAN);");
+    run!("CREATE TABLE TableC (uid INTEGER, null_val INTEGER NULL);");
+    run!("INSERT INTO TableB VALUES (FALSE);");
+    run!("INSERT INTO TableC VALUES (1, NULL);");
+
+    let test_cases = vec![
+        (
+            "INSERT INTO TableB SELECT uid FROM TableC;",
+            Err(ValueError::IncompatibleDataType {
+                data_type: DataType::Boolean.to_string(),
+                value: format!("{:?}", Value::I64(1)),
+            }
+            .into()),
+        ),
+        (
+            "INSERT INTO TableC (uid) VALUES (\"A\")",
+            Err(ValueError::IncompatibleLiteralForDataType {
+                data_type: DataType::Int.to_string(),
+                literal: format!("{:?}", data::Literal::Text(Cow::Owned("A".to_owned()))),
+            }
+            .into()),
+        ),
+        (
+            "INSERT INTO TableC VALUES (NULL, 30);",
+            Err(ValueError::NullValueOnNotNullField.into()),
+        ),
+        (
+            "INSERT INTO TableC SELECT null_val FROM TableC;",
+            Err(ValueError::NullValueOnNotNullField.into()),
+        ),
+        (
+            "UPDATE TableC SET uid = TRUE;",
+            Err(ValueError::IncompatibleLiteralForDataType {
+                data_type: DataType::Int.to_string(),
+                literal: format!("{:?}", data::Literal::Boolean(true)),
+            }
+            .into()),
+        ),
+        (
+            "UPDATE TableC SET uid = (SELECT id FROM TableB LIMIT 1) WHERE uid = 1",
+            Err(ValueError::IncompatibleDataType {
+                data_type: DataType::Int.to_string(),
+                value: format!("{:?}", Value::Bool(false)),
+            }
+            .into()),
+        ),
+        (
+            "UPDATE TableC SET uid = NULL;",
+            Err(ValueError::NullValueOnNotNullField.into()),
+        ),
+        (
+            "UPDATE TableC SET uid = (SELECT null_val FROM TableC);",
+            Err(ValueError::NullValueOnNotNullField.into()),
+        ),
+    ];
+
+    for (sql, expected) in test_cases.into_iter() {
+        test!(expected, sql);
+    }
 });