From 095517c3176daaf03db53c402e3c5ea002f88788 Mon Sep 17 00:00:00 2001 From: benkhalife Date: Tue, 4 Aug 2026 00:28:49 -0700 Subject: [PATCH] fix(schema): stop double-emitting foreign keys in Schema::create() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Schema::create() compiled every $table->foreign() twice: once inlined as a column-level CONSTRAINT by compileCreate(), and again as a separate ALTER TABLE ... ADD CONSTRAINT via compileForeignKeys() — no grammar suppresses the second pass. MySQL rejects the duplicate constraint (errno 121) and SQLite rejects the statement outright (ALTER TABLE ADD CONSTRAINT isn't valid SQLite DDL), so any table defined with foreign() could never be created via Schema::create() on either driver. Fix: drop the redundant compileForeignKeys() pass from Schema::create() — the constraints are already present from compileCreate(). This does not affect Schema::table(), which still needs compileForeignKeys() to add constraints to already-existing tables. Adds tests/Integration/SchemaForeignKeyTest.php, reproduced and verified fixed against real MySQL (errno 121) and SQLite (syntax error) connections; full unit + integration suites pass on both drivers. Co-Authored-By: Claude Sonnet 5 --- src/Schema.php | 14 ++- tests/Integration/SchemaForeignKeyTest.php | 129 +++++++++++++++++++++ 2 files changed, 138 insertions(+), 5 deletions(-) create mode 100644 tests/Integration/SchemaForeignKeyTest.php diff --git a/src/Schema.php b/src/Schema.php index c178e82..1e02a43 100644 --- a/src/Schema.php +++ b/src/Schema.php @@ -74,11 +74,15 @@ public static function create(string $table, callable $callback, ?string $connec $conn->statement($idx); } - // Foreign keys already inlined in CREATE TABLE for MySQL. - // For PostgreSQL emit separately if any. - foreach ($grammar->compileForeignKeys($blueprint) as $fk) { - $conn->statement($fk); - } + // Foreign keys are already inlined as column-level CONSTRAINTs by + // compileCreate() above (see SchemaGrammar::compileCreate()), so + // they must NOT be re-emitted here via compileForeignKeys() — doing + // so duplicates every constraint as a separate ALTER TABLE ADD + // CONSTRAINT statement, which MySQL rejects (errno 121, duplicate + // constraint) and SQLite rejects outright (ALTER TABLE ADD + // CONSTRAINT isn't valid SQLite DDL). compileForeignKeys() is still + // used correctly by Schema::table() below, where the table (and + // its inline constraints) already exist. // PostgreSQL column comments (separate COMMENT ON COLUMN statements) if ($grammar instanceof PostgresSchemaGrammar) { diff --git a/tests/Integration/SchemaForeignKeyTest.php b/tests/Integration/SchemaForeignKeyTest.php new file mode 100644 index 0000000..375659a --- /dev/null +++ b/tests/Integration/SchemaForeignKeyTest.php @@ -0,0 +1,129 @@ +foreign()` as a column-level CONSTRAINT, see + * SchemaGrammar::compileCreate()) and then, unconditionally, ALSO runs + * SchemaGrammar::compileForeignKeys() — which re-emits the very same + * constraints as separate `ALTER TABLE ... ADD CONSTRAINT` statements. + * No grammar overrides compileForeignKeys() to suppress this for the + * "already inlined at CREATE TABLE time" case, so every driver executes + * a duplicate constraint statement: + * + * - MySQL: rejects the duplicate constraint name (errno 121, + * "Duplicate key on write or update"). + * - SQLite: has no `ALTER TABLE ... ADD CONSTRAINT` syntax at all, so + * the statement is a hard syntax error. + * + * A table defined with `$table->foreign()` therefore cannot be created + * via Schema::create() on any driver. + */ +class SchemaForeignKeyTest extends IntegrationTestCase +{ + public function testCreateWithForeignKeyDoesNotThrow(): void + { + $parent = 'sfk_test_parents'; + $child = 'sfk_test_children'; + + try { + Schema::create($parent, function (Blueprint $t) { + $t->id(); + $t->string('name')->nullable(); + }); + + Schema::create($child, function (Blueprint $t) use ($parent) { + $t->id(); + $t->bigInteger('parent_id')->unsigned(); + $t->foreign('parent_id')->references('id')->on($parent)->cascadeOnDelete(); + }); + + $this->assertTrue(Schema::hasTable($child)); + } finally { + Schema::dropIfExists($child); + Schema::dropIfExists($parent); + } + } + + /** + * SQLite never enforces foreign keys unless the connection issues + * `PRAGMA foreign_keys = ON` — this library does not do so, which is a + * separate, pre-existing gap unrelated to the double-emission bug this + * file targets. Skipped here rather than silently asserting something + * false for that driver. + */ + public function testCreateWithForeignKeyActuallyEnforcesTheConstraint(): void + { + if (strtolower((string) (getenv('DB_DRIVER') ?: 'sqlite')) === 'sqlite') { + $this->markTestSkipped('SQLite FK enforcement requires PRAGMA foreign_keys=ON, which this library does not set (separate gap).'); + } + + $parent = 'sfk_test_parents2'; + $child = 'sfk_test_children2'; + + try { + Schema::create($parent, function (Blueprint $t) { + $t->id(); + }); + + Schema::create($child, function (Blueprint $t) use ($parent) { + $t->id(); + $t->bigInteger('parent_id')->unsigned(); + $t->foreign('parent_id')->references('id')->on($parent); + }); + + // A reference to a non-existent parent row must be rejected — + // proof the constraint is really enforced by the DB, not just + // that CREATE TABLE happened to succeed some other way. + $threw = false; + try { + \Foxdb\DB::table($child)->insert(['parent_id' => 999999]); + } catch (\Throwable $e) { + $threw = true; + } + $this->assertTrue($threw, 'Expected the foreign key constraint to reject an orphan reference.'); + } finally { + Schema::dropIfExists($child); + Schema::dropIfExists($parent); + } + } + + public function testCreateWithMultipleForeignKeysOnSameTableDoesNotThrow(): void + { + $a = 'sfk_test_a'; + $b = 'sfk_test_b'; + $c = 'sfk_test_c'; + + try { + Schema::create($a, function (Blueprint $t) { + $t->id(); + }); + Schema::create($b, function (Blueprint $t) { + $t->id(); + }); + + Schema::create($c, function (Blueprint $t) use ($a, $b) { + $t->id(); + $t->bigInteger('a_id')->unsigned(); + $t->bigInteger('b_id')->unsigned(); + $t->foreign('a_id')->references('id')->on($a); + $t->foreign('b_id')->references('id')->on($b); + }); + + $this->assertTrue(Schema::hasTable($c)); + } finally { + Schema::dropIfExists($c); + Schema::dropIfExists($b); + Schema::dropIfExists($a); + } + } +}