Views: throw a human-readable error in case of a missing WITH (security_invoker = true)

ydb/core/kqp/gateway/behaviour/view/manager.cpp

@@ -14,11 +14,6 @@ using TYqlConclusionStatus = TViewManager::TYqlConclusionStatus;
 using TInternalModificationContext = TViewManager::TInternalModificationContext;
 using TExternalModificationContext = TViewManager::TExternalModificationContext;
 
-TString GetByKeyOrDefault(const NYql::TCreateObjectSettings& container, const TString& key) {
-    const auto value = container.GetFeaturesExtractor().Extract(key);
-    return value ? *value : TString{};
-}
-
 TYqlConclusionStatus CheckFeatureFlag(const TInternalModificationContext& context) {
     auto* const actorSystem = context.GetExternalData().GetActorSystem();
     if (!actorSystem) {
@@ -48,6 +43,16 @@ std::pair<TString, TString> 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 TExternalModificationContext& context) {
@@ -58,12 +63,12 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
 
     auto& viewDesc = *modifyScheme.MutableCreateView();
     viewDesc.SetName(pathPair.second);
-    viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text"));
-    NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext());
 
-    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);
+
+    NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext());
 }
 
 void FillDropViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,

ydb/core/kqp/ut/view/view_ut.cpp

@@ -213,6 +213,90 @@ Y_UNIT_TEST_SUITE(TCreateAndDropViewTest) {
         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);
+        }
+        {
+            // evaluated expression
+            const TString creationQuery = std::format(R"(
+                    $lambda = ($x) -> {{
+                        RETURN CAST($x as String)
+                    }};
+                    $value = $lambda(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);

ydb/library/yql/sql/v1/SQLv1.g.in

@@ -600,7 +600,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
 ;
 
@@ -628,7 +628,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;
@@ -750,11 +750,11 @@ local_index: LOCAL;
 
 index_subtype: an_id;
 
-with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN; 
+with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN;
 index_setting_entry: an_id EQUALS index_setting_value;
 index_setting_value:
       id_or_type
-    | STRING_VALUE      
+    | STRING_VALUE
     | integer
     | bool_value
 ;

ydb/library/yql/sql/v1/SQLv1Antlr4.g.in

@@ -599,7 +599,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
 ;
 
@@ -627,7 +627,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;
@@ -749,11 +749,11 @@ local_index: LOCAL;
 
 index_subtype: an_id;
 
-with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN; 
+with_index_settings: WITH LPAREN index_setting_entry (COMMA index_setting_entry)* COMMA? RPAREN;
 index_setting_entry: an_id EQUALS index_setting_value;
 index_setting_value:
       id_or_type
-    | STRING_VALUE      
+    | STRING_VALUE
     | integer
     | bool_value
 ;

ydb/library/yql/sql/v1/sql_query.cpp

@@ -1239,8 +1239,10 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
             }
 
             std::map<TString, TDeferredAtom> features;
-            if (!ParseViewOptions(features, node.GetRule_with_table_settings4())) {
-                return false;
+            if (node.HasBlock4()) {
+                if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) {
+                    return false;
+                }
             }
             if (!ParseViewQuery(features, node.GetRule_select_stmt6())) {
                 return false;

ydb/library/yql/sql/v1/sql_translation.cpp

@@ -1795,16 +1795,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<bool>(value)), ctx);
-        return true;
-    }
-
     bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector<TVector<TNodePtr>>& to,
             TSqlExpression& expr, TContext& ctx) {
         TVector<TNodePtr> boundaryKeys;
@@ -1931,26 +1921,6 @@ namespace {
         return true;
     }
 
-    bool StoreViewOptionsEntry(const TIdentifier& id,
-                               const TRule_table_setting_value& value,
-                               std::map<TString, TDeferredAtom>& 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<typename TChar>
     struct TPatternComponent {
         TBasicString<TChar> Prefix;
@@ -4625,7 +4595,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:
         {
@@ -4650,6 +4620,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<bool>(value)), Ctx);
+            break;
+        }
         case TRule_object_feature_value::ALT_NOT_SET:
             Y_ABORT("You should change implementation according to grammar changes");
     }
@@ -4839,32 +4815,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params
     return true;
 }
 
-bool TSqlTranslation::ParseViewOptions(std::map<TString, TDeferredAtom>& 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<TString, TDeferredAtom>& features,
     const TRule_select_stmt& query

ydb/library/yql/sql/v1/sql_ut.cpp

@@ -6705,24 +6705,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"(

ydb/library/yql/sql/v1/sql_ut_antlr4.cpp

@@ -6702,24 +6702,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(), "mismatched input 'AS' expecting WITH");
-    }
-
-    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"(

ydb/services/metadata/abstract/parsing.h

@@ -74,8 +74,10 @@ class TObjectSettingsImpl {
                 NNodes::TCoNameValueTuple tuple = maybeTuple.Cast();
                 if (auto maybeAtom = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
                     Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value());
-                } else if (auto maybeDataCtor = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
-                    Features.emplace(tuple.Name().Value(), maybeDataCtor.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
+                } else if (auto maybeInt = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
+                    Features.emplace(tuple.Name().Value(), maybeInt.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
+                } else if (auto maybeBool = tuple.Value().template Maybe<NNodes::TCoBool>()) {
+                    Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
                 }
             }
         }