Skip to content

Commit

Permalink
Through human-readable error in case of a missing `WITH (security_inv…
Browse files Browse the repository at this point in the history 
…oker = true)`

First version just to check if other project's tests fail.
  • Loading branch information
@jepett0
jepett0 committed Sep 16, 2024
1 parent 6ee3d1c commit fdd2ace
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 15 deletions.
22 changes: 22 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 @@ -194,6 +194,28 @@ Y_UNIT_TEST_SUITE(TCreateAndDropViewTest) {
UNIT_ASSERT_STRING_CONTAINS(creationResult.GetIssues().ToString(), "Error: Cannot divide type String and String");
}

Y_UNIT_TEST(UnspecifiedSecurityInvoker) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
EnableViewsFeatureFlag(kikimr);
auto session = kikimr.GetTableClient().CreateSession().GetValueSync().GetSession();

constexpr const char* path = "/Root/TheView";
constexpr const char* queryInView = "SELECT 1";

const TString creationQuery = std::format(R"(
CREATE VIEW `{}` AS {};
)",
path,
queryInView
);

const auto creationResult = session.ExecuteSchemeQuery(creationQuery).ExtractValueSync();
UNIT_ASSERT(!creationResult.IsSuccess());
UNIT_ASSERT_STRING_CONTAINS(
creationResult.GetIssues().ToString(), "SECURITY_INVOKER option must be explicitly enabled"
);
}

Y_UNIT_TEST(ListCreatedView) {
TKikimrRunner kikimr(TKikimrSettings().SetWithSampleTables(false));
EnableViewsFeatureFlag(kikimr);
Expand Down
8 changes: 4 additions & 4 deletions ydb/library/yql/sql/v1/SQLv1.g.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
;
Expand Down
8 changes: 4 additions & 4 deletions ydb/library/yql/sql/v1/SQLv1Antlr4.g.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
;
Expand Down
20 changes: 17 additions & 3 deletions ydb/library/yql/sql/v1/sql_query.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&
if (rule.HasBlock2()) { // OR REPLACE
replaceIfExists = true;
Y_DEBUG_ABORT_UNLESS(
(IS_TOKEN(rule.GetBlock2().GetToken1().GetId(), OR) &&
(IS_TOKEN(rule.GetBlock2().GetToken1().GetId(), OR) &&
IS_TOKEN(rule.GetBlock2().GetToken2().GetId(), REPLACE))
);
}
Expand Down Expand Up @@ -1239,7 +1239,21 @@ 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()) {
ParseViewOptions(features, node.GetBlock4().GetRule_with_table_settings1());
}
*/
if (node.HasBlock4()) {
if (!ParseObjectFeatures(features, node.GetBlock4().GetRule_create_object_features1().GetRule_object_features2())) {
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;
}
ParseViewQuery(features, node.GetRule_select_stmt6());

const TString objectId = Id(node.GetRule_object_ref3().GetRule_id_or_at2(), *this).second;
Expand Down Expand Up @@ -1465,7 +1479,7 @@ bool TSqlQuery::Statement(TVector<TNodePtr>& blocks, const TRule_sql_stmt_core&

TVector<TString> columns;
if (analyzeTable.HasBlock2()) {
auto columnsNode =
auto columnsNode =
analyzeTable.GetBlock2().GetRule_column_list2();

if (columnsNode.HasRule_column_name1()) {
Expand Down
17 changes: 13 additions & 4 deletions ydb/library/yql/sql/v1/sql_translation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -785,8 +785,8 @@ std::tuple<bool, bool, TString> TSqlTranslation::GetIndexSettingValue(const TRul
return {true, value, stringValue};
}

bool TSqlTranslation::CreateIndexSettingEntry(const TIdentifier &id,
const TRule_index_setting_value& node,
bool TSqlTranslation::CreateIndexSettingEntry(const TIdentifier &id,
const TRule_index_setting_value& node,
TIndexDescription::EType indexType,
TIndexDescription::TIndexSettings& indexSettings) {

Expand Down Expand Up @@ -4632,6 +4632,12 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value&
result = TDeferredAtom(Ctx.Pos(), strValue->Content);
break;
}
case TRule_object_feature_value::kAltObjectFeatureValue4:
{
TString value = to_lower(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 All @@ -4641,15 +4647,18 @@ bool TSqlTranslation::ObjectFeatureValueClause(const TRule_object_feature_value&
bool TSqlTranslation::AddObjectFeature(std::map<TString, TDeferredAtom>& result, const TRule_object_feature& feature) {
if (feature.has_alt_object_feature1()) {
auto& kv = feature.GetAlt_object_feature1().GetRule_object_feature_kv1();
const TString& key = Id(kv.GetRule_an_id_or_type1(), *this);
const TString key = to_lower(Id(kv.GetRule_an_id_or_type1(), *this));
auto& ruleValue = kv.GetRule_object_feature_value3();
TDeferredAtom value;
if (!ObjectFeatureValueClause(ruleValue, value)) {
return false;
}
result[key] = value;
} else if (feature.has_alt_object_feature2()) {
result[Id(feature.GetAlt_object_feature2().GetRule_object_feature_flag1().GetRule_an_id_or_type1(), *this)] = TDeferredAtom();
const TString key = to_lower(Id(
feature.GetAlt_object_feature2().GetRule_object_feature_flag1().GetRule_an_id_or_type1(), *this
));
result[key] = TDeferredAtom();
}
return true;
}
Expand Down

0 comments on commit fdd2ace

Please sign in to comment.