SQLite Foreign Key Behavior BC Break

[taylorotwell]

BC Break Report

Q	A
BC Break	yes
Version	2.10.3

Summary

A change in SQLite foreign key constraint behavior is a breaking change that was released as a patch release.

See: laravel/framework#34093

[MyIgel]

It looks like the regex in

dbal/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

Lines 126 to 139 in f69c990

	$createSql !== null && preg_match_all(
	'#
	(?:CONSTRAINT\s+([^\s]+)\s+)?
	(?:FOREIGN\s+KEY[^\)]+\)\s*)?
	REFERENCES\s+[^\s]+\s+(?:\([^\)]+\))?
	(?:
	[^,]*?
	(NOT\s+DEFERRABLE|DEFERRABLE)
	(?:\s+INITIALLY\s+(DEFERRED|IMMEDIATE))?
	)?#isx',
	$createSql,
	$match
	)
	) {

does not find what it wants:

$createSql:

CREATE TABLE "users_contact" (
    "user_id" integer not null,
    "dect" varchar null,
    "mobile" varchar null,
    "email" varchar null,
    foreign key("user_id") references "users"("id") on delete cascade on update cascade,
    primary key ("user_id")
)

$match:

array:4 [
  0 => array:1 [
    0 => "foreign key("user_id") references "users"("id") "
  ]
  1 => array:1 [
    0 => ""
  ]
  2 => array:1 [
    0 => ""
  ]
  3 => array:1 [
    0 => ""
  ]
]

[MyIgel]

The problem is that it creates queries like, #4244 contains example code

CONSTRAINT 0 FOREIGN KEY (user_id) REFERENCES users (id) ON UPDATE CASCADE ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE

The 0 value comes from getListTableForeignKeysSQL

dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

Lines 859 to 864 in f69c990

	public function getListTableForeignKeysSQL($table, $database = null)
	{
	$table = str_replace('.', '__', $table);
	return sprintf('PRAGMA foreign_key_list(%s)', $this->quoteStringLiteral($table));
	}

(called in listTableForeignKeys)

dbal/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

Lines 119 to 120 in ce85bd8

	$sql = $this->_platform->getListTableForeignKeysSQL($table, $database);
	$tableForeignKeys = $this->_conn->fetchAllAssociative($sql);

$tableForeignKeys has the following format:

^ array:1 [
  0 => array:8 [
    "id" => "0"
    "seq" => "0"
    "table" => "users"
    "from" => "user_id"
    "to" => "id"
    "on_update" => "CASCADE"
    "on_delete" => "CASCADE"
    "match" => "NONE"
  ]
]

-> the id field is used as a fallback when no CONSTRAINT name is defined:

dbal/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

Line 151 in ce85bd8

'constraint_name' => isset($names[$id]) && $names[$id] !== '' ? $names[$id] : $id,

and it's in that case the 0 which then does not get escaped

[MyIgel]

A solution could be to change it to null as its expected on other parts of the code #4246
or to find somehow a way to escape numbers here but thats imho not intended as there was no name configured previously

[MyIgel]

That one could be #4247 but it somehow does not feel that right as it adds a name where none was before

[MyIgel]

Any thoughts about that @taylorotwell?

[greg0ire]

In the list of changes since previous version, #3762 stands out as related by its title.

EDIT: I see that you found it too in the linked bug report

[MyIgel]

Jea, thats the change that "broke" it aka "enabled the bug" but it was already working "wrong" before it

[greg0ire]

Is the enabled bug also located in doctrine/dbal?

[MyIgel]

It is, yes

The problem is that numbers in reference constraint names are not escaped and/or that "ids" are used as names, depending on how you see it

[greg0ire]

@MyIgel thanks for your investigation, I edited your messages to add more links, please check I did not mess them up 🙏

@beberlei @morozov can you please take a look/recommend a solution?

[greg0ire]

@MyIgel the issues you looked into seem a little "late", meaning they relate to listing foreign keys, but you also reported that "it creates queries like"

CONSTRAINT 0 FOREIGN KEY (user_id) REFERENCES users (id) ON UPDATE CASCADE ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE

That sounds more interesting… is it possible that the issue is with the creation of the foreign key, as opposed to listing a table that has it? I'm not sure creating a foreign key named 0 is actually what is intended here, a stack trace would help understanding how we end with that declaration.

[morozov]

I'm not familiar with this change enough to propose a quick solution. Having a failing test at hand could help to understand the problem and facilitate the solution.

[MyIgel]

The relevant stack traces are

Creating the "interesting" constraint without escaping its name in \Doctrine\DBAL\Platforms\AbstractPlatform::getForeignKeyBaseDeclarationSQL:

#0  Doctrine\DBAL\Platforms\AbstractPlatform->getForeignKeyBaseDeclarationSQL() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php:2522]
#1  Doctrine\DBAL\Platforms\AbstractPlatform->getForeignKeyDeclarationSQL() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php:320]
#2  Doctrine\DBAL\Platforms\SqlitePlatform->getForeignKeyDeclarationSQL() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php:342]
#3  Doctrine\DBAL\Platforms\SqlitePlatform->_getCreateTableSQL() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php:1627]
#4  Doctrine\DBAL\Platforms\AbstractPlatform->getCreateTableSQL() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php:850]
#5  Doctrine\DBAL\Platforms\SqlitePlatform->getCreateTableSQL() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php:983]
#6  Doctrine\DBAL\Platforms\SqlitePlatform->getAlterTableSQL() called at [/var/www/vendor/illuminate/database/Schema/Grammars/ChangeColumn.php:45]
#7  Illuminate\Database\Schema\Grammars\ChangeColumn::compile() called at [/var/www/vendor/illuminate/database/Schema/Grammars/Grammar.php:55]
#8  Illuminate\Database\Schema\Grammars\Grammar->compileChange() called at [/var/www/vendor/illuminate/database/Schema/Blueprint.php:128]
#9  Illuminate\Database\Schema\Blueprint->toSql() called at [/var/www/vendor/illuminate/database/Schema/Blueprint.php:101]
#10 Illuminate\Database\Schema\Blueprint->build() called at [/var/www/vendor/illuminate/database/Schema/Builder.php:290]
#11 Illuminate\Database\Schema\Builder->build() called at [/var/www/vendor/illuminate/database/Schema/Builder.php:151]
#12 Illuminate\Database\Schema\Builder->table() called at [...]

Call of Blueprint $table->string($column, $length)->change(); in a migration while running unit tests

And when getting the foreign keys from the sqlite db in \Doctrine\DBAL\Schema\SqliteSchemaManager::listTableForeignKeys:

#0  Doctrine\DBAL\Schema\SqliteSchemaManager->listTableForeignKeys() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php:276]
#1  Doctrine\DBAL\Schema\AbstractSchemaManager->listTableDetails() called at [/var/www/vendor/doctrine/dbal/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php:554]
#2  Doctrine\DBAL\Schema\SqliteSchemaManager->listTableDetails() called at [/var/www/vendor/illuminate/database/Schema/Grammars/ChangeColumn.php:61]
#3  Illuminate\Database\Schema\Grammars\ChangeColumn::getChangedDiff() called at [/var/www/vendor/illuminate/database/Schema/Grammars/ChangeColumn.php:41]
#4  Illuminate\Database\Schema\Grammars\ChangeColumn::compile() called at [/var/www/vendor/illuminate/database/Schema/Grammars/Grammar.php:55]
#5  Illuminate\Database\Schema\Grammars\Grammar->compileChange() called at [/var/www/vendor/illuminate/database/Schema/Blueprint.php:128]
#6  Illuminate\Database\Schema\Blueprint->toSql() called at [/var/www/vendor/illuminate/database/Schema/Blueprint.php:101]
#7  Illuminate\Database\Schema\Blueprint->build() called at [/var/www/vendor/illuminate/database/Schema/Builder.php:290]
#8  Illuminate\Database\Schema\Builder->build() called at [/var/www/vendor/illuminate/database/Schema/Builder.php:151]
#9  Illuminate\Database\Schema\Builder->table() called at [...]

Call of Blueprint $table->string($column, $length)->change(); in a migration while running unit tests

The important points here:

• The existing sqlite table before the change has a foreign key without a name
• The \Doctrine\DBAL\Schema\SqliteSchemaManager::listTableForeignKeys has a "fallback" for creating a name (when it does not find one after the CONSTRAINT keyword) which uses its "id" which is a number generated by the sqlite database (effectively "adding" one that was never there, for functionality using null here is better as its the value thats expected by other methods)
• The \Doctrine\DBAL\Platforms\AbstractPlatform::getForeignKeyBaseDeclarationSQL uses $foreignKey->getQuotedName($this) but that does not escape numbers which leads us to the above 0 in the query

My two change suggestions try to fix both of the points and i can create PRs for both of them if it helps