-
Notifications
You must be signed in to change notification settings - Fork 668
Add basic support for GRANT and REVOKE #365
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
Conversation
alamb
left a comment
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.
Thank you @blx -- sorry for the lengthy delay -- I didn't see this PR until recently.
I think this PR is well constructed and well tested.
| /// Optional keyword from the spec, ignored in practice | ||
| with_privileges_keyword: bool, |
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.
Maybe worth removing it from the struct then?
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.
@alamb I erred on the side of losslessness here, but I wasn't sure to what degree that's a goal.
I guess there are already many cases where an implied default becomes explicit when parsed, and would then produce different output after a roundtrip if the input had omitted the default (eg. cascade in Statement::Drop).
If you think it would be cleaner I'm happy to PR removing this.
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.
If this field is needed to survive the round trip I think keeping it makes sense.
Pull Request Test Coverage Report for Build 1452484256Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
@blx I merged your code to master and fixed some clippy warnings |
Hello 👋
Preliminary PR here for basic
GRANTandREVOKEsupport. I'm not sure if permissions statements like GRANT are in scope here; if not please feel free to close!My personal experience is mostly postgres-flavored but I've tried to stick to the spec here. Note, this only addresses granting privileges regarding objects (like tables); there is also a
GRANT <role_name> TO <role_specification>for adding users to groups, which is not considered here.This also doesn't yet consider
column permissions (other object types likeGRANT SELECT ON a (id, created) ...), norDATABASEorDOMAIN.Please let me know if there's interest in this, thanks!