Stop printing a trailing space after PUBLIC in GRANT and REVOKE - #2416
Stop printing a trailing space after PUBLIC in GRANT and REVOKE#2416LucaCappelletti94 wants to merge 1 commit into
PUBLIC in GRANT and REVOKE#2416Conversation
ting-hong-shieh
left a comment
ting-hong-shieh
left a comment
There was a problem hiding this comment.
Verified this against main at 2f3b5b82. The trailing space reproduces, and the fix is correct.
One case worth adding to the test: the same bug shows up mid-statement, which is the more damaging form. On the merge base:
GRANT SELECT ON t TO PUBLIC, ROLE r
-> GRANT SELECT ON t TO PUBLIC , ROLE r
With this PR it round-trips unchanged. A trailing space is easy to miss, but a space before the comma corrupts generated SQL visibly. Since verified_stmt("GRANT SELECT ON t TO PUBLIC") still passes when only the end-of-statement case is handled, a multi-grantee case would guard the part that matters most:
all_dialects_except(|d| d.is::<MsSqlDialect>())
.verified_stmt("GRANT SELECT ON t TO PUBLIC, ROLE r");Two things I checked while reviewing:
PUBLICreally is the only grantee type the parser builds without a name —parse_granteestakesSome(name)in every other branch (src/parser/mod.rs:17758). The change does also drop the trailing space for any other type withname: None, but that is only reachable by building the AST directly, and the new output is right there too.- The
MsSqlDialectexclusion is load-bearing: with it removed the test fails, because MsSql parsesPUBLICasgrantee_type: None, name: Some("PUBLIC")while PostgreSQL givesgrantee_type: Public, name: None.
cargo test --all-features at 9d5f8e21: 1584 passed, 0 failed. cargo fmt --all -- --check and cargo clippy --all-targets --all-features -- -D warnings both clean.
|
@iffyio and also this one is rather small |
|
@iffyio ah yes there is also this one that is tiny |
| let keyword = match self.grantee_type { | ||
| GranteesType::Role => "ROLE", | ||
| GranteesType::Share => "SHARE", | ||
| GranteesType::User => "USER", | ||
| GranteesType::Group => "GROUP", | ||
| GranteesType::Public => "PUBLIC", | ||
| GranteesType::DatabaseRole => "DATABASE ROLE", | ||
| GranteesType::Application => "APPLICATION", | ||
| GranteesType::ApplicationRole => "APPLICATION ROLE", | ||
| GranteesType::None => "", | ||
| }; | ||
| f.write_str(keyword)?; | ||
| if let Some(name) = &self.name { | ||
| if !keyword.is_empty() { | ||
| f.write_str(" ")?; |
There was a problem hiding this comment.
not sure I followed the diff correctly (the old and new code look identical in terms of behavior), is the PUBLIC keyword is being handled differently somehow?
3 participants
DisplayforGranteewrote the grantee type keyword with a trailing space baked in, then appended the name.PUBLICis the only grantee type the parser ever builds without a name, so for it the space had nothing to follow it andGRANT SELECT ON t TO PUBLICprinted with a trailing space, which does not match the input it was parsed from.The separator now belongs to the name rather than the keyword, so it is only written when a name follows.