src: make parsing compatible with motdotla/dotenv package #54215

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

@@ -98,127 +109,99 @@ std::string_view trim_spaces(std::string_view input) {
  return input;
 }

-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

@@ -267,4 +250,13 @@ void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) const {
  }
 }

+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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make parsing compatible with motdotla/dotenv package #54215

src: make parsing compatible with motdotla/dotenv package #54215

marekpiechut commented Aug 5, 2024

nodejs-github-bot commented Aug 5, 2024

marekpiechut commented Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

anonrig Aug 5, 2024

marekpiechut Aug 5, 2024

codecov bot commented Aug 5, 2024 •

edited

Loading

marekpiechut commented Aug 28, 2024

	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;

src: make parsing compatible with motdotla/dotenv package #54215

Are you sure you want to change the base?

src: make parsing compatible with motdotla/dotenv package #54215

Conversation

marekpiechut commented Aug 5, 2024

nodejs-github-bot commented Aug 5, 2024

marekpiechut commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 5, 2024 • edited Loading

Codecov Report

marekpiechut commented Aug 28, 2024

codecov bot commented Aug 5, 2024 •

edited

Loading