Skip to content

Commit

Permalink
Adding a couple of changes for config JSON file parsing
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@angela28chen
angela28chen committed Aug 14, 2023
1 parent bada521 commit f0a7a69
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 38 deletions.
11 changes: 7 additions & 4 deletions include/ppx/command_line_parser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,9 @@ class CliOptions
T Parse(std::string_view valueStr, const T defaultValue) const
{
if constexpr (std::is_same_v<T, std::string>) {
return static_cast<std::string>(valueStr);
return std::string(valueStr);
}
std::stringstream ss{static_cast<std::string>(valueStr)};
std::stringstream ss((std::string(valueStr)));
T valueAsNum;
ss >> valueAsNum;
if (ss.fail()) {
Expand Down Expand Up @@ -163,17 +163,20 @@ class CommandLineParser
// Adds all options specified within jsonConfig to mOpts.
std::optional<ParsingError> 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.
// Expects option names without the "--" prefix.
std::optional<ParsingError> 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:
Expand Down
11 changes: 7 additions & 4 deletions src/ppx/application.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -807,10 +807,13 @@ void Application::InitStandardKnobs()
"Add a path in front of the assets search path list.");
mStandardOpts.pAssetsPaths->SetFlagParameters("<path>");

mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>("config-json-path", defaultEmptyList);
mStandardOpts.pAssetsPaths->SetFlagDescription(
"Additional commandline flags specified in a JSON file.");
mStandardOpts.pAssetsPaths->SetFlagParameters("<path>");
mStandardOpts.pConfigJsonPath = mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>(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("<path>");

mStandardOpts.pDeterministic =
mKnobManager.CreateKnob<KnobFlag<bool>>("deterministic", false);
Expand Down
79 changes: 59 additions & 20 deletions src/ppx/command_line_parser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,20 @@ std::pair<int, int> 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<std::string> v{valueStr};
mAllOptions.emplace(optionName, v);
std::vector<std::string> v{std::move(valueStr)};
mAllOptions.emplace(std::move(optionNameStr), std::move(v));
return;
}
it->second.push_back(valueStr);
}

void CliOptions::AddOption(std::string_view optionName, const std::vector<std::string>& valueArray)
{
std::string optionNameStr{optionName};
std::string optionNameStr(optionName);
auto it = mAllOptions.find(optionNameStr);
if (it == mAllOptions.cend()) {
mAllOptions.emplace(optionNameStr, valueArray);
Expand Down Expand Up @@ -125,7 +125,54 @@ std::optional<CommandLineParser::ParsingError> 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<std::string> 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)) {
Expand All @@ -143,20 +190,12 @@ std::optional<CommandLineParser::ParsingError> 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<std::string> 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;
}
}
Expand All @@ -172,12 +211,12 @@ std::optional<CommandLineParser::ParsingError> 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<std::string> 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);
Expand Down
110 changes: 100 additions & 10 deletions src/test/command_line_parser_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,20 +250,51 @@ 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);
if (auto error = parser.AddJsonOptions(jsonConfig)) {
FAIL() << error->errorMsg;
}
auto opts = parser.GetOptions();
EXPECT_EQ(opts.GetNumUniqueOptions(), 5);
EXPECT_EQ(opts.GetNumUniqueOptions(), 7);
EXPECT_EQ(opts.GetOptionValueOrDefault<bool>("a", false), true);
EXPECT_EQ(opts.GetOptionValueOrDefault<bool>("b", true), false);
EXPECT_FLOAT_EQ(opts.GetOptionValueOrDefault<float>("c", 6.0f), 1.234f);
EXPECT_EQ(opts.GetOptionValueOrDefault<int>("d", 0), 5);
EXPECT_EQ(opts.GetOptionValueOrDefault<std::string>("e", "foo"), "hello world");
EXPECT_EQ(opts.GetOptionValueOrDefault<std::string>("e", "foo"), "helloworld");
EXPECT_EQ(opts.GetOptionValueOrDefault<std::string>("f", "foo"), "hello world");
std::pair<int, int> 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<bool>("a", false), true);
EXPECT_TRUE(opts.HasExtraOption("b"));
EXPECT_EQ(opts.GetOptionValueOrDefault<std::string>("b", "default"), "{\"c\":1,\"d\":2}");
EXPECT_FALSE(opts.HasExtraOption("c"));
EXPECT_FALSE(opts.HasExtraOption("d"));
}

TEST(CommandLineParserTest, JsonWithIntArray)
Expand Down Expand Up @@ -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<bool>("a", false), true);
EXPECT_TRUE(opts.HasExtraOption("b"));
std::vector<std::string> defaultB = {};
std::vector<std::string> gotB = opts.GetOptionValueOrDefault<std::string>("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);
Expand All @@ -311,9 +364,46 @@ TEST(CommandLineParserTest, JsonWithNestedStructure)
EXPECT_EQ(opts.GetNumUniqueOptions(), 2);
EXPECT_EQ(opts.GetOptionValueOrDefault<bool>("a", false), true);
EXPECT_TRUE(opts.HasExtraOption("b"));
EXPECT_EQ(opts.GetOptionValueOrDefault<std::string>("b", "default"), "{\"c\":1,\"d\":2}");
EXPECT_FALSE(opts.HasExtraOption("c"));
EXPECT_FALSE(opts.HasExtraOption("d"));
std::vector<std::string> defaultB;
std::vector<std::string> gotB = opts.GetOptionValueOrDefault<std::string>("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<int>("a", 0), 3);
EXPECT_TRUE(opts.HasExtraOption("b"));
std::vector<std::string> defaultB = {};
std::vector<std::string> gotB = opts.GetOptionValueOrDefault<std::string>("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
Expand Down

0 comments on commit f0a7a69

Please sign in to comment.