diff --git a/lib/Db/MountPointRequest.php b/lib/Db/MountPointRequest.php deleted file mode 100644 index c7d1f368a..000000000 --- a/lib/Db/MountPointRequest.php +++ /dev/null @@ -1,59 +0,0 @@ -getMountPointInsertSql(); - - $hash = ($mountpoint->getMountPoint() === '-') ? '' : md5($mountpoint->getMountPoint()); - $qb->setValue('mountpoint', $qb->createNamedParameter($mountpoint->getMountPoint())) - ->setValue('mountpoint_hash', $qb->createNamedParameter($hash)) - ->setValue('mount_id', $qb->createNamedParameter($mountpoint->getMountId())) - ->setValue('single_id', $qb->createNamedParameter($mountpoint->getSingleId())); - $qb->executeStatement(); - } - - /** - * @param Mountpoint $mountpoint - * @throws MountNotFoundException - */ - public function update(Mountpoint $mountpoint): void { - $qb = $this->getMountPointUpdateSql(); - - $hash = ($mountpoint->getMountPoint() === '-') ? '' : md5($mountpoint->getMountPoint()); - - $qb->set('mountpoint', $qb->createNamedParameter($mountpoint->getMountPoint())) - ->set('mountpoint_hash', $qb->createNamedParameter($hash)); - $qb->limit('mount_id', $mountpoint->getMountId()); - $qb->limitToSingleId($mountpoint->getSingleId()); - $nb = $qb->executeStatement(); - - if ($nb === 0) { - throw new MountNotFoundException('Mount not found'); - } - } - - public function deleteByMountId(string $mountId): void { - $qb = $this->getMountPointDeleteSql(); - - $qb->limit('mount_id', $mountId); - $qb->executeStatement(); - } -} diff --git a/lib/Db/MountPointRequestBuilder.php b/lib/Db/MountPointRequestBuilder.php deleted file mode 100644 index 35e22d4ce..000000000 --- a/lib/Db/MountPointRequestBuilder.php +++ /dev/null @@ -1,76 +0,0 @@ -getQueryBuilder(); - $qb->insert(self::TABLE_MOUNTPOINT); - - return $qb; - } - - /** - * @return CoreQueryBuilder&IQueryBuilder - */ - protected function getMountPointUpdateSql(): CoreQueryBuilder { - $qb = $this->getQueryBuilder(); - $qb->update(self::TABLE_MOUNTPOINT); - - return $qb; - } - - /** - * @return CoreQueryBuilder&IQueryBuilder - */ - protected function getMountPointSelectSql(string $alias = CoreQueryBuilder::MOUNTPOINT): CoreQueryBuilder { - $qb = $this->getQueryBuilder(); - $qb->generateSelect(self::TABLE_MOUNTPOINT, self::$tables[self::TABLE_MOUNTPOINT], $alias); - - return $qb; - } - - /** - * @return CoreQueryBuilder&IQueryBuilder - */ - protected function getMountPointDeleteSql(): CoreQueryBuilder { - $qb = $this->getQueryBuilder(); - $qb->delete(self::TABLE_MOUNTPOINT); - - return $qb; - } - - public function getItemFromRequest(CoreQueryBuilder $qb): MountPoint { - try { - $mountpoint = $qb->asItem(MountPoint::class); - } catch (RowNotFoundException) { - throw new MountNotFoundException('Mount not found'); - } - - return $mountpoint; - } - - /** - * @param CoreQueryBuilder $qb - * - * @return list - */ - public function getItemsFromRequest(CoreQueryBuilder $qb): array { - return $qb->asItems(MountPoint::class); - } -} diff --git a/lib/Db/MountRequest.php b/lib/Db/MountRequest.php index 6a1216edb..af1b6eee8 100644 --- a/lib/Db/MountRequest.php +++ b/lib/Db/MountRequest.php @@ -12,6 +12,7 @@ use OCA\Circles\Exceptions\RequestBuilderException; use OCA\Circles\IFederatedUser; use OCA\Circles\Model\Mount; +use OCA\Circles\Repository\MountpointRepository; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\TimezoneService; use OCA\Circles\Tools\Traits\TStringTools; @@ -28,7 +29,7 @@ class MountRequest extends MountRequestBuilder { public function __construct( TimezoneService $timezoneService, ConfigService $configService, - private MountPointRequest $mountPointRequest, + private MountpointRepository $mountpointRepository, ) { parent::__construct($timezoneService, $configService); } @@ -61,7 +62,7 @@ public function delete(string $token): void { $qb->executeStatement(); foreach ($mounts as $mount) { - $this->mountPointRequest->deleteByMountId($mount->getMountId()); + $this->mountpointRepository->deleteBy(['mountId' => $mount->getMountId()]); } } @@ -83,7 +84,7 @@ public function deleteByCircleId(string $circleId): void { $qb->executeStatement(); foreach ($mounts as $mount) { - $this->mountPointRequest->deleteByMountId($mount->getMountId()); + $this->mountpointRepository->deleteBy(['mountId' => $mount->getMountId()]); } } diff --git a/lib/Entity/Mountpoint.php b/lib/Entity/Mountpoint.php new file mode 100644 index 000000000..e4328325f --- /dev/null +++ b/lib/Entity/Mountpoint.php @@ -0,0 +1,49 @@ + $this->mountId, + 'singleId' => $this->singleId, + 'mountPoint' => $this->mountPoint, + ]; + } +} diff --git a/lib/Exceptions/MountPointNotFoundException.php b/lib/Exceptions/MountPointNotFoundException.php deleted file mode 100644 index d1b4bccc2..000000000 --- a/lib/Exceptions/MountPointNotFoundException.php +++ /dev/null @@ -1,13 +0,0 @@ -connection->getQueryBuilder(); + $delete->delete('circles_mountpoint') + ->where($delete->expr()->orX( + $delete->expr()->isNull('mount_id'), + $delete->expr()->eq('mount_id', $delete->createNamedParameter('')), + $delete->expr()->isNull('single_id'), + $delete->expr()->eq('single_id', $delete->createNamedParameter('')), + )); + $delete->executeStatement(); + + // A few installs may have raced two writers into inserting two rows for the same + // (mount_id, single_id) before this constraint existed; keep the oldest one. + $select = $this->connection->getQueryBuilder(); + $select->select('id', 'mount_id', 'single_id') + ->from('circles_mountpoint') + ->orderBy('id', 'ASC'); + $result = $select->executeQuery(); + + $seen = []; + $duplicateIds = []; + while ($row = $result->fetch()) { + $key = $row['mount_id'] . "\0" . $row['single_id']; + if (isset($seen[$key])) { + $duplicateIds[] = (int)$row['id']; + continue; + } + $seen[$key] = true; + } + $result->closeCursor(); + + foreach ($duplicateIds as $id) { + $delete = $this->connection->getQueryBuilder(); + $delete->delete('circles_mountpoint') + ->where($delete->expr()->eq('id', $delete->createNamedParameter($id, IQueryBuilder::PARAM_INT))); + $delete->executeStatement(); + } + } + + #[Override] + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + if (!$schema->hasTable('circles_mountpoint')) { + return null; + } + + $table = $schema->getTable('circles_mountpoint'); + $table->modifyColumn('mount_id', ['notnull' => true, 'default' => '']); + $table->modifyColumn('single_id', ['notnull' => true, 'default' => '']); + + if ($table->hasIndex('circles_mountpoint_ms')) { + $table->dropIndex('circles_mountpoint_ms'); + } + + if ($table->hasPrimaryKey()) { + $table->dropPrimaryKey(); + } + + if ($table->hasColumn('id')) { + $table->dropColumn('id'); + } + + $table->setPrimaryKey(['mount_id', 'single_id']); + + return $schema; + } +} diff --git a/lib/Model/ModelManager.php b/lib/Model/ModelManager.php index e6aeb2419..dd9bf5b02 100644 --- a/lib/Model/ModelManager.php +++ b/lib/Model/ModelManager.php @@ -15,6 +15,7 @@ use OCA\Circles\Db\MemberRequest; use OCA\Circles\Db\MembershipRequest; use OCA\Circles\Entity\CircleInvitation; +use OCA\Circles\Entity\Mountpoint; use OCA\Circles\Entity\ShareToken; use OCA\Circles\Exceptions\CircleNotFoundException; use OCA\Circles\Exceptions\FederatedItemException; @@ -22,7 +23,6 @@ use OCA\Circles\Exceptions\FileCacheNotFoundException; use OCA\Circles\Exceptions\MemberNotFoundException; use OCA\Circles\Exceptions\MembershipNotFoundException; -use OCA\Circles\Exceptions\MountPointNotFoundException; use OCA\Circles\Exceptions\OwnerNotFoundException; use OCA\Circles\Exceptions\RemoteInstanceException; use OCA\Circles\Exceptions\RemoteNotFoundException; @@ -457,12 +457,15 @@ private function importIntoMount( break; case CoreQueryBuilder::MOUNTPOINT: - try { - $mountPoint = new Mountpoint(); - $mountPoint->importFromDatabase($data, $prefix); - $mount->setAlternateMountPoint($mountPoint); - } catch (MountPointNotFoundException) { + if (($data[$prefix . 'mountpoint'] ?? '') === '') { + break; } + + $mountPoint = new Mountpoint(); + $mountPoint->mountId = $data[$prefix . 'mount_id'] ?? ''; + $mountPoint->singleId = $data[$prefix . 'single_id'] ?? ''; + $mountPoint->mountPoint = $data[$prefix . 'mountpoint'] ?? ''; + $mount->setAlternateMountPoint($mountPoint); break; } } diff --git a/lib/Model/Mount.php b/lib/Model/Mount.php index 9c2d967d1..ac8a628ac 100644 --- a/lib/Model/Mount.php +++ b/lib/Model/Mount.php @@ -10,6 +10,7 @@ namespace OCA\Circles\Model; use JsonSerializable; +use OCA\Circles\Entity\Mountpoint; use OCA\Circles\Exceptions\CircleNotFoundException; use OCA\Circles\Tools\Db\IQueryRow; use OCA\Circles\Tools\IDeserializable; @@ -107,7 +108,7 @@ public function setCircleId(string $circleId): self { * @return string */ public function getMountPoint(bool $raw = true): string { - $mountPoint = $this->getAlternateMountPoint()?->getMountPoint() ?? $this->getOriginalMountPoint(); + $mountPoint = $this->getAlternateMountPoint()?->mountPoint ?? $this->getOriginalMountPoint(); if ($raw) { return $mountPoint; } diff --git a/lib/Model/Mountpoint.php b/lib/Model/Mountpoint.php deleted file mode 100644 index e1eab5e7d..000000000 --- a/lib/Model/Mountpoint.php +++ /dev/null @@ -1,80 +0,0 @@ -mountId; - } - - public function setMountId(string $mountId): self { - $this->mountId = $mountId; - - return $this; - } - - public function getSingleId(): string { - return $this->singleId; - } - - public function setSingleId(string $singleId): self { - $this->singleId = $singleId; - - return $this; - } - - public function getMountPoint(): string { - return $this->mountPoint; - } - public function setMountPoint(string $mountPoint): self { - $this->mountPoint = $mountPoint; - - return $this; - } - - public function importFromDatabase(array $data, string $prefix = ''): IQueryRow { - if ($this->get($prefix . 'mountpoint', $data) === '') { - throw new MountPointNotFoundException(); - } - - $this->setMountId($this->get($prefix . 'mount_id', $data)); - $this->setSingleId($this->get($prefix . 'single_id', $data)); - $this->setMountPoint($this->get($prefix . 'mountpoint', $data)); - - return $this; - } - - public function jsonSerialize(): array { - return [ - 'mountId' => $this->getMountId(), - 'singleId' => $this->getSingleId(), - 'mountPoint' => $this->getMountPoint(), - ]; - } -} diff --git a/lib/MountManager/CircleMountProvider.php b/lib/MountManager/CircleMountProvider.php index 6a4bf47ab..4555b02d6 100644 --- a/lib/MountManager/CircleMountProvider.php +++ b/lib/MountManager/CircleMountProvider.php @@ -8,8 +8,8 @@ namespace OCA\Circles\MountManager; -use OCA\Circles\Db\MountPointRequest; use OCA\Circles\Db\MountRequest; +use OCA\Circles\Entity\Mountpoint; use OCA\Circles\Exceptions\FederatedUserException; use OCA\Circles\Exceptions\FederatedUserNotFoundException; use OCA\Circles\Exceptions\InitiatorNotFoundException; @@ -21,7 +21,7 @@ use OCA\Circles\IFederatedUser; use OCA\Circles\Model\Member; use OCA\Circles\Model\Mount; -use OCA\Circles\Model\Mountpoint; +use OCA\Circles\Repository\MountpointRepository; use OCA\Circles\Service\ConfigService; use OCA\Circles\Service\FederatedUserService; use OCA\Circles\Tools\Traits\TArrayTools; @@ -53,7 +53,7 @@ public function __construct( private IRootFolder $rootFolder, private ICloudIdManager $cloudIdManager, private MountRequest $mountRequest, - private MountPointRequest $mountPointRequest, + private MountpointRepository $mountpointRepository, private FederatedUserService $federatedUserService, private ConfigService $configService, private LoggerInterface $logger, @@ -126,9 +126,12 @@ private function fixDuplicateFile(string $userId, Mount $mount): void { } $federatedUser = $this->federatedUserService->getLocalFederatedUser($userId); - $mountPoint = new Mountpoint($mount->getMountId(), $federatedUser->getSingleId(), $mount->getOriginalMountPoint()); + $mountPoint = new Mountpoint(); + $mountPoint->mountId = $mount->getMountId(); + $mountPoint->singleId = $federatedUser->getSingleId(); + $mountPoint->mountPoint = $mount->getOriginalMountPoint(); try { - $this->mountPointRequest->insert($mountPoint); + $this->mountpointRepository->insertMountpoint($mountPoint); return; } catch (Exception $e) { // meaning a duplicate mountpoint already exists, we need to set a new filename @@ -154,13 +157,16 @@ private function generateIncrementedMountpoint(Folder $fs, Mount $mount, IFedera try { $fs->get($path); } catch (NotFoundException) { - $mountPoint = new Mountpoint($mount->getMountId(), $federatedUser->getSingleId(), $path); + $mountPoint = new Mountpoint(); + $mountPoint->mountId = $mount->getMountId(); + $mountPoint->singleId = $federatedUser->getSingleId(); + $mountPoint->mountPoint = $path; $mount->setAlternateMountPoint($mountPoint); try { try { - $this->mountPointRequest->update($mountPoint); + $this->mountpointRepository->updateMountpoint($mountPoint); } catch (MountNotFoundException) { - $this->mountPointRequest->insert($mountPoint); + $this->mountpointRepository->insertMountpoint($mountPoint); } return; } catch (Exception $e) { diff --git a/lib/Repository/MountpointRepository.php b/lib/Repository/MountpointRepository.php new file mode 100644 index 000000000..b5cace2d9 --- /dev/null +++ b/lib/Repository/MountpointRepository.php @@ -0,0 +1,50 @@ + + */ +class MountpointRepository extends Repository { + public const string entityClass = Mountpoint::class; + + /** + * @throws \OCP\DB\Exception if a mountpoint with the same hash already exists for this member + */ + public function insertMountpoint(Mountpoint $mountpoint): Mountpoint { + $mountpoint->mountpointHash = self::computeHash($mountpoint->mountPoint); + + return $this->insert($mountpoint); + } + + /** + * @throws MountNotFoundException if no row matches this mountId/singleId + */ + public function updateMountpoint(Mountpoint $mountpoint): void { + $mountpoint->mountpointHash = self::computeHash($mountpoint->mountPoint); + + try { + $this->findOneBy(['mountId' => $mountpoint->mountId, 'singleId' => $mountpoint->singleId]); + } catch (DoesNotExistException) { + throw new MountNotFoundException('Mount not found'); + } + + $this->update($mountpoint); + } + + private static function computeHash(string $mountPoint): string { + return $mountPoint === '-' ? '' : md5($mountPoint); + } +} diff --git a/tests/unit/lib/Repository/MountpointRepositoryTest.php b/tests/unit/lib/Repository/MountpointRepositoryTest.php new file mode 100644 index 000000000..d702d3e3a --- /dev/null +++ b/tests/unit/lib/Repository/MountpointRepositoryTest.php @@ -0,0 +1,103 @@ +mountpointRepository = Server::get(MountpointRepository::class); + } + + protected function tearDown(): void { + $this->mountpointRepository->deleteBy(['mountId' => 'mount1234567890123456']); + + parent::tearDown(); + } + + public function testGetTableName(): void { + $this->assertSame('circles_mountpoint', $this->mountpointRepository->getTableName()); + } + + public function testEntityClassPointsToMountpointEntity(): void { + $this->assertSame(Mountpoint::class, MountpointRepository::entityClass); + } + + public function testInsertMountpointComputesHash(): void { + $mountpoint = new Mountpoint(); + $mountpoint->mountId = 'mount1234567890123456'; + $mountpoint->singleId = 'single1234567890123456'; + $mountpoint->mountPoint = 'Some Folder'; + + $inserted = $this->mountpointRepository->insertMountpoint($mountpoint); + + $this->assertSame(md5('Some Folder'), $inserted->mountpointHash); + + $stored = $this->mountpointRepository->findOneBy(['mountId' => 'mount1234567890123456']); + $this->assertSame('single1234567890123456', $stored->singleId); + $this->assertSame('Some Folder', $stored->mountPoint); + $this->assertSame(md5('Some Folder'), $stored->mountpointHash); + } + + public function testInsertMountpointWithPlaceholderGetsEmptyHash(): void { + $mountpoint = new Mountpoint(); + $mountpoint->mountId = 'mount1234567890123456'; + $mountpoint->singleId = 'single1234567890123456'; + $mountpoint->mountPoint = '-'; + + $inserted = $this->mountpointRepository->insertMountpoint($mountpoint); + + $this->assertSame('', $inserted->mountpointHash); + } + + public function testUpdateMountpointChangesMountPointAndHash(): void { + $mountpoint = new Mountpoint(); + $mountpoint->mountId = 'mount1234567890123456'; + $mountpoint->singleId = 'single1234567890123456'; + $mountpoint->mountPoint = 'Original Name'; + $this->mountpointRepository->insertMountpoint($mountpoint); + + $update = new Mountpoint(); + $update->mountId = 'mount1234567890123456'; + $update->singleId = 'single1234567890123456'; + $update->mountPoint = 'Renamed Folder'; + $this->mountpointRepository->updateMountpoint($update); + + $stored = $this->mountpointRepository->findOneBy(['mountId' => 'mount1234567890123456']); + $this->assertSame('Renamed Folder', $stored->mountPoint); + $this->assertSame(md5('Renamed Folder'), $stored->mountpointHash); + } + + public function testUpdateMountpointThrowsWhenNoRowMatched(): void { + $mountpoint = new Mountpoint(); + $mountpoint->mountId = 'mount1234567890123456'; + $mountpoint->singleId = 'single1234567890123456'; + $mountpoint->mountPoint = 'Nothing to update'; + + $this->expectException(MountNotFoundException::class); + $this->mountpointRepository->updateMountpoint($mountpoint); + } + + public function testFindOneByThrowsWhenNoRowMatched(): void { + $this->expectException(DoesNotExistException::class); + $this->mountpointRepository->findOneBy(['mountId' => 'mount1234567890123456']); + } +}