From d755bf9e0965e9c3d5a3d7d2e967a8235d077705 Mon Sep 17 00:00:00 2001 From: Filip Gospodinov Date: Fri, 29 Dec 2023 19:18:48 +0100 Subject: [PATCH] preprocess/links: fail for invalid links Before, `mdbook` would continue processing even when errors, such as invalid links, are encountered. Moreover, it would exit with a `0` return code. Such behavior is unexpected and leads to confusion when run in CI. Now, when links that don't point to existing files are encounter and error is returned which yields to `mdbook` exiting with an error code. The change in behavior has revealed that some tests were run with invalid links. --- src/book/book.rs | 17 +++++----- src/preprocess/index.rs | 3 +- src/preprocess/links.rs | 70 +++++++++++++++++++--------------------- tests/dummy_book/mod.rs | 6 ++-- tests/rendered_output.rs | 4 +-- 5 files changed, 49 insertions(+), 51 deletions(-) diff --git a/src/book/book.rs b/src/book/book.rs index 96c70abc..df8503a1 100644 --- a/src/book/book.rs +++ b/src/book/book.rs @@ -102,11 +102,11 @@ impl Book { /// Unlike the `iter()` method, this requires a closure instead of returning /// an iterator. This is because using iterators can possibly allow you /// to have iterator invalidation errors. - pub fn for_each_mut(&mut self, mut func: F) + pub fn for_each_mut(&mut self, mut func: F) -> Result<()> where - F: FnMut(&mut BookItem), + F: FnMut(&mut BookItem) -> Result<()>, { - for_each_mut(&mut func, &mut self.sections); + for_each_mut(&mut func, &mut self.sections) } /// Append a `BookItem` to the `Book`. @@ -116,18 +116,19 @@ impl Book { } } -pub fn for_each_mut<'a, F, I>(func: &mut F, items: I) +pub fn for_each_mut<'a, F, I>(func: &mut F, items: I) -> Result<()> where - F: FnMut(&mut BookItem), + F: FnMut(&mut BookItem) -> Result<()>, I: IntoIterator, { for item in items { if let BookItem::Chapter(ch) = item { - for_each_mut(func, &mut ch.sub_items); + for_each_mut(func, &mut ch.sub_items)?; } - func(item); + func(item)?; } + Ok(()) } /// Enum representing any type of item which can be added to a book. @@ -595,7 +596,7 @@ And here is some \ let num_items = book.iter().count(); let mut visited = 0; - book.for_each_mut(|_| visited += 1); + book.for_each_mut(|_| Ok(visited += 1)).unwrap(); assert_eq!(visited, num_items); } diff --git a/src/preprocess/index.rs b/src/preprocess/index.rs index 004b7eda..94a04518 100644 --- a/src/preprocess/index.rs +++ b/src/preprocess/index.rs @@ -41,7 +41,8 @@ impl Preprocessor for IndexPreprocessor { } } } - }); + Ok(()) + })?; Ok(book) } diff --git a/src/preprocess/links.rs b/src/preprocess/links.rs index 0af21196..04304fd5 100644 --- a/src/preprocess/links.rs +++ b/src/preprocess/links.rs @@ -56,7 +56,7 @@ impl Preprocessor for LinkPreprocessor { let mut chapter_title = ch.name.clone(); let content = - replace_all(&ch.content, base, chapter_path, 0, &mut chapter_title); + replace_all(&ch.content, base, chapter_path, 0, &mut chapter_title)?; ch.content = content; if chapter_title != ch.name { ctx.chapter_titles @@ -65,7 +65,8 @@ impl Preprocessor for LinkPreprocessor { } } } - }); + Ok(()) + })?; Ok(book) } @@ -77,7 +78,7 @@ fn replace_all( source: P2, depth: usize, chapter_title: &mut String, -) -> String +) -> Result where P1: AsRef, P2: AsRef, @@ -93,43 +94,32 @@ where for link in find_links(s) { replaced.push_str(&s[previous_end_index..link.start_index]); - match link.render_with_path(path, chapter_title) { - Ok(new_content) => { - if depth < MAX_LINK_NESTED_DEPTH { - if let Some(rel_path) = link.link_type.relative_path(path) { - replaced.push_str(&replace_all( - &new_content, - rel_path, - source, - depth + 1, - chapter_title, - )); - } else { - replaced.push_str(&new_content); - } - } else { - error!( - "Stack depth exceeded in {}. Check for cyclic includes", - source.display() - ); - } - previous_end_index = link.end_index; - } - Err(e) => { - error!("Error updating \"{}\", {}", link.link_text, e); - for cause in e.chain().skip(1) { - warn!("Caused By: {}", cause); - } - - // This should make sure we include the raw `{{# ... }}` snippet - // in the page content if there are any errors. - previous_end_index = link.start_index; + let new_content = link + .render_with_path(path, chapter_title) + .context(format!("Error updating \"{}\"", link.link_text))?; + if depth < MAX_LINK_NESTED_DEPTH { + if let Some(rel_path) = link.link_type.relative_path(path) { + replaced.push_str(&replace_all( + &new_content, + rel_path, + source, + depth + 1, + chapter_title, + )?); + } else { + replaced.push_str(&new_content); } + } else { + error!( + "Stack depth exceeded in {}. Check for cyclic includes", + source.display() + ); } + previous_end_index = link.end_index; } replaced.push_str(&s[previous_end_index..]); - replaced + Ok(replaced) } #[derive(PartialEq, Debug, Clone)] @@ -444,7 +434,10 @@ mod tests { {{#include file.rs}} << an escaped link! ```"; let mut chapter_title = "test_replace_all_escaped".to_owned(); - assert_eq!(replace_all(start, "", "", 0, &mut chapter_title), end); + assert_eq!( + replace_all(start, "", "", 0, &mut chapter_title).unwrap(), + end + ); } #[test] @@ -456,7 +449,10 @@ mod tests { # My Chapter "; let mut chapter_title = "test_set_chapter_title".to_owned(); - assert_eq!(replace_all(start, "", "", 0, &mut chapter_title), end); + assert_eq!( + replace_all(start, "", "", 0, &mut chapter_title).unwrap(), + end + ); assert_eq!(chapter_title, "My Title"); } diff --git a/tests/dummy_book/mod.rs b/tests/dummy_book/mod.rs index f91ed9f0..05d62554 100644 --- a/tests/dummy_book/mod.rs +++ b/tests/dummy_book/mod.rs @@ -136,10 +136,10 @@ fn recursive_copy, B: AsRef>(from: A, to: B) -> Result<()> pub fn new_copy_of_example_book() -> Result { let temp = TempFileBuilder::new().prefix("guide").tempdir()?; + let manifest_dir = Path::new(env!("CARGO_MANIFEST_DIR")); - let guide = Path::new(env!("CARGO_MANIFEST_DIR")).join("guide"); - - recursive_copy(guide, temp.path())?; + recursive_copy(manifest_dir.join("guide"), temp.path().join("guide"))?; + recursive_copy(manifest_dir.join("examples"), temp.path().join("examples"))?; Ok(temp) } diff --git a/tests/rendered_output.rs b/tests/rendered_output.rs index 7626b9e8..6893dacc 100644 --- a/tests/rendered_output.rs +++ b/tests/rendered_output.rs @@ -420,7 +420,7 @@ Around the world, around the world"]; fn example_book_can_build() { let example_book_dir = dummy_book::new_copy_of_example_book().unwrap(); - let md = MDBook::load(example_book_dir.path()).unwrap(); + let md = MDBook::load(example_book_dir.path().join("guide")).unwrap(); md.build().unwrap(); } @@ -484,7 +484,7 @@ fn first_chapter_is_copied_as_index_even_if_not_first_elem() { #[test] fn theme_dir_overrides_work_correctly() { let book_dir = dummy_book::new_copy_of_example_book().unwrap(); - let book_dir = book_dir.path(); + let book_dir = &book_dir.path().join("guide"); let theme_dir = book_dir.join("theme"); let mut index = mdbook::theme::INDEX.to_vec();