Skip to content

Clarify syntax for role grants#19340

Merged
mosabua merged 1 commit intotrinodb:masterfrom
Jessie212:jt/grant-role
Oct 23, 2023
Merged

Clarify syntax for role grants#19340
mosabua merged 1 commit intotrinodb:masterfrom
Jessie212:jt/grant-role

Conversation

@Jessie212
Copy link
Contributor

@Jessie212 Jessie212 commented Oct 10, 2023

Description

Clarify syntax for role grants. The current syntax has misled multiple readers to think that this is valid.

GRANT ROLE test TO jessie212;

We assume that readers think that the lowercase role is a typo.

I worked with @mosabua and confirmed in the antlr spec that this is wrong and the syntax is

GRANT test TO jessie212;

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Oct 10, 2023
@Jessie212 Jessie212 requested a review from mosabua October 10, 2023 22:08
@mosabua mosabua changed the title Correct GRANT ROLES syntax Clarify syntax for role grants Oct 10, 2023
@mosabua
Copy link
Member

mosabua commented Oct 10, 2023

Also note that this mimics the syntax example for CREATE ROLE

CREATE ROLE role_name
[ WITH ADMIN ( user | USER user | ROLE role | CURRENT_USER | CURRENT_ROLE ) ]
[ IN catalog ]

@mosabua
Copy link
Member

mosabua commented Oct 10, 2023

Can you chime in here @martint and @dain ?

Its kinda weird that we have grant and grant roles as separate statements with similar syntax .. but I guess the SQL spec says so..

@Jessie212
Copy link
Contributor Author

Also note that this mimics the syntax example for CREATE ROLE

CREATE ROLE role_name
[ WITH ADMIN ( user | USER user | ROLE role | CURRENT_USER | CURRENT_ROLE ) ]
[ IN catalog ]

There's a See also section way at the bottom that links to the CREATE ROLES , DROP ROLES, SET ROLES, and REVOKE ROLES docs.

@mosabua
Copy link
Member

mosabua commented Oct 10, 2023

Also .. maybe we should change the title of this page to

GRANT roles

or even

GRANT role

And to adjust for consistency change the GRANT page to

GRANT privilege

as title.

@Jessie212
Copy link
Contributor Author

GRANT role

And to adjust for consistency change the GRANT page to

GRANT privilege

as title.

Should we condense the statements? For instance, all CREATE statements go on the CREATE page. Do the same for DROP, GRANT, ALTER etc...

@mosabua
Copy link
Member

mosabua commented Oct 12, 2023

GRANT role

And to adjust for consistency change the GRANT page to

GRANT privilege

as title.

Should we condense the statements? For instance, all CREATE statements go on the CREATE page. Do the same for DROP, GRANT, ALTER etc...

No ... the objects are very important .. so the statements are all a verb and a object typically .. CREATE TABLE .. and so on. GRANT is kinda the exception.

@Jessie212
Copy link
Contributor Author

No ... the objects are very important .. so the statements are all a verb and a object typically .. CREATE TABLE .. and so on. GRANT is kinda the exception.

Okay. I will move and rename the GRANT files in the follow-up PR.

What about this comment: #19340 (comment)

@mosabua
Copy link
Member

mosabua commented Oct 12, 2023

Lets wait and see until we get some feedback/input from @martint and @electrum .. probably next week

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now. Follow approach discussed and approved by @electrum . Thank you @Jessie212

@mosabua mosabua merged commit ccd79be into trinodb:master Oct 23, 2023
@github-actions github-actions bot added this to the 431 milestone Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants