Merge pull request #35539 from nextcloud/cleanup/carl/application-contactsmanager

Cleanup psalm issues in DB/ContactsManager and Console

lib/private/Console/Application.php

@@ -47,17 +47,12 @@ use Symfony\Component\Console\Output\ConsoleOutputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class Application {
-	/** @var IConfig */
-	private $config;
+	private IConfig $config;
 	private SymfonyApplication $application;
-	/** @var IEventDispatcher */
-	private $dispatcher;
-	/** @var IRequest */
-	private $request;
-	/** @var LoggerInterface */
-	private $logger;
-	/** @var MemoryInfo */
-	private $memoryInfo;
+	private IEventDispatcher $dispatcher;
+	private IRequest $request;
+	private LoggerInterface $logger;
+	private MemoryInfo $memoryInfo;
 
 	public function __construct(IConfig $config,
 								IEventDispatcher $dispatcher,
@@ -74,8 +69,6 @@ class Application {
 	}
 
 	/**
-	 * @param InputInterface $input
-	 * @param ConsoleOutputInterface $output
 	 * @throws \Exception
 	 */
 	public function loadCommands(

lib/private/ContactsManager.php

@@ -91,20 +91,20 @@ class ContactsManager implements IManager {
 	 * This function can be used to delete the contact identified by the given id
 	 *
 	 * @param int $id the unique identifier to a contact
-	 * @param string $address_book_key identifier of the address book in which the contact shall be deleted
+	 * @param string $addressBookKey identifier of the address book in which the contact shall be deleted
 	 * @return bool successful or not
 	 */
-	public function delete($id, $address_book_key) {
-		$addressBook = $this->getAddressBook($address_book_key);
+	public function delete($id, $addressBookKey) {
+		$addressBook = $this->getAddressBook($addressBookKey);
 		if (!$addressBook) {
-			return null;
+			return false;
 		}
 
 		if ($addressBook->getPermissions() & Constants::PERMISSION_DELETE) {
 			return $addressBook->delete($id);
 		}
 
-		return null;
+		return false;
 	}
 
 	/**
@@ -112,11 +112,11 @@ class ContactsManager implements IManager {
 	 * Otherwise the contact will be updated by replacing the entire data set.
 	 *
 	 * @param array $properties this array if key-value-pairs defines a contact
-	 * @param string $address_book_key identifier of the address book in which the contact shall be created or updated
-	 * @return array representing the contact just created or updated
+	 * @param string $addressBookKey identifier of the address book in which the contact shall be created or updated
+	 * @return ?array representing the contact just created or updated
 	 */
-	public function createOrUpdate($properties, $address_book_key) {
-		$addressBook = $this->getAddressBook($address_book_key);
+	public function createOrUpdate($properties, $addressBookKey) {
+		$addressBook = $this->getAddressBook($addressBookKey);
 		if (!$addressBook) {
 			return null;
 		}
@@ -133,7 +133,7 @@ class ContactsManager implements IManager {
 	 *
 	 * @return bool true if enabled, false if not
 	 */
-	public function isEnabled() {
+	public function isEnabled(): bool {
 		return !empty($this->addressBooks) || !empty($this->addressBookLoaders);
 	}
 
@@ -192,11 +192,8 @@ class ContactsManager implements IManager {
 
 	/**
 	 * Get (and load when needed) the address book for $key
-	 *
-	 * @param string $addressBookKey
-	 * @return IAddressBook
 	 */
-	protected function getAddressBook($addressBookKey) {
+	protected function getAddressBook(string $addressBookKey): ?IAddressBook {
 		$this->loadAddressBooks();
 		if (!array_key_exists($addressBookKey, $this->addressBooks)) {
 			return null;

lib/private/DB/Adapter.php

@@ -30,6 +30,7 @@ namespace OC\DB;
 
 use Doctrine\DBAL\Exception;
 use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
+use OC\DB\Exceptions\DbalException;
 
 /**
  * This handles the way we use to write queries, into something that can be
@@ -142,9 +143,12 @@ class Adapter {
 			foreach ($values as $key => $value) {
 				$builder->setValue($key, $builder->createNamedParameter($value));
 			}
-			return $builder->execute();
-		} catch (UniqueConstraintViolationException $e) {
-			return 0;
+			return $builder->executeStatement();
+		} catch (DbalException $e) {
+			if ($e->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+				return 0;
+			}
+			throw $e;
 		}
 	}
 }

lib/private/DB/Connection.php

@@ -220,7 +220,7 @@ class Connection extends \Doctrine\DBAL\Connection {
 	 * @return Statement The prepared statement.
 	 * @throws Exception
 	 */
-	public function prepare($statement, $limit = null, $offset = null): Statement {
+	public function prepare($sql, $limit = null, $offset = null): Statement {
 		if ($limit === -1 || $limit === null) {
 			$limit = null;
 		} else {
@@ -231,9 +231,9 @@ class Connection extends \Doctrine\DBAL\Connection {
 		}
 		if (!is_null($limit)) {
 			$platform = $this->getDatabasePlatform();
-			$statement = $platform->modifyLimitQuery($statement, $limit, $offset);
+			$sql = $platform->modifyLimitQuery($sql, $limit, $offset);
 		}
-		$statement = $this->replaceTablePrefix($statement);
+		$statement = $this->replaceTablePrefix($sql);
 		$statement = $this->adapter->fixupStatement($statement);
 
 		return parent::prepare($statement);
@@ -321,14 +321,14 @@ class Connection extends \Doctrine\DBAL\Connection {
 	 *
 	 * @param string $seqName Name of the sequence object from which the ID should be returned.
 	 *
-	 * @return string the last inserted ID.
+	 * @return int the last inserted ID.
 	 * @throws Exception
 	 */
-	public function lastInsertId($seqName = null) {
-		if ($seqName) {
-			$seqName = $this->replaceTablePrefix($seqName);
+	public function lastInsertId($name = null): int {
+		if ($name) {
+			$name = $this->replaceTablePrefix($name);
 		}
-		return $this->adapter->lastInsertId($seqName);
+		return $this->adapter->lastInsertId($name);
 	}
 
 	/**

lib/private/DB/ConnectionAdapter.php

@@ -87,7 +87,7 @@ class ConnectionAdapter implements IDBConnection {
 
 	public function lastInsertId(string $table): int {
 		try {
-			return (int)$this->inner->lastInsertId($table);
+			return $this->inner->lastInsertId($table);
 		} catch (Exception $e) {
 			throw DbalException::wrap($e);
 		}

lib/private/DB/MigrationService.php

@@ -58,7 +58,7 @@ 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) {
 		$this->appName = $appName;
 		$this->connection = $connection;
 		if ($output === null) {
@@ -100,18 +100,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;
 		}
@@ -188,7 +185,7 @@ 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();
 
@@ -197,15 +194,17 @@ class MigrationService {
 
 	/**
 	 * 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));
 	}
 
-	protected function findMigrations() {
+	/**
+	 * @return array<string, string>
+	 */
+	protected function findMigrations(): array {
 		$directory = realpath($this->migrationsPath);
 		if ($directory === false || !file_exists($directory) || !is_dir($directory)) {
 			return [];
@@ -322,10 +321,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 +340,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 +365,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,21 +379,16 @@ 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->migrateSchemaOnly($to);
 			return;
@@ -425,11 +409,9 @@ class MigrationService {
 
 	/**
 	 * 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);
 

lib/private/DB/OracleConnection.php

@@ -29,6 +29,8 @@ namespace OC\DB;
 class OracleConnection extends Connection {
 	/**
 	 * Quote the keys of the array
+	 * @param array<string, string> $data
+	 * @return array<string, string>
 	 */
 	private function quoteKeys(array $data) {
 		$return = [];

lib/public/Contacts/IManager.php

@@ -107,22 +107,22 @@ interface IManager {
 	 * This function can be used to delete the contact identified by the given id
 	 *
 	 * @param int $id the unique identifier to a contact
-	 * @param string $address_book_key identifier of the address book in which the contact shall be deleted
+	 * @param string $addressBookKey identifier of the address book in which the contact shall be deleted
 	 * @return bool successful or not
 	 * @since 6.0.0
 	 */
-	public function delete($id, $address_book_key);
+	public function delete($id, $addressBookKey);
 
 	/**
 	 * This function is used to create a new contact if 'id' is not given or not present.
 	 * Otherwise the contact will be updated by replacing the entire data set.
 	 *
 	 * @param array $properties this array if key-value-pairs defines a contact
-	 * @param string $address_book_key identifier of the address book in which the contact shall be created or updated
-	 * @return array an array representing the contact just created or updated
+	 * @param string $addressBookKey identifier of the address book in which the contact shall be created or updated
+	 * @return ?array an array representing the contact just created or updated
 	 * @since 6.0.0
 	 */
-	public function createOrUpdate($properties, $address_book_key);
+	public function createOrUpdate($properties, $addressBookKey);
 
 	/**
 	 * Check if contacts are available (e.g. contacts app enabled)
@@ -135,20 +135,19 @@ interface IManager {
 	/**
 	 * Registers an address book
 	 *
-	 * @param \OCP\IAddressBook $address_book
 	 * @return void
 	 * @since 6.0.0
 	 */
-	public function registerAddressBook(\OCP\IAddressBook $address_book);
+	public function registerAddressBook(\OCP\IAddressBook $addressBook);
 
 	/**
 	 * Unregisters an address book
 	 *
-	 * @param \OCP\IAddressBook $address_book
+	 * @param \OCP\IAddressBook $addressBook
 	 * @return void
 	 * @since 6.0.0
 	 */
-	public function unregisterAddressBook(\OCP\IAddressBook $address_book);
+	public function unregisterAddressBook(\OCP\IAddressBook $addressBook);
 
 	/**
 	 * In order to improve lazy loading a closure can be registered which will be called in case

tests/lib/ContactsManagerTest.php

@@ -109,6 +109,9 @@ class ContactsManagerTest extends \Test\TestCase {
 			->method('delete')
 			->willReturn('returnMe');
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
 
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->delete(1, $addressbook->getKey());
@@ -128,6 +131,10 @@ class ContactsManagerTest extends \Test\TestCase {
 		$addressbook->expects($this->never())
 			->method('delete');
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
+
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->delete(1, $addressbook->getKey());
 		$this->assertEquals($result, null);
@@ -142,6 +149,10 @@ class ContactsManagerTest extends \Test\TestCase {
 		$addressbook->expects($this->never())
 			->method('delete');
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
+
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->delete(1, 'noaddressbook');
 		$this->assertEquals($result, null);
@@ -161,6 +172,10 @@ class ContactsManagerTest extends \Test\TestCase {
 			->method('createOrUpdate')
 			->willReturn('returnMe');
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
+
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->createOrUpdate([], $addressbook->getKey());
 		$this->assertEquals($result, 'returnMe');
@@ -179,6 +194,10 @@ class ContactsManagerTest extends \Test\TestCase {
 		$addressbook->expects($this->never())
 			->method('createOrUpdate');
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
+
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->createOrUpdate([], $addressbook->getKey());
 		$this->assertEquals($result, null);
@@ -193,6 +212,10 @@ class ContactsManagerTest extends \Test\TestCase {
 		$addressbook->expects($this->never())
 			->method('createOrUpdate');
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
+
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->createOrUpdate([], 'noaddressbook');
 		$this->assertEquals($result, null);
@@ -209,6 +232,10 @@ class ContactsManagerTest extends \Test\TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 
+		$addressbook->expects($this->any())
+			->method('getKey')
+			->willReturn('addressbookKey');
+
 		$this->cm->registerAddressBook($addressbook);
 		$result = $this->cm->isEnabled();
 		$this->assertTrue($result);