Merge pull request #1818 from acelaya-forks/feature/fix-sqlite-db-creation

Fix Shlink trying to create SQLite database tables even if they already exist

.github/workflows/publish-swagger-spec.yml

@@ -26,7 +26,7 @@ jobs:
       - run: mkdir ${{ steps.determine_version.outputs.version }}
       - run: mv docs/swagger/swagger-inlined.json ${{ steps.determine_version.outputs.version }}/open-api-spec.json
       - name: Publish spec
-        uses: JamesIves/github-pages-deploy-action@4
+        uses: JamesIves/github-pages-deploy-action@4.4.1
         with:
           token: ${{ secrets.OAS_PUBLISH_TOKEN }}
           repository-name: 'shlinkio/shlink-open-api-specs'

CHANGELOG.md

@@ -4,6 +4,58 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org).
 
+## [3.6.3] - 2023-06-14
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1817](https://github.com/shlinkio/shlink/issues/1817) Fix Shlink trying to create SQLite database tables even if they already exist.
+
+
+## [3.6.2] - 2023-06-08
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1808](https://github.com/shlinkio/shlink/issues/1808) Fix `rr` binary downloading during Shlink update.
+
+
+## [3.6.1] - 2023-06-04
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#1413](https://github.com/shlinkio/shlink/issues/1413) Fix error when creating initial DB in Postgres in a cluster where a default `postgres` db does not exist or the credentials do not grant permissions to connect.
+* [#1803](https://github.com/shlinkio/shlink/issues/1803) Fix default RoadRunner port when not using docker image.
+
+
 ## [3.6.0] - 2023-05-24
 ### Added
 * [#1148](https://github.com/shlinkio/shlink/issues/1148) Add support to delete short URL visits.

composer.json

@@ -49,7 +49,7 @@
         "shlinkio/shlink-config": "^2.4",
         "shlinkio/shlink-event-dispatcher": "^3.0",
         "shlinkio/shlink-importer": "^5.1",
-        "shlinkio/shlink-installer": "^8.4",
+        "shlinkio/shlink-installer": "^8.4.1",
         "shlinkio/shlink-ip-geolocation": "^3.2",
         "shlinkio/shlink-json": "^1.0",
         "spiral/roadrunner": "^2023.1",
@@ -72,10 +72,10 @@
         "phpstan/phpstan-phpunit": "^1.3",
         "phpstan/phpstan-symfony": "^1.2",
         "phpunit/php-code-coverage": "^10.0",
-        "phpunit/phpunit": "^10.1",
+        "phpunit/phpunit": "~10.1.0",
         "roave/security-advisories": "dev-master",
         "shlinkio/php-coding-standard": "~2.3.0",
-        "shlinkio/shlink-test-utils": "^3.6",
+        "shlinkio/shlink-test-utils": "~3.6.0",
         "symfony/var-dumper": "^6.2",
         "veewee/composer-run-parallel": "^1.2"
     },

config/roadrunner/.rr.yml

@@ -7,18 +7,18 @@ server:
   command: 'php -dopcache.enable_cli=1 -dopcache.validate_timestamps=0 ../../bin/roadrunner-worker.php'
 
 http:
-  address: '0.0.0.0:${PORT}'
+  address: '0.0.0.0:${PORT:-8080}'
   middleware: ['static']
   static:
     dir: '../../public'
     forbid: ['.php', '.htaccess']
   pool:
-    num_workers: ${WEB_WORKER_NUM}
+    num_workers: ${WEB_WORKER_NUM:-0}
 
 jobs:
   timeout: 300 # 5 minutes
   pool:
-    num_workers: ${TASK_WORKER_NUM}
+    num_workers: ${TASK_WORKER_NUM:-0}
   consume: ['shlink']
   pipelines:
     shlink:

docker/docker-entrypoint.sh

@@ -20,14 +20,6 @@ if [ "${ENABLE_PERIODIC_VISIT_LOCATE}" = "true" ] && [ "${SHLINK_USER_ID}" = "ro
   /usr/sbin/crond &
 fi
 
-# RoadRunner config needs these to have been set, so falling back to default values if not set yet
-if [ "$SHLINK_RUNTIME" == 'rr' ]; then
-  export PORT="${PORT:-"8080"}"
-  # Default to 0 so that RoadRunner decides the number of workers based on the amount of logical CPUs
-  export WEB_WORKER_NUM="${WEB_WORKER_NUM:-"0"}"
-  export TASK_WORKER_NUM="${TASK_WORKER_NUM:-"0"}"
-fi
-
 if [ "$SHLINK_RUNTIME" == 'openswoole' ]; then
   # When restarting the container, openswoole might think it is already in execution
   # This forces the app to be started every second until the exit code is 0

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

@@ -5,7 +5,6 @@ declare(strict_types=1);
 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\ExitCode;
@@ -15,6 +14,7 @@ use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Console\Style\SymfonyStyle;
 use Symfony\Component\Lock\LockFactory;
 use Symfony\Component\Process\PhpExecutableFinder;
+use Throwable;
 
 use function Functional\contains;
 use function Functional\map;
@@ -53,9 +53,7 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
     {
         $io = new SymfonyStyle($input, $output);
 
-        $this->checkDbExists();
-
-        if ($this->schemaExists()) {
+        if ($this->databaseTablesExist()) {
             $io->success('Database already exists. Run "db:migrate" command to make sure it is up to date.');
             return ExitCode::EXIT_SUCCESS;
         }
@@ -68,30 +66,9 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
         return ExitCode::EXIT_SUCCESS;
     }
 
-    private function checkDbExists(): void
+    private function databaseTablesExist(): bool
     {
-        if ($this->regularConn->getDriver()->getDatabasePlatform() instanceof SqlitePlatform) {
-            return;
-        }
-
-        // In order to create the new database, we have to use a connection where the dbname was not set.
-        // 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)) {
-            $schemaManager->createDatabase($shlinkDatabase);
-        }
-    }
-
-    private function schemaExists(): bool
-    {
-        $schemaManager = $this->regularConn->createSchemaManager();
-        $existingTables = $schemaManager->listTableNames();
-
+        $existingTables = $this->ensureDatabaseExistsAndGetTables();
         $allMetadata = $this->em->getMetadataFactory()->getAllMetadata();
         $shlinkTables = map($allMetadata, static fn (ClassMetadata $metadata) => $metadata->getTableName());
 
@@ -99,4 +76,21 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
         // Any other inconsistency will be taken care of by the migrations.
         return some($shlinkTables, static fn (string $shlinkTable) => contains($existingTables, $shlinkTable));
     }
+
+    private function ensureDatabaseExistsAndGetTables(): array
+    {
+        try {
+            // Trying to list tables requires opening a connection to configured database.
+            // If it fails, it means it does not exist yet.
+            return $this->regularConn->createSchemaManager()->listTableNames();
+        } catch (Throwable) {
+            // We cannot use getDatabase() to get the database name here, because then the driver will try to connect.
+            // Instead, we read from the raw params.
+            $shlinkDatabase = $this->regularConn->getParams()['dbname'] ?? '';
+            // Create the database using a connection where the dbname was not set.
+            $this->noDbNameConn->createSchemaManager()->createDatabase($shlinkDatabase);
+
+            return [];
+        }
+    }
 }

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

@@ -7,11 +7,11 @@ namespace ShlinkioTest\Shlink\CLI\Command\Db;
 use Doctrine\DBAL\Connection;
 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 Exception;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
 use PHPUnit\Framework\MockObject\MockObject;
@@ -69,17 +69,14 @@ class CreateDatabaseCommandTest extends TestCase
     #[Test]
     public function successMessageIsPrintedIfDatabaseAlreadyExists(): void
     {
-        $shlinkDatabase = 'shlink_database';
-        $this->regularConn->expects($this->once())->method('getParams')->willReturn(['dbname' => $shlinkDatabase]);
+        $this->regularConn->expects($this->never())->method('getParams');
+        $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
+
         $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'],
-        );
         $this->schemaManager->expects($this->never())->method('createDatabase');
         $this->schemaManager->expects($this->once())->method('listTableNames')->willReturn(['foo_table', 'bar_table']);
-        $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
 
         $this->commandTester->execute([]);
         $output = $this->commandTester->getDisplay();
@@ -90,15 +87,13 @@ class CreateDatabaseCommandTest extends TestCase
     #[Test]
     public function databaseIsCreatedIfItDoesNotExist(): void
     {
+        $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
+
         $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'],
-        );
-        $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
+        $this->schemaManager->expects($this->once())->method('listTableNames')->willThrowException(new Exception(''));
 
         $this->commandTester->execute([]);
     }
@@ -106,14 +101,12 @@ class CreateDatabaseCommandTest extends TestCase
     #[Test, DataProvider('provideEmptyDatabase')]
     public function tablesAreCreatedIfDatabaseIsEmpty(array $tables): void
     {
-        $shlinkDatabase = 'shlink_database';
-        $this->regularConn->expects($this->once())->method('getParams')->willReturn(['dbname' => $shlinkDatabase]);
+        $this->regularConn->expects($this->never())->method('getParams');
+        $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
+
         $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'],
-        );
         $this->schemaManager->expects($this->never())->method('createDatabase');
         $this->schemaManager->expects($this->once())->method('listTableNames')->willReturn($tables);
         $this->processHelper->expects($this->once())->method('run')->with($this->isInstanceOf(OutputInterface::class), [
@@ -122,7 +115,6 @@ class CreateDatabaseCommandTest extends TestCase
             CreateDatabaseCommand::DOCTRINE_CREATE_SCHEMA_COMMAND,
             '--no-interaction',
         ]);
-        $this->driver->method('getDatabasePlatform')->willReturn($this->createMock(AbstractPlatform::class));
 
         $this->commandTester->execute([]);
         $output = $this->commandTester->getDisplay();
@@ -136,18 +128,4 @@ class CreateDatabaseCommandTest extends TestCase
         yield 'no tables' => [[]];
         yield 'migrations table' => [['non_shlink_table']];
     }
-
-    #[Test]
-    public function databaseCheckIsSkippedForSqlite(): void
-    {
-        $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']);
-
-        $this->commandTester->execute([]);
-    }
 }