config: log parsing Errors to a ParseResult object (which is queryable)

rwgame/GameConfig.cpp

@@ -13,7 +13,7 @@ GameConfig::GameConfig(const std::string& configName,
                        const std::string& configPath)
     : m_configName(configName)
     , m_configPath(configPath)
-    , m_valid(false)
+    , m_parseResult()
     , m_inputInvertY(false) {
     if (m_configPath.empty()) {
         m_configPath = getDefaultConfigPath();
@@ -23,7 +23,7 @@ GameConfig::GameConfig(const std::string& configName,
     auto configFile = getConfigFile();
 
     std::string dummy;
-    m_valid = parseConfig(ParseType::FILE, configFile, ParseType::CONFIG, dummy);
+    m_parseResult = parseConfig(ParseType::FILE, configFile, ParseType::CONFIG, dummy);
 }
 
 std::string GameConfig::getConfigFile() const {
@@ -31,7 +31,11 @@ std::string GameConfig::getConfigFile() const {
 }
 
 bool GameConfig::isValid() const {
-    return m_valid;
+    return m_parseResult.isValid();
+}
+
+const GameConfig::ParseResult &GameConfig::getParseResult() const {
+    return m_parseResult;
 }
 
 std::string GameConfig::getDefaultConfigPath() {
@@ -109,7 +113,7 @@ struct IntTranslator {
     }
 };
 
-bool GameConfig::saveConfig() {
+GameConfig::ParseResult GameConfig::saveConfig() {
     auto filename = getConfigFile();
     return parseConfig(ParseType::CONFIG, "",
         ParseType::FILE, filename);
@@ -121,32 +125,32 @@ std::string GameConfig::getDefaultINIString() {
     return result;
 }
 
-bool GameConfig::parseConfig(
+GameConfig::ParseResult GameConfig::parseConfig(
     GameConfig::ParseType srcType, const std::string &source,
     ParseType destType, std::string &destination)
 {
     pt::ptree srcTree;
+    ParseResult parseResult;
 
     try {
         if (srcType == ParseType::STRING) {
             pt::read_ini(source, srcTree);
         } else if (srcType == ParseType::FILE) {
-            std::ifstream ifs(source);
+            pt::read_ini(source, srcTree);
-            pt::read_ini(ifs, srcTree);
         }
     } catch (pt::ini_parser_error &e) {
         // Catches illegal input files (nonsensical input, duplicate keys)
+        parseResult.failInputFile(e.filename(), e.line(), e.message());
         RW_MESSAGE(e.what());
-        return false;
+        return parseResult;
     }
 
     if (destType == ParseType::DEFAULT) {
+        parseResult.failArgument();
         RW_ERROR("Target cannot be DEFAULT.");
-        return false;
+        return parseResult;
     }
 
-    bool success = true;
-    
     auto read_config = [&](const std::string &key, auto &target,
                           const auto &defaultValue, auto &translator,
                           bool optional=true) {
@@ -168,15 +172,14 @@ bool GameConfig::parseConfig(
                 } catch (pt::ptree_bad_path &e) {
                     // Catches missing key-value pairs: fail when required
                     if (!optional) {
-                      success = false;
+                        parseResult.failRequiredMissing(key);
-                      RW_MESSAGE(e.what());
+                        RW_MESSAGE(e.what());
-                      return;
+                        return;
                     }
                     sourceValue = defaultValue;
                 } catch (pt::ptree_bad_data &e) {
                     // Catches illegal value data: always fail
-                    success = false;
+                    parseResult.failInvalidData(key);
-                    RW_MESSAGE("invalid data");
                     RW_MESSAGE(e.what());
                     return;
                 }
@@ -187,7 +190,7 @@ bool GameConfig::parseConfig(
         switch (destType) {
             case ParseType::DEFAULT:
                 // Target cannot be DEFAULT (case already handled)
-                success = false;
+                parseResult.failArgument();
                 break;
             case ParseType::CONFIG:
                 // Don't care if success == false
@@ -211,8 +214,8 @@ bool GameConfig::parseConfig(
 
     read_config("input.invert_y", this->m_inputInvertY, false, boolt);
 
-    if (!success)
+    if (!parseResult.isValid())
-        return success;
+        return parseResult;
 
     try {
         if (destType == ParseType::STRING) {
@@ -223,9 +226,64 @@ bool GameConfig::parseConfig(
             pt::write_ini(destination, srcTree);
         }
     } catch (pt::ini_parser_error &e) {
-        success = false;
+        parseResult.failOutputFile(e.filename(), e.line(), e.message());
         RW_MESSAGE(e.what());
     }
 
-    return success;
+    return parseResult;
 }
+
+GameConfig::ParseResult::ParseResult()
+    : m_result(ErrorType::GOOD)
+    , m_filename()
+    , m_line(0)
+    , m_message()
+    , m_keys_requiredMissing()
+    , m_keys_invalidData() {
+}
+
+GameConfig::ParseResult::ErrorType GameConfig::ParseResult::type() const {
+    return this->m_result;
+}
+
+bool GameConfig::ParseResult::isValid() const {
+    return this->type() == ErrorType::GOOD;
+}
+
+void GameConfig::ParseResult::failInputFile(const std::string &filename, size_t line,
+        const std::string &message) {
+    this->m_result = ParseResult::ErrorType::INVALIDINPUTFILE;
+    this->m_filename = filename;
+    this->m_line = line;
+    this->m_message = message;
+}
+
+void GameConfig::ParseResult::failArgument() {
+    this->m_result = ParseResult::ErrorType::INVALIDARGUMENT;
+}
+
+void GameConfig::ParseResult::failRequiredMissing(const std::string &key) {
+    this->m_result = ParseResult::ErrorType::INVALIDCONTENT;
+    this->m_keys_requiredMissing.push_back(key);
+}
+
+void GameConfig::ParseResult::failInvalidData(const std::string &key) {
+    this->m_result = ParseResult::ErrorType::INVALIDCONTENT;
+    this->m_keys_invalidData.push_back(key);
+}
+
+void GameConfig::ParseResult::failOutputFile(const std::string &filename, size_t line, const std::string &message) {
+    this->m_result = ParseResult::ErrorType::INVALIDOUTPUTFILE;
+    this->m_filename = filename;
+    this->m_line = line;
+    this->m_message = message;
+}
+
+const std::vector<std::string> &GameConfig::ParseResult::getKeysRequiredMissing() const {
+    return this->m_keys_requiredMissing;
+}
+
+const std::vector<std::string> &GameConfig::ParseResult::getKeysInvalidData() const {
+    return this->m_keys_invalidData;
+}
+

rwgame/GameConfig.hpp

@@ -1,9 +1,104 @@
 #ifndef RWGAME_GAMECONFIG_HPP
 #define RWGAME_GAMECONFIG_HPP
 #include <string>
+#include <vector>
 
 class GameConfig {
 public:
+    class ParseResult {
+    public:
+        enum ErrorType {
+            /// GOOD: Input file/string was good
+            GOOD,
+            /// INVALIDINPUTFILE: There was some error while reading from a file or string or the input was ambiguous (e.g. duplicate keys)
+            INVALIDINPUTFILE,
+            /// INVALIDARGUMENT: The parser received impossible arguments
+            INVALIDARGUMENT,
+            /// INVALIDCONTENT: Some required keys were missing or some values were of incorrect type
+            INVALIDCONTENT,
+            /// INVALIDOUTPUTFILE: There was some error while writing to a file or string
+            INVALIDOUTPUTFILE
+        };
+        /**
+         * @brief ParseResult Holds the issues occurred while parsing of a config file.
+         */
+
+        ParseResult();
+        /**
+         * @brief type Get the type of error
+         * @return Type of error or GOOD if there was no error
+         */
+        ErrorType type() const;
+
+        /**
+         * @brief getKeysRequiredMissing Get the keys that were missing
+         * @return A vector with all the keys
+         */
+        const std::vector<std::string> &getKeysRequiredMissing() const;
+
+        /**
+         * @brief getKeysInvalidData Get the keys that contained invalid data
+         * @return A vector with all the keys
+         */
+        const std::vector<std::string> &getKeysInvalidData() const;
+        /**
+         * @brief failInputFile Fail because the input file was invalid
+         * @param filename Filename of the invalid file
+         * @param line Line number where the error is located
+         * @param message Description of the error
+         */
+        void failInputFile(const std::string &filename, size_t line, const std::string &message);
+
+        /**
+         * @brief failArgument Fail because an argument was invalid
+         */
+        void failArgument();
+
+        /**
+         * @brief failRequiredMissing Fail because a required key is missing
+         * @param key The key that is missing
+         */
+        void failRequiredMissing(const std::string &key);
+
+        /**
+         * @brief failInvalidData Fail because a key contains invalid data
+         * @param key The key that contains invalid data
+         */
+        void failInvalidData(const std::string &key);
+
+        /**
+         * @brief failOutputFile Fail because an error occurred while while writing to the output
+         * @param filename Filename of the invalid file
+         * @param line Line number where the error is located
+         * @param message Description of the error
+         */
+        void failOutputFile(const std::string &filename, size_t line, const std::string &message);
+
+        /**
+         * @brief isValid
+         * @return True if the loaded configuration is valid
+         */
+        bool isValid() const;
+    private:
+        /// Type of the failure
+        ErrorType m_result;
+
+        /// Filename of the invalid input or output file
+        std::string m_filename;
+
+        /// Line number where the failure occurred (on invalid input or output file)
+        size_t m_line;
+
+        /// Description of the failure (on invalid input or output file)
+        std::string m_message;
+
+        /// All required keys that are missing
+        std::vector<std::string> m_keys_requiredMissing;
+
+        /// All keys that contain invalid data
+        std::vector<std::string> m_keys_invalidData;
+    };
+
     /**
      * @brief GameConfig Loads a game configuration
      * @param configName The configuration filename to load
@@ -20,7 +115,7 @@ public:
     /**
      * @brief writeConfig Save the game configuration
      */
-    bool saveConfig();
+    ParseResult saveConfig();
 
     /**
      * @brief isValid
@@ -28,6 +123,12 @@ public:
      */
     bool isValid() const;
 
+    /**
+     * @brief getParseResult Get more information on parsing failures
+     * @return A ParseResult object  containing more information
+     */
+    const ParseResult &getParseResult() const;
+
     /**
      * @brief getConfigString Returns the content of the default INI configuration.
      * @return INI string
@@ -66,13 +167,13 @@ private:
      *                    INI string if srcType == STRING
      * @return True if the parsing succeeded
      */
-    bool parseConfig(ParseType srcType, const std::string &source,
+    ParseResult parseConfig(ParseType srcType, const std::string &source,
         ParseType destType, std::string &destination);
 
     /* Config State */
     std::string m_configName;
     std::string m_configPath;
-    bool m_valid;
+    ParseResult m_parseResult;
 
     /* Actual Configuration */
 

tests/test_config.cpp

@@ -58,13 +58,27 @@ public:
     std::string dirname() {
         return this->m_path.parent_path().string();
     }
+    void change_perms_readonly() {
+        fs::permissions(this->m_path, fs::perms::owner_read
+            | fs::perms::group_read | fs::perms::others_read);
+    }
     template<typename T>
-    void write(T t) {
+    bool append(T t) {
         // Append argument at the end of the file.
         // File is open/closes repeatedly. Not optimal.
         std::ofstream ofs(this->path(), std::ios::out | std::ios::app);
         ofs << t;
         ofs.close();
+        return ofs.good();
+    }
+    template<typename T>
+    bool write(T t) {
+        // Write the argument to the file, discarding all contents.
+        // File is open/closes repeatedly. Not optimal.
+        std::ofstream ofs(this->path(), std::ios::out | std::ios::trunc);
+        ofs << t;
+        ofs.close();
+        return ofs.good();
     }
 private:
     static fs::path getRandomFilePath() {
@@ -88,14 +102,18 @@ BOOST_AUTO_TEST_CASE(test_TempFile) {
     BOOST_CHECK_EQUAL(tempFile.exists(), true);
     tempFile.remove();
 
-    tempFile.write("abc");
+    BOOST_CHECK_EQUAL(tempFile.append("abc"), true);
-    tempFile.write("def");
+    BOOST_CHECK_EQUAL(tempFile.append("def"), true);
     BOOST_CHECK_EQUAL(tempFile.exists(), true);
     tempFile.touch();
     std::ifstream ifs(tempFile.path());
     std::string line;
     std::getline(ifs, line);
     BOOST_CHECK_EQUAL(line, "abcdef");
+
+    tempFile.change_perms_readonly();
+    BOOST_CHECK_EQUAL(tempFile.write("abc"), false);
+    BOOST_CHECK_EQUAL(tempFile.append("def"), false);
 }
 
 BOOST_AUTO_TEST_CASE(test_config_valid) {
@@ -103,11 +121,14 @@ BOOST_AUTO_TEST_CASE(test_config_valid) {
     auto cfg = getValidConfig();
 
     TempFile tempFile;
-    tempFile.write(cfg);
+    tempFile.append(cfg);
 
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(config.isValid());
+    BOOST_CHECK_EQUAL(config.getParseResult().type(), GameConfig::ParseResult::GOOD);
+    BOOST_CHECK_EQUAL(config.getParseResult().getKeysRequiredMissing().size(), 0);
+    BOOST_CHECK_EQUAL(config.getParseResult().getKeysInvalidData().size(), 0);
 
     BOOST_CHECK_EQUAL(config.getGameDataPath(), "/dev/test");
     BOOST_CHECK_EQUAL(config.getGameLanguage(), "american");
@@ -121,11 +142,14 @@ BOOST_AUTO_TEST_CASE(test_config_valid_modified) {
     cfg["input"]["invert_y"] = "0";
 
     TempFile tempFile;
-    tempFile.write(cfg);
+    tempFile.append(cfg);
 
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(config.isValid());
+    BOOST_CHECK_EQUAL(config.getParseResult().type(), GameConfig::ParseResult::GOOD);
+    BOOST_CHECK_EQUAL(config.getParseResult().getKeysRequiredMissing().size(), 0);
+    BOOST_CHECK_EQUAL(config.getParseResult().getKeysInvalidData().size(), 0);
 
     BOOST_CHECK_EQUAL(config.getInputInvertY(), false);
     BOOST_CHECK_EQUAL(config.getGameDataPath(), "Liberty City");
@@ -137,7 +161,7 @@ BOOST_AUTO_TEST_CASE(test_config_save) {
     cfg["game"]["path"] = "Liberty City";
 
     TempFile tempFile;
-    tempFile.write(cfg);
+    tempFile.append(cfg);
 
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
@@ -146,14 +170,30 @@ BOOST_AUTO_TEST_CASE(test_config_save) {
     tempFile.remove();
     BOOST_CHECK(!tempFile.exists());
 
-    BOOST_CHECK(config.saveConfig());
+    auto writeResult = config.saveConfig();
+    BOOST_CHECK(writeResult.isValid());
     BOOST_CHECK(tempFile.exists());
 
     GameConfig config2(tempFile.filename(), tempFile.dirname());
-
     BOOST_CHECK_EQUAL(config2.getGameDataPath(), "Liberty City");
 }
 
+BOOST_AUTO_TEST_CASE(test_config_save_readonly) {
+    // Test whether saving to a readonly INI file fails
+    auto cfg = getValidConfig();
+
+    TempFile tempFile;
+    tempFile.append(cfg);
+    tempFile.change_perms_readonly();
+
+    GameConfig config(tempFile.filename(), tempFile.dirname());
+    BOOST_CHECK_EQUAL(config.isValid(), true);
+
+    auto writeResult = config.saveConfig();
+    BOOST_CHECK(!writeResult.isValid());
+    BOOST_CHECK_EQUAL(writeResult.type(), GameConfig::ParseResult::INVALIDOUTPUTFILE);
+}
+
 BOOST_AUTO_TEST_CASE(test_config_valid_default) {
     // Test whether the default INI string is valid
     TempFile tempFile;
@@ -163,7 +203,7 @@ BOOST_AUTO_TEST_CASE(test_config_valid_default) {
     BOOST_CHECK(!config.isValid());
 
     auto defaultINI = config.getDefaultINIString();
-    tempFile.write(defaultINI);
+    tempFile.append(defaultINI);
 
     config = GameConfig(tempFile.filename(), tempFile.dirname());
     BOOST_CHECK(config.isValid());
@@ -175,11 +215,13 @@ BOOST_AUTO_TEST_CASE(test_config_invalid_duplicate) {
     cfg["input"]["invert_y    "] = "0";
 
     TempFile tempFile;
-    tempFile.write(cfg);
+    tempFile.append(cfg);
 
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(!config.isValid());
+    const auto &parseResult = config.getParseResult();
+    BOOST_CHECK_EQUAL(parseResult.type(), GameConfig::ParseResult::INVALIDINPUTFILE);
 }
 
 BOOST_AUTO_TEST_CASE(test_config_invalid_required_missing) {
@@ -188,11 +230,19 @@ BOOST_AUTO_TEST_CASE(test_config_invalid_required_missing) {
     cfg["game"].erase("path");
 
     TempFile tempFile;
-    tempFile.write(cfg);
+    tempFile.append(cfg);
 
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(!config.isValid());
+
+    const auto &parseResult = config.getParseResult();
+    BOOST_CHECK_EQUAL(parseResult.type(), GameConfig::ParseResult::INVALIDCONTENT);
+
+    BOOST_CHECK_EQUAL(parseResult.getKeysRequiredMissing().size(), 1);
+    BOOST_CHECK_EQUAL(parseResult.getKeysInvalidData().size(), 0);
+
+    BOOST_CHECK_EQUAL(parseResult.getKeysRequiredMissing()[0], "game.path");
 }
 
 BOOST_AUTO_TEST_CASE(test_config_invalid_wrong_type) {
@@ -201,16 +251,24 @@ BOOST_AUTO_TEST_CASE(test_config_invalid_wrong_type) {
     cfg["input"]["invert_y"]="d";
 
     TempFile tempFile;
-    tempFile.write(cfg);
+    tempFile.append(cfg);
 
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(!config.isValid());
+
+    const auto &parseResult = config.getParseResult();
+    BOOST_CHECK_EQUAL(parseResult.type(), GameConfig::ParseResult::INVALIDCONTENT);
+
+    BOOST_CHECK_EQUAL(parseResult.getKeysRequiredMissing().size(), 0);
+    BOOST_CHECK_EQUAL(parseResult.getKeysInvalidData().size(), 1);
+
+    BOOST_CHECK_EQUAL(parseResult.getKeysInvalidData()[0], "input.invert_y");
 }
 
 BOOST_AUTO_TEST_CASE(test_config_invalid_empty) {
     // Test reading empty configuration file
-
+    // An empty file has a valid data structure, but has missing keys and is thus invalid.
     TempFile tempFile;
     tempFile.touch();
     BOOST_CHECK(tempFile.exists());
@@ -218,6 +276,10 @@ BOOST_AUTO_TEST_CASE(test_config_invalid_empty) {
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(!config.isValid());
+
+    const auto &parseResult = config.getParseResult();
+    BOOST_CHECK_EQUAL(parseResult.type(), GameConfig::ParseResult::INVALIDCONTENT);
+    BOOST_CHECK_GE(parseResult.getKeysRequiredMissing().size(), 1);
 }
 
 BOOST_AUTO_TEST_CASE(test_config_invalid_nonexisting) {
@@ -228,6 +290,9 @@ BOOST_AUTO_TEST_CASE(test_config_invalid_nonexisting) {
     GameConfig config(tempFile.filename(), tempFile.dirname());
 
     BOOST_CHECK(!config.isValid());
+
+    const auto &parseResult = config.getParseResult();
+    BOOST_CHECK_EQUAL(parseResult.type(), GameConfig::ParseResult::INVALIDINPUTFILE);
 }
 
 BOOST_AUTO_TEST_SUITE_END()