dbal icon indicating copy to clipboard operation
dbal copied to clipboard

Changing/renaming/dropping columns on sqlite doesn't work in v 3.4.x

Open oraslaci opened this issue 2 years ago • 2 comments

Bug Report

Q A
Version 3.4.x

Summary

I ran into the issue while running migrations in laravel 9.0, which uses docrine/dbal behind the curtains.

Any kind of change (dropColumn, renameColumn, change) to columns on sqlite db, throws an error

Current behaviour

Migrations throw an exception: ErrorException

Undefined array key -1

  at vendor/doctrine/dbal/src/Schema/SqliteSchemaManager.php:605
    601▕         $foreignKeyCount   = count($foreignKeyDetails);
    602▕ 
    603▕         foreach ($columns as $i => $column) {
    604▕             // SQLite identifies foreign keys in reverse order of appearance in SQL
  ➜ 605▕             $columns[$i] = array_merge($column, $foreignKeyDetails[$foreignKeyCount - $column['id'] - 1]);
    606▕         }
    607▕ 
    608▕         return $columns;
    609▕     }

I traced the issue to this function, where the regex fails to match what createSql returns

 private function getForeignKeyDetails($table)
    {
        $createSql = $this->getCreateTableSQL($table);
        \Log::info($createSql);
//CREATE TABLE "files" ("id" integer not null primary key autoincrement, "user_id" integer not null, "uuid" varchar not null, "type" varchar not null, "state" varchar not null, "name" varchar not null, ... "is_flagged" tinyint(1), foreign key("user_id") references "users"("id"))  
        dd( 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
        ));
// 0
        if (
            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
            ) === 0
        ) {
            return [];
        }

Somehow the regex fails to match the following

How to reproduce

  1. Setup a basic project with laravel, use sqlite as connection (when running tests you might run into this use case)
DB_CONNECTION=sqlite
DB_HOST=database/database.sqlite
  1. Create a table with a foreign key using a migration
  2. Create another migration where you try to modify a column from the previous

Expected behaviour

The migration should run w/o a problem.

Workaround:

Installed "doctrine/dbal": "^2.13.3" and it works w/o a problem.

oraslaci avatar Aug 10 '22 13:08 oraslaci

Hi @oraslaci

This has been raised and merged in, just waiting on them to release a 4.0.1. For now we have rolled back to 3.3.7

chrispage1 avatar Aug 10 '22 14:08 chrispage1

This looks like a duplicate of https://github.com/doctrine/dbal/issues/5572 which has been fixed by https://github.com/doctrine/dbal/pull/5577 but the fix hasn't been released yet.

@oraslaci could you try reproducing your scenario using ^3.4@dev as the version constraint for doctrine/dbal?

morozov avatar Aug 11 '22 02:08 morozov

@morozov We ran into the same issue noted in #5572

The issue appears to persist when using ^3.4@dev as the version constraint for doctrine/dbal but does not impact 3.3.7.

Screenshot_2022-08-12_22-31-35

GlitchWitch avatar Aug 12 '22 22:08 GlitchWitch

@GlitchWitch it doesn't look like your locked version of doctrine/dbal has changed. Did you do composer update doctrine/dbal? Could you run composer show doctrine/dbal and post the output? Specifically, check the following lines:

$ composer show doctrine/dbal

name     : doctrine/dbal
...
source   : [git] https://github.com/doctrine/dbal.git 564edcd17beaf4ed063fb4f453c75d01b6683a98
dist     : [zip] https://api.github.com/repos/doctrine/dbal/zipball/564edcd17beaf4ed063fb4f453c75d01b6683a98 564edcd17beaf4ed063fb4f453c75d01b6683a98

morozov avatar Aug 12 '22 22:08 morozov

@morozov I did run composer update and composer update doctrine/dbal after setting ^3.4@dev in my composer.json file.

Screenshot_2022-08-12_22-48-17

This caused the following to get added to the lock file.

    "stability-flags": {
        "doctrine/dbal": 20
    },
$ composer show doctrine/dbal
[...]
source   : [git] https://github.com/doctrine/dbal.git 118a360e9437e88d49024f36283c8bcbd76105f5
dist     : [zip] https://api.github.com/repos/doctrine/dbal/zipball/118a360e9437e88d49024f36283c8bcbd76105f5 118a360e9437e88d49024f36283c8bcbd76105f5
[...]

GlitchWitch avatar Aug 12 '22 22:08 GlitchWitch

According to the above output, you're using commit 118a360e9437e88d49024f36283c8bcbd76105f5 which is the 3.4.0 release commit and doesn't contain the fix.

morozov avatar Aug 12 '22 23:08 morozov

Yeah, I just realised that. I'm not sure why setting ^3.4@dev per your suggestion along with composer update doctrine/dbal isn't grabbing the latest commit.

I've just tested by manually changing the lock file to use the latest commit 564edcd17beaf4ed063fb4f453c75d01b6683a98. ~Will update in a few minutes when GH actions finishes.~

Edit: that seems to solve it and things are back to normal. :+1:

GlitchWitch avatar Aug 12 '22 23:08 GlitchWitch

Thanks for the confirmation, @GlitchWitch.

morozov avatar Aug 12 '22 23:08 morozov

Hello,

I tried out the newly released version 3.4.1 and it isn't fixed on my side. On version 3.3.x all went good. The error message is:

ErrorException : Undefined array key -1
\tkfz-local\vendor\laravel\framework\src\Illuminate\Foundation\Bootstrap\HandleExceptions.php:257
\tkfz-local\vendor\doctrine\dbal\src\Schema\SqliteSchemaManager.php:607

My composer.lock file is:

{
  "name": "doctrine/dbal",
  "version": "3.4.1",
  "source": {
    "type": "git",
    "url": "https://github.com/doctrine/dbal.git",
    "reference": "94e016428884227245fb1219e0de7d8b86ca16d7"
    },
    "dist": {
      "type": "zip",
      "url": "https://api.github.com/repos/doctrine/dbal/zipball/94e016428884227245fb1219e0de7d8b86ca16d7",
      "reference": "94e016428884227245fb1219e0de7d8b86ca16d7",
     "shasum": ""
  },
}

composer show doctrine/dbal looks like this:

versions : * 3.4.1
type     : library
license  : MIT License (MIT) (OSI approved) https://spdx.org/licenses/MIT.html#licenseText
homepage : https://www.doctrine-project.org/projects/dbal.html
source   : [git] https://github.com/doctrine/dbal.git 94e016428884227245fb1219e0de7d8b86ca16d7
dist     : [zip] https://api.github.com/repos/doctrine/dbal/zipball/94e016428884227245fb1219e0de7d8b86ca16d7 94e016428884227245fb1219e0de7d8b86ca16d7
path     : C:\Repository\tkfz-local\vendor\doctrine\dbal
names    : doctrine/dbal

Steps for reproduction:

1.) New Laravel project with doctrine 2.) New migration which creates a new table with a foreign key to users table 3.) New migration which drops the foreign key and the column from the fresh created table 4.) Set your connections for testing to sqlite in memory in phpunit.xml

<env name="DB_CONNECTION" value="sqlite"/>
<env name="DB_DATABASE" value=":memory:"/>

5.) Add the following code in to your TestCase.php (This adds supports for dropping foreign keys before version 3.4.x)

    use CreatesApplication, RefreshDatabase;

    public function __construct(?string $name = null, array $data = [], $dataName = '')
    {
        $this->hotfixSqlite();
        parent::__construct($name, $data, $dataName);
    }

    /**
     * Fix for: BadMethodCallException : SQLite doesn't support dropping foreign keys (you would need to re-create the table).
     */
    public function hotfixSqlite()
    {
        Connection::resolverFor('sqlite', function ($connection, $database, $prefix, $config) {
            return new class($connection, $database, $prefix, $config) extends SQLiteConnection
            {
                public function getSchemaBuilder()
                {
                    if ($this->schemaGrammar === null) {
                        $this->useDefaultSchemaGrammar();
                    }

                    return new class($this) extends SQLiteBuilder
                    {
                        protected function createBlueprint($table, \Closure $callback = null)
                        {
                            return new class($table, $callback) extends Blueprint
                            {
                                public function dropForeign($index)
                                {
                                    return new Fluent();
                                }
                            };
                        }
                    };
                }
            };
        });
    }

6.) Run the ExampleTest from the tests/Feature directory. 7.) The above error message is shown.

Here is a link to an example repository https://github.com/schonhoff/example-app-vite

The output for the dd from the variables dd($foreignKeyDetails, $foreignKeyCount, $columns); of addDetailsToTableForeignKeyColumns from SqliteSchemaManger.php is:

\example-app-vite\vendor\doctrine\dbal\src\Schema\SqliteSchemaManager.php#L604 
[]
\example-app-vite\vendor\doctrine\dbal\src\Schema\SqliteSchemaManager.php#L604 
0
\example-app-vite\vendor\doctrine\dbal\src\Schema\SqliteSchemaManager.php#L604
[
  0 => array:9 [
    "table_name" => "testing_sqlite"
    "id" => 0
    "seq" => 0
    "table" => "users"
    "from" => "created_by"
    "to" => "id"
    "on_update" => "NO ACTION"
    "on_delete" => "NO ACTION"
    "match" => "NONE"
  ]
]

Because it is a memory sqlite database maybe that can be the issue.

schonhoff avatar Aug 17 '22 07:08 schonhoff

I can also confirm that the issue still persists. I did make sure I have the correct v3.4.1 installed and composer show doctrine/dbal shows pretty much the exact same as shared above.

VicGUTT avatar Aug 17 '22 15:08 VicGUTT

Thanks for the reproducer, @schonhoff. I should be able to debug the regular expression and question and hopefully adjust it to work properly with the DDL in question.

morozov avatar Aug 17 '22 17:08 morozov

There is a pending pull request that should address this issue (https://github.com/doctrine/dbal/pull/5597). Please provide your feedback if you can test it in your environments.

morozov avatar Aug 17 '22 18:08 morozov

I did the change myself in my project and it worked like intended. Maybe another person can confirm it?

schonhoff avatar Aug 18 '22 09:08 schonhoff

Yep, I manually did the change as well and it's all good and well. Thank you ! 🙌

VicGUTT avatar Aug 20 '22 11:08 VicGUTT

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

github-actions[bot] avatar Sep 20 '22 00:09 github-actions[bot]