Skip to content

Conversation

@blx
Copy link
Contributor

@blx blx commented Nov 11, 2021

Hello 👋

Preliminary PR here for basic GRANT and REVOKE support. 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 (GRANT SELECT ON a (id, created) ...), nor other object types like DATABASE or DOMAIN.

Please let me know if there's interest in this, thanks!

@blx blx changed the title Add basic support for GRANT Add basic support for GRANT and REVOKE Nov 12, 2021
Copy link
Contributor

@alamb alamb left a 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.

Comment on lines +1418 to +1419
/// Optional keyword from the spec, ignored in practice
with_privileges_keyword: bool,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@coveralls
Copy link

coveralls commented Dec 8, 2021

Pull Request Test Coverage Report for Build 1452484256

Warning: 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

  • 237 of 284 (83.45%) changed or added relevant lines in 3 files are covered.
  • 37 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.6%) to 89.674%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 65 80 81.25%
src/parser.rs 62 94 65.96%
Files with Coverage Reduction New Missed Lines %
src/ast/data_type.rs 1 76.32%
src/ast/query.rs 1 87.08%
src/ast/value.rs 1 86.67%
src/ast/ddl.rs 2 81.67%
src/ast/mod.rs 5 73.16%
src/tokenizer.rs 7 90.61%
src/parser.rs 20 84.09%
Totals Coverage Status
Change from base Build 1342693636: -0.6%
Covered Lines: 6018
Relevant Lines: 6711

💛 - Coveralls

@alamb
Copy link
Contributor

alamb commented Dec 8, 2021

@blx I merged your code to master and fixed some clippy warnings

@alamb alamb merged commit d7e84be into apache:main Dec 8, 2021
@blx blx deleted the grant branch December 9, 2021 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants