Skip to content

Commit

Permalink
Merge 091633a into f4c79be
Browse files Browse the repository at this point in the history
  • Loading branch information
@jepett0
jepett0 authored Sep 27, 2024
2 parents f4c79be + 091633a commit 284ff2f
Show file tree
Hide file tree
Showing 7 changed files with 116 additions and 91 deletions.
25 changes: 15 additions & 10 deletions ydb/core/kqp/gateway/behaviour/view/manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down
84 changes: 84 additions & 0 deletions ydb/core/kqp/ut/view/view_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions ydb/library/yql/sql/v1/SQLv1.g.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,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
;

Expand Down Expand Up @@ -621,7 +621,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;
Expand Down
6 changes: 4 additions & 2 deletions ydb/library/yql/sql/v1/sql_query.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1233,8 +1233,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;
Expand Down
64 changes: 7 additions & 57 deletions ydb/library/yql/sql/v1/sql_translation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1647,16 +1647,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;
Expand Down Expand Up @@ -1783,26 +1773,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;
Expand Down Expand Up @@ -4394,7 +4364,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:
{
Expand All @@ -4419,6 +4389,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");
}
Expand Down Expand Up @@ -4608,32 +4584,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
Expand Down
18 changes: 0 additions & 18 deletions ydb/library/yql/sql/v1/sql_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6592,24 +6592,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"(
Expand Down
6 changes: 4 additions & 2 deletions ydb/services/metadata/abstract/parsing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}
Expand Down

0 comments on commit 284ff2f

Please sign in to comment.