Merge pull request #2942 from ehuss/fix-env-config

Add error handling to env config handling

Author: ehuss

CHANGELOG.md

@@ -9,7 +9,10 @@ This release also includes many new features described below.
 
 We have prepared a [0.5 Migration Guide](#05-migration-guide) to help existing authors switch from 0.4.
 
-The final 0.5.0 release does not contain any changes since [0.5.0-beta.2](#mdbook-050-beta2).
+The final 0.5.0 release only contains the following changes since [0.5.0-beta.2](#mdbook-050-beta2):
+
+- Added error handling to environment config handling. This checks that environment variables starting with `MDBOOK_` are correctly specified instead of silently ignoring. This also fixed being able to replace entire top-level tables like `MDBOOK_OUTPUT`.
+  [#2942](https://github.com/rust-lang/mdBook/pull/2942)
 
 ## 0.5 Migration Guide
 
@@ -56,6 +59,10 @@ The following is a summary of the changes that may require your attention when u
   [#2775](https://github.com/rust-lang/mdBook/pull/2775)
 - Removed the very old legacy config support. Warnings have been displayed in previous versions on how to migrate.
   [#2783](https://github.com/rust-lang/mdBook/pull/2783)
+- Top-level config values set from the environment like `MDBOOK_BOOK` now *replace* the contents of the top-level table instead of merging into it.
+  [#2942](https://github.com/rust-lang/mdBook/pull/2942)
+- Invalid environment variables are now rejected. Previously unknown keys like `MDBOOK_FOO` would be ignored, or keys or invalid values inside objects like the `[book]` table would be ignored.
+  [#2942](https://github.com/rust-lang/mdBook/pull/2942)
 
 ### Theme changes
 

crates/mdbook-core/src/config.rs

@@ -123,11 +123,10 @@ impl Config {
     ///
     /// For example:
     ///
-    /// - `MDBOOK_foo` -> `foo`
-    /// - `MDBOOK_FOO` -> `foo`
-    /// - `MDBOOK_FOO__BAR` -> `foo.bar`
-    /// - `MDBOOK_FOO_BAR` -> `foo-bar`
-    /// - `MDBOOK_FOO_bar__baz` -> `foo-bar.baz`
+    /// - `MDBOOK_book` -> `book`
+    /// - `MDBOOK_BOOK` -> `book`
+    /// - `MDBOOK_BOOK__TITLE` -> `book.title`
+    /// - `MDBOOK_BOOK__TEXT_DIRECTION` -> `book.text-direction`
     ///
     /// So by setting the `MDBOOK_BOOK__TITLE` environment variable you can
     /// override the book's title without needing to touch your `book.toml`.
@@ -147,7 +146,7 @@ impl Config {
     /// The latter case may be useful in situations where `mdbook` is invoked
     /// from a script or CI, where it sometimes isn't possible to update the
     /// `book.toml` before building.
-    pub fn update_from_env(&mut self) {
+    pub fn update_from_env(&mut self) -> Result<()> {
         debug!("Updating the config from environment variables");
 
         let overrides =
@@ -162,19 +161,9 @@ impl Config {
             let parsed_value = serde_json::from_str(&value)
                 .unwrap_or_else(|_| serde_json::Value::String(value.to_string()));
 
-            if key == "book" || key == "build" {
-                if let serde_json::Value::Object(ref map) = parsed_value {
-                    // To `set` each `key`, we wrap them as `prefix.key`
-                    for (k, v) in map {
-                        let full_key = format!("{key}.{k}");
-                        self.set(&full_key, v).expect("unreachable");
-                    }
-                    return;
-                }
-            }
-
-            self.set(key, parsed_value).expect("unreachable");
+            self.set(key, parsed_value)?;
         }
+        Ok(())
     }
 
     /// Get a value from the configuration.
@@ -266,24 +255,39 @@ impl Config {
     /// `output.html.playground` will set the "playground" in the html output
     /// table).
     ///
-    /// The only way this can fail is if we can't serialize `value` into a
-    /// `toml::Value`.
+    /// # Errors
+    ///
+    /// This will fail if:
+    ///
+    /// - The value cannot be represented as TOML.
+    /// - The value is not a correct type.
+    /// - The key is an unknown configuration option.
     pub fn set<S: Serialize, I: AsRef<str>>(&mut self, index: I, value: S) -> Result<()> {
         let index = index.as_ref();
 
         let value = Value::try_from(value)
             .with_context(|| "Unable to represent the item as a JSON Value")?;
 
-        if let Some(key) = index.strip_prefix("book.") {
-            self.book.update_value(key, value);
+        if index == "book" {
+            self.book = value.try_into()?;
+        } else if index == "build" {
+            self.build = value.try_into()?;
+        } else if index == "rust" {
+            self.rust = value.try_into()?;
+        } else if index == "output" {
+            self.output = value;
+        } else if index == "preprocessor" {
+            self.preprocessor = value;
+        } else if let Some(key) = index.strip_prefix("book.") {
+            self.book.update_value(key, value)?;
         } else if let Some(key) = index.strip_prefix("build.") {
-            self.build.update_value(key, value);
+            self.build.update_value(key, value)?;
         } else if let Some(key) = index.strip_prefix("rust.") {
-            self.rust.update_value(key, value);
+            self.rust.update_value(key, value)?;
         } else if let Some(key) = index.strip_prefix("output.") {
-            self.output.update_value(key, value);
+            self.output.update_value(key, value)?;
         } else if let Some(key) = index.strip_prefix("preprocessor.") {
-            self.preprocessor.update_value(key, value);
+            self.preprocessor.update_value(key, value)?;
         } else {
             bail!("invalid key `{index}`");
         }
@@ -703,18 +707,13 @@ pub struct SearchChapterSettings {
 /// This is definitely not the most performant way to do things, which means you
 /// should probably keep it away from tight loops...
 trait Updateable<'de>: Serialize + Deserialize<'de> {
-    fn update_value<S: Serialize>(&mut self, key: &str, value: S) {
+    fn update_value<S: Serialize>(&mut self, key: &str, value: S) -> Result<()> {
         let mut raw = Value::try_from(&self).expect("unreachable");
-
-        if let Ok(value) = Value::try_from(value) {
-            raw.insert(key, value);
-        } else {
-            return;
-        }
-
-        if let Ok(updated) = raw.try_into() {
-            *self = updated;
-        }
+        let value = Value::try_from(value)?;
+        raw.insert(key, value);
+        let updated = raw.try_into()?;
+        *self = updated;
+        Ok(())
     }
 }
 

crates/mdbook-driver/src/mdbook.rs

@@ -56,7 +56,7 @@ impl MDBook {
             Config::default()
         };
 
-        config.update_from_env();
+        config.update_from_env()?;
 
         if tracing::enabled!(tracing::Level::TRACE) {
             for line in format!("Config: {config:#?}").lines() {

guide/src/format/configuration/environment-variables.md

@@ -12,11 +12,10 @@ underscore (`_`) is replaced with a dash (`-`).
 
 For example:
 
-- `MDBOOK_foo` -> `foo`
-- `MDBOOK_FOO` -> `foo`
-- `MDBOOK_FOO__BAR` -> `foo.bar`
-- `MDBOOK_FOO_BAR` -> `foo-bar`
-- `MDBOOK_FOO_bar__baz` -> `foo-bar.baz`
+- `MDBOOK_book` -> `book`
+- `MDBOOK_BOOK` -> `book`
+- `MDBOOK_BOOK__TITLE` -> `book.title`
+- `MDBOOK_BOOK__TEXT_DIRECTION` -> `book.text-direction`
 
 So by setting the `MDBOOK_BOOK__TITLE` environment variable you can override the
 book's title without needing to touch your `book.toml`.

tests/testsuite/config.rs

@@ -207,3 +207,137 @@ unknown field `title`, expected `edition`
 "#]]);
         });
 }
+
+// An invalid top-level key in the environment.
+#[test]
+fn env_invalid_config_key() {
+    BookTest::from_dir("config/empty").run("build", |cmd| {
+        cmd.env("MDBOOK_FOO", "testing")
+            .expect_failure()
+            .expect_stdout(str![[""]])
+            .expect_stderr(str![[r#"
+ERROR invalid key `foo`
+
+"#]]);
+    });
+}
+
+// An invalid value in the environment.
+#[test]
+fn env_invalid_value() {
+    BookTest::from_dir("config/empty")
+        .run("build", |cmd| {
+            cmd.env("MDBOOK_BOOK", r#"{"titlez": "typo"}"#)
+                .expect_failure()
+                .expect_stdout(str![[""]])
+                .expect_stderr(str![[r#"
+ERROR unknown field `titlez`, expected one of `title`, `authors`, `description`, `src`, `language`, `text-direction`
+
+
+"#]]);
+        })
+        .run("build", |cmd| {
+            cmd.env("MDBOOK_BOOK__TITLE", r#"{"looks like obj": "abc"}"#)
+                .expect_failure()
+                .expect_stdout(str![[""]])
+                .expect_stderr(str![[r#"
+ERROR invalid type: map, expected a string
+in `title`
+
+
+"#]]);
+        })
+        // This is not valid JSON, so falls back to be interpreted as a string.
+        .run("build", |cmd| {
+            cmd.env("MDBOOK_BOOK__TITLE", r#"{braces}"#)
+                .expect_stdout(str![[""]])
+                .expect_stderr(str![[r#"
+ INFO Book building has started
+ INFO Running the html backend
+ INFO HTML book written to `[ROOT]/book`
+
+"#]]);
+        })
+        .check_file_contains("book/index.html", "<title>Chapter 1 - {braces}</title>");
+}
+
+// Replacing the entire book table from the environment.
+#[test]
+fn env_entire_book_table() {
+    BookTest::init(|_| {})
+        .change_file(
+            "book.toml",
+            "[book]\n\
+             title = \"config title\"\n\
+            ",
+        )
+        .run("build", |cmd| {
+            cmd.env("MDBOOK_BOOK", r#"{"description": "custom description"}"#);
+        })
+        // The book.toml title is removed.
+        .check_file_contains("book/index.html", "<title>Chapter 1</title>")
+        .check_file_contains(
+            "book/index.html",
+            r#"<meta name="description" content="custom description">"#,
+        );
+}
+
+// Replacing the entire output or preprocessor table from the environment.
+#[test]
+fn env_entire_output_preprocessor_table() {
+    BookTest::from_dir("config/empty")
+        .rust_program(
+            "mdbook-my-preprocessor",
+            r#"
+            fn main() {
+                let mut args = std::env::args().skip(1);
+                if args.next().as_deref() == Some("supports") {
+                    return;
+                }
+                use std::io::Read;
+                let mut s = String::new();
+                std::io::stdin().read_to_string(&mut s).unwrap();
+                assert!(s.contains("custom preprocessor config"));
+                println!("{{\"items\": []}}");
+            }
+            "#,
+        )
+        .rust_program(
+            "mdbook-my-output",
+            r#"
+            fn main() {
+                use std::io::Read;
+                let mut s = String::new();
+                std::io::stdin().read_to_string(&mut s).unwrap();
+                assert!(s.contains("custom output config"));
+                eprintln!("preprocessor saw custom config");
+            }
+            "#,
+        )
+        .run("build", |cmd| {
+            let mut paths: Vec<_> =
+                std::env::split_paths(&std::env::var_os("PATH").unwrap_or_default()).collect();
+            paths.push(cmd.dir.clone());
+            let path = std::env::join_paths(paths).unwrap().into_string().unwrap();
+
+            cmd.env(
+                "MDBOOK_OUTPUT",
+                r#"{"my-output": {"foo": "custom output config"}}"#,
+            )
+            .env(
+                "MDBOOK_PREPROCESSOR",
+                r#"{"my-preprocessor": {"foo": "custom preprocessor config"}}"#,
+            )
+            .env("PATH", path)
+            .expect_stdout(str![[""]])
+            .expect_stderr(str![[r#"
+ INFO Book building has started
+ INFO Running the my-output backend
+ INFO Invoking the "my-output" renderer
+preprocessor saw custom config
+
+"#]]);
+        })
+        // No HTML output
+        .check_file_list("book", str![[""]]);
+}