Improve foreign key introspection on SQLite

[morozov]

Q	A
Type	improvement, deprecation

With this patch, DBAL can support all foreign key operations except for altering foreign keys on an existing table. IMO, it's still sufficient to declare that the platform supports foreign keys.

This might be considered a minor breaking change (see xkcd #1172) in the cases where code that alters a foreign key based on whether the current platform supports foreign keys. I'd argue that it's a made up/synthetic scenario that primarily applies to testing the DBAL or other libraries that provide an abstraction on top of the DBAL (e.g. the ORM).

In real practical cases, the application would use a subset of the features supported by all target platforms. So it would either not attempt to alter foreign keys because SQLite doesn't support that or it wouldn't support SQLite. In such cases, this change is backward-compatible.

UPD: it still looks like it will cause the same failure in the Laravel test suite that #3762 caused since the test suite relies on AbstractPlatform::supportsForeignKeyConstraints() (see laravel/framework#34093 (comment)).

[greg0ire]

Should #4258 be cherry-picked into this?

[morozov]

Should #4258 be cherry-picked into this?

It looks like it depends on supportsCreateDropForeignKeyConstraints(). I don't believe we should introduce any new supports*() method. This is exactly the reason why such changes may be considered BC breaks.

[greg0ire]

the test suite relies on AbstractPlatform::supportsForeignKeyConstraints()

Maybe that's indirect somehow because git log -S supportsForeignKeyConstraints from the git repo of Laravel yields nothing…

@driesvints hi! 👋 Can you please review this, and maybe try it on the Laravel test suite? So that we can know if something is going to break and give you some time to prevent it?

[morozov]

Maybe that's indirect somehow because git log -S supportsForeignKeyConstraints from the git repo of Laravel yields nothing…

I did the same and also didn't find anything. Having somebody from Laravel take a look would be nice.

[driesvints]

Hi all. I tried these changes out on Laravel 9.x's test suite and everything still passed so I think we're good. Of course there could always be edge cases in apps.

[greg0ire]

Great! Thanks for taking the time to check!

[greg0ire]

@morozov @derrabus I think this breaks the ORM test suite: https://github.com/doctrine/orm/runs/6563292058?check_suite_focus=true 😞

[morozov]

It looks like the ORM always creates foreign keys via the ALTER statements as opposed to creating the table and the foreign keys at once. As a temporary fix, we could replace if supports foreign keys with if supports foreign keys and not sqlite. But it should be possible to fix the ORM property.

[morozov]

I believe I see what's going on here. When it comes to creating a schema where some tables contain foreign keys, neither ORM nor DBAL know in which order they need to be created in order to have the foreign key dependencies satisfied at every step of the migration, so the current implementation does this:

1. There's a flag in AbstractPlatform::getCreateTableSQL():

   dbal/src/Platforms/AbstractPlatform.php

   Line 1928 in 9b5215a

   public function getCreateTableSQL(Table $table, $createFlags = self::CREATE_INDEXES)

   The "indexes" part here is for show, nobody wants to disable creating indexes. The signature should be read as "do not create foreign keys by default". Note that SQLite cannot create foreign keys afterwards and it doesn't support them at all, so $createFlags for SQLite include foreign keys as well:

   dbal/src/Platforms/SqlitePlatform.php

   Lines 884 to 886 in 7df5dd5

   	public function getCreateTableSQL(Table $table, $createFlags = null)
   	{
   	$createFlags = $createFlags ?? self::CREATE_INDEXES | self::CREATE_FOREIGNKEYS;

2. When accepting a visitor, the Table class not only passes itself to the visitor but also passes its columns, indices and foreign keys:

   dbal/src/Schema/Table.php

   Lines 911 to 926 in 83d1f70

   	public function visit(Visitor $visitor)
   	{
   	$visitor->acceptTable($this);
   	foreach ($this->getColumns() as $column) {
   	$visitor->acceptColumn($this, $column);
   	}
   	foreach ($this->getIndexes() as $index) {
   	$visitor->acceptIndex($this, $index);
   	}
   	foreach ($this->getForeignKeys() as $constraint) {
   	$visitor->acceptForeignKey($this, $constraint);
   	}
   	}

   Why? Doesn't the table itself contain them? You should already know the answer. Because we want to create tables and foreign keys separately in CreateSchemaSqlCollector which conveniently doesn't implement acceptColumn() or acceptIndex() but does implement acceptForeignKey().
3. Note that the Schema Manager API doesn't support creating schemas (the createSchema() method doesn't do what you think it does). It's the CreateSchemaSqlCollector visitor which attempts to accept a schema and generate SQL for it.

In order to unscrew this all we need to:

1. Have the collector create tables in the right order in order to be able to create foreign keys right away.
2. Have the collector create tables together with the foreign keys. This would be a breaking change 💣 because then it will create the keys again and fail because they already exist.
3. Have Table not pass its columns, indexes and foreign keys to the visitor explicitly and expect visitors to process them all processing the table itself. This would be a breaking change 💣 again.
4. Remove acceptColumn(), etc from the Visitor interface because they are part of the table. What the heck does accepting a column mean if we don't even know which table it belongs to? Again, breaking change 💣.
5. Drop the support for $createFlags because it should be no longer necessary. Yes, this is also a breaking change 💣.

Alternatively, we could move the logic of creating schemas to the schema manager where it belongs (how would we name the method?). Then we could ditch all this visitor thing in one shot.

[greg0ire]

Thanks for the in-depth analysis @morozov!

Deprecating the collector in favor of a new schema manager method sounds more sensible

how would we name the method?

deploySchema?