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

Tests broken after upgrade doctrine/dbal to 2.10.3 #34093

Closed
hakanersu opened this issue Sep 2, 2020 · 24 comments
Closed

Tests broken after upgrade doctrine/dbal to 2.10.3 #34093

hakanersu opened this issue Sep 2, 2020 · 24 comments
Labels

Comments

@hakanersu
Copy link

hakanersu commented Sep 2, 2020

  • Laravel Version: v7.27.0
  • PHP Version: 7.4.3
  • Database Driver & Version: Sqlite

Description:

After upgrading from 7.25.0 to v7.27.0 tests getting broken. Im getting query exceptions when using DatabaseMigrations, RefreshDatabase at same time.

It seems to related to #34091

 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();
        });
    }
}

Downgrading doctrine/dbal from 2.10.3 to 2.10.2 fixes problem.

Steps To Reproduce:

Upgrade 7.27.0 and create any basic test with DatabaseMigrations, RefreshDatabase. If i use just one of them tests works fine.

@hakanersu hakanersu changed the title Test broken after upgrade to 7.27.0 Test broken after upgrade to 7.27.0 with doctrine/dbal 2.10.3 Sep 2, 2020
@hakanersu hakanersu changed the title Test broken after upgrade to 7.27.0 with doctrine/dbal 2.10.3 Tests broken after upgrade to 7.27.0 with doctrine/dbal 2.10.3 Sep 2, 2020
@hakanersu hakanersu changed the title Tests broken after upgrade to 7.27.0 with doctrine/dbal 2.10.3 Tests broken after upgrade doctrine/dbal to 2.10.3 Sep 2, 2020
@uuf6429
Copy link

uuf6429 commented Sep 2, 2020

I have the same problem and spent some time investigating it.

What happened is that \Doctrine\DBAL\Platforms\SqlitePlatform::supportsForeignKeyConstraints has been changed to true in 2.10.3 (if you set this to true on 2.10.2, the same problems happens).

I don't know why, but it seems that doctrine/dbal does not create indexes when creating a table with foreign keys, but when changing an existing table with foreign keys, it tries to remove some non-existent index.

@MyIgel
Copy link

MyIgel commented Sep 2, 2020

Can cofirm that, the doctrine/dbal@85a983c commit is the one that breaks it in dbal for sqlite, so it does not work after an upgrade from dbal v2.10.2 to v2.10.3

@SirNarsh
Copy link

SirNarsh commented Sep 2, 2020

Facing a similar problem here also after upgrading, seems not directly laravel but doctrine/dbal 2.10.3, switching to v2.10.2 fixed test failing sqlite failing:
General error: 1 no such index: IDX_DC8C84A116FE72E1 (SQL: DROP INDEX IDX_DC8C84A116FE72E1)

PHP 7.4.8
laravel/framework: v7.27.0
doctrine/dbal 2.10.3

@taylorotwell
Copy link
Member

Feel free to share your information on the Doctrine issue I created: doctrine/dbal#4243

@greg0ire
Copy link
Contributor

greg0ire commented Sep 2, 2020

@hakanersu there are 2 exceptions in the code:

catch (Exception $e) {
throw new QueryException(
$query, $this->prepareBindings($bindings), $e

The one that appears in your screenshots comes from Laravel, and is triggered by the one line 670 , that is issued by doctrine/dbal.

When posting an exception you should:

  • prefer text over screenshots so that people can copy paste from them, and to be more friendly to search engines;
  • include all exceptions, not just the first one.

If you still have access to the stack trace of the second exception, please post it as text on the page linked by @taylorotwell above, it will be helpful 🙏

@greg0ire
Copy link
Contributor

greg0ire commented Sep 2, 2020

I don't know why, but it seems that doctrine/dbal does not create indexes when creating a table with foreign keys

@uuf6429 was the table created with the latest version of doctrine/dbal?

@uuf6429
Copy link

uuf6429 commented Sep 2, 2020

I don't know why, but it seems that doctrine/dbal does not create indexes when creating a table with foreign keys

@uuf6429 was the table created with the latest version of doctrine/dbal?

Yes - this happens before running tests, when running migrations through artisan. The migrations are all based on laravel blueprint.

@driesvints driesvints added the bug label Sep 3, 2020
@greg0ire
Copy link
Contributor

greg0ire commented Sep 3, 2020

Is this method used at any point, and if yes, is $name null?

/**
* Specify a foreign key for the table.
*
* @param string|array $columns
* @param string|null $name
* @return \Illuminate\Database\Schema\ForeignKeyDefinition
*/
public function foreign($columns, $name = null)
{
$command = new ForeignKeyDefinition(
$this->indexCommand('foreign', $columns, $name)->getAttributes()
);
$this->commands[count($this->commands) - 1] = $command;

Shouldn't that result in

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
+    FOREIGN KEY (user_id) REFERENCES users (id) ON UPDATE NO ACTION ON DELETE SET NULL NOT DEFERRABLE INITIALLY IMMEDIATE, 
+    FOREIGN KEY (account_id) REFERENCES accounts (id) ON UPDATE NO ACTION ON DELETE CASCADE NOT DEFERRABLE INITIALLY IMMEDIATE
)

Note that this syntax does not even appear in SQlite's official doc about foreign keys

Here is where that syntax might come from: https://github.com/doctrine/dbal/blob/f69c990e359931753232a81c3a1821a7ea060bfd/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L2586-L2588

@taylorotwell
Copy link
Member

taylorotwell commented Sep 3, 2020

@greg0ire

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

create table "posts" 
("id" integer not null primary key autoincrement, 
"user_id" integer not null, 
"body" varchar not null, 
"created_at" datetime null, 
"updated_at" datetime null, 
foreign key("user_id") references "users"("id"))

However, if I later try to use doctrine/dbal to modify that table - such as changing "body" from varchar to text - I run into the exception noted above.

$name is not used when building foreign keys with SQLite. I believe it is used with MySQL or other databases that support it.

@taylorotwell
Copy link
Member

@greg0ire

When attempting to change the column, here is an array of SQL statements doctrine/dbal informs us should be run to make the change on SQLite:

array:7 [
  0 => "DROP INDEX IDX_885DBAFAA76ED395"
  1 => "CREATE TEMPORARY TABLE __temp__posts AS SELECT id, user_id, body, created_at, updated_at FROM posts"
  2 => "DROP TABLE posts"
  3 => "CREATE TABLE posts (id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, user_id INTEGER NOT NULL, created_at DATETIME DEFAULT NULL, updated_at DATETIME DEFAULT NULL, body CLOB NOT NULL COLLATE BINARY, CONSTRAINT 0 FOREIGN KEY (user_id) REFERENCES users (id) ON UPDATE NO ACTION ON DELETE NO ACTION NOT DEFERRABLE INITIALLY IMMEDIATE)"
  4 => "INSERT INTO posts (id, user_id, body, created_at, updated_at) SELECT id, user_id, body, created_at, updated_at FROM __temp__posts"
  5 => "DROP TABLE __temp__posts"
  6 => "CREATE INDEX IDX_885DBAFAA76ED395 ON posts (user_id)"
]

@greg0ire
Copy link
Contributor

greg0ire commented Sep 3, 2020

Thanks, it explains a lot! I didn't understand why we were reading information from posts before creating it, didn't know it was dropped then re-created instead of altered.

@MyIgel contributed doctrine/dbal#4246 yesterday, and I think it fixes the "reading information from posts before dropping it" part.

Can people here please test doctrine/[email protected] and report if it does fix the issue for them?

@taylorotwell
Copy link
Member

taylorotwell commented Sep 3, 2020

@greg0ire

I installed it (Upgrading doctrine/dbal (2.10.3 => 2.10.x-dev cd2ddda)).

I still receive the same error:

General error: 1 no such index: IDX_885DBAFAA76ED395 (SQL: DROP INDEX IDX_885DBAFAA76ED395)

Verified the change from that PR in my vendor directory so I do appear to be using 2.10.x-dev.

@greg0ire
Copy link
Contributor

greg0ire commented Sep 3, 2020

Ok so there are in facts 2 bugs? One with query number 0 and one with query number 3, and I think @MyIgel fixed the one with query number 3 with doctrine/dbal#4246 . No idea why the DBAL thinks there should be an index in addition to the foreign key yet.

Will try the reproducer in doctrine/dbal#4243 (comment) later, but feel free to beat me to it, anyone.

EDIT: Started an investigation here: doctrine/dbal#4243 (comment)

@uuf6429
Copy link

uuf6429 commented Sep 4, 2020

By the way, a bit late, but I have a further suggestion (which worked in my case).

Depending on how many laravel users have this problem, we could add the following to laravel/framework's composer.json, if it doesn't happen often, this could also be done for individual use-cases:

    "conflict": {
        "doctrine/dbal": "2.10.3"
    },

@greg0ire
Copy link
Contributor

greg0ire commented Sep 5, 2020

For the moment I have this workaround/experiment that is ugly but works: greg0ire/dbal#3
I think the issue could also be fixed in Laravel by making the thing that creates foreign key (so Blueprint?) also create indices on foreign keys (that should take care of issue with query number 0), and by making sure foreign keys are named (by using the CONSTRAINT blah FOREIGN KEY… syntax) (that should take care of issue with query number 3). Not saying the issue should be solved only in Laravel, but I can foresee a debate coming about what the correct behavior for the DBAL should be whereas, the 2 changes in Laravel pointed above might be less subject to debate.

@serhatculhalikp
Copy link

serhatculhalikp commented Sep 5, 2020

@greg0ire this may be related as well or coincidence?
symfony/symfony#38067

@greg0ire
Copy link
Contributor

greg0ire commented Sep 6, 2020

Too early to tell, but it's possible that it's at least related to the same pull request. Thanks for linking to that, I will keep an eye on it.

@cretueusebiu
Copy link
Contributor

Adding <server name="DB_FOREIGN_KEYS" value="(false)"/> in phpunit.xml fixed the tests for me.

@uuf6429
Copy link

uuf6429 commented Sep 10, 2020

Thanks, it explains a lot! I didn't understand why we were reading information from posts before creating it, didn't know it was dropped then re-created instead of altered.

@MyIgel contributed doctrine/dbal#4246 yesterday, and I think it fixes the "reading information from posts before dropping it" part.

Can people here please test doctrine/[email protected] and report if it does fix the issue for them?

A little context that might help, I learned recently that sqlite does not allow alter statements, so DBALs tend to recreate the table with the new structure.

@ltribolet
Copy link

doctrine/dbal (2.10.x-dev 03bc93b) fixes it for me.

@greg0ire
Copy link
Contributor

We have released the bugfix as 2.10.4, please confirm the bug is fixed and close this.

@driesvints
Copy link
Member

Awesome, thanks.

@greg0ire
Copy link
Contributor

greg0ire commented Sep 14, 2020

You're very welcome, sorry for the trouble actually, and thanks to the Laravel community for helping out with the reproducers, PRs and investigations :)

@driesvints
Copy link
Member

@greg0ire no sorry needed. Thanks for all your work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants