Support CREATE ROLE and DROP ROLE #598
Conversation
Pull Request Test Coverage Report for Build 2969593123
💛 - Coveralls |
| } | ||
| } | ||
| Keyword::IN => { | ||
| if self.parse_keyword(Keyword::ROLE) || self.parse_keyword(Keyword::GROUP) { |
There was a problem hiding this comment.
IN GROUP is an obsolete spelling of IN ROLE.
(postgres docs)
There was a problem hiding this comment.
I think the typical pattern in sqlparser is that we parse the sql as provided into an AST that could recover the same pattern rather than applying semantic meaning
So in this case, can you please change CreateRole to have an explicit group: Option<Ident> rather than reusing in_role?
The same comment applies to Role/User
| self.expected("ROLE or GROUP after IN", self.peek_token()) | ||
| } | ||
| } | ||
| Keyword::ROLE | Keyword::USER => { |
There was a problem hiding this comment.
The USER clause is an obsolete spelling of the ROLE clause.
(postgres docs)
| let if_not_exists = self.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]); | ||
| let names = self.parse_comma_separated(Parser::parse_object_name)?; | ||
|
|
||
| let _ = self.parse_keyword(Keyword::WITH); |
There was a problem hiding this comment.
As written this ignores errors. I think it should be
| let _ = self.parse_keyword(Keyword::WITH); | |
| self.parse_keyword(Keyword::WITH)?; |
There was a problem hiding this comment.
Sorry -- I misunderstood this code. It actually skips the optional WITH clause. Added a PR with a test #627
| } | ||
| } | ||
| Keyword::IN => { | ||
| if self.parse_keyword(Keyword::ROLE) || self.parse_keyword(Keyword::GROUP) { |
There was a problem hiding this comment.
I think the typical pattern in sqlparser is that we parse the sql as provided into an AST that could recover the same pattern rather than applying semantic meaning
So in this case, can you please change CreateRole to have an explicit group: Option<Ident> rather than reusing in_role?
The same comment applies to Role/User
| login: Option<bool>, | ||
| inherit: Option<bool>, | ||
| bypassrls: Option<bool>, | ||
| password: Option<Password>, | ||
| superuser: Option<bool>, | ||
| create_db: Option<bool>, | ||
| create_role: Option<bool>, | ||
| replication: Option<bool>, | ||
| connection_limit: Option<Expr>, | ||
| valid_until: Option<Expr>, |
There was a problem hiding this comment.
One downside with this implementation is that the order of the grants is not preserved and thus the original SQL can not be reconstructed
What do you think about an alternate encoding like:
...
role_grants: Vec<RoleGrant>
...And then
enum RoleGrant {
Login,
Inherit,
Password(Password)
..
}However, I see this PR follows the model of GRANT so I think it is also fine as is
| vec!["browser"], | ||
| grantees.iter().map(ToString::to_string).collect::<Vec<_>>() | ||
| ); | ||
| assert_eq_vec(&["public"], &schemas); |
There was a problem hiding this comment.
thank you -- this is much nicer
Adds some support for parsing CREATE ROLE and DROP ROLE.
CREATE ROLE varies considerably across dialects:
CREATE ROLE [IF NOT EXISTS] role_name, role_name, ...for all dialects[NO]SUPERUSER,[NO]REPLICATION,[NO]BYPASSRLS,PASSWORD ..., etc, etc) - following https://www.postgresql.org/docs/current/sql-createrole.htmlNot yet handled: