allan2 / dotenvy

A well-maintained fork of the Rust dotenv crate
MIT License
625 stars 39 forks source link

Storing a JSON object as an env value leads to a parsing error #84

Open cbeck88 opened 10 months ago

cbeck88 commented 10 months ago

This syntax isn't accepted by dotenvy:

.env:

ACCOUNT={ "r": "1", "a": "2" }

When testing at version 0.15, when dotenvy parses this file, it fails to set the ACCOUNT variable, instead of setting it to the json string payload.

code:

let _ = dotenvy::from_filename(".env");

But storing json in an env file is quite useful.

Meanwhile,

(docker version Docker version 24.0.6, build ed223bc, python dotenv 1.0 (https://pypi.org/project/python-dotenv/)) Node.js v20.5.0 and npm dotenv 16.3: https://www.npmjs.com/package/dotenv)

I didn't test ruby but I'm pretty sure I've seen examples of storing json in env files this way in the context of ruby on rails development.


From my point of view, making rust dotenv parsing match docker run -env-file as closely as possible is pretty important because in 12-factor app methodology, you usually store an env-file with the code for development (and read it with e.g. dotenvy), but then strip it out for deployment, and pass a different env-file to docker / related infrastructure.

If dotenvy fails to parse things that work fine with docker, it may mean that developers commit files that work locally but not in docker, or work in docker but not locally. Then this forces us to create workarounds or make files that work in both parsers, and increases complexity and testing burden.

(Edit: the initial post complained that this parse failure didn't produce a loud error, but further testing shows that it does, it's just that in my actual binary I am ignoring errors from dotenvy, because I want the .env file to be optional but dotenvy reports an error when it isn't found, and that error is an stdio error that I can't easily pattern match away.)


From reading the code, it looks that the problem is that dotenvy sees { and always tries to treat this as a variable substitution expression:

https://github.com/allan2/dotenvy/blob/ce5bfde5d758df2cf26ecd765f26be7219d348b2/dotenv/src/parse.rs#L178

Would you be interested in exploring patches to improve this behavior?

cbeck88 commented 10 months ago

Here's an example of a failing test that I would hope to be passing:

diff --git a/dotenv/src/parse.rs b/dotenv/src/parse.rs
index 8e7842e..d956b08 100644
--- a/dotenv/src/parse.rs
+++ b/dotenv/src/parse.rs
@@ -329,6 +329,38 @@ export   SHELL_LOVER=1
         assert_eq!(count, 13);
     }

+    #[test]
+    fn test_parse_braces_env() {
+        let actual_iter = Iter::new(
+            r#"
+KEY=1
+KEY2="2"
+KEY3='3'
+KEY4={ "foo": "bar" }
+"#
+            .as_bytes(),
+        );
+
+        let expected_iter = vec![
+            ("KEY", "1"),
+            ("KEY2", "2"),
+            ("KEY3", "3"),
+            ("KEY4", "{ \"foo\": \"bar\" }"),
+        ]
+        .into_iter()
+        .map(|(key, value)| (key.to_string(), value.to_string()));
+
+        let mut count = 0;
+        for (expected, actual) in expected_iter.zip(actual_iter) {
+            assert!(actual.is_ok());
+            assert_eq!(expected, actual.unwrap());
+            count += 1;
+        }
+
+        assert_eq!(count, 4);
+    }
+
     #[test]
     fn test_parse_line_comment() {
         let result: Result<Vec<(String, String)>> = Iter::new(
polarathene commented 9 months ago

An alternative crate dotenvs does not reproduce the failure case.

It may be a viable replacement to consider in the meantime?

allan2 commented 8 months ago

Hi @cbeck88, thanks for the detailed issue.

There are a lot of issues popping up about dotenvy not supporting X, which is supported by other dotenv implementations. I'm currently working on a comparison of them to better define the scope of this crate.

Hopefully, this will help map out what "correct" dotenv really means.

In the meantime, I'm inclined to support your example because it seems like has widespread support (like Python and JS like you mentioned, although I have not tested it yet).

A PR would be welcome!