chore: consolidating path validation logic (#2200)

Closes #2091.

Consolidated repetitive checks for existence of paths and creation of
directories into a single function within the `ioutil` crate.

Added a dependency to each subcrate where this logic existed.

Added a new variant to the `MetastoreError` that converts from
`io::Error`.

Co-authored-by: universalmind303 <cory.grinstead@gmail.com>

bindings/nodejs/Cargo.toml

@@ -8,6 +8,7 @@ crate-type = ["cdylib"]
 
 [dependencies]
 # Default enable napi4 feature, see https://nodejs.org/api/n-api.html#node-api-version-matrix
+ioutil = { path = "../../crates/ioutil" }
 napi = { version = "2.14.1", default-features = false, features = ["full"] }
 napi-derive = "2.14.2"
 sqlexec = { path = "../../crates/sqlexec" }

bindings/nodejs/src/connection.rs

@@ -3,11 +3,12 @@ use crate::logical_plan::JsLogicalPlan;
 use datafusion::logical_expr::LogicalPlan as DFLogicalPlan;
 use datafusion_ext::vars::SessionVars;
 use futures::lock::Mutex;
+use ioutil::ensure_dir;
 use sqlexec::engine::{Engine, SessionStorageConfig, TrackedSession};
 use sqlexec::remote::client::{RemoteClient, RemoteClientType};
 use sqlexec::{LogicalPlan, OperationInfo};
 use std::collections::HashMap;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::sync::Arc;
 use url::Url;
 
@@ -284,21 +285,3 @@ impl Connection {
         Ok(())
     }
 }
-
-/// Ensure that a directory at the given path exists. Errors if the path exists
-/// and isn't a directory.
-fn ensure_dir(path: impl AsRef<Path>) -> napi::Result<()> {
-    let path = path.as_ref();
-    if !path.exists() {
-        std::fs::create_dir_all(path)?;
-    }
-
-    if path.exists() && !path.is_dir() {
-        Err(napi::Error::from_reason(format!(
-            "Path is not a valid directory {:?}",
-            &path
-        )))
-    } else {
-        Ok(())
-    }
-}

bindings/python/Cargo.toml

@@ -10,6 +10,7 @@ name = "glaredb"
 crate-type = ["cdylib"]
 
 [dependencies]
+ioutil = { path = "../../crates/ioutil" }
 tokio.workspace = true
 datafusion = { workspace = true, features = ["pyarrow"] }
 thiserror.workspace = true

bindings/python/src/connect.rs

@@ -9,20 +9,18 @@ use crate::error::PyGlareDbError;
 use crate::runtime::wait_for_future;
 use futures::lock::Mutex;
 use std::collections::HashMap;
-use std::{
-    fs,
-    path::{Path, PathBuf},
-    sync::Arc,
-};
+use std::{path::PathBuf, sync::Arc};
 use url::Url;
 
 use datafusion_ext::vars::SessionVars;
-use pyo3::{exceptions::PyRuntimeError, prelude::*};
+use pyo3::prelude::*;
 use sqlexec::{
     engine::{Engine, SessionStorageConfig},
     remote::client::{RemoteClient, RemoteClientType},
 };
 
+use ioutil::ensure_dir;
+
 #[derive(Debug, Clone)]
 struct PythonSessionConf {
     /// Where to store both metastore and user data.
@@ -158,21 +156,3 @@ pub fn connect(
         })
     })
 }
-
-/// Ensure that a directory at the given path exists. Errors if the path exists
-/// and isn't a directory.
-fn ensure_dir(path: impl AsRef<Path>) -> PyResult<()> {
-    let path = path.as_ref();
-    if !path.exists() {
-        fs::create_dir_all(path)?;
-    }
-
-    if path.exists() && !path.is_dir() {
-        Err(PyRuntimeError::new_err(format!(
-            "Path is not a valid directory {:?}",
-            &path
-        )))
-    } else {
-        Ok(())
-    }
-}

crates/glaredb/Cargo.toml

@@ -8,6 +8,7 @@ name = "glaredb"
 path = "src/bin/main.rs"
 
 [dependencies]
+ioutil = { path = "../ioutil" }
 arrow_util = { path = "../arrow_util" }
 logutil = { path = "../logutil" }
 sqlexec = { path = "../sqlexec" }

crates/glaredb/src/commands.rs

@@ -8,10 +8,10 @@ use crate::server::{ComputeServer, ServerConfig};
 use anyhow::{anyhow, Result};
 use atty::Stream;
 use clap::Subcommand;
+use ioutil::ensure_dir;
 use object_store_util::conf::StorageConfig;
 use pgsrv::auth::{LocalAuthenticator, PasswordlessAuthenticator, SingleUserAuthenticator};
 use std::collections::HashMap;
-use std::fs;
 use std::net::SocketAddr;
 use std::sync::atomic::{AtomicU64, Ordering};
 use tokio::net::TcpListener;
@@ -226,17 +226,7 @@ impl RunCommand for MetastoreArgs {
                 }
             }
             (None, None, Some(p)) => {
-                // Error if the path exists and is not a directory else
-                // create the directory.
-                if p.exists() && !p.is_dir() {
-                    return Err(anyhow!(
-                        "Path '{}' is not a valid directory",
-                        p.to_string_lossy()
-                    ));
-                } else if !p.exists() {
-                    fs::create_dir_all(&p)?;
-                }
-
+                ensure_dir(&p)?;
                 StorageConfig::Local { path: p }
             }
             (None, None, None) => StorageConfig::Memory,

crates/ioutil/src/lib.rs

@@ -13,6 +13,22 @@ pub fn resolve_path(path: &Path) -> std::io::Result<PathBuf> {
     path.canonicalize().map(|p| p.to_path_buf())
 }
 
+pub fn ensure_dir(path: impl AsRef<Path>) -> std::io::Result<()> {
+    let path = path.as_ref();
+    std::fs::create_dir_all(path).and_then(|()| {
+        if path.exists() && !path.is_dir() {
+            Err(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                format!(
+                    "Error, path {} is not a valid directory",
+                    path.to_string_lossy()
+                ),
+            ))?
+        }
+        Ok(())
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/metastore/Cargo.toml

@@ -6,6 +6,7 @@ edition = {workspace = true}
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
+ioutil = {path = "../ioutil"}
 logutil = {path = "../logutil"}
 protogen = {path = "../protogen"}
 sqlbuiltins = { path = "../sqlbuiltins" }

crates/metastore/src/errors.rs

@@ -103,6 +103,9 @@ pub enum MetastoreError {
 
     #[error("Cannot specify both 'IF NOT EXISTS' and 'OR REPLACE'")]
     InvalidCreatePolicy,
+
+    #[error(transparent)]
+    Io(#[from] std::io::Error),
 }
 
 pub type Result<T, E = MetastoreError> = std::result::Result<T, E>;

crates/metastore/src/util.rs

@@ -1,8 +1,9 @@
-use crate::errors::{MetastoreError, Result};
+use crate::errors::Result;
 use crate::local::{start_inprocess_inmemory, start_inprocess_local};
+use ioutil::ensure_dir;
 use protogen::gen::metastore::service::metastore_service_client::MetastoreServiceClient;
 use std::path::PathBuf;
-use std::{fs, time::Duration};
+use std::time::Duration;
 use tonic::transport::{Channel, Endpoint};
 use tracing::info;
 
@@ -39,20 +40,7 @@ impl MetastoreClientMode {
                 Ok(MetastoreServiceClient::new(channel))
             }
             Self::LocalDisk { path } => {
-                if !path.exists() {
-                    fs::create_dir_all(&path).map_err(|e| {
-                        MetastoreError::FailedInProcessStartup(format!(
-                            "Failed creating directory at path {}: {e}",
-                            path.to_string_lossy()
-                        ))
-                    })?;
-                }
-                if path.exists() && !path.is_dir() {
-                    return Err(MetastoreError::FailedInProcessStartup(format!(
-                        "Error creating metastore client, path {} is not a valid directory",
-                        path.to_string_lossy()
-                    )));
-                }
+                ensure_dir(&path)?;
                 start_inprocess_local(path).await
             }
             Self::LocalInMemory => start_inprocess_inmemory().await,

crates/sqlexec/Cargo.toml

@@ -6,6 +6,7 @@ edition = { workspace = true }
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
+ioutil = { path = "../ioutil" }
 arrow_util = { path = "../arrow_util" }
 logutil = { path = "../logutil" }
 protogen = { path = "../protogen" }

crates/sqlexec/src/engine.rs

@@ -7,6 +7,7 @@ use catalog::client::{MetastoreClientSupervisor, DEFAULT_METASTORE_CLIENT_CONFIG
 use sqlbuiltins::builtins::{SCHEMA_CURRENT_SESSION, SCHEMA_DEFAULT};
 use std::collections::HashMap;
 
+use ioutil::ensure_dir;
 use object_store::aws::AmazonS3ConfigKey;
 use object_store::{path::Path as ObjectPath, prefix::PrefixStore};
 use object_store::{Error as ObjectStoreError, ObjectStore};
@@ -63,9 +64,7 @@ pub struct EngineStorageConfig {
 
 impl EngineStorageConfig {
     pub fn try_from_path_buf(path: &PathBuf) -> Result<Self> {
-        if !path.exists() {
-            std::fs::create_dir_all(path)?;
-        }
+        ensure_dir(path)?;
         let path = fs::canonicalize(path)?;
         Ok(Self {
             location: Url::from_directory_path(&path).map_err(|_| {

crates/testing/Cargo.toml

@@ -20,6 +20,7 @@ regex = "1.8.1"
 uuid = { version = "1.6.1", features = ["v4", "fast-rng", "macro-diagnostics"] }
 openssh = "0.10.1"
 futures = "0.3.29"
+ioutil = { path = "../ioutil" }
 logutil = { path = "../logutil" }
 glaredb = { path = "../glaredb" }
 pgsrv = { path = "../pgsrv" }