From ca33a62ec6313fad24eac84e32c9a6acbd42e7df Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 3 Oct 2024 17:35:56 +0200 Subject: [PATCH 1/2] fix: smuggle storage id to metadata insert queries Signed-off-by: Robin Appelman --- apps/dav/lib/Connector/Sabre/FilesPlugin.php | 3 +++ .../lib/Listener/SyncLivePhotosListener.php | 5 +++++ .../FilesMetadata/FilesMetadataManager.php | 5 ++++- .../FilesMetadata/Model/FilesMetadata.php | 8 ++++++++ .../Service/MetadataRequestService.php | 20 +++++++++++++++++++ 5 files changed, 40 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php index fb8f6328683f0..2db23f390c888 100644 --- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php @@ -8,6 +8,7 @@ namespace OCA\DAV\Connector\Sabre; use OC\AppFramework\Http\Request; +use OC\FilesMetadata\Model\FilesMetadata; use OCA\DAV\Connector\Sabre\Exception\InvalidPath; use OCP\Constants; use OCP\Files\ForbiddenException; @@ -581,7 +582,9 @@ private function handleUpdatePropertiesMetadata(PropPatch $propPatch, Node $node $propPatch->handle( $mutation, function (mixed $value) use ($accessRight, $knownMetadata, $node, $mutation, $filesMetadataManager): bool { + /** @var FilesMetadata $metadata */ $metadata = $filesMetadataManager->getMetadata((int)$node->getFileId(), true); + $metadata->setStorageId($node->getNode()->getStorage()->getCache()->getNumericStorageId()); $metadataKey = substr($mutation, strlen(self::FILE_METADATA_PREFIX)); // confirm metadata key is editable via PROPPATCH diff --git a/apps/files/lib/Listener/SyncLivePhotosListener.php b/apps/files/lib/Listener/SyncLivePhotosListener.php index c74a83708181e..02cf85f991708 100644 --- a/apps/files/lib/Listener/SyncLivePhotosListener.php +++ b/apps/files/lib/Listener/SyncLivePhotosListener.php @@ -8,6 +8,7 @@ namespace OCA\Files\Listener; +use OC\FilesMetadata\Model\FilesMetadata; use OCA\Files\Service\LivePhotosService; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; @@ -154,10 +155,14 @@ private function handleCopy(NodeCopiedEvent $event, Node $peerFile): void { * We have everything to update metadata and keep the link between the 2 copies. */ $newPeerFile = $peerFile->copy($targetParent->getPath() . '/' . $peerTargetName); + /** @var FilesMetadata $targetMetadata */ $targetMetadata = $this->filesMetadataManager->getMetadata($targetFile->getId(), true); + $targetMetadata->setStorageId($targetFile->getStorage()->getCache()->getNumericStorageId()); $targetMetadata->setString('files-live-photo', (string)$newPeerFile->getId()); $this->filesMetadataManager->saveMetadata($targetMetadata); + /** @var FilesMetadata $peerMetadata */ $peerMetadata = $this->filesMetadataManager->getMetadata($newPeerFile->getId(), true); + $peerMetadata->setStorageId($newPeerFile->getStorage()->getCache()->getNumericStorageId()); $peerMetadata->setString('files-live-photo', (string)$targetFile->getId()); $this->filesMetadataManager->saveMetadata($peerMetadata); } diff --git a/lib/private/FilesMetadata/FilesMetadataManager.php b/lib/private/FilesMetadata/FilesMetadataManager.php index 5c1acb77ea7b0..ed8a1f935c13c 100644 --- a/lib/private/FilesMetadata/FilesMetadataManager.php +++ b/lib/private/FilesMetadata/FilesMetadataManager.php @@ -77,10 +77,13 @@ public function refreshMetadata( int $process = self::PROCESS_LIVE, string $namedEvent = '' ): IFilesMetadata { + $storageId = $node->getStorage()->getCache()->getNumericStorageId(); try { + /** @var FilesMetadata $metadata */ $metadata = $this->metadataRequestService->getMetadataFromFileId($node->getId()); + $metadata->setStorageId($storageId); } catch (FilesMetadataNotFoundException) { - $metadata = new FilesMetadata($node->getId()); + $metadata = new FilesMetadata($node->getId(), $storageId); } // if $process is LIVE, we enforce LIVE diff --git a/lib/private/FilesMetadata/Model/FilesMetadata.php b/lib/private/FilesMetadata/Model/FilesMetadata.php index dfeb429507a3a..9bf1ca866ac01 100644 --- a/lib/private/FilesMetadata/Model/FilesMetadata.php +++ b/lib/private/FilesMetadata/Model/FilesMetadata.php @@ -42,6 +42,14 @@ public function getFileId(): int { return $this->fileId; } + public function getStorageId(): ?int { + return $this->storageId; + } + + public function setStorageId(int $storageId): void { + $this->storageId = $storageId; + } + /** * @inheritDoc * @return int timestamp diff --git a/lib/private/FilesMetadata/Service/MetadataRequestService.php b/lib/private/FilesMetadata/Service/MetadataRequestService.php index 08982a2a6592c..49cd888604622 100644 --- a/lib/private/FilesMetadata/Service/MetadataRequestService.php +++ b/lib/private/FilesMetadata/Service/MetadataRequestService.php @@ -28,6 +28,24 @@ public function __construct( ) { } + private function getStorageId(IFilesMetadata $filesMetadata): int { + if ($filesMetadata instanceof FilesMetadata && $filesMetadata->getStorageId()) { + return $filesMetadata->getStorageId(); + } + // all code paths that lead to saving metadata *should* have the storage id set + // this fallback is there just in case + $query = $this->dbConnection->getQueryBuilder(); + $query->select('storage') + ->from('filecache') + ->where($query->expr()->eq('fileid', $query->createNamedParameter($filesMetadata->getFileId(), IQueryBuilder::PARAM_INT))); + $storageId = $query->executeQuery()->fetchColumn(); + + if ($filesMetadata instanceof FilesMetadata) { + $filesMetadata->setStorageId($storageId); + } + return $storageId; + } + /** * store metadata into database * @@ -38,6 +56,7 @@ public function __construct( public function store(IFilesMetadata $filesMetadata): void { $qb = $this->dbConnection->getQueryBuilder(); $qb->insert(self::TABLE_METADATA) + ->hintShardKey('storage', $this->getStorageId($filesMetadata)) ->setValue('file_id', $qb->createNamedParameter($filesMetadata->getFileId(), IQueryBuilder::PARAM_INT)) ->setValue('json', $qb->createNamedParameter(json_encode($filesMetadata->jsonSerialize()))) ->setValue('sync_token', $qb->createNamedParameter($this->generateSyncToken())) @@ -134,6 +153,7 @@ public function updateMetadata(IFilesMetadata $filesMetadata): int { $expr = $qb->expr(); $qb->update(self::TABLE_METADATA) + ->hintShardKey('files_metadata', $this->getStorageId($filesMetadata)) ->set('json', $qb->createNamedParameter(json_encode($filesMetadata->jsonSerialize()))) ->set('sync_token', $qb->createNamedParameter($this->generateSyncToken())) ->set('last_update', $qb->createFunction('NOW()')) From be7b6a7b1b54e6ff967cf54dd305eeb15782490a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 3 Oct 2024 18:05:43 +0200 Subject: [PATCH 2/2] test: add some minimal testing for metadata storage Signed-off-by: Robin Appelman --- .../FilesMetadata/FilesMetadataManager.php | 4 +- .../FilesMetadata/Model/FilesMetadata.php | 9 ++ .../Service/MetadataRequestService.php | 7 +- .../FilesMetadataManagerTest.php | 98 +++++++++++++++++++ 4 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 tests/lib/FilesMetadata/FilesMetadataManagerTest.php diff --git a/lib/private/FilesMetadata/FilesMetadataManager.php b/lib/private/FilesMetadata/FilesMetadataManager.php index ed8a1f935c13c..04eec1c85a4f4 100644 --- a/lib/private/FilesMetadata/FilesMetadataManager.php +++ b/lib/private/FilesMetadata/FilesMetadataManager.php @@ -81,10 +81,10 @@ public function refreshMetadata( try { /** @var FilesMetadata $metadata */ $metadata = $this->metadataRequestService->getMetadataFromFileId($node->getId()); - $metadata->setStorageId($storageId); } catch (FilesMetadataNotFoundException) { - $metadata = new FilesMetadata($node->getId(), $storageId); + $metadata = new FilesMetadata($node->getId()); } + $metadata->setStorageId($storageId); // if $process is LIVE, we enforce LIVE // if $process is NAMED, we go NAMED diff --git a/lib/private/FilesMetadata/Model/FilesMetadata.php b/lib/private/FilesMetadata/Model/FilesMetadata.php index 9bf1ca866ac01..edcceb48ae7d9 100644 --- a/lib/private/FilesMetadata/Model/FilesMetadata.php +++ b/lib/private/FilesMetadata/Model/FilesMetadata.php @@ -27,6 +27,7 @@ class FilesMetadata implements IFilesMetadata { private bool $updated = false; private int $lastUpdate = 0; private string $syncToken = ''; + private ?int $storageId = null; public function __construct( private int $fileId = 0 @@ -46,6 +47,14 @@ public function getStorageId(): ?int { return $this->storageId; } + /** + * Set which storage the file this metadata belongs to. + * + * This helps with sharded filecache setups to know where to store the metadata + * + * @param int $storageId + * @return void + */ public function setStorageId(int $storageId): void { $this->storageId = $storageId; } diff --git a/lib/private/FilesMetadata/Service/MetadataRequestService.php b/lib/private/FilesMetadata/Service/MetadataRequestService.php index 49cd888604622..bb3833c2fb600 100644 --- a/lib/private/FilesMetadata/Service/MetadataRequestService.php +++ b/lib/private/FilesMetadata/Service/MetadataRequestService.php @@ -29,8 +29,11 @@ public function __construct( } private function getStorageId(IFilesMetadata $filesMetadata): int { - if ($filesMetadata instanceof FilesMetadata && $filesMetadata->getStorageId()) { - return $filesMetadata->getStorageId(); + if ($filesMetadata instanceof FilesMetadata) { + $storage = $filesMetadata->getStorageId(); + if ($storage) { + return $storage; + } } // all code paths that lead to saving metadata *should* have the storage id set // this fallback is there just in case diff --git a/tests/lib/FilesMetadata/FilesMetadataManagerTest.php b/tests/lib/FilesMetadata/FilesMetadataManagerTest.php new file mode 100644 index 0000000000000..2f9edba001598 --- /dev/null +++ b/tests/lib/FilesMetadata/FilesMetadataManagerTest.php @@ -0,0 +1,98 @@ + + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +namespace Test\FilesMetadata; + +use OC\BackgroundJob\JobList; +use OC\Files\Storage\Temporary; +use OC\FilesMetadata\FilesMetadataManager; +use OC\FilesMetadata\Service\IndexRequestService; +use OC\FilesMetadata\Service\MetadataRequestService; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\Folder; +use OCP\Files\IRootFolder; +use OCP\FilesMetadata\AMetadataEvent; +use OCP\IAppConfig; +use OCP\IDBConnection; +use OCP\Server; +use Psr\Log\LoggerInterface; +use Test\TestCase; +use Test\Traits\MountProviderTrait; +use Test\Traits\UserTrait; + +/** + * @group DB + */ +class FilesMetadataManagerTest extends TestCase { + use UserTrait; + use MountProviderTrait; + + private IEventDispatcher $eventDispatcher; + private JobList $jobList; + private IAppConfig $appConfig; + private LoggerInterface $logger; + private MetadataRequestService $metadataRequestService; + private IndexRequestService $indexRequestService; + private FilesMetadataManager $manager; + private IDBConnection $connection; + private Folder $userFolder; + private array $metadata = []; + + protected function setUp(): void { + parent::setUp(); + + $this->jobList = $this->createMock(JobList::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->eventDispatcher->method('dispatchTyped')->willReturnCallback(function (Event $event) { + if ($event instanceof AMetadataEvent) { + $name = $event->getNode()->getName(); + if (isset($this->metadata[$name])) { + $meta = $event->getMetadata(); + foreach ($this->metadata[$name] as $key => $value) { + $meta->setString($key, $value); + } + } + } + }); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->connection = Server::get(IDBConnection::class); + $this->metadataRequestService = new MetadataRequestService($this->connection, $this->logger); + $this->indexRequestService = new IndexRequestService($this->connection, $this->logger); + $this->manager = new FilesMetadataManager( + $this->eventDispatcher, + $this->jobList, + $this->appConfig, + $this->logger, + $this->metadataRequestService, + $this->indexRequestService, + ); + + $this->createUser('metatest', ''); + $this->registerMount('metatest', new Temporary([]), '/metatest'); + + $rootFolder = Server::get(IRootFolder::class); + $this->userFolder = $rootFolder->getUserFolder('metatest'); + } + + public function testRefreshMetadata(): void { + $this->metadata['test.txt'] = [ + 'istest' => 'yes' + ]; + $file = $this->userFolder->newFile('test.txt', 'test'); + $stored = $this->manager->refreshMetadata($file); + $this->assertEquals($file->getId(), $stored->getFileId()); + $this->assertEquals('yes', $stored->getString('istest')); + + $retrieved = $this->manager->getMetadata($file->getId()); + $this->assertEquals($file->getId(), $retrieved->getFileId()); + $this->assertEquals('yes', $retrieved->getString('istest')); + } +}