diff --git a/crates/formats/src/html.rs b/crates/formats/src/html.rs index 4970855ed..f1eb1693c 100644 --- a/crates/formats/src/html.rs +++ b/crates/formats/src/html.rs @@ -1588,17 +1588,20 @@ fn collapse_ws(s: &str) -> String { /// Parse HTML into a document. pub fn import(s: &str) -> Document { + model::begin_import(); model::to_doc(&parse(s)) } /// [`import`], loading pictures that aren't `data:` URIs through `images`. pub fn import_with(s: &str, images: ImageLoader) -> Document { + model::begin_import(); model::to_doc(&parse_with(s, images)) } /// [`import_with`] onto `page` (its size and margins), which tables size to: HTML gives no page /// size, so the caller picks it (the region's paper, #1381). pub fn import_on(s: &str, images: ImageLoader, page: &wordcraft_doc::SectionProps) -> Document { + model::begin_import(); model::to_doc_in(&parse_with(s, images), page.clone()) } diff --git a/crates/formats/src/latex.rs b/crates/formats/src/latex.rs index 18d807fdd..a79f3a9f0 100644 --- a/crates/formats/src/latex.rs +++ b/crates/formats/src/latex.rs @@ -1057,6 +1057,7 @@ fn math_seq(t: &[MTok], pos: &mut usize, depth: usize, in_opt: bool) -> String { /// Import a LaTeX source file. pub fn import(bytes: &[u8]) -> Document { + model::begin_import(); let mut d = model::to_doc(&parse(&crate::txt::decode(bytes.get(..MAX_SOURCE).unwrap_or(bytes)))); d.ensure_nonempty(); d diff --git a/crates/formats/src/lib.rs b/crates/formats/src/lib.rs index 6678b5c95..8af0d2a50 100644 --- a/crates/formats/src/lib.rs +++ b/crates/formats/src/lib.rs @@ -91,5 +91,7 @@ pub fn export_with(ext: &str, doc: &Document, opts: &ExportOptions) -> Option Document { + model::begin_import(); model::to_doc(&parse(text)) } /// [`import`] onto `page`: Markdown gives no page size (#1381). pub fn import_on(text: &str, page: &wordcraft_doc::SectionProps) -> Document { + model::begin_import(); model::to_doc_in(&parse(text), page.clone()) } @@ -1032,6 +1034,9 @@ fn tokenize(s: &str, base: &Fmt, depth: usize, refs: &Refs) -> Vec { let mut f = base.clone(); // A destination too long to keep leaves its text unlinked. f.link = (dest.len() <= model::MAX_LINK).then_some(dest); + if f.link.is_none() { + model::dropped(model::Dropped::Link); + } for x in parse_inlines(&inner, &f, depth + 1, refs) { nodes.push(Node::Inl(x)); } diff --git a/crates/formats/src/model.rs b/crates/formats/src/model.rs index 156269b1b..61ae4aea5 100644 --- a/crates/formats/src/model.rs +++ b/crates/formats/src/model.rs @@ -30,14 +30,72 @@ pub const MAX_LINK: usize = 8 << 10; /// lose their link and font rather than multiply a small file into gigabytes (#3713). const MAX_RUN_STRINGS: usize = 64 << 20; -/// `name` as a span's font, unless it is longer than [`MAX_FONT_NAME`]. +/// `name` as a span's font, unless it is longer than [`MAX_FONT_NAME`] (then reported, see +/// [`dropped`]). pub fn kept_font(name: &str) -> Option> { - (name.len() <= MAX_FONT_NAME).then(|| name.into()) + let kept = (name.len() <= MAX_FONT_NAME).then(|| name.into()); + if kept.is_none() { + dropped(Dropped::Font); + } + kept } -/// `target` as a span's link, unless it is longer than [`MAX_LINK`]. +/// `target` as a span's link, unless it is longer than [`MAX_LINK`] (then reported, see +/// [`dropped`]). pub fn kept_link(target: &str) -> Option> { - (target.len() <= MAX_LINK).then(|| target.into()) + let kept = (target.len() <= MAX_LINK).then(|| target.into()); + if kept.is_none() { + dropped(Dropped::Link); + } + kept +} + +/// What an importer leaves out to stay within a limit (review of #3873: no limit drops data +/// without saying so). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum Dropped { + /// A font name over [`MAX_FONT_NAME`]. + Font, + /// A link target over [`MAX_LINK`]. + Link, + /// Runs' links and fonts past [`MAX_RUN_STRINGS`]. + RunStrings, + /// A style name cut to [`MAX_FONT_NAME`]. + StyleName, +} + +impl Dropped { + fn message(self) -> String { + match self { + Dropped::Font => format!("import: font names longer than {MAX_FONT_NAME} bytes are dropped; that text keeps the surrounding font"), + Dropped::Link => format!("import: links longer than {MAX_LINK} bytes are dropped; their text is kept"), + Dropped::RunStrings => { + format!("import: the document's runs carry more than {MAX_RUN_STRINGS} bytes of links and font names; later runs lose them") + } + Dropped::StyleName => format!("import: style names longer than {MAX_FONT_NAME} bytes are cut short"), + } + } +} + +thread_local! { + /// The kinds of [`Dropped`] already reported for the document being imported on this thread. + /// Per thread because a document is parsed and built on one, and the parsers drop things + /// from places that have no log of their own to hand. + static REPORTED: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// Log that the importer left something of `kind` out: once per kind per imported document +/// ([`begin_import`]), not once per item. +pub(crate) fn dropped(kind: Dropped) { + let bit = 1u8 << (kind as u8); + if REPORTED.with(|r| r.replace(r.get() | bit)) & bit == 0 { + log::warn!("{}", kind.message()); + } +} + +/// Start a new document's reports: each importer's entry point calls this first. +pub(crate) fn begin_import() { + REPORTED.with(|r| r.set(0)); } /// Character formatting the text formats can express. @@ -580,7 +638,12 @@ impl<'a> Builder<'a> { let n = cp.link.as_ref().map_or(0, String::len).saturating_add(cp.font.as_ref().map_or(0, String::len)); match self.run_strings.checked_add(n).filter(|&t| t <= MAX_RUN_STRINGS) { Some(t) => self.run_strings = t, - None => (cp.link, cp.font) = (None, None), + None => { + if cp.link.is_some() || cp.font.is_some() { + dropped(Dropped::RunStrings); + } + (cp.link, cp.font) = (None, None); + } } cp } diff --git a/crates/formats/src/odt.rs b/crates/formats/src/odt.rs index 6be960811..472507f94 100644 --- a/crates/formats/src/odt.rs +++ b/crates/formats/src/odt.rs @@ -2627,6 +2627,7 @@ fn parse_tracked(bytes: &[u8]) -> Result<(Flow, SectionProps, Option, Opti /// Parse an `.odt` package into a document. pub fn import(bytes: &[u8]) -> Result { + model::begin_import(); let (flow, last, track, font, parts) = parse_tracked(bytes)?; let mut doc = model::to_doc(&flow); if font.is_some() { diff --git a/crates/formats/src/rtf.rs b/crates/formats/src/rtf.rs index 7b06aa082..2a9503fbd 100644 --- a/crates/formats/src/rtf.rs +++ b/crates/formats/src/rtf.rs @@ -1654,6 +1654,8 @@ impl Reader { self.commit_style(); } else if self.style_name.len() < model::MAX_FONT_NAME { self.style_name.push(c); + } else { + model::dropped(model::Dropped::StyleName); } } } @@ -2856,6 +2858,7 @@ fn column_grid(rows: &mut [Vec], rights: &[Vec]) -> Vec { /// Parse RTF bytes into a document. pub fn import(bytes: &[u8]) -> Result { + model::begin_import(); let Parsed { flow, last_section, default_font, headers_footers, .. } = parse_full(bytes)?; let mut doc = model::to_doc_in(&flow, last_section); // Each section's own headers and footers; those it leaves out stay linked to the previous. diff --git a/crates/formats/src/sheet.rs b/crates/formats/src/sheet.rs index 97cf1f868..e3ff85f29 100644 --- a/crates/formats/src/sheet.rs +++ b/crates/formats/src/sheet.rs @@ -343,6 +343,8 @@ struct Grid { rows: Vec>, cells: usize, text: TextBudget, + /// A row has been left out for its text (and logged, once per sheet). + refused: bool, } impl Grid { @@ -350,6 +352,15 @@ impl Grid { self.rows.len() >= MAX_ROWS || self.cells >= MAX_CELLS || self.text.0 >= MAX_TEXT_BYTES } + /// Note that a row is left out because its text is over what is left of [`MAX_TEXT_BYTES`]: + /// logged once per sheet, not once per row. + fn refuse(&mut self) { + if !self.refused { + self.refused = true; + log::warn!("sheet: rows with more text than the {MAX_TEXT_BYTES} byte budget has left are left out of the recipient list"); + } + } + /// Keep `row` (trailing blanks trimmed) `times` times, as far as the caps allow. fn push(&mut self, mut row: Vec, times: u64) { while row.last().is_some_and(String::is_empty) { @@ -364,6 +375,7 @@ impl Grid { let keep = row.len().min(MAX_CELLS.saturating_sub(self.cells)); let r = row.get(..keep).unwrap_or_default(); if !self.text.take(r.iter().map(String::len).sum()) { + self.refuse(); return; } self.cells = self.cells.saturating_add(r.len()); @@ -989,6 +1001,7 @@ fn ods_read(xml: &[u8], want: Option) -> Result<(Vec, Vec MAX_TEXT_BYTES.saturating_sub(grid.text.0) { cells.clear(); *budget = None; + grid.refuse(); return; } cells.resize(cells.len() + blanks, String::new()); @@ -1274,8 +1287,25 @@ mod tests { let book = ods(&format!( r#"next"# )); - let (_, rows) = read_rows(&book, Pick::Index(0)).unwrap(); + let ((_, rows), warned) = crate::test_log::warnings(|| read_rows(&book, Pick::Index(0)).unwrap()); assert_eq!(rows, vec![vec!["next".to_string()]]); + // The row left out is said to be (review of #3873). + assert_eq!(warned.iter().filter(|w| w.contains("are left out of the recipient list")).count(), 1, "{warned:?}"); + } + + #[test] + fn rows_over_the_text_budget_are_logged_once_per_sheet() { + let wide = "€".repeat(MAX_CELL_CHARS); + let over = format!( + r#""# + ); + let book = ods(&format!("{over}{over}{over}next")); + let ((_, rows), warned) = crate::test_log::warnings(|| read_rows(&book, Pick::Index(0)).unwrap()); + assert_eq!(rows, vec![vec!["next".to_string()]]); + assert_eq!(warned.iter().filter(|w| w.contains("recipient list")).count(), 1, "one warning, not one per row: {warned:?}"); + let book = ods("fine"); + let (_, warned) = crate::test_log::warnings(|| read_rows(&book, Pick::Index(0)).unwrap()); + assert!(warned.is_empty(), "{warned:?}"); } #[test] diff --git a/crates/formats/src/test_log.rs b/crates/formats/src/test_log.rs new file mode 100644 index 000000000..745ce21fe --- /dev/null +++ b/crates/formats/src/test_log.rs @@ -0,0 +1,36 @@ +//! Test helper: the warnings this crate logs, captured per test thread, so a test can check that +//! a limit which leaves something out says so. + +use std::cell::RefCell; + +struct Capture; + +thread_local! { + static LINES: RefCell> = const { RefCell::new(Vec::new()) }; +} + +impl log::Log for Capture { + fn enabled(&self, m: &log::Metadata) -> bool { + m.level() <= log::Level::Warn + } + + fn log(&self, r: &log::Record) { + if self.enabled(r.metadata()) { + LINES.with(|l| l.borrow_mut().push(r.args().to_string())); + } + } + + fn flush(&self) {} +} + +static CAPTURE: Capture = Capture; + +/// `f`'s result and the warnings logged on this thread while it ran. +pub(crate) fn warnings(f: impl FnOnce() -> T) -> (T, Vec) { + // Only this logger is ever installed in a test binary; a second call keeps it. + let _ = log::set_logger(&CAPTURE); + log::set_max_level(log::LevelFilter::Warn); + LINES.with(|l| l.borrow_mut().clear()); + let out = f(); + (out, LINES.with(|l| std::mem::take(&mut *l.borrow_mut()))) +} diff --git a/crates/formats/src/tests.rs b/crates/formats/src/tests.rs index dc2344534..bc2442d80 100644 --- a/crates/formats/src/tests.rs +++ b/crates/formats/src/tests.rs @@ -958,10 +958,47 @@ fn run_link_and_font_copies_are_budgeted() { let f = model::Fmt { bold: i % 2 == 0, link: Some(link.clone()), ..Default::default() }; p.push_text("a", &f); } - let doc = model::to_doc(&model::Flow { blocks: vec![FBlock::Para(p)], ..Default::default() }); + model::begin_import(); + let (doc, warned) = crate::test_log::warnings(|| model::to_doc(&model::Flow { blocks: vec![FBlock::Para(p)], ..Default::default() })); let paras: Vec<_> = doc.body.iter().filter_map(|b| if let Block::Para(p) = &**b { Some(p) } else { None }).collect(); assert_eq!(paras.iter().map(|p| p.text.len()).sum::(), 20_000, "all text kept"); let runs: Vec<_> = paras.iter().flat_map(|p| p.runs.iter()).collect(); let kept: usize = runs.iter().filter_map(|r| r.props.link.as_ref()).map(String::len).sum(); assert!(kept > 0 && kept <= 64 << 20, "{kept}"); + // The runs that lose theirs are said to, once for the document (review of #3873). + assert_eq!(warned.iter().filter(|w| w.contains("later runs lose them")).count(), 1, "{warned:?}"); +} + +/// Every limit that leaves a font name, link or style name out says so, once per kind per +/// document rather than once per item (review of #3873; the tracker complains of silent loss). +#[test] +fn dropped_names_and_links_are_logged_once_per_document() { + let long = "x".repeat(model::MAX_LINK + 1); + let html = format!( + "

a b c

" + ); + let count = |w: &[String], what: &str| w.iter().filter(|l| l.contains(what)).count(); + for _ in 0..2 { + // Each import is its own document: the second is reported too. + let (doc, warned) = crate::test_log::warnings(|| crate::html::import(&html)); + assert!(doc.body.len() == 1, "the text is kept"); + assert_eq!(count(&warned, "font names longer"), 1, "{warned:?}"); + assert_eq!(count(&warned, "links longer"), 1, "{warned:?}"); + } + let (_, warned) = crate::test_log::warnings(|| crate::html::import("

fine

")); + assert!(warned.is_empty(), "nothing left out, nothing said: {warned:?}"); + + let md = format!("[a](https://e.com/{long}) [b](https://e.com/{long})"); + let (_, warned) = crate::test_log::warnings(|| crate::markdown::import(&md)); + assert_eq!(count(&warned, "links longer"), 1, "{warned:?}"); + + let rtf = + format!("{{\\rtf1{{\\fonttbl{{\\f0 Arial;}}{{\\f1 {long};}}{{\\f2 {long};}}}}{{\\stylesheet{{\\s1 {long};}}{{\\s2 {long};}}}}\\f1 a\\f2 b}}"); + let (_, warned) = crate::test_log::warnings(|| crate::rtf::import(rtf.as_bytes()).unwrap()); + assert_eq!(count(&warned, "font names longer"), 1, "{warned:?}"); + assert_eq!(count(&warned, "style names longer"), 1, "{warned:?}"); + + let tex = format!("\\href{{https://e.com/{long}}}{{a}} \\url{{https://e.com/{long}}}"); + let (_, warned) = crate::test_log::warnings(|| crate::latex::import(tex.as_bytes())); + assert_eq!(count(&warned, "links longer"), 1, "{warned:?}"); }