Add support for DENY statements
#1836
Conversation
- this is required to have common tests with identifiers with that quote style
src/parser/mod.rs
Outdated
| self.parse_keywords(&[Keyword::AS]) | ||
| .then(|| self.parse_identifier().unwrap()) |
There was a problem hiding this comment.
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.
I've refactored these three related methods to avoid unwrap 👍
src/parser/mod.rs
Outdated
|
|
||
| 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.
similar here, we would want to return an error vs unwrap
src/parser/mod.rs
Outdated
| let cascade = self.parse_cascade_option(); | ||
| let granted_by = self | ||
| .parse_keywords(&[Keyword::AS]) | ||
| .then(|| self.parse_identifier().unwrap()); |
| if dialect_of!(self is MsSqlDialect) { | ||
| grantee_type | ||
| } else { | ||
| GranteesType::Public | ||
| } |
There was a problem hiding this comment.
hmm not sure I followed the intention here, public is represented differently for mssql? (we would want to describe that behavior without the dialect_of macro I think)
There was a problem hiding this comment.
hmm not sure I followed the intention here, public is represented differently for mssql? (we would want to describe that behavior without the
dialect_ofmacro I think)
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:
DENY SELECT ON my_table TO public
With regards to the code, what is the preferred way to implement this behavior? Brainstorming...
- Use
dialect_of - Copy/paste this entire method into the dialect so it's fully duplicated except for this keyword
- Implement a dialect function to describe "tokens that are listed as keywords but should be treated as identifiers", or similar, and then it's blank for most dialects but returns "public" for SQL Server
- Something else...?
This is another statement for SQL Server: https://learn.microsoft.com/en-us/sql/t-sql/statements/deny-transact-sql, but implemented in a common way. Similar to GRANT & REVOKE, so using a lot of those patterns with a similar test.
Supplementary improvements:
EXECwas missing from the privileges list, so that's added now too (with a test example).[like this]. That dialect function hadn't been implemented yet, which caused a test failure on example that use that quoting style. So I've added that now too with a couple example cases for grant/deny.Finally,"public" is a built in role on SQL Server, so doing something likegrant select on mytable to publicshould parse public normally. However, the former logic was reading it as a keyword. This has been adjusted to be not special on SQL Server and tests cases have been added as well.GRANT ... AS roleparsing is now supported (formerly only worked for REVOKE/DENY) and a new test case example