Skip to content

Commit 284ff2f

Browse files
jepett0jepett0
authored
Merge 091633a into f4c79be 
2 parents f4c79be + 091633a commit 284ff2f

File tree

7 files changed

+116
-91
lines changed

7 files changed

+116
-91
lines changed

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

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,6 @@ using TYqlConclusionStatus = TViewManager::TYqlConclusionStatus;
1414
using TInternalModificationContext = TViewManager::TInternalModificationContext;
1515
using TExternalModificationContext = TViewManager::TExternalModificationContext;
1616

17-
TString GetByKeyOrDefault(const NYql::TCreateObjectSettings& container, const TString& key) {
18-
const auto value = container.GetFeaturesExtractor().Extract(key);
19-
return value ? *value : TString{};
20-
}
21-
2217
TYqlConclusionStatus CheckFeatureFlag(const TInternalModificationContext& context) {
2318
auto* const actorSystem = context.GetExternalData().GetActorSystem();
2419
if (!actorSystem) {
@@ -48,6 +43,16 @@ std::pair<TString, TString> SplitPathByObjectId(const TString& objectId) {
4843
return pathPair;
4944
}
5045

46+
void ValidateOptions(NYql::TFeaturesExtractor& features) {
47+
// Current implementation does not persist the security_invoker option value.
48+
if (features.Extract("security_invoker") != "true") {
49+
ythrow TBadArgumentException() << "security_invoker option must be explicitly enabled";
50+
}
51+
if (!features.IsFinished()) {
52+
ythrow TBadArgumentException() << "Unknown property: " << features.GetRemainedParamsString();
53+
}
54+
}
55+
5156
void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
5257
const NYql::TCreateObjectSettings& settings,
5358
const TExternalModificationContext& context) {
@@ -58,12 +63,12 @@ void FillCreateViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,
5863

5964
auto& viewDesc = *modifyScheme.MutableCreateView();
6065
viewDesc.SetName(pathPair.second);
61-
viewDesc.SetQueryText(GetByKeyOrDefault(settings, "query_text"));
62-
NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext());
6366

64-
if (!settings.GetFeaturesExtractor().IsFinished()) {
65-
ythrow TBadArgumentException() << "Unknown property: " << settings.GetFeaturesExtractor().GetRemainedParamsString();
66-
}
67+
auto& features = settings.GetFeaturesExtractor();
68+
viewDesc.SetQueryText(features.Extract("query_text").value_or(""));
69+
ValidateOptions(features);
70+
71+
NSQLTranslation::Serialize(context.GetTranslationSettings(), *viewDesc.MutableCapturedContext());
6772
}
6873

6974
void FillDropViewProposal(NKikimrSchemeOp::TModifyScheme& modifyScheme,

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

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,90 @@ Y_UNIT_TEST_SUITE(TCreateAndDropViewTest) {
213213
UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String");
214214
}
215215

216+
Y_UNIT_TEST(ParsingSecurityInvoker) {
217+
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
218+
EnableViewsFeatureFlag(kikimr);
219+
auto session = kikimr.GetQueryClient().GetSession().ExtractValueSync().GetSession();
220+
221+
constexpr const char* path = "TheView";
222+
constexpr const char* query = "SELECT 1";
223+
224+
auto fail = [&](const char* options) {
225+
const TString creationQuery = std::format(R"(
226+
CREATE VIEW {} {} AS {};
227+
)",
228+
path,
229+
options,
230+
query
231+
);
232+
233+
const auto creationResult = session.ExecuteQuery(
234+
creationQuery,
235+
NQuery::TTxControl::NoTx()
236+
).ExtractValueSync();
237+
238+
UNIT_ASSERT_C(!creationResult.IsSuccess(), creationQuery);
239+
UNIT_ASSERT_STRING_CONTAINS(
240+
creationResult.GetIssues().ToString(), "security_invoker option must be explicitly enabled"
241+
);
242+
};
243+
fail("");
244+
fail("WITH security_invoker");
245+
fail("WITH security_invoker = false");
246+
fail("WITH SECURITY_INVOKER = true"); // option name is case-sensitive
247+
fail("WITH (security_invoker)");
248+
fail("WITH (security_invoker = false)");
249+
fail("WITH (security_invoker = true, security_invoker = false)");
250+
251+
auto succeed = [&](const char* options) {
252+
const TString creationQuery = std::format(R"(
253+
CREATE VIEW {} {} AS {};
254+
DROP VIEW {};
255+
)",
256+
path,
257+
options,
258+
query,
259+
path
260+
);
261+
ExecuteQuery(session, creationQuery);
262+
};
263+
succeed("WITH security_invoker = true");
264+
succeed("WITH (security_invoker = true)");
265+
succeed("WITH (security_invoker = tRuE)"); // bool parsing is flexible enough
266+
succeed("WITH (security_invoker = false, security_invoker = true)");
267+
268+
{
269+
// literal named expression
270+
const TString creationQuery = std::format(R"(
271+
$value = "true";
272+
CREATE VIEW {} WITH security_invoker = $value AS {};
273+
DROP VIEW {};
274+
)",
275+
path,
276+
query,
277+
path
278+
);
279+
ExecuteQuery(session, creationQuery);
280+
}
281+
{
282+
// evaluated expression
283+
const TString creationQuery = std::format(R"(
284+
$lambda = ($x) -> {{
285+
RETURN CAST($x as String)
286+
}};
287+
$value = $lambda(true);
288+
289+
CREATE VIEW {} WITH security_invoker = $value AS {};
290+
DROP VIEW {};
291+
)",
292+
path,
293+
query,
294+
path
295+
);
296+
ExecuteQuery(session, creationQuery);
297+
}
298+
}
299+
216300
Y_UNIT_TEST(ListCreatedView) {
217301
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
218302
EnableViewsFeatureFlag(kikimr);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -593,7 +593,7 @@ alter_external_data_source_action:
593593
drop_external_data_source_stmt: DROP EXTERNAL DATA SOURCE (IF EXISTS)? object_ref;
594594

595595
create_view_stmt: CREATE VIEW object_ref
596-
with_table_settings
596+
create_object_features?
597597
AS select_stmt
598598
;
599599

@@ -621,7 +621,7 @@ drop_object_stmt: DROP OBJECT (IF EXISTS)? object_ref
621621
;
622622
drop_object_features: WITH object_features;
623623

624-
object_feature_value: id_or_type | bind_parameter | STRING_VALUE;
624+
object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
625625
object_feature_kv: an_id_or_type EQUALS object_feature_value;
626626
object_feature_flag: an_id_or_type;
627627
object_feature: object_feature_kv | object_feature_flag;

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1233,8 +1233,10 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
12331233
}
12341234

12351235
std::map<TString, TDeferredAtom> features;
1236-
if (!ParseViewOptions(features, node.GetRule_with_table_settings4())) {
1237-
return false;
1236+
if (node.HasBlock4()) {
1237+
if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) {
1238+
return false;
1239+
}
12381240
}
12391241
if (!ParseViewQuery(features, node.GetRule_select_stmt6())) {
12401242
return false;

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

Lines changed: 7 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1647,16 +1647,6 @@ namespace {
16471647
return true;
16481648
}
16491649

1650-
bool StoreBool(const TRule_table_setting_value& from, TDeferredAtom& to, TContext& ctx) {
1651-
if (!from.HasAlt_table_setting_value6()) {
1652-
return false;
1653-
}
1654-
// bool_value
1655-
const TString value = to_lower(ctx.Token(from.GetAlt_table_setting_value6().GetRule_bool_value1().GetToken1()));
1656-
to = TDeferredAtom(BuildLiteralBool(ctx.Pos(), FromString<bool>(value)), ctx);
1657-
return true;
1658-
}
1659-
16601650
bool StoreSplitBoundary(const TRule_literal_value_list& boundary, TVector<TVector<TNodePtr>>& to,
16611651
TSqlExpression& expr, TContext& ctx) {
16621652
TVector<TNodePtr> boundaryKeys;
@@ -1783,26 +1773,6 @@ namespace {
17831773
return true;
17841774
}
17851775

1786-
bool StoreViewOptionsEntry(const TIdentifier& id,
1787-
const TRule_table_setting_value& value,
1788-
std::map<TString, TDeferredAtom>& features,
1789-
TContext& ctx) {
1790-
const auto name = to_lower(id.Name);
1791-
const auto publicName = to_upper(name);
1792-
1793-
if (features.find(name) != features.end()) {
1794-
ctx.Error(ctx.Pos()) << publicName << " is a duplicate";
1795-
return false;
1796-
}
1797-
1798-
if (!StoreBool(value, features[name], ctx)) {
1799-
ctx.Error(ctx.Pos()) << "Value of " << publicName << " must be a bool";
1800-
return false;
1801-
}
1802-
1803-
return true;
1804-
}
1805-
18061776
template<typename TChar>
18071777
struct TPatternComponent {
18081778
TBasicString<TChar> Prefix;
@@ -4394,7 +4364,7 @@ bool TSqlTranslation::BindParameterClause(const TRule_bind_parameter& node, TDef
43944364
}
43954365

43964366
bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value& node, TDeferredAtom& result) {
4397-
// object_feature_value: an_id_or_type | bind_parameter;
4367+
// object_feature_value: id_or_type | bind_parameter | STRING_VALUE | bool_value;
43984368
switch (node.Alt_case()) {
43994369
case TRule_object_feature_value::kAltObjectFeatureValue1:
44004370
{
@@ -4419,6 +4389,12 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value&
44194389
result = TDeferredAtom(Ctx.Pos(), strValue->Content);
44204390
break;
44214391
}
4392+
case TRule_object_feature_value::kAltObjectFeatureValue4:
4393+
{
4394+
TString value = Ctx.Token(node.GetAlt_object_feature_value4().GetRule_bool_value1().GetToken1());
4395+
result = TDeferredAtom(BuildLiteralBool(Ctx.Pos(), FromString<bool>(value)), Ctx);
4396+
break;
4397+
}
44224398
case TRule_object_feature_value::ALT_NOT_SET:
44234399
Y_ABORT("You should change implementation according to grammar changes");
44244400
}
@@ -4608,32 +4584,6 @@ bool TSqlTranslation::ValidateExternalTable(const TCreateTableParameters& params
46084584
return true;
46094585
}
46104586

4611-
bool TSqlTranslation::ParseViewOptions(std::map<TString, TDeferredAtom>& features,
4612-
const TRule_with_table_settings& options) {
4613-
const auto& firstEntry = options.GetRule_table_settings_entry3();
4614-
if (!StoreViewOptionsEntry(IdEx(firstEntry.GetRule_an_id1(), *this),
4615-
firstEntry.GetRule_table_setting_value3(),
4616-
features,
4617-
Ctx)) {
4618-
return false;
4619-
}
4620-
for (const auto& block : options.GetBlock4()) {
4621-
const auto& entry = block.GetRule_table_settings_entry2();
4622-
if (!StoreViewOptionsEntry(IdEx(entry.GetRule_an_id1(), *this),
4623-
entry.GetRule_table_setting_value3(),
4624-
features,
4625-
Ctx)) {
4626-
return false;
4627-
}
4628-
}
4629-
if (const auto securityInvoker = features.find("security_invoker");
4630-
securityInvoker == features.end() || securityInvoker->second.Build()->GetLiteralValue() != "true") {
4631-
Ctx.Error(Ctx.Pos()) << "SECURITY_INVOKER option must be explicitly enabled";
4632-
return false;
4633-
}
4634-
return true;
4635-
}
4636-
46374587
bool TSqlTranslation::ParseViewQuery(
46384588
std::map<TString, TDeferredAtom>& features,
46394589
const TRule_select_stmt& query

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

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -6592,24 +6592,6 @@ Y_UNIT_TEST_SUITE(TViewSyntaxTest) {
65926592
UNIT_ASSERT_C(res.Root, res.Issues.ToString());
65936593
}
65946594

6595-
Y_UNIT_TEST(CreateViewNoSecurityInvoker) {
6596-
NYql::TAstParseResult res = SqlToYql(R"(
6597-
USE plato;
6598-
CREATE VIEW TheView AS SELECT 1;
6599-
)"
6600-
);
6601-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "Unexpected token 'AS' : syntax error");
6602-
}
6603-
6604-
Y_UNIT_TEST(CreateViewSecurityInvokerTurnedOff) {
6605-
NYql::TAstParseResult res = SqlToYql(R"(
6606-
USE plato;
6607-
CREATE VIEW TheView WITH (security_invoker = FALSE) AS SELECT 1;
6608-
)"
6609-
);
6610-
UNIT_ASSERT_STRING_CONTAINS(res.Issues.ToString(), "SECURITY_INVOKER option must be explicitly enabled");
6611-
}
6612-
66136595
Y_UNIT_TEST(CreateViewFromTable) {
66146596
constexpr const char* path = "/PathPrefix/TheView";
66156597
constexpr const char* query = R"(

‎ydb/services/metadata/abstract/parsing.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,10 @@ class TObjectSettingsImpl {
7474
NNodes::TCoNameValueTuple tuple = maybeTuple.Cast();
7575
if (auto maybeAtom = tuple.Value().template Maybe<NNodes::TCoAtom>()) {
7676
Features.emplace(tuple.Name().Value(), maybeAtom.Cast().Value());
77-
} else if (auto maybeDataCtor = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
78-
Features.emplace(tuple.Name().Value(), maybeDataCtor.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
77+
} else if (auto maybeInt = tuple.Value().template Maybe<NNodes::TCoIntegralCtor>()) {
78+
Features.emplace(tuple.Name().Value(), maybeInt.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
79+
} else if (auto maybeBool = tuple.Value().template Maybe<NNodes::TCoBool>()) {
80+
Features.emplace(tuple.Name().Value(), maybeBool.Cast().Literal().Cast<NNodes::TCoAtom>().Value());
7981
}
8082
}
8183
}

0 commit comments

Comments
 (0)