From c5d6e2e67cac238868df3522a2538a8c5843a63a Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 1 Oct 2020 14:02:53 +0300 Subject: [PATCH 1/4] Add context to error messages --- src/Common/Exception.h | 7 +++++-- src/Core/BaseSettings.h | 22 +++++++++++++++------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/Common/Exception.h b/src/Common/Exception.h index 763b90048bb..d0de8d6a3f2 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -39,8 +39,11 @@ public: const char * name() const throw() override { return "DB::Exception"; } const char * what() const throw() override { return message().data(); } - /// Add something to the existing message. - void addMessage(const std::string & arg) { extendedMessage(arg); } + template + void addMessage(Fmt&&... fmt) + { + extendedMessage(fmt::format(std::forward(fmt)...)); + } std::string getStackTraceString() const; diff --git a/src/Core/BaseSettings.h b/src/Core/BaseSettings.h index 7de87b345c1..b193fdd4c93 100644 --- a/src/Core/BaseSettings.h +++ b/src/Core/BaseSettings.h @@ -390,13 +390,21 @@ String BaseSettings::valueToStringUtil(const std::string_view & name, c template Field BaseSettings::stringToValueUtil(const std::string_view & name, const String & str) { - const auto & accessor = Traits::Accessor::instance(); - if (size_t index = accessor.find(name); index != static_cast(-1)) - return accessor.stringToValueUtil(index, str); - if constexpr (Traits::allow_custom_settings) - return Field::restoreFromDump(str); - else - BaseSettingsHelpers::throwSettingNotFound(name); + try + { + const auto & accessor = Traits::Accessor::instance(); + if (size_t index = accessor.find(name); index != static_cast(-1)) + return accessor.stringToValueUtil(index, str); + if constexpr (Traits::allow_custom_settings) + return Field::restoreFromDump(str); + else + BaseSettingsHelpers::throwSettingNotFound(name); + } + catch (Exception & e) + { + e.addMessage("while parsing value '{}' for setting '{}'", str, name); + throw; + } } template From c20053351c0f09d772fcdeedf810f3165d08f19e Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 1 Oct 2020 17:23:48 +0300 Subject: [PATCH 2/4] Make the test throw more comprehensive errors --- .../gtest_exception_on_incorrect_pipeline.cpp | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/Processors/tests/gtest_exception_on_incorrect_pipeline.cpp b/src/Processors/tests/gtest_exception_on_incorrect_pipeline.cpp index 10a0fae2696..3d85ffede9a 100644 --- a/src/Processors/tests/gtest_exception_on_incorrect_pipeline.cpp +++ b/src/Processors/tests/gtest_exception_on_incorrect_pipeline.cpp @@ -49,21 +49,15 @@ TEST(Processors, PortsNotConnected) processors.emplace_back(std::move(source)); processors.emplace_back(std::move(sink)); - auto exec = [&]() + try { - - try - { - PipelineExecutor executor(processors); - executor.execute(1); - } - catch (DB::Exception & e) - { - std::cout << e.displayText() << std::endl; - ASSERT_TRUE(e.displayText().find("pipeline") != std::string::npos); - throw; - } - }; - - ASSERT_THROW(exec(), DB::Exception); + PipelineExecutor executor(processors); + executor.execute(1); + ASSERT_TRUE(false) << "Should have thrown."; + } + catch (DB::Exception & e) + { + std::cout << e.displayText() << std::endl; + ASSERT_TRUE(e.displayText().find("pipeline") != std::string::npos) << "Expected 'pipeline', got: " << e.displayText(); + } } From 6e07bfe7bc93721676ec0b7f99ef267e26acea80 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Thu, 1 Oct 2020 17:29:22 +0300 Subject: [PATCH 3/4] return the comment --- src/Common/Exception.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Common/Exception.h b/src/Common/Exception.h index d0de8d6a3f2..09d923fe60e 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -39,6 +39,7 @@ public: const char * name() const throw() override { return "DB::Exception"; } const char * what() const throw() override { return message().data(); } + /// Add something to the existing message. template void addMessage(Fmt&&... fmt) { From 3be7c7745e2e455478489b5970762ada4d434d62 Mon Sep 17 00:00:00 2001 From: Alexander Kuzmenkov Date: Fri, 2 Oct 2020 14:06:38 +0300 Subject: [PATCH 4/4] Use a separate overload for formatting with fmt --- src/Common/Exception.h | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/Common/Exception.h b/src/Common/Exception.h index 09d923fe60e..314c59cbf51 100644 --- a/src/Common/Exception.h +++ b/src/Common/Exception.h @@ -22,10 +22,14 @@ public: Exception() = default; Exception(const std::string & msg, int code); + Exception(int code, const std::string & message) + : Exception(message, code) + {} + // Format message with fmt::format, like the logging functions. - template - Exception(int code, Fmt&&... fmt) - : Exception(fmt::format(std::forward(fmt)...), code) + template + Exception(int code, const std::string & fmt, Args&&... args) + : Exception(fmt::format(fmt, std::forward(args)...), code) {} struct CreateFromPocoTag {}; @@ -40,10 +44,15 @@ public: const char * what() const throw() override { return message().data(); } /// Add something to the existing message. - template - void addMessage(Fmt&&... fmt) + template + void addMessage(const std::string& format, Args&&... args) { - extendedMessage(fmt::format(std::forward(fmt)...)); + extendedMessage(fmt::format(format, std::forward(args)...)); + } + + void addMessage(const std::string& message) + { + extendedMessage(message); } std::string getStackTraceString() const;