Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
cbeck88 opened this issue Sep 18, 2023 · 3 comments
Open

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

cbeck88 opened this issue Sep 18, 2023 · 3 comments
Labels
good first issue Good for newcomers

Comments

@cbeck88
Copy link

cbeck88 commented Sep 18, 2023

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 run -env-file works fine with this and behaves as expected
  • python dotenv works fine with this and behaves as expected
  • nodejs dotenv works fine with this and behaves as expected

(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:

if c == '{' && substitution_name.is_empty() {

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

@cbeck88
Copy link
Author

cbeck88 commented Sep 18, 2023

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
Copy link

An alternative crate dotenvs does not reproduce the failure case.

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

@allan2
Copy link
Owner

allan2 commented Nov 30, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants