1 files changed, 156 insertions, 87 deletions

diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php
index 7993730b230..40579c7a898 100644
--- a/lib/private/DB/MigrationService.php
+++ b/lib/private/DB/MigrationService.php
@@ -1,48 +1,28 @@
 <?php
+
 /**
- * @copyright Copyright (c) 2017 Joas Schilling <coding@schilljs.com>
- * @copyright Copyright (c) 2017, ownCloud GmbH
- *
- * @author Christoph Wurst <christoph@winzerhof-wurst.at>
- * @author Daniel Kesselberg <mail@danielkesselberg.de>
- * @author Joas Schilling <coding@schilljs.com>
- * @author Julius Härtl <jus@bitgrid.net>
- * @author Morris Jobke <hey@morrisjobke.de>
- * @author Robin Appelman <robin@icewind.nl>
- *
- * @license AGPL-3.0
- *
- * This code is free software: you can redistribute it and/or modify
- * it under the terms of the GNU Affero General Public License, version 3,
- * as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Affero General Public License for more details.
- *
- * You should have received a copy of the GNU Affero General Public License, version 3,
- * along with this program. If not, see <http://www.gnu.org/licenses/>
- *
+ * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-FileCopyrightText: 2017 ownCloud GmbH
+ * SPDX-License-Identifier: AGPL-3.0-only
  */
 namespace OC\DB;
 
-use Doctrine\DBAL\Platforms\OraclePlatform;
-use Doctrine\DBAL\Platforms\PostgreSQL94Platform;
 use Doctrine\DBAL\Schema\Index;
 use Doctrine\DBAL\Schema\Schema;
 use Doctrine\DBAL\Schema\SchemaException;
 use Doctrine\DBAL\Schema\Sequence;
 use Doctrine\DBAL\Schema\Table;
-use Doctrine\DBAL\Types\Types;
 use OC\App\InfoParser;
 use OC\IntegrityCheck\Helpers\AppLocator;
 use OC\Migration\SimpleOutput;
 use OCP\AppFramework\App;
 use OCP\AppFramework\QueryException;
 use OCP\DB\ISchemaWrapper;
+use OCP\DB\Types;
+use OCP\IDBConnection;
 use OCP\Migration\IMigrationStep;
 use OCP\Migration\IOutput;
+use OCP\Server;
 use Psr\Log\LoggerInterface;
 
 class MigrationService {
@@ -51,6 +31,7 @@ class MigrationService {
 	private string $migrationsPath;
 	private string $migrationsNamespace;
 	private IOutput $output;
+	private LoggerInterface $logger;
 	private Connection $connection;
 	private string $appName;
 	private bool $checkOracle;
@@ -58,11 +39,16 @@ class MigrationService {
 	/**
 	 * @throws \Exception
 	 */
-	public function __construct($appName, Connection $connection, ?IOutput $output = null, ?AppLocator $appLocator = null) {
+	public function __construct(string $appName, Connection $connection, ?IOutput $output = null, ?AppLocator $appLocator = null, ?LoggerInterface $logger = null) {
 		$this->appName = $appName;
 		$this->connection = $connection;
+		if ($logger === null) {
+			$this->logger = Server::get(LoggerInterface::class);
+		} else {
+			$this->logger = $logger;
+		}
 		if ($output === null) {
-			$this->output = new SimpleOutput(\OC::$server->get(LoggerInterface::class), $appName);
+			$this->output = new SimpleOutput($this->logger, $appName);
 		} else {
 			$this->output = $output;
 		}
@@ -72,7 +58,7 @@ class MigrationService {
 			$this->migrationsNamespace = 'OC\\Core\\Migrations';
 			$this->checkOracle = true;
 		} else {
-			if (null === $appLocator) {
+			if ($appLocator === null) {
 				$appLocator = new AppLocator();
 			}
 			$appPath = $appLocator->getAppPath($appName);
@@ -100,18 +86,15 @@ class MigrationService {
 
 	/**
 	 * Returns the name of the app for which this migration is executed
-	 *
-	 * @return string
 	 */
-	public function getApp() {
+	public function getApp(): string {
 		return $this->appName;
 	}
 
 	/**
-	 * @return bool
 	 * @codeCoverageIgnore - this will implicitly tested on installation
 	 */
-	private function createMigrationTable() {
+	private function createMigrationTable(): bool {
 		if ($this->migrationTableCreated) {
 			return false;
 		}
@@ -176,7 +159,7 @@ class MigrationService {
 	/**
 	 * Returns all versions which have already been applied
 	 *
-	 * @return string[]
+	 * @return list<string>
 	 * @codeCoverageIgnore - no need to test this
 	 */
 	public function getMigratedVersions() {
@@ -188,24 +171,44 @@ class MigrationService {
 			->where($qb->expr()->eq('app', $qb->createNamedParameter($this->getApp())))
 			->orderBy('version');
 
-		$result = $qb->execute();
+		$result = $qb->executeQuery();
 		$rows = $result->fetchAll(\PDO::FETCH_COLUMN);
 		$result->closeCursor();
 
+		usort($rows, $this->sortMigrations(...));
+
 		return $rows;
 	}
 
 	/**
 	 * Returns all versions which are available in the migration folder
-	 *
-	 * @return array
+	 * @return list<string>
 	 */
-	public function getAvailableVersions() {
+	public function getAvailableVersions(): array {
 		$this->ensureMigrationsAreLoaded();
-		return array_map('strval', array_keys($this->migrations));
+		$versions = array_map('strval', array_keys($this->migrations));
+		usort($versions, $this->sortMigrations(...));
+		return $versions;
 	}
 
-	protected function findMigrations() {
+	protected function sortMigrations(string $a, string $b): int {
+		preg_match('/(\d+)Date(\d+)/', basename($a), $matchA);
+		preg_match('/(\d+)Date(\d+)/', basename($b), $matchB);
+		if (!empty($matchA) && !empty($matchB)) {
+			$versionA = (int)$matchA[1];
+			$versionB = (int)$matchB[1];
+			if ($versionA !== $versionB) {
+				return ($versionA < $versionB) ? -1 : 1;
+			}
+			return strnatcmp($matchA[2], $matchB[2]);
+		}
+		return strnatcmp(basename($a), basename($b));
+	}
+
+	/**
+	 * @return array<string, string>
+	 */
+	protected function findMigrations(): array {
 		$directory = realpath($this->migrationsPath);
 		if ($directory === false || !file_exists($directory) || !is_dir($directory)) {
 			return [];
@@ -220,23 +223,13 @@ class MigrationService {
 			\RegexIterator::GET_MATCH);
 
 		$files = array_keys(iterator_to_array($iterator));
-		uasort($files, function ($a, $b) {
-			preg_match('/^Version(\d+)Date(\d+)\\.php$/', basename($a), $matchA);
-			preg_match('/^Version(\d+)Date(\d+)\\.php$/', basename($b), $matchB);
-			if (!empty($matchA) && !empty($matchB)) {
-				if ($matchA[1] !== $matchB[1]) {
-					return ($matchA[1] < $matchB[1]) ? -1 : 1;
-				}
-				return ($matchA[2] < $matchB[2]) ? -1 : 1;
-			}
-			return (basename($a) < basename($b)) ? -1 : 1;
-		});
+		usort($files, $this->sortMigrations(...));
 
 		$migrations = [];
 
 		foreach ($files as $file) {
 			$className = basename($file, '.php');
-			$version = (string) substr($className, 7);
+			$version = (string)substr($className, 7);
 			if ($version === '0') {
 				throw new \InvalidArgumentException(
 					"Cannot load a migrations with the name '$version' because it is a reserved number"
@@ -258,7 +251,7 @@ class MigrationService {
 
 		$toBeExecuted = [];
 		foreach ($availableMigrations as $v) {
-			if ($to !== 'latest' && $v > $to) {
+			if ($to !== 'latest' && ($this->sortMigrations($v, $to) > 0)) {
 				continue;
 			}
 			if ($this->shallBeExecuted($v, $knownMigrations)) {
@@ -322,10 +315,9 @@ class MigrationService {
 	/**
 	 * Return the explicit version for the aliases; current, next, prev, latest
 	 *
-	 * @param string $alias
 	 * @return mixed|null|string
 	 */
-	public function getMigration($alias) {
+	public function getMigration(string $alias) {
 		switch ($alias) {
 			case 'current':
 				return $this->getCurrentVersion();
@@ -342,29 +334,22 @@ class MigrationService {
 		return '0';
 	}
 
-	/**
-	 * @param string $version
-	 * @param int $delta
-	 * @return null|string
-	 */
-	private function getRelativeVersion($version, $delta) {
+	private function getRelativeVersion(string $version, int $delta): ?string {
 		$this->ensureMigrationsAreLoaded();
 
 		$versions = $this->getAvailableVersions();
-		array_unshift($versions, 0);
+		array_unshift($versions, '0');
+		/** @var int $offset */
 		$offset = array_search($version, $versions, true);
 		if ($offset === false || !isset($versions[$offset + $delta])) {
 			// Unknown version or delta out of bounds.
 			return null;
 		}
 
-		return (string) $versions[$offset + $delta];
+		return (string)$versions[$offset + $delta];
 	}
 
-	/**
-	 * @return string
-	 */
-	private function getCurrentVersion() {
+	private function getCurrentVersion(): string {
 		$m = $this->getMigratedVersions();
 		if (count($m) === 0) {
 			return '0';
@@ -374,11 +359,9 @@ class MigrationService {
 	}
 
 	/**
-	 * @param string $version
-	 * @return string
 	 * @throws \InvalidArgumentException
 	 */
-	private function getClass($version) {
+	private function getClass(string $version): string {
 		$this->ensureMigrationsAreLoaded();
 
 		if (isset($this->migrations[$version])) {
@@ -390,22 +373,18 @@ class MigrationService {
 
 	/**
 	 * Allows to set an IOutput implementation which is used for logging progress and messages
-	 *
-	 * @param IOutput $output
 	 */
-	public function setOutput(IOutput $output) {
+	public function setOutput(IOutput $output): void {
 		$this->output = $output;
 	}
 
 	/**
 	 * Applies all not yet applied versions up to $to
-	 *
-	 * @param string $to
-	 * @param bool $schemaOnly
 	 * @throws \InvalidArgumentException
 	 */
-	public function migrate($to = 'latest', $schemaOnly = false) {
+	public function migrate(string $to = 'latest', bool $schemaOnly = false): void {
 		if ($schemaOnly) {
+			$this->output->debug('Migrating schema only');
 			$this->migrateSchemaOnly($to);
 			return;
 		}
@@ -418,18 +397,16 @@ class MigrationService {
 			} catch (\Exception $e) {
 				// The exception itself does not contain the name of the migration,
 				// so we wrap it here, to make debugging easier.
-				throw new \Exception('Database error when running migration ' . $version . ' for app ' . $this->getApp() . PHP_EOL. $e->getMessage(), 0, $e);
+				throw new \Exception('Database error when running migration ' . $version . ' for app ' . $this->getApp() . PHP_EOL . $e->getMessage(), 0, $e);
 			}
 		}
 	}
 
 	/**
 	 * Applies all not yet applied versions up to $to
-	 *
-	 * @param string $to
 	 * @throws \InvalidArgumentException
 	 */
-	public function migrateSchemaOnly($to = 'latest') {
+	public function migrateSchemaOnly(string $to = 'latest'): void {
 		// read known migrations
 		$toBeExecuted = $this->getMigrationsToExecute($to);
 
@@ -439,6 +416,7 @@ class MigrationService {
 
 		$toSchema = null;
 		foreach ($toBeExecuted as $version) {
+			$this->output->debug('- Reading ' . $version);
 			$instance = $this->createInstance($version);
 
 			$toSchema = $instance->changeSchema($this->output, function () use ($toSchema): ISchemaWrapper {
@@ -447,15 +425,20 @@ class MigrationService {
 		}
 
 		if ($toSchema instanceof SchemaWrapper) {
+			$this->output->debug('- Checking target database schema');
 			$targetSchema = $toSchema->getWrappedSchema();
+			$this->ensureUniqueNamesConstraints($targetSchema, true);
 			if ($this->checkOracle) {
 				$beforeSchema = $this->connection->createSchema();
 				$this->ensureOracleConstraints($beforeSchema, $targetSchema, strlen($this->connection->getPrefix()));
 			}
+
+			$this->output->debug('- Migrate database schema');
 			$this->connection->migrateToSchema($targetSchema);
 			$toSchema->performDropTableCalls();
 		}
 
+		$this->output->debug('- Mark migrations as executed');
 		foreach ($toBeExecuted as $version) {
 			$this->markAsExecuted($version);
 		}
@@ -484,10 +467,10 @@ class MigrationService {
 	 * @return IMigrationStep
 	 * @throws \InvalidArgumentException
 	 */
-	protected function createInstance($version) {
+	public function createInstance($version) {
 		$class = $this->getClass($version);
 		try {
-			$s = \OC::$server->query($class);
+			$s = \OCP\Server::get($class);
 
 			if (!$s instanceof IMigrationStep) {
 				throw new \InvalidArgumentException('Not a valid migration');
@@ -525,6 +508,7 @@ class MigrationService {
 
 		if ($toSchema instanceof SchemaWrapper) {
 			$targetSchema = $toSchema->getWrappedSchema();
+			$this->ensureUniqueNamesConstraints($targetSchema, $schemaOnly);
 			if ($this->checkOracle) {
 				$sourceSchema = $this->connection->createSchema();
 				$this->ensureOracleConstraints($sourceSchema, $targetSchema, strlen($this->connection->getPrefix()));
@@ -623,7 +607,7 @@ class MigrationService {
 				$indexName = strtolower($primaryKey->getName());
 				$isUsingDefaultName = $indexName === 'primary';
 
-				if ($this->connection->getDatabasePlatform() instanceof PostgreSQL94Platform) {
+				if ($this->connection->getDatabaseProvider() === IDBConnection::PLATFORM_POSTGRES) {
 					$defaultName = $table->getName() . '_pkey';
 					$isUsingDefaultName = strtolower($defaultName) === $indexName;
 
@@ -633,7 +617,7 @@ class MigrationService {
 							return $sequence->getName() !== $sequenceName;
 						});
 					}
-				} elseif ($this->connection->getDatabasePlatform() instanceof OraclePlatform) {
+				} elseif ($this->connection->getDatabaseProvider() === IDBConnection::PLATFORM_ORACLE) {
 					$defaultName = $table->getName() . '_seq';
 					$isUsingDefaultName = strtolower($defaultName) === $indexName;
 				}
@@ -659,6 +643,91 @@ class MigrationService {
 		}
 	}
 
+	/**
+	 * Ensure naming constraints
+	 *
+	 * Naming constraints:
+	 * - Index, sequence and primary key names must be unique within a Postgres Schema
+	 *
+	 * Only on installation we want to break hard, so that all developers notice
+	 * the bugs when installing the app on any database or CI, and can work on
+	 * fixing their migrations before releasing a version incompatible with Postgres.
+	 *
+	 * In case of updates we might be running on production instances and the
+	 * administrators being faced with the error would not know how to resolve it
+	 * anyway. This can also happen with instances, that had the issue before the
+	 * current update, so we don't want to make their life more complicated
+	 * than needed.
+	 *
+	 * @param Schema $targetSchema
+	 * @param bool $isInstalling
+	 */
+	public function ensureUniqueNamesConstraints(Schema $targetSchema, bool $isInstalling): void {
+		$constraintNames = [];
+		$sequences = $targetSchema->getSequences();
+
+		foreach ($targetSchema->getTables() as $table) {
+			foreach ($table->getIndexes() as $thing) {
+				$indexName = strtolower($thing->getName());
+				if ($indexName === 'primary' || $thing->isPrimary()) {
+					continue;
+				}
+
+				if (isset($constraintNames[$thing->getName()])) {
+					if ($isInstalling) {
+						throw new \InvalidArgumentException('Index name "' . $thing->getName() . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+					}
+					$this->logErrorOrWarning('Index name "' . $thing->getName() . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+				}
+				$constraintNames[$thing->getName()] = $table->getName();
+			}
+
+			foreach ($table->getForeignKeys() as $thing) {
+				if (isset($constraintNames[$thing->getName()])) {
+					if ($isInstalling) {
+						throw new \InvalidArgumentException('Foreign key name "' . $thing->getName() . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+					}
+					$this->logErrorOrWarning('Foreign key name "' . $thing->getName() . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+				}
+				$constraintNames[$thing->getName()] = $table->getName();
+			}
+
+			$primaryKey = $table->getPrimaryKey();
+			if ($primaryKey instanceof Index) {
+				$indexName = strtolower($primaryKey->getName());
+				if ($indexName === 'primary') {
+					continue;
+				}
+
+				if (isset($constraintNames[$indexName])) {
+					if ($isInstalling) {
+						throw new \InvalidArgumentException('Primary index name "' . $indexName . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+					}
+					$this->logErrorOrWarning('Primary index name "' . $indexName . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+				}
+				$constraintNames[$indexName] = $table->getName();
+			}
+		}
+
+		foreach ($sequences as $sequence) {
+			if (isset($constraintNames[$sequence->getName()])) {
+				if ($isInstalling) {
+					throw new \InvalidArgumentException('Sequence name "' . $sequence->getName() . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+				}
+				$this->logErrorOrWarning('Sequence name "' . $sequence->getName() . '" for table "' . $table->getName() . '" collides with the constraint on table "' . $constraintNames[$thing->getName()] . '".');
+			}
+			$constraintNames[$sequence->getName()] = 'sequence';
+		}
+	}
+
+	protected function logErrorOrWarning(string $log): void {
+		if ($this->output instanceof SimpleOutput) {
+			$this->output->warning($log);
+		} else {
+			$this->logger->error($log);
+		}
+	}
+
 	private function ensureMigrationsAreLoaded() {
 		if (empty($this->migrations)) {
 			$this->migrations = $this->findMigrations();