Skip to content

Harden --migration_table_name: validation, naming consistency, and truncate support - #163

Merged
sinmetal merged 4 commits into
cloudspannerecosystem:masterfrom
sinmetal:fix-migration-table-name-validation
Aug 7, 2026
Merged

Harden --migration_table_name: validation, naming consistency, and truncate support#163
sinmetal merged 4 commits into
cloudspannerecosystem:masterfrom
sinmetal:fix-migration-table-name-validation

Conversation

@sinmetal

@sinmetal sinmetal commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

WHY

#152 added the --migration-table-name flag. It works, but it left three issues behind.

  1. The table name is interpolated into SQL/DDL without any validation.
    GetSchemaMigrationVersion builds "SELECT Version, Dirty FROM " + tableName + " LIMIT 1"
    and EnsureMigrationTable builds fmt.Sprintf("CREATE TABLE %s ..."). Until Add --migration-table-name flag to migrate command #152 the
    name was a compile-time constant, so this was safe. Now it is user input, and a value
    passed from a CI variable can change the statement. Passing an empty string also produced
    a confusing CREATE TABLE ( error from Cloud Spanner instead of a clear message.

  2. The flag name does not follow the existing convention.
    Every other multi-word flag in wrench uses snake_case: credentials_file, schema_file,
    proto_descriptor_file. Only this one was kebab-case.

  3. truncate silently destroys the migration version when a custom table is used.
    TruncateAllTables skips the literal SchemaMigrations table so that a truncated
    database keeps its migration version. With a custom table name, that exclusion does not
    match, the version row is deleted, and the next migrate up re-applies every migration
    from version 0 — failing with errors like Duplicate column name ... for DDL migrations.
    This is easy to hit because truncate is a routine command in development and CI, and
    the failure surfaces later in an unrelated command.

WHAT

Validate the table name and quote it (507e775)

  • Validate against ^[A-Za-z][A-Za-z0-9_]{0,127}$ in getMigrationTableName, before
    connecting to Cloud Spanner, matching the Cloud Spanner table name rules
  • Fall back to the default SchemaMigrations when the flag is empty, consistently with
    schemaFilePath()
  • Quote the table name with backticks in GetSchemaMigrationVersion and
    EnsureMigrationTable, as TruncateAllTables already does
  • Resolve the table name once per command instead of on every use

Rename the flag to --migration_table_name (6ec6969)

  • The flag is not included in any release yet (latest tag is v1.13.5), so it is renamed
    without keeping an alias

Keep the specified migration table on truncate (6337f22)

  • TruncateAllTables now takes the migration table name and excludes that table
  • truncate accepts --migration_table_name
  • getMigrationTableName moved to cmd/cmd.go since it is no longer migrate specific

Docs are updated: the README describes the name restriction, and notes that the same
--migration_table_name value must be given to migrate up / version / set and
truncate.

Testing

  • TestGetMigrationTableName covers the empty fallback, a leading digit, an invalid
    character, a backtick-based injection string, and a name longer than 128 characters
  • TestTruncateAllTables is new and covers both the default and a custom table name.
    Verified that it fails against the previous hardcoded exclusion
  • Full suite passes against the Spanner emulator
  • Manually verified against the emulator: truncate --migration_table_name Data_Migrations
    keeps the version, while truncate without the flag drops it to No migrations.ion why you submit this pull request
    -->

The --migration-table-name flag turned a compile-time constant into user
input that is interpolated into SQL and DDL statements.

- Validate the name against ^[A-Za-z][A-Za-z0-9_]{0,127}$ in
  getMigrationTableName, before connecting to Cloud Spanner
- Fall back to the default SchemaMigrations when the flag is empty,
  consistently with schemaFilePath
- Quote the table name with backticks in GetSchemaMigrationVersion and
  EnsureMigrationTable, as TruncateAllTables already does
- Resolve the table name once per command instead of on every use
All other multi-word flags in wrench use snake_case
(credentials_file, schema_file, proto_descriptor_file). The flag is not
included in any release yet, so it is renamed without an alias.
TruncateAllTables excluded the literal SchemaMigrations table, so a
database using --migration_table_name lost its migration version on
truncate and the next migrate up re-applied every migration.

- Pass the migration table name to TruncateAllTables
- Add --migration_table_name to the truncate command
- Move getMigrationTableName to cmd.go, it is no longer migrate specific
- Add TestTruncateAllTables covering the default and a custom table name
@google-cla

google-cla Bot commented Aug 5, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sinmetal
sinmetal force-pushed the fix-migration-table-name-validation branch from 6337f22 to c57af8d Compare August 5, 2026 05:55
@sinmetal
sinmetal marked this pull request as ready for review August 5, 2026 05:59
@sinmetal
sinmetal requested review from knwoop, utahta and vvakame August 5, 2026 06:00
Cloud Spanner identifiers are case insensitive, but INFORMATION_SCHEMA
returns the declared name. --migration_table_name schemamigrations
therefore resolved to the existing SchemaMigrations table everywhere
except the exclusion check in TruncateAllTables, which deleted the
migration version.

@vvakame vvakame left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

Note: This changes contains breaking change.

@sinmetal
sinmetal merged commit 6ea1cd9 into cloudspannerecosystem:master Aug 7, 2026
8 checks passed
@sinmetal
sinmetal deleted the fix-migration-table-name-validation branch August 7, 2026 09:37
@knwoop

knwoop commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

@sinmetal Sorry for the late look — catching up after the merge. Thanks for the fix!

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