|
| 1 | +# DBDiff v3.0.0 — Bugs & Issues for Fixing + Regression Tests |
| 2 | + |
| 3 | +Please note, not all of the bugs below may be applicable or valid according to the current codebase (as a lot of them were raised a long time ago) |
| 4 | + |
| 5 | +## Priority Key |
| 6 | +- P1 = Critical: causes crash, invalid SQL output, or data corruption |
| 7 | +- P2 = Medium: incorrect behaviour, edge-case failures, bad output |
| 8 | + |
| 9 | +## Fix Status (branch `feature/bug-fixes-and-prs`) |
| 10 | + |
| 11 | +| # | Status | Commit | Summary | |
| 12 | +|---|--------|--------|---------| |
| 13 | +| 1 | **FIXED** | `b26e981` | Added RuntimeException guard when constraint name is empty in `AlterTableDropConstraintSQL::getUp()`. Test: `tests/Unit/AlterTableDropConstraintSQLTest.php`. | |
| 14 | +| 2 | **FIXED** (combined with #9) | `6a1ddff` | `UpdateDataSQL::getUp()` and `getDown()` now handle `DiffOpAdd`, `DiffOpRemove`, and `DiffOpChange` correctly, with proper null-value guards. Test: `tests/Unit/UpdateDataSQLTest.php`. | |
| 15 | +| 3 | **FIXED** | `3f7f94d` | All cross-DB qualified table references in `LocalTableData` data diff queries now use backtick-escaped `` `db`.`table` `` syntax. Test: `tests/Unit/MySQLDialectQuoteTest.php`. | |
| 16 | +| 4 | **FIXED** | `1ffd937` | `TableIterator::next()` now calls `->toArray()` on the Illuminate Collection and normalises stdClass rows to plain arrays. Test: `tests/Unit/ArrayDiffTest.php`. | |
| 17 | +| 5 | **FIXED** | `820ab11` | `AlterTableEngineSQL::getUp()`/`getDown()` return empty string when engine is null/empty. `TableSchema::getDiff()` requires both engines non-empty before creating a diff entry. Test: `tests/Unit/AlterTableEngineSQLTest.php`. | |
| 18 | +| 6 | **FIXED** | `c5c2b75` | `MySQLAdapter::getTables()` now uses `SHOW FULL TABLES ... WHERE Table_type = 'BASE TABLE'` to exclude views. Postgres and SQLite adapters already filtered correctly. Test: `tests/Unit/ViewsAsTablesSQLTest.php`. | |
| 19 | +| 7 | **FIXED** | `cc63c43` | `LocalTableData` checksum queries use `IFNULL(col, '\0')` inside CAST, plus a NULL-bitmap column (`nullmap`) so `NULL` vs empty-string remain distinguishable. WHERE clause extended to detect nullmap changes. | |
| 20 | +| 8 | **FIXED** | `60465b5` | `InsertDataSQL::getUp()` and `DeleteDataSQL::getDown()` now emit explicit column-name lists: `INSERT INTO t (col1, col2) VALUES(...)`. Test: `tests/Unit/InsertDeleteDataSQLTest.php`. | |
| 21 | +| 9 | **FIXED** (combined with #2) | `6a1ddff` | Null-value guard in `UpdateDataSQL::getDown()` was part of the same fix as Bug #2 — see above. | |
| 22 | +| 10 | **FIXED** | `50b75b3` | Removed both `ini_set('memory_limit', '512M')` calls from `DBDiff::run()` and `DBDiff::getDiffResult()`. Memory limit is now controlled entirely by the user's PHP environment. Test: `tests/Unit/MemoryLimitTest.php`. | |
| 23 | + |
| 24 | +--- |
| 25 | + |
| 26 | +## Original Bug Details |
| 27 | + |
| 28 | +| # | Priority | Connected PR | Title | Affected File(s) | Root Cause (Detailed) | Symptoms / Reproduction | Suggested Fix | Regression Test Type | |
| 29 | +|---|----------|--------------|-------|------------------|-----------------------|-------------------------|---------------|----------------------| |
| 30 | +| 1 | P1 | None | **FK `DROP CONSTRAINT` generates empty constraint name** | `src/SQLGen/DiffToSQL/AlterTableDropConstraintSQL.php`, `src/DB/Adapters/` | `TableSchema::getDiff()` diffs constraints via `MapDiffer::doDiff($targetConstraints, $sourceConstraints)` and passes the diff key as `$name` to `new AlterTableDropConstraint($table, $name, $diff)`. `AlterTableDropConstraintSQL::getUp()` then calls `$this->dialect->quote($this->obj->name)`. If the MySQL adapter returns the constraints array keyed by anything other than the FK constraint name (e.g. numeric index, or an empty string key), `$this->obj->name` is empty/null and `quote('')` produces ` `` ` → `ALTER TABLE \`xxx\` DROP CONSTRAINT \`\`;` — invalid SQL, MySQL error 1064. The `getDown()` side calls `$this->obj->diff->getOldValue()` on a `DiffOpRemove` which is safe, but `getUp()` emitting an empty constraint name makes the migration entirely unexecutable. | Diffing two DBs where a FK exists in one: DOWN migration contains `ALTER TABLE \`xxx\` DROP CONSTRAINT \`\`;` — fails on execution with MySQL error 1064. | 1) Audit all DB adapters (`src/DB/Adapters/`) — confirm `getTableSchema()` returns constraints as `['fk_constraint_name' => 'FOREIGN KEY (\`col\`) REFERENCES \`tbl\`(\`id\`)']` associative maps, not numerically indexed. 2) Add a guard in `AlterTableDropConstraintSQL::getUp()` — throw a descriptive `\RuntimeException` if `$this->obj->name` is empty rather than silently emitting broken SQL. | Unit test: construct `AlterTableDropConstraintSQL` with a known non-empty name; assert `getUp()` = `ALTER TABLE \`t\` DROP CONSTRAINT \`fk_name\`;`. Unit test: assert exception thrown when name is empty. DB integration test: table with FK in source only → diff → assert UP SQL contains the actual FK name. | |
| 31 | +| 2 | P1 | **PR #93** | **`UpdateDataSQL::getDown()` crashes with "Call to undefined method DiffOpAdd::getOldValue()"** | `src/SQLGen/DiffToSQL/UpdateDataSQL.php` | `getDown()` runs `array_walk($values, function (&$diff, $column) { ... $diff->getOldValue() ... })` where `$values = $this->obj->diff['diff']`. The `diff/diff` `MapDiffer` can return `DiffOpAdd` or `DiffOpRemove` (not just `DiffOpChange`) when a column is present only in source or target — e.g. when schema has diverged. `DiffOpAdd` has only `getNewValue()`, not `getOldValue()`. Calling `getOldValue()` on it is a fatal PHP error. `getUp()` already has the correct guard (`is_null($diff->getNewValue()) ? "$q = NULL" : ...`), but `getDown()` has no equivalent check at all. Confirmed in current master `UpdateDataSQL.php`. | Running `--type=data` or `--type=all` when any diffed row has a column that exists only on one side → `PHP Fatal Error: Call to undefined method Diff\DiffOp\DiffOpAdd::getOldValue()`. | In `getDown()`, mirror `getUp()`'s pattern: check `$diff instanceof \Diff\DiffOp\DiffOpAdd` → `"$q = NULL"`, `$diff instanceof \Diff\DiffOp\DiffOpRemove` → `"$q = '" . addslashes($diff->getOldValue()) . "'"`, otherwise `$diff->getOldValue()` with null guard. PR #93 implements a version of this — rebase and verify it handles all three DiffOp types. | Unit test: construct `UpdateDataSQL` with a diff map containing `DiffOpAdd`, `DiffOpRemove`, and `DiffOpChange` entries; assert both `getUp()` and `getDown()` return valid SQL strings without throwing. | |
| 32 | +| 3 | P1 | **PR #92** | **Database names with hyphens not backtick-escaped in data diff SQL queries** | `src/DB/Data/` (iterator/query-building code) | When building the cross-database `LEFT JOIN` query for data diffing, the database name is interpolated directly into SQL without quoting: `FROM kp-small.admin_rule as a LEFT JOIN kp-stage-small.admin_rule as b`. MySQL interprets `-` as subtraction, causing syntax error 1064. The schema diff path is unaffected (uses `SHOW CREATE TABLE` / `INFORMATION_SCHEMA` separately). The cross-DB qualified reference needs to be `` `dbname`.`tablename` `` throughout the data diff queries. The dialect's `quote()` helper is used for table names in `DiffToSQL` classes but is not applied to the DB-qualified table references built in the data iterator. | Any database name with a hyphen (e.g. `my-db`, `kp-stage-small`) → PDO syntax error 1064 on `--type=data` or `--type=all`. Schema-only diff works fine. | Find all raw SQL string construction in `src/DB/Data/` where `$db.$table` is interpolated and replace with `` `$db`.`$table` `` or use `$dialect->quote($db) . '.' . $dialect->quote($table)`. PR #92 does this — rebase onto current master and verify all data iterator query sites are covered. | Unit test: dialect `quote()` on a hyphenated string returns correct backtick-quoted output. DB integration test: run `--type=data` against two MySQL DBs named `test-source` / `test-target`; assert no PDO exception and correct diff output. | |
| 33 | +| 4 | P1 | None | **`ArrayDiff::iterate()` — `array_merge()` called with Illuminate `Collection` instead of `array`** | `src/DB/Data/ArrayDiff.php` | Confirmed in master source: `$data1 = $this->dbiterator1->next(ArrayDiff::$size)` then immediately `$this->sourceBucket = array_merge($this->sourceBucket, $data1)`. The `next()` method returns the result of an Illuminate `take()->get()` call which returns `Illuminate\Support\Collection`, not a plain PHP array. `array_merge()` requires both arguments to be arrays — passing a `Collection` causes `PHP Warning: Invalid argument supplied for foreach()` on the subsequent foreach loops (lines 47, 79, 88). The `Collection` rows are dropped from `$sourceBucket`/`$targetBucket` entirely — **silent data loss**, not a visible crash. Same issue on both `$data1` and `$data2`. | Any `--type=data` or `--type=all` run produces PHP Warnings and silently drops rows from the diff. Real data differences may go undetected. | Change both lines in `iterate()` to call `->toArray()`: `$this->sourceBucket = array_merge($this->sourceBucket, $data1->toArray())` and same for `$data2`. Add a defensive `is_array()` check first for forward-compatibility. | Unit test: mock a `DBIterator` stub whose `next()` returns a `Collection`; call `ArrayDiff::iterate()`; assert `$sourceBucket` is a plain PHP array containing the correct row data. | |
| 34 | +| 5 | P1 | None | **`AlterTableEngineSQL` outputs `ENGINE = ;` when engine value is null/empty** | `src/SQLGen/DiffToSQL/AlterTableEngineSQL.php`, `src/DB/Schema/TableSchema.php` | Confirmed in master: `getUp()` returns `"ALTER TABLE $t ENGINE = {$this->obj->engine};"` and `getDown()` returns `"ALTER TABLE $t ENGINE = {$this->obj->prevEngine};"` with no null/empty guard on either property. `TableSchema::getDiff()` only creates `AlterTableEngine` when `$sourceEngine != $targetEngine`, so if both are null no diff is created — but if one is null/empty and the other is a real value (e.g. views don't have an ENGINE in `SHOW CREATE TABLE`, or adapter returns `''`), the object is created with one empty engine value and `ENGINE = ;` appears in the output — invalid SQL. | Schema diff on tables where engine info is null or missing → invalid SQL `ALTER TABLE \`t\` ENGINE = ;` in output migration. Particularly affects views diffed against tables. | 1) In `TableSchema::getDiff()`, only create `AlterTableEngine` if both `$sourceEngine` and `$targetEngine` are non-empty strings. 2) As a belt-and-braces guard, in `AlterTableEngineSQL::getUp()/getDown()`, return `''` if the engine property is falsy. | Unit test: construct `AlterTableEngineSQL` with `engine = null`; assert `getUp()` returns `''`. Unit test: valid engines `InnoDB`/`MyISAM`; assert correct `ALTER TABLE \`t\` ENGINE = InnoDB;` output. | |
| 35 | +| 6 | P2 | **PR #123** | **Views treated as tables — `DROP TABLE` generated instead of `DROP VIEW`** | `src/DB/Schema/DBSchema.php`, `src/SQLGen/DiffToSQL/DropTableSQL.php`, `src/SQLGen/DiffToSQL/AddTableSQL.php` | `DBSchema::getDiff()` calls `$this->manager->getTables('source')` and `getTables('target')`. If the underlying adapter uses `SHOW TABLES` (without `WHERE Table_type = 'BASE TABLE'`) or otherwise includes views in the result, views enter the diff pipeline as regular tables. `DropTable` and `AddTable` diff objects are created for them, and `DropTableSQL`/`AddTableSQL` generate `DROP TABLE`/`CREATE TABLE` SQL — not `DROP VIEW`/`CREATE VIEW`. Executing the migration fails with a MySQL error because `DROP TABLE` on a view is invalid. `DBSchema.php` currently has no separation between base tables and views. PR #123 includes a "do not try to change views" fix. | Diffing two DBs where one has a view → `DROP TABLE \`my_view\`` in output instead of `DROP VIEW \`my_view\``. Running the migration errors on execution. | 1) Audit all adapters — use `SHOW FULL TABLES WHERE Table_type = 'BASE TABLE'` for MySQL to exclude views from `getTables()`. 2) Add a separate `getViews()` adapter method and handle `DropView`/`AddView` diff objects with correct SQL. 3) PR #123 has a partial fix — rebase, review the view-handling commit specifically, and expand with full view support. | DB integration test: source DB contains a view; target does not; run schema diff; assert output does NOT contain `DROP TABLE` for the view name and either skips it or generates `DROP VIEW`. | |
| 36 | +| 7 | P2 | **PR #77, PR #63** | **NULL column values cause `MD5(CONCAT(...))` to return NULL — data differences silently missed** | `src/DB/Data/` (data iterator / checksum query builder) | MySQL's `CONCAT()` returns `NULL` if **any** argument is `NULL`. The data diff iterator builds a checksum using `MD5(CONCAT(col1, col2, ...))` to fingerprint rows for comparison. Therefore, any row containing even a single NULL column produces `MD5(NULL)` = `NULL`. Two rows with genuinely different non-NULL values but both containing at least one NULL column will both hash to `NULL`, compare as equal, and the real difference is **silently dropped from the diff**. PR #77 (dev-jan) wraps each column in `IFNULL(col, '')` and adds a secondary NULL-bitmap check to distinguish `NULL` from `''`. PR #63 (SergioMBS) independently switches to `CONCAT_WS` which also avoids the NULL collapse. Both need to be evaluated for the cleanest fix. | `--type=data` on tables with nullable columns silently misses data differences where any column in the row is NULL. No error is shown — rows just aren't in the diff output. | Apply `IFNULL(col, '\0')` (use a non-printable sentinel distinct from `''` to differentiate NULL from empty string) inside the concat/checksum, **plus** a NULL-presence bitmask secondary column so `(NULL, 'foo')` and `('', 'foo')` remain distinguishable. Review PR #77's approach and verify the sentinel choice. | Unit test: two row sets where one row has a NULL column that differs → assert diff contains that row. DB integration test: table with nullable column; change NULL→value in source; assert migration contains the correct UPDATE statement. | |
| 37 | +| 8 | P2 | None | **`INSERT` statements generated without column names — breaks on column-order mismatch** | `src/SQLGen/DiffToSQL/InsertDataSQL.php`, `src/SQLGen/DiffToSQL/DeleteDataSQL.php` | `InsertDataSQL::getUp()` builds `INSERT INTO \`table\` VALUES(val1, val2, ...)` with positional values and no column name list. If source and target tables have identical columns but in different order (common after `ALTER TABLE ADD COLUMN` on different servers at different times), positional INSERTs silently insert values into wrong columns — **data corruption with no error**. The fix is always emitting explicit column names: `INSERT INTO \`table\` (\`col1\`, \`col2\`) VALUES(...)` using `array_keys()` of the diff row. Same issue applies to `DeleteDataSQL::getDown()` (which generates the inverse INSERT). | Data diff on tables where column order differs between source and target → wrong values inserted into wrong columns during migration execution. No error — silent data corruption. | Change `InsertDataSQL::getUp()` to build `INSERT INTO $t (` + `implode(', ', array_map([$d, 'quote'], array_keys($row)))` + `) VALUES(...)`. Same change in `DeleteDataSQL::getDown()`. | Unit test: construct `InsertDataSQL` with a known associative row; assert output SQL contains explicit column list matching the row keys in order. Unit test: column order differs from a second table's schema; assert same SQL still names columns explicitly. | |
| 38 | +| 9 | P2 | **PR #93** (partial) | **`UpdateDataSQL::getDown()` does not null-guard `getOldValue()`** | `src/SQLGen/DiffToSQL/UpdateDataSQL.php` | Confirmed in master: `getDown()`'s `array_walk` closure calls `addslashes($diff->getOldValue())` and wraps in single quotes unconditionally, with no `is_null()` check. If `getOldValue()` returns `null` (column previously held NULL), output is `col = ''` instead of `col = NULL` — the rollback migration silently corrupts the row by setting a NULL column to an empty string. `getUp()` already has the correct pattern: `is_null($diff->getNewValue()) ? "$q = NULL" : "$q = '" . addslashes($diff->getNewValue()) . "'"`. PR #93 addresses the wrong-DiffOp-type crash (Bug #2) but the null value guard is a separate issue in the same method — confirm PR #93 covers it, otherwise add separately. | Rolling back a migration where a column changed FROM NULL to a value → rollback SQL sets that column to `''` instead of `NULL`, corrupting the row. | Mirror `getUp()`'s null-safety guard in `getDown()`: `is_null($diff->getOldValue()) ? "$q = NULL" : "$q = '" . addslashes($diff->getOldValue()) . "'"`. | Unit test: `UpdateDataSQL` with `DiffOpChange` where old value is `null`; assert `getDown()` contains `col = NULL` not `col = ''`. | |
| 39 | +| 10 | P2 | None | **`memory_limit` hardcoded to `512M` in source** | `src/DBDiff.php` | `ini_set('memory_limit', '512M')` is called unconditionally at runtime. This overrides both `php.ini` and any CLI `-d memory_limit=...` flag set by the user, because DBDiff re-applies `512M` after PHP has already parsed the CLI option. Users diffing large databases who set a higher limit via environment cannot exceed 512M. Users on shared/restricted hosts where 512M is above their allowed ceiling will get an ini warning. The value should be left entirely to the user's PHP environment. | Large database diffs OOM even when user sets higher `memory_limit` in `php.ini` or via `-d`. | Remove `ini_set('memory_limit', ...)` from source entirely. If a sensible default is desired, expose it as an optional CLI flag `--memory-limit=512M` which calls `ini_set()` only when explicitly passed by the user. | Unit test: instantiate/invoke `DBDiff` and assert `ini_get('memory_limit')` was not forcibly changed (set a known value before construction and assert it is unchanged after). | |
0 commit comments