From 40ae9b210b911cbc6554069222a773a6e382871f Mon Sep 17 00:00:00 2001 From: Georgiy Komarov Date: Sun, 3 Oct 2021 21:46:16 +0300 Subject: [PATCH] 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 | 13 +-- src/components/config_parser.cpp | 82 ++++++++----------- tests/unit_tests/components/config_parser.cpp | 72 ++++++++++++---- 3 files changed, 97 insertions(+), 70 deletions(-) diff --git a/include/components/config_parser.hpp b/include/components/config_parser.hpp index f341074a..dc0900c5 100644 --- a/include/components/config_parser.hpp +++ b/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 parse_key(const string& line); + std::pair 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 diff --git a/src/components/config_parser.cpp b/src/components/config_parser.cpp index eb97a2c0..090d0550 100644 --- a/src/components/config_parser.cpp +++ b/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 = parse_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); - } + line.file_index = file_index; + line.line_no = line_no; + parse_line(line, line_str); // 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) { - if (string_util::contains(line, "\ufeff")) { +void config_parser::parse_line(line_t& line, const string& line_str) { + 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; - return result; + line.useful = false; + 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; - result.header = parse_header(line_trimmed); + line.is_header = true; + line.header = parse_header(line, line_trimmed); } else if (type == line_type::KEY) { - result.is_header = false; - auto key_value = parse_key(line_trimmed); - result.key = key_value.first; - result.value = key_value.second; + line.is_header = false; + auto key_value = parse_key(line, line_trimmed); + line.key = key_value.first; + 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) { - if (line.back() != ']') { - throw syntax_error("Missing ']' in header '" + line + "'"); +string config_parser::parse_header(const line_t& line, const string& line_str) { + if (line_str.back() != ']') { + 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 config_parser::parse_key(const string& line) { - size_t pos = line.find_first_of('='); +std::pair config_parser::parse_key(const line_t& line, const string& line_str) { + size_t pos = line_str.find_first_of('='); - string key = string_util::trim(line.substr(0, pos), isspace); - string value = string_util::trim(line.substr(pos + 1), isspace); + string key = string_util::trim(line_str.substr(0, pos), 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); } diff --git a/tests/unit_tests/components/config_parser.cpp b/tests/unit_tests/components/config_parser.cpp index 71b118c7..05d55220 100644 --- a/tests/unit_tests/components/config_parser.cpp +++ b/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, 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); - EXPECT_THROW(parser->parse_line("\ufeff"), syntax_error); + line_t line; + 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); - EXPECT_THROW(parser->parse_key("forbidden char = value"), syntax_error); - EXPECT_THROW(parser->parse_key("forbidden\tchar = value"), syntax_error); + line_t line; + line.file_index = 0; + 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); - EXPECT_THROW(parser->parse_header("[no_end"), syntax_error); - EXPECT_THROW(parser->parse_header("[forbidden char]"), syntax_error); - EXPECT_THROW(parser->parse_header("[forbidden\tchar]"), syntax_error); + line_t line; + line.file_index = 0; + line.line_no = 0; + + 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("[BAR]"), syntax_error); - EXPECT_THROW(parser->parse_header("[root]"), syntax_error); + EXPECT_THROW(parser->parse_header(line, "[self]"), syntax_error); + EXPECT_THROW(parser->parse_header(line, "[BAR]"), 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); } // }}}