dmaicher / doctrine-test-bundle

Symfony bundle to isolate your app's doctrine database tests and improve the test performance
MIT License
1.07k stars 60 forks source link

Impossible to leverage SQLite foreign key support #299

Closed MatTheCat closed 1 month ago

MatTheCat commented 1 month ago

I spent some time trying to understand why a specific test passed, but not when using this bundle.

Turns out it depends on a cascade DELETE, which can only work on SQLite if you register the EnableForeignKeys middleware.

As the StaticDriver will start a transaction before it can kick in, it will have no effect:

It is not possible to enable or disable foreign key constraints in the middle of a multi-statement transaction (when SQLite is not in autocommit mode). Attempting to do so does not return an error; it simply has no effect.

https://www.sqlite.org/foreignkeys.html#fk_enable

Maybe the transaction could be started in another middleware running last?

dmaicher commented 1 month ago

Hm indeed. I guess currently the workaround would be to register that EnableForeignKeys middleware before the middleware of this bundle (re-ordering them) with a custom compiler pass :thinking:

Probably would be fixed by https://github.com/dmaicher/doctrine-test-bundle/issues/251 and just giving the bundle middleware a priority of 1000 for example so you could choose a higher priority for any custom middlewares

Sorry I misunderstood the issue at first. Indeed currently this is not possible to work around :thinking:

EDIT: or actually it should be doable by re-ordering the middlewares?

MatTheCat commented 1 month ago

it should be doable by re-ordering the middlewares?

Indeed the compiler pass I wrote seems to fix my issue, but I’m concerned about the our middleware needs to be the first one in the DoctrineTestCompilerPass, because this won’t be the case anymore :thinking:

Anyways it was a bit cumbersome to write, and https://github.com/dmaicher/doctrine-test-bundle/issues/251 seems like it would ease that :+1:

dmaicher commented 1 month ago

yeah this

our middleware needs to be the first one

comment is invalid now :smile: I simply was not aware of any scenario where the middleware should not be the first one.

I will look into implementing https://github.com/dmaicher/doctrine-test-bundle/issues/251 and then you could just increase the priority accordingly :+1:

MatTheCat commented 1 month ago

Nice! Should I close this issue when #251 is fixed?

In the meantime this is what I ended up with:

<?php

namespace App;

use Doctrine\DBAL\Driver\AbstractSQLiteDriver\Middleware\EnableForeignKeys;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

class ReorderMiddlewaresPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        if (!$container->hasDefinition('dama.doctrine.dbal.middleware')) {
            return;
        }

        foreach (array_keys($container->getParameter('doctrine.connections')) as $connectionName) {
            $definition = $container->getDefinition(sprintf('doctrine.dbal.%s_connection.configuration', $connectionName));
            $calls = $definition->getMethodCalls();
            foreach ($calls as &$call) {
                if ('setMiddlewares' !== $call[0]) {
                    continue;
                }

                foreach ($call[1][0] as $i => $middleware) {
                    if (!$middleware instanceof ChildDefinition || EnableForeignKeys::class !== $middleware->getParent()) {
                        continue;
                    }

                    array_unshift($call[1][0], $middleware);
                    unset($call[1][0][$i]);

                    break;
                }

                break;
            }

            $definition->setMethodCalls($calls);
        }
    }
}

It must run after the DoctrineTestCompilerPass so I added it with TYPE_BEFORE_OPTIMIZATION and priority: -2.

dmaicher commented 1 month ago

Closing as it will be fixed by https://github.com/dmaicher/doctrine-test-bundle/issues/251