dbal 🚀 - SQLite Foreign Key Behavior BC Break

It looks like the regex in https://github.com/doctrine/dbal/blob/f69c990e359931753232a81c3a1821a7ea060bfd/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php#L126-L139 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 on 2 Sep 2020

🚀1

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

https://github.com/doctrine/dbal/blob/f69c990e359931753232a81c3a1821a7ea060bfd/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php#L859-L864

(called in listTableForeignKeys)

https://github.com/doctrine/dbal/blob/ce85bd8719b18b61c3171f3324df281c7716aec4/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php#L119-L120

$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: https://github.com/doctrine/dbal/blob/ce85bd8719b18b61c3171f3324df281c7716aec4/lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php#L151
and it's in that case the 0 which then does not get escaped

MyIgel on 2 Sep 2020

A solution could be to change it to null as its expected on other parts of the code https://github.com/doctrine/dbal/pull/4246
or to find somehow a way to escape numbers here but thats imho not intended as there was no name configured previously

MyIgel on 2 Sep 2020

That one could be https://github.com/doctrine/dbal/pull/4247 but it somehow does not feel that right as it adds a name where none was before

MyIgel on 2 Sep 2020

Any thoughts about that @taylorotwell?

MyIgel on 2 Sep 2020

In the list of changes since previous version, https://github.com/doctrine/dbal/pull/3762 stands out as related by its title.

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

greg0ire on 2 Sep 2020

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

MyIgel on 2 Sep 2020

Is the enabled bug also located in doctrine/dbal?

greg0ire on 2 Sep 2020

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

MyIgel on 2 Sep 2020

👍1

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

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

greg0ire on 2 Sep 2020

👍1

@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.

greg0ire on 2 Sep 2020

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.

morozov on 2 Sep 2020

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

MyIgel on 2 Sep 2020

👍1

Ok, I understand far better now, thanks a lot! Please do send the PRs for your suggested changes 👍

greg0ire on 2 Sep 2020

And here they are, also updated the links above

MyIgel on 2 Sep 2020

👍1

The existing sqlite table before the change has a foreign key without a name

Are newly-created FKs created without a name too?

greg0ire on 2 Sep 2020

Are newly-created FKs created without a name too?

As these are unit test migrations that run before every test i would say no, they have no name

MyIgel on 2 Sep 2020

Hi here is my call stack. I created main issue on laravel repository (https://github.com/laravel/framework/issues/34093) and after re-checking my fresh installation, found that is not relevant to this problem and removed that part from main issue. I also added migration file that can be seen in call stack.

 SQLSTATE[HY000]: General error: 1 near "0": syntax error (SQL: CREATE TABLE tasks (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, account_id INTEGER NOT NULL, user_id INTEGER DEFAULT NULL, subject VARCHAR(255) NOT NULL COLLATE BINARY, start_date DATETIME NOT NULL, state INTEGER DEFAULT 0 NOT NULL, task_type INTEGER DEFAULT 0 NOT NULL, task_queue_id INTEGER NOT NULL, created_by_type VARCHAR(255) DEFAULT NULL COLLATE BINARY, created_by_id INTEGER DEFAULT NULL, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, content CLOB DEFAULT NULL COLLATE BINARY, end_date DATETIME DEFAULT NULL, CONSTRAINT 0 FOREIGN KEY (user_id) REFERENCES users (id) ON UPDATE NO ACTION ON DELETE SET NULL NOT DEFERRABLE INITIALLY IMMEDIATE, CONSTRAINT 1 FOREIGN KEY (account_id) REFERENCES accounts (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE))

  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:671
    667|         // If an exception occurs when attempting to run a query, we'll format the error
    668|         // message to include the bindings with SQL, which will make this exception a
    669|         // lot more helpful to the developer instead of just the database's errors.
    670|         catch (Exception $e) {
  > 671|             throw new QueryException(
    672|                 $query, $this->prepareBindings($bindings), $e
    673|             );
    674|         }
    675| 

  1   vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:72
      Doctrine\DBAL\Driver\PDOException::("SQLSTATE[HY000]: General error: 1 near "0": syntax error")

  2   vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:67
      PDOException::("SQLSTATE[HY000]: General error: 1 near "0": syntax error")

  3   vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:67
      PDO::prepare()

  4   vendor/laravel/framework/src/Illuminate/Database/Connection.php:458
      Doctrine\DBAL\Driver\PDOConnection::prepare()

  5   vendor/laravel/framework/src/Illuminate/Database/Connection.php:664
      Illuminate\Database\Connection::Illuminate\Database\{closure}()

  6   vendor/laravel/framework/src/Illuminate/Database/Connection.php:631
      Illuminate\Database\Connection::runQueryCallback()

  7   vendor/laravel/framework/src/Illuminate/Database/Connection.php:465
      Illuminate\Database\Connection::run()

  8   vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:102
      Illuminate\Database\Connection::statement()

  9   vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:290
      Illuminate\Database\Schema\Blueprint::build()

  10  vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:151
      Illuminate\Database\Schema\Builder::build()

  11  vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:261
      Illuminate\Database\Schema\Builder::table()

  12  database/migrations/2020_05_04_085002_add_nullable_to_some_fields_in_tasks_table.php:19
      Illuminate\Support\Facades\Facade::__callStatic()

  13  vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:392
      AddNullableToSomeFieldsInTasksTable::up()

  14  vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:401
      Illuminate\Database\Migrations\Migrator::Illuminate\Database\Migrations\{closure}()

  15  vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:200
      Illuminate\Database\Migrations\Migrator::runMigration()

  16  vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:165
      Illuminate\Database\Migrations\Migrator::runUp()

  17  vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:110
      Illuminate\Database\Migrations\Migrator::runPending()

  18  vendor/laravel/framework/src/Illuminate/Database/Console/Migrations/MigrateCommand.php:72
      Illuminate\Database\Migrations\Migrator::run()

  19  vendor/laravel/framework/src/Illuminate/Database/Migrations/Migrator.php:541
      Illuminate\Database\Console\Migrations\MigrateCommand::Illuminate\Database\Console\Migrations\{closure}()

  20  vendor/laravel/framework/src/Illuminate/Database/Console/Migrations/MigrateCommand.php:81
      Illuminate\Database\Migrations\Migrator::usingConnection()

  21  vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:37
      Illuminate\Database\Console\Migrations\MigrateCommand::handle()

  22  vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:37
      call_user_func_array()

  23  vendor/laravel/framework/src/Illuminate/Container/Util.php:37
      Illuminate\Container\BoundMethod::Illuminate\Container\{closure}()

  24  vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:95
      Illuminate\Container\Util::unwrapIfClosure()

  25  vendor/laravel/framework/src/Illuminate/Container/BoundMethod.php:39
      Illuminate\Container\BoundMethod::callBoundMethod()

  26  vendor/laravel/framework/src/Illuminate/Container/Container.php:596
      Illuminate\Container\BoundMethod::call()

  27  vendor/laravel/framework/src/Illuminate/Console/Command.php:134
      Illuminate\Container\Container::call()

  28  vendor/symfony/console/Command/Command.php:258
      Illuminate\Console\Command::execute()

  29  vendor/laravel/framework/src/Illuminate/Console/Command.php:121
      Symfony\Component\Console\Command\Command::run()

  30  vendor/symfony/console/Application.php:916
      Illuminate\Console\Command::run()

  31  vendor/symfony/console/Application.php:264
      Symfony\Component\Console\Application::doRunCommand()

  32  vendor/symfony/console/Application.php:140
      Symfony\Component\Console\Application::doRun()

  33  vendor/laravel/framework/src/Illuminate/Console/Application.php:93
      Symfony\Component\Console\Application::run()

  34  vendor/laravel/framework/src/Illuminate/Console/Application.php:185
      Illuminate\Console\Application::run()

  35  vendor/laravel/framework/src/Illuminate/Foundation/Console/Kernel.php:263
      Illuminate\Console\Application::call()

  36  vendor/laravel/framework/src/Illuminate/Testing/PendingCommand.php:171
      Illuminate\Foundation\Console\Kernel::call()

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

class AddNullableToSomeFieldsInTasksTable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::table('tasks', function (Blueprint $table) {
            $table->text('content')->nullable()->change();
            $table->dateTime('end_date')->nullable()->change();
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::table('tasks', function (Blueprint $table) {
            $table->text('content')->change();
            $table->dateTime('end_date')->change();
        });
    }
}

hakanersu on 3 Sep 2020

👍2

Thanks for the stack trace! It shows that Laravel is calling Doctrine with an SQL query as string: https://github.com/laravel/framework/blob/caec40ed2695c2aa84b5eb195c822e3a961fc445/src/Illuminate/Database/Connection.php#L458

Where is that query built? Is it built by Blueprint, and built differently because Doctrine says SQlite supports foreign keys?

greg0ire on 3 Sep 2020

Regarding General error: 1 no such index: IDX_DC8C84A116FE72E1 (SQL: DROP INDEX IDX_DC8C84A116FE72E1) (also reported here), I created a small example: https://github.com/uuf6429/doc-sqlite-poc (first run composer install and then run the test and it should fail with a similar error).

uuf6429 on 3 Sep 2020

👍3

For me the create table migration is not failing, but what is failing is the migration that alters table, for some reason it tries to drop an index that was not created or at least drop it with wrong name maybe? ...

The create table

CREATE TABLE "items" ("id" integer not null primary key autoincrement, "package_id" integer null, "sort" integer not null default '0', "title" varchar null, "sub_title" varchar null, "center_text" varchar null, "background_image_id" integer null, "created_by" integer null, "updated_by" integer null, "created_at" datetime null, "updated_at" datetime null, foreign key("package_id") references "packages"("id"), foreign key("background_image_id") references "images"("id"), foreign key("created_by") references "users"("id"), foreign key("updated_by") references "users"("id")

Then here the failing migration (alter table by deleting center_text column)

   Illuminate\Database\QueryException 
  SQLSTATE[HY000]: General error: 1 no such index: IDX_DC8C84A116FE72E1 (SQL: DROP INDEX IDX_DC8C84A116FE72E1)

  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:671
    667|         // If an exception occurs when attempting to run a query, we'll format the error
    668|         // message to include the bindings with SQL, which will make this exception a
    669|         // lot more helpful to the developer instead of just the database's errors.
    670|         catch (Exception $e) {
  > 671|             throw new QueryException(
    672|                 $query, $this->prepareBindings($bindings), $e
    673|             );
    674|         }
    675|

I traced it down to this behaviour, but I'm not familiar really with dbal internals

-> Method getPreAlterTableIndexForeignKeySQL() -> SqllitePlatform.php (Doctrine\DBAL\Platforms\SqlitePlatform)
Gets called with different TableDiff than before

The diff $diff->fromTable->getIndexes() in new version has indexes it didn't have in 2.10.2

dumping $diff->fromTable->getIndexes() for 2.10.3
contains

array:5 [
  "primary" => Doctrine\DBAL\Schema\Index^ {#2803
    #_columns: array:1 [
      "id" => Doctrine\DBAL\Schema\Identifier^ {#2804
        #_name: "id"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_isUnique: true
    #_isPrimary: true
    #_flags: []
    -options: array:1 [
      "lengths" => array:1 [
        0 => null
      ]
    ]
    #_name: "primary"
    #_namespace: null
    #_quoted: false
  }
  "idx_dc8c84a116fe72e1" => Doctrine\DBAL\Schema\Index^ {#2805
    #_columns: array:1 [
      "updated_by" => Doctrine\DBAL\Schema\Identifier^ {#2806
        #_name: "updated_by"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_isUnique: false
    #_isPrimary: false
    #_flags: []
    -options: []
    #_name: "IDX_DC8C84A116FE72E1"
    #_namespace: null
    #_quoted: false
  }
  "idx_dc8c84a1de12ab56" => Doctrine\DBAL\Schema\Index^ {#2807
    #_columns: array:1 [
      "created_by" => Doctrine\DBAL\Schema\Identifier^ {#2808
        #_name: "created_by"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_isUnique: false
    #_isPrimary: false
    #_flags: []
    -options: []
    #_name: "IDX_DC8C84A1DE12AB56"
    #_namespace: null
    #_quoted: false
  }
  "idx_dc8c84a1e6da28aa" => Doctrine\DBAL\Schema\Index^ {#2809
    #_columns: array:1 [
      "background_image_id" => Doctrine\DBAL\Schema\Identifier^ {#2810
        #_name: "background_image_id"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_isUnique: false
    #_isPrimary: false
    #_flags: []
    -options: []
    #_name: "IDX_DC8C84A1E6DA28AA"
    #_namespace: null
    #_quoted: false
  }
  "idx_dc8c84a1445b8adf" => Doctrine\DBAL\Schema\Index^ {#2811
    #_columns: array:1 [
      "package_id" => Doctrine\DBAL\Schema\Identifier^ {#2812
        #_name: "package_id"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_isUnique: false
    #_isPrimary: false
    #_flags: []
    -options: []
    #_name: "IDX_DC8C84A1445B8ADF"
    #_namespace: null
    #_quoted: false
  }
]

dumping $diff->fromTable->getIndexes() for 2.10.2
it's just the primary key

array:1 [
  "primary" => Doctrine\DBAL\Schema\Index^ {#2787
    #_columns: array:1 [
      "id" => Doctrine\DBAL\Schema\Identifier^ {#2788
        #_name: "id"
        #_namespace: null
        #_quoted: false
      }
    ]
    #_isUnique: true
    #_isPrimary: true
    #_flags: []
    -options: array:1 [
      "lengths" => array:1 [
        0 => null
      ]
    ]
    #_name: "primary"
    #_namespace: null
    #_quoted: false
  }
]

Maybe the problem (newly enabled bug) is with getPreAlterTableIndexForeignKeySQL as for all other platforms it checks first what was the removed indexes from diff, and then drop them, for Sqllite instead it drops all indexes now?
But this also doesn't answer why is it trying to drop by a name that was not used to create the index...

SirNarsh on 3 Sep 2020

👍1

Where is that query built? Is it built by Blueprint, and built differently because Doctrine says SQlite supports foreign keys?

It looks like this issue is only affecting Laravel users… how come? Is maybe Laravel not doing something other client libraries are doing? Like maybe not picking a (proper) name for foreign keys in Blueprint?

greg0ire on 3 Sep 2020

@uuf6429 the reproducer works great, thanks! I pinpointed the origin of that weirdly named index here: https://github.com/doctrine/dbal/blob/f69c990e359931753232a81c3a1821a7ea060bfd/lib/Doctrine/DBAL/Schema/Table.php#L557-L577

That was done in bc9c8c2b139ebee2e94e94ca5b572ee921b336fa 10 years ago

@taylorotwell, you said on the Laravel issue

Here is the actual SQL created when I create a table with a foreign key using Laravel:

Am I to understand that the normal flow for creating tables in Laravel does not involve doctrine/dbal at all, but that it is involved for migrations only? That would explain why only Laravel users seem to be hit by this bug, if doctrine/dbal makes the (wrong) assumption that these implicit indices are present in the original table as soon as there is a foreign key.

Things to clarify:

the purpose of a so-called implicit index, aren't foreign keys enough?
why the first comment in the code above is about an _explicit_ index, is that a different thing? Similarly, why does "Explicitly" appear in the commit message of bc9c8c2b139ebee2e94e94ca5b572ee921b336fa?

Possible fix: drop the index only if it exists.

To be discussed: should the implicit index be created if it does not exist?

greg0ire on 3 Sep 2020

👍3

@greg0ire is there more work on this issue required besides #4246? If so, what reproducer worked for you so I could try reproducing the issue as well?

morozov on 4 Sep 2020

👍1

is there more work on this issue required

@morozov I believe so yes, as pointed out in https://github.com/laravel/framework/issues/34093#issuecomment-686507332, I think there are 2 issues with 2.10.3

The reproducer can be found here: https://github.com/doctrine/dbal/issues/4243#issuecomment-686340932

After cloning, and installing, I ran vendor/bin/phpunit tests/DatabaseTest.php to spot the issue.

greg0ire on 4 Sep 2020

@greg0ire no, we do not use doctrine/dbal to create tables. We only use doctrine/dbal to change column types / attributes.

taylorotwell on 4 Sep 2020

👍1

About implicit indexes: some databases, but not all, will automatically create indexes when you create a foreign key and

They need to be represented in schema so that dbal does not create a second redundant index. — @beberlei

SQLite seems to be part of the group that _does not_ create such indices. At least that's what I understand when reading this section of the official docs

Indices are not required for child key columns but they are almost always beneficial.

and

So, in most real systems, an index should be created on the child key columns of each foreign key constraint. The child key index does not have to be (and usually will not be) a UNIQUE index

I suppose dbal elected to automatically implement such indices for SQLite (in which case it's an explicit index), but is able to avoid doing so in the case of MySQL or MariaDB where creation happens automatically (that would be an implicit index?),

https://github.com/doctrine/dbal/blob/f69c990e359931753232a81c3a1821a7ea060bfd/lib/Doctrine/DBAL/Schema/Table.php#L570-L574

Apparently Laravel didn't implement these recommended indices and that's why we have this bug. This raises another question: do Laravel users experience the bug for SQLite only, or does it happen for Postgres and Sql Server too? I think if it did, we would know, wouldn't we? So something is off…

If what I said is true then there might be a naming issue Table::implicitIndexes should in fact have be called Table::explicitIndexes, which would be consistent with

https://github.com/doctrine/dbal/blob/f69c990e359931753232a81c3a1821a7ea060bfd/lib/Doctrine/DBAL/Schema/Table.php#L557

But maybe they were named implicitIndexes because the DBAL adds them for you even if you don't ask it to? That would be confusing because that terminology seems to be used in 2 of the pages I linked (about sql server and about postgres) to refer to indices implicitely created by those systems.

@morozov when this is over it will definitely warrant an explanation

greg0ire on 4 Sep 2020

Sad news, with the reproducer, replacing DROP INDEX with DROP INDEX IF EXISTS leads to

Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 number of columns in foreign key does not match the number of columns in the referenced table (SQL: CREATE TABLE documents (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, type_id INTEGER NOT NULL, group_id INTEGER DEFAULT NULL, hash VARCHAR(255) NOT NULL COLLATE BINARY, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, title VARCHAR(255) NOT NULL COLLATE BINARY, deleted_at DATETIME DEFAULT NULL, ek VARCHAR(50) DEFAULT NULL COLLATE BINARY, iv VARCHAR(50) DEFAULT NULL COLLATE BINARY, FOREIGN KEY (group_id, type_id) REFERENCES groups (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE))

```sql
CREATE TABLE documents (
id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
type_id INTEGER NOT NULL,
group_id INTEGER DEFAULT NULL,
hash VARCHAR(255) NOT NULL COLLATE BINARY,
created_at DATETIME DEFAULT NULL,
updated_at DATETIME DEFAULT NULL,
title VARCHAR(255) NOT NULL COLLATE BINARY,
deleted_at DATETIME DEFAULT NULL,
ek VARCHAR(50) DEFAULT NULL COLLATE BINARY,
iv VARCHAR(50) DEFAULT NULL COLLATE BINARY,
FOREIGN KEY (group_id, type_id) REFERENCES groups (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE)

So there is probably another bug to fix after this one :sweat: 

If I revert your PR @MyIgel , this becomes

Illuminate\Database\QueryException: SQLSTATE[HY000]: General error: 1 near "0": syntax error (SQL: CREATE TABLE documents (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, type_id INTEGER NOT NULL, group_id INTEGER DEFAULT NULL, hash VARCHAR(255) NOT NULL COLLATE BINARY, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, title VARCHAR(255) NOT NULL COLLATE BINARY, deleted_at DATETIME DEFAULT NULL, ek VARCHAR(50) DEFAULT NULL COLLATE BINARY, iv VARCHAR(50) DEFAULT NULL COLLATE BINARY, CONSTRAINT 0 FOREIGN KEY (group_id) REFERENCES groups (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE, CONSTRAINT 1 FOREIGN KEY (type_id) REFERENCES types (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE))

```sql
CREATE TABLE documents (
    id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL,
    type_id INTEGER NOT NULL,
    group_id INTEGER DEFAULT NULL,
    hash VARCHAR(255) NOT NULL COLLATE BINARY,
    created_at DATETIME DEFAULT NULL,
    updated_at DATETIME DEFAULT NULL,
    title VARCHAR(255) NOT NULL COLLATE BINARY,
    deleted_at DATETIME DEFAULT NULL,
    ek VARCHAR(50) DEFAULT NULL COLLATE BINARY,
    iv VARCHAR(50) DEFAULT NULL COLLATE BINARY,
    CONSTRAINT 0 FOREIGN KEY (group_id) REFERENCES groups (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE,
    CONSTRAINT 1 FOREIGN KEY (type_id) REFERENCES types (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE)

So I think you introduced that bug where 2 foreign keys become one, sorry… :see_no_evil:

greg0ire on 4 Sep 2020

We still have lots of things to discuss but I believe this shows I am on the right track: https://github.com/greg0ire/dbal/pull/3

Questions that need an answer:

Should the DBAL add implicit indexes if they are missing? Or is it overstepping? Or should that behavior somehow be configurable? Is the use case of migrating a table not created by the DBAL even officially supported?
How should anonymous foreign keys be handled? I think using fk_$id as a name would be a reasonable solution, but again, maybe naming the foreign key should be considered overstepping.

greg0ire on 4 Sep 2020

Things that can be done without waiting for any discussion: write a failing test case for the bug outlined in https://github.com/doctrine/dbal/issues/4243#issuecomment-687362580 and fix it by reverting #4246

greg0ire on 5 Sep 2020

I propose to revert #3762 and all subsequent changes (#4255) since it opened a can of worms nobody of the participants can comprehend. Unlike this behavior that existing projects rely on, the logic fixed/improved in #3762 never worked.

morozov on 5 Sep 2020

that makes no sense imho, sqlite is the only platform that did not support foreign keys, and since its used for functional tests in laravel, that provides a false sense of security IMHO. I believe there is just a small thing missing here to get it in line with all other platforms.

beberlei on 5 Sep 2020

The sense of the revert is to prevent the regression in the projects that depend on DBAL. At the moment, the regression obviously created more harm than the value added by the bugfix.

I'm fine if you @beberlei own this issue and solve it the way you see fit.

morozov on 5 Sep 2020

@MyIgel fix is right, but misses another component, SqliteSchemaManager::_getPortableTableForeignKeysList uses the constraint_name to create the right foreign key objects. It does not handle the case where constraint_name == NULL correctly and leads to the merge of constraints from #4246

beberlei on 6 Sep 2020

Dbal: SQLite Foreign Key Behavior BC Break

BC Break Report

Summary

Most helpful comment

All 34 comments

Related issues