Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
oraslaci opened this issue Aug 10, 2022 · 15 comments · Fixed by #5597
Closed

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

oraslaci opened this issue Aug 10, 2022 · 15 comments · Fixed by #5597

Comments

@oraslaci
Copy link

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.

@chrispage1
Copy link

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

@morozov
Copy link
Member

morozov commented Aug 11, 2022

This looks like a duplicate of #5572 which has been fixed by #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?

@GlitchWitch
Copy link

GlitchWitch commented Aug 12, 2022

@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

@morozov
Copy link
Member

morozov commented Aug 12, 2022

@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

@GlitchWitch
Copy link

GlitchWitch commented Aug 12, 2022

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

@morozov
Copy link
Member

morozov commented Aug 12, 2022

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

@GlitchWitch
Copy link

GlitchWitch commented Aug 12, 2022

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 564edcd. Will update in a few minutes when GH actions finishes.

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

@morozov
Copy link
Member

morozov commented Aug 12, 2022

Thanks for the confirmation, @GlitchWitch.

@schonhoff
Copy link

schonhoff commented Aug 17, 2022

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.

@VicGUTT
Copy link

VicGUTT commented Aug 17, 2022

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.

@morozov
Copy link
Member

morozov commented Aug 17, 2022

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
Copy link
Member

morozov commented Aug 17, 2022

There is a pending pull request that should address this issue (#5597). Please provide your feedback if you can test it in your environments.

@schonhoff
Copy link

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

@VicGUTT
Copy link

VicGUTT commented Aug 20, 2022

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

@github-actions
Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants