src: make parsing compatible with motdotla/dotenv package by marekpiechut · Pull Request #54215 · nodejs/node

marekpiechut · 2024-08-05T14:51:43Z

nodejs-github-bot · 2024-08-05T14:51:48Z

Review requested:

@nodejs/gyp

marekpiechut · 2024-08-05T14:57:05Z

Hmm, no idea which subsystem to use here 🤔. I see that previous commits in these files were marked as src. Should I also use that?

anonrig · 2024-08-05T14:54:43Z

src/node_dotenv.cc

+  auto first = input.front();
+  if ((first == '\'' || first == '"' || first == '`') &&
+      input.back() == first) {
+    input = input.substr(1, input.size() - 2);


A more readable way:

input.remove_prefix(1);
input.remove_suffix(2);

src/node_dotenv.cc

anonrig · 2024-08-05T14:56:09Z

src/node_dotenv.cc


-void Dotenv::ParseContent(const std::string_view input) {
-  std::string lines(input);
+std::string_view parse_key(std::string_view key) {


Can you add a comment to what this function does?

anonrig · 2024-08-05T14:57:49Z

src/node_dotenv.cc

+    // Expand \n to newline in double-quote strings
+    size_t pos = 0;
+    auto expanded = std::string(trimmed);
+    while ((pos = expanded.find("\\n", pos)) != std::string_view::npos) {


expanded is a string now. std::string::npos is the correct return value here.

👍Done, thanks

anonrig · 2024-08-05T14:58:24Z

src/node_dotenv.cc

+  if (value.empty()) return "";
+
+  auto trimmed = trim_quotes(value);
+  if (value.front() == '\"' && value.back() == '\"') {


Can you add a documentation to here?
Returning the else statement early will make this function much more readable.

anonrig · 2024-08-05T14:59:24Z

src/node_dotenv.cc

+  std::string::size_type start = 0;
+  std::string::size_type end = 0;
+
+  for (std::string::size_type i = 0; i < input.size(); i++) {


You removed all of the comments in this function, and it is a lot less readable right now. I can't review it with the current state.

This is basically a new function. Old one was doing a lot of back & forth searching for chars and newlines in string and it was hard for me to fit in a fix. This goes through the content of file only once and parses it char by char.

anonrig · 2024-08-05T15:00:29Z

src/node_dotenv.cc

  }
 }

+std::optional<std::string> Dotenv::GetValue(const std::string_view key) const {


This can return std::string_view since the underlying store will not be disposed.

Suggested change

std::optional<std::string> Dotenv::GetValue(const std::string_view key) const {

std::optional<std::string_view> Dotenv::GetValue(const std::string_view key) const {

anonrig · 2024-08-05T15:00:42Z

src/node_dotenv.cc

+  auto match = store_.find(key.data());
+
+  if (match != store_.end()) {
+    return std::optional<std::string>{match->second};


Suggested change

return std::optional<std::string>{match->second};

return match->second;

anonrig · 2024-08-05T15:01:19Z

test/parallel/test-dotenv.js

+
+// These are no longer true, parser is now more strict when it comes to balancing double quotes
+// assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"');
+// assert.strictEqual(process.env.MULTI_NOT_VALID, 'THIS');
+


Suggested change

// These are no longer true, parser is now more strict when it comes to balancing double quotes

// assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"');

// assert.strictEqual(process.env.MULTI_NOT_VALID, 'THIS');

codecov · 2024-08-05T17:47:57Z

Codecov Report

Attention: Patch coverage is 96.20253% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (67f7137) to head (00fa796).
Report is 634 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_dotenv.cc	96.20%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54215   +/-   ##
=======================================
  Coverage   87.09%   87.09%           
=======================================
  Files         647      647           
  Lines      181845   181900   +55     
  Branches    34913    34924   +11     
=======================================
+ Hits       158373   158426   +53     
+ Misses      16751    16749    -2     
- Partials     6721     6725    +4

Files with missing lines	Coverage Δ
src/node_dotenv.h	`100.00% <ø> (ø)`
src/node_dotenv.cc	`84.35% <96.20%> (+3.98%)`	⬆️

... and 27 files with indirect coverage changes

marekpiechut · 2024-08-28T12:07:56Z

Hey @anonrig did you have a chance to take a second look? I've resolved all small things and only thing left is the parsing function that I have rewritten.

If you think that the change is too risky I can revert it, keep the tests and try to fit in the fix in current implementation.

tniessen · 2025-04-10T10:17:32Z

This appears to be waiting for feedback from @nodejs/config.

dotenv: make parsing compatible with motdotla/dotenv package

5e4b537

Fixes: nodejs#54134

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 5, 2024

marekpiechut mentioned this pull request Aug 5, 2024

--env-file does not support inner quotes (does not behave like dotenv) #54134

Open

anonrig reviewed Aug 5, 2024

View reviewed changes

marekpiechut changed the title ~~dotenv: make parsing compatible with motdotla/dotenv package~~ src: make parsing compatible with motdotla/dotenv package Aug 5, 2024

marekpiechut added 2 commits August 5, 2024 18:06

cleanup & add comments

1456714

Fix linter issues

00fa796

avivkeller added the dotenv Issues and PRs related to .env file parsing label Aug 5, 2024

tniessen requested a review from anonrig June 26, 2025 15:05

dario-piotrowicz mentioned this pull request Jul 13, 2025

doc: add new environment variables doc page #59052

Merged

	std::optional<std::string> Dotenv::GetValue(const std::string_view key) const {
	std::optional<std::string_view> Dotenv::GetValue(const std::string_view key) const {

	return std::optional<std::string>{match->second};
	return match->second;

Uh oh!

Conversation

marekpiechut commented Aug 5, 2024

Uh oh!

nodejs-github-bot commented Aug 5, 2024

Uh oh!

marekpiechut commented Aug 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marekpiechut commented Aug 28, 2024

Uh oh!

tniessen commented Apr 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Aug 5, 2024 •

edited

Loading