-
Notifications
You must be signed in to change notification settings - Fork 605
Add support for DENY
statements
#1836
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
87b9e03
0eaee16
279732e
681cfe1
e467279
474a968
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,6 +273,7 @@ define_keywords!( | |
DELIMITER, | ||
DELTA, | ||
DENSE_RANK, | ||
DENY, | ||
DEREF, | ||
DESC, | ||
DESCRIBE, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -575,6 +575,10 @@ impl<'a> Parser<'a> { | |
Keyword::SHOW => self.parse_show(), | ||
Keyword::USE => self.parse_use(), | ||
Keyword::GRANT => self.parse_grant(), | ||
Keyword::DENY => { | ||
self.prev_token(); | ||
self.parse_deny() | ||
} | ||
Keyword::REVOKE => self.parse_revoke(), | ||
Keyword::START => self.parse_start_transaction(), | ||
Keyword::BEGIN => self.parse_begin(), | ||
|
@@ -12987,23 +12991,34 @@ impl<'a> Parser<'a> { | |
|
||
/// Parse a GRANT statement. | ||
pub fn parse_grant(&mut self) -> Result<Statement, ParserError> { | ||
let (privileges, objects) = self.parse_grant_revoke_privileges_objects()?; | ||
let (privileges, objects) = self.parse_grant_deny_revoke_privileges_objects()?; | ||
|
||
self.expect_keyword_is(Keyword::TO)?; | ||
let grantees = self.parse_grantees()?; | ||
|
||
let with_grant_option = | ||
self.parse_keywords(&[Keyword::WITH, Keyword::GRANT, Keyword::OPTION]); | ||
|
||
let granted_by = self | ||
.parse_keywords(&[Keyword::GRANTED, Keyword::BY]) | ||
.then(|| self.parse_identifier().unwrap()); | ||
let as_grantor = if self.peek_keyword(Keyword::AS) { | ||
self.parse_keywords(&[Keyword::AS]) | ||
.then(|| self.parse_identifier().unwrap()) | ||
} else { | ||
None | ||
}; | ||
|
||
let granted_by = if self.peek_keywords(&[Keyword::GRANTED, Keyword::BY]) { | ||
self.parse_keywords(&[Keyword::GRANTED, Keyword::BY]) | ||
.then(|| self.parse_identifier().unwrap()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar here, we would want to return an error vs unwrap |
||
} else { | ||
None | ||
}; | ||
|
||
Ok(Statement::Grant { | ||
privileges, | ||
objects, | ||
grantees, | ||
with_grant_option, | ||
as_grantor, | ||
granted_by, | ||
}) | ||
} | ||
|
@@ -13020,14 +13035,18 @@ impl<'a> Parser<'a> { | |
GranteesType::Share | ||
} else if self.parse_keyword(Keyword::GROUP) { | ||
GranteesType::Group | ||
} else if self.parse_keyword(Keyword::PUBLIC) { | ||
GranteesType::Public | ||
} else if self.parse_keywords(&[Keyword::DATABASE, Keyword::ROLE]) { | ||
GranteesType::DatabaseRole | ||
} else if self.parse_keywords(&[Keyword::APPLICATION, Keyword::ROLE]) { | ||
GranteesType::ApplicationRole | ||
} else if self.parse_keyword(Keyword::APPLICATION) { | ||
GranteesType::Application | ||
} else if self.peek_keyword(Keyword::PUBLIC) { | ||
if dialect_of!(self is MsSqlDialect) { | ||
grantee_type | ||
} else { | ||
GranteesType::Public | ||
} | ||
Comment on lines
+13043
to
+13047
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm not sure I followed the intention here, public is represented differently for mssql? (we would want to describe that behavior without the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My interpretation of the existing code is that "public" is being interpreted as a keyword. With SQL Server, "public" is a built in role (ref) From a parsing perspective, there wouldn't be any difference between a built-in role & any other role. The consideration here is "keyword" vs "not keyword". The additional test case examples here cover this behavior. Put simply, this (and GRANT) should be able to parse normally:
With regards to the code, what is the preferred way to implement this behavior? Brainstorming...
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, yeah option 3. sounds reasonable and we could try that? |
||
} else { | ||
grantee_type // keep from previous iteraton, if not specified | ||
}; | ||
|
@@ -13066,7 +13085,7 @@ impl<'a> Parser<'a> { | |
Ok(values) | ||
} | ||
|
||
pub fn parse_grant_revoke_privileges_objects( | ||
pub fn parse_grant_deny_revoke_privileges_objects( | ||
&mut self, | ||
) -> Result<(Privileges, Option<GrantObjects>), ParserError> { | ||
let privileges = if self.parse_keyword(Keyword::ALL) { | ||
|
@@ -13116,7 +13135,6 @@ impl<'a> Parser<'a> { | |
let object_type = self.parse_one_of_keywords(&[ | ||
Keyword::SEQUENCE, | ||
Keyword::DATABASE, | ||
Keyword::DATABASE, | ||
Keyword::SCHEMA, | ||
Keyword::TABLE, | ||
Keyword::VIEW, | ||
|
@@ -13211,6 +13229,9 @@ impl<'a> Parser<'a> { | |
Ok(Action::Create { obj_type }) | ||
} else if self.parse_keyword(Keyword::DELETE) { | ||
Ok(Action::Delete) | ||
} else if self.parse_keyword(Keyword::EXEC) { | ||
let obj_type = self.maybe_parse_action_execute_obj_type(); | ||
Ok(Action::Exec { obj_type }) | ||
} else if self.parse_keyword(Keyword::EXECUTE) { | ||
let obj_type = self.maybe_parse_action_execute_obj_type(); | ||
Ok(Action::Execute { obj_type }) | ||
|
@@ -13409,9 +13430,40 @@ impl<'a> Parser<'a> { | |
} | ||
} | ||
|
||
/// Parse [`Statement::Deny`] | ||
pub fn parse_deny(&mut self) -> Result<Statement, ParserError> { | ||
self.expect_keyword(Keyword::DENY)?; | ||
|
||
let (privileges, objects) = self.parse_grant_deny_revoke_privileges_objects()?; | ||
let objects = match objects { | ||
Some(o) => o, | ||
None => { | ||
return parser_err!( | ||
"DENY statements must specify an object", | ||
self.peek_token().span.start | ||
) | ||
} | ||
}; | ||
|
||
self.expect_keyword_is(Keyword::TO)?; | ||
let grantees = self.parse_grantees()?; | ||
let cascade = self.parse_cascade_option(); | ||
let granted_by = self | ||
.parse_keywords(&[Keyword::AS]) | ||
.then(|| self.parse_identifier().unwrap()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment here re unwrap |
||
|
||
Ok(Statement::Deny(DenyStatement { | ||
privileges, | ||
objects, | ||
grantees, | ||
cascade, | ||
granted_by, | ||
})) | ||
} | ||
|
||
/// Parse a REVOKE statement | ||
pub fn parse_revoke(&mut self) -> Result<Statement, ParserError> { | ||
let (privileges, objects) = self.parse_grant_revoke_privileges_objects()?; | ||
let (privileges, objects) = self.parse_grant_deny_revoke_privileges_objects()?; | ||
|
||
self.expect_keyword_is(Keyword::FROM)?; | ||
let grantees = self.parse_grantees()?; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3283,6 +3283,7 @@ fn parse_grant() { | |
objects, | ||
grantees, | ||
with_grant_option, | ||
as_grantor: _, | ||
granted_by, | ||
} = stmt | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I realise this unwrap wasn't introduced by this PR, but can we switch it to return an error instead? it looks otherwise like an scenario where the parser will panic on invalid sql which would be undesirable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored these three related methods to avoid unwrap 👍