Merge pull request #1704 from acelaya-forks/feature/stronger-db-detection

Feature/stronger db detection

CHANGELOG.md

@@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 ### Fixed
 * [#1698](https://github.com/shlinkio/shlink/issues/1698) Fixed error 500 in `robots.txt`.
 * [#1688](https://github.com/shlinkio/shlink/issues/1688) Fixed huge performance degradation on `/tags/stats` endpoint.
+* [#1693](https://github.com/shlinkio/shlink/issues/1693) Fixed Shlink thinking database already exists if it finds foreign tables.
 
 
 ## [3.5.1] - 2023-02-04

config/autoload/entity-manager.local.php.dist

@@ -6,12 +6,32 @@ return [
 
     'entity_manager' => [
         'connection' => [
+            // MySQL
             'user' => 'root',
             'password' => 'root',
             'driver' => 'pdo_mysql',
             'host' => 'shlink_db_mysql',
             'dbname' => 'shlink',
+//            'dbname' => 'shlink_foo',
             'charset' => 'utf8mb4',
+
+            // Postgres
+//            'user' => 'postgres',
+//            'password' => 'root',
+//            'driver' => 'pdo_pgsql',
+//            'host' => 'shlink_db_postgres',
+//            'dbname' => 'shlink_foo',
+//            'charset' => 'utf8',
+
+            // MSSQL
+//            'user' => 'sa',
+//            'password' => 'Passw0rd!',
+//            'driver' => 'pdo_sqlsrv',
+//            'host' => 'shlink_db_ms',
+//            'dbname' => 'shlink_foo',
+//            'driverOptions' => [
+//                'TrustServerCertificate' => 'true',
+//            ],
         ],
     ],
 

config/constants.php

@@ -19,4 +19,3 @@ const DEFAULT_QR_CODE_FORMAT = 'png';
 const DEFAULT_QR_CODE_ERROR_CORRECTION = 'l';
 const DEFAULT_QR_CODE_ROUND_BLOCK_SIZE = true;
 const MIN_TASK_WORKERS = 4;
-const MIGRATIONS_TABLE = 'migrations';

config/test/bootstrap_cli_tests.php

@@ -23,10 +23,10 @@ if (file_exists($covFile)) {
 }
 
 $testHelper->createTestDb(
-    ['bin/cli', 'db:create'],
-    ['bin/cli', 'db:migrate'],
-    ['bin/doctrine', 'orm:schema-tool:drop'],
-    ['bin/doctrine', 'dbal:run-sql'],
+    createDbCommand: ['bin/cli', 'db:create'],
+    migrateDbCommand: ['bin/cli', 'db:migrate'],
+    dropSchemaCommand: ['bin/doctrine', 'orm:schema-tool:drop'],
+    runSqlCommand: ['bin/doctrine', 'dbal:run-sql'],
 );
 CliTest\CliTestCase::setSeedFixturesCallback(
     static fn () => $testHelper->seedFixtures($em, $config['data_fixtures'] ?? []),

migrations.php

@@ -2,15 +2,13 @@
 
 declare(strict_types=1);
 
-use const Shlinkio\Shlink\MIGRATIONS_TABLE;
-
 return [
 
     'migrations_paths' => [
         'ShlinkMigrations' => 'data/migrations',
     ],
     'table_storage' => [
-        'table_name' => MIGRATIONS_TABLE,
+        'table_name' => 'migrations',
     ],
     'custom_template' => 'data/migrations_template.txt',
 

module/CLI/config/dependencies.config.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\CLI;
 
-use Doctrine\DBAL\Connection;
 use GeoIp2\Database\Reader;
 use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory;
 use Laminas\ServiceManager\Factory\InvokableFactory;
@@ -116,7 +115,7 @@ return [
             LockFactory::class,
             Util\ProcessRunner::class,
             PhpExecutableFinder::class,
-            Connection::class,
+            'em',
             NoDbNameConnectionFactory::SERVICE_NAME,
         ],
         Command\Db\MigrateDatabaseCommand::class => [

module/CLI/src/Command/Db/CreateDatabaseCommand.php

@@ -6,6 +6,8 @@ namespace Shlinkio\Shlink\CLI\Command\Db;
 
 use Doctrine\DBAL\Connection;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Doctrine\ORM\EntityManagerInterface;
+use Doctrine\ORM\Mapping\ClassMetadata;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Shlinkio\Shlink\CLI\Util\ProcessRunnerInterface;
 use Symfony\Component\Console\Input\InputInterface;
@@ -15,12 +17,13 @@ use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Process\PhpExecutableFinder;
 
 use function Functional\contains;
-use function Functional\filter;
-
-use const Shlinkio\Shlink\MIGRATIONS_TABLE;
+use function Functional\map;
+use function Functional\some;
 
 class CreateDatabaseCommand extends AbstractDatabaseCommand
 {
+    private readonly Connection $regularConn;
+
     public const NAME = 'db:create';
     public const DOCTRINE_SCRIPT = 'bin/doctrine';
     public const DOCTRINE_CREATE_SCHEMA_COMMAND = 'orm:schema-tool:create';
@@ -29,9 +32,10 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
         LockFactory $locker,
         ProcessRunnerInterface $processRunner,
         PhpExecutableFinder $phpFinder,
-        private Connection $regularConn,
-        private Connection $noDbNameConn,
+        private readonly EntityManagerInterface $em,
+        private readonly Connection $noDbNameConn,
     ) {
+        $this->regularConn = $this->em->getConnection();
         parent::__construct($locker, $processRunner, $phpFinder);
     }
 
@@ -74,6 +78,8 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
         // Otherwise, it will fail to connect and will not be able to create the new database
         $schemaManager = $this->noDbNameConn->createSchemaManager();
         $databases = $schemaManager->listDatabases();
+        // We cannot use getDatabase() to get the database name here, because then the driver will try to connect, and
+        // it does not exist yet. We need to read from the raw params instead.
         $shlinkDatabase = $this->regularConn->getParams()['dbname'] ?? null;
 
         if ($shlinkDatabase !== null && ! contains($databases, $shlinkDatabase)) {
@@ -83,10 +89,14 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
 
     private function schemaExists(): bool
     {
-        // If at least one of the shlink tables exist, we will consider the database exists somehow.
-        // We exclude the migrations table, in case db:migrate was run first by mistake.
-        // Any other inconsistency will be taken care by the migrations.
         $schemaManager = $this->regularConn->createSchemaManager();
-        return ! empty(filter($schemaManager->listTableNames(), fn (string $table) => $table !== MIGRATIONS_TABLE));
+        $existingTables = $schemaManager->listTableNames();
+
+        $allMetadata = $this->em->getMetadataFactory()->getAllMetadata();
+        $shlinkTables = map($allMetadata, static fn (ClassMetadata $metadata) => $metadata->getTableName());
+
+        // If at least one of the shlink tables exist, we will consider the database exists somehow.
+        // Any other inconsistency will be taken care of by the migrations.
+        return some($shlinkTables, static fn (string $shlinkTable) => contains($existingTables, $shlinkTable));
     }
 }

module/CLI/test/Command/Db/CreateDatabaseCommandTest.php

@@ -9,6 +9,9 @@ use Doctrine\DBAL\Driver;
 use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
 use Doctrine\DBAL\Schema\AbstractSchemaManager;
+use Doctrine\ORM\EntityManagerInterface;
+use Doctrine\ORM\Mapping\ClassMetadata;
+use Doctrine\Persistence\Mapping\ClassMetadataFactory;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -22,8 +25,6 @@ use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Lock\LockInterface;
 use Symfony\Component\Process\PhpExecutableFinder;
 
-use const Shlinkio\Shlink\MIGRATIONS_TABLE;
-
 class CreateDatabaseCommandTest extends TestCase
 {
     use CliTestUtilsTrait;
@@ -31,6 +32,7 @@ class CreateDatabaseCommandTest extends TestCase
     private CommandTester $commandTester;
     private MockObject & ProcessRunnerInterface $processHelper;
     private MockObject & Connection $regularConn;
+    private MockObject & ClassMetadataFactory $metadataFactory;
     private MockObject & AbstractSchemaManager $schemaManager;
     private MockObject & Driver $driver;
 
@@ -51,17 +53,16 @@ class CreateDatabaseCommandTest extends TestCase
         $this->regularConn->method('createSchemaManager')->willReturn($this->schemaManager);
         $this->driver = $this->createMock(Driver::class);
         $this->regularConn->method('getDriver')->willReturn($this->driver);
+
+        $this->metadataFactory = $this->createMock(ClassMetadataFactory::class);
+        $em = $this->createMock(EntityManagerInterface::class);
+        $em->method('getConnection')->willReturn($this->regularConn);
+        $em->method('getMetadataFactory')->willReturn($this->metadataFactory);
+
         $noDbNameConn = $this->createMock(Connection::class);
         $noDbNameConn->method('createSchemaManager')->withAnyParameters()->willReturn($this->schemaManager);
 
-        $command = new CreateDatabaseCommand(
-            $locker,
-            $this->processHelper,
-            $phpExecutableFinder,
-            $this->regularConn,
-            $noDbNameConn,
-        );
-
+        $command = new CreateDatabaseCommand($locker, $this->processHelper, $phpExecutableFinder, $em, $noDbNameConn);
         $this->commandTester = $this->testerForCommand($command);
     }
 
@@ -70,6 +71,9 @@ class CreateDatabaseCommandTest extends TestCase
     {
         $shlinkDatabase = 'shlink_database';
         $this->regularConn->expects($this->once())->method('getParams')->willReturn(['dbname' => $shlinkDatabase]);
+        $metadataMock = $this->createMock(ClassMetadata::class);
+        $metadataMock->expects($this->once())->method('getTableName')->willReturn('foo_table');
+        $this->metadataFactory->method('getAllMetadata')->willReturn([$metadataMock]);
         $this->schemaManager->expects($this->once())->method('listDatabases')->willReturn(
             ['foo', $shlinkDatabase, 'bar'],
         );
@@ -88,10 +92,11 @@ class CreateDatabaseCommandTest extends TestCase
     {
         $shlinkDatabase = 'shlink_database';
         $this->regularConn->expects($this->once())->method('getParams')->willReturn(['dbname' => $shlinkDatabase]);
+        $this->metadataFactory->method('getAllMetadata')->willReturn([]);
         $this->schemaManager->expects($this->once())->method('listDatabases')->willReturn(['foo', 'bar']);
         $this->schemaManager->expects($this->once())->method('createDatabase')->with($shlinkDatabase);
         $this->schemaManager->expects($this->once())->method('listTableNames')->willReturn(
-            ['foo_table', 'bar_table', MIGRATIONS_TABLE],
+            ['foo_table', 'bar_table'],
         );
         $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
 
@@ -103,6 +108,9 @@ class CreateDatabaseCommandTest extends TestCase
     {
         $shlinkDatabase = 'shlink_database';
         $this->regularConn->expects($this->once())->method('getParams')->willReturn(['dbname' => $shlinkDatabase]);
+        $metadata = $this->createMock(ClassMetadata::class);
+        $metadata->method('getTableName')->willReturn('shlink_table');
+        $this->metadataFactory->method('getAllMetadata')->willReturn([$metadata]);
         $this->schemaManager->expects($this->once())->method('listDatabases')->willReturn(
             ['foo', $shlinkDatabase, 'bar'],
         );
@@ -126,7 +134,7 @@ class CreateDatabaseCommandTest extends TestCase
     public static function provideEmptyDatabase(): iterable
     {
         yield 'no tables' => [[]];
-        yield 'migrations table' => [[MIGRATIONS_TABLE]];
+        yield 'migrations table' => [['non_shlink_table']];
     }
 
     #[Test]
@@ -135,6 +143,7 @@ class CreateDatabaseCommandTest extends TestCase
         $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(SqlitePlatform::class));
 
         $this->regularConn->expects($this->never())->method('getParams');
+        $this->metadataFactory->expects($this->once())->method('getAllMetadata')->willReturn([]);
         $this->schemaManager->expects($this->never())->method('listDatabases');
         $this->schemaManager->expects($this->never())->method('createDatabase');
         $this->schemaManager->expects($this->once())->method('listTableNames')->willReturn(['foo_table', 'bar_table']);