Skip to content

Conversation

psteinroe
Copy link
Collaborator

@psteinroe psteinroe commented Sep 12, 2025

we were blindly converting named parameters to positional parameters. but the latter is only valid as a literal, and not as an identifier.

statements like

grant usage on schema public, app_public, app_hidden to :DB_ROLE;

are not valid when converted to

grant usage on schema public, app_public, app_hidden to $1;

i went a bit back and forth on this and decided the easiest way to fix this is to convert to identifiers like a if the previous non-trivia token is one of a set list of tokens. We will probably have a bunch of edge cases here but fixing them should be as easy as adding a keyword to the list.

now, we convert to

grant usage on schema public, app_public, app_hidden to a;

closes #510

Copy link
Collaborator

@juleswritescode juleswritescode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

So the typechecker doesn't properly check whether a role exists?
I would've expected it to complain about the "missing role a"

"{}{}",
ALPHABET[pos],
if iteration > 0 {
deterministic_identifier(iteration - 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i love me a good recursion

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

das kam sogar ohne Claude 😂

SyntaxKind::POSITIONAL_PARAM
SyntaxKind::NAMED_PARAM
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

das war n bug nehm ich an?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ja, allerdings kein wichtiger für die Statement Splitter Logik deshalb kam es nie raus

@psteinroe
Copy link
Collaborator Author

nice!

So the typechecker doesn't properly check whether a role exists?

I would've expected it to complain about the "missing role a"

The typechecker just runs on DML statements and such replacement are less of an issue there. but at some point someone will report it too I guess. If that happens, we can find another solution for it. 😅👍🏼

@psteinroe psteinroe merged commit e3d1576 into main Sep 13, 2025
8 checks passed
@psteinroe psteinroe deleted the feat/grahile-vars branch September 13, 2025 09:52
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.

Support graphile migrate placeholders
2 participants