From eea7214b14dcd096afcc5b690699b4f3051676c9 Mon Sep 17 00:00:00 2001 From: I515719 Date: Thu, 20 Aug 2026 10:49:14 +0800 Subject: [PATCH 1/6] fix(DM01-6184): filter migrations by file number instead of list index --- src/db/migrate.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/db/migrate.py b/src/db/migrate.py index 52981447f..2511cf144 100644 --- a/src/db/migrate.py +++ b/src/db/migrate.py @@ -14,7 +14,7 @@ def get_files(current_schema_version): files = [f for f in os.listdir(migration_path) if os.path.isfile(os.path.join(migration_path, f)) and f.startswith('0')] files.sort(key=lambda f: int(f[:5])) - files = files[current_schema_version:] + files = [f for f in files if int(f[:5]) > current_schema_version] return [(os.path.join(migration_path, f), int(f[:5])) for f in files] From ee722eb4de9c5b931fc3bbd9cdfa6531a34c6cc0 Mon Sep 17 00:00:00 2001 From: Jiachen Fan Date: Mon, 24 Aug 2026 13:52:48 +0800 Subject: [PATCH 2/6] fix(DM01-6184): skip execute when migration file has no non-comment SQL 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. --- src/db/migrate.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/db/migrate.py b/src/db/migrate.py index 2511cf144..7cc5ada91 100644 --- a/src/db/migrate.py +++ b/src/db/migrate.py @@ -18,6 +18,27 @@ def get_files(current_schema_version): return [(os.path.join(migration_path, f), int(f[:5])) for f in files] +def _has_executable_sql(sql): + """Return True iff `sql` contains at least one non-comment, non-blank line. + + `str.strip()` only removes leading/trailing whitespace; SQL line comments + (`-- ...`) are kept. Passing a comment-only string to `cur.execute()` + raises `psycopg2.ProgrammingError: can't execute an empty query`, which + breaks migration runs whenever a placeholder file (e.g. an intentionally + empty migration to close a numbering gap) is applied. This helper skips + such files safely. + + NOTE: This is a lightweight heuristic — it does NOT parse SQL and does + not understand `/* ... */` block comments. For a comment-only migration + file to be recognised as empty, use `--` line comments. + """ + for line in sql.splitlines(): + stripped = line.strip() + if stripped and not stripped.startswith('--'): + return True + return False + + def apply_migration(conn, migration): filename = migration[0] logger.info("Starting to apply migration %s", filename) @@ -27,8 +48,13 @@ def apply_migration(conn, migration): sql = sql_file.read().strip() cur = conn.cursor() - if sql: + if _has_executable_sql(sql): cur.execute(sql) + else: + logger.info( + "Skipping execute for %s (no non-comment SQL statements)", + filename, + ) cur.close() elif filename.endswith('.py'): From ab88ffbae84707f2876872b3e683b74cd333982f Mon Sep 17 00:00:00 2001 From: Jiachen Fan Date: Mon, 24 Aug 2026 14:04:47 +0800 Subject: [PATCH 3/6] fix(DM01-6184): also recognise C-style /* ... */ block comments as empty 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 --- src/db/migrate.py | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/src/db/migrate.py b/src/db/migrate.py index 7cc5ada91..5f9f80491 100644 --- a/src/db/migrate.py +++ b/src/db/migrate.py @@ -1,10 +1,17 @@ import os +import re + import bcrypt import importlib from pyinfraboxutils import get_logger, get_env from pyinfraboxutils.db import connect_db + +# Matches C-style block comments (/* ... */), across multiple lines. +# Non-greedy so consecutive `/* a */ x /* b */` blocks are matched independently. +_BLOCK_COMMENT_RE = re.compile(r'/\*.*?\*/', re.DOTALL) + logger = get_logger("migrate") def get_files(current_schema_version): @@ -21,18 +28,27 @@ def get_files(current_schema_version): def _has_executable_sql(sql): """Return True iff `sql` contains at least one non-comment, non-blank line. - `str.strip()` only removes leading/trailing whitespace; SQL line comments - (`-- ...`) are kept. Passing a comment-only string to `cur.execute()` - raises `psycopg2.ProgrammingError: can't execute an empty query`, which - breaks migration runs whenever a placeholder file (e.g. an intentionally - empty migration to close a numbering gap) is applied. This helper skips - such files safely. - - NOTE: This is a lightweight heuristic — it does NOT parse SQL and does - not understand `/* ... */` block comments. For a comment-only migration - file to be recognised as empty, use `--` line comments. + `str.strip()` only removes leading/trailing whitespace; SQL comments are + kept. Passing a comment-only string to `cur.execute()` raises + `psycopg2.ProgrammingError: can't execute an empty query`, which breaks + migration runs whenever a placeholder file (e.g. an intentionally empty + migration to close a numbering gap) is applied. This helper detects such + files so the caller can skip the execute call. + + Recognised as "no SQL": + - completely empty / whitespace-only files + - only `-- ...` line comments + - only `/* ... */` block comments (even across lines) + - any combination of the above + + Lightweight heuristic — does not tokenise SQL, so quoted strings that + happen to contain `--` or `/* */` are still treated as executable + (which is fine, they contain real statements). """ - for line in sql.splitlines(): + # Strip C-style block comments first so any `/* ... */`-only content is + # collapsed away before the per-line `--` check. + sql_no_block = _BLOCK_COMMENT_RE.sub('', sql) + for line in sql_no_block.splitlines(): stripped = line.strip() if stripped and not stripped.startswith('--'): return True From 2d7504559ec4df192b19e3b40b285e22994466b9 Mon Sep 17 00:00:00 2001 From: Jiachen Fan Date: Mon, 24 Aug 2026 15:04:48 +0800 Subject: [PATCH 4/6] test(DM01-6184): add 00049.sql (empty) and 00050.sql (block-comment only) as end-to-end no-op fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/db/migrations/00049.sql | 0 src/db/migrations/00050.sql | 11 +++++++++++ 2 files changed, 11 insertions(+) create mode 100644 src/db/migrations/00049.sql create mode 100644 src/db/migrations/00050.sql diff --git a/src/db/migrations/00049.sql b/src/db/migrations/00049.sql new file mode 100644 index 000000000..e69de29bb diff --git a/src/db/migrations/00050.sql b/src/db/migrations/00050.sql new file mode 100644 index 000000000..7821aa7f3 --- /dev/null +++ b/src/db/migrations/00050.sql @@ -0,0 +1,11 @@ +/* Intentionally empty placeholder migration (block-comment variant). + * + * This file is a no-op used to verify that migrate.py correctly recognises + * a migration whose only non-blank content is a C-style block comment + * and skips the psycopg2 execute call, while still advancing schema_version. + * + * Companion of 00049.sql (a completely empty file, 0 bytes) which exercises + * the whitespace-only branch of the same guard. + * + * See DM01-6184 (SAP/InfraBox#636) for details. + */ \ No newline at end of file From f8e35ebe897e6f50d6c1a1f840cd8c3476539bb2 Mon Sep 17 00:00:00 2001 From: Jiachen Fan Date: Mon, 24 Aug 2026 15:10:10 +0800 Subject: [PATCH 5/6] test(DM01-6184): add 00060.sql with a 9-file gap to exercise get_files() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/db/migrations/00060.sql | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 src/db/migrations/00060.sql diff --git a/src/db/migrations/00060.sql b/src/db/migrations/00060.sql new file mode 100644 index 000000000..fb1a2cfd8 --- /dev/null +++ b/src/db/migrations/00060.sql @@ -0,0 +1,13 @@ +-- Non-contiguous migration fixture. +-- +-- Numbers 00051..00059 are intentionally missing so this file exercises the +-- gap-tolerant selection logic added in the first commit of DM01-6184 +-- (SAP/InfraBox#636): get_files() now filters by file number +-- (`int(f[:5]) > current_schema_version`) instead of slicing the sorted list +-- with the index, so a gap of any size no longer causes later migrations to +-- be skipped or re-run. +-- +-- The statement below is intentionally trivial (no schema change), so +-- apply_migration is expected to run it via psycopg2 without side effects +-- and still advance schema_version to 60. +SELECT 1; \ No newline at end of file From 9667974177d55b4d3539883bf743b867ff6e3695 Mon Sep 17 00:00:00 2001 From: Jiachen Fan Date: Mon, 24 Aug 2026 15:58:03 +0800 Subject: [PATCH 6/6] test(DM01-6184): remove end-to-end fixtures 00049/00050/00060 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. --- src/db/migrations/00049.sql | 0 src/db/migrations/00050.sql | 11 ----------- src/db/migrations/00060.sql | 13 ------------- 3 files changed, 24 deletions(-) delete mode 100644 src/db/migrations/00049.sql delete mode 100644 src/db/migrations/00050.sql delete mode 100644 src/db/migrations/00060.sql diff --git a/src/db/migrations/00049.sql b/src/db/migrations/00049.sql deleted file mode 100644 index e69de29bb..000000000 diff --git a/src/db/migrations/00050.sql b/src/db/migrations/00050.sql deleted file mode 100644 index 7821aa7f3..000000000 --- a/src/db/migrations/00050.sql +++ /dev/null @@ -1,11 +0,0 @@ -/* Intentionally empty placeholder migration (block-comment variant). - * - * This file is a no-op used to verify that migrate.py correctly recognises - * a migration whose only non-blank content is a C-style block comment - * and skips the psycopg2 execute call, while still advancing schema_version. - * - * Companion of 00049.sql (a completely empty file, 0 bytes) which exercises - * the whitespace-only branch of the same guard. - * - * See DM01-6184 (SAP/InfraBox#636) for details. - */ \ No newline at end of file diff --git a/src/db/migrations/00060.sql b/src/db/migrations/00060.sql deleted file mode 100644 index fb1a2cfd8..000000000 --- a/src/db/migrations/00060.sql +++ /dev/null @@ -1,13 +0,0 @@ --- Non-contiguous migration fixture. --- --- Numbers 00051..00059 are intentionally missing so this file exercises the --- gap-tolerant selection logic added in the first commit of DM01-6184 --- (SAP/InfraBox#636): get_files() now filters by file number --- (`int(f[:5]) > current_schema_version`) instead of slicing the sorted list --- with the index, so a gap of any size no longer causes later migrations to --- be skipped or re-run. --- --- The statement below is intentionally trivial (no schema change), so --- apply_migration is expected to run it via psycopg2 without side effects --- and still advance schema_version to 60. -SELECT 1; \ No newline at end of file