fix(DM01-6184): filter migrations by file number + skip comment-only files - #638
Closed
Jiachen0715 wants to merge 2 commits into
Closed
fix(DM01-6184): filter migrations by file number + skip comment-only files#638Jiachen0715 wants to merge 2 commits into
Jiachen0715 wants to merge 2 commits into
Conversation
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.
Contributor
Author
|
Superseded — pushed the additional commit directly to the PR #636 branch (DM01-6184) instead. |
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
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.
Summary
Builds on #636 (
fix(DM01-6184): filter migrations by file number instead of list index) and adds a second fix that is needed to fully unblock theinfrabox-dbmigration Job.Two independent bugs, one root symptom
test-newdatabases starting belowschema_version=46currently see the migration Job crash with:There are actually two bugs at play, so #636 alone is not enough. This PR keeps #636's fix and adds the missing one.
Bug 1 —
get_filesused list index instead of file number ✅ fixed by #636Historically
00046.sqldid not exist (the sequence went45 → 47), andfiles = files[current_schema_version:]used list index to slice pending migrations. Any gap in the numbering desynchronised the index from the file number, silently skipping later migrations. #636 replaces the slice with a file-number filter — kept as-is in this PR.Bug 2 —
apply_migrationtreats comment-only files as executable ← this PR addsEven with #636's filter selecting the correct files, once
00046.sql(an intentionally empty placeholder that only contains-- ...lines) is chosen, the runner still fails:The result: the migration Job crashes before ever reaching
00047.sql(which createsmcp_token,mcp_access_log, and addsbuild.source) or00048.sql(secret_read_token). Downstream, MCP-related endpoints return500because the tables don't exist.Fix in this PR
Add a small
_has_executable_sql()helper that returnsTrueonly if the file has at least one non-blank line that isn't a--comment.apply_migrationuses it to decide whether to callcur.execute():schema_versionis still bumped afterwards, so the placeholder still serves its purpose (closing a numbering gap).Scope
get_filesfix from fix: filter migrations by file number instead of list index #636 (unchanged)._has_executable_sqlhelper + guard swap inapply_migration.src/db/migrate.pychanges. No migration file, no DB schema, no API/UI impact.Behaviour after this PR
.sqlwith statements.sqlwith only-- comments.pymigrationimport+migrate(conn)Test plan
schema_version=45, run against a migrations folder containing the current00046.sql(comment-only) +00047.sql. Confirm the pre-fix code crashes and the post-fix code logsSkipping execute for ...00046.sqland completes.test-new) that is currently stuck atschema_version=45. Expect theinfrabox-dbJob to succeed and MCP endpoints (GET/POST /api/v1/mcp/tokens) to return 200 instead of 500.Notes
_has_executable_sql()is a lightweight line-based heuristic. It only recognises-- linecomments; it does not parse SQL and does not understand/* block */comments. That's fine for the current use case (the placeholder file uses--comments), but if we ever want to be stricter we can swap in a real tokenizer later.SAP/InfraBox#637(unrelated feature branch) which also had to work around this same crash. Once this PR merges, that workaround (addingSELECT 1;inside00046.sql) becomes unnecessary and can be reverted.