Skip to content

fix: Key Bug Fixes from DBDiff issues and community PRs#157

Merged
jasdeepkhalsa merged 18 commits intomasterfrom
feature/bug-fixes-and-prs
Mar 26, 2026
Merged

fix: Key Bug Fixes from DBDiff issues and community PRs#157
jasdeepkhalsa merged 18 commits intomasterfrom
feature/bug-fixes-and-prs

Conversation

@jasdeepkhalsa
Copy link
Copy Markdown
Member

@jasdeepkhalsa jasdeepkhalsa commented Mar 25, 2026

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 dedicated InvalidConstraintException (extending BaseException) instead of emitting DROP CONSTRAINT \`. New custom exception class: src/Exceptions/InvalidConstraintException.php`.

Bug 2 & 9 — UpdateDataSQL crashes on DiffOpAdd / emits empty string for NULL

getDown() crashed with a fatal error when the diff contained DiffOpAdd objects (which have no getOldValue()). Also, both directions wrote empty string instead of SQL NULL for null column values.
Closes contributor PR #93.

Bug 3 — Hyphenated database names cause MySQL syntax error 1064

LocalTableData data-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 Illuminate Collection to array_merge()

->get() returns a Collection; it is now converted with ->toArray() and stdClass rows are normalised to associative arrays before being passed to ArrayDiff.

Bug 5 — Null engine value generates ALTER TABLE … ENGINE = ;

AlterTableEngineSQL::getUp() and getDown() 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 TABLE instead of DROP VIEW

MySQLAdapter::getTables() now uses SHOW 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() returns NULL if any argument is NULL, collapsing all such rows to an identical hash. Fix wraps each column in IFNULL(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 columns

InsertDataSQL::getUp() and DeleteDataSQL::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 = 512M via ini_set

Both ini_set('memory_limit', '512M') calls removed from DBDiff::run() and DBDiff::getDiffResult(). The CLI entry points now set a sensible 1G default instead (see below).


Tests

Area What was added
Unit AlterTableDropConstraintSQLTest, UpdateDataSQLTest, MySQLDialectQuoteTest, ArrayDiffTest, AlterTableEngineSQLTest, InsertDataSQLTest, DropTableSQLTest, AddTableSQLTest, MemoryLimitTest
E2E End2EndTest::testHyphenatedDatabaseNames (Bug 3)
Comprehensive AbstractComprehensiveTest::testViewsExcludedFromDiff (Bug 6), ::testNullableColumnDataDetected (Bug 7)
Baselines All PostgreSQL 14–18 and SQLite comprehensive/e2e baselines updated for Bug 8's new column-list INSERT format

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):

  1. --memory-limit=<value> CLI flag (e.g. --memory-limit=2G)
  2. memory_limit: <value> top-level key in .dbdiff / dbdiff.yml
  3. 1G hard default in the entry point scripts

Any PHP shorthand is accepted (512M, 1G, 2G, -1 for unlimited). The ini_set lives only in the CLI entry points — library consumers embedding DBDiff via Composer are unaffected.


SonarQube

  • Trailing whitespace removed from LocalTableData.php L413–414
  • \RuntimeException replaced with dedicated InvalidConstraintException in AlterTableDropConstraintSQL
  • $memory_limit renamed to $memoryLimit in DefaultParams to match camelCase convention

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.
@jasdeepkhalsa jasdeepkhalsa changed the title Feature/bug fixes and prs Fix: Key Bug Fixes from DBDiff issues and community PRs Mar 25, 2026
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.
@jasdeepkhalsa jasdeepkhalsa changed the title Fix: Key Bug Fixes from DBDiff issues and community PRs fix: Key Bug Fixes from DBDiff issues and community PRs Mar 26, 2026
- 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.
@jasdeepkhalsa jasdeepkhalsa force-pushed the feature/bug-fixes-and-prs branch from e6db68e to bd95729 Compare March 26, 2026 00:43
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.
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant