From 659a09763132d1a35add6b867035772eca506630 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Tue, 25 Oct 2022 09:35:10 +0400 Subject: [PATCH 01/10] Fixed SettingsProfilesInfo::getProfileNames() throwing std::exception - Using profiles_with_implicit for profile IDs, since it seems to be better synchronized with names - Trying harder: attempting to get name from AccessControl if it is missing from local cached names - Throwing Exception if everything else fails --- src/Access/SettingsProfilesCache.cpp | 22 +++++++++++++++++++ src/Access/SettingsProfilesInfo.cpp | 32 ++++++++++++++++++++++++++++ src/Access/SettingsProfilesInfo.h | 12 ++--------- 3 files changed, 56 insertions(+), 10 deletions(-) diff --git a/src/Access/SettingsProfilesCache.cpp b/src/Access/SettingsProfilesCache.cpp index 2a3dedbbd7a..27a7d10f99c 100644 --- a/src/Access/SettingsProfilesCache.cpp +++ b/src/Access/SettingsProfilesCache.cpp @@ -139,8 +139,30 @@ void SettingsProfilesCache::mergeSettingsAndConstraintsFor(EnabledSettings & ena merged_settings.merge(enabled.params.settings_from_user); auto info = std::make_shared(access_control); + info->profiles = enabled.params.settings_from_user.toProfileIDs(); substituteProfiles(merged_settings, info->profiles_with_implicit, info->names_of_profiles); + +// TODO (vnemkov): check if it fails on CI/CD to localize the previous bug names_of_profiles/profiles mismatch. Failed to reporoduce locally, remove before merging. +// +#if !defined(NDEBUG) + for (const auto & profile_id : info->profiles) + { + if (!info->names_of_profiles.contains(profile_id)) + { + const auto p = all_profiles.find(profile_id); + const auto profile_name = p != all_profiles.end() ? p->second->getName() : ""; + + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Profile {} ({}) missing from SettingsProfilesInfo::names_of_profiles", + toString(profile_id), + profile_name); + } + } + +#endif + info->settings = merged_settings.toSettingsChanges(); info->constraints = merged_settings.toSettingsConstraints(access_control); diff --git a/src/Access/SettingsProfilesInfo.cpp b/src/Access/SettingsProfilesInfo.cpp index d8b139020e8..643ee4cc59a 100644 --- a/src/Access/SettingsProfilesInfo.cpp +++ b/src/Access/SettingsProfilesInfo.cpp @@ -1,11 +1,18 @@ #include +#include #include #include +#include namespace DB { +namespace ErrorCodes +{ + extern const int LOGICAL_ERROR; +} + bool operator==(const SettingsProfilesInfo & lhs, const SettingsProfilesInfo & rhs) { if (lhs.settings != rhs.settings) @@ -55,4 +62,29 @@ SettingsProfilesInfo::getConstraintsAndProfileIDs(const std::shared_ptrsecond); + } + + return result; +} + } diff --git a/src/Access/SettingsProfilesInfo.h b/src/Access/SettingsProfilesInfo.h index 579125f7e04..e925f674595 100644 --- a/src/Access/SettingsProfilesInfo.h +++ b/src/Access/SettingsProfilesInfo.h @@ -26,7 +26,7 @@ struct SettingsProfilesInfo /// The order of IDs in this vector corresponds the order of applying of these profiles. std::vector profiles_with_implicit; - /// Names of all the profiles in `profiles`. + /// Names of all the profiles in `profiles_with_implicit`. std::unordered_map names_of_profiles; explicit SettingsProfilesInfo(const AccessControl & access_control_) : constraints(access_control_), access_control(access_control_) {} @@ -36,15 +36,7 @@ struct SettingsProfilesInfo friend bool operator ==(const SettingsProfilesInfo & lhs, const SettingsProfilesInfo & rhs); friend bool operator !=(const SettingsProfilesInfo & lhs, const SettingsProfilesInfo & rhs) { return !(lhs == rhs); } - Strings getProfileNames() const - { - Strings result; - result.reserve(profiles.size()); - for (const auto & profile_id : profiles) - result.push_back(names_of_profiles.at(profile_id)); - - return result; - } + Strings getProfileNames() const; private: const AccessControl & access_control; From c569adfaf6a8f5f5e67744eb2655c88f3c2f490e Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Tue, 25 Oct 2022 13:42:54 +0400 Subject: [PATCH 02/10] Minor fixes: typo and missing forward declaration --- src/Access/SettingsProfilesCache.cpp | 3 ++- src/Access/SettingsProfilesInfo.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Access/SettingsProfilesCache.cpp b/src/Access/SettingsProfilesCache.cpp index 27a7d10f99c..71995583db1 100644 --- a/src/Access/SettingsProfilesCache.cpp +++ b/src/Access/SettingsProfilesCache.cpp @@ -10,6 +10,7 @@ namespace DB namespace ErrorCodes { extern const int THERE_IS_NO_PROFILE; + extern const int LOGICAL_ERROR; } SettingsProfilesCache::SettingsProfilesCache(const AccessControl & access_control_) @@ -151,7 +152,7 @@ void SettingsProfilesCache::mergeSettingsAndConstraintsFor(EnabledSettings & ena if (!info->names_of_profiles.contains(profile_id)) { const auto p = all_profiles.find(profile_id); - const auto profile_name = p != all_profiles.end() ? p->second->getName() : ""; + const auto profile_name = p != all_profiles.end() ? p->second->getName() : ""; throw Exception( ErrorCodes::LOGICAL_ERROR, diff --git a/src/Access/SettingsProfilesInfo.cpp b/src/Access/SettingsProfilesInfo.cpp index 643ee4cc59a..7862598fa1e 100644 --- a/src/Access/SettingsProfilesInfo.cpp +++ b/src/Access/SettingsProfilesInfo.cpp @@ -65,7 +65,7 @@ SettingsProfilesInfo::getConstraintsAndProfileIDs(const std::shared_ptr Date: Wed, 2 Nov 2022 18:52:38 +0400 Subject: [PATCH 03/10] Removed debug code --- src/Access/SettingsProfilesCache.cpp | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/Access/SettingsProfilesCache.cpp b/src/Access/SettingsProfilesCache.cpp index 71995583db1..65dee051fac 100644 --- a/src/Access/SettingsProfilesCache.cpp +++ b/src/Access/SettingsProfilesCache.cpp @@ -144,26 +144,6 @@ void SettingsProfilesCache::mergeSettingsAndConstraintsFor(EnabledSettings & ena info->profiles = enabled.params.settings_from_user.toProfileIDs(); substituteProfiles(merged_settings, info->profiles_with_implicit, info->names_of_profiles); -// TODO (vnemkov): check if it fails on CI/CD to localize the previous bug names_of_profiles/profiles mismatch. Failed to reporoduce locally, remove before merging. -// -#if !defined(NDEBUG) - for (const auto & profile_id : info->profiles) - { - if (!info->names_of_profiles.contains(profile_id)) - { - const auto p = all_profiles.find(profile_id); - const auto profile_name = p != all_profiles.end() ? p->second->getName() : ""; - - throw Exception( - ErrorCodes::LOGICAL_ERROR, - "Profile {} ({}) missing from SettingsProfilesInfo::names_of_profiles", - toString(profile_id), - profile_name); - } - } - -#endif - info->settings = merged_settings.toSettingsChanges(); info->constraints = merged_settings.toSettingsConstraints(access_control); From c16c06eabad079db972d0506ea02c4d08b224ddf Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Wed, 2 Nov 2022 20:28:40 +0400 Subject: [PATCH 04/10] Removed declaration of unused LOGICAL_ERROR --- src/Access/SettingsProfilesCache.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Access/SettingsProfilesCache.cpp b/src/Access/SettingsProfilesCache.cpp index 65dee051fac..1ae0aef63e7 100644 --- a/src/Access/SettingsProfilesCache.cpp +++ b/src/Access/SettingsProfilesCache.cpp @@ -10,7 +10,6 @@ namespace DB namespace ErrorCodes { extern const int THERE_IS_NO_PROFILE; - extern const int LOGICAL_ERROR; } SettingsProfilesCache::SettingsProfilesCache(const AccessControl & access_control_) From aa78f1c8421aa78b7a7c6de53d4ea8d235bd5263 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Wed, 23 Nov 2022 17:04:00 +0400 Subject: [PATCH 05/10] Made sure SettingsProfilesInfo::profiles is a subset of SettingsProfilesInfo::profiles_with_implicit That means SettingsProfilesInfo::profiles now: - has default_profile_id (if enabled) - doesn't have profiles that are not matching for current user/roles --- src/Access/SettingsProfilesCache.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Access/SettingsProfilesCache.cpp b/src/Access/SettingsProfilesCache.cpp index 1ae0aef63e7..68caab5836c 100644 --- a/src/Access/SettingsProfilesCache.cpp +++ b/src/Access/SettingsProfilesCache.cpp @@ -140,7 +140,7 @@ void SettingsProfilesCache::mergeSettingsAndConstraintsFor(EnabledSettings & ena auto info = std::make_shared(access_control); - info->profiles = enabled.params.settings_from_user.toProfileIDs(); + info->profiles = merged_settings.toProfileIDs(); substituteProfiles(merged_settings, info->profiles_with_implicit, info->names_of_profiles); info->settings = merged_settings.toSettingsChanges(); From 246c6ff32030f4d305e91e6294e235577d25a00f Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Wed, 23 Nov 2022 17:27:55 +0400 Subject: [PATCH 06/10] Cleanup --- src/Access/SettingsProfilesInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Access/SettingsProfilesInfo.cpp b/src/Access/SettingsProfilesInfo.cpp index 7862598fa1e..50d519e242f 100644 --- a/src/Access/SettingsProfilesInfo.cpp +++ b/src/Access/SettingsProfilesInfo.cpp @@ -69,7 +69,7 @@ Strings SettingsProfilesInfo::getProfileNames() const // see invocations of SettingsProfilesCache::substituteProfiles. Strings result; result.reserve(profiles_with_implicit.size()); - for (const auto & profile_id : profiles_with_implicit) + for (const auto & profile_id : profiles) { const auto p = names_of_profiles.find(profile_id); if (p == names_of_profiles.end()) From d7fc4fee5e2f03ce8a009376a7fcd00cb1ad160e Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Wed, 23 Nov 2022 22:07:11 +0400 Subject: [PATCH 07/10] Update SettingsProfilesInfo.cpp --- src/Access/SettingsProfilesInfo.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Access/SettingsProfilesInfo.cpp b/src/Access/SettingsProfilesInfo.cpp index 50d519e242f..9b29502eb05 100644 --- a/src/Access/SettingsProfilesInfo.cpp +++ b/src/Access/SettingsProfilesInfo.cpp @@ -64,9 +64,6 @@ SettingsProfilesInfo::getConstraintsAndProfileIDs(const std::shared_ptr Date: Mon, 28 Nov 2022 16:06:11 +0400 Subject: [PATCH 08/10] Fixed accessing invalid iterator --- src/Access/SettingsProfilesInfo.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Access/SettingsProfilesInfo.cpp b/src/Access/SettingsProfilesInfo.cpp index 9b29502eb05..d8b52ecf5e4 100644 --- a/src/Access/SettingsProfilesInfo.cpp +++ b/src/Access/SettingsProfilesInfo.cpp @@ -65,11 +65,13 @@ SettingsProfilesInfo::getConstraintsAndProfileIDs(const std::shared_ptrsecond); + else { if (const auto name = access_control.tryReadName(profile_id)) // We could've updated cache here, but it is a very rare case, so don't bother. @@ -77,8 +79,6 @@ Strings SettingsProfilesInfo::getProfileNames() const else throw Exception(ErrorCodes::LOGICAL_ERROR, "Unable to get profile name for {}", toString(profile_id)); } - - result.push_back(p->second); } return result; From aff3e283e6be69fea5872e19fc83254ca95450f2 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Mon, 28 Nov 2022 16:06:56 +0400 Subject: [PATCH 09/10] Update SettingsProfilesInfo.h --- src/Access/SettingsProfilesInfo.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Access/SettingsProfilesInfo.h b/src/Access/SettingsProfilesInfo.h index e925f674595..ec289a5ec0a 100644 --- a/src/Access/SettingsProfilesInfo.h +++ b/src/Access/SettingsProfilesInfo.h @@ -26,7 +26,7 @@ struct SettingsProfilesInfo /// The order of IDs in this vector corresponds the order of applying of these profiles. std::vector profiles_with_implicit; - /// Names of all the profiles in `profiles_with_implicit`. + /// Names of all the profiles in `profiles`. std::unordered_map names_of_profiles; explicit SettingsProfilesInfo(const AccessControl & access_control_) : constraints(access_control_), access_control(access_control_) {} From 3d0c07ac5b8f18917f2314474030910176ec7940 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Wed, 30 Nov 2022 02:32:27 +0400 Subject: [PATCH 10/10] Updated tests to match improved behaviour of currentProfiles() --- tests/integration/test_settings_profile/test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_settings_profile/test.py b/tests/integration/test_settings_profile/test.py index 3358315cca7..335a0db53c0 100644 --- a/tests/integration/test_settings_profile/test.py +++ b/tests/integration/test_settings_profile/test.py @@ -550,7 +550,7 @@ def test_function_current_profiles(): user="robin", params={"session_id": session_id}, ) - == "['P1','P2']\t['P1','P2']\t['default','P3','P4','P5','P1','P2']\n" + == "['P1','P2']\t['default','P3','P5','P1','P2']\t['default','P3','P4','P5','P1','P2']\n" ) instance.http_query(