diff --git a/src/extension/writer.rs b/src/extension/writer.rs index 6a5e890..2b5b038 100644 --- a/src/extension/writer.rs +++ b/src/extension/writer.rs @@ -3,15 +3,16 @@ use crate::database::column::Column; use crate::database::column::ColumnType; use crate::error::RustySheetError; +use crate::spreadsheet::SpreadsheetError; use crate::spreadsheet::cell::Cell; use crate::spreadsheet::cell::CellType; +use crate::spreadsheet::resolve_loaded_shared_string; +use crate::spreadsheet::sheet::Sheet; use duckdb::core::FlatVector; use duckdb::core::Inserter; use libduckdb_sys::duckdb_date; use libduckdb_sys::duckdb_time; use libduckdb_sys::duckdb_timestamp; -use crate::spreadsheet::sheet::Sheet; -use crate::spreadsheet::SpreadsheetError; /// Writes a cell value to a DuckDB vector based on column type. /// Handles type conversion and error mapping for different data types. @@ -25,8 +26,9 @@ pub(super) fn write_to_vector(sheet: &Sheet, column: &Column, cell: &Cell, vecto ) }; let cell = if cell.kind == CellType::SharedString { - let index = cell.value.parse::()?; - if let Some(shared_string) = &shared_strings[index] { + if let Some(shared_string) = + resolve_loaded_shared_string(shared_strings, &sheet.file_name, &sheet.name, cell)? + { &Cell { row: cell.row, col: cell.col, diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index 6ab6f4d..623fbe3 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -27,6 +27,77 @@ pub(crate) mod xlsx; pub(crate) mod criteria; pub(crate) mod sheet; +pub(crate) fn resolve_number_format( + number_formats: &[CellType], + file_name: &str, + sheet_name: &str, + row: usize, + col: usize, + style_index: usize, +) -> Result { + number_formats.get(style_index).copied().ok_or_else(|| { + SpreadsheetError::CellStyleIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + crate::spreadsheet::reference::index_to_reference(row, col), + style_index, + number_formats.len(), + ) + }) +} + +pub(crate) fn resolve_shared_string<'a>( + shared_strings: &'a [String], + mappings: &HashMap, + file_name: &str, + sheet_name: &str, + cell: &Cell, +) -> Result<&'a str, RustySheetError> { + let shared_string_index = cell.value.parse::()?; + let mapped_index = mappings.get(&shared_string_index).copied().ok_or_else(|| { + SpreadsheetError::CellSharedStringIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + shared_string_index, + ) + })?; + + shared_strings + .get(mapped_index) + .map(String::as_str) + .ok_or_else(|| { + SpreadsheetError::CellSharedStringIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + shared_string_index, + ) + .into() + }) +} + +pub(crate) fn resolve_loaded_shared_string<'a>( + shared_strings: &'a [Option], + file_name: &str, + sheet_name: &str, + cell: &Cell, +) -> Result, RustySheetError> { + let shared_string_index = cell.value.parse::()?; + shared_strings + .get(shared_string_index) + .map(Option::as_deref) + .ok_or_else(|| { + SpreadsheetError::CellSharedStringIndexError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + shared_string_index, + ) + .into() + }) +} + #[derive(Error, Debug)] pub(crate) enum SpreadsheetError { /// Error indicating the spreadsheet format is not supported @@ -48,6 +119,14 @@ pub(crate) enum SpreadsheetError { /// Error indicating a specific cell value is invalid #[error("Cell '[{0}]{1}!{2}': {3}")] CellValueError(String, String, String, String), + + /// Error indicating a cell style index is outside the workbook's style table. + #[error("Cell '[{0}]{1}!{2}': invalid style index {3}; workbook defines {4} styles")] + CellStyleIndexError(String, String, String, usize, usize), + + /// Error indicating a shared string index is outside the workbook's shared string table. + #[error("Cell '[{0}]{1}!{2}': invalid shared string index {3}")] + CellSharedStringIndexError(String, String, String, usize), } pub(crate) trait Spreadsheet { @@ -109,56 +188,71 @@ pub(crate) trait Spreadsheet { let (shared_strings, mappings) = self.load_shared_strings(Some(shared_indexes))?; let mut tables = Vec::::new(); - for (name, header, data, row_lower_bound, col_lower_bound, col_upper_bound) in sheets.into_iter() { - let names = (col_lower_bound..=col_upper_bound).map(|col| { - let index = col - col_lower_bound; - if let Some(cell) = &header[index] { - let value = if cell.kind == CellType::SharedString { - let id = cell.value.parse::().expect("Shared string index"); - let index = mappings[&id]; - shared_strings[index].to_owned() - } else { - cell.to_string() - }; - if !criteria.nulls.contains(&value) { - value + let file_name = self.name(); + for (sheet_name, header, data, row_lower_bound, col_lower_bound, col_upper_bound) in sheets.into_iter() { + let names = (col_lower_bound..=col_upper_bound) + .map(|col| -> Result { + let index = col - col_lower_bound; + if let Some(cell) = &header[index] { + let value = if cell.kind == CellType::SharedString { + resolve_shared_string( + &shared_strings, + &mappings, + &file_name, + &sheet_name, + cell, + )?.to_owned() + } else { + cell.to_string() + }; + Ok(if !criteria.nulls.contains(&value) { + value + } else { + index_to_col(col).to_owned() + }) } else { - index_to_col(col).to_owned() + Ok(index_to_col(col).to_owned()) } - } else { - index_to_col(col).to_owned() - } - }).collect::>(); + }) + .collect::, _>>()?; let columns = names.iter().zip(data) - .map(|(name, cells)| { + .map(|(column_name, cells)| -> Result { let types = cells.iter() - .map(|cell| { + .map(|cell| -> Result, RustySheetError> { if cell.kind == CellType::SharedString { - let id = cell.value.parse::().expect("Shared string index"); - let index = mappings[&id]; - let value = shared_strings[index].as_str(); - ColumnType::from(if criteria.nulls.contains(value) { - &CellType::Empty - } else { - &cell.kind - }, value) + let value = resolve_shared_string( + &shared_strings, + &mappings, + &file_name, + &sheet_name, + cell, + )?; + Ok(ColumnType::from( + if criteria.nulls.contains(value) { + &CellType::Empty + } else { + &cell.kind + }, + value, + )) } else { - ColumnType::from(&cell.kind, &cell.value) + Ok(ColumnType::from(&cell.kind, &cell.value)) } }) - .collect::>(); - Column { - name: name.to_owned(), - kind: presets.iter() - .find(|(pattern, _)| pattern.matches(name)) + .collect::, _>>()?; + Ok(Column { + name: column_name.to_owned(), + kind: presets + .iter() + .find(|(pattern, _)| pattern.matches(column_name)) .map(|(_, kind)| kind.to_owned()) .unwrap_or(ColumnType::detect(types)), - } + }) }) - .collect::>(); + .collect::, _>>()?; tables.push(Table { - name, + name: sheet_name, columns, row_lower_bound, col_lower_bound, diff --git a/src/spreadsheet/xls.rs b/src/spreadsheet/xls.rs index 0e68ef1..02119c4 100644 --- a/src/spreadsheet/xls.rs +++ b/src/spreadsheet/xls.rs @@ -10,6 +10,7 @@ use crate::spreadsheet::cell::CellType; use crate::spreadsheet::criteria::Criteria; use crate::spreadsheet::excel::load_number_formats; use crate::spreadsheet::reference::index_to_reference; +use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; use crate::spreadsheet::Spreadsheet; use crate::spreadsheet::SpreadsheetError; @@ -201,7 +202,14 @@ impl Spreadsheet for XlsSpreadsheet { } last_row = Some(row); let index = self.reader.read_u16()? as usize; - let kind = self.number_formats[index]; + let kind = resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?; let value = self.reader.read_rk_number()?; sheet.push(Cell { row, @@ -234,7 +242,14 @@ impl Spreadsheet for XlsSpreadsheet { }; let kind = match either { Either::Left(kind) => kind, - Either::Right(index) => self.number_formats[index], + Either::Right(index) => resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?, }; if kind != CellType::Error { if !criteria.nulls.contains(&value) { diff --git a/src/spreadsheet/xlsb.rs b/src/spreadsheet/xlsb.rs index bb1231e..12dafe7 100644 --- a/src/spreadsheet/xlsb.rs +++ b/src/spreadsheet/xlsb.rs @@ -10,6 +10,7 @@ use crate::spreadsheet::criteria::Criteria; use crate::spreadsheet::excel; use crate::spreadsheet::excel::load_relationships; use crate::spreadsheet::reference::index_to_reference; +use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; use crate::spreadsheet::Spreadsheet; use crate::spreadsheet::SpreadsheetError; @@ -221,7 +222,14 @@ impl Spreadsheet for XlsbSpreadsheet { }; let kind = match either { Either::Left(kind) => kind, - Either::Right(index) => (self.number_formats)[index], + Either::Right(index) => resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?, }; if kind != CellType::Error { if !criteria.nulls.contains(&value) { @@ -451,4 +459,4 @@ fn read_rk_cell(reader: &mut Biff12Reader>>) -> }; (Either::Right(index), value) - } \ No newline at end of file + } diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index 026954b..e983145 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -15,6 +15,7 @@ use crate::spreadsheet::excel; use crate::spreadsheet::excel::load_relationships; use crate::spreadsheet::reference::index_to_reference; use crate::spreadsheet::reference::reference_to_index; +use crate::spreadsheet::resolve_number_format; use crate::spreadsheet::sheet::Sheet; use quick_xml::events::Event; use quick_xml::name::QName; @@ -231,7 +232,14 @@ impl Spreadsheet for XlsxSpreadsheet { if let Some(format_id) = event.get_attribute_value("s")? { if kind == CellType::Number && !format_id.is_empty() { let index = format_id.parse::()?; - kind = self.number_formats[index]; + kind = resolve_number_format( + &self.number_formats, + &sheet.file_name, + &sheet.name, + row, + col, + index, + )?; } } } else { @@ -519,3 +527,200 @@ fn read_string_value( }); Ok(text) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::spreadsheet::criteria::Criteria; + use std::collections::HashSet; + use std::fs::File; + use std::io::Write; + use std::path::Path; + use std::path::PathBuf; + use std::time::SystemTime; + use std::time::UNIX_EPOCH; + use zip::write::SimpleFileOptions; + use zip::ZipWriter; + + #[test] + fn invalid_style_index_returns_error() { + let path = invalid_workbook_path("style"); + write_invalid_style_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&default_criteria()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid style index error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A11")); + assert!(error.contains("invalid style index 999")); + assert!(error.contains("workbook defines 1 styles")); + } + + #[test] + fn invalid_shared_string_index_returns_error() { + let path = invalid_workbook_path("shared-string"); + write_invalid_shared_string_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.analyze_sheets(true, &default_criteria(), &Vec::new()); + + std::fs::remove_file(path).unwrap(); + let error = match result { + Ok(_) => panic!("expected invalid shared string index error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A1")); + assert!(error.contains("invalid shared string index 999")); + } + + fn invalid_workbook_path(kind: &str) -> PathBuf { + let id = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + std::env::temp_dir().join(format!("rusty-sheet-invalid-{kind}-{id}.xlsx")) + } + + fn default_criteria() -> Criteria { + Criteria { + sheet_name_patterns: None, + sheet_limit: None, + range: None, + rows_limit: None, + nulls: HashSet::from(["".to_string()]), + error_as_null: false, + skip_empty_rows: false, + end_at_empty_row: false, + spread_merged_cells: false, + } + } + + fn write_invalid_style_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + let rows = (2..11) + .map(|row| format!("{row}")) + .collect::(); + let sheet = format!( + r#" + + value{rows}11 +"# + ); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + + +"#.to_string(), + ), + ( + "xl/styles.xml", + r#" + + +"#.to_string(), + ), + ("xl/worksheets/sheet1.xml", sheet), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn write_invalid_shared_string_workbook(path: &Path) { + let file = File::create(path).unwrap(); + let mut zip = ZipWriter::new(file); + let options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); + + for (name, content) in [ + ( + "[Content_Types].xml", + r#" + + + + + + +"#.to_string(), + ), + ( + "_rels/.rels", + r#" + + +"#.to_string(), + ), + ( + "xl/workbook.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/_rels/workbook.xml.rels", + r#" + + + +"#.to_string(), + ), + ( + "xl/sharedStrings.xml", + r#" + + name +"#.to_string(), + ), + ( + "xl/worksheets/sheet1.xml", + r#" + + 999 +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } +}