From 77ee4c04aa017271e7deceb305413bdfceceae03 Mon Sep 17 00:00:00 2001 From: Sergei Trifonov Date: Tue, 6 Sep 2022 20:28:10 +0200 Subject: [PATCH] fix stateless tests --- programs/server/config.xml | 4 +- src/Access/SettingsConstraints.cpp | 41 ++++++--------- src/Access/SettingsConstraints.h | 15 ++---- src/Access/SettingsProfileElement.cpp | 52 +++---------------- src/Access/SettingsProfileElement.h | 6 +-- src/Access/UsersConfigAccessStorage.cpp | 13 +++-- src/Common/SettingConstraintType.h | 26 ++++++++++ .../Access/ASTSettingsProfileElement.cpp | 8 +-- .../Access/ASTSettingsProfileElement.h | 11 +--- .../Access/ParserSettingsProfileElement.cpp | 16 +++--- .../Access/ParserSettingsProfileElement.h | 2 +- src/Storages/System/StorageSystemSettings.cpp | 6 +-- .../StorageSystemSettingsProfileElements.cpp | 6 +-- .../02117_show_create_table_system.reference | 2 +- 14 files changed, 91 insertions(+), 117 deletions(-) create mode 100644 src/Common/SettingConstraintType.h diff --git a/programs/server/config.xml b/programs/server/config.xml index cdc81d91024..a11dc7ea83f 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -66,7 +66,7 @@ For example, as below: {"date_time":"1650918987.180175","thread_name":"#1","thread_id":"254545","level":"Trace","query_id":"","logger_name":"BaseDaemon","message":"Received signal 2","source_file":"../base/daemon/BaseDaemon.cpp; virtual void SignalListener::run()","source_line":"192"} To enable JSON logging support, please uncomment the entire tag below. - + a) You can modify key names by changing values under tag values inside tag. For example, to change DATE_TIME to MY_DATE_TIME, you can do like: MY_DATE_TIME @@ -667,7 +667,7 @@ previous profile. You can change this behaviour by setting this to true. If it's set to true then if settings profile has a constraint for a specific setting, then this constraint completely cancels all actions of previous constraint (defined in other profiles) for the same specific setting, including fields that are not set by new constraint. - Also it enables 'changeable_in_readonly' constraint type --> + It also enables 'changeable_in_readonly' constraint type --> false diff --git a/src/Access/SettingsConstraints.cpp b/src/Access/SettingsConstraints.cpp index 5d99aadb1e3..26e5cc930d7 100644 --- a/src/Access/SettingsConstraints.cpp +++ b/src/Access/SettingsConstraints.cpp @@ -35,33 +35,24 @@ void SettingsConstraints::clear() constraints.clear(); } - -void SettingsConstraints::setMinValue(const String & setting_name, const Field & min_value) +void SettingsConstraints::set(const String & setting_name, const Field & min_value, const Field & max_value, SettingConstraintType type) { - constraints[setting_name].min_value = Settings::castValueUtil(setting_name, min_value); + if (min_value.isNull() && max_value.isNull() && type == SettingConstraintType::NONE) + return; // Do not even create entry to avoid issues during profile inheritance + auto & constraint = constraints[setting_name]; + if (!min_value.isNull()) + constraint.min_value = Settings::castValueUtil(setting_name, min_value); + if (!max_value.isNull()) + constraint.max_value = Settings::castValueUtil(setting_name, max_value); + constraint.type = type; } -void SettingsConstraints::setMaxValue(const String & setting_name, const Field & max_value) -{ - constraints[setting_name].max_value = Settings::castValueUtil(setting_name, max_value); -} - -void SettingsConstraints::setIsConst(const String & setting_name, bool is_const) -{ - constraints[setting_name].is_const = is_const; -} - -void SettingsConstraints::setChangableInReadonly(const String & setting_name, bool changeable_in_readonly) -{ - constraints[setting_name].changeable_in_readonly = changeable_in_readonly; -} - -void SettingsConstraints::get(const Settings & current_settings, std::string_view setting_name, Field & min_value, Field & max_value, bool & is_const) const +void SettingsConstraints::get(const Settings & current_settings, std::string_view setting_name, Field & min_value, Field & max_value, SettingConstraintType & type) const { auto range = getRange(current_settings, setting_name); min_value = range.min_value; max_value = range.max_value; - is_const = range.is_const; + type = range.type; } void SettingsConstraints::merge(const SettingsConstraints & other) @@ -80,8 +71,8 @@ void SettingsConstraints::merge(const SettingsConstraints & other) constraint.min_value = other_constraint.min_value; if (!other_constraint.max_value.isNull()) constraint.max_value = other_constraint.max_value; - if (other_constraint.is_const) - constraint.is_const = true; // NOTE: In this mode flag cannot be overridden to be false + if (other_constraint.type == SettingConstraintType::CONST) + constraint.type = SettingConstraintType::CONST; // NOTE: In this mode flag cannot be overridden to be false } } } @@ -217,7 +208,7 @@ bool SettingsConstraints::Range::check(SettingChange & change, const Field & new return false; } - if (is_const) + if (type == SettingConstraintType::CONST) { if (reaction == THROW_ON_VIOLATION) throw Exception("Setting " + setting_name + " should not be changed", ErrorCodes::SETTING_CONSTRAINT_VIOLATION); @@ -277,7 +268,7 @@ SettingsConstraints::Range SettingsConstraints::getRange(const Settings & curren auto it = constraints.find(setting_name); if (current_settings.readonly == 1) { - if (it == constraints.end() || !it->second.changeable_in_readonly) + if (it == constraints.end() || it->second.type != SettingConstraintType::CHANGEABLE_IN_READONLY) return Range::forbidden("Cannot modify '" + String(setting_name) + "' setting in readonly mode", ErrorCodes::READONLY); } else // For both readonly=0 and readonly=2 @@ -290,7 +281,7 @@ SettingsConstraints::Range SettingsConstraints::getRange(const Settings & curren bool SettingsConstraints::Range::operator==(const Range & other) const { - return is_const == other.is_const && changeable_in_readonly == other.changeable_in_readonly && min_value == other.min_value && max_value == other.max_value; + return type == other.type && min_value == other.min_value && max_value == other.max_value; } bool operator ==(const SettingsConstraints & left, const SettingsConstraints & right) diff --git a/src/Access/SettingsConstraints.h b/src/Access/SettingsConstraints.h index 5a90d7e698c..e94db6ad705 100644 --- a/src/Access/SettingsConstraints.h +++ b/src/Access/SettingsConstraints.h @@ -1,9 +1,9 @@ #pragma once +#include #include #include - namespace Poco::Util { class AbstractConfiguration; @@ -65,12 +65,8 @@ public: void clear(); bool empty() const { return constraints.empty(); } - void setMinValue(const String & setting_name, const Field & min_value); - void setMaxValue(const String & setting_name, const Field & max_value); - void setIsConst(const String & setting_name, bool is_const); - void setChangableInReadonly(const String & setting_name, bool is_const); - - void get(const Settings & current_settings, std::string_view setting_name, Field & min_value, Field & max_value, bool & is_const) const; + void set(const String & setting_name, const Field & min_value, const Field & max_value, SettingConstraintType type); + void get(const Settings & current_settings, std::string_view setting_name, Field & min_value, Field & max_value, SettingConstraintType & type) const; void merge(const SettingsConstraints & other); @@ -94,8 +90,7 @@ private: struct Range { - bool is_const = false; - bool changeable_in_readonly = false; + SettingConstraintType type = SettingConstraintType::NONE; Field min_value; Field max_value; @@ -115,7 +110,7 @@ private: static Range forbidden(const String & explain, int code) { - return Range{.is_const = true, .explain = explain, .code = code}; + return Range{.type = SettingConstraintType::CONST, .explain = explain, .code = code}; } }; diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index a0efe14090a..6851169db5e 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -60,25 +60,10 @@ void SettingsProfileElement::init(const ASTSettingsProfileElement & ast, const A value = ast.value; min_value = ast.min_value; max_value = ast.max_value; - changeable_in_readonly = false; - switch (ast.type) - { - case ASTSettingsProfileElement::ConstraintType::NONE: - is_const.reset(); - break; - case ASTSettingsProfileElement::ConstraintType::WRITABLE: - is_const.emplace(false); - break; - case ASTSettingsProfileElement::ConstraintType::CONST: - is_const.emplace(true); - break; - case ASTSettingsProfileElement::ConstraintType::CHANGEABLE_IN_READONLY: - if (!access_control->doesSettingsConstraintsReplacePrevious()) - throw Exception("CHANGEABLE_IN_READONLY for " + setting_name + " is not allowed unless settings_constraints_replace_previous is enabled", ErrorCodes::NOT_IMPLEMENTED); - is_const.reset(); - changeable_in_readonly = true; - break; - } + type = ast.type; + + if (type == SettingConstraintType::CHANGEABLE_IN_READONLY && !access_control->doesSettingsConstraintsReplacePrevious()) + throw Exception("CHANGEABLE_IN_READONLY for " + setting_name + " is not allowed unless settings_constraints_replace_previous is enabled", ErrorCodes::NOT_IMPLEMENTED); if (!value.isNull()) value = Settings::castValueUtil(setting_name, value); @@ -102,12 +87,7 @@ std::shared_ptr SettingsProfileElement::toAST() const ast->value = value; ast->min_value = min_value; ast->max_value = max_value; - if (changeable_in_readonly) - ast->type = ASTSettingsProfileElement::ConstraintType::CHANGEABLE_IN_READONLY; - if (is_const.has_value()) - ast->type = *is_const ? ASTSettingsProfileElement::ConstraintType::CONST : ASTSettingsProfileElement::ConstraintType::WRITABLE; - else - ast->type = ASTSettingsProfileElement::ConstraintType::NONE; + ast->type = type; return ast; } @@ -128,12 +108,7 @@ std::shared_ptr SettingsProfileElement::toASTWithName ast->value = value; ast->min_value = min_value; ast->max_value = max_value; - if (changeable_in_readonly) - ast->type = ASTSettingsProfileElement::ConstraintType::CHANGEABLE_IN_READONLY; - if (is_const.has_value()) - ast->type = *is_const ? ASTSettingsProfileElement::ConstraintType::CONST : ASTSettingsProfileElement::ConstraintType::WRITABLE; - else - ast->type = ASTSettingsProfileElement::ConstraintType::NONE; + ast->type = type; return ast; } @@ -237,19 +212,8 @@ SettingsConstraints SettingsProfileElements::toSettingsConstraints(const AccessC { SettingsConstraints res{access_control}; for (const auto & elem : *this) - { - if (!elem.setting_name.empty() && (elem.setting_name != ALLOW_BACKUP_SETTING_NAME)) - { - if (!elem.min_value.isNull()) - res.setMinValue(elem.setting_name, elem.min_value); - if (!elem.max_value.isNull()) - res.setMaxValue(elem.setting_name, elem.max_value); - if (elem.is_const) - res.setIsConst(elem.setting_name, *elem.is_const); - if (elem.changeable_in_readonly) - res.setChangableInReadonly(elem.setting_name, true); - } - } + if (!elem.setting_name.empty() && elem.setting_name != ALLOW_BACKUP_SETTING_NAME) + res.set(elem.setting_name, elem.min_value, elem.max_value, elem.type); return res; } diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index 2d6e5cf7b2f..2d0444a1589 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -25,10 +26,9 @@ struct SettingsProfileElement Field value; Field min_value; Field max_value; - std::optional is_const; - bool changeable_in_readonly; + SettingConstraintType type = SettingConstraintType::NONE; - auto toTuple() const { return std::tie(parent_profile, setting_name, value, min_value, max_value, is_const, changeable_in_readonly); } + auto toTuple() const { return std::tie(parent_profile, setting_name, value, min_value, max_value, type); } friend bool operator==(const SettingsProfileElement & lhs, const SettingsProfileElement & rhs) { return lhs.toTuple() == rhs.toTuple(); } friend bool operator!=(const SettingsProfileElement & lhs, const SettingsProfileElement & rhs) { return !(lhs == rhs); } friend bool operator <(const SettingsProfileElement & lhs, const SettingsProfileElement & rhs) { return lhs.toTuple() < rhs.toTuple(); } diff --git a/src/Access/UsersConfigAccessStorage.cpp b/src/Access/UsersConfigAccessStorage.cpp index b3280ac1fd3..4adbfeef98b 100644 --- a/src/Access/UsersConfigAccessStorage.cpp +++ b/src/Access/UsersConfigAccessStorage.cpp @@ -441,6 +441,7 @@ namespace String path_to_name = path_to_constraints + "." + setting_name; config.keys(path_to_name, constraint_types); + size_t type_specifiers_count = 0; for (const String & constraint_type : constraint_types) { if (constraint_type == "min") @@ -448,19 +449,23 @@ namespace else if (constraint_type == "max") profile_element.max_value = Settings::stringToValueUtil(setting_name, config.getString(path_to_name + "." + constraint_type)); else if (constraint_type == "readonly" || constraint_type == "const") - profile_element.is_const = true; + { + type_specifiers_count++; + profile_element.type = SettingConstraintType::CONST; + } else if (constraint_type == "changeable_in_readonly") { + type_specifiers_count++; if (access_control.doesSettingsConstraintsReplacePrevious()) - profile_element.changeable_in_readonly = true; + profile_element.type = SettingConstraintType::CHANGEABLE_IN_READONLY; else throw Exception("Setting changeable_in_readonly for " + setting_name + " is not allowed unless settings_constraints_replace_previous is enabled", ErrorCodes::NOT_IMPLEMENTED); } else throw Exception("Setting " + constraint_type + " value for " + setting_name + " isn't supported", ErrorCodes::NOT_IMPLEMENTED); } - if (profile_element.is_const && profile_element.changeable_in_readonly) - throw Exception("Both settings changeable_in_readonly and const/readonly cannot be used for " + setting_name, ErrorCodes::NOT_IMPLEMENTED); + if (type_specifiers_count > 1) + throw Exception("Not more than one constraint type specifier (const/readonly/changeable_in_readonly) is allowed for " + setting_name, ErrorCodes::NOT_IMPLEMENTED); profile_elements.push_back(std::move(profile_element)); } diff --git a/src/Common/SettingConstraintType.h b/src/Common/SettingConstraintType.h new file mode 100644 index 00000000000..8a515c8dfb8 --- /dev/null +++ b/src/Common/SettingConstraintType.h @@ -0,0 +1,26 @@ +#pragma once + +#include + + +namespace DB +{ + +enum class SettingConstraintType +{ + // Default. Behave in the same way as WRITABLE, but is not inherited unless `settings_constraints_replace_previous` is set. + NONE, + + // Setting can be change within specified range only in `readonly=0` or `readonly=2` mode. + WRITABLE, + + // Setting cannot be changed at all. + // Either READONLY or CONST keyword in SQL syntax can be used ( or in config.xml) to enable this. + // NOTE: name `CONST` is choosen to avoid confusion with `readonly` setting. + CONST, + + // Setting can be changed within specified range, regardless of `readonly` setting value. + CHANGEABLE_IN_READONLY, +}; + +} diff --git a/src/Parsers/Access/ASTSettingsProfileElement.cpp b/src/Parsers/Access/ASTSettingsProfileElement.cpp index 12de74ab417..85ec5bc9233 100644 --- a/src/Parsers/Access/ASTSettingsProfileElement.cpp +++ b/src/Parsers/Access/ASTSettingsProfileElement.cpp @@ -54,17 +54,17 @@ void ASTSettingsProfileElement::formatImpl(const FormatSettings & settings, Form switch (type) { - case ConstraintType::NONE: + case SettingConstraintType::NONE: break; - case ConstraintType::WRITABLE: + case SettingConstraintType::WRITABLE: settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << " WRITABLE" << (settings.hilite ? IAST::hilite_none : ""); break; - case ConstraintType::CONST: + case SettingConstraintType::CONST: settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << " READONLY" << (settings.hilite ? IAST::hilite_none : ""); break; - case ConstraintType::CHANGEABLE_IN_READONLY: + case SettingConstraintType::CHANGEABLE_IN_READONLY: settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << " CHANGEABLE_IN_READONLY" << (settings.hilite ? IAST::hilite_none : ""); break; diff --git a/src/Parsers/Access/ASTSettingsProfileElement.h b/src/Parsers/Access/ASTSettingsProfileElement.h index 5ae5316c318..c3816c65958 100644 --- a/src/Parsers/Access/ASTSettingsProfileElement.h +++ b/src/Parsers/Access/ASTSettingsProfileElement.h @@ -2,7 +2,7 @@ #include #include - +#include namespace DB { @@ -17,14 +17,7 @@ public: Field value; Field min_value; Field max_value; - enum class ConstraintType - { - NONE, // default - WRITABLE, - CONST, // equals READONLY - CHANGEABLE_IN_READONLY, - }; - ConstraintType type; + SettingConstraintType type = SettingConstraintType::NONE; bool id_mode = false; /// If true then `parent_profile` keeps UUID, not a name. bool use_inherit_keyword = false; /// If true then this element is a part of ASTCreateSettingsProfileQuery. diff --git a/src/Parsers/Access/ParserSettingsProfileElement.cpp b/src/Parsers/Access/ParserSettingsProfileElement.cpp index 7c39c1fbb7b..c7b73a78d0c 100644 --- a/src/Parsers/Access/ParserSettingsProfileElement.cpp +++ b/src/Parsers/Access/ParserSettingsProfileElement.cpp @@ -95,23 +95,23 @@ namespace } - bool parseConstraintTypeKeyword(IParserBase::Pos & pos, Expected & expected, ASTSettingsProfileElement::ConstraintType & type) + bool parseConstraintTypeKeyword(IParserBase::Pos & pos, Expected & expected, SettingConstraintType & type) { return IParserBase::wrapParseImpl(pos, [&] { if (ParserKeyword{"READONLY"}.ignore(pos, expected) || ParserKeyword{"CONST"}.ignore(pos, expected)) { - type = ASTSettingsProfileElement::ConstraintType::CONST; + type = SettingConstraintType::CONST; return true; } else if (ParserKeyword{"WRITABLE"}.ignore(pos, expected)) { - type = ASTSettingsProfileElement::ConstraintType::WRITABLE; + type = SettingConstraintType::WRITABLE; return true; } else if (ParserKeyword{"CHANGEABLE_IN_READONLY"}.ignore(pos, expected)) { - type = ASTSettingsProfileElement::ConstraintType::CHANGEABLE_IN_READONLY; + type = SettingConstraintType::CHANGEABLE_IN_READONLY; return true; } else @@ -127,7 +127,7 @@ namespace Field & value, Field & min_value, Field & max_value, - ASTSettingsProfileElement::ConstraintType & type) + SettingConstraintType & type) { return IParserBase::wrapParseImpl(pos, [&] { @@ -139,7 +139,7 @@ namespace Field res_value; Field res_min_value; Field res_max_value; - ASTSettingsProfileElement::ConstraintType res_type; + SettingConstraintType res_type; bool has_value_or_constraint = false; while (parseValue(pos, expected, res_value) || parseMinMaxValue(pos, expected, res_min_value, res_max_value) @@ -152,7 +152,7 @@ namespace return false; if (boost::iequals(res_setting_name, "PROFILE") && res_value.isNull() && res_min_value.isNull() && res_max_value.isNull() - && res_type == ASTSettingsProfileElement::ConstraintType::CONST) + && res_type == SettingConstraintType::CONST) { /// Ambiguity: "profile readonly" can be treated either as a profile named "readonly" or /// as a setting named 'profile' with the readonly constraint. @@ -184,7 +184,7 @@ namespace Field value; Field min_value; Field max_value; - ASTSettingsProfileElement::ConstraintType type; + SettingConstraintType type = SettingConstraintType::NONE; bool ok = parseSettingNameWithValueOrConstraints(pos, expected, setting_name, value, min_value, max_value, type); diff --git a/src/Parsers/Access/ParserSettingsProfileElement.h b/src/Parsers/Access/ParserSettingsProfileElement.h index 8843591a56c..082fc66625f 100644 --- a/src/Parsers/Access/ParserSettingsProfileElement.h +++ b/src/Parsers/Access/ParserSettingsProfileElement.h @@ -6,7 +6,7 @@ namespace DB { /** Parses a string like this: - * {variable [= value] [MIN [=] min_value] [MAX [=] max_value] [READONLY|WRITABLE]} | PROFILE 'profile_name' + * {variable [= value] [MIN [=] min_value] [MAX [=] max_value] [CONST|READONLY|WRITABLE|CHANGEABLE_IN_READONLY]} | PROFILE 'profile_name' */ class ParserSettingsProfileElement : public IParserBase { diff --git a/src/Storages/System/StorageSystemSettings.cpp b/src/Storages/System/StorageSystemSettings.cpp index 858d1973e25..33f1c4cc6b1 100644 --- a/src/Storages/System/StorageSystemSettings.cpp +++ b/src/Storages/System/StorageSystemSettings.cpp @@ -40,8 +40,8 @@ void StorageSystemSettings::fillData(MutableColumns & res_columns, ContextPtr co res_columns[3]->insert(setting.getDescription()); Field min, max; - bool is_const = false; - constraints.get(settings, setting_name, min, max, is_const); + SettingConstraintType type; + constraints.get(settings, setting_name, min, max, type); /// These two columns can accept strings only. if (!min.isNull()) @@ -51,7 +51,7 @@ void StorageSystemSettings::fillData(MutableColumns & res_columns, ContextPtr co res_columns[4]->insert(min); res_columns[5]->insert(max); - res_columns[6]->insert(is_const); + res_columns[6]->insert(type == SettingConstraintType::CONST); res_columns[7]->insert(setting.getTypeName()); } } diff --git a/src/Storages/System/StorageSystemSettingsProfileElements.cpp b/src/Storages/System/StorageSystemSettingsProfileElements.cpp index 3a79aa632d0..7e83b75c468 100644 --- a/src/Storages/System/StorageSystemSettingsProfileElements.cpp +++ b/src/Storages/System/StorageSystemSettingsProfileElements.cpp @@ -104,15 +104,15 @@ void StorageSystemSettingsProfileElements::fillData(MutableColumns & res_columns } bool inserted_readonly = false; - if (element.is_const && !element.setting_name.empty()) + if ((element.type == SettingConstraintType::CONST || element.type == SettingConstraintType::WRITABLE) && !element.setting_name.empty()) { - column_readonly.push_back(*element.is_const); + column_readonly.push_back(element.type == SettingConstraintType::CONST); column_readonly_null_map.push_back(false); inserted_readonly = true; } bool inserted_changeable_in_readonly = false; - if (element.changeable_in_readonly && !element.setting_name.empty()) + if (element.type == SettingConstraintType::CHANGEABLE_IN_READONLY && !element.setting_name.empty()) { column_changeable_in_readonly.push_back(true); column_changeable_in_readonly_null_map.push_back(false); diff --git a/tests/queries/0_stateless/02117_show_create_table_system.reference b/tests/queries/0_stateless/02117_show_create_table_system.reference index dd2f6651d5c..2cfd99ec9f4 100644 --- a/tests/queries/0_stateless/02117_show_create_table_system.reference +++ b/tests/queries/0_stateless/02117_show_create_table_system.reference @@ -969,7 +969,7 @@ CREATE TABLE system.settings_profile_elements `min` Nullable(String), `max` Nullable(String), `readonly` Nullable(UInt8), - `allowance` Nullable(UInt8), + `changeable_in_readonly` Nullable(UInt8), `inherit_profile` Nullable(String) ) ENGINE = SystemSettingsProfileElements