Conversation
liuwei08
changed the title
Follow-up on the get_files() fix that filters migrations by file number instead of list index: even with the correct file selected, the runner still failed on a comment-only migration file (like 00046.sql). apply_migration() only did sql.strip() and then guarded with 'if sql:', which considers any non-empty string as executable. For a file that contains only SQL line comments, strip() keeps all the '--' lines, so the guard passes and psycopg2 rejects the call with: ProgrammingError: can't execute an empty query This blocked the infrabox-db migration Job whenever a comment-only placeholder was in the pending set (observed on databases at schema_version < 46, where 00046.sql is a pure comment file). Add a _has_executable_sql() helper that treats a file as empty when every non-blank line is a '-- ...' comment. When empty, skip the cur.execute() call but still advance schema_version, so subsequent migrations (00047, 00048, ...) can proceed.
The initial guard only matched -- line comments. If a migration file happens to contain only /* ... */ block comments (or a mix with --), apply_migration would still call cur.execute() and psycopg2 would reject it with 'can't execute an empty query'. Strip block comments with a non-greedy DOTALL regex before the per-line -- check, so any comment-only file (empty, whitespace, --, /* */, or mixed) is now correctly detected as having no executable SQL and skipped, while still advancing schema_version. Tested manually against the following cases (all pass): empty -> SKIP whitespace only -> SKIP only -- lines -> SKIP only /* */ block -> SKIP multiline /* */ -> SKIP mixed comments -> SKIP real SQL -> EXEC -- + real SQL -> EXEC /* */ + real SQL -> EXEC
…nly) as end-to-end no-op fixtures Two additional placeholder migrations exercise the two remaining branches of the _has_executable_sql() guard on a live database, not just in unit tests: - 00049.sql — 0 bytes, exercises the completely-empty / whitespace-only path. - 00050.sql — a single /* ... */ block comment, exercises the block- comment stripping path. Together with 00046.sql (only -- line comments), all three comment/empty variants are now covered by real migration files that the runner will encounter in production, so any regression in _has_executable_sql() would immediately be caught by a failing migration Job.
added 2 commits
August 24, 2026 15:10
Third fixture in the DM01-6184 test suite: numbers 00051..00059 are intentionally missing so this file exercises the gap-tolerant selection logic introduced by the first commit on this branch. Before the fix, get_files() used files[current_schema_version:] to slice the sorted list. Any gap in the numbering desynchronised the index from the file number, causing later migrations to be silently skipped or re-run. With the fix (files filtered by int(f[:5]) > current_schema_version) a gap of any size is handled correctly, and this file — placed 9 numbers after the previous one — verifies that behaviour end-to-end in a real migration run. The statement is just SELECT 1;, i.e. no schema change, so it is safe to land on master.
The three test fixtures were added to exercise the migration runner fixes on a live database. They land unnecessarily on master (advancing schema_version to 60 across all deployments), and the fixes themselves are already covered by the code changes and manual verification. Removing them here so the PR only carries the runner fixes; the fixtures can be reapplied on a separate ephemeral branch when we want to validate end-to-end against a real DB, without polluting the master schema history.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.