Skip to content

Commit 31b6f4e

Browse files
avivkelleravivkeller
committed
src: parse dotenv with the rest of the options
1 parent 9f9069d commit 31b6f4e

8 files changed

+182
-103
lines changed

‎src/node.cc

+46-29
Original file line numberDiff line numberDiff line change
@@ -800,6 +800,49 @@ int ProcessGlobalArgs(std::vector<std::string>* args,
800800

801801
static std::atomic_bool init_called{false};
802802

803+
static ExitCode ProcessEnvFiles(std::vector<std::string>* errors) {
804+
std::vector<DetailedOption<std::string>>& env_files =
805+
per_process::cli_options->per_isolate->per_env->env_files;
806+
if (env_files.empty()) return ExitCode::kNoFailure;
807+
808+
CHECK(!per_process::v8_initialized);
809+
810+
for (const auto& env_file : env_files) {
811+
switch (per_process::dotenv_file.ParsePath(env_file.value)) {
812+
case Dotenv::ParseResult::Valid:
813+
break;
814+
case Dotenv::ParseResult::InvalidContent:
815+
errors->emplace_back(env_file.value + ": invalid format");
816+
break;
817+
case Dotenv::ParseResult::FileError:
818+
if (env_file.flag == "--env-file-if-exists") {
819+
fprintf(stderr,
820+
"%s not found. Continuing without it.\n",
821+
env_file.value.c_str());
822+
} else {
823+
errors->emplace_back(env_file.value + ": not found");
824+
}
825+
break;
826+
default:
827+
UNREACHABLE();
828+
}
829+
}
830+
831+
#if !defined(NODE_WITHOUT_NODE_OPTIONS)
832+
// Parse and process Node.js options from the environment
833+
std::vector<std::string> env_argv =
834+
ParseNodeOptionsEnvVar(per_process::dotenv_file.GetNodeOptions(), errors);
835+
env_argv.insert(env_argv.begin(), per_process::cli_options->cmdline.at(0));
836+
837+
// Process global arguments
838+
const ExitCode exit_code =
839+
ProcessGlobalArgsInternal(&env_argv, nullptr, errors, kAllowedInEnvvar);
840+
if (exit_code != ExitCode::kNoFailure) return exit_code;
841+
#endif
842+
843+
return ExitCode::kNoFailure;
844+
}
845+
803846
// TODO(addaleax): Turn this into a wrapper around InitializeOncePerProcess()
804847
// (with the corresponding additional flags set), then eventually remove this.
805848
static ExitCode InitializeNodeWithArgsInternal(
@@ -851,35 +894,6 @@ static ExitCode InitializeNodeWithArgsInternal(
851894
HandleEnvOptions(per_process::cli_options->per_isolate->per_env);
852895

853896
std::string node_options;
854-
auto env_files = node::Dotenv::GetDataFromArgs(*argv);
855-
856-
if (!env_files.empty()) {
857-
CHECK(!per_process::v8_initialized);
858-
859-
for (const auto& file_data : env_files) {
860-
switch (per_process::dotenv_file.ParsePath(file_data.path)) {
861-
case Dotenv::ParseResult::Valid:
862-
break;
863-
case Dotenv::ParseResult::InvalidContent:
864-
errors->push_back(file_data.path + ": invalid format");
865-
break;
866-
case Dotenv::ParseResult::FileError:
867-
if (file_data.is_optional) {
868-
fprintf(stderr,
869-
"%s not found. Continuing without it.\n",
870-
file_data.path.c_str());
871-
continue;
872-
}
873-
errors->push_back(file_data.path + ": not found");
874-
break;
875-
default:
876-
UNREACHABLE();
877-
}
878-
}
879-
880-
per_process::dotenv_file.AssignNodeOptionsIfAvailable(&node_options);
881-
}
882-
883897
#if !defined(NODE_WITHOUT_NODE_OPTIONS)
884898
if (!(flags & ProcessInitializationFlags::kDisableNodeOptionsEnv)) {
885899
// NODE_OPTIONS environment variable is preferred over the file one.
@@ -915,6 +929,9 @@ static ExitCode InitializeNodeWithArgsInternal(
915929
if (exit_code != ExitCode::kNoFailure) return exit_code;
916930
}
917931

932+
const ExitCode exit_code = ProcessEnvFiles(errors);
933+
if (exit_code != ExitCode::kNoFailure) return exit_code;
934+
918935
// Set the process.title immediately after processing argv if --title is set.
919936
if (!per_process::cli_options->title.empty())
920937
uv_set_process_title(per_process::cli_options->title.c_str());

‎src/node_dotenv.cc

+3-54
Original file line numberDiff line numberDiff line change
@@ -11,54 +11,6 @@ using v8::NewStringType;
1111
using v8::Object;
1212
using v8::String;
1313

14-
std::vector<Dotenv::env_file_data> Dotenv::GetDataFromArgs(
15-
const std::vector<std::string>& args) {
16-
const std::string_view optional_env_file_flag = "--env-file-if-exists";
17-
18-
const auto find_match = [](const std::string& arg) {
19-
return arg == "--" || arg == "--env-file" ||
20-
arg.starts_with("--env-file=") || arg == "--env-file-if-exists" ||
21-
arg.starts_with("--env-file-if-exists=");
22-
};
23-
24-
std::vector<Dotenv::env_file_data> env_files;
25-
// This will be an iterator, pointing to args.end() if no matches are found
26-
auto matched_arg = std::find_if(args.begin(), args.end(), find_match);
27-
28-
while (matched_arg != args.end()) {
29-
if (*matched_arg == "--") {
30-
return env_files;
31-
}
32-
33-
auto equal_char_index = matched_arg->find('=');
34-
35-
if (equal_char_index != std::string::npos) {
36-
// `--env-file=path`
37-
auto flag = matched_arg->substr(0, equal_char_index);
38-
auto file_path = matched_arg->substr(equal_char_index + 1);
39-
40-
struct env_file_data env_file_data = {
41-
file_path, flag.starts_with(optional_env_file_flag)};
42-
env_files.push_back(env_file_data);
43-
} else {
44-
// `--env-file path`
45-
auto file_path = std::next(matched_arg);
46-
47-
if (file_path == args.end()) {
48-
return env_files;
49-
}
50-
51-
struct env_file_data env_file_data = {
52-
*file_path, matched_arg->starts_with(optional_env_file_flag)};
53-
env_files.push_back(env_file_data);
54-
}
55-
56-
matched_arg = std::find_if(++matched_arg, args.end(), find_match);
57-
}
58-
59-
return env_files;
60-
}
61-
6214
void Dotenv::SetEnvironment(node::Environment* env) {
6315
auto isolate = env->isolate();
6416

@@ -277,12 +229,9 @@ Dotenv::ParseResult Dotenv::ParsePath(const std::string_view path) {
277229
return ParseResult::Valid;
278230
}
279231

280-
void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) const {
281-
auto match = store_.find("NODE_OPTIONS");
282-
283-
if (match != store_.end()) {
284-
*node_options = match->second;
285-
}
232+
std::string Dotenv::GetNodeOptions() const {
233+
auto it = store_.find("NODE_OPTIONS");
234+
return (it != store_.end()) ? it->second : "";
286235
}
287236

288237
} // namespace node

‎src/node_dotenv.h

+1-4
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,10 @@ class Dotenv {
2727

2828
void ParseContent(const std::string_view content);
2929
ParseResult ParsePath(const std::string_view path);
30-
void AssignNodeOptionsIfAvailable(std::string* node_options) const;
30+
std::string GetNodeOptions() const;
3131
void SetEnvironment(Environment* env);
3232
v8::Local<v8::Object> ToObject(Environment* env) const;
3333

34-
static std::vector<env_file_data> GetDataFromArgs(
35-
const std::vector<std::string>& args);
36-
3734
private:
3835
std::map<std::string, std::string> store_;
3936
};

‎src/node_options-inl.h

+21
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,23 @@ void OptionsParser<Options>::AddOption(
9393
});
9494
}
9595

96+
template <typename Options>
97+
void OptionsParser<Options>::AddOption(
98+
const char* name,
99+
const char* help_text,
100+
std::vector<DetailedOption<std::string>> Options::*field,
101+
OptionEnvvarSettings env_setting) {
102+
options_.emplace(
103+
name,
104+
OptionInfo{
105+
kDetailedStringList,
106+
std::make_shared<
107+
SimpleOptionField<std::vector<DetailedOption<std::string>>>>(
108+
field),
109+
env_setting,
110+
help_text});
111+
}
112+
96113
template <typename Options>
97114
void OptionsParser<Options>::AddOption(const char* name,
98115
const char* help_text,
@@ -466,6 +483,10 @@ void OptionsParser<Options>::Parse(
466483
Lookup<std::vector<std::string>>(info.field, options)
467484
->emplace_back(std::move(value));
468485
break;
486+
case kDetailedStringList:
487+
Lookup<std::vector<DetailedOption<std::string>>>(info.field, options)
488+
->emplace_back(DetailedOption<std::string>{value, name});
489+
break;
469490
case kHostPort:
470491
Lookup<HostPort>(info.field, options)
471492
->Update(SplitHostPort(value, errors));

‎src/node_options.cc

+35-2
Original file line numberDiff line numberDiff line change
@@ -643,11 +643,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
643643
"[has_env_file_string]", "", &EnvironmentOptions::has_env_file_string);
644644
AddOption("--env-file",
645645
"set environment variables from supplied file",
646-
&EnvironmentOptions::env_file);
646+
&EnvironmentOptions::env_files);
647647
Implies("--env-file", "[has_env_file_string]");
648648
AddOption("--env-file-if-exists",
649649
"set environment variables from supplied file",
650-
&EnvironmentOptions::optional_env_file);
650+
&EnvironmentOptions::env_files);
651651
Implies("--env-file-if-exists", "[has_env_file_string]");
652652
AddOption("--test",
653653
"launch test runner on startup",
@@ -1341,6 +1341,39 @@ void GetCLIOptionsValues(const FunctionCallbackInfo<Value>& args) {
13411341
return;
13421342
}
13431343
break;
1344+
case kDetailedStringList: {
1345+
const std::vector<DetailedOption<std::string>>& detailed_options =
1346+
*_ppop_instance.Lookup<std::vector<DetailedOption<std::string>>>(
1347+
field, opts);
1348+
v8::Local<v8::Array> value_arr =
1349+
v8::Array::New(isolate, detailed_options.size());
1350+
for (size_t i = 0; i < detailed_options.size(); ++i) {
1351+
// Create a new V8 object for each DetailedOption
1352+
v8::Local<v8::Object> option_object = v8::Object::New(isolate);
1353+
1354+
option_object
1355+
->Set(isolate->GetCurrentContext(),
1356+
v8::String::NewFromUtf8(isolate, "flag").ToLocalChecked(),
1357+
v8::String::NewFromUtf8(isolate,
1358+
detailed_options[i].flag.c_str())
1359+
.ToLocalChecked())
1360+
.Check();
1361+
1362+
option_object
1363+
->Set(isolate->GetCurrentContext(),
1364+
v8::String::NewFromUtf8(isolate, "value").ToLocalChecked(),
1365+
v8::String::NewFromUtf8(isolate,
1366+
detailed_options[i].value.c_str())
1367+
.ToLocalChecked())
1368+
.Check();
1369+
1370+
// Add the object to the array at the current index
1371+
value_arr->Set(isolate->GetCurrentContext(), i, option_object)
1372+
.Check();
1373+
}
1374+
value = value_arr;
1375+
break;
1376+
}
13441377
case kHostPort: {
13451378
const HostPort& host_port =
13461379
*_ppop_instance.Lookup<HostPort>(field, opts);

‎src/node_options.h

+16-2
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,16 @@ class HostPort {
4444
uint16_t port_;
4545
};
4646

47+
template <typename ValueType>
48+
class DetailedOption {
49+
public:
50+
DetailedOption(const ValueType& value, std::string flag)
51+
: value(value), flag(flag) {}
52+
53+
ValueType value;
54+
std::string flag;
55+
};
56+
4757
class Options {
4858
public:
4959
virtual void CheckOptions(std::vector<std::string>* errors,
@@ -177,8 +187,7 @@ class EnvironmentOptions : public Options {
177187
#endif // HAVE_INSPECTOR
178188
std::string redirect_warnings;
179189
std::string diagnostic_dir;
180-
std::string env_file;
181-
std::string optional_env_file;
190+
std::vector<DetailedOption<std::string>> env_files;
182191
bool has_env_file_string = false;
183192
bool test_runner = false;
184193
uint64_t test_runner_concurrency = 0;
@@ -380,6 +389,7 @@ enum OptionType {
380389
kString,
381390
kHostPort,
382391
kStringList,
392+
kDetailedStringList,
383393
};
384394

385395
template <typename Options>
@@ -416,6 +426,10 @@ class OptionsParser {
416426
const char* help_text,
417427
std::vector<std::string> Options::*field,
418428
OptionEnvvarSettings env_setting = kDisallowedInEnvvar);
429+
void AddOption(const char* name,
430+
const char* help_text,
431+
std::vector<DetailedOption<std::string>> Options::*field,
432+
OptionEnvvarSettings env_setting = kDisallowedInEnvvar);
419433
void AddOption(const char* name,
420434
const char* help_text,
421435
HostPort Options::*field,

‎test/parallel/test-dotenv-edge-cases.js

+34-12
Original file line numberDiff line numberDiff line change
@@ -135,17 +135,39 @@ describe('.env supports edge cases', () => {
135135
assert.strictEqual(child.code, 0);
136136
});
137137

138-
it('should handle when --env-file is passed along with --', async () => {
139-
const child = await common.spawnPromisified(
140-
process.execPath,
141-
[
142-
'--eval', `require('assert').strictEqual(process.env.BASIC, undefined);`,
143-
'--', '--env-file', validEnvFilePath,
144-
],
145-
{ cwd: __dirname },
146-
);
147-
assert.strictEqual(child.stdout, '');
148-
assert.strictEqual(child.stderr, '');
149-
assert.strictEqual(child.code, 0);
138+
// Ref: https://github.com/nodejs/node/pull/54913
139+
it('should handle CLI edge cases', async () => {
140+
const edgeCases = [
141+
{
142+
// If the flag is passed AFTER the script, ignore it
143+
flags: [fixtures.path('empty.js'), '--env-file=nonexistent.env'],
144+
},
145+
{
146+
// If the flag is passed AFTER '--', ignore it
147+
flags: ['--eval=""', '--', '--env-file=nonexistent.env'],
148+
},
149+
{
150+
// If the flag is passed AFTER an invalid argument, check the argument first
151+
flags: ['--invalid-argument', '--env-file=nonexistent.env'],
152+
error: 'bad option: --invalid-argument',
153+
},
154+
{
155+
// If the flag is passed as an invalid argument, check the argument first
156+
flags: ['--env-file-ABCD=nonexistent.env'],
157+
error: 'bad option: --env-file-ABCD=nonexistent.env'
158+
},
159+
];
160+
for (const { flags, error } of edgeCases) {
161+
const child = await common.spawnPromisified(process.execPath, flags);
162+
if (error) {
163+
assert.notStrictEqual(child.code, 0);
164+
// Remove the leading '<path>: '
165+
assert.strictEqual(child.stderr.substring(process.execPath.length + 2).trim(), error);
166+
} else {
167+
assert.strictEqual(child.code, 0);
168+
assert.strictEqual(child.stderr, '');
169+
assert.strictEqual(child.stdout, '');
170+
}
171+
}
150172
});
151173
});

‎test/parallel/test-dotenv-without-node-options.js

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('node:assert');
5+
const { test } = require('node:test');
6+
7+
if (!process.config.variables.node_without_node_options) {
8+
common.skip('Requires the lack of NODE_OPTIONS support');
9+
}
10+
11+
const relativePath = '../fixtures/dotenv/node-options.env';
12+
13+
test('.env does not support NODE_OPTIONS', async () => {
14+
const code = 'assert.strictEqual(process.permission, undefined)';
15+
const child = await common.spawnPromisified(
16+
process.execPath,
17+
[ `--env-file=${relativePath}`, '--eval', code ],
18+
{ cwd: __dirname },
19+
);
20+
// NODE_NO_WARNINGS is set, so `stderr` should not contain
21+
// "ExperimentalWarning: Permission is an experimental feature" message
22+
// and thus be empty
23+
assert.strictEqual(child.stdout, '');
24+
assert.strictEqual(child.stderr, '');
25+
assert.strictEqual(child.code, 0);
26+
});

0 commit comments

Comments
 (0)