Skip to content

Commit a214a9d

Browse files
notSumit25claude
andcommitted
fix: fail closed on DDL the migration risk parser doesn't fully understand
Three classification bugs in DdlStatementParser shared one root cause: branches assumed rather than verified what the parse tree contained. - Multi-statement SQL: parse() kept only the first statement and silently discarded the rest, so a hidden DROP TABLE riding after a harmless ALTER reported SAFE while Postgres executed both. Switch to parseStatements() and require exactly one statement, matching the existing multi-clause guard. - DROP CONSTRAINT shared operation=DROP with DROP COLUMN and was reported as DROP_COLUMN (wrong operation, false "VACUUM reclaims it" reason, and locks missing the referenced table). Gate DROP_COLUMN on getColumnName() actually being set; anything else falls through to UNKNOWN. - ALTER COLUMN ... DROP NOT NULL fell into the same weak ALTER branch as a real type change and was reported as ALTER_COLUMN_TYPE with rewritesTable= true, though it is metadata-only. No existing rule models it correctly (SET_NOT_NULL's rule is specific to adding the constraint), so it now returns UNKNOWN rather than a confident wrong answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c288004 commit a214a9d

2 files changed

Lines changed: 57 additions & 3 deletions

File tree

backend/src/main/java/com/dbaagent/service/migration/DdlStatementParser.java

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,17 @@ public Optional<DdlFacts> parse(String sql) {
4444
null, null, null, null, null, false, notValid, false, null, sql));
4545
}
4646

47-
Statement stmt;
47+
List<Statement> statements;
4848
try {
49-
stmt = CCJSqlParserUtil.parse(normalized);
49+
statements = CCJSqlParserUtil.parseStatements(normalized);
5050
} catch (Exception e) {
5151
return Optional.empty();
5252
}
53+
// Postgres executes every statement in the string, not just the first; a single
54+
// DdlFacts cannot honestly represent more than one, so refuse rather than judge
55+
// the string on its first statement alone.
56+
if (statements.size() != 1) return Optional.empty();
57+
Statement stmt = statements.get(0);
5358

5459
if (stmt instanceof CreateIndex ci) {
5560
return Optional.of(new DdlFacts(DdlOperation.CREATE_INDEX,
@@ -72,7 +77,10 @@ private Optional<DdlFacts> fromAlter(Alter alter, String rawSql, boolean notVali
7277
String table = strip(alter.getTable().getName());
7378
String op = e.getOperation() == null ? "" : e.getOperation().name().toUpperCase(Locale.ROOT);
7479

75-
if ("DROP".equals(op)) {
80+
// DROP CONSTRAINT arrives with the same operation=DROP and a null column name;
81+
// only a genuine DROP COLUMN names a column. A dropped constraint also takes a
82+
// lock on the referenced table, which this parser has no data for — fail closed.
83+
if ("DROP".equals(op) && e.getColumnName() != null) {
7684
return Optional.of(new DdlFacts(DdlOperation.DROP_COLUMN, table, strip(e.getColumnName()),
7785
null, null, null, null, false, notValid, false, null, rawSql));
7886
}
@@ -114,6 +122,9 @@ private Optional<DdlFacts> fromAlter(Alter alter, String rawSql, boolean notVali
114122
strip(cdt.getColumnName()), null, null, null, null, true,
115123
notValid, false, null, rawSql));
116124
}
125+
// "DROP NOT NULL" arrives with colDataType=null, specs=[DROP, NOT, NULL] — it is
126+
// metadata-only, not a type change, but no rule models it correctly; fail closed.
127+
if (cdt.getColDataType() == null) return Optional.empty();
117128
return Optional.of(new DdlFacts(DdlOperation.ALTER_COLUMN_TYPE, table,
118129
strip(cdt.getColumnName()), null, type, null, null, false,
119130
notValid, false, null, rawSql));

backend/src/test/java/com/dbaagent/service/migration/DdlStatementParserTest.java

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,4 +120,47 @@ void renameColumn_capturesNewNameOnly() {
120120
assertThat(f.newColumnName()).isEqualTo("b");
121121
assertThat(f.column()).isNull();
122122
}
123+
124+
// The parser used to keep only the first of several statements, so a hidden DROP TABLE
125+
// riding along after a harmless ALTER reported SAFE. Must fail closed instead.
126+
@Test
127+
void multiStatementSql_returnsEmptySoCallerFailsClosed() {
128+
assertThat(parser.parse("ALTER TABLE t ADD COLUMN a text; DROP TABLE users;")).isEmpty();
129+
}
130+
131+
@Test
132+
void singleStatementWithTrailingSemicolon_stillParses() {
133+
var f = parser.parse("ALTER TABLE orders ADD COLUMN a text;").orElseThrow();
134+
assertThat(f.operation()).isEqualTo(DdlOperation.ADD_COLUMN);
135+
}
136+
137+
// DROP CONSTRAINT shares operation=DROP with DROP COLUMN but names no column; it used to
138+
// be misclassified as DROP_COLUMN with a false "metadata-only, VACUUM reclaims it" reason
139+
// and locks that omitted the referenced table. Must fail closed instead.
140+
@Test
141+
void dropConstraint_returnsEmptySoCallerFailsClosed() {
142+
assertThat(parser.parse("ALTER TABLE pc_child DROP CONSTRAINT fk")).isEmpty();
143+
}
144+
145+
@Test
146+
void dropConstraintCascade_returnsEmptySoCallerFailsClosed() {
147+
assertThat(parser.parse("ALTER TABLE pc_child DROP CONSTRAINT fk CASCADE")).isEmpty();
148+
}
149+
150+
// The DROP CONSTRAINT fix must not break the ordinary DROP COLUMN path it shares an
151+
// operation code with.
152+
@Test
153+
void dropColumnStillClassifiesCorrectly_afterDropConstraintFix() {
154+
var f = parser.parse("ALTER TABLE orders DROP COLUMN c").orElseThrow();
155+
assertThat(f.operation()).isEqualTo(DdlOperation.DROP_COLUMN);
156+
assertThat(f.column()).isEqualTo("c");
157+
}
158+
159+
// DROP NOT NULL used to fall into the same weak ALTER branch as ALTER COLUMN TYPE and
160+
// was misreported as a table-rewriting type change. It is metadata-only, but no existing
161+
// rule models that correctly, so it must report UNKNOWN rather than a wrong DANGER.
162+
@Test
163+
void dropNotNull_returnsEmptySoCallerFailsClosed() {
164+
assertThat(parser.parse("ALTER TABLE orders ALTER COLUMN a DROP NOT NULL")).isEmpty();
165+
}
123166
}

0 commit comments

Comments
 (0)