Repository: gluesql/gluesql
Pull request #107 (2020-10-28 09:30:01)

Change Store->fetch_schema interface return type to Option<_>,
Remove StoreError, now fetch_schema returns Result<Option<Schema>>.
StoreError::SchemaNotFound was error, but now it simply returns "None".
This error handling is up to execution layer, not for store layer.

---

@leoppro Your previous PR became a good motivation. (https://github.com/gluesql/gluesql/pull/68)
`StoreError::SchemaNotFound` was something... not consistent with other interfaces.

Other mutating functions like insert and delete don't return error no matter there already exists data or not.
But only `fetch_schema` returns error when data not found case.




Patch:
diff --git a/src/executor/execute.rs b/src/executor/execute.rs
index 198c5801..4a35a0ef 100644
--- a/src/executor/execute.rs
+++ b/src/executor/execute.rs
@@ -31,9 +31,9 @@ pub enum ExecuteError {
     #[error("unsupported alter table operation: {0}")]
     UnsupportedAlterTableOperation(String),
 
-    #[error("TableNotExists")]
+    #[error("table does not exist")]
     TableNotExists,
-    #[error("TableAlreadyExists")]
+    #[error("table already exists")]
     TableAlreadyExists,
 }
 
@@ -57,26 +57,33 @@ pub fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>(
     storage: U,
     query: &Query,
 ) -> MutResult<U, Payload> {
+    macro_rules! try_into {
+        ($storage: expr, $expr: expr) => {
+            match $expr {
+                Err(e) => {
+                    return Err(($storage, e.into()));
+                }
+                Ok(v) => v,
+            }
+        };
+    }
+
     let Query(query) = query;
-    let prepared = match prepare(&storage, query) {
-        Ok(prepared) => prepared,
-        Err(error) => {
-            return Err((storage, error));
-        }
-    };
+    let prepared = try_into!(storage, prepare(&storage, query));
 
     match prepared {
         Prepared::Create {
             schema,
             if_not_exists,
         } => {
-            if storage.fetch_schema(&schema.table_name).is_ok() {
+            if try_into!(storage, storage.fetch_schema(&schema.table_name)).is_some() {
                 return if if_not_exists {
                     Ok((storage, Payload::Create))
                 } else {
                     Err((storage, ExecuteError::TableAlreadyExists.into()))
                 };
             }
+
             storage
                 .insert_schema(&schema)
                 .map(|(storage, _)| (storage, Payload::Create))
@@ -117,29 +124,20 @@ pub fn execute<T: 'static + Debug, U: Store<T> + StoreMut<T> + AlterTable>(
         Prepared::DropTable {
             schema_names,
             if_exists,
-        } => {
-            let storage = match if_exists {
-                true => Ok(storage),
-                false => schema_names
-                    .iter()
-                    .try_fold(storage, |storage, table_name| {
-                        if storage.fetch_schema(table_name).is_ok() {
-                            Ok(storage)
-                        } else {
-                            Err((storage, ExecuteError::TableNotExists.into()))
-                        }
-                    }),
-            }?;
-            let storage = schema_names
-                .iter()
-                .try_fold(storage, |storage, table_name| {
-                    let (storage, _) = storage.delete_schema(table_name)?;
+        } => schema_names
+            .iter()
+            .try_fold(storage, |storage, table_name| {
+                let schema = try_into!(storage, storage.fetch_schema(table_name));
 
-                    Ok(storage)
-                })?;
+                if !if_exists {
+                    try_into!(storage, schema.ok_or(ExecuteError::TableNotExists));
+                }
 
-            Ok((storage, Payload::DropTable))
-        }
+                storage
+                    .delete_schema(table_name)
+                    .map(|(storage, _)| storage)
+            })
+            .map(|storage| (storage, Payload::DropTable)),
         Prepared::Select { labels, rows } => Ok((storage, Payload::Select { labels, rows })),
 
         #[cfg(feature = "alter-table")]
@@ -229,7 +227,10 @@ fn prepare<'a, T: 'static + Debug>(
             source,
         } => {
             let table_name = get_name(table_name)?;
-            let Schema { column_defs, .. } = storage.fetch_schema(table_name)?;
+            let Schema { column_defs, .. } = storage
+                .fetch_schema(table_name)?
+                .ok_or(ExecuteError::TableNotExists)?;
+
             let values_list = match &source.body {
                 SetExpr::Values(Values(values_list)) => values_list,
                 set_expr => {
diff --git a/src/executor/fetch.rs b/src/executor/fetch.rs
index 82d64535..b8c019fb 100644
--- a/src/executor/fetch.rs
+++ b/src/executor/fetch.rs
@@ -1,5 +1,7 @@
 use boolinator::Boolinator;
+use serde::Serialize;
 use std::fmt::Debug;
+use thiserror::Error;
 
 use sqlparser::ast::{ColumnDef, Ident};
 
@@ -8,12 +10,19 @@ use crate::data::Row;
 use crate::result::Result;
 use crate::store::Store;
 
+#[derive(Error, Serialize, Debug, PartialEq)]
+pub enum FetchError {
+    #[error("table not found: {0}")]
+    TableNotFound(String),
+}
+
 pub fn fetch_columns<T: 'static + Debug>(
     storage: &dyn Store<T>,
     table_name: &str,
 ) -> Result<Vec<Ident>> {
     Ok(storage
         .fetch_schema(table_name)?
+        .ok_or_else(|| FetchError::TableNotFound(table_name.to_string()))?
         .column_defs
         .into_iter()
         .map(|ColumnDef { name, .. }| name)
diff --git a/src/executor/mod.rs b/src/executor/mod.rs
index 1598e093..0d9844b3 100644
--- a/src/executor/mod.rs
+++ b/src/executor/mod.rs
@@ -15,6 +15,7 @@ pub use blend::BlendError;
 pub use context::{BlendContextError, FilterContextError};
 pub use evaluate::EvaluateError;
 pub use execute::{execute, ExecuteError, Payload};
+pub use fetch::FetchError;
 pub use filter::FilterError;
 pub use join::JoinError;
 pub use limit::LimitError;
diff --git a/src/result.rs b/src/result.rs
index 1dea8e5c..8afa13fb 100644
--- a/src/result.rs
+++ b/src/result.rs
@@ -3,19 +3,15 @@ use thiserror::Error as ThisError;
 
 use crate::data::{RowError, TableError, ValueError};
 use crate::executor::{
-    AggregateError, BlendContextError, BlendError, EvaluateError, ExecuteError, FilterContextError,
-    FilterError, JoinError, LimitError, SelectError, UpdateError,
+    AggregateError, BlendContextError, BlendError, EvaluateError, ExecuteError, FetchError,
+    FilterContextError, FilterError, JoinError, LimitError, SelectError, UpdateError,
 };
 
 #[cfg(feature = "alter-table")]
 use crate::store::AlterTableError;
-use crate::store::StoreError;
 
 #[derive(ThisError, Serialize, Debug)]
 pub enum Error {
-    #[error(transparent)]
-    Store(#[from] StoreError),
-
     #[cfg(feature = "alter-table")]
     #[error(transparent)]
     AlterTable(#[from] AlterTableError),
@@ -27,6 +23,8 @@ pub enum Error {
     #[error(transparent)]
     Execute(#[from] ExecuteError),
     #[error(transparent)]
+    Fetch(#[from] FetchError),
+    #[error(transparent)]
     Evaluate(#[from] EvaluateError),
     #[error(transparent)]
     Select(#[from] SelectError),
@@ -62,10 +60,10 @@ impl PartialEq for Error {
         use Error::*;
 
         match (self, other) {
-            (Store(e), Store(e2)) => e == e2,
             #[cfg(feature = "alter-table")]
             (AlterTable(e), AlterTable(e2)) => e == e2,
             (Execute(e), Execute(e2)) => e == e2,
+            (Fetch(e), Fetch(e2)) => e == e2,
             (Evaluate(e), Evaluate(e2)) => e == e2,
             (Select(e), Select(e2)) => e == e2,
             (Join(e), Join(e2)) => e == e2,
diff --git a/src/storages/sled_storage/alter_table.rs b/src/storages/sled_storage/alter_table.rs
index 976125d9..130f16c5 100644
--- a/src/storages/sled_storage/alter_table.rs
+++ b/src/storages/sled_storage/alter_table.rs
@@ -19,10 +19,21 @@ macro_rules! try_self {
     };
 }
 
+macro_rules! fetch_schema {
+    ($self: expr, $tree: expr, $table_name: expr) => {{
+        let (key, schema) = try_self!($self, fetch_schema($tree, $table_name));
+        let schema = try_into!(
+            $self,
+            schema.ok_or_else(|| AlterTableError::TableNotFound($table_name.to_string()))
+        );
+
+        (key, schema)
+    }};
+}
+
 impl AlterTable for SledStorage {
     fn rename_schema(self, table_name: &str, new_table_name: &str) -> MutResult<Self, ()> {
-        let (_, Schema { column_defs, .. }) = try_self!(self, fetch_schema(&self.tree, table_name));
-
+        let (_, Schema { column_defs, .. }) = fetch_schema!(self, &self.tree, table_name);
         let schema = Schema {
             table_name: new_table_name.to_string(),
             column_defs,
@@ -62,8 +73,7 @@ impl AlterTable for SledStorage {
         old_column_name: &str,
         new_column_name: &str,
     ) -> MutResult<Self, ()> {
-        let (key, Schema { column_defs, .. }) =
-            try_self!(self, fetch_schema(&self.tree, table_name));
+        let (key, Schema { column_defs, .. }) = fetch_schema!(self, &self.tree, table_name);
 
         let i = column_defs
             .iter()
@@ -106,7 +116,7 @@ impl AlterTable for SledStorage {
                 table_name,
                 column_defs,
             },
-        ) = try_self!(self, fetch_schema(&self.tree, table_name));
+        ) = fetch_schema!(self, &self.tree, table_name);
 
         if column_defs
             .iter()
@@ -190,7 +200,7 @@ impl AlterTable for SledStorage {
                 table_name,
                 column_defs,
             },
-        ) = try_self!(self, fetch_schema(&self.tree, table_name));
+        ) = fetch_schema!(self, &self.tree, table_name);
 
         let index = column_defs
             .iter()
diff --git a/src/storages/sled_storage/mod.rs b/src/storages/sled_storage/mod.rs
index dce0d445..348c8e8b 100644
--- a/src/storages/sled_storage/mod.rs
+++ b/src/storages/sled_storage/mod.rs
@@ -7,7 +7,7 @@ use thiserror::Error as ThisError;
 use crate::AlterTable;
 #[cfg(feature = "alter-table")]
 use crate::AlterTableError;
-use crate::{Error, Result, Schema, StoreError};
+use crate::{Error, Result, Schema};
 
 #[cfg(feature = "alter-table")]
 mod alter_table;
@@ -18,9 +18,6 @@ mod store;
 
 #[derive(ThisError, Debug)]
 enum StorageError {
-    #[error(transparent)]
-    Store(#[from] StoreError),
-
     #[cfg(feature = "alter-table")]
     #[error(transparent)]
     AlterTable(#[from] AlterTableError),
@@ -41,7 +38,6 @@ impl Into<Error> for StorageError {
             Sled(e) => Error::Storage(Box::new(e)),
             Bincode(e) => Error::Storage(e),
             Str(e) => Error::Storage(Box::new(e)),
-            Store(e) => e.into(),
 
             #[cfg(feature = "alter-table")]
             AlterTable(e) => e.into(),
@@ -95,11 +91,10 @@ impl TryFrom<Config> for SledStorage {
     }
 }
 
-fn fetch_schema(tree: &Db, table_name: &str) -> Result<(String, Schema)> {
+fn fetch_schema(tree: &Db, table_name: &str) -> Result<(String, Option<Schema>)> {
     let key = format!("schema/{}", table_name);
     let value = try_into!(tree.get(&key.as_bytes()));
-    let value = value.ok_or(StoreError::SchemaNotFound)?;
-    let schema = try_into!(bincode::deserialize(&value));
+    let schema = try_into!(value.map(|v| bincode::deserialize(&v)).transpose());
 
     Ok((key, schema))
 }
diff --git a/src/storages/sled_storage/store.rs b/src/storages/sled_storage/store.rs
index 270ed119..42e74109 100644
--- a/src/storages/sled_storage/store.rs
+++ b/src/storages/sled_storage/store.rs
@@ -54,7 +54,7 @@ impl StoreMut<IVec> for SledStorage {
 }
 
 impl Store<IVec> for SledStorage {
-    fn fetch_schema(&self, table_name: &str) -> Result<Schema> {
+    fn fetch_schema(&self, table_name: &str) -> Result<Option<Schema>> {
         fetch_schema(&self.tree, table_name).map(|(_, schema)| schema)
     }
 
diff --git a/src/store/alter_table.rs b/src/store/alter_table.rs
index 3bd52fe4..29b735b8 100644
--- a/src/store/alter_table.rs
+++ b/src/store/alter_table.rs
@@ -8,6 +8,9 @@ use crate::result::MutResult;
 
 #[derive(Error, Serialize, Debug, PartialEq)]
 pub enum AlterTableError {
+    #[error("Table not found: {0}")]
+    TableNotFound(String),
+
     #[error("Renaming column not found")]
     RenamingColumnNotFound,
 
diff --git a/src/store/mod.rs b/src/store/mod.rs
index 076cb70b..8db9c8d6 100644
--- a/src/store/mod.rs
+++ b/src/store/mod.rs
@@ -5,25 +5,17 @@ pub use alter_table::*;
 #[cfg(not(feature = "alter-table"))]
 pub trait AlterTable {}
 
-use serde::Serialize;
 use std::fmt::Debug;
 use std::marker::Sized;
-use thiserror::Error;
 
 use crate::data::{Row, Schema};
 use crate::result::{MutResult, Result};
 
-#[derive(Error, Serialize, Debug, PartialEq)]
-pub enum StoreError {
-    #[error("Schema not found")]
-    SchemaNotFound,
-}
-
 pub type RowIter<T> = Box<dyn Iterator<Item = Result<(T, Row)>>>;
 
 /// By implementing `Store` trait, you can run `SELECT` queries.
 pub trait Store<T: Debug> {
-    fn fetch_schema(&self, table_name: &str) -> Result<Schema>;
+    fn fetch_schema(&self, table_name: &str) -> Result<Option<Schema>>;
 
     fn scan_data(&self, table_name: &str) -> Result<RowIter<T>>;
 }


Test patch:
diff --git a/src/tests/alter_table.rs b/src/tests/alter_table.rs
index e73e6622..7c245bd8 100644
--- a/src/tests/alter_table.rs
+++ b/src/tests/alter_table.rs
@@ -19,6 +19,10 @@ pub fn rename(tester: impl tests::Tester) {
             Ok(Payload::Insert(3)),
         ),
         ("SELECT id FROM Foo", Ok(select!(id; I64; 1; 2; 3))),
+        (
+            "ALTER TABLE Foo2 RENAME TO Bar;",
+            Err(AlterTableError::TableNotFound("Foo2".to_owned()).into()),
+        ),
         ("ALTER TABLE Foo RENAME TO Bar;", Ok(Payload::AlterTable)),
         ("SELECT id FROM Bar", Ok(select!(id; I64; 1; 2; 3))),
         (
diff --git a/src/tests/drop_table.rs b/src/tests/drop_table.rs
index a27b79fc..844de747 100644
--- a/src/tests/drop_table.rs
+++ b/src/tests/drop_table.rs
@@ -45,7 +45,7 @@ CREATE TABLE DropTable (
         ("DROP TABLE IF EXISTS DropTable;", Ok(Payload::DropTable)),
         (
             "SELECT id, num, name FROM DropTable;",
-            Err(StoreError::SchemaNotFound.into()),
+            Err(FetchError::TableNotFound("DropTable".to_owned()).into()),
         ),
         (create_sql, Ok(Payload::Create)),
         (
diff --git a/src/tests/error.rs b/src/tests/error.rs
index 28f63c3c..b334e156 100644
--- a/src/tests/error.rs
+++ b/src/tests/error.rs
@@ -6,7 +6,10 @@ pub fn error(mut tester: impl tests::Tester) {
 
     let test_cases = vec![
         (ExecuteError::QueryNotSupported.into(), "COMMIT;"),
-        (StoreError::SchemaNotFound.into(), "SELECT * FROM Nothing;"),
+        (
+            FetchError::TableNotFound("Nothing".to_owned()).into(),
+            "SELECT * FROM Nothing;",
+        ),
         (
             SelectError::TooManyTables.into(),
             "SELECT * FROM TableA, TableB",