Skip to content

storage-cassandra: the migration runner splits a file at a ; inside a comment or a string literal #675

Open

Description

@LeeroyHannigan

The Cassandra migration runner splits each migration file at every ; and runs each fragment as a statement: sql.split(';') in crates/storage-cassandra/src/migrations.rs, in execute_migration on main and in migration_statements in #638. It doesn't skip comments or string literals, so a ; inside one ends the statement there. @jcshepherd raised this in review on #638, for comments and for string literals.

Running migration_statements from #638's head (370a6b71) with keyspace ks:

  • A ; in a comment that follows a statement ends the statement, and the rest of the comment becomes the next statement. For this line:

    CREATE TABLE IF NOT EXISTS b (k int PRIMARY KEY) -- was: ; INSERT INTO settings (key, value) VALUES ('catalog_version', '9.9.9') ;
    

    migration_statements returns CREATE TABLE IF NOT EXISTS ks.b (k int PRIMARY KEY) -- was: and INSERT INTO ks.settings (key, value) VALUES ('catalog_version', '9.9.9'), and the runner runs both. main splits the line the same way and runs both statements too.

  • A ; in a string literal cuts the literal in two. INSERT INTO settings (key, value) VALUES ('note', 'a;b'); fails with has a statement that names no table the runner can qualify with a keyspace: b').

  • fix(storage-cassandra): qualify migration statements with their keyspace instead of USE #638 skips -- and // comments at the start of a statement before it looks for the table name, but not /* */ comments, so /* header */ CREATE TABLE a (k int PRIMARY KEY); fails with the same error. This one is new in fix(storage-cassandra): qualify migration statements with their keyspace instead of USE #638: main sends the comment through to Cassandra, and CQL allows /* */ comments.

#638 also rejects a fragment that holds only a comment. That catches a ; in a comment between two statements, but not one in a comment that follows a statement.

None of the seven files under crates/storage-cassandra/migrations has a ; in a comment or a string literal, or a /* */ comment, so this affects only migrations added later.

Fix

Split with a scanner that reads the file once and tracks whether it's inside:

  • a string literal, in single quotes with '' for a quote inside, or between two $$
  • a quoted identifier, in double quotes with "" for a quote inside
  • a -- or // comment, which runs to the end of the line
  • a /* */ comment, which CQL doesn't nest

The scanner drops comments and splits only at a ; outside all four. A file that ends inside a literal, a quoted identifier or a /* */ comment fails with an error naming the file. Since the scanner drops comments, #638's error for a fragment that holds only a comment is no longer needed. The scanner only tracks quotes and comments, so it doesn't need CQL's grammar.

Tests: the three inputs above, 'x -- y' and 'it''s' in a literal (both stay in the statement), and a $$a;b$$ literal. every_migration_statement_names_the_keyspace (from #638) builds the statements it expects with split(';'), so it has to build them with the scanner instead, and a_fragment_of_only_a_comment_fails_naming_the_file is removed along with the error it tests.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions