Fix parsing of `direnv export json` to support unset of environment variables + better logging (#32559)

Michael Sloan created 6 months ago

Release Notes:

- Fixed parsing of `direnv export json` output to support unset of
environment variables.

Change summary

crates/project/src/direnv.rs      | 30 +++++++++++++++++++-----------
crates/project/src/environment.rs |  8 +++++++-
2 files changed, 26 insertions(+), 12 deletions(-)

Detailed changes

crates/project/src/direnv.rs

@@ -9,7 +9,6 @@ pub enum DirenvError {
     NotFound,
     FailedRun,
     NonZeroExit(ExitStatus, Vec<u8>),
-    EmptyOutput,
     InvalidJson,
 }
 
@@ -22,7 +21,6 @@ impl From<DirenvError> for Option<EnvironmentErrorMessage> {
                     "Failed to run direnv. See logs for more info",
                 )))
             }
-            DirenvError::EmptyOutput => None,
             DirenvError::InvalidJson => Some(EnvironmentErrorMessage(String::from(
                 "Direnv returned invalid json. See logs for more info",
             ))),
@@ -34,13 +32,14 @@ impl From<DirenvError> for Option<EnvironmentErrorMessage> {
 pub async fn load_direnv_environment(
     env: &HashMap<String, String>,
     dir: &Path,
-) -> Result<HashMap<String, String>, DirenvError> {
+) -> Result<HashMap<String, Option<String>>, DirenvError> {
     let Ok(direnv_path) = which::which("direnv") else {
         return Err(DirenvError::NotFound);
     };
 
-    let Some(direnv_output) = smol::process::Command::new(direnv_path)
-        .args(["export", "json"])
+    let args = &["export", "json"];
+    let Some(direnv_output) = smol::process::Command::new(&direnv_path)
+        .args(args)
         .envs(env)
         .env("TERM", "dumb")
         .current_dir(dir)
@@ -65,12 +64,21 @@ pub async fn load_direnv_environment(
 
     let output = String::from_utf8_lossy(&direnv_output.stdout);
     if output.is_empty() {
-        return Err(DirenvError::EmptyOutput);
+        // direnv outputs nothing when it has no changes to apply to environment variables
+        return Ok(HashMap::new());
     }
 
-    let Some(env) = serde_json::from_str(&output).log_err() else {
-        return Err(DirenvError::InvalidJson);
-    };
-
-    Ok(env)
+    match serde_json::from_str(&output) {
+        Ok(env) => Ok(env),
+        Err(err) => {
+            log::error!(
+                "json parse error {}, while parsing output of `{} {}`:\n{}",
+                err,
+                direnv_path.display(),
+                args.join(" "),
+                output
+            );
+            Err(DirenvError::InvalidJson)
+        }
+    }
 }

crates/project/src/environment.rs

@@ -274,7 +274,13 @@ async fn load_shell_environment(
         },
     };
     if let Some(direnv_environment) = direnv_environment {
-        envs.extend(direnv_environment);
+        for (key, value) in direnv_environment {
+            if let Some(value) = value {
+                envs.insert(key, value);
+            } else {
+                envs.remove(&key);
+            }
+        }
     }
 
     (Some(envs), direnv_error)