fix: Key Bug Fixes from DBDiff issues and community PRs#157
Merged
jasdeepkhalsa merged 18 commits intomasterfrom Mar 26, 2026
Merged
fix: Key Bug Fixes from DBDiff issues and community PRs#157jasdeepkhalsa merged 18 commits intomasterfrom
jasdeepkhalsa merged 18 commits intomasterfrom
Conversation
AlterTableDropConstraintSQL::getUp() now throws a RuntimeException when the constraint name is empty/null instead of emitting invalid SQL like ALTER TABLE `t` DROP CONSTRAINT ``; Added unit tests covering: - Valid constraint name generates correct SQL - getDown() restores the constraint via ADD - Empty name throws RuntimeException - Null name throws RuntimeException
…taSQL getDown() previously called getOldValue() unconditionally, which crashes with a fatal error when the diff contains DiffOpAdd objects (which lack that method). Also, getDown() did not null-guard getOldValue(), causing NULL column values to be written as empty strings instead of SQL NULL. Both getUp() and getDown() now: - Check for DiffOpAdd/DiffOpRemove to avoid calling missing methods - Null-guard return values to emit col = NULL instead of col = '' Added comprehensive unit tests covering DiffOpChange, DiffOpAdd, DiffOpRemove, null values, and mixed DiffOp types in a single UPDATE. Related contributor PR: #93
MySQL data diff queries in LocalTableData now use backtick-quoted `db`.`table` references instead of bare db.table. This prevents MySQL from interpreting hyphens in database names (e.g. 'my-db') as subtraction operators, which caused PDO syntax error 1064. Fixed in getOldNewDiff() (2 queries) and getChangeDiff() (1 query + key column references). Added unit test verifying MySQLDialect::quote() correctly handles hyphenated names, names with backticks, and names with spaces. Related contributor PR: #92
TableIterator::next() now calls ->toArray() on the Illuminate Collection returned by ->get(), and normalises stdClass rows to associative arrays. This prevents array_merge() warnings and silent data loss in ArrayDiff when the Collection was incorrectly passed as-is. Added unit tests for ArrayDiff verifying: - Identical data produces no diff - New rows are detected as DiffOpAdd - Deleted rows are detected as DiffOpRemove - Changed rows are detected with correct diff keys
AlterTableEngineSQL::getUp() and getDown() now return empty string when the engine or prevEngine value is empty/null, preventing invalid SQL like 'ALTER TABLE `t` ENGINE = ;'. Also hardened TableSchema::getDiff() to skip engine diff creation when either engine value is empty (e.g. views that have no ENGINE). Added unit tests for valid engines, null engines, and empty string engines.
MySQLAdapter::getTables() now uses SHOW FULL TABLES WHERE Table_type = 'BASE TABLE' instead of SHOW TABLES, preventing views from entering the diff pipeline and being incorrectly handled as tables (which would generate DROP TABLE instead of DROP VIEW). PostgreSQL and SQLite adapters were already correct — pg_tables only includes base tables and SQLite filters type = 'table'. Added unit tests for DropTableSQL and AddTableSQL output verification. Related contributor PR: #123
MySQL CONCAT() returns NULL if any argument is NULL, causing SHA2(CONCAT(...)) to produce NULL for any row with nullable columns. Two genuinely different rows both hash to NULL and compare as equal — silently dropping real data differences from the diff output. Fix: wrap each column in IFNULL(col, '\0') inside the CAST before concatenation, and add a NULL-presence bitmap (IF(col IS NULL, '1', '0')) as a secondary comparison column. The WHERE clause now checks both hash1 <> hash2 OR nullmap1 <> nullmap2, so: - NULL vs empty string are distinguished (different bitmaps) - NULL vs non-NULL values are caught (different hashes + bitmaps) - Regular value changes continue to work via hash comparison Related contributor PRs: #77, #63
InsertDataSQL::getUp() and DeleteDataSQL::getDown() now emit explicit column name lists: INSERT INTO `t` (`col1`,`col2`) VALUES(...) instead of positional INSERT INTO `t` VALUES(...). This prevents silent data corruption when source and target tables have identical columns in different order (common after ALTER TABLE ADD on different servers). Also handles NULL values correctly in the column list. Added unit tests covering: - Column names included in INSERT output - NULL value handling in INSERT - DELETE UP/DOWN direction correctness
Remove both ini_set('memory_limit', '512M') calls from DBDiff::run()
and DBDiff::getDiffResult(). The memory limit should be controlled by
the user's php.ini or CLI -d flag, not overridden by the library.
Add MemoryLimitTest to verify the source no longer contains
ini_set('memory_limit', ...) and that autoloading DBDiff does not
change the runtime memory_limit.
Add a Fix Status summary table at the top of bugs.md documenting: - Commit hash for each bug fix - Brief description of what was changed - Test file references - Note that bugs 2 and 9 were combined (same file/method) Original bug detail table preserved below for reference.
Bug 8 changed INSERT statements from positional VALUES to explicit
column lists:
Before: INSERT INTO "posts" VALUES('3','2',...)
After: INSERT INTO "posts" ("id","user_id",...) VALUES('3','2',...)
Update all affected PostgreSQL (14-18) comprehensive test baselines
and the end2end baselines for both PostgreSQL and SQLite to match
the new correct output. MySQL baselines were unaffected.
Bug 3 — Hyphenated database names (End2EndTest::testHyphenatedDatabaseNames) Runs the full MySQL end-to-end diff with databases named 'diff-1'/'diff-2'. Before the fix, unquoted hyphens caused MySQL error 1064. The test asserts that DBDiff completes, writes output, and that output matches the standard baseline (same schema/data, only the DB name differs). Bug 6 — Views excluded from diff (AbstractComprehensiveTest::testViewsExcludedFromDiff) New fixture 'views_ignored': source DB has a TABLE 'products' + VIEW 'active_products'; target DB has only the table with an extra column. Test asserts 'active_products' never appears in the diff output, while the real schema difference (price column) is still detected. Fixture files added for MySQL, PostgreSQL, and SQLite. Bug 7 — NULL column changes detected (AbstractComprehensiveTest::testNullableColumnDataDetected) New fixture 'nullable_data': rows 1 and 2 have NULL<->value changes in a nullable column; row 3 has NULL in both source and target (must be ignored). Test asserts UPDATE statements are generated and row 3 is absent. Fixture files added for MySQL, PostgreSQL, and SQLite.
- Add InvalidConstraintException extending BaseException for the empty constraint name guard in AlterTableDropConstraintSQL::getUp() - Remove trailing whitespace from LocalTableData.php SQL lines (FROM / INNER JOIN) - Update AlterTableDropConstraintSQLTest to expect InvalidConstraintException
The CLI entry points (dbdiff, dbdiff.php / PHAR) now set a default PHP memory limit of 1G on startup. PHP's built-in default of 128M is too low for diffing realistic database sizes. The limit is fully configurable at three levels (highest wins): 1. --memory-limit=<value> CLI flag (e.g. --memory-limit=2G) 2. memory_limit: <value> top-level key in .dbdiff / dbdiff.yml 3. 1G default in the entry point scripts Any PHP shorthand is accepted (512M, 1G, 2G, -1 for unlimited). The ini_set is placed in the CLI entry points only — not in the library — so programmatic consumers (e.g. embedding DBDiff as a Composer dependency) are never affected. For native binary users (@dbdiff/cli npm package) ini_set has no effect, and they are unaffected.
e6db68e to
bd95729
Compare
Bug 8 changed InsertDataSQL::getUp() and DeleteDataSQL::getDown() to emit explicit column-name lists. Update all 7 SQLite comprehensive test baselines to match the new format. Also fix createTestConfig() to create tests/config/ if absent, and add a .gitkeep so the directory is tracked in git. Without the directory the config- file-based test cases silently skipped, leaving their baselines stale.
|
This was referenced Apr 2, 2026
Closed
This was referenced Apr 2, 2026
Closed
Closed
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.



Fix 10 bugs — data integrity, SQL generation, and robustness
This PR fixes 10 bugs across the SQL generator, data diff pipeline, and core runtime. Each fix has independent unit tests and, where applicable, dedicated end-to-end scenarios.
Changes
Bug 1 — Empty constraint name generates invalid SQL
AlterTableDropConstraintSQL::getUp()now throws a dedicatedInvalidConstraintException(extendingBaseException) instead of emittingDROP CONSTRAINT \`. New custom exception class:src/Exceptions/InvalidConstraintException.php`.Bug 2 & 9 —
UpdateDataSQLcrashes onDiffOpAdd/ emits empty string for NULLgetDown()crashed with a fatal error when the diff containedDiffOpAddobjects (which have nogetOldValue()). Also, both directions wrote empty string instead of SQLNULLfor null column values.Closes contributor PR #93.
Bug 3 — Hyphenated database names cause MySQL syntax error 1064
LocalTableDatadata-diff queries now use backtick-quoted`db`.`table`references. MySQL was interpreting hyphens as subtraction operators.Closes contributor PR #92.
Bug 4 —
TableIterator::next()passes IlluminateCollectiontoarray_merge()->get()returns aCollection; it is now converted with->toArray()andstdClassrows are normalised to associative arrays before being passed toArrayDiff.Bug 5 — Null engine value generates
ALTER TABLE … ENGINE = ;AlterTableEngineSQL::getUp()andgetDown()return empty string when either engine value is empty/null.TableSchema::getDiff()also skips engine diff creation in this case.Bug 6 — Views appear in table diff and generate
DROP TABLEinstead ofDROP VIEWMySQLAdapter::getTables()now usesSHOW FULL TABLES WHERE Table_type = 'BASE TABLE'. PostgreSQL and SQLite adapters were already correct.Closes contributor PR #123.
Bug 7 — Rows with NULL columns are silently dropped from data diff
MySQL
CONCAT()returnsNULLif any argument isNULL, collapsing all such rows to an identical hash. Fix wraps each column inIFNULL(col, '\0')and adds a NULL-presence bitmap as a secondary comparison field.Closes contributor PRs #77 and #63.
Bug 8 — INSERT statements use positional
VALUES(...)instead of named columnsInsertDataSQL::getUp()andDeleteDataSQL::getDown()now emit explicit column lists:INSERT INTO `t` (`col1`,`col2`) VALUES(...). This prevents silent data corruption when column order differs between source and target.Bug 10 — Library hardcodes
memory_limit = 512Mviaini_setBoth
ini_set('memory_limit', '512M')calls removed fromDBDiff::run()andDBDiff::getDiffResult(). The CLI entry points now set a sensible 1G default instead (see below).Tests
AlterTableDropConstraintSQLTest,UpdateDataSQLTest,MySQLDialectQuoteTest,ArrayDiffTest,AlterTableEngineSQLTest,InsertDataSQLTest,DropTableSQLTest,AddTableSQLTest,MemoryLimitTestEnd2EndTest::testHyphenatedDatabaseNames(Bug 3)AbstractComprehensiveTest::testViewsExcludedFromDiff(Bug 6),::testNullableColumnDataDetected(Bug 7)Configurable memory limit
The CLI entry points (
dbdiff,dbdiff.php, PHAR) now set a default PHP memory limit of 1G on startup. PHP's built-in default of 128M is too low for real-world database sizes. The limit is fully configurable at three levels (highest wins):--memory-limit=<value>CLI flag (e.g.--memory-limit=2G)memory_limit: <value>top-level key in.dbdiff/dbdiff.yml1Ghard default in the entry point scriptsAny PHP shorthand is accepted (
512M,1G,2G,-1for unlimited). Theini_setlives only in the CLI entry points — library consumers embedding DBDiff via Composer are unaffected.SonarQube
LocalTableData.phpL413–414\RuntimeExceptionreplaced with dedicatedInvalidConstraintExceptioninAlterTableDropConstraintSQL$memory_limitrenamed to$memoryLimitinDefaultParamsto match camelCase convention