From e5ac9474dfc8d0162cb81eefcfff16b73bb1ebe0 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 07:40:55 -0500 Subject: [PATCH] engine: Scenario Summary size limit, bounded solver report names, undo history counts comment and link text (#3308) * engine: bound Scenario Summary size and solver report name searches; charge comment and link text to undo history * address review: count result rows in the scenario summary limit and reply/date vector storage in the history estimate --- crates/engine/src/cmd/solver/reports.rs | 23 +++++++++++++++++++++-- crates/engine/src/cmd/whatif/scenarios.rs | 12 +++++++++++- crates/engine/src/cmd/whatif/tests.rs | 22 ++++++++++++++++++++++ crates/engine/src/lib.rs | 17 +++++++++++++++++ crates/engine/src/tests.rs | 23 +++++++++++++++++++++++ 5 files changed, 94 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/solver/reports.rs b/crates/engine/src/cmd/solver/reports.rs index 19500cab0..c2a5c6d84 100644 --- a/crates/engine/src/cmd/solver/reports.rs +++ b/crates/engine/src/cmd/solver/reports.rs @@ -52,14 +52,19 @@ pub(super) fn abs_ref(c: CellRef) -> String { format!("${}${}", gridcraft_core::col_to_letters(c.col), u64::from(c.row) + 1) } +/// How many cells to the left and above a cell its name is searched for. +const NAME_REACH: u32 = 1_000; + /// A cell's name in the reports: the nearest text to its left and the nearest text above it. pub(super) fn cell_name(sh: &Sheet, c: CellRef) -> String { let text = |c: CellRef| match sh.value(c) { Value::Text(t) if !t.trim().is_empty() => Some(t.to_string()), _ => None, }; - let left = (0..c.col).rev().find_map(|col| text(CellRef::new(c.row, col))); - let top = (0..c.row).rev().find_map(|row| text(CellRef::new(row, c.col))); + // Labels are next to their cells: looking further than this on a sparse sheet only costs time + // (a thousand cells near the last row would each probe a million empty coordinates). + let left = (c.col.saturating_sub(NAME_REACH)..c.col).rev().find_map(|col| text(CellRef::new(c.row, col))); + let top = (c.row.saturating_sub(NAME_REACH)..c.row).rev().find_map(|row| text(CellRef::new(row, c.col))); [left, top].into_iter().flatten().collect::>().join(" ") } @@ -438,3 +443,17 @@ pub(super) fn report(s: &mut Session, p: &Json) -> Result { } Ok(out) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_name_is_searched_only_a_bounded_distance() { + let mut sh = Sheet::new("S".to_string()); + sh.set_cell(CellRef::new(0, 0), gridcraft_model::Cell::value(Value::text("Far"))); + sh.set_cell(CellRef::new(5, 4), gridcraft_model::Cell::value(Value::text("Near"))); + assert_eq!(cell_name(&sh, CellRef::new(5, 5)), "Near"); + assert_eq!(cell_name(&sh, CellRef::new(0, NAME_REACH + 1)), ""); + } +} diff --git a/crates/engine/src/cmd/whatif/scenarios.rs b/crates/engine/src/cmd/whatif/scenarios.rs index 75bf147a0..9ebb1efb5 100644 --- a/crates/engine/src/cmd/whatif/scenarios.rs +++ b/crates/engine/src/cmd/whatif/scenarios.rs @@ -189,6 +189,9 @@ pub(super) fn scenario_delete(s: &mut Session, p: &Json) -> Result { Ok(json!({"deleted": removed.name})) } +/// Most changing-cell-by-scenario cells one Scenario Summary writes. +const MAX_SUMMARY_CELLS: usize = 1_000_000; + pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result { const CMD: &str = "data.scenarioSummary"; let d = s.doc()?; @@ -211,17 +214,24 @@ pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result { // Changing cells of every scenario, first-seen order; per scenario its values. let mut changing: Vec = Vec::new(); let mut per: Vec> = Vec::new(); + let mut seen: std::collections::HashSet = Default::default(); for sc in &scenarios { let (_, cells) = cell_list(&d.wb, &sc.changing, CMD, MAX_SCENARIO_CELLS)?; let mut m = HashMap::new(); for (c, v) in cells.iter().zip(sc.values.iter()) { - if !changing.contains(c) { + if seen.insert(*c) { changing.push(*c); } m.insert(*c, json_value(v, &d.wb)); } per.push(m); } + // The summary has a cell for every changing cell in every scenario, filled from the current + // value where a scenario doesn't set it, so small disjoint scenarios multiply. + // Each result cell is a row of the summary too, with a cell per scenario. + if changing.len().saturating_add(results.len()).saturating_mul(per.len()) > MAX_SUMMARY_CELLS { + return Err(bad(CMD, "the scenarios together change too many cells for a summary")); + } let current: Vec = changing.iter().map(|c| sh.value(*c)).collect(); let current_results: Vec = results.iter().map(|c| sh.value(*c)).collect(); let mut trial = Trial::new(d); diff --git a/crates/engine/src/cmd/whatif/tests.rs b/crates/engine/src/cmd/whatif/tests.rs index 5e5da0e4c..ec3f5262d 100644 --- a/crates/engine/src/cmd/whatif/tests.rs +++ b/crates/engine/src/cmd/whatif/tests.rs @@ -310,3 +310,25 @@ fn consolidate_keys_are_indexed_and_the_output_is_bounded() { let r = s.execute("data.consolidate", json!({"sources": ["A1:A10000", "A1:XFC1"], "destination": "A1"})); assert!(r.unwrap_err().to_string().contains("too large")); } + +#[test] +fn scenario_summary_refuses_a_matrix_over_its_cell_limit() { + let mut s = s(); + // 200 scenarios of 32 disjoint cells: 6,400 changing cells x 200 scenarios = 1.28 million cells. + for i in 0..200 { + let (a, b) = (i * 32 + 1, i * 32 + 32); + s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{a}:A{b}")})).unwrap(); + } + assert!(s.execute("data.scenarioSummary", json!({})).is_err()); +} + +#[test] +fn scenario_summary_counts_result_rows_in_its_cell_limit() { + let mut s = s(); + // 900 scenarios of one distinct cell each are 810,000 cells; 255 result rows take it past the limit. + for i in 0..900 { + s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{}", i + 1)})).unwrap(); + } + let err = s.execute("data.scenarioSummary", json!({"resultCells": "C1:C255"})).unwrap_err().to_string(); + assert!(err.contains("too many cells"), "{err}"); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index c608a8e39..58979526c 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -237,6 +237,22 @@ const JOURNAL_MAX_ENTRIES: usize = 10_000; pub(crate) const JOURNAL_MAX_BYTES: usize = 64 * 1024 * 1024; const UNDO_BYTES_LIMIT: usize = 128 * 1024 * 1024; +/// Comment and hyperlink text of a sheet: a changed sheet's snapshot keeps its own copy of these +/// strings (an edit deep-copies them), so they count against the history budget with the cells. +fn sheet_text_bytes(sheet: &gridcraft_model::Sheet) -> usize { + let comments = sheet.comments.values().fold(0usize, |sum, c| { + // The vectors' own storage counts too: an empty reply still takes an element. + let replies = c.replies.iter().fold(0usize, |n, (a, t)| n.saturating_add(a.len()).saturating_add(t.len())); + let replies = replies.saturating_add(c.replies.capacity().saturating_mul(std::mem::size_of::<(String, String)>())); + let dates = c.dates.iter().fold(0usize, |n, d| n.saturating_add(d.len())); + let dates = dates.saturating_add(c.dates.capacity().saturating_mul(std::mem::size_of::())); + sum.saturating_add(std::mem::size_of::() + c.author.len() + c.text.len() + replies + dates) + }); + sheet.hyperlinks.values().fold(comments, |sum, h| { + sum.saturating_add(std::mem::size_of::() + h.target.len() + h.tooltip.as_ref().map_or(0, String::len)) + }) +} + fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize { let mut bytes = std::mem::size_of::(); for (index, sheet) in older.sheets.iter().enumerate() { @@ -248,6 +264,7 @@ fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize { let empty = gridcraft_model::CellStore::new(); let next = newer.sheets.get(index).map_or(&empty, |next| &next.cells); bytes = bytes.saturating_add(sheet.cells.retained_bytes_against(next)); + bytes = bytes.saturating_add(sheet_text_bytes(sheet)); } // Snapshots share the macro list until a change to it; a snapshot with its own list keeps it. if !older.macros.shares(&newer.macros) { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 55e795a01..6555a1027 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6555,3 +6555,26 @@ fn pivot_new_sheet_destination_respects_workbook_structure_protection() { } assert!(s.execute("insert.pivotTable", json!({"source": "A1:B3", "location": "New"})).is_err()); } + +#[test] +fn history_charges_a_changed_sheets_comment_text() { + let mut s = s(); + s.execute("review.newNote", json!({"cell": "A1", "text": "n".repeat(30_000), "author": "Example"})).unwrap(); + let older = (*s.doc().unwrap().wb).clone(); + s.execute("cell.set", json!({"cell": "B2", "input": "x"})).unwrap(); + let newer = (*s.doc().unwrap().wb).clone(); + // The older snapshot owns the comment text now that the sheet changed under it. + assert!(crate::history_retained_bytes(&older, &newer) >= 30_000); +} + +#[test] +fn history_charges_the_storage_of_empty_comment_replies() { + let mut s = s(); + s.execute("review.newComment", json!({"cell": "A1", "text": "c", "author": "Ann"})).unwrap(); + for _ in 0..500 { + s.execute("review.replyComment", json!({"cell": "A1", "text": "", "author": ""})).unwrap(); + } + let sheet = s.doc().unwrap().wb.sheet(0).unwrap().clone(); + // 500 replies of no text are still 500 elements. + assert!(crate::sheet_text_bytes(&sheet) >= 500 * std::mem::size_of::<(String, String)>()); +}