fix(ownership): pin XLSX excerpt row count, report Excel row numbers

Addresses review on #25: sheet XML omits blank rows, so error messages
now use each <row r=...> number instead of the position; the excerpt
test pins the exact row count so dropped rows fail loudly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017a5Cv5Z1NzdYKgKnTiAF4E
This commit is contained in:
hermes 2026-09-24 21:08:57 +00:00
commit 06d0a7a2ae

View file

@ -155,8 +155,26 @@ fn parse_shared_strings(xml: &str) -> Result<Vec<String>, IdxError> {
Ok(strings) Ok(strings)
} }
/// One sheet row: column letters (`A`, `B`, ...) to cell text. /// One sheet row: its 1-based Excel row number and column letters
type SheetRow = HashMap<String, String>; /// (`A`, `B`, ...) mapped to cell text. Sheet XML omits blank rows, so the
/// number comes from the row's `r` attribute, not its position.
#[derive(Debug, Default)]
struct SheetRow {
number: usize,
cells: HashMap<String, String>,
}
fn row_number(element: &BytesStart<'_>, fallback: usize) -> Result<usize, IdxError> {
for attribute in element.attributes() {
let attribute = attribute.map_err(|e| xml_error("worksheet", e))?;
if attribute.key.local_name().as_ref() == b"r" {
if let Ok(number) = String::from_utf8_lossy(&attribute.value).trim().parse() {
return Ok(number);
}
}
}
Ok(fallback)
}
fn parse_sheet_rows(xml: &str, shared_strings: &[String]) -> Result<Vec<SheetRow>, IdxError> { fn parse_sheet_rows(xml: &str, shared_strings: &[String]) -> Result<Vec<SheetRow>, IdxError> {
let mut reader = Reader::from_str(xml); let mut reader = Reader::from_str(xml);
@ -169,7 +187,12 @@ fn parse_sheet_rows(xml: &str, shared_strings: &[String]) -> Result<Vec<SheetRow
loop { loop {
match reader.read_event() { match reader.read_event() {
Ok(Event::Start(e)) => match e.local_name().as_ref() { Ok(Event::Start(e)) => match e.local_name().as_ref() {
b"row" => row = Some(SheetRow::new()), b"row" => {
row = Some(SheetRow {
number: row_number(&e, rows.len() + 1)?,
cells: HashMap::new(),
})
}
b"c" => { b"c" => {
cell = Some(cell_ref_and_type(&e)?); cell = Some(cell_ref_and_type(&e)?);
value.clear(); value.clear();
@ -177,7 +200,10 @@ fn parse_sheet_rows(xml: &str, shared_strings: &[String]) -> Result<Vec<SheetRow
b"v" | b"t" if cell.is_some() => in_value = true, b"v" | b"t" if cell.is_some() => in_value = true,
_ => {} _ => {}
}, },
Ok(Event::Empty(e)) if e.local_name().as_ref() == b"row" => rows.push(SheetRow::new()), Ok(Event::Empty(e)) if e.local_name().as_ref() == b"row" => rows.push(SheetRow {
number: row_number(&e, rows.len() + 1)?,
cells: HashMap::new(),
}),
Ok(Event::Text(text)) if in_value => { Ok(Event::Text(text)) if in_value => {
let unescaped = text.unescape().map_err(|e| xml_error("worksheet", e))?; let unescaped = text.unescape().map_err(|e| xml_error("worksheet", e))?;
value.push_str(&unescaped); value.push_str(&unescaped);
@ -201,7 +227,7 @@ fn parse_sheet_rows(xml: &str, shared_strings: &[String]) -> Result<Vec<SheetRow
} else { } else {
value.clone() value.clone()
}; };
current_row.insert(column, text); current_row.cells.insert(column, text);
} }
} }
b"row" => { b"row" => {
@ -254,12 +280,12 @@ fn drafts_from_rows(rows: &[SheetRow]) -> Result<Vec<KseiHoldingDraft>, IdxError
})?; })?;
let mut drafts = Vec::new(); let mut drafts = Vec::new();
for (offset, row) in rows[header_index + 1..].iter().enumerate() { for row in &rows[header_index + 1..] {
let line = header_index + offset + 2; let line = row.number;
let field = |name: &str| -> &str { let field = |name: &str| -> &str {
columns columns
.get(name) .get(name)
.and_then(|column| row.get(column)) .and_then(|column| row.cells.get(column))
.map(|value| value.trim()) .map(|value| value.trim())
.unwrap_or("") .unwrap_or("")
}; };
@ -303,7 +329,7 @@ fn drafts_from_rows(rows: &[SheetRow]) -> Result<Vec<KseiHoldingDraft>, IdxError
/// Map header labels to column letters if `row` is exactly the expected header. /// Map header labels to column letters if `row` is exactly the expected header.
fn header_columns(row: &SheetRow) -> Option<HashMap<&'static str, String>> { fn header_columns(row: &SheetRow) -> Option<HashMap<&'static str, String>> {
let mut found: HashMap<&'static str, String> = HashMap::new(); let mut found: HashMap<&'static str, String> = HashMap::new();
for (column, text) in row { for (column, text) in &row.cells {
let mut label = text.trim().to_ascii_uppercase().replace(' ', "_"); let mut label = text.trim().to_ascii_uppercase().replace(' ', "_");
if label.is_empty() { if label.is_empty() {
continue; continue;
@ -393,11 +419,8 @@ mod tests {
#[test] #[test]
fn parses_above1_xlsx_excerpt() { fn parses_above1_xlsx_excerpt() {
let drafts = fixture(); let drafts = fixture();
assert!( // Pinned so a regression that silently drops rows fails loudly.
drafts.len() >= 10, assert_eq!(drafts.len(), 17);
"expected excerpt rows, got {}",
drafts.len()
);
assert!( assert!(
drafts drafts
.iter() .iter()
@ -422,16 +445,24 @@ mod tests {
} }
fn header_row(labels: &[&str]) -> SheetRow { fn header_row(labels: &[&str]) -> SheetRow {
labels sheet_row(1, labels)
.iter() }
.enumerate()
.map(|(index, label)| { fn sheet_row(number: usize, values: &[&str]) -> SheetRow {
( SheetRow {
((b'A' + index as u8) as char).to_string(), number,
label.to_string(), cells: values
) .iter()
}) .enumerate()
.collect() .filter(|(_, value)| !value.is_empty())
.map(|(index, value)| {
(
((b'A' + index as u8) as char).to_string(),
value.to_string(),
)
})
.collect(),
}
} }
#[test] #[test]
@ -441,21 +472,23 @@ mod tests {
let columns = header_columns(&header_row(&labels)).expect("legacy header accepted"); let columns = header_columns(&header_row(&labels)).expect("legacy header accepted");
assert_eq!(columns["INVESTOR_CLASSIFICATION"], "E"); assert_eq!(columns["INVESTOR_CLASSIFICATION"], "E");
let mut row = header_row(&[ let row = sheet_row(
"46112", 2,
"AADI", &[
"ADARO ANDALAN INDONESIA Tbk", "46112",
"GARIBALDI THOHIR", "AADI",
"ID", "ADARO ANDALAN INDONESIA Tbk",
"L ", "GARIBALDI THOHIR",
"INDONESIAN", "ID",
"INDONESIA", "L ",
"454011607", "INDONESIAN",
"0", "INDONESIA",
"454011607", "454011607",
"5.83", "0",
]); "454011607",
row.retain(|_, value| !value.is_empty()); "5.83",
],
);
let drafts = drafts_from_rows(&[header_row(&labels), row]).expect("legacy rows parse"); let drafts = drafts_from_rows(&[header_row(&labels), row]).expect("legacy rows parse");
assert_eq!( assert_eq!(
drafts[0].investor_type.as_ref().map(|t| t.0.as_str()), drafts[0].investor_type.as_ref().map(|t| t.0.as_str()),
@ -468,6 +501,17 @@ mod tests {
assert!(drafts[0].locality.is_some()); assert!(drafts[0].locality.is_some());
} }
#[test]
fn row_errors_report_excel_row_numbers() {
let missing_investor = sheet_row(
709,
&["46265", "BBCA", "BANK CENTRAL ASIA Tbk", "", "Corporate"],
);
let err = drafts_from_rows(&[header_row(&EXPECTED_HEADER), missing_investor])
.expect_err("row without investor rejected");
assert!(err.to_string().contains("row 709"), "{err}");
}
#[test] #[test]
fn rejects_header_with_unknown_or_missing_columns() { fn rejects_header_with_unknown_or_missing_columns() {
let mut labels = EXPECTED_HEADER.to_vec(); let mut labels = EXPECTED_HEADER.to_vec();