Merge pull request #13305 from vitlibar/correct-error-message-if-setting-not-found-in-users_xml

Correct error message if setting not found in users.xml

programs/server/Server.cpp

@@ -570,6 +570,9 @@ int Server::main(const std::vector<std::string> & /*args*/)
     if (users_config_path != config_path)
         checkForUsersNotInMainConfig(config(), config_path, users_config_path, log);
 
+    if (config().has("custom_settings_prefixes"))
+        global_context->getAccessControlManager().setCustomSettingsPrefixes(config().getString("custom_settings_prefixes"));
+
     auto users_config_reloader = std::make_unique<ConfigReloader>(
         users_config_path,
         include_from_path,
@@ -614,9 +617,6 @@ int Server::main(const std::vector<std::string> & /*args*/)
     }
     global_context->setUncompressedCache(uncompressed_cache_size);
 
-    if (config().has("custom_settings_prefixes"))
-        global_context->getAccessControlManager().setCustomSettingsPrefixes(config().getString("custom_settings_prefixes"));
-
     /// Load global settings from default_profile and system_profile.
     global_context->setDefaultProfiles(config());
     const Settings & settings = global_context->getSettingsRef();

src/Access/AccessControlManager.cpp

@@ -123,6 +123,10 @@ AccessControlManager::AccessControlManager()
       external_authenticators(std::make_unique<ExternalAuthenticators>()),
       custom_settings_prefixes(std::make_unique<CustomSettingsPrefixes>())
 {
+    /// Allow UsersConfigAccessStorage to check the names of settings which it will read from users.xml.
+    auto check_setting_name_function = [this](const std::string_view & setting_name) { checkSettingNameIsAllowed(setting_name); };
+    auto & users_config_access_storage = dynamic_cast<UsersConfigAccessStorage &>(getStorageByIndex(USERS_CONFIG_ACCESS_STORAGE_INDEX));
+    users_config_access_storage.setCheckSettingNameFunction(check_setting_name_function);
 }
 
 

src/Access/SettingsProfileElement.cpp

@@ -37,6 +37,11 @@ void SettingsProfileElement::init(const ASTSettingsProfileElement & ast, const A
     if (!ast.setting_name.empty())
     {
         setting_name = ast.setting_name;
+
+        /// Optionally check if a setting with that name is allowed.
+        if (manager)
+            manager->checkSettingNameIsAllowed(setting_name);
+
         value = ast.value;
         min_value = ast.min_value;
         max_value = ast.max_value;
@@ -48,10 +53,6 @@ void SettingsProfileElement::init(const ASTSettingsProfileElement & ast, const A
             min_value = Settings::castValueUtil(setting_name, min_value);
         if (!max_value.isNull())
             max_value = Settings::castValueUtil(setting_name, max_value);
-
-        /// Optionally check if a setting with that name is allowed.
-        if (manager)
-            manager->checkSettingNameIsAllowed(setting_name);
     }
 }
 

src/Access/UsersConfigAccessStorage.cpp

@@ -359,18 +359,24 @@ namespace
 
 
     SettingsProfileElements parseSettingsConstraints(const Poco::Util::AbstractConfiguration & config,
-                                                     const String & path_to_constraints)
+                                                     const String & path_to_constraints,
+                                                     const std::function<void(const std::string_view &)> & check_setting_name_function)
     {
         SettingsProfileElements profile_elements;
         Poco::Util::AbstractConfiguration::Keys keys;
         config.keys(path_to_constraints, keys);
+
         for (const String & setting_name : keys)
         {
+            if (check_setting_name_function)
+                check_setting_name_function(setting_name);
+
             SettingsProfileElement profile_element;
             profile_element.setting_name = setting_name;
             Poco::Util::AbstractConfiguration::Keys constraint_types;
             String path_to_name = path_to_constraints + "." + setting_name;
             config.keys(path_to_name, constraint_types);
+
             for (const String & constraint_type : constraint_types)
             {
                 if (constraint_type == "min")
@@ -384,12 +390,14 @@ namespace
             }
             profile_elements.push_back(std::move(profile_element));
         }
+
         return profile_elements;
     }
 
     std::shared_ptr<SettingsProfile> parseSettingsProfile(
         const Poco::Util::AbstractConfiguration & config,
-        const String & profile_name)
+        const String & profile_name,
+        const std::function<void(const std::string_view &)> & check_setting_name_function)
     {
         auto profile = std::make_shared<SettingsProfile>();
         profile->setName(profile_name);
@@ -411,11 +419,14 @@ namespace
 
             if (key == "constraints" || key.starts_with("constraints["))
             {
-                profile->elements.merge(parseSettingsConstraints(config, profile_config + "." + key));
+                profile->elements.merge(parseSettingsConstraints(config, profile_config + "." + key, check_setting_name_function));
                 continue;
             }
 
             const auto & setting_name = key;
+            if (check_setting_name_function)
+                check_setting_name_function(setting_name);
+
             SettingsProfileElement profile_element;
             profile_element.setting_name = setting_name;
             profile_element.value = Settings::stringToValueUtil(setting_name, config.getString(profile_config + "." + key));
@@ -426,7 +437,10 @@ namespace
     }
 
 
-    std::vector<AccessEntityPtr> parseSettingsProfiles(const Poco::Util::AbstractConfiguration & config, Poco::Logger * log)
+    std::vector<AccessEntityPtr> parseSettingsProfiles(
+        const Poco::Util::AbstractConfiguration & config,
+        const std::function<void(const std::string_view &)> & check_setting_name_function,
+        Poco::Logger * log)
     {
         std::vector<AccessEntityPtr> profiles;
         Poco::Util::AbstractConfiguration::Keys profile_names;
@@ -435,7 +449,7 @@ namespace
         {
             try
             {
-                profiles.push_back(parseSettingsProfile(config, profile_name));
+                profiles.push_back(parseSettingsProfile(config, profile_name, check_setting_name_function));
             }
             catch (...)
             {
@@ -452,6 +466,13 @@ UsersConfigAccessStorage::UsersConfigAccessStorage() : IAccessStorage("users.xml
 }
 
 
+void UsersConfigAccessStorage::setCheckSettingNameFunction(
+    const std::function<void(const std::string_view &)> & check_setting_name_function_)
+{
+    check_setting_name_function = check_setting_name_function_;
+}
+
+
 void UsersConfigAccessStorage::setConfiguration(const Poco::Util::AbstractConfiguration & config)
 {
     std::vector<std::pair<UUID, AccessEntityPtr>> all_entities;
@@ -461,7 +482,7 @@ void UsersConfigAccessStorage::setConfiguration(const Poco::Util::AbstractConfig
         all_entities.emplace_back(generateID(*entity), entity);
     for (const auto & entity : parseRowPolicies(config, getLogger()))
         all_entities.emplace_back(generateID(*entity), entity);
-    for (const auto & entity : parseSettingsProfiles(config, getLogger()))
+    for (const auto & entity : parseSettingsProfiles(config, check_setting_name_function, getLogger()))
         all_entities.emplace_back(generateID(*entity), entity);
     memory_storage.setAll(all_entities);
 }

src/Access/UsersConfigAccessStorage.h

@@ -20,6 +20,7 @@ class UsersConfigAccessStorage : public IAccessStorage
 public:
     UsersConfigAccessStorage();
 
+    void setCheckSettingNameFunction(const std::function<void(const std::string_view &)> & check_setting_name_function_);
     void setConfiguration(const Poco::Util::AbstractConfiguration & config);
 
 private:
@@ -38,5 +39,6 @@ private:
     bool hasSubscriptionImpl(EntityType type) const override;
 
     MemoryAccessStorage memory_storage;
+    std::function<void(const std::string_view &)> check_setting_name_function;
 };
 }

tests/integration/helpers/0_common_instance_config.xml

 <yandex>
     <timezone>Europe/Moscow</timezone>
     <listen_host>0.0.0.0</listen_host>
+    <custom_settings_prefixes>custom_</custom_settings_prefixes>
     <path>/var/lib/clickhouse/</path>
     <tmp_path>/var/lib/clickhouse/tmp/</tmp_path>
 </yandex>

tests/integration/test_custom_settings/configs/config.d/text_log.xml

+<yandex>
+    <text_log/>
+</yandex>

tests/integration/test_custom_settings/configs/users.d/custom_settings.xml

+<yandex>
+    <profiles>
+        <default>
+            <custom_a>Int64_-5</custom_a>
+            <custom_b>UInt64_10000000000</custom_b>
+            <custom_c>Float64_-43.25e-1</custom_c>
+            <custom_d>'some text'</custom_d>
+        </default>
+
+        <profile_with_unknown_setting>
+            <x>1</x>
+        </profile_with_unknown_setting>
+
+        <profile_illformed_setting>
+            <custom_f>1</custom_f>
+        </profile_illformed_setting>
+    </profiles>
+</yandex>

tests/integration/test_custom_settings/test.py

+import pytest
+from helpers.cluster import ClickHouseCluster
+
+cluster = ClickHouseCluster(__file__)
+node = cluster.add_instance('node', config_dir='configs')
+
+
+@pytest.fixture(scope="module", autouse=True)
+def started_cluster():
+    try:
+        cluster.start()
+        yield cluster
+
+    finally:
+        cluster.shutdown()
+
+
+def test():
+    assert node.query("SELECT getSetting('custom_a')") == "-5\n"
+    assert node.query("SELECT getSetting('custom_b')") == "10000000000\n"
+    assert node.query("SELECT getSetting('custom_c')") == "-4.325\n"
+    assert node.query("SELECT getSetting('custom_d')") == "some text\n"
+
+    assert "custom_a = -5, custom_b = 10000000000, custom_c = -4.325, custom_d = \\'some text\\'" \
+        in node.query("SHOW CREATE SETTINGS PROFILE default")
+
+    assert "no settings profile" in node.query_and_get_error("SHOW CREATE SETTINGS PROFILE profile_with_unknown_setting")
+    assert "no settings profile" in node.query_and_get_error("SHOW CREATE SETTINGS PROFILE profile_illformed_setting")
+
+
+def test_invalid_settings():
+    node.query("SYSTEM RELOAD CONFIG")
+    node.query("SYSTEM FLUSH LOGS")
+
+    assert node.query("SELECT COUNT() FROM system.text_log WHERE"
+        " message LIKE '%Could not parse profile `profile_illformed_setting`%'"
+        " AND message LIKE '%Couldn\\'t restore Field from dump%'") == "1\n"
+
+    assert node.query("SELECT COUNT() FROM system.text_log WHERE"
+        " message LIKE '%Could not parse profile `profile_with_unknown_setting`%'"
+        " AND message LIKE '%Setting x is neither a builtin setting nor started with the prefix \\'custom_\\'%'") == "1\n"