From f0a7a69207984e5570f161c8f2676649b39d237a Mon Sep 17 00:00:00 2001 From: Angela Chen Date: Mon, 14 Aug 2023 12:03:45 -0400 Subject: [PATCH] Adding a couple of changes for config JSON file parsing * JSON file flags are lower priority than flags specified on the command line * Using JSON is_array and is_object functions * Using round bracket syntax for initalization * Fix Parse() function * Better error reporting for parsing the JSON file * Fixed flag description typo --- include/ppx/command_line_parser.h | 11 ++- src/ppx/application.cpp | 11 ++- src/ppx/command_line_parser.cpp | 79 +++++++++++++----- src/test/command_line_parser_test.cpp | 110 +++++++++++++++++++++++--- 4 files changed, 173 insertions(+), 38 deletions(-) diff --git a/include/ppx/command_line_parser.h b/include/ppx/command_line_parser.h index c3097a164..1c6a84b5d 100644 --- a/include/ppx/command_line_parser.h +++ b/include/ppx/command_line_parser.h @@ -124,9 +124,9 @@ class CliOptions T Parse(std::string_view valueStr, const T defaultValue) const { if constexpr (std::is_same_v) { - return static_cast(valueStr); + return std::string(valueStr); } - std::stringstream ss{static_cast(valueStr)}; + std::stringstream ss((std::string(valueStr))); T valueAsNum; ss >> valueAsNum; if (ss.fail()) { @@ -163,9 +163,11 @@ class CommandLineParser // Adds all options specified within jsonConfig to mOpts. std::optional AddJsonOptions(const nlohmann::json& jsonConfig); + std::string GetJsonConfigFlagName() const { return mJsonConfigFlagName; } const CliOptions& GetOptions() const { return mOpts; } std::string GetUsageMsg() const { return mUsageMsg; } - void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; } + + void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; } private: // Adds an option to mOpts and handles the special --no-flag-name case. @@ -173,7 +175,8 @@ class CommandLineParser std::optional AddOption(std::string_view optionName, std::string_view valueStr); CliOptions mOpts; - std::string mUsageMsg = R"( + std::string mJsonConfigFlagName = "config-json-path"; + std::string mUsageMsg = R"( USAGE ============================== Boolean options can be turned on with: diff --git a/src/ppx/application.cpp b/src/ppx/application.cpp index f82c96127..1230b7ed7 100644 --- a/src/ppx/application.cpp +++ b/src/ppx/application.cpp @@ -807,10 +807,13 @@ void Application::InitStandardKnobs() "Add a path in front of the assets search path list."); mStandardOpts.pAssetsPaths->SetFlagParameters(""); - mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob>>("config-json-path", defaultEmptyList); - mStandardOpts.pAssetsPaths->SetFlagDescription( - "Additional commandline flags specified in a JSON file."); - mStandardOpts.pAssetsPaths->SetFlagParameters(""); + mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList); + mStandardOpts.pConfigJsonPath->SetFlagDescription( + "Additional commandline flags specified in a JSON file. Values specified in JSON files are " + "always lower priority than those specified on the command line. Between different files, the " + "later ones take priority. For lists, the JSON values will come earlier in the array than the " + "command-line values"); + mStandardOpts.pConfigJsonPath->SetFlagParameters(""); mStandardOpts.pDeterministic = mKnobManager.CreateKnob>("deterministic", false); diff --git a/src/ppx/command_line_parser.cpp b/src/ppx/command_line_parser.cpp index 9e768951f..c3dda0b52 100644 --- a/src/ppx/command_line_parser.cpp +++ b/src/ppx/command_line_parser.cpp @@ -57,12 +57,12 @@ std::pair CliOptions::GetOptionValueOrDefault(const std::string& optio void CliOptions::AddOption(std::string_view optionName, std::string_view value) { - std::string optionNameStr{optionName}; - std::string valueStr{value}; + std::string optionNameStr(optionName); + std::string valueStr(value); auto it = mAllOptions.find(optionNameStr); if (it == mAllOptions.cend()) { - std::vector v{valueStr}; - mAllOptions.emplace(optionName, v); + std::vector v{std::move(valueStr)}; + mAllOptions.emplace(std::move(optionNameStr), std::move(v)); return; } it->second.push_back(valueStr); @@ -70,7 +70,7 @@ void CliOptions::AddOption(std::string_view optionName, std::string_view value) void CliOptions::AddOption(std::string_view optionName, const std::vector& valueArray) { - std::string optionNameStr{optionName}; + std::string optionNameStr(optionName); auto it = mAllOptions.find(optionNameStr); if (it == mAllOptions.cend()) { mAllOptions.emplace(optionNameStr, valueArray); @@ -125,7 +125,54 @@ std::optional CommandLineParser::Parse(int argc args.emplace_back(res->second); } - // Process arguments into either standalone flags or options with parameters. + // Special initial pass to identify JSON config files + for (size_t i = 0; i < args.size() - 1; ++i) { + std::string_view name = ppx::string_util::TrimBothEnds(args[i]); + if (name != "--" + mJsonConfigFlagName) { + continue; + } + name = name.substr(2); + std::string_view value = ppx::string_util::TrimBothEnds(args[i + 1]); + ++i; + + if (auto error = AddOption(name, value)) { + return error; + } + } + + // Flags inside JSON files are processed first + // These are always lower priority than flags on the command-line, will be "overwritten" + std::vector configJsonPaths; + configJsonPaths = mOpts.GetOptionValueOrDefault(mJsonConfigFlagName, configJsonPaths); + for (const auto& jsonPath : configJsonPaths) { + // Attempt to open JSON file at specified path + std::ifstream f(jsonPath); + if (f.fail()) { + return "Cannot locate file --" + mJsonConfigFlagName + ": " + jsonPath; + } + + // Expect that JSON file specifies a valid JSON object + PPX_LOG_INFO("Parsing JSON config file: " << jsonPath); + nlohmann::json data; + try { + data = nlohmann::json::parse(f); + } + catch (nlohmann::json::parse_error& e) { + PPX_LOG_ERROR("nlohmann::json::parse error: " << e.what() << '\n' + << "exception id: " << e.id << '\n' + << "byte position of error: " << e.byte); + } + if (!(data.is_object())) { + return "The following config file could not be parsed as a JSON object: " + jsonPath; + } + + // Attempt to add all options specified in this JSON object + if (auto error = AddJsonOptions(data)) { + return error; + } + } + + // Main pass, process arguments into either standalone flags or options with parameters. for (size_t i = 0; i < args.size(); ++i) { std::string_view name = ppx::string_util::TrimBothEnds(args[i]); if (!StartsWithDoubleDash(name)) { @@ -143,20 +190,12 @@ std::optional CommandLineParser::Parse(int argc } } - if (auto error = AddOption(name, value)) { - return error; + // Avoid re-adding option for JSON config file + if (name == mJsonConfigFlagName) { + continue; } - } - std::vector configJsonPaths; - configJsonPaths = mOpts.GetOptionValueOrDefault("config-json-path", configJsonPaths); - for (const auto& jsonPath : configJsonPaths) { - std::ifstream f(jsonPath); - if (f.fail()) { - return "Invalid --config-json-path: " + jsonPath; - } - nlohmann::json data = nlohmann::json::parse(f); - if (auto error = AddJsonOptions(data)) { + if (auto error = AddOption(name, value)) { return error; } } @@ -172,12 +211,12 @@ std::optional CommandLineParser::AddJsonOptions std::string value = ss.str(); ss.str(""); - if (value.length() > 0 && value.substr(0, 1) == "[") { + if ((it.value()).is_array()) { // Special case, arrays specified in JSON are added directly to mOpts to avoid inserting element by element std::vector jsonStringArray; for (const auto& elem : it.value()) { ss << elem; - jsonStringArray.push_back(ss.str()); + jsonStringArray.push_back(std::string(ppx::string_util::TrimBothEnds(ss.str(), " \t\""))); ss.str(""); } mOpts.AddOption(it.key(), jsonStringArray); diff --git a/src/test/command_line_parser_test.cpp b/src/test/command_line_parser_test.cpp index ad994554e..b2d2c0efd 100644 --- a/src/test/command_line_parser_test.cpp +++ b/src/test/command_line_parser_test.cpp @@ -250,7 +250,9 @@ TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) "b": false, "c": 1.234, "d": 5, - "e": "hello world" + "e": "helloworld", + "f": "hello world", + "g": "200x300" } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); @@ -258,12 +260,41 @@ TEST(CommandLineParserTest, JsonSimpleParsedSuccessfully) FAIL() << error->errorMsg; } auto opts = parser.GetOptions(); - EXPECT_EQ(opts.GetNumUniqueOptions(), 5); + EXPECT_EQ(opts.GetNumUniqueOptions(), 7); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_EQ(opts.GetOptionValueOrDefault("b", true), false); EXPECT_FLOAT_EQ(opts.GetOptionValueOrDefault("c", 6.0f), 1.234f); EXPECT_EQ(opts.GetOptionValueOrDefault("d", 0), 5); - EXPECT_EQ(opts.GetOptionValueOrDefault("e", "foo"), "hello world"); + EXPECT_EQ(opts.GetOptionValueOrDefault("e", "foo"), "helloworld"); + EXPECT_EQ(opts.GetOptionValueOrDefault("f", "foo"), "hello world"); + std::pair gFlag = opts.GetOptionValueOrDefault("g", std::make_pair(1, 1)); + EXPECT_EQ(gFlag.first, 200); + EXPECT_EQ(gFlag.second, 300); +} + +TEST(CommandLineParserTest, JsonWithNestedStructure) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": { + "c" : 1, + "d" : 2 + } + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_TRUE(opts.HasExtraOption("b")); + EXPECT_EQ(opts.GetOptionValueOrDefault("b", "default"), "{\"c\":1,\"d\":2}"); + EXPECT_FALSE(opts.HasExtraOption("c")); + EXPECT_FALSE(opts.HasExtraOption("d")); } TEST(CommandLineParserTest, JsonWithIntArray) @@ -291,16 +322,38 @@ TEST(CommandLineParserTest, JsonWithIntArray) EXPECT_EQ(gotB.at(2), 3); } -TEST(CommandLineParserTest, JsonWithNestedStructure) +TEST(CommandLineParserTest, JsonWithStrArray) { CommandLineParser parser; std::string jsonText = R"( { "a": true, - "b": { - "c" : 1, - "d" : 2 + "b": ["first", "second", "third"] + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; } + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); + EXPECT_TRUE(opts.HasExtraOption("b")); + std::vector defaultB = {}; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 3); + EXPECT_EQ(gotB.at(0), "first"); + EXPECT_EQ(gotB.at(1), "second"); + EXPECT_EQ(gotB.at(2), "third"); +} + +TEST(CommandLineParserTest, JsonWithHeterogeneousArray) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": true, + "b": [1, "2", {"c" : 3}, 4.0] } )"; nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); @@ -311,9 +364,46 @@ TEST(CommandLineParserTest, JsonWithNestedStructure) EXPECT_EQ(opts.GetNumUniqueOptions(), 2); EXPECT_EQ(opts.GetOptionValueOrDefault("a", false), true); EXPECT_TRUE(opts.HasExtraOption("b")); - EXPECT_EQ(opts.GetOptionValueOrDefault("b", "default"), "{\"c\":1,\"d\":2}"); - EXPECT_FALSE(opts.HasExtraOption("c")); - EXPECT_FALSE(opts.HasExtraOption("d")); + std::vector defaultB; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 4); + EXPECT_EQ(gotB.at(0), "1"); + EXPECT_EQ(gotB.at(1), "2"); + EXPECT_EQ(gotB.at(2), "{\"c\":3}"); + EXPECT_EQ(gotB.at(3), "4.0"); +} + +TEST(CommandLineParserTest, JsonVsCommandlinePriority) +{ + CommandLineParser parser; + std::string jsonText = R"( + { + "a": 1, + "b": ["one", "two", "three"] + } +)"; + nlohmann::json jsonConfig = nlohmann::json::parse(jsonText); + if (auto error = parser.AddJsonOptions(jsonConfig)) { + FAIL() << error->errorMsg; + } + + const char* args[] = {"/path/to/executable", "--b", "four", "--a", "2", "--b", "five", "--a", "3"}; + if (auto error = parser.Parse(sizeof(args) / sizeof(args[0]), args)) { + FAIL() << error->errorMsg; + } + + auto opts = parser.GetOptions(); + EXPECT_EQ(opts.GetNumUniqueOptions(), 2); + EXPECT_EQ(opts.GetOptionValueOrDefault("a", 0), 3); + EXPECT_TRUE(opts.HasExtraOption("b")); + std::vector defaultB = {}; + std::vector gotB = opts.GetOptionValueOrDefault("b", defaultB); + EXPECT_EQ(gotB.size(), 5); + EXPECT_EQ(gotB.at(0), "one"); + EXPECT_EQ(gotB.at(1), "two"); + EXPECT_EQ(gotB.at(2), "three"); + EXPECT_EQ(gotB.at(3), "four"); + EXPECT_EQ(gotB.at(4), "five"); } } // namespace