diff --git a/ydb/core/kqp/gateway/behaviour/view/manager.cpp b/ydb/core/kqp/gateway/behaviour/view/manager.cpp index 236125c0e85a..4d8c13d88d5b 100644 --- a/ydb/core/kqp/gateway/behaviour/view/manager.cpp +++ b/ydb/core/kqp/gateway/behaviour/view/manager.cpp @@ -12,11 +12,6 @@ namespace { using TYqlConclusionStatus = TViewManager::TYqlConclusionStatus; using TInternalModificationContext = TViewManager::TInternalModificationContext; -TString GetByKeyOrDefault(const NYql::TCreateObjectSettings& container, const TString& key) { - const auto value = container.GetFeaturesExtractor().Extract(key); - return value ? *value : TString{}; -} - TYqlConclusionStatus CheckFeatureFlag(TInternalModificationContext& context) { auto* const actorSystem = context.GetExternalData().GetActorSystem(); if (!actorSystem) { @@ -46,6 +41,16 @@ std::pair SplitPathByObjectId(const TString& objectId) { return pathPair; } +void ValidateOptions(NYql::TFeaturesExtractor& features) { + // Current implementation does not persist the security_invoker option value. + if (features.Extract("security_invoker") != "true") { + ythrow TBadArgumentException() << "security_invoker option must be explicitly enabled"; + } + if (!features.IsFinished()) { + ythrow TBadArgumentException() << "Unknown property: " << features.GetRemainedParamsString(); + } +} + void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme, const NYql::TCreateObjectSettings& settings, const TString& database) { @@ -56,11 +61,10 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme, auto& viewDesc = *modifyScheme.MutableCreateView(); viewDesc.SetName(pathPair.second); - viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text")); - if (!settings.GetFeaturesExtractor().IsFinished()) { - ythrow TBadArgumentException() << "Unknown property: " << settings.GetFeaturesExtractor().GetRemainedParamsString(); - } + auto& features = settings.GetFeaturesExtractor(); + viewDesc.SetQueryText(features.Extract("query_text").value_or("")); + ValidateOptions(features); } void FillDropViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme, diff --git a/ydb/core/kqp/ut/view/view_ut.cpp b/ydb/core/kqp/ut/view/view_ut.cpp index 949f08e906bd..7aab338ca15f 100644 --- a/ydb/core/kqp/ut/view/view_ut.cpp +++ b/ydb/core/kqp/ut/view/view_ut.cpp @@ -56,6 +56,20 @@ TString ReadWholeFile(const TString& path) { return file.ReadAll(); } +NQuery::TExecuteQueryResult ExecuteQuery(NQuery::TSession& session, const TString& query) { + const auto result = session.ExecuteQuery( + query, + NQuery::TTxControl::NoTx() + ).ExtractValueSync(); + + UNIT_ASSERT_C(result.IsSuccess(), + "Failed to execute the following query:\n" << query << '\n' + << "The issues:\n" << result.GetIssues().ToString() + ); + + return result; +} + void ExecuteDataDefinitionQuery(TSession& session, const TString& script) { const auto result = session.ExecuteSchemeQuery(script).ExtractValueSync(); UNIT_ASSERT_C(result.IsSuccess(), "Failed to execute the following DDL script:\n" @@ -186,6 +200,73 @@ Y_UNIT_TEST_SUITE(TKQPViewTest) { UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String"); } + Y_UNIT_TEST(ParsingSecurityInvoker) { + TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false)); + EnableViewsFeatureFlag(kikimr); + auto session = kikimr.GetQueryClient().GetSession().ExtractValueSync().GetSession(); + + constexpr const char* path = "TheView"; + constexpr const char* query = "SELECT 1"; + + auto fail = [&](const char* options) { + const TString creationQuery = std::format(R"( + CREATE VIEW {} {} AS {}; + )", + path, + options, + query + ); + + const auto creationResult = session.ExecuteQuery( + creationQuery, + NQuery::TTxControl::NoTx() + ).ExtractValueSync(); + + UNIT_ASSERT_C(!creationResult.IsSuccess(), creationQuery); + UNIT_ASSERT_STRING_CONTAINS( + creationResult.GetIssues().ToString(), "security_invoker option must be explicitly enabled" + ); + }; + fail(""); + fail("WITH security_invoker"); + fail("WITH security_invoker = false"); + fail("WITH SECURITY_INVOKER = true"); // option name is case-sensitive + fail("WITH (security_invoker)"); + fail("WITH (security_invoker = false)"); + fail("WITH (security_invoker = true, security_invoker = false)"); + + auto succeed = [&](const char* options) { + const TString creationQuery = std::format(R"( + CREATE VIEW {} {} AS {}; + DROP VIEW {}; + )", + path, + options, + query, + path + ); + ExecuteQuery(session, creationQuery); + }; + succeed("WITH security_invoker = true"); + succeed("WITH (security_invoker = true)"); + succeed("WITH (security_invoker = tRuE)"); // bool parsing is flexible enough + succeed("WITH (security_invoker = false, security_invoker = true)"); + + { + // literal named expression + const TString creationQuery = std::format(R"( + $value = "true"; + CREATE VIEW {} WITH security_invoker = $value AS {}; + DROP VIEW {}; + )", + path, + query, + path + ); + ExecuteQuery(session, creationQuery); + } + } + Y_UNIT_TEST(ListCreatedView) { TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false)); EnableViewsFeatureFlag(kikimr); diff --git a/ydb/library/yql/sql/v1/SQLv1.g.in b/ydb/library/yql/sql/v1/SQLv1.g.in index e58e33c584b5..4cd8665a228b 100644 --- a/ydb/library/yql/sql/v1/SQLv1.g.in +++ b/ydb/library/yql/sql/v1/SQLv1.g.in @@ -587,7 +587,7 @@ alter_external_data_source_action: drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref; create_view_stmt: CREATE VIEW object_ref - with_table_settings + create_object_features? AS select_stmt ; @@ -615,7 +615,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref ; drop_object_features: WITH object_features; -object_feature_value: id_or_type | bind_parameter | STRING_VALUE; +object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value; object_feature_kv: an_id_or_type EQUALS object_feature_value; object_feature_flag: an_id_or_type; object_feature: object_feature_kv | object_feature_flag; diff --git a/ydb/library/yql/sql/v1/sql_query.cpp b/ydb/library/yql/sql/v1/sql_query.cpp index ac6859afeb34..d307277b2a1e 100644 --- a/ydb/library/yql/sql/v1/sql_query.cpp +++ b/ydb/library/yql/sql/v1/sql_query.cpp @@ -1104,7 +1104,9 @@ bool TSqlQuery::Statement(TVector& blocks, const TRule_sql_stmt_core& } std::map features; - ParseViewOptions(features, node.GetRule_with_table_settings4()); + if (node.HasBlock4()) { + ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2()); + } ParseViewQuery(features, node.GetRule_select_stmt6()); const TString objectId = Id(node.GetRule_object_ref3().GetRule_id_or_at2(), *this).second; diff --git a/ydb/library/yql/sql/v1/sql_translation.cpp b/ydb/library/yql/sql/v1/sql_translation.cpp index aaef357bd06c..42ab64b4174b 100644 --- a/ydb/library/yql/sql/v1/sql_translation.cpp +++ b/ydb/library/yql/sql/v1/sql_translation.cpp @@ -55,7 +55,7 @@ TString CollectTokens(const TRule_select_stmt& selectStatement) { NSQLTranslation::TTranslationSettings CreateViewTranslationSettings(const NSQLTranslation::TTranslationSettings& base) { NSQLTranslation::TTranslationSettings settings; - + settings.ClusterMapping = base.ClusterMapping; settings.Mode = NSQLTranslation::ESqlMode::LIMITED_VIEW; @@ -1596,16 +1596,6 @@ namespace { return true; } - bool StoreBool(const TRule_table_setting_value& from, TDeferredAtom& to, TContext& ctx) { - if (!from.HasAlt_table_setting_value6()) { - return false; - } - // bool_value - const TString value = to_lower(ctx.Token(from.GetAlt_table_setting_value6().GetRule_bool_value1().GetToken1())); - to = TDeferredAtom(BuildLiteralBool(ctx.Pos(), FromString(value)), ctx); - return true; - } - bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector>& to, TSqlExpression& expr, TContext& ctx) { TVector boundaryKeys; @@ -1732,26 +1722,6 @@ namespace { return true; } - bool StoreViewOptionsEntry(const TIdentifier& id, - const TRule_table_setting_value& value, - std::map& features, - TContext& ctx) { - const auto name = to_lower(id.Name); - const auto publicName = to_upper(name); - - if (features.find(name) != features.end()) { - ctx.Error(ctx.Pos()) << publicName << " is a duplicate"; - return false; - } - - if (!StoreBool(value, features[name], ctx)) { - ctx.Error(ctx.Pos()) << "Value of " << publicName << " must be a bool"; - return false; - } - - return true; - } - template struct TPatternComponent { TBasicString Prefix; @@ -4268,7 +4238,7 @@ bool TSqlTranslation::BindParameterClause(const TRule_bind_parameter& node, TDef } bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& node, TDeferredAtom& result) { - // object_feature_value: an_id_or_type | bind_parameter; + // object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value; switch (node.Alt_case()) { case TRule_object_feature_value::kAltObjectFeatureValue1: { @@ -4293,6 +4263,12 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& result = TDeferredAtom(Ctx.Pos(), strValue->Content); break; } + case TRule_object_feature_value::kAltObjectFeatureValue4: + { + TString value = Ctx.Token(node.GetAlt_object_feature_value4().GetRule_bool_value1().GetToken1()); + result = TDeferredAtom(BuildLiteralBool(Ctx.Pos(), FromString(value)), Ctx); + break; + } case TRule_object_feature_value::ALT_NOT_SET: Y_ABORT("You should change implementation according to grammar changes"); } @@ -4480,32 +4456,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params return true; } -bool TSqlTranslation::ParseViewOptions(std::map& features, - const TRule_with_table_settings& options) { - const auto& firstEntry = options.GetRule_table_settings_entry3(); - if (!StoreViewOptionsEntry(IdEx(firstEntry.GetRule_an_id1(), *this), - firstEntry.GetRule_table_setting_value3(), - features, - Ctx)) { - return false; - } - for (const auto& block : options.GetBlock4()) { - const auto& entry = block.GetRule_table_settings_entry2(); - if (!StoreViewOptionsEntry(IdEx(entry.GetRule_an_id1(), *this), - entry.GetRule_table_setting_value3(), - features, - Ctx)) { - return false; - } - } - if (const auto securityInvoker = features.find("security_invoker"); - securityInvoker == features.end() || securityInvoker->second.Build()->GetLiteralValue() != "true") { - Ctx.Error(Ctx.Pos()) << "SECURITY_INVOKER option must be explicitly enabled"; - return false; - } - return true; -} - bool TSqlTranslation::ParseViewQuery(std::map& features, const TRule_select_stmt& query) { const TString queryText = CollectTokens(query); diff --git a/ydb/library/yql/sql/v1/sql_ut.cpp b/ydb/library/yql/sql/v1/sql_ut.cpp index 089c6c76feec..f7be6846bf77 100644 --- a/ydb/library/yql/sql/v1/sql_ut.cpp +++ b/ydb/library/yql/sql/v1/sql_ut.cpp @@ -6330,24 +6330,6 @@ Y_UNIT_TEST_SUITE(TViewSyntaxTest) { UNIT_ASSERT_C(res.Root, res.Issues.ToString()); } - Y_UNIT_TEST(CreateViewNoSecurityInvoker) { - NYql::TAstParseResult res = SqlToYql(R"( - USE plato; - CREATE VIEW TheView AS SELECT 1; - )" - ); - UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "Unexpected token 'AS' : syntax error"); - } - - Y_UNIT_TEST(CreateViewSecurityInvokerTurnedOff) { - NYql::TAstParseResult res = SqlToYql(R"( - USE plato; - CREATE VIEW TheView WITH (security_invoker = FALSE) AS SELECT 1; - )" - ); - UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "SECURITY_INVOKER option must be explicitly enabled"); - } - Y_UNIT_TEST(CreateViewFromTable) { constexpr const char* path = "/PathPrefix/TheView"; constexpr const char* query = R"( diff --git a/ydb/services/metadata/abstract/parsing.h b/ydb/services/metadata/abstract/parsing.h index 6e6743327bf8..7d168f9ea1cb 100644 --- a/ydb/services/metadata/abstract/parsing.h +++ b/ydb/services/metadata/abstract/parsing.h @@ -60,9 +60,11 @@ class TObjectSettingsImpl { if (auto maybeAtom = i.template Maybe()) { Features.emplace(maybeAtom.Cast().StringValue(), ""); } else if (auto maybeTuple = i.template Maybe()) { - auto tuple = maybeTuple.Cast(); - if (auto tupleValue = tuple.Value().template Maybe()) { - Features.emplace(tuple.Name().Value(), tupleValue.Cast().Value()); + NNodes::TCoNameValueTuple tuple = maybeTuple.Cast(); + if (auto maybeAtom = tuple.Value().template Maybe()) { + Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value()); + } else if (auto maybeBool = tuple.Value().template Maybe()) { + Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast().Value()); } } }