Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

jepett0
Copy link
Collaborator

@jepett0 jepett0 commented Sep 16, 2024

You can see by the changes in the YQL grammar that previously with_table_settings clause was mandatory for the CREATE VIEW statement. It caused a grammar mismatch error whenever a user forgot to add the WITH (security_invoker = true) clause. We want the error to be easy to understand, so we made the with_table_settings⭐ clause an optional parameter in the grammar (i.e. added a ? sign after the with_table_settings clause) and are validating the contents of this clause later during compilation of the CREATE VIEW statement.

⭐ The other interesting part of the PR is the change of the with_table_settings clause to the create_object_features clause in the create_view_stmt. I don't remember why I have chosen with_table_settings initially, but now it seems an error. (Probably, the reason was that the old create_object_features clause couldn't handle bool object features. I have corrected this flaw in this PR too.) All objects can be created with a generic CREATE OBJECT statement, so in order to support this scenario for views, we need to use the generic create_object_features in the create_view_stmt.

Lastly, I have moved the view options validation to KQP which have enabled us to use named expressions for the security_invoker option. It would not be possible if the validation stayed in the parser.

@jepett0 jepett0 force-pushed the VIEW.missing_with.1 branch from fdd2ace to 38b5866 Compare September 16, 2024 21:27
…er = true)`

Validation is moved to KQP to be able to use evaluated expressions to set the `security_invoker` option. See the added unit tests for details.
@jepett0 jepett0 force-pushed the VIEW.missing_with.1 branch from 38b5866 to cd8f8de Compare September 17, 2024 14:45

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@ydb-platform ydb-platform deleted a comment from github-actions bot Sep 17, 2024
@ydb-platform ydb-platform deleted a comment from github-actions bot Sep 17, 2024
@ydb-platform ydb-platform deleted a comment from github-actions bot Sep 17, 2024
@ydb-platform ydb-platform deleted a comment from github-actions bot Sep 17, 2024
@ydb-platform ydb-platform deleted a comment from github-actions bot Sep 17, 2024
@ydb-platform ydb-platform deleted a comment from github-actions bot Sep 17, 2024
@jepett0 jepett0 marked this pull request as ready for review September 17, 2024 16:43
@jepett0 jepett0 requested a review from a team as a code owner September 17, 2024 16:43
nepal
nepal previously approved these changes Sep 18, 2024
show that the option name is case-sensitive
Copy link

github-actions bot commented Sep 19, 2024

2024-09-19 08:42:00 UTC Pre-commit check linux-x86_64-relwithdebinfo for 0d17e3d has started.
2024-09-19 08:42:04 UTC Artifacts will be uploaded here
2024-09-19 08:45:02 UTC ya make is running...
🟡 2024-09-19 10:10:35 UTC Some tests failed, follow the links below. Going to retry failed tests...

Test history | Ya make output

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
76729 62638 0 10 14043 38

2024-09-19 10:17:58 UTC ya make is running... (failed tests rerun, try 2)
🟢 2024-09-19 10:29:20 UTC Tests successful.

Test history | Ya make output

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
295 (only retried tests) 198 0 0 7 90

🟢 2024-09-19 10:29:28 UTC Build successful.
🟡 2024-09-19 10:30:09 UTC ydbd size 8.4 GiB changed* by +237.9 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: 6e3fc43 merge: 0d17e3d diff diff %
ydbd size 9 038 227 872 Bytes 9 038 471 464 Bytes +237.9 KiB +0.003%
ydbd stripped size 489 058 536 Bytes 489 077 832 Bytes +18.8 KiB +0.004%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

Copy link

github-actions bot commented Sep 19, 2024

2024-09-19 08:42:22 UTC Pre-commit check linux-x86_64-release-clang14 for 0d17e3d has started.
2024-09-19 08:42:25 UTC Artifacts will be uploaded here
2024-09-19 08:45:29 UTC ya make is running...
🟢 2024-09-19 09:01:42 UTC Build successful.

Copy link

github-actions bot commented Sep 19, 2024

2024-09-19 08:42:33 UTC Pre-commit check linux-x86_64-release-asan for 0d17e3d has started.
2024-09-19 08:42:43 UTC Artifacts will be uploaded here
2024-09-19 08:45:39 UTC ya make is running...
🔴 2024-09-19 10:34:39 UTC Some tests failed, follow the links below.

Test history | Ya make output

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
14295 14093 0 38 58 106

🟢 2024-09-19 10:35:50 UTC Build successful.
🟡 2024-09-19 10:36:27 UTC ydbd size 5.6 GiB changed* by +587.1 KiB, which is >= 100.0 KiB vs main: Warning

ydbd size dash main: a430757 merge: 0d17e3d diff diff %
ydbd size 6 054 203 864 Bytes 6 054 805 104 Bytes +587.1 KiB +0.010%
ydbd stripped size 1 514 633 424 Bytes 1 514 770 768 Bytes +134.1 KiB +0.009%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

@jepett0 jepett0 requested a review from nepal September 19, 2024 12:28
@jepett0 jepett0 merged commit f90b59c into ydb-platform:main Sep 19, 2024
10 of 12 checks passed
jepett0 added a commit to jepett0/ydb that referenced this pull request Sep 26, 2024
jepett0 added a commit to jepett0/ydb that referenced this pull request Sep 27, 2024
jepett0 added a commit that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants