Merge pull request ClickHouse#78725 from azat/extract-from-config-better-try

azat · web-flow · commit 81a654b7cd38 · 2025-04-07T10:24:09.000Z
Ignore non existing include_from files with --try for clickhouse-extract-from-config
diff --git a/programs/extract-from-config/ExtractFromConfig.cpp b/programs/extract-from-config/ExtractFromConfig.cpp
@@ -41,7 +41,7 @@ static void setupLogging(const std::string & log_level)
 }
 
 
-static std::vector<std::string> extactFromConfigAccordingToGlobs(DB::ConfigurationPtr configuration, const std::string & pattern, bool try_get)
+static std::vector<std::string> extactFromConfigAccordingToGlobs(DB::ConfigurationPtr configuration, const std::string & pattern, bool ignore_errors)
 {
     auto pattern_prefix = pattern.substr(0, pattern.find_first_of("*?{"));
     boost::algorithm::trim_if(pattern_prefix, [](char s){ return s == '.'; });
@@ -68,7 +68,7 @@ static std::vector<std::string> extactFromConfigAccordingToGlobs(DB::Configurati
                 continue;
 
 
-            if (try_get)
+            if (ignore_errors)
             {
                 auto value = configuration->getString(node, "");
                 if (!value.empty())
@@ -90,9 +90,13 @@ static std::vector<std::string> extactFromConfigAccordingToGlobs(DB::Configurati
 }
 
 
-static DB::ConfigurationPtr get_configuration(const std::string & config_path, bool process_zk_includes)
+static DB::ConfigurationPtr get_configuration(const std::string & config_path, bool process_zk_includes, bool throw_on_bad_include_from)
 {
-    DB::ConfigProcessor processor(config_path, /* throw_on_bad_incl = */ false, /* log_to_console = */ false);
+    DB::ConfigProcessor processor(config_path,
+        /* throw_on_bad_incl = */ false,
+        /* log_to_console = */ false,
+        /* substitutions= */ {},
+        /* throw_on_bad_include_from= */ throw_on_bad_include_from);
     bool has_zk_includes;
     DB::XMLDocumentPtr config_xml = processor.processConfig(&has_zk_includes);
     if (has_zk_includes && process_zk_includes)
@@ -111,30 +115,29 @@ static DB::ConfigurationPtr get_configuration(const std::string & config_path, b
 }
 
 
-static std::vector<std::string> extractFromConfig(
-        const std::string & config_path, const std::string & key, bool process_zk_includes, bool try_get = false, bool get_users = false)
+static std::vector<std::string> extractFromConfig(const std::string & config_path, const std::string & key, bool process_zk_includes, bool ignore_errors, bool get_users)
 {
-    DB::ConfigurationPtr configuration = get_configuration(config_path, process_zk_includes);
+    DB::ConfigurationPtr configuration = get_configuration(config_path, process_zk_includes, !ignore_errors);
 
     if (get_users)
     {
         bool has_user_directories = configuration->has("user_directories");
-        if (!has_user_directories && !try_get)
+        if (!has_user_directories && !ignore_errors)
             throw DB::Exception(DB::ErrorCodes::CANNOT_LOAD_CONFIG, "Can't load config for users");
 
         std::string users_config_path = configuration->getString("user_directories.users_xml.path");
         const auto config_dir = fs::path{config_path}.remove_filename().string();
         if (fs::path(users_config_path).is_relative() && fs::exists(fs::path(config_dir) / users_config_path))
             users_config_path = fs::path(config_dir) / users_config_path;
-        configuration = get_configuration(users_config_path, process_zk_includes);
+        configuration = get_configuration(users_config_path, process_zk_includes, !ignore_errors);
     }
 
     /// Check if a key has globs.
     if (key.find_first_of("*?{") != std::string::npos)
-        return extactFromConfigAccordingToGlobs(configuration, key, try_get);
+        return extactFromConfigAccordingToGlobs(configuration, key, ignore_errors);
 
     /// Do not throw exception if not found.
-    if (try_get)
+    if (ignore_errors)
         return {configuration->getString(key, "")};
     return {configuration->getString(key)};
 }
@@ -146,7 +149,7 @@ int mainEntryClickHouseExtractFromConfig(int argc, char ** argv)
 {
     bool print_stacktrace = false;
     bool process_zk_includes = false;
-    bool try_get = false;
+    bool ignore_errors = false;
     bool get_users = false;
     std::string log_level;
     std::string config_path;
@@ -160,7 +163,7 @@ int mainEntryClickHouseExtractFromConfig(int argc, char ** argv)
         ("stacktrace", po::bool_switch(&print_stacktrace), "print stack traces of exceptions")
         ("process-zk-includes", po::bool_switch(&process_zk_includes),
          "if there are from_zk elements in config, connect to ZooKeeper and process them")
-        ("try", po::bool_switch(&try_get), "Do not warn about missing keys")
+        ("try", po::bool_switch(&ignore_errors), "Do not warn about missing keys, missing users configurations or non existing file from include_from tag")
         ("users", po::bool_switch(&get_users), "Return values from users.xml config")
         ("log-level", po::value<std::string>(&log_level)->default_value("error"), "log level")
         ("config-file,c", po::value<std::string>(&config_path)->required(), "path to config file")
@@ -175,7 +178,7 @@ int mainEntryClickHouseExtractFromConfig(int argc, char ** argv)
         po::variables_map options;
         po::store(po::command_line_parser(argc, argv).options(options_desc).positional(positional_desc).run(), options);
 
-        if (options.count("help"))
+        if (options.contains("help"))
         {
             std::cerr << "Preprocess config file and extract value of the given key." << std::endl
                 << std::endl;
@@ -188,7 +191,7 @@ int mainEntryClickHouseExtractFromConfig(int argc, char ** argv)
         po::notify(options);
 
         setupLogging(log_level);
-        for (const auto & value : extractFromConfig(config_path, key, process_zk_includes, try_get, get_users))
+        for (const auto & value : extractFromConfig(config_path, key, process_zk_includes, ignore_errors, get_users))
             std::cout << value << std::endl;
     }
     catch (...)
diff --git a/src/Common/Config/ConfigProcessor.cpp b/src/Common/Config/ConfigProcessor.cpp
@@ -68,9 +68,11 @@ ConfigProcessor::ConfigProcessor(
     const std::string & path_,
     bool throw_on_bad_incl_,
     bool log_to_console,
-    const Substitutions & substitutions_)
+    const Substitutions & substitutions_,
+    bool throw_on_bad_include_from_)
     : path(path_)
     , throw_on_bad_incl(throw_on_bad_incl_)
+    , throw_on_bad_include_from(throw_on_bad_include_from_)
     , substitutions(substitutions_)
     /// We need larger name pool to allow to support vast amount of users in users.xml files for ClickHouse.
     /// Size is prime because Poco::XML::NamePool uses bad (inefficient, low quality)
@@ -800,17 +802,24 @@ XMLDocumentPtr ConfigProcessor::processConfig(
                 include_from_path = default_path;
         }
 
-        processIncludes(
-            config,
-            substitutions,
-            include_from_path,
-            throw_on_bad_incl,
-            dom_parser,
-            log,
-            &contributing_zk_paths,
-            &contributing_files,
-            zk_node_cache,
-            zk_changed_event);
+        if (!throw_on_bad_include_from && !fs::exists(include_from_path))
+        {
+            LOG_WARNING(log, "File {} (from 'include_from') does not exist. Ignoring.", include_from_path);
+        }
+        else
+        {
+            processIncludes(
+                config,
+                substitutions,
+                include_from_path,
+                throw_on_bad_incl,
+                dom_parser,
+                log,
+                &contributing_zk_paths,
+                &contributing_files,
+                zk_node_cache,
+                zk_changed_event);
+        }
     }
     catch (Exception & e)
     {
diff --git a/src/Common/Config/ConfigProcessor.h b/src/Common/Config/ConfigProcessor.h
@@ -44,7 +44,8 @@ class ConfigProcessor
         const std::string & path,
         bool throw_on_bad_incl = false,
         bool log_to_console = false,
-        const Substitutions & substitutions = Substitutions());
+        const Substitutions & substitutions = Substitutions(),
+        bool throw_on_bad_include_from = true);
 
     /// Perform config includes and substitutions and return the resulting XML-document.
     ///
@@ -146,6 +147,7 @@ class ConfigProcessor
     std::string preprocessed_path;
 
     bool throw_on_bad_incl;
+    bool throw_on_bad_include_from;
 
     LoggerPtr log;
     Poco::AutoPtr<Poco::Channel> channel_ptr;
diff --git a/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.reference b/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.reference
@@ -0,0 +1,8 @@
+xml
+Not found: merge_tree.anything_xml
+
+yaml
+Not found: merge_tree.anything_yaml
+
+File not found: I hope such path will never exists, since it does not even contain a single slash
+I hope such path will never exists, since it does not even contain a single slash
diff --git a/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.sh b/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.sh
@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+
+CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CUR_DIR"/../shell_config.sh
+
+# NOTE: Cannot use CLICKHOUSE_EXTRACT_CONFIG, since it uses default config.
+
+prefix=$CUR_DIR/"$(basename "${BASH_SOURCE[0]}" .sh)"
+
+$CLICKHOUSE_BINARY extract-from-config --key merge_tree.comment --config $prefix.xml
+$CLICKHOUSE_BINARY extract-from-config --key merge_tree.anything_xml --config $prefix.xml |& grep -o 'Not found: merge_tree.anything_xml'
+$CLICKHOUSE_BINARY extract-from-config --key merge_tree.anything_xml --config $prefix.xml --try
+
+$CLICKHOUSE_BINARY extract-from-config --key merge_tree.comment --config $prefix.yaml
+$CLICKHOUSE_BINARY extract-from-config --key merge_tree.anything_yaml --config $prefix.yaml |& grep -o 'Not found: merge_tree.anything_yaml'
+$CLICKHOUSE_BINARY extract-from-config --key merge_tree.anything_yaml --config $prefix.yaml --try
+
+$CLICKHOUSE_BINARY extract-from-config --key include_from --config ${prefix}_bad_include_from.xml |& grep -o 'File not found: I hope such path will never exists, since it does not even contain a single slash'
+$CLICKHOUSE_BINARY extract-from-config --key include_from --config ${prefix}_bad_include_from.xml --try
diff --git a/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.xml b/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.xml
@@ -0,0 +1,5 @@
+<clickhouse>
+  <merge_tree>
+    <comment>xml</comment>
+  </merge_tree>
+</clickhouse>
diff --git a/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.yaml b/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try.yaml
@@ -0,0 +1,2 @@
+merge_tree:
+  comment: yaml
diff --git a/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try_bad_include_from.xml b/tests/queries/0_stateless/03344_clickhouse_extract_from_config_try_bad_include_from.xml
@@ -0,0 +1,3 @@
+<clickhouse>
+  <include_from>I hope such path will never exists, since it does not even contain a single slash</include_from>
+</clickhouse>

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+<clickhouse>`
	`2`	`+ <include_from>I hope such path will never exists, since it does not even contain a single slash</include_from>`
	`3`	`+</clickhouse>`