config_parser: Improve error messages (#2522)

* config_parser: Improve error messages

This commit adds an additional context with configuration file name and
line number to some error messages in the config_parser.

* config_parser: Add file name and line number to invalid_name_error

* config_parser: Update unit tests to avoid crashes on regressions

include/components/config_parser.hpp

@@ -40,6 +40,9 @@ class invalid_name_error : public syntax_error {
    */
   invalid_name_error(const string& type, const string& name)
       : syntax_error(type + " name '" + name + "' is empty or contains forbidden characters.") {}
+
+  invalid_name_error(const string& type, const string& name, const string& file, int line_no)
+      : syntax_error(type + " name '" + name + "' is empty or contains forbidden characters.", file, line_no) {}
 };
 
 /**
@@ -122,7 +125,7 @@ class config_parser {
   void parse_file(const string& file, file_list path);
 
   /**
-   * \brief Parses the given line string to create a line_t struct
+   * \brief Parses the given line string to the given line_t struct
    *
    * We use the INI file syntax (https://en.wikipedia.org/wiki/INI_file)
    * Whitespaces (tested with isspace()) at the beginning and end of a line are ignored
@@ -155,7 +158,7 @@ class config_parser {
    *         doesn't know about those. Whoever calls parse_line needs to
    *         catch those exceptions and set the file path and line number
    */
-  line_t parse_line(const string& line);
+  void parse_line(line_t& line, const string& line_str);
 
   /**
    * \brief Determines the type of a line read from a config file
@@ -178,7 +181,7 @@ class config_parser {
    * \throws syntax_error if the line doesn't end with ']' or the header name
    *         contains forbidden characters
    */
-  string parse_header(const string& line);
+  string parse_header(const line_t& line, const string& line_str);
 
   /**
    * \brief Parses a line containing a key-value pair and returns the key name
@@ -188,13 +191,13 @@ class config_parser {
    *
    * \throws syntax_error if the key contains forbidden characters
    */
-  std::pair<string, string> parse_key(const string& line);
+  std::pair<string, string> parse_key(const line_t& line, const string& line_str);
 
   /**
    * \brief Parses the given value, checks if the given value contains
    *        one or more unescaped backslashes and logs an error if yes
    */
-  string parse_escaped_value(string&& value, const string& key);
+  string parse_escaped_value(const line_t& line, string&& value, const string& key);
 
   /**
    * \brief Name of all the files the config includes values from

src/components/config_parser.cpp

@@ -136,19 +136,9 @@ void config_parser::parse_file(const string& file, file_list path) {
   while (std::getline(in, line_str)) {
     line_no++;
     line_t line;
-    try {
+    line.file_index = file_index;
-      line = parse_line(line_str);
+    line.line_no = line_no;
-
+    parse_line(line, line_str);
-      // parse_line doesn't set these
-      line.file_index = file_index;
-      line.line_no = line_no;
-    } catch (syntax_error& err) {
-      /*
-       * Exceptions thrown by parse_line doesn't have the line
-       * numbers and files set, so we have to add them here
-       */
-      throw syntax_error(err.get_msg(), m_files[file_index], line_no);
-    }
 
     // Skip useless lines (comments, empty lines)
     if (!line.useful) {
@@ -170,39 +160,36 @@ void config_parser::parse_file(const string& file, file_list path) {
   }
 }
 
-line_t config_parser::parse_line(const string& line) {
+void config_parser::parse_line(line_t& line, const string& line_str) {
-  if (string_util::contains(line, "\ufeff")) {
+  if (string_util::contains(line_str, "\ufeff")) {
     throw syntax_error(
-        "This config file uses UTF-8 with BOM, which is not supported. Please use plain UTF-8 without BOM.");
+        "This config file uses UTF-8 with BOM, which is not supported. Please use plain UTF-8 without BOM.",
+        m_files[line.file_index], line.line_no);
   }
 
-  string line_trimmed = string_util::trim(line, isspace);
+  string line_trimmed = string_util::trim(line_str, isspace);
   line_type type = get_line_type(line_trimmed);
 
-  line_t result = {};
-
   if (type == line_type::EMPTY || type == line_type::COMMENT) {
-    result.useful = false;
+    line.useful = false;
-    return result;
+    return;
   }
 
   if (type == line_type::UNKNOWN) {
-    throw syntax_error("Unknown line type: " + line_trimmed);
+    throw syntax_error("Unknown line type: " + line_trimmed, m_files[line.file_index], line.line_no);
   }
 
-  result.useful = true;
+  line.useful = true;
 
   if (type == line_type::HEADER) {
-    result.is_header = true;
+    line.is_header = true;
-    result.header = parse_header(line_trimmed);
+    line.header = parse_header(line, line_trimmed);
   } else if (type == line_type::KEY) {
-    result.is_header = false;
+    line.is_header = false;
-    auto key_value = parse_key(line_trimmed);
+    auto key_value = parse_key(line, line_trimmed);
-    result.key = key_value.first;
+    line.key = key_value.first;
-    result.value = key_value.second;
+    line.value = key_value.second;
   }
-
-  return result;
 }
 
 line_type config_parser::get_line_type(const string& line) {
@@ -228,36 +215,37 @@ line_type config_parser::get_line_type(const string& line) {
   }
 }
 
-string config_parser::parse_header(const string& line) {
+string config_parser::parse_header(const line_t& line, const string& line_str) {
-  if (line.back() != ']') {
+  if (line_str.back() != ']') {
-    throw syntax_error("Missing ']' in header '" + line + "'");
+    throw syntax_error("Missing ']' in header '" + line_str + "'", m_files[line.file_index], line.line_no);
   }
 
   // Stripping square brackets
-  string header = line.substr(1, line.size() - 2);
+  string header = line_str.substr(1, line_str.size() - 2);
 
   if (!is_valid_name(header)) {
-    throw invalid_name_error("Section", header);
+    throw invalid_name_error("Section", header, m_files[line.file_index], line.line_no);
   }
 
   if (m_reserved_section_names.find(header) != m_reserved_section_names.end()) {
-    throw syntax_error("'" + header + "' is reserved and cannot be used as a section name");
+    throw syntax_error(
+        "'" + header + "' is reserved and cannot be used as a section name", m_files[line.file_index], line.line_no);
   }
 
   return header;
 }
 
-std::pair<string, string> config_parser::parse_key(const string& line) {
+std::pair<string, string> config_parser::parse_key(const line_t& line, const string& line_str) {
-  size_t pos = line.find_first_of('=');
+  size_t pos = line_str.find_first_of('=');
 
-  string key = string_util::trim(line.substr(0, pos), isspace);
+  string key = string_util::trim(line_str.substr(0, pos), isspace);
-  string value = string_util::trim(line.substr(pos + 1), isspace);
+  string value = string_util::trim(line_str.substr(pos + 1), isspace);
 
   if (!is_valid_name(key)) {
-    throw invalid_name_error("Key", key);
+    throw invalid_name_error("Key", key, m_files[line.file_index], line.line_no);
   }
 
-  value = parse_escaped_value(move(value), key);
+  value = parse_escaped_value(line, move(value), key);
 
   /*
    * Only if the string is surrounded with double quotes, do we treat them
@@ -294,7 +282,7 @@ bool config_parser::is_valid_name(const string& name) {
   return true;
 }
 
-string config_parser::parse_escaped_value(string&& value, const string& key) {
+string config_parser::parse_escaped_value(const line_t& line, string&& value, const string& key) {
   string cfg_value = value;
   bool log = false;
   auto backslash_pos = value.find('\\');
@@ -307,11 +295,11 @@ string config_parser::parse_escaped_value(string&& value, const string& key) {
     backslash_pos = value.find('\\', backslash_pos + 1);
   }
   if (log) {
-    // TODO Log filename, and line number
     m_log.err(
-        "Value '%s' of key '%s' contains one or more unescaped backslashes, please prepend them with the backslash "
+        "%s:%d: Value '%s' of key '%s' contains one or more unescaped backslashes, please prepend them with the "
+        "backslash "
         "escape character.",
-        cfg_value, key);
+        m_files[line.file_index], line.line_no, cfg_value, key);
   }
   return move(value);
 }

tests/unit_tests/components/config_parser.cpp

@@ -12,6 +12,12 @@ using namespace std;
 class TestableConfigParser : public config_parser {
   using config_parser::config_parser;
 
+ public:
+  TestableConfigParser(const logger& logger, string&& file, string&& bar)
+      : config_parser(logger, move(file), move(bar)) {
+    m_files.push_back("test_config");
+  }
+
  public:
   using config_parser::get_line_type;
 
@@ -78,7 +84,10 @@ vector<pair<pair<string, string>, string>> parse_line_key_list = {
 INSTANTIATE_TEST_SUITE_P(Inst, ParseLineInValidTest, ::testing::ValuesIn(parse_line_invalid_list));
 
 TEST_P(ParseLineInValidTest, correctness) {
-  line_t line = parser->parse_line(GetParam());
+  line_t line;
+  line.file_index = 0;
+  line.line_no = 0;
+  parser->parse_line(line, GetParam());
 
   EXPECT_FALSE(line.useful);
 }
@@ -86,7 +95,10 @@ TEST_P(ParseLineInValidTest, correctness) {
 INSTANTIATE_TEST_SUITE_P(Inst, ParseLineHeaderTest, ::testing::ValuesIn(parse_line_header_list));
 
 TEST_P(ParseLineHeaderTest, correctness) {
-  line_t line = parser->parse_line(GetParam().second);
+  line_t line;
+  line.file_index = 0;
+  line.line_no = 0;
+  parser->parse_line(line, GetParam().second);
 
   EXPECT_TRUE(line.useful);
 
@@ -97,7 +109,10 @@ TEST_P(ParseLineHeaderTest, correctness) {
 INSTANTIATE_TEST_SUITE_P(Inst, ParseLineKeyTest, ::testing::ValuesIn(parse_line_key_list));
 
 TEST_P(ParseLineKeyTest, correctness) {
-  line_t line = parser->parse_line(GetParam().second);
+  line_t line;
+  line.file_index = 0;
+  line.line_no = 0;
+  parser->parse_line(line, GetParam().second);
 
   EXPECT_TRUE(line.useful);
 
@@ -107,8 +122,12 @@ TEST_P(ParseLineKeyTest, correctness) {
 }
 
 TEST_F(ParseLineInValidTest, throwsSyntaxError) {
-  EXPECT_THROW(parser->parse_line("unknown"), syntax_error);
+  line_t line;
-  EXPECT_THROW(parser->parse_line("\ufeff"), syntax_error);
+  line.file_index = 0;
+  line.line_no = 0;
+
+  EXPECT_THROW(parser->parse_line(line, "unknown"), syntax_error);
+  EXPECT_THROW(parser->parse_line(line, "\ufeff"), syntax_error);
 }
 // }}}
 
@@ -192,16 +211,23 @@ INSTANTIATE_TEST_SUITE_P(Inst, ParseKeyTest, ::testing::ValuesIn(parse_key_list)
  * Parameterized test for parse_key with valid line
  */
 TEST_P(ParseKeyTest, correctness) {
-  EXPECT_EQ(GetParam().first, parser->parse_key(GetParam().second));
+  line_t line;
+  line.file_index = 0;
+  line.line_no = 0;
+  EXPECT_EQ(GetParam().first, parser->parse_key(line, GetParam().second));
 }
 
 /**
  * Tests if exception is thrown for invalid key line
  */
 TEST_F(ParseKeyTest, throwsSyntaxError) {
-  EXPECT_THROW(parser->parse_key("= empty name"), syntax_error);
+  line_t line;
-  EXPECT_THROW(parser->parse_key("forbidden char = value"), syntax_error);
+  line.file_index = 0;
-  EXPECT_THROW(parser->parse_key("forbidden\tchar = value"), syntax_error);
+  line.line_no = 0;
+
+  EXPECT_THROW(parser->parse_key(line, "= empty name"), syntax_error);
+  EXPECT_THROW(parser->parse_key(line, "forbidden char = value"), syntax_error);
+  EXPECT_THROW(parser->parse_key(line, "forbidden\tchar = value"), syntax_error);
 }
 // }}}
 
@@ -227,22 +253,29 @@ INSTANTIATE_TEST_SUITE_P(Inst, ParseHeaderTest, ::testing::ValuesIn(parse_header
  * Parameterized test for parse_header with valid line
  */
 TEST_P(ParseHeaderTest, correctness) {
-  EXPECT_EQ(GetParam().first, parser->parse_header(GetParam().second));
+  line_t line;
+  line.file_index = 0;
+  line.line_no = 0;
+  EXPECT_EQ(GetParam().first, parser->parse_header(line, GetParam().second));
 }
 
 /**
  * Tests if exception is thrown for invalid header line
  */
 TEST_F(ParseHeaderTest, throwsSyntaxError) {
-  EXPECT_THROW(parser->parse_header("[]"), syntax_error);
+  line_t line;
-  EXPECT_THROW(parser->parse_header("[no_end"), syntax_error);
+  line.file_index = 0;
-  EXPECT_THROW(parser->parse_header("[forbidden char]"), syntax_error);
+  line.line_no = 0;
-  EXPECT_THROW(parser->parse_header("[forbidden\tchar]"), syntax_error);
+
+  EXPECT_THROW(parser->parse_header(line, "[]"), syntax_error);
+  EXPECT_THROW(parser->parse_header(line, "[no_end"), syntax_error);
+  EXPECT_THROW(parser->parse_header(line, "[forbidden char]"), syntax_error);
+  EXPECT_THROW(parser->parse_header(line, "[forbidden\tchar]"), syntax_error);
 
   // Reserved names
-  EXPECT_THROW(parser->parse_header("[self]"), syntax_error);
+  EXPECT_THROW(parser->parse_header(line, "[self]"), syntax_error);
-  EXPECT_THROW(parser->parse_header("[BAR]"), syntax_error);
+  EXPECT_THROW(parser->parse_header(line, "[BAR]"), syntax_error);
-  EXPECT_THROW(parser->parse_header("[root]"), syntax_error);
+  EXPECT_THROW(parser->parse_header(line, "[root]"), syntax_error);
 }
 // }}}
 
@@ -271,8 +304,11 @@ INSTANTIATE_TEST_SUITE_P(Inst, ParseEscapedValueTest, ::testing::ValuesIn(parse_
  * Parameterized test for parse_escaped_value with valid line
  */
 TEST_P(ParseEscapedValueTest, correctness) {
+  line_t line;
+  line.file_index = 0;
+  line.line_no = 0;
   string value = GetParam().second;
-  value = parser->parse_escaped_value(move(value), "key");
+  value = parser->parse_escaped_value(line, move(value), "key");
   EXPECT_EQ(GetParam().first, value);
 }
 // }}}