Add --config-json-path to accept flags specified in a JSON file. (#265)

Add flag --config-json-path to accept commandline flags that are
specified in a JSON file. This flag can be used multiple times. The
priority of any conflicting flags is as follows:

later flag on commandline > earlier flag on commandline > flag in later
JSON file > flag in earlier JSON file

For flags that accept multiple values, the same priority is used and
only flags specified in the same JSON file or flags all on the
commandline will be combined.

Detailed changes:
* Changed map `mAllOptions` to hold `strings` instead of `string_views`
since there are now temporary strings originating from the JSON
structure, changed some input parameters for `CliOptions` member
functions to match.
* Created some `CommandLineParser` methods to aid in parsing options
(these are public for unit testing purposes)
  * `ParseJson`
  * `ParseOption`
* Created more methods for `CliOptions`
* `AddOption` overload that accepts one key and a vector of values to
reduce the time needed to insert arrays. This function is only used by
the JSON parsing flow. On the command line the flag must still be
specified multiple times (`--flag-name 1,2,3` is still invalid)
  * `OverwriteOptions`
* Updated `CommandLineParser::Parse`
* Added a second initial pass of argv during commandline parsing to
identify any provided JSON config files
  * Implemented overwriting
* Added `mJsonConfigFlagName` to `CommandLineParser` so that the string
`config-json-path` isn't specified in multiple places

include/ppx/application.h

@@ -252,6 +252,7 @@ struct StandardOptions
 #endif
 
     std::shared_ptr<KnobFlag<std::vector<std::string>>> pAssetsPaths;
+    std::shared_ptr<KnobFlag<std::vector<std::string>>> pConfigJsonPaths;
 };
 
 // -------------------------------------------------------------------------------------------------

include/ppx/command_line_parser.h

@@ -15,6 +15,8 @@
 #ifndef ppx_command_line_parser_h
 #define ppx_command_line_parser_h
 
+#include "nlohmann/json.hpp"
+
 #include <cstdint>
 #include <ios>
 #include <optional>
@@ -44,7 +46,7 @@ class CliOptions
 public:
     CliOptions() = default;
 
-    bool HasExtraOption(std::string_view option) const { return mAllOptions.contains(option); }
+    bool HasExtraOption(const std::string& option) const { return mAllOptions.contains(option); }
 
     // Returns the number of unique options and flags that were specified on the commandline,
     // not counting multiple appearances of the same flag such as: --assets-path a --assets-path b
@@ -55,7 +57,7 @@ class CliOptions
     // Warning: If this is called instead of the vector overload for multiple-value flags,
     //          only the last value will be returned.
     template <typename T>
-    T GetOptionValueOrDefault(std::string_view optionName, const T& defaultValue) const
+    T GetOptionValueOrDefault(const std::string& optionName, const T& defaultValue) const
     {
         auto it = mAllOptions.find(optionName);
         if (it == mAllOptions.cend()) {
@@ -68,7 +70,7 @@ class CliOptions
     // Same as above, but intended for list flags that are specified on the command line
     // with multiple instances of the same flag
     template <typename T>
-    std::vector<T> GetOptionValueOrDefault(std::string_view optionName, const std::vector<T>& defaultValues) const
+    std::vector<T> GetOptionValueOrDefault(const std::string& optionName, const std::vector<T>& defaultValues) const
     {
         auto it = mAllOptions.find(optionName);
         if (it == mAllOptions.cend()) {
@@ -84,14 +86,14 @@ class CliOptions
 
     // Same as above, but intended for resolution flags that are specified on command line
     // with <Width>x<Height>
-    std::pair<int, int> GetOptionValueOrDefault(std::string_view optionName, const std::pair<int, int>& defaultValue) const;
+    std::pair<int, int> GetOptionValueOrDefault(const std::string& optionName, const std::pair<int, int>& defaultValue) const;
 
     // (WILL BE DEPRECATED, USE KNOBS INSTEAD)
     // Get the parameter value after converting it into the desired integral,
     // floating-point, or boolean type. If the value fails to be converted,
     // return the specified default value.
     template <typename T>
-    T GetExtraOptionValueOrDefault(std::string_view optionName, const T& defaultValue) const
+    T GetExtraOptionValueOrDefault(const std::string& optionName, const T& defaultValue) const
     {
         static_assert(std::is_integral_v<T> || std::is_floating_point_v<T> || std::is_same_v<T, std::string>, "GetExtraOptionValueOrDefault must be called with an integral, floating-point, boolean, or std::string type");
 
@@ -101,8 +103,13 @@ class CliOptions
 private:
     // Adds new option if the option does not already exist
     // Otherwise, the new value is appended to the end of the vector of stored parameters for this option
-    void
-    AddOption(std::string_view optionName, std::string_view value);
+    void AddOption(std::string_view optionName, std::string_view value);
+
+    // Same as above, but appends an array of values at the same key
+    void AddOption(std::string_view optionName, const std::vector<std::string>& valueArray);
+
+    // For all options existing in newOptions, current entries in mAllOptions will be replaced by them
+    void OverwriteOptions(const CliOptions& newOptions);
 
     template <typename T>
     T GetParsedOrDefault(std::string_view valueStr, const T& defaultValue) const
@@ -120,9 +127,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()) {
@@ -133,7 +140,7 @@ class CliOptions
 
 private:
     // All flag names (string) and parameters (vector of strings) specified on the command line
-    std::unordered_map<std::string_view, std::vector<std::string_view>> mAllOptions;
+    std::unordered_map<std::string, std::vector<std::string>> mAllOptions;
 
     friend class CommandLineParser;
 };
@@ -156,14 +163,23 @@ class CommandLineParser
     // and write the error to `out_error`.
     std::optional<ParsingError> Parse(int argc, const char* argv[]);
 
+    // Parses all options specified within jsonConfig and adds them to cliOptions.
+    std::optional<ParsingError> ParseJson(CliOptions& cliOptions, const nlohmann::json& jsonConfig);
+
+    // Parses an option, handles the special --no-flag-name case, then adds the option to cliOptions
+    // Expects option names without the "--" prefix.
+    std::optional<ParsingError> ParseOption(CliOptions& cliOptions, std::string_view optionName, std::string_view valueStr);
+
+    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; }
 
-private:
-    CliOptions mOpts;
+    void AppendUsageMsg(const std::string& additionalMsg) { mUsageMsg += additionalMsg; }
 
-    std::string mUsageMsg = R"(
+private:
+    CliOptions  mOpts;
+    std::string mJsonConfigFlagName = "config-json-path";
+    std::string mUsageMsg           = R"(
 USAGE
 ==============================
 Boolean options can be turned on with:

src/ppx/application.cpp

@@ -806,13 +806,20 @@ void Application::SaveMetricsReportToDisk()
 void Application::InitStandardKnobs()
 {
     // Flag names in alphabetical order
-    std::vector<std::string> defaultAssetsPaths = {};
+    std::vector<std::string> defaultEmptyList = {};
     mStandardOpts.pAssetsPaths =
-        mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>("assets-path", defaultAssetsPaths);
+        mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>("assets-path", defaultEmptyList);
     mStandardOpts.pAssetsPaths->SetFlagDescription(
-        "Add a path in front of the assets search path list.");
+        "Add a path before the default assets folder in the search list.");
     mStandardOpts.pAssetsPaths->SetFlagParameters("<path>");
 
+    mStandardOpts.pConfigJsonPaths = mKnobManager.CreateKnob<KnobFlag<std::vector<std::string>>>(mCommandLineParser.GetJsonConfigFlagName(), defaultEmptyList);
+    mStandardOpts.pConfigJsonPaths->SetFlagDescription(
+        "Additional commandline flags specified in a JSON file. Values specified in JSON files are "
+        "always overwritten by those specified on the command line. Between different files, the "
+        "later ones take priority.");
+    mStandardOpts.pConfigJsonPaths->SetFlagParameters("<path>");
+
     mStandardOpts.pDeterministic =
         mKnobManager.CreateKnob<KnobFlag<bool>>("deterministic", false);
     mStandardOpts.pDeterministic->SetFlagDescription(

src/ppx/command_line_parser.cpp

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include <algorithm>
+#include <fstream>
 #include <iostream>
 #include <locale>
 #include <vector>
@@ -37,7 +38,14 @@ bool StartsWithDoubleDash(std::string_view s)
 
 namespace ppx {
 
-std::pair<int, int> CliOptions::GetOptionValueOrDefault(std::string_view optionName, const std::pair<int, int>& defaultValue) const
+void CliOptions::OverwriteOptions(const CliOptions& newOptions)
+{
+    for (auto& it : newOptions.mAllOptions) {
+        mAllOptions[it.first] = it.second;
+    }
+}
+
+std::pair<int, int> CliOptions::GetOptionValueOrDefault(const std::string& optionName, const std::pair<int, int>& defaultValue) const
 {
     auto it = mAllOptions.find(optionName);
     if (it == mAllOptions.cend()) {
@@ -54,17 +62,31 @@ std::pair<int, int> CliOptions::GetOptionValueOrDefault(std::string_view optionN
     return std::make_pair(N, M);
 }
 
-void CliOptions::AddOption(std::string_view optionName, std::string_view valueStr)
+void CliOptions::AddOption(std::string_view optionName, std::string_view value)
 {
-    auto it = mAllOptions.find(optionName);
+    std::string optionNameStr(optionName);
+    std::string valueStr(value);
+    auto        it = mAllOptions.find(optionNameStr);
     if (it == mAllOptions.cend()) {
-        std::vector<std::string_view> 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);
+    auto        it = mAllOptions.find(optionNameStr);
+    if (it == mAllOptions.cend()) {
+        mAllOptions.emplace(std::move(optionNameStr), valueArray);
+        return;
+    }
+    auto storedValueArray = it->second;
+    storedValueArray.insert(storedValueArray.end(), valueArray.cbegin(), valueArray.cend());
+}
+
 bool CliOptions::Parse(std::string_view valueStr, bool defaultValue) const
 {
     if (valueStr == "") {
@@ -91,7 +113,7 @@ std::optional<CommandLineParser::ParsingError> CommandLineParser::Parse(int argc
         return std::nullopt;
     }
 
-    // Split flag and parameters connected with '='
+    // Initial pass to trim the name of executable and to split any flag and parameters that are connected with '='
     std::vector<std::string_view> args;
     for (size_t i = 1; i < argc; ++i) {
         std::string_view argString(argv[i]);
@@ -103,40 +125,122 @@ std::optional<CommandLineParser::ParsingError> CommandLineParser::Parse(int argc
         if (res->first.empty() || res->second.empty()) {
             return "Malformed flag with '=': \"" + std::string(argString) + "\"";
         }
-        else if (res->second.find('=') != std::string_view::npos) {
+        if (res->second.find('=') != std::string_view::npos) {
             return "Unexpected number of '=' symbols in the following string: \"" + std::string(argString) + "\"";
         }
         args.emplace_back(res->first);
         args.emplace_back(res->second);
     }
 
-    // Process arguments into either standalone flags or options with parameters.
+    // Another pass to identify JSON config files, add that option, and remove it from the argument list
+    std::vector<std::string_view> argsFiltered;
+    for (size_t i = 0; i < args.size(); ++i) {
+        bool nextArgumentIsParameter = (i + 1 < args.size()) && !StartsWithDoubleDash(args[i + 1]);
+        if ((args[i] == "--" + mJsonConfigFlagName) && nextArgumentIsParameter) {
+            mOpts.AddOption(mJsonConfigFlagName, ppx::string_util::TrimBothEnds(args[i + 1]));
+            ++i;
+            continue;
+        }
+        argsFiltered.emplace_back(args[i]);
+    }
+    args = argsFiltered;
+    argsFiltered.clear();
+
+    // Flags inside JSON files are processed first
+    // These are always lower priority than flags on the command-line
+    std::vector<std::string> configJsonPaths;
+    configJsonPaths = mOpts.GetOptionValueOrDefault(mJsonConfigFlagName, configJsonPaths);
+    for (const auto& jsonPath : configJsonPaths) {
+        std::ifstream f(jsonPath);
+        if (f.fail()) {
+            return "Cannot locate file --" + mJsonConfigFlagName + ": " + jsonPath;
+        }
+
+        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;
+        }
+
+        CliOptions jsonOptions;
+        if (auto error = ParseJson(jsonOptions, data)) {
+            return error;
+        }
+        mOpts.OverwriteOptions(jsonOptions);
+    }
+
+    CliOptions commandlineOptions;
+    // 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)) {
             return "Invalid command-line option: \"" + std::string(name) + "\"";
         }
         name = name.substr(2);
 
+        std::string_view value = "";
         if (i + 1 < args.size()) {
-            std::string_view nextElem = ppx::string_util::TrimBothEnds(args[i + 1]);
+            auto nextElem = ppx::string_util::TrimBothEnds(args[i + 1]);
             if (!StartsWithDoubleDash(nextElem)) {
-                // We found an option with a parameter.
-                mOpts.AddOption(name, nextElem);
+                // The next element is a parameter for the current option
+                value = nextElem;
                 ++i;
-                continue;
             }
         }
-        // There is no parameter so it's likely a flag
-        if (name.substr(0, 3) == "no-") {
-            mOpts.AddOption(name.substr(3), "0");
+
+        if (auto error = ParseOption(commandlineOptions, name, value)) {
+            return error;
+        }
+    }
+    mOpts.OverwriteOptions(commandlineOptions);
+
+    return std::nullopt;
+}
+
+std::optional<CommandLineParser::ParsingError> CommandLineParser::ParseJson(CliOptions& cliOptions, const nlohmann::json& jsonConfig)
+{
+    std::stringstream ss;
+    for (auto it = jsonConfig.cbegin(); it != jsonConfig.cend(); ++it) {
+        if ((it.value()).is_array()) {
+            // Special case, arrays specified in JSON are added directly to cliOptions to avoid inserting element by element
+            std::vector<std::string> jsonStringArray;
+            for (const auto& elem : it.value()) {
+                ss << elem;
+                jsonStringArray.push_back(std::string(ppx::string_util::TrimBothEnds(ss.str(), " \t\"")));
+                ss.str("");
+            }
+            cliOptions.AddOption(it.key(), jsonStringArray);
+            continue;
         }
-        else {
-            // Do not assign "1" in case it's an option lacking a parameter
-            mOpts.AddOption(name, "");
+
+        ss << it.value();
+        std::string value = ss.str();
+        ss.str("");
+        if (auto error = ParseOption(cliOptions, it.key(), ppx::string_util::TrimBothEnds(value, " \t\""))) {
+            return error;
         }
     }
+    return std::nullopt;
+}
 
+std::optional<CommandLineParser::ParsingError> CommandLineParser::ParseOption(CliOptions& cliOptions, std::string_view optionName, std::string_view valueStr)
+{
+    if (optionName.length() > 2 && optionName.substr(0, 3) == "no-") {
+        if (valueStr.length() > 0) {
+            return "invalid prefix no- for option \"" + std::string(optionName) + "\" and value \"" + std::string(valueStr) + "\"";
+        }
+        optionName = optionName.substr(3);
+        valueStr   = "0";
+    }
+    cliOptions.AddOption(optionName, valueStr);
     return std::nullopt;
 }