Add error handling to env config handling

This adds several changes to how environment variables are handled to
more closely align with how configs are handled, and to fix an issue
with replacing entire tables. The changes are:

- Top-level tables like `MDBOOK_BOOK` now *replace* the contents of the
  `book` table instead of merging it. This adds consistency with how all
  the other environment objects work.
- Fixed allowing top-level replacement of `MDBOOK_BOOK` and
  `MDBOOK_OUTPUT`. This was inadvertently recently broken.
- Added ability to replace top-level `MDBOOK_RUST`. I don't recall why
  that wasn't included.
- Reject invalid keys like `MDBOOK_FOO`.
- Reject unknown keys, like `MDBOOK_BOOK='{"xyz": 123}'`
- Reject invalid types, like `MDBOOK_BOOK='{"title": 123}'`

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_book` -> `book`
-    /// - `MDBOOK_FOO` -> `foo`
+    /// - `MDBOOK_BOOK` -> `book`
-    /// - `MDBOOK_FOO__BAR` -> `foo.bar`
+    /// - `MDBOOK_BOOK__TITLE` -> `book.title`
-    /// - `MDBOOK_FOO_BAR` -> `foo-bar`
+    /// - `MDBOOK_BOOK__TEXT_DIRECTION` -> `book.text-direction`
-    /// - `MDBOOK_FOO_bar__baz` -> `foo-bar.baz`
     ///
     /// 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" {
+            self.set(key, parsed_value)?;
-                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");
         }
+        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
+    /// # Errors
-    /// `toml::Value`.
+    ///
+    /// 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.") {
+        if index == "book" {
-            self.book.update_value(key, value);
+            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");
-
+        let value = Value::try_from(value)?;
-        if let Ok(value) = Value::try_from(value) {
+        raw.insert(key, value);
-            raw.insert(key, value);
+        let updated = raw.try_into()?;
-        } else {
+        *self = updated;
-            return;
+        Ok(())
-        }
-
-        if let Ok(updated) = raw.try_into() {
-            *self = updated;
-        }
     }
 }
 

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_book` -> `book`
-- `MDBOOK_FOO` -> `foo`
+- `MDBOOK_BOOK` -> `book`
-- `MDBOOK_FOO__BAR` -> `foo.bar`
+- `MDBOOK_BOOK__TITLE` -> `book.title`
-- `MDBOOK_FOO_BAR` -> `foo-bar`
+- `MDBOOK_BOOK__TEXT_DIRECTION` -> `book.text-direction`
-- `MDBOOK_FOO_bar__baz` -> `foo-bar.baz`
 
 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

@@ -216,10 +216,7 @@ fn env_invalid_config_key() {
             .expect_failure()
             .expect_stdout(str![[""]])
             .expect_stderr(str![[r#"
-
+ERROR invalid key `foo`
-thread 'main' ([..]) panicked at [..]
-unreachable: invalid key `foo`
-note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 
 "#]]);
     });
@@ -231,25 +228,26 @@ 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#"
- INFO Book building has started
+ERROR unknown field `titlez`, expected one of `title`, `authors`, `description`, `src`, `language`, `text-direction`
- INFO Running the html backend
+
- INFO HTML book written to `[ROOT]/book`
 
 "#]]);
         })
         .run("build", |cmd| {
             cmd.env("MDBOOK_BOOK__TITLE", r#"{"looks like obj": "abc"}"#)
+                .expect_failure()
                 .expect_stdout(str![[""]])
                 .expect_stderr(str![[r#"
- INFO Book building has started
+ERROR invalid type: map, expected a string
- INFO Running the html backend
+in `title`
- INFO HTML book written to `[ROOT]/book`
+
 
 "#]]);
         })
-        .check_file_contains("book/index.html", "<title>Chapter 1</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![[""]])
@@ -276,7 +274,8 @@ fn env_entire_book_table() {
         .run("build", |cmd| {
             cmd.env("MDBOOK_BOOK", r#"{"description": "custom description"}"#);
         })
-        .check_file_contains("book/index.html", "<title>Chapter 1 - config title</title>")
+        // 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">"#,
@@ -311,6 +310,7 @@ fn env_entire_output_preprocessor_table() {
                 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");
             }
             "#,
         )
@@ -329,14 +329,15 @@ fn env_entire_output_preprocessor_table() {
                 r#"{"my-preprocessor": {"foo": "custom preprocessor config"}}"#,
             )
             .env("PATH", path)
-            .expect_failure()
             .expect_stdout(str![[""]])
             .expect_stderr(str![[r#"
-
+ INFO Book building has started
-thread 'main' ([..]) panicked at [..]
+ INFO Running the my-output backend
-unreachable: invalid key `output`
+ INFO Invoking the "my-output" renderer
-note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+preprocessor saw custom config
 
 "#]]);
-        });
+        })
+        // No HTML output
+        .check_file_list("book", str![[""]]);
 }