24-2: Views: throw a human-readable error in case of a missing WITH (security_invoker = true) (#9320) (#9783)

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<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 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,

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);

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;

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

@@ -1104,7 +1104,9 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
             }
 
             std::map<TString, TDeferredAtom> 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;

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<bool>(value)), ctx);
-        return true;
-    }
-
     bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector<TVector<TNodePtr>>& to,
             TSqlExpression& expr, TContext& ctx) {
         TVector<TNodePtr> boundaryKeys;
@@ -1732,26 +1722,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;
@@ -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<bool>(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<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) {
     const TString queryText = CollectTokens(query);

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"(

ydb/services/metadata/abstract/parsing.h

@@ -60,9 +60,11 @@ class TObjectSettingsImpl {
             if (auto maybeAtom = i.template Maybe<NYql::NNodes::TCoAtom>()) {
                 Features.emplace(maybeAtom.Cast().StringValue(), "");
             } else if (auto maybeTuple = i.template Maybe<NNodes::TCoNameValueTuple>()) {
-                auto tuple = maybeTuple.Cast();
-                if (auto tupleValue = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
-                    Features.emplace(tuple.Name().Value(), tupleValue.Cast().Value());
+                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 maybeBool = tuple.Value().template Maybe<NNodes::TCoBool>()) {
+                    Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
                 }
             }
         }