From 630ee2fbd3bd8b9a7b6f3a10e19219aa435d4ffc Mon Sep 17 00:00:00 2001 From: RedYetiDev <38299977+RedYetiDev@users.noreply.github.com> Date: Thu, 10 Oct 2024 15:08:25 -0400 Subject: [PATCH] src: parse dotenv with the rest of the options --- src/node.cc | 75 ++++++++++++------- src/node_dotenv.cc | 57 +------------- src/node_dotenv.h | 5 +- src/node_options-inl.h | 21 ++++++ src/node_options.cc | 37 ++++++++- src/node_options.h | 18 ++++- test/parallel/test-dotenv-edge-cases.js | 46 +++++++++--- .../test-dotenv-without-node-options.js | 26 +++++++ 8 files changed, 182 insertions(+), 103 deletions(-) create mode 100644 test/parallel/test-dotenv-without-node-options.js diff --git a/src/node.cc b/src/node.cc index 26f94be94bae70..bc5c12b11407e4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -800,6 +800,49 @@ int ProcessGlobalArgs(std::vector* args, static std::atomic_bool init_called{false}; +static ExitCode ProcessEnvFiles(std::vector* errors) { + std::vector>& env_files = + per_process::cli_options->per_isolate->per_env->env_files; + if (env_files.empty()) return ExitCode::kNoFailure; + + CHECK(!per_process::v8_initialized); + + for (const auto& env_file : env_files) { + switch (per_process::dotenv_file.ParsePath(env_file.value)) { + case Dotenv::ParseResult::Valid: + break; + case Dotenv::ParseResult::InvalidContent: + errors->emplace_back(env_file.value + ": invalid format"); + break; + case Dotenv::ParseResult::FileError: + if (env_file.flag == "--env-file-if-exists") { + fprintf(stderr, + "%s not found. Continuing without it.\n", + env_file.value.c_str()); + } else { + errors->emplace_back(env_file.value + ": not found"); + } + break; + default: + UNREACHABLE(); + } + } + +#if !defined(NODE_WITHOUT_NODE_OPTIONS) + // Parse and process Node.js options from the environment + std::vector env_argv = + ParseNodeOptionsEnvVar(per_process::dotenv_file.GetNodeOptions(), errors); + env_argv.insert(env_argv.begin(), per_process::cli_options->cmdline.at(0)); + + // Process global arguments + const ExitCode exit_code = + ProcessGlobalArgsInternal(&env_argv, nullptr, errors, kAllowedInEnvvar); + if (exit_code != ExitCode::kNoFailure) return exit_code; +#endif + + return ExitCode::kNoFailure; +} + // TODO(addaleax): Turn this into a wrapper around InitializeOncePerProcess() // (with the corresponding additional flags set), then eventually remove this. static ExitCode InitializeNodeWithArgsInternal( @@ -851,35 +894,6 @@ static ExitCode InitializeNodeWithArgsInternal( HandleEnvOptions(per_process::cli_options->per_isolate->per_env); std::string node_options; - auto env_files = node::Dotenv::GetDataFromArgs(*argv); - - if (!env_files.empty()) { - CHECK(!per_process::v8_initialized); - - for (const auto& file_data : env_files) { - switch (per_process::dotenv_file.ParsePath(file_data.path)) { - case Dotenv::ParseResult::Valid: - break; - case Dotenv::ParseResult::InvalidContent: - errors->push_back(file_data.path + ": invalid format"); - break; - case Dotenv::ParseResult::FileError: - if (file_data.is_optional) { - fprintf(stderr, - "%s not found. Continuing without it.\n", - file_data.path.c_str()); - continue; - } - errors->push_back(file_data.path + ": not found"); - break; - default: - UNREACHABLE(); - } - } - - per_process::dotenv_file.AssignNodeOptionsIfAvailable(&node_options); - } - #if !defined(NODE_WITHOUT_NODE_OPTIONS) if (!(flags & ProcessInitializationFlags::kDisableNodeOptionsEnv)) { // NODE_OPTIONS environment variable is preferred over the file one. @@ -915,6 +929,9 @@ static ExitCode InitializeNodeWithArgsInternal( if (exit_code != ExitCode::kNoFailure) return exit_code; } + const ExitCode exit_code = ProcessEnvFiles(errors); + if (exit_code != ExitCode::kNoFailure) return exit_code; + // Set the process.title immediately after processing argv if --title is set. if (!per_process::cli_options->title.empty()) uv_set_process_title(per_process::cli_options->title.c_str()); diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index f594df875d7a0c..446176f2f83a9f 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -11,54 +11,6 @@ using v8::NewStringType; using v8::Object; using v8::String; -std::vector Dotenv::GetDataFromArgs( - const std::vector& args) { - const std::string_view optional_env_file_flag = "--env-file-if-exists"; - - const auto find_match = [](const std::string& arg) { - return arg == "--" || arg == "--env-file" || - arg.starts_with("--env-file=") || arg == "--env-file-if-exists" || - arg.starts_with("--env-file-if-exists="); - }; - - std::vector env_files; - // This will be an iterator, pointing to args.end() if no matches are found - auto matched_arg = std::find_if(args.begin(), args.end(), find_match); - - while (matched_arg != args.end()) { - if (*matched_arg == "--") { - return env_files; - } - - auto equal_char_index = matched_arg->find('='); - - if (equal_char_index != std::string::npos) { - // `--env-file=path` - auto flag = matched_arg->substr(0, equal_char_index); - auto file_path = matched_arg->substr(equal_char_index + 1); - - struct env_file_data env_file_data = { - file_path, flag.starts_with(optional_env_file_flag)}; - env_files.push_back(env_file_data); - } else { - // `--env-file path` - auto file_path = std::next(matched_arg); - - if (file_path == args.end()) { - return env_files; - } - - struct env_file_data env_file_data = { - *file_path, matched_arg->starts_with(optional_env_file_flag)}; - env_files.push_back(env_file_data); - } - - matched_arg = std::find_if(++matched_arg, args.end(), find_match); - } - - return env_files; -} - void Dotenv::SetEnvironment(node::Environment* env) { auto isolate = env->isolate(); @@ -277,12 +229,9 @@ Dotenv::ParseResult Dotenv::ParsePath(const std::string_view path) { return ParseResult::Valid; } -void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) const { - auto match = store_.find("NODE_OPTIONS"); - - if (match != store_.end()) { - *node_options = match->second; - } +std::string Dotenv::GetNodeOptions() const { + auto it = store_.find("NODE_OPTIONS"); + return (it != store_.end()) ? it->second : ""; } } // namespace node diff --git a/src/node_dotenv.h b/src/node_dotenv.h index d508b13fc5db74..44a480fefb5def 100644 --- a/src/node_dotenv.h +++ b/src/node_dotenv.h @@ -27,13 +27,10 @@ class Dotenv { void ParseContent(const std::string_view content); ParseResult ParsePath(const std::string_view path); - void AssignNodeOptionsIfAvailable(std::string* node_options) const; + std::string GetNodeOptions() const; void SetEnvironment(Environment* env); v8::Local ToObject(Environment* env) const; - static std::vector GetDataFromArgs( - const std::vector& args); - private: std::map store_; }; diff --git a/src/node_options-inl.h b/src/node_options-inl.h index 24954e0b583834..6d1cebd9e71de7 100644 --- a/src/node_options-inl.h +++ b/src/node_options-inl.h @@ -93,6 +93,23 @@ void OptionsParser::AddOption( }); } +template +void OptionsParser::AddOption( + const char* name, + const char* help_text, + std::vector> Options::*field, + OptionEnvvarSettings env_setting) { + options_.emplace( + name, + OptionInfo{ + kDetailedStringList, + std::make_shared< + SimpleOptionField>>>( + field), + env_setting, + help_text}); +} + template void OptionsParser::AddOption(const char* name, const char* help_text, @@ -466,6 +483,10 @@ void OptionsParser::Parse( Lookup>(info.field, options) ->emplace_back(std::move(value)); break; + case kDetailedStringList: + Lookup>>(info.field, options) + ->emplace_back(DetailedOption{value, name}); + break; case kHostPort: Lookup(info.field, options) ->Update(SplitHostPort(value, errors)); diff --git a/src/node_options.cc b/src/node_options.cc index 3bfab2759b18f4..ba84ce38d8089a 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -643,11 +643,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { "[has_env_file_string]", "", &EnvironmentOptions::has_env_file_string); AddOption("--env-file", "set environment variables from supplied file", - &EnvironmentOptions::env_file); + &EnvironmentOptions::env_files); Implies("--env-file", "[has_env_file_string]"); AddOption("--env-file-if-exists", "set environment variables from supplied file", - &EnvironmentOptions::optional_env_file); + &EnvironmentOptions::env_files); Implies("--env-file-if-exists", "[has_env_file_string]"); AddOption("--test", "launch test runner on startup", @@ -1341,6 +1341,39 @@ void GetCLIOptionsValues(const FunctionCallbackInfo& args) { return; } break; + case kDetailedStringList: { + const std::vector>& detailed_options = + *_ppop_instance.Lookup>>( + field, opts); + v8::Local value_arr = + v8::Array::New(isolate, detailed_options.size()); + for (size_t i = 0; i < detailed_options.size(); ++i) { + // Create a new V8 object for each DetailedOption + v8::Local option_object = v8::Object::New(isolate); + + option_object + ->Set(isolate->GetCurrentContext(), + v8::String::NewFromUtf8(isolate, "flag").ToLocalChecked(), + v8::String::NewFromUtf8(isolate, + detailed_options[i].flag.c_str()) + .ToLocalChecked()) + .Check(); + + option_object + ->Set(isolate->GetCurrentContext(), + v8::String::NewFromUtf8(isolate, "value").ToLocalChecked(), + v8::String::NewFromUtf8(isolate, + detailed_options[i].value.c_str()) + .ToLocalChecked()) + .Check(); + + // Add the object to the array at the current index + value_arr->Set(isolate->GetCurrentContext(), i, option_object) + .Check(); + } + value = value_arr; + break; + } case kHostPort: { const HostPort& host_port = *_ppop_instance.Lookup(field, opts); diff --git a/src/node_options.h b/src/node_options.h index ed89f08f3b4140..68203f654cfe8e 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -44,6 +44,16 @@ class HostPort { uint16_t port_; }; +template +class DetailedOption { + public: + DetailedOption(const ValueType& value, std::string flag) + : value(value), flag(flag) {} + + ValueType value; + std::string flag; +}; + class Options { public: virtual void CheckOptions(std::vector* errors, @@ -177,8 +187,7 @@ class EnvironmentOptions : public Options { #endif // HAVE_INSPECTOR std::string redirect_warnings; std::string diagnostic_dir; - std::string env_file; - std::string optional_env_file; + std::vector> env_files; bool has_env_file_string = false; bool test_runner = false; uint64_t test_runner_concurrency = 0; @@ -380,6 +389,7 @@ enum OptionType { kString, kHostPort, kStringList, + kDetailedStringList, }; template @@ -416,6 +426,10 @@ class OptionsParser { const char* help_text, std::vector Options::*field, OptionEnvvarSettings env_setting = kDisallowedInEnvvar); + void AddOption(const char* name, + const char* help_text, + std::vector> Options::*field, + OptionEnvvarSettings env_setting = kDisallowedInEnvvar); void AddOption(const char* name, const char* help_text, HostPort Options::*field, diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 30fd9d20618ba0..67f10973b9d892 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -135,17 +135,39 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.code, 0); }); - it('should handle when --env-file is passed along with --', async () => { - const child = await common.spawnPromisified( - process.execPath, - [ - '--eval', `require('assert').strictEqual(process.env.BASIC, undefined);`, - '--', '--env-file', validEnvFilePath, - ], - { cwd: __dirname }, - ); - assert.strictEqual(child.stdout, ''); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); + // Ref: https://github.com/nodejs/node/pull/54913 + it('should handle CLI edge cases', async () => { + const edgeCases = [ + { + // If the flag is passed AFTER the script, ignore it + flags: [fixtures.path('empty.js'), '--env-file=nonexistent.env'], + }, + { + // If the flag is passed AFTER '--', ignore it + flags: ['--eval=""', '--', '--env-file=nonexistent.env'], + }, + { + // If the flag is passed AFTER an invalid argument, check the argument first + flags: ['--invalid-argument', '--env-file=nonexistent.env'], + error: 'bad option: --invalid-argument', + }, + { + // If the flag is passed as an invalid argument, check the argument first + flags: ['--env-file-ABCD=nonexistent.env'], + error: 'bad option: --env-file-ABCD=nonexistent.env' + }, + ]; + for (const { flags, error } of edgeCases) { + const child = await common.spawnPromisified(process.execPath, flags); + if (error) { + assert.notStrictEqual(child.code, 0); + // Remove the leading ': ' + assert.strictEqual(child.stderr.substring(process.execPath.length + 2).trim(), error); + } else { + assert.strictEqual(child.code, 0); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.stdout, ''); + } + } }); }); diff --git a/test/parallel/test-dotenv-without-node-options.js b/test/parallel/test-dotenv-without-node-options.js new file mode 100644 index 00000000000000..474950b54fb94c --- /dev/null +++ b/test/parallel/test-dotenv-without-node-options.js @@ -0,0 +1,26 @@ +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const { test } = require('node:test'); + +if (!process.config.variables.node_without_node_options) { + common.skip('Requires the lack of NODE_OPTIONS support'); +} + +const relativePath = '../fixtures/dotenv/node-options.env'; + +test('.env does not support NODE_OPTIONS', async () => { + const code = 'assert.strictEqual(process.permission, undefined)'; + const child = await common.spawnPromisified( + process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], + { cwd: __dirname }, + ); + // NODE_NO_WARNINGS is set, so `stderr` should not contain + // "ExperimentalWarning: Permission is an experimental feature" message + // and thus be empty + assert.strictEqual(child.stdout, ''); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); +});