From 93b8bc0b3fdffec8af4a2413458571b309add189 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Fri, 7 Aug 2026 19:50:14 -0400 Subject: [PATCH 1/4] Return errors for invalid style indexes --- src/spreadsheet/mod.rs | 23 ++++++++ src/spreadsheet/xls.rs | 19 +++++- src/spreadsheet/xlsb.rs | 12 +++- src/spreadsheet/xlsx.rs | 124 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 5 deletions(-) diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index 6ab6f4d..ae71930 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -27,6 +27,25 @@ 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(), + ) + }) +} + #[derive(Error, Debug)] pub(crate) enum SpreadsheetError { /// Error indicating the spreadsheet format is not supported @@ -48,6 +67,10 @@ 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), } pub(crate) trait Spreadsheet { 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..1cda225 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,117 @@ 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_style_workbook_path(); + write_invalid_style_workbook(&path); + + let mut spreadsheet = XlsxSpreadsheet::open(path.to_str().unwrap()).unwrap(); + let result = spreadsheet.read_sheets(&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, + }); + + 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")); + } + + fn invalid_style_workbook_path() -> PathBuf { + let id = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + std::env::temp_dir().join(format!("rusty-sheet-invalid-style-{id}.xlsx")) + } + + 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(); + } +} From f5b73ac95a403313dcd89ee004c61682730e1643 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 12:53:36 -0400 Subject: [PATCH 2/4] Return errors for invalid shared string indexes --- src/extension/writer.rs | 10 +-- src/spreadsheet/mod.rs | 145 ++++++++++++++++++++++++++++++---------- src/spreadsheet/xlsx.rs | 113 ++++++++++++++++++++++++++----- 3 files changed, 212 insertions(+), 56 deletions(-) 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 ae71930..623fbe3 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -46,6 +46,58 @@ pub(crate) fn resolve_number_format( }) } +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 @@ -71,6 +123,10 @@ pub(crate) enum SpreadsheetError { /// 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 { @@ -132,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/xlsx.rs b/src/spreadsheet/xlsx.rs index 1cda225..e983145 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -544,21 +544,11 @@ mod tests { #[test] fn invalid_style_index_returns_error() { - let path = invalid_style_workbook_path(); + 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(&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, - }); + let result = spreadsheet.read_sheets(&default_criteria()); std::fs::remove_file(path).unwrap(); let error = match result { @@ -570,18 +560,50 @@ mod tests { assert!(error.contains("workbook defines 1 styles")); } - fn invalid_style_workbook_path() -> PathBuf { + #[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-style-{id}.xlsx")) + 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 options = + SimpleFileOptions::default().compression_method(zip::CompressionMethod::Deflated); let rows = (2..11) .map(|row| format!("{row}")) .collect::(); @@ -640,4 +662,65 @@ mod tests { } 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(); + } } From 9e8925fc39a3b0d5fb254e11948dc3e442a7a1de Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 12:58:38 -0400 Subject: [PATCH 3/4] Return errors for invalid cell references --- src/spreadsheet/mod.rs | 27 +++++++++ src/spreadsheet/reference.rs | 103 ++++++++++++++++++++--------------- src/spreadsheet/xlsx.rs | 83 +++++++++++++++++++++++++++- 3 files changed, 166 insertions(+), 47 deletions(-) diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index 623fbe3..c9e7fa4 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -46,6 +46,29 @@ pub(crate) fn resolve_number_format( }) } +#[cfg(test)] +mod number_format_tests { + use super::*; + + #[test] + fn invalid_style_with_out_of_range_column_returns_error() { + let error = resolve_number_format( + &[CellType::Number], + "workbook.xlsb", + "Sheet1", + 10, + 16_384, + 999, + ) + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Cell '[workbook.xlsb]Sheet1!R11C16385': invalid style index 999; workbook defines 1 styles" + ); + } +} + pub(crate) fn resolve_shared_string<'a>( shared_strings: &'a [String], mappings: &HashMap, @@ -127,6 +150,10 @@ pub(crate) enum SpreadsheetError { /// 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), + + /// Error indicating a worksheet cell reference is outside the supported spreadsheet bounds. + #[error("Sheet '[{0}]{1}': invalid cell reference '{2}'")] + CellReferenceError(String, String, String), } pub(crate) trait Spreadsheet { diff --git a/src/spreadsheet/reference.rs b/src/spreadsheet/reference.rs index ce9a7f6..c409022 100644 --- a/src/spreadsheet/reference.rs +++ b/src/spreadsheet/reference.rs @@ -1,3 +1,6 @@ +const MAX_COL_INDEX: usize = 16_383; +const MAX_ROW_NUMBER: usize = 1_048_576; + /// Converts a zero-based column index to Excel column letter notation /// /// Uses a precomputed lookup table for optimal performance. Supports @@ -44,40 +47,12 @@ pub(crate) fn col_to_index(letters: &str) -> Option { let mut column = 0usize; for char in letters.chars() { if 'A' <= char && char <= 'Z' { - column = column * 26 + char as usize - 64; + column = column.checked_mul(26)?.checked_add(char as usize - 64)?; } else { break; } } - if column > 0 { - Some(column - 1) - } else { - None - } -} - -/// Converts a zero-based row index to Excel row number string -/// -/// Transforms internal zero-based row indexing into Excel's 1-based row numbering system. -/// This conversion bridges the gap between internal storage (0-based) and Excel's -/// user-facing representation (1-based). -/// -/// The conversion is straightforward: adds 1 to the zero-based index and converts -/// the result to a string. This maintains compatibility with Excel's row numbering -/// convention while using efficient zero-based indexing internally. -/// -/// # Arguments -/// * `index` - Zero-based row index (0 = row 1, 1 = row 2, etc.) -/// -/// # Returns -/// Excel-style row number as a string (1-based) -/// -/// # Examples -/// - 0 -> "1" // First row -/// - 1 -> "2" // Second row -/// - 99 -> "100" // 100th row -pub(crate) fn index_to_row(index: usize) -> String { - (index + 1).to_string() + zero_based_column(column) } /// Converts an Excel row number string to a zero-based row index @@ -89,22 +64,25 @@ pub(crate) fn index_to_row(index: usize) -> String { /// `Some(usize)` containing the zero-based row index if valid, /// `None` if the input cannot be parsed as a valid row number pub(crate) fn row_to_index(row: &str) -> Option { - row.parse::().ok().map(|row| row - 1) + row.parse::().ok().and_then(zero_based_row) } -/// Converts zero-based row and column indices to an Excel-style cell reference +/// Converts zero-based row and column indices to a cell reference /// /// # Arguments /// * `row_index` - Zero-based row index (0 = row 1) /// * `col_index` - Zero-based column index (0 = column "A") /// /// # Returns -/// Excel-style cell reference as a string (e.g., "A1", "B2", "AB100") +/// A1 notation for valid Excel coordinates, or R1C1 notation for out-of-range +/// coordinates so malformed-file error reporting remains panic-free. pub(crate) fn index_to_reference(row_index: usize, col_index: usize) -> String { - let mut reference = String::new(); - reference.push_str(index_to_col(col_index)); - reference.push_str(&index_to_row(row_index)); - reference + let row_number = row_index.saturating_add(1); + if let Some(column) = INDEXES_TO_COLUMNS.get(col_index) { + format!("{column}{row_number}") + } else { + format!("R{row_number}C{}", col_index.saturating_add(1)) + } } /// Converts an Excel-style cell reference to zero-based row and column indices @@ -135,17 +113,32 @@ pub(crate) fn index_to_reference(row_index: usize, col_index: usize) -> String { pub(crate) fn reference_to_index(letters: &str) -> Option<(usize, usize)> { let mut column = 0usize; let mut row = 0usize; + let mut row_started = false; for char in letters.chars() { - if 'A' <= char && char <= 'Z' { - column = column * 26 + char as usize - 64; + if 'A' <= char && char <= 'Z' && !row_started { + column = column.checked_mul(26)?.checked_add(char as usize - 64)?; } else if '0' <= char && char <= '9' { - row = row * 10 + char as usize - 48; + row_started = true; + row = row.checked_mul(10)?.checked_add(char as usize - 48)?; } else { - break; + return None; } } - if column > 0 { - Some((row - 1, column - 1)) + Some((zero_based_row(row)?, zero_based_column(column)?)) +} + +fn zero_based_column(column: usize) -> Option { + let index = column.checked_sub(1)?; + if index <= MAX_COL_INDEX { + Some(index) + } else { + None + } +} + +fn zero_based_row(row: usize) -> Option { + if (1..=MAX_ROW_NUMBER).contains(&row) { + Some(row - 1) } else { None } @@ -1418,4 +1411,26 @@ const INDEXES_TO_COLUMNS: [&'static str; 16384] = [ "XDS", "XDT", "XDU", "XDV", "XDW", "XDX", "XDY", "XDZ", "XEA", "XEB", "XEC", "XED", "XEE", "XEF", "XEG", "XEH", "XEI", "XEJ", "XEK", "XEL", "XEM", "XEN", "XEO", "XEP", "XEQ", "XER", "XES", "XET", "XEU", "XEV", "XEW", "XEX", "XEY", "XEZ", "XFA", "XFB", "XFC", "XFD", -]; \ No newline at end of file +]; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_reference_bounds() { + assert_eq!(col_to_index("XFD"), Some(16_383)); + assert_eq!(row_to_index("1048576"), Some(1_048_575)); + assert_eq!(reference_to_index("XFD1048576"), Some((1_048_575, 16_383))); + } + + #[test] + fn rejects_out_of_bounds_references() { + assert_eq!(col_to_index("XFE"), None); + assert_eq!(row_to_index("0"), None); + assert_eq!(row_to_index("1048577"), None); + assert_eq!(reference_to_index("A0"), None); + assert_eq!(reference_to_index("XFE1"), None); + assert_eq!(reference_to_index("A1048577"), None); + } +} diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index e983145..c0a256e 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -208,9 +208,17 @@ impl Spreadsheet for XlsxSpreadsheet { col_count = 0; } Event::Start(event) if event.name() == TAG_CELL => { - (row, col) = event.get_attribute_value("r")? - .and_then(|reference| reference_to_index(&reference)) - .unwrap_or((row_count, col_count)); + if let Some(reference) = event.get_attribute_value("r")? { + (row, col) = reference_to_index(&reference).ok_or_else(|| { + SpreadsheetError::CellReferenceError( + sheet.file_name.to_owned(), + sheet.name.to_owned(), + reference.to_string(), + ) + })?; + } else { + (row, col) = (row_count, col_count); + } col_count += 1; if sheet.after_row_upper_bound(row) { break; @@ -577,6 +585,23 @@ mod tests { assert!(error.contains("invalid shared string index 999")); } + #[test] + fn invalid_cell_reference_returns_error() { + let path = invalid_workbook_path("cell-reference"); + write_invalid_cell_reference_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 cell reference error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1")); + assert!(error.contains("invalid cell reference 'XFE1'")); + } + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -715,6 +740,58 @@ mod tests { r#" 999 +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn write_invalid_cell_reference_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/worksheets/sheet1.xml", + r#" + + value "#.to_string(), ), ] { From 1ac73cc60c08f9a707d9e5322271682addb9aeb7 Mon Sep 17 00:00:00 2001 From: Carl Patenaude-Poulin Date: Sat, 8 Aug 2026 13:01:52 -0400 Subject: [PATCH 4/4] Return errors for invalid display values --- src/extension/writer.rs | 2 +- src/spreadsheet/cell.rs | 90 ++++++++++++----------------------------- src/spreadsheet/mod.rs | 9 ++++- src/spreadsheet/xlsx.rs | 78 +++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 67 deletions(-) diff --git a/src/extension/writer.rs b/src/extension/writer.rs index 2b5b038..5f82228 100644 --- a/src/extension/writer.rs +++ b/src/extension/writer.rs @@ -43,7 +43,7 @@ pub(super) fn write_to_vector(sheet: &Sheet, column: &Column, cell: &Cell, vecto cell }; match column.kind { - ColumnType::Varchar => vector.insert(row, &cell.to_string()), + ColumnType::Varchar => vector.insert(row, &cell.to_display_string().map_err(mapper)?), ColumnType::Boolean => write_primitive(vector, row, cell.to_boolean()), ColumnType::BigInt => write_primitive(vector, row, cell.to_bigint().map_err(mapper)?), ColumnType::Double => write_primitive(vector, row, cell.to_double().map_err(mapper)?), diff --git a/src/spreadsheet/cell.rs b/src/spreadsheet/cell.rs index 8f4e5c5..715b471 100644 --- a/src/spreadsheet/cell.rs +++ b/src/spreadsheet/cell.rs @@ -243,76 +243,36 @@ impl Cell { _ => Err(format!("parse '{}' to datetime failed", self.value))?, } } -} -impl Display for Cell { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let value = match self.kind { - CellType::Boolean => if self.value == "1" { "true" } else { "false" }.to_owned(), - CellType::NumberDateTime1900 => { - if let Ok(value) = to_datetime_string(&self.value, false) { - value - } else { - panic!( - "Parse cell value '{}' at {} to DateTime(1900) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberDate1900 => { - if let Ok(value) = to_date_string(&self.value, false) { - value - } else { - panic!( - "Parse cell value '{}' at {} to Date(1900) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberDateTime1904 => { - if let Ok(value) = to_datetime_string(&self.value, true) { - value - } else { - panic!( - "Parse cell value '{}' at {} to DateTime(1904) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberDate1904 => { - if let Ok(value) = to_date_string(&self.value, true) { - value - } else { - panic!( - "Parse cell value '{}' at {} to Date(1904) failed", - self.value, - self.reference() - ); - } - } - CellType::NumberTime1900 | CellType::NumberTime1904 => { - if let Ok(value) = to_time_string(&self.value) { - value - } else { - panic!( - "Parse cell value '{}' at {} to Time failed", - self.value, - self.reference() - ); - } - } - CellType::IsoDateTime => self.value.replace("T", " "), - CellType::IsoDuration => self + /// Converts the cell to the user-visible string representation. + pub(crate) fn to_display_string(&self) -> Result { + match self.kind { + CellType::Boolean => Ok(if self.value == "1" { "true" } else { "false" }.to_owned()), + CellType::NumberDateTime1900 => to_datetime_string(&self.value, false) + .map_err(|_| format!("parse '{}' to DateTime(1900) failed", self.value)), + CellType::NumberDate1900 => to_date_string(&self.value, false) + .map_err(|_| format!("parse '{}' to Date(1900) failed", self.value)), + CellType::NumberDateTime1904 => to_datetime_string(&self.value, true) + .map_err(|_| format!("parse '{}' to DateTime(1904) failed", self.value)), + CellType::NumberDate1904 => to_date_string(&self.value, true) + .map_err(|_| format!("parse '{}' to Date(1904) failed", self.value)), + CellType::NumberTime1900 | CellType::NumberTime1904 => to_time_string(&self.value) + .map_err(|_| format!("parse '{}' to Time failed", self.value)), + CellType::IsoDateTime => Ok(self.value.replace("T", " ")), + CellType::IsoDuration => Ok(self .value .replace("PT", "") .replace("H", ":") .replace("M", ":") - .replace("S", ""), - _ => self.value.to_owned(), - }; + .replace("S", "")), + _ => Ok(self.value.to_owned()), + } + } +} + +impl Display for Cell { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let value = self.to_display_string().unwrap_or_else(|_| self.value.to_owned()); write!(f, "{}", value) } } diff --git a/src/spreadsheet/mod.rs b/src/spreadsheet/mod.rs index c9e7fa4..2a49881 100644 --- a/src/spreadsheet/mod.rs +++ b/src/spreadsheet/mod.rs @@ -230,7 +230,14 @@ pub(crate) trait Spreadsheet { cell, )?.to_owned() } else { - cell.to_string() + cell.to_display_string().map_err(|message| { + SpreadsheetError::CellValueError( + file_name.to_owned(), + sheet_name.to_owned(), + cell.reference(), + message, + ) + })? }; Ok(if !criteria.nulls.contains(&value) { value diff --git a/src/spreadsheet/xlsx.rs b/src/spreadsheet/xlsx.rs index c0a256e..b0fe368 100644 --- a/src/spreadsheet/xlsx.rs +++ b/src/spreadsheet/xlsx.rs @@ -602,6 +602,23 @@ mod tests { assert!(error.contains("invalid cell reference 'XFE1'")); } + #[test] + fn invalid_date_header_value_returns_error() { + let path = invalid_workbook_path("date-header"); + write_invalid_date_header_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 date header value error"), + Err(error) => error.to_string(), + }; + assert!(error.contains("Sheet1!A1")); + assert!(error.contains("parse 'not-a-number' to Date(1900) failed")); + } + fn invalid_workbook_path(kind: &str) -> PathBuf { let id = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -792,6 +809,67 @@ mod tests { r#" value +"#.to_string(), + ), + ] { + zip.start_file(name, options).unwrap(); + zip.write_all(content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn write_invalid_date_header_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/styles.xml", + r#" + + +"#.to_string(), + ), + ( + "xl/worksheets/sheet1.xml", + r#" + + not-a-number "#.to_string(), ), ] {