From 778ec3fc187350ddf61ae046fc4cfb7b7b7763b0 Mon Sep 17 00:00:00 2001 From: jonathanwxh-cell Date: Mon, 9 Nov 2026 18:15:12 +0800 Subject: [PATCH] fix(organize): preserve numbered names, indirect resource dicts, and comment threads (#5612) --- crates/organize/src/comments.rs | 81 +++++++++++++++-- crates/organize/src/dedupe.rs | 128 ++++++++++++++++++++++++++- crates/organize/src/import/carry.rs | 72 +++++++++++++-- crates/organize/src/import/fields.rs | 87 ++++++++++++++++-- crates/organize/src/tests.rs | 57 ++++++++++++ 5 files changed, 403 insertions(+), 22 deletions(-) diff --git a/crates/organize/src/comments.rs b/crates/organize/src/comments.rs index dd5d63ebc4..5ab894b48c 100644 --- a/crates/organize/src/comments.rs +++ b/crates/organize/src/comments.rs @@ -54,9 +54,13 @@ pub fn import_comments(dst: &mut Document, src: &Document) -> Result = from.iter().zip(&to).map(|((s, _), (d, _))| (*s, *d)).collect(); let mut incoming: Vec> = pages.iter().map(|(s, _)| page_comments(src, *s)).collect(); - let kept = threads_kept(&incoming); + let kept = threads_kept(src, &incoming); for list in &mut incoming { - list.retain(|c| c.dict.get(b"IRT").is_none_or(|irt| irt.as_ref().is_some_and(|p| kept.contains(&p)))); + list.retain(|c| match irt_parent(src, &c.dict) { + None => true, + Some(Some(p)) => kept.contains(&p), + Some(None) => false, + }); } let mut copier = Copier::new(src, HashMap::new(), pages.iter().copied().collect()); // Every comment's object is chosen before any is copied, so a comment that another one reaches @@ -155,13 +159,34 @@ fn reserve(dst: &mut Document, src: &Document, annots: &mut PageAnnots, mut dict (target, dict) } +/// Resolve `dict`'s `/IRT` entry: `None` when `/IRT` is absent or resolves to `null`, +/// `Some(Some(r))` when it references annotation object `r` (directly or through indirect +/// reference objects), or `Some(None)` for any other non-reference value. +fn irt_parent(src: &Document, dict: &Dict) -> Option> { + let v = dict.get(b"IRT")?; + if matches!(v, Object::Null) { + return None; + } + let Some(mut r) = v.as_ref() else { + return Some(None); + }; + for _ in 0..8 { + match *src.get(r) { + Object::Null => return None, + Object::Ref(next) => r = next, + _ => break, + } + } + Some(Some(r)) +} + /// The comments of `incoming` that go: a reply goes only with the comment it answers, and that one /// only with the comment it answers, up to the first comment of the thread. A thread whose replies /// answer each other in a loop goes, as all of its comments are there. Each comment is visited once. -fn threads_kept(incoming: &[Vec]) -> HashSet { +fn threads_kept(src: &Document, incoming: &[Vec]) -> HashSet { // For each comment: no parent, a parent comment, or a parent that is no comment of the source. let parents: HashMap>> = - incoming.iter().flatten().filter_map(|c| Some((c.entry.as_ref()?, c.dict.get(b"IRT").map(Object::as_ref)))).collect(); + incoming.iter().flatten().filter_map(|c| Some((c.entry.as_ref()?, irt_parent(src, &c.dict)))).collect(); let mut known: HashMap = HashMap::new(); for &start in parents.keys() { let (mut path, mut on_path) = (Vec::new(), HashSet::new()); @@ -202,8 +227,11 @@ fn page_comments(src: &Document, page: ObjRef) -> Vec { list.unwrap_or_default() .into_iter() .filter_map(|entry| { - let dict = src.resolve(&entry).as_dict().cloned()?; + let mut dict = src.resolve(&entry).as_dict().cloned()?; let subtype = dict.get(b"Subtype").map(|s| src.resolve(s)).and_then(|s| s.as_name().map(<[u8]>::to_vec))?; + if let Some(Some(r)) = irt_parent(src, &dict) { + dict.set(b"IRT".to_vec(), Object::Ref(r)); + } COMMENTS.contains(&subtype.as_slice()).then_some(Incoming { entry, dict }) }) .collect() @@ -217,7 +245,8 @@ fn page_popups(src: &Document, page: ObjRef) -> Vec<(ObjRef, ObjRef)> { .filter_map(|entry| { let r = entry.as_ref()?; let d = src.get(r).as_dict().cloned()?; - (d.name(b"Subtype") == Some(b"Popup".as_slice())).then(|| d.reference(b"Parent").map(|p| (r, p)))? + let is_popup = d.get(b"Subtype").map(|s| src.resolve(s)).as_deref().and_then(Object::as_name) == Some(b"Popup".as_slice()); + is_popup.then(|| d.reference(b"Parent").map(|p| (r, p)))? }) .collect() } @@ -265,7 +294,7 @@ impl PageAnnots { if let Some(name) = d.get(b"NM").and_then(|n| dst.resolve(n).as_string().map(|s| s.to_text())) { names.entry(name).or_default().push(r); } - if d.name(b"Subtype") == Some(b"Popup".as_slice()) + if d.get(b"Subtype").map(|s| dst.resolve(s)).as_deref().and_then(Object::as_name) == Some(b"Popup".as_slice()) && let Some(parent) = d.reference(b"Parent") { popups.entry(parent).or_default().push(r); @@ -654,4 +683,42 @@ mod tests { assert_eq!(import_comments(&mut doc, &src).unwrap(), 0); assert_eq!(page_annots(&doc, 0).len(), before); } + + #[test] + fn comment_with_null_irt_and_popup_with_indirect_subtype_are_imported() { + let src = build(&[ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R 8 0 R] /Count 2 /MediaBox [0 0 300 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R 6 0 R 9 0 R 10 0 R 11 0 R 12 0 R 13 0 R 16 0 R] >>", + "<< /Type /Annot /Subtype /Text /NM (root-null-irt) /IRT null /Contents (root comment) /Rect [10 10 30 30] /P 3 0 R >>", + "<< /Type /Annot /Subtype /Text /IRT 14 0 R /Contents (reply to root) /Rect [10 10 30 30] /P 3 0 R >>", + "<< /Type /Annot /Subtype 7 0 R /Parent 4 0 R /Rect [40 40 200 120] /P 3 0 R >>", + "/Popup", + "<< /Type /Page /Parent 2 0 R /Annots [15 0 R] >>", + // Three-reply chain on page 1 whose root comment (15 0 R) is on dropped page 2: + "<< /Type /Annot /Subtype /Text /IRT 15 0 R /Contents (dropped-1) /Rect [10 10 30 30] /P 3 0 R >>", + "<< /Type /Annot /Subtype /Text /IRT 9 0 R /Contents (dropped-2) /Rect [10 10 30 30] /P 3 0 R >>", + "<< /Type /Annot /Subtype /Text /IRT 10 0 R /Contents (dropped-3) /Rect [10 10 30 30] /P 3 0 R >>", + // Two-reply cycle on page 1: + "<< /Type /Annot /Subtype /Text /IRT 13 0 R /Contents (cycle-a) /Rect [10 10 30 30] /P 3 0 R >>", + "<< /Type /Annot /Subtype /Text /IRT 12 0 R /Contents (cycle-b) /Rect [10 10 30 30] /P 3 0 R >>", + "4 0 R", + "<< /Type /Annot /Subtype /Text /Contents (dropped root) /Rect [10 10 30 30] /P 8 0 R >>", + // Comment whose /IRT chains through an intermediate reference (17 0 R -> 18 0 R -> null): + "<< /Type /Annot /Subtype /Text /IRT 17 0 R /Contents (indirect null irt) /Rect [10 10 30 30] /P 3 0 R >>", + "18 0 R", + "null", + ]); + let mut dst = build(&[ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 400] >>", + "<< /Type /Page /Parent 2 0 R >>", + ]); + assert_eq!(import_comments(&mut dst, &src).unwrap(), 5); + let annots = page_annots(&dst, 0); + assert_eq!(annots.len(), 6, "two root comments, reply via indirect IRT, two cycling replies, and the popup"); + let (root_ref, _) = by_contents(&annots, "root comment"); + assert_eq!(by_contents(&annots, "reply to root").1.reference(b"IRT"), Some(*root_ref)); + assert!(by_contents(&annots, "indirect null irt").1.get(b"IRT").is_some()); + } } diff --git a/crates/organize/src/dedupe.rs b/crates/organize/src/dedupe.rs index fe5cb0749d..f242fea0e1 100644 --- a/crates/organize/src/dedupe.rs +++ b/crates/organize/src/dedupe.rs @@ -45,6 +45,39 @@ fn is_resource(o: &Object) -> bool { /// out on purpose: it names optional content groups, whose identity matters. const RESOURCE_CATEGORIES: &[&[u8]] = &[b"ExtGState", b"ColorSpace", b"Pattern", b"Shading", b"XObject", b"Font"]; +/// Whether `d` carries annotation, page, field, or structure identity rather than resource data. +/// Annotations often omit `/Type /Annot` (ISO 32000-2 Table 166 requires `/Subtype` and `/Rect`), +/// whereas resource dictionaries and category maps never carry `/Rect`, `/Popup`, `/IRT`, or `/FT`. +fn is_identity_dict(doc: &Document, d: &pdfcraft_cos::Dict) -> bool { + if let Some(t) = d.get(b"Type").map(|v| doc.resolve(v)).as_deref().and_then(Object::as_name) + && !RESOURCE_TYPES.contains(&t) + { + return true; + } + d.contains(b"Rect") || d.contains(b"Popup") || d.contains(b"IRT") || d.contains(b"FT") || d.contains(b"Parent") +} + +/// Whether a category member has the expected resource shape and is not an identity-bearing +/// dictionary such as an annotation or form field. +fn category_entry(doc: &Document, category: &[u8], value: &Object) -> bool { + let value = doc.resolve(value); + match category { + b"Font" | b"ExtGState" => matches!(&*value, Object::Dict(d) if !is_identity_dict(doc, d)), + b"XObject" => matches!(&*value, Object::Stream(s) if !is_identity_dict(doc, &s.dict)), + b"Pattern" | b"Shading" => match &*value { + Object::Dict(d) => !is_identity_dict(doc, d), + Object::Stream(s) => !is_identity_dict(doc, &s.dict), + _ => false, + }, + b"ColorSpace" => match &*value { + Object::Name(name) => matches!(name.as_slice(), b"DeviceGray" | b"DeviceRGB" | b"DeviceCMYK" | b"Pattern"), + Object::Array(items) => items.first().and_then(Object::as_name).is_some_and(|name| COLOR_SPACE_FAMILIES.contains(&name)), + _ => false, + }, + _ => false, + } +} + /// References listed in any `/Resources` dictionary reachable inside `o` (pages, forms, Type 3 /// fonts, annotation appearances), including the `/Resources` dictionary itself when indirect. fn collect_resource_refs(doc: &Document, o: &Object, out: &mut HashSet) { @@ -69,6 +102,15 @@ fn collect_resource_refs(doc: &Document, o: &Object, out: &mut HashSet) for cat in RESOURCE_CATEGORIES { let entries = res.get(cat).map(|c| doc.resolve(c)); if let Some(entries) = entries.as_ref().and_then(|e| e.as_dict()) { + // A malformed category can point directly at an annotation or list + // annotations inside a category map. Only resource-name maps whose + // members are non-identity resources are containers. + if is_identity_dict(doc, entries) || !entries.iter().all(|(_, value)| category_entry(doc, cat, value)) { + continue; + } + if let Some(Object::Ref(cat_ref)) = res.get(cat) { + out.insert(*cat_ref); + } out.extend(entries.iter().filter_map(|(_, v)| if let Object::Ref(r) = v { Some(*r) } else { None })); } } @@ -189,11 +231,15 @@ fn contains_ref(o: &Object, map: &HashMap) -> bool { /// Merge duplicate resources among `candidates` (into each other or into equal objects already /// in `doc`), rewrite references inside `candidates`, and free the merged-away objects. /// When `index_existing` is false, only candidates are compared (a brand-new document). +/// Repeated candidates are processed once, by complete object reference. /// Returns how many objects were removed. pub fn dedupe_resources(doc: &mut Document, candidates: &[ObjRef], index_existing: bool) -> usize { let cand: HashSet = candidates.iter().copied().collect(); let mut sorted: Vec = candidates.to_vec(); - sorted.sort_unstable_by_key(|r| r.num); + sorted.sort_unstable_by_key(|r| (r.num, r.generation)); + // A repeated candidate must not merge into itself: that would cycle `canon` and free + // the canonical object. Keep generations distinct when normalizing references. + sorted.dedup(); // Existing objects first, so candidates merge into them rather than the reverse. let mut all: Vec<(ObjRef, std::sync::Arc)> = Vec::new(); if index_existing { @@ -273,6 +319,37 @@ mod tests { doc.add(Object::Dict(d)) } + #[test] + fn repeated_candidate_keeps_the_canonical_object() { + for index_existing in [false, true] { + let mut doc = Document::new_empty(); + let r = doc.add(stream(b"glyphs")); + assert_eq!(dedupe_resources(&mut doc, &[r, r], index_existing), 0); + assert_eq!(*doc.get(r), stream(b"glyphs")); + } + } + + #[test] + fn repeated_candidate_with_referrer_terminates_and_keeps_generations() { + for index_existing in [false, true] { + let mut doc = Document::new_empty(); + let r = ObjRef::new(20, 7); + doc.set(r, stream(b"glyphs")); + let s = font(&mut doc, r, "A"); + assert_eq!(dedupe_resources(&mut doc, &[r, r, s], index_existing), 0); + assert_eq!(*doc.get(r), stream(b"glyphs")); + assert_eq!(doc.generation(r.num), 7); + assert_eq!(doc.get(s).as_dict().unwrap().get(b"FontFile"), Some(&Object::Ref(r))); + + let duplicate = ObjRef::new(30, 9); + doc.set(duplicate, stream(b"glyphs")); + let t = font(&mut doc, duplicate, "B"); + assert_eq!(dedupe_resources(&mut doc, &[r, r, s, duplicate, duplicate, t], index_existing), 1); + assert_eq!(*doc.get(r), stream(b"glyphs")); + assert_eq!(doc.get(t).as_dict().unwrap().get(b"FontFile"), Some(&Object::Ref(r))); + } + } + #[test] fn merges_bottom_up_and_keeps_identity_objects() { let mut doc = Document::new_empty(); @@ -367,4 +444,53 @@ mod tests { let cs = doc.get(h).as_dict().unwrap().get(b"CS").unwrap().clone(); assert_eq!(cs, Object::Array(vec![Object::name("ICCBased"), Object::Ref(old)])); } + + #[test] + fn indirect_resource_category_and_resources_dictionaries_merge() { + let mut doc = Document::new_empty(); + let mut gs = Dict::new(); + gs.set(b"CA".to_vec(), Object::Real(0.5)); + let g1 = doc.add(Object::Dict(gs.clone())); + let g2 = doc.add(Object::Dict(gs)); + + let page_with_indirect_res = |doc: &mut Document, g: ObjRef| { + let mut ext = Dict::new(); + ext.set(b"G0".to_vec(), Object::Ref(g)); + let ext_ref = doc.add(Object::Dict(ext)); + let mut res = Dict::new(); + res.set(b"ExtGState".to_vec(), Object::Ref(ext_ref)); + let res_ref = doc.add(Object::Dict(res)); + let mut p = Dict::new(); + p.set(b"Type".to_vec(), Object::name("Page")); + p.set(b"Resources".to_vec(), Object::Ref(res_ref)); + let p_ref = doc.add(Object::Dict(p)); + (ext_ref, res_ref, p_ref) + }; + let (ext1, res1, p1) = page_with_indirect_res(&mut doc, g1); + let (ext2, res2, p2) = page_with_indirect_res(&mut doc, g2); + assert_eq!(dedupe_resources(&mut doc, &[g1, g2, ext1, res1, p1, ext2, res2, p2], false), 3); + assert_eq!(doc.get(p1).as_dict().unwrap().get(b"Resources"), Some(&Object::Ref(res1))); + assert_eq!(doc.get(p2).as_dict().unwrap().get(b"Resources"), Some(&Object::Ref(res1))); + assert_eq!(doc.get(res1).as_dict().unwrap().get(b"ExtGState"), Some(&Object::Ref(ext1))); + + // Distinct font maps (`F1` and `F10` with different fonts on one page vs the same on another) + // deduplicate identical fonts while preserving distinct `F1`/`F10` entries. + let ff1 = doc.add(stream(b"font-f1")); + let ff10 = doc.add(stream(b"font-f10")); + let f1_a = font(&mut doc, ff1, "Helvetica"); + let f10_a = font(&mut doc, ff10, "Courier"); + let f1_b = font(&mut doc, ff1, "Helvetica"); + let f10_b = font(&mut doc, ff10, "Times-Roman"); + let mut fmap_a = Dict::new(); + fmap_a.set(b"F1".to_vec(), Object::Ref(f1_a)); + fmap_a.set(b"F10".to_vec(), Object::Ref(f10_a)); + let fmap_a_ref = doc.add(Object::Dict(fmap_a)); + let mut fmap_b = Dict::new(); + fmap_b.set(b"F1".to_vec(), Object::Ref(f1_b)); + fmap_b.set(b"F10".to_vec(), Object::Ref(f10_b)); + let fmap_b_ref = doc.add(Object::Dict(fmap_b)); + assert_eq!(dedupe_resources(&mut doc, &[f1_a, f10_a, f1_b, f10_b, fmap_a_ref, fmap_b_ref], false), 1); + assert_eq!(doc.get(fmap_b_ref).as_dict().unwrap().get(b"F1"), Some(&Object::Ref(f1_a))); + assert_eq!(doc.get(fmap_b_ref).as_dict().unwrap().get(b"F10"), Some(&Object::Ref(f10_b))); + } } diff --git a/crates/organize/src/import/carry.rs b/crates/organize/src/import/carry.rs index 00fb9bd0b8..fdfb9ab597 100644 --- a/crates/organize/src/import/carry.rs +++ b/crates/organize/src/import/carry.rs @@ -28,14 +28,20 @@ pub(crate) fn set_attachments(dst: &mut Document, mut entries: Vec<(Vec, Obj } // Names are compared and numbered as text: a key written as UTF-16 (a name with an accent // or Chinese letters) must not get ASCII bytes appended to it. + let reserved: std::collections::HashSet = entries.iter().map(|(k, _)| pdfcraft_cos::PdfString::literal(k.clone()).to_text()).collect(); let mut seen = std::collections::HashSet::new(); for (k, _) in entries.iter_mut() { let base = pdfcraft_cos::PdfString::literal(k.clone()).to_text(); let mut text = base.clone(); let mut n = 2; - while !seen.insert(text.clone()) { - text = format!("{base} ({n})"); - n += 1; + if !seen.insert(text.clone()) { + loop { + text = format!("{base} ({n})"); + n += 1; + if !reserved.contains(&text) && seen.insert(text.clone()) { + break; + } + } } if text != base { *k = pdfcraft_cos::PdfString::text(&text).bytes; @@ -141,10 +147,11 @@ fn outline_level(m: &Copier<'_>, first: Option, depth: u8, budget: &mut let target = item.get(b"Dest").cloned().or_else(|| { let a = m.src.resolve(item.get(b"A")?); let a = a.as_dict()?; - (a.name(b"S") == Some(b"GoTo")).then(|| a.get(b"D").cloned()).flatten() + let is_goto = a.get(b"S").map(|s| m.src.resolve(s)).as_deref().and_then(Object::as_name) == Some(b"GoTo"); + is_goto.then(|| a.get(b"D").cloned()).flatten() }); let dest = target.and_then(|t| m.map_dest(&t)); - let open = item.int(b"Count").is_some_and(|c| c > 0); + let open = item.get(b"Count").and_then(|c| m.src.resolve(c).as_int()).is_some_and(|c| c > 0); let children = outline_level(m, item.reference(b"First"), depth + 1, budget); made.push(OutlineNode { title, colour, flags, dest, open, children }); } @@ -235,3 +242,58 @@ pub(super) fn add_outline(doc: &mut Document, marks: &[Mark]) -> Result<(), Orga })?; Ok(()) } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use super::*; + use crate::attachment_keys; + + fn build(bodies: &[&str]) -> Document { + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, b) in bodies.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{b}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", bodies.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", bodies.len() + 1).as_bytes()); + Document::open(Arc::new(out)).expect("opens") + } + + #[test] + fn set_attachments_does_not_clobber_later_numbered_attachment_name() { + let mut doc = build(&[ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>", + "<< /Type /Page /Parent 2 0 R >>", + ]); + let entries = + vec![(b"report.pdf".to_vec(), Object::Null), (b"report.pdf".to_vec(), Object::Null), (b"report.pdf (2)".to_vec(), Object::Null)]; + set_attachments(&mut doc, entries).unwrap(); + assert_eq!(attachment_keys(&doc), ["report.pdf", "report.pdf (2)", "report.pdf (3)"]); + } + + #[test] + fn source_outline_resolves_indirect_goto_action_type_and_count() { + let src = build(&[ + "<< /Type /Catalog /Pages 2 0 R /Outlines 4 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>", + "<< /Type /Page /Parent 2 0 R >>", + "<< /Type /Outlines /First 5 0 R /Last 5 0 R /Count 1 >>", + "<< /Title (Parent) /Parent 4 0 R /First 6 0 R /Last 6 0 R /Count 8 0 R /A << /S 7 0 R /D [3 0 R /Fit] >> >>", + "<< /Title (Child) /Parent 5 0 R /Dest [3 0 R /Fit] >>", + "/GoTo", + "1", + ]); + let nodes = source_outline(&src, HashMap::from([(ObjRef::new(3, 0), ObjRef::new(3, 0))])); + assert_eq!(nodes.len(), 1); + assert!(nodes[0].dest.is_some()); + assert!(nodes[0].open); + } +} diff --git a/crates/organize/src/import/fields.rs b/crates/organize/src/import/fields.rs index 44910d0406..daeaecd3f6 100644 --- a/crates/organize/src/import/fields.rs +++ b/crates/organize/src/import/fields.rs @@ -50,19 +50,26 @@ pub fn combine_field_conflicts(sources: &[&Document]) -> Vec) -> Vec<(ObjRef, String, String, String)> { + let mut reserved = std::collections::HashSet::new(); + let mut seen = std::collections::HashSet::new(); + for &root in roots { + collect_names(doc, root, "", &mut seen, 0, &mut reserved); + } let mut planned = Vec::new(); - let mut walk = RenameWalk::default(); + let mut walk = RenameWalk { reserved, ..RenameWalk::default() }; for &root in roots { plan_renames_under(doc, root, "", taken, &mut walk, 0, &mut planned); } planned } -/// What one [`plan_renames`] call remembers: the nodes it visited, and for each colliding name -/// the suffix to try next. +/// What one [`plan_renames`] call remembers: the nodes it visited, the incoming tree's own +/// names (so an earlier rename does not steal a later sibling's name), and for each colliding +/// name the suffix to try next. #[derive(Default)] struct RenameWalk { seen: HashSet, + reserved: HashSet, next: HashMap, } @@ -88,18 +95,19 @@ fn plan_renames_under( // still taken: resume there instead of at 1. let mut n = walk.next.get(&fqn).copied().unwrap_or(1); let mut variant; + let mut candidate; while { #[cfg(test)] crate::work(1); variant = format!("{name}-{n}"); - taken.contains(&qualified(prefix, &variant)) + candidate = qualified(prefix, &variant); + taken.contains(&candidate) || walk.reserved.contains(&candidate) } { n = n.saturating_add(1); } walk.next.insert(fqn.clone(), n.saturating_add(1)); - let new_fqn = qualified(prefix, &variant); - planned.push((node, variant.clone(), fqn, new_fqn.clone())); - taken.insert(new_fqn); + planned.push((node, variant.clone(), fqn, candidate.clone())); + taken.insert(candidate); name = variant; } else { taken.insert(fqn); @@ -143,15 +151,16 @@ fn field_roots(doc: &Document) -> Result, OrganizeError> { let annots = d.get(b"Annots").map(|a| doc.resolve(a)).and_then(|a| a.as_array().cloned()).unwrap_or_default(); for a in annots.iter() { let Some(w) = doc.resolve(a).as_dict().cloned() else { continue }; - if w.name(b"Subtype") != Some(b"Widget") { + if w.get(b"Subtype").map(|s| doc.resolve(s)).as_deref().and_then(Object::as_name) != Some(b"Widget") { continue; } // Walk up to the field the widget belongs to, however deep it sits; a widget that // is its own field is its own root. let Some(mut root) = a.as_ref() else { continue }; let mut up = 0; + let mut visited = HashSet::from([root]); while let Some(parent) = doc.get(root).as_dict().and_then(|p| p.reference(b"Parent")) { - if up >= 32 || root == parent { + if up >= 32 || !visited.insert(parent) || doc.get(parent).as_dict().is_none() { break; } root = parent; @@ -295,3 +304,63 @@ pub(super) fn register_fields(dst: &mut Document, fields: &[ObjRef]) -> Result<( } Ok(()) } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use super::*; + + fn build(bodies: &[&str]) -> Document { + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, b) in bodies.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{b}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", bodies.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", bodies.len() + 1).as_bytes()); + Document::open(Arc::new(out)).expect("opens") + } + + #[test] + fn renaming_colliding_field_does_not_steal_later_sibling_name() { + let dst = build(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (item) /Rect [0 0 10 10] >>", + ]); + let src = build(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R 5 0 R] >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R 5 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (item) /Rect [0 0 10 10] >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (item-1) /Rect [0 20 10 30] >>", + ]); + // Only "item" collides with dst; "item-1" in src is already unique and must not be stolen. + assert_eq!(field_name_conflicts(&dst, &src), [("item".to_string(), "item-2".to_string())]); + } + + #[test] + fn field_roots_keeps_widget_with_dangling_parent_or_indirect_subtype() { + let dst = build(&[ + "<< /Type /Catalog /Pages 2 0 R >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype 5 0 R /FT /Tx /T (orphan) /Parent 99 0 R /Rect [0 0 10 10] >>", + "/Widget", + ]); + let src = build(&[ + "<< /Type /Catalog /Pages 2 0 R /AcroForm << /Fields [4 0 R] >> >>", + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 100 100] >>", + "<< /Type /Page /Parent 2 0 R /Annots [4 0 R] >>", + "<< /Type /Annot /Subtype /Widget /FT /Tx /T (orphan) /Rect [0 0 10 10] >>", + ]); + assert_eq!(field_name_conflicts(&dst, &src), [("orphan".to_string(), "orphan-1".to_string())]); + } +} diff --git a/crates/organize/src/tests.rs b/crates/organize/src/tests.rs index 4e8aa02c67..c79c3d8667 100644 --- a/crates/organize/src/tests.rs +++ b/crates/organize/src/tests.rs @@ -1747,6 +1747,63 @@ fn bookmarks_report_the_view_their_destination_asks_for() { ); } +#[test] +fn malformed_resource_categories_preserve_annotation_identity_when_combining_and_inserting() { + for typed in [true, false] { + for with_popups in [false, true] { + let mut source = doc_a(); + let page = pages(&source).unwrap()[0].obj; + let mut annot = Dict::new(); + if typed { + annot.set(b"Type".to_vec(), Object::name("Annot")); + } + annot.set(b"Subtype".to_vec(), Object::name("Text")); + annot.set(b"Rect".to_vec(), Object::Array([0, 0, 10, 10].map(Object::Int).to_vec())); + let first = source.add(Object::Dict(annot.clone())); + let second = source.add(Object::Dict(annot.clone())); + if with_popups { + annot.set(b"Subtype".to_vec(), Object::name("Popup")); + let popup1 = source.add(Object::Dict(annot.clone())); + let popup2 = source.add(Object::Dict(annot)); + source.update_dict(first, |d| d.set(b"Popup".to_vec(), Object::Ref(popup1))).unwrap(); + source.update_dict(second, |d| d.set(b"Popup".to_vec(), Object::Ref(popup2))).unwrap(); + } + source + .update_dict(page, |d| { + let mut font_map = Dict::new(); + font_map.set(b"F1".to_vec(), Object::Ref(first)); + font_map.set(b"F2".to_vec(), Object::Ref(second)); + let mut resources = Dict::new(); + resources.set(b"Font".to_vec(), Object::Dict(font_map)); + resources.set(b"ExtGState".to_vec(), Object::Ref(second)); + d.set(b"Resources".to_vec(), Object::Dict(resources)); + d.set(b"Annots".to_vec(), Object::Array(vec![Object::Ref(first), Object::Ref(second)])); + }) + .unwrap(); + let combined = crate::combine(&[("source", &source)]).unwrap(); + let mut inserted = doc_a(); + crate::import_pages(&mut inserted, &source, &[0], 0).unwrap(); + for doc in [combined, inserted] { + let doc = full_roundtrip(&doc); + let page = pages(&doc).unwrap()[0].obj; + let refs = doc.get(page).as_dict().unwrap().get(b"Annots").unwrap().as_array().unwrap().clone(); + assert_eq!(refs.len(), 2); + assert_ne!(refs[0], refs[1], "equal annotations retain their separate identities"); + if with_popups { + let popups: Vec<_> = refs.iter().map(|r| doc.resolve(r).as_dict().unwrap().reference(b"Popup").unwrap()).collect(); + assert_ne!(popups[0], popups[1], "malformed category entries must not merge distinct popups"); + for popup in popups { + assert_eq!(doc.get(popup).as_dict().unwrap().name(b"Subtype"), Some(b"Popup".as_slice())); + } + } + for r in refs { + assert_eq!(doc.resolve(&r).as_dict().unwrap().name(b"Subtype"), Some(b"Text".as_slice())); + } + } + } + } +} + #[test] fn combine_and_insert_share_identical_resources() { let a = doc_a();