feat: Add OptionValidator trait for FDW options validation and Saniti…

…ze user mapping options (paradedb#37)

* feat: Sanitize the user mapping options

* fix: Correct the bucket name

Cargo.toml

@@ -30,7 +30,7 @@ serde = "1.0.201"
 serde_json = "1.0.120"
 signal-hook = "0.3.17"
 strum = { version = "0.26.3", features = ["derive"] }
-supabase-wrappers = { git = "https://github.com/paradedb/wrappers.git", default-features = false, rev = "c6f5e79" }
+supabase-wrappers = { git = "https://github.com/paradedb/wrappers.git", default-features = false, rev = "719628f" }
 thiserror = "1.0.59"
 uuid = "1.9.1"
 

src/duckdb/csv.rs

@@ -19,6 +19,8 @@ use anyhow::{anyhow, Result};
 use std::collections::HashMap;
 use strum::{AsRefStr, EnumIter};
 
+use crate::fdw::base::OptionValidator;
+
 use super::utils;
 
 #[derive(EnumIter, AsRefStr, PartialEq, Debug)]
@@ -93,8 +95,8 @@ pub enum CsvOption {
     UnionByName,
 }
 
-impl CsvOption {
-    pub fn is_required(&self) -> bool {
+impl OptionValidator for CsvOption {
+    fn is_required(&self) -> bool {
         match self {
             Self::AllVarchar => false,
             Self::AllowQuotedNulls => false,

src/duckdb/delta.rs

@@ -15,6 +15,7 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program. If not, see <http://www.gnu.org/licenses/>.
 
+use crate::fdw::base::OptionValidator;
 use anyhow::{anyhow, Result};
 use std::collections::HashMap;
 use strum::{AsRefStr, EnumIter};
@@ -29,8 +30,8 @@ pub enum DeltaOption {
     Select,
 }
 
-impl DeltaOption {
-    pub fn is_required(&self) -> bool {
+impl OptionValidator for DeltaOption {
+    fn is_required(&self) -> bool {
         match self {
             Self::Files => true,
             Self::PreserveCasing => false,

src/duckdb/iceberg.rs

@@ -19,6 +19,8 @@ use anyhow::{anyhow, Result};
 use std::collections::HashMap;
 use strum::{AsRefStr, EnumIter};
 
+use crate::fdw::base::OptionValidator;
+
 #[derive(EnumIter, AsRefStr, PartialEq, Debug)]
 pub enum IcebergOption {
     #[strum(serialize = "allow_moved_paths")]
@@ -31,8 +33,8 @@ pub enum IcebergOption {
     Select,
 }
 
-impl IcebergOption {
-    pub fn is_required(&self) -> bool {
+impl OptionValidator for IcebergOption {
+    fn is_required(&self) -> bool {
         match self {
             Self::AllowMovedPaths => false,
             Self::Files => true,

src/duckdb/parquet.rs

@@ -19,6 +19,8 @@ use anyhow::{anyhow, Result};
 use std::collections::HashMap;
 use strum::{AsRefStr, EnumIter};
 
+use crate::fdw::base::OptionValidator;
+
 use super::utils;
 
 #[derive(EnumIter, AsRefStr, PartialEq, Debug)]
@@ -46,8 +48,8 @@ pub enum ParquetOption {
     // TODO: EncryptionConfig
 }
 
-impl ParquetOption {
-    pub fn is_required(&self) -> bool {
+impl OptionValidator for ParquetOption {
+    fn is_required(&self) -> bool {
         match self {
             Self::BinaryAsString => false,
             Self::FileName => false,

src/duckdb/secret.rs

@@ -19,6 +19,8 @@ use anyhow::{anyhow, bail, Result};
 use std::collections::HashMap;
 use strum::{AsRefStr, EnumIter};
 
+use crate::fdw::base::OptionValidator;
+
 #[derive(EnumIter, AsRefStr, PartialEq, Debug)]
 pub enum UserMappingOptions {
     // Universal
@@ -70,9 +72,8 @@ pub enum UserMappingOptions {
     ProxyPassword,
 }
 
-impl UserMappingOptions {
-    #[allow(unused)]
-    pub fn is_required(&self) -> bool {
+impl OptionValidator for UserMappingOptions {
+    fn is_required(&self) -> bool {
         match self {
             Self::Type => true,
             Self::Provider => false,

src/duckdb/spatial.rs

@@ -20,6 +20,8 @@ use std::collections::HashMap;
 use strum::IntoEnumIterator;
 use strum::{AsRefStr, EnumIter};
 
+use crate::fdw::base::OptionValidator;
+
 /// SpatialOption is an enum that represents the options that can be passed to the st_read function.
 /// Reference https://github.com/duckdb/duckdb_spatial/blob/main/docs/functions.md#st_read
 #[derive(EnumIter, AsRefStr, PartialEq, Debug)]
@@ -44,8 +46,8 @@ pub enum SpatialOption {
     KeepWkb,
 }
 
-impl SpatialOption {
-    pub fn is_required(&self) -> bool {
+impl OptionValidator for SpatialOption {
+    fn is_required(&self) -> bool {
         match self {
             Self::Files => true,
             Self::SequentialLayerScan => false,

src/fdw/base.rs

@@ -19,6 +19,7 @@ use anyhow::{anyhow, bail, Result};
 use duckdb::arrow::array::RecordBatch;
 use pgrx::*;
 use std::collections::HashMap;
+use strum::IntoEnumIterator;
 use supabase_wrappers::prelude::*;
 use thiserror::Error;
 
@@ -288,3 +289,22 @@ impl DuckDbFormatter {
         Self {}
     }
 }
+
+pub(crate) trait OptionValidator {
+    fn is_required(&self) -> bool;
+}
+
+pub fn validate_mapping_option<T: IntoEnumIterator + OptionValidator + AsRef<str>>(
+    opt_list: Vec<Option<String>>,
+) -> Result<()> {
+    let valid_options: Vec<String> = T::iter().map(|opt| opt.as_ref().to_string()).collect();
+
+    validate_options(opt_list.clone(), valid_options)?;
+
+    for opt in T::iter() {
+        if opt.is_required() {
+            check_options_contain(&opt_list, opt.as_ref())?;
+        }
+    }
+    Ok(())
+}

src/fdw/csv.rs

@@ -20,11 +20,10 @@ use async_std::task;
 use duckdb::arrow::array::RecordBatch;
 use pgrx::*;
 use std::collections::HashMap;
-use strum::IntoEnumIterator;
 use supabase_wrappers::prelude::*;
 
 use super::base::*;
-use crate::duckdb::csv::CsvOption;
+use crate::duckdb::{csv::CsvOption, secret::UserMappingOptions};
 
 #[wrappers_fdw(
     author = "ParadeDB",
@@ -111,23 +110,14 @@ impl ForeignDataWrapper<BaseFdwError> for CsvFdw {
                 FOREIGN_DATA_WRAPPER_RELATION_ID => {}
                 FOREIGN_SERVER_RELATION_ID => {}
                 FOREIGN_TABLE_RELATION_ID => {
-                    let valid_options: Vec<String> = CsvOption::iter()
-                        .map(|opt| opt.as_ref().to_string())
-                        .collect();
-
-                    validate_options(opt_list.clone(), valid_options)?;
-
-                    for opt in CsvOption::iter() {
-                        if opt.is_required() {
-                            check_options_contain(&opt_list, opt.as_ref())?;
-                        }
-                    }
+                    validate_mapping_option::<CsvOption>(opt_list)?;
+                }
+                USER_MAPPING_RELATION_ID => {
+                    validate_mapping_option::<UserMappingOptions>(opt_list)?;
                 }
-                // TODO: Sanitize user mapping options
                 _ => {}
             }
         }
-
         Ok(())
     }
 

src/fdw/delta.rs

@@ -20,11 +20,10 @@ use async_std::task;
 use duckdb::arrow::array::RecordBatch;
 use pgrx::*;
 use std::collections::HashMap;
-use strum::IntoEnumIterator;
 use supabase_wrappers::prelude::*;
 
 use super::base::*;
-use crate::duckdb::delta::DeltaOption;
+use crate::duckdb::{delta::DeltaOption, secret::UserMappingOptions};
 
 #[wrappers_fdw(
     author = "ParadeDB",
@@ -111,23 +110,14 @@ impl ForeignDataWrapper<BaseFdwError> for DeltaFdw {
                 FOREIGN_DATA_WRAPPER_RELATION_ID => {}
                 FOREIGN_SERVER_RELATION_ID => {}
                 FOREIGN_TABLE_RELATION_ID => {
-                    let valid_options: Vec<String> = DeltaOption::iter()
-                        .map(|opt| opt.as_ref().to_string())
-                        .collect();
-
-                    validate_options(opt_list.clone(), valid_options)?;
-
-                    for opt in DeltaOption::iter() {
-                        if opt.is_required() {
-                            check_options_contain(&opt_list, opt.as_ref())?;
-                        }
-                    }
+                    validate_mapping_option::<DeltaOption>(opt_list)?;
+                }
+                USER_MAPPING_RELATION_ID => {
+                    validate_mapping_option::<UserMappingOptions>(opt_list)?;
                 }
-                // TODO: Sanitize user mapping options
                 _ => {}
             }
         }
-
         Ok(())
     }
 

src/fdw/iceberg.rs

@@ -20,11 +20,10 @@ use async_std::task;
 use duckdb::arrow::array::RecordBatch;
 use pgrx::*;
 use std::collections::HashMap;
-use strum::IntoEnumIterator;
 use supabase_wrappers::prelude::*;
 
 use super::base::*;
-use crate::duckdb::iceberg::IcebergOption;
+use crate::duckdb::{iceberg::IcebergOption, secret::UserMappingOptions};
 
 #[wrappers_fdw(
     author = "ParadeDB",
@@ -111,19 +110,11 @@ impl ForeignDataWrapper<BaseFdwError> for IcebergFdw {
                 FOREIGN_DATA_WRAPPER_RELATION_ID => {}
                 FOREIGN_SERVER_RELATION_ID => {}
                 FOREIGN_TABLE_RELATION_ID => {
-                    let valid_options: Vec<String> = IcebergOption::iter()
-                        .map(|opt| opt.as_ref().to_string())
-                        .collect();
-
-                    validate_options(opt_list.clone(), valid_options)?;
-
-                    for opt in IcebergOption::iter() {
-                        if opt.is_required() {
-                            check_options_contain(&opt_list, opt.as_ref())?;
-                        }
-                    }
+                    validate_mapping_option::<IcebergOption>(opt_list)?;
+                }
+                USER_MAPPING_RELATION_ID => {
+                    validate_mapping_option::<UserMappingOptions>(opt_list)?;
                 }
-                // TODO: Sanitize user mapping options
                 _ => {}
             }
         }

src/fdw/parquet.rs

@@ -20,11 +20,10 @@ use async_std::task;
 use duckdb::arrow::array::RecordBatch;
 use pgrx::*;
 use std::collections::HashMap;
-use strum::IntoEnumIterator;
 use supabase_wrappers::prelude::*;
 
 use super::base::*;
-use crate::duckdb::parquet::ParquetOption;
+use crate::duckdb::{parquet::ParquetOption, secret::UserMappingOptions};
 
 #[wrappers_fdw(
     author = "ParadeDB",
@@ -111,23 +110,14 @@ impl ForeignDataWrapper<BaseFdwError> for ParquetFdw {
                 FOREIGN_DATA_WRAPPER_RELATION_ID => {}
                 FOREIGN_SERVER_RELATION_ID => {}
                 FOREIGN_TABLE_RELATION_ID => {
-                    let valid_options: Vec<String> = ParquetOption::iter()
-                        .map(|opt| opt.as_ref().to_string())
-                        .collect();
-
-                    validate_options(opt_list.clone(), valid_options)?;
-
-                    for opt in ParquetOption::iter() {
-                        if opt.is_required() {
-                            check_options_contain(&opt_list, opt.as_ref())?;
-                        }
-                    }
+                    validate_mapping_option::<ParquetOption>(opt_list)?;
+                }
+                USER_MAPPING_RELATION_ID => {
+                    validate_mapping_option::<UserMappingOptions>(opt_list)?;
                 }
-                // TODO: Sanitize user mapping options
                 _ => {}
             }
         }
-
         Ok(())
     }
 

src/fdw/spatial.rs

@@ -20,11 +20,10 @@ use async_std::task;
 use duckdb::arrow::array::RecordBatch;
 use pgrx::*;
 use std::collections::HashMap;
-use strum::IntoEnumIterator;
 use supabase_wrappers::prelude::*;
 
 use super::base::*;
-use crate::duckdb::spatial::SpatialOption;
+use crate::duckdb::{secret::UserMappingOptions, spatial::SpatialOption};
 
 #[wrappers_fdw(
     author = "ParadeDB",
@@ -111,19 +110,11 @@ impl ForeignDataWrapper<BaseFdwError> for SpatialFdw {
                 FOREIGN_DATA_WRAPPER_RELATION_ID => {}
                 FOREIGN_SERVER_RELATION_ID => {}
                 FOREIGN_TABLE_RELATION_ID => {
-                    let valid_options: Vec<String> = SpatialOption::iter()
-                        .map(|opt| opt.as_ref().to_string())
-                        .collect();
-
-                    validate_options(opt_list.clone(), valid_options)?;
-
-                    for opt in SpatialOption::iter() {
-                        if opt.is_required() {
-                            check_options_contain(&opt_list, opt.as_ref())?;
-                        }
-                    }
+                    validate_mapping_option::<SpatialOption>(opt_list)?;
+                }
+                USER_MAPPING_RELATION_ID => {
+                    validate_mapping_option::<UserMappingOptions>(opt_list)?;
                 }
-                // TODO: Sanitize user mapping options
                 _ => {}
             }
         }