Skip to content

Commit

Permalink
Preload the files metadata and cache photos-size
Browse files Browse the repository at this point in the history
Signed-off-by: Joas Schilling <[email protected]>
  • Loading branch information
nickvergessen committed Dec 5, 2023
1 parent fda6853 commit 66975ec
Show file tree
Hide file tree
Showing 5 changed files with 142 additions and 43 deletions.
21 changes: 8 additions & 13 deletions lib/Chat/Parser/SystemMessage.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
use OCA\Talk\Participant;
use OCA\Talk\Room;
use OCA\Talk\Service\ParticipantService;
use OCA\Talk\Share\Helper\FilesMetadataCache;
use OCA\Talk\Share\RoomShareProvider;
use OCP\Comments\IComment;
use OCP\EventDispatcher\Event;
Expand All @@ -44,7 +45,6 @@
use OCP\Files\Node;
use OCP\Files\NotFoundException;
use OCP\FilesMetadata\Exceptions\FilesMetadataNotFoundException;
use OCP\FilesMetadata\IFilesMetadataManager;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IL10N;
Expand Down Expand Up @@ -87,7 +87,7 @@ public function __construct(
protected IRootFolder $rootFolder,
protected ICloudIdManager $cloudIdManager,
protected IURLGenerator $url,
protected IFilesMetadataManager $metadataManager,
protected FilesMetadataCache $metadataCache,
) {
}

Expand Down Expand Up @@ -739,19 +739,14 @@ protected function getFileFromShare(?Participant $participant, string $shareId):
];

// If a preview is available, check if we can get the dimensions of the file from the metadata API
if ($isPreviewAvailable) {
if ($isPreviewAvailable && str_starts_with($node->getMimeType(), 'image/')) {
try {
$metadata = $this->metadataManager->getMetaData($fileId, false);

if ($metadata->hasKey('photos-size')) {
$sizeMetadata = $metadata->getArray('photos-size');

if (isset($sizeMetadata['width']) && isset($sizeMetadata['height'])) {
$data['width'] = $sizeMetadata['width'];
$data['height'] = $sizeMetadata['height'];
}
$sizeMetadata = $this->metadataCache->getMetadataPhotosSizeForFileId($fileId);
if (isset($sizeMetadata['width'], $sizeMetadata['height'])) {
$data['width'] = $sizeMetadata['width'];
$data['height'] = $sizeMetadata['height'];
}
} catch (FilesMetadataNotFoundException $e) {
} catch (FilesMetadataNotFoundException) {
}
}

Expand Down
14 changes: 11 additions & 3 deletions lib/Controller/ChatController.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
use OCA\Talk\Service\ParticipantService;
use OCA\Talk\Service\ReminderService;
use OCA\Talk\Service\SessionService;
use OCA\Talk\Share\Helper\FilesMetadataCache;
use OCA\Talk\Share\RoomShareProvider;
use OCP\App\IAppManager;
use OCP\AppFramework\Db\DoesNotExistException;
Expand All @@ -73,6 +74,7 @@
use OCP\RichObjectStrings\IValidator;
use OCP\Security\ITrustedDomainHelper;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IShare;
use OCP\User\Events\UserLiveStatusEvent;
use OCP\UserStatus\IManager as IUserStatusManager;
use OCP\UserStatus\IUserStatus;
Expand Down Expand Up @@ -103,6 +105,7 @@ public function __construct(
private GuestManager $guestManager,
private MessageParser $messageParser,
protected RoomShareProvider $shareProvider,
protected FilesMetadataCache $filesMetadataCache,
private IManager $autoCompleteManager,
private IUserStatusManager $statusManager,
protected MatterbridgeManager $matterbridgeManager,
Expand Down Expand Up @@ -308,7 +311,8 @@ public function shareObjectToChat(string $objectType, string $objectId, string $

/*
* Gather share IDs from the comments and preload share definitions
* to avoid separate database query for each individual share.
* and files metadata to avoid separate database query for each
* individual share/node later on.
*
* @param IComment[] $comments
*/
Expand All @@ -326,10 +330,14 @@ protected function preloadShares(array $comments): void {
}
}
if (!empty($shareIds)) {
// Ignore the result for now. Retrieved Share objects will be cached by
// Retrieved Share objects will be cached by
// the RoomShareProvider and returned from the cache to
// the Parser\SystemMessage without additional database queries.
$this->shareProvider->getSharesByIds($shareIds);
$shares = $this->shareProvider->getSharesByIds($shareIds);

// Preload files metadata as well
$fileIds = array_filter(array_map(static fn (IShare $share) => $share->getNodeId(), $shares));
$this->filesMetadataCache->preloadMetadata($fileIds);
}
}

Expand Down
103 changes: 103 additions & 0 deletions lib/Share/Helper/FilesMetadataCache.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Joas Schilling <[email protected]>
*
* @author Joas Schilling <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* 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
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\Talk\Share\Helper;

use OCP\FilesMetadata\Exceptions\FilesMetadataNotFoundException;
use OCP\FilesMetadata\Exceptions\FilesMetadataTypeException;
use OCP\FilesMetadata\IFilesMetadataManager;
use OCP\FilesMetadata\Model\IFilesMetadata;

class FilesMetadataCache {
/** @var array<int, ?array{width: int, height: int}> */
protected array $filesSizeData = [];

public function __construct(
protected IFilesMetadataManager $filesMetadataManager,
) {
}

/**
* @param list<int> $fileIds
*/
public function preloadMetadata(array $fileIds): void {
$missingFileIds = array_diff($fileIds, array_keys($this->filesSizeData));
if (empty($missingFileIds)) {
return;
}

$data = $this->filesMetadataManager->getMetadataForFiles($missingFileIds);
foreach ($data as $fileId => $metadata) {
$this->cachePhotosSize($fileId, $metadata);
}
}

/**
* @param int $fileId
* @return array
* @psalm-return array{width: int, height: int}
* @throws FilesMetadataNotFoundException
*/
public function getMetadataPhotosSizeForFileId(int $fileId): array {
if (!array_key_exists($fileId, $this->filesSizeData)) {
try {
$this->cachePhotosSize($fileId, $this->filesMetadataManager->getMetadata($fileId, true));
} catch (FilesMetadataNotFoundException) {
$this->filesSizeData[$fileId] = null;
}
}

if ($this->filesSizeData[$fileId] === null) {
throw new FilesMetadataNotFoundException();
}

return $this->filesSizeData[$fileId];
}

protected function cachePhotosSize(int $fileId, IFilesMetadata $metadata): void {
if ($metadata->hasKey('photos-size')) {
try {
$sizeMetadata = $metadata->getArray('photos-size');
} catch (FilesMetadataNotFoundException|FilesMetadataTypeException) {
$this->filesSizeData[$fileId] = null;
return;
}

if (isset($sizeMetadata['width'], $sizeMetadata['height'])) {
$dimensions = [
'width' => $sizeMetadata['width'],
'height' => $sizeMetadata['height'],
];
$this->filesSizeData[$fileId] = $dimensions;
} else {
$this->filesSizeData[$fileId] = null;
}
} else {
$this->filesSizeData[$fileId] = null;
}

}
}
42 changes: 15 additions & 27 deletions tests/php/Chat/Parser/SystemMessageTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
use OCA\Talk\Participant;
use OCA\Talk\Room;
use OCA\Talk\Service\ParticipantService;
use OCA\Talk\Share\Helper\FilesMetadataCache;
use OCA\Talk\Share\RoomShareProvider;
use OCP\Comments\IComment;
use OCP\Federation\ICloudIdManager;
Expand All @@ -40,8 +41,6 @@
use OCP\Files\IRootFolder;
use OCP\Files\Node;
use OCP\Files\NotFoundException;
use OCP\FilesMetadata\IFilesMetadataManager;
use OCP\FilesMetadata\Model\IFilesMetadata;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IL10N;
Expand Down Expand Up @@ -77,8 +76,8 @@ class SystemMessageTest extends TestCase {
protected $url;
/** @var ICloudIdManager|MockObject */
protected $cloudIdManager;
/** @var IFilesMetadataManager|MockObject */
protected $metadataManager;
/** @var FilesMetadataCache|MockObject */
protected $filesMetadataCache;
/** @var IL10N|MockObject */
protected $l;

Expand All @@ -95,7 +94,7 @@ public function setUp(): void {
$this->rootFolder = $this->createMock(IRootFolder::class);
$this->url = $this->createMock(IURLGenerator::class);
$this->cloudIdManager = $this->createMock(ICloudIdManager::class);
$this->metadataManager = $this->createMock(IFilesMetadataManager::class);
$this->filesMetadataCache = $this->createMock(FilesMetadataCache::class);
$this->l = $this->createMock(IL10N::class);
$this->l->method('t')
->willReturnCallback(function ($text, $parameters = []) {
Expand Down Expand Up @@ -126,7 +125,7 @@ protected function getParser(array $methods = []): SystemMessage {
$this->rootFolder,
$this->cloudIdManager,
$this->url,
$this->metadataManager,
$this->filesMetadataCache,
])
->onlyMethods($methods)
->getMock();
Expand All @@ -144,7 +143,7 @@ protected function getParser(array $methods = []): SystemMessage {
$this->rootFolder,
$this->cloudIdManager,
$this->url,
$this->metadataManager,
$this->filesMetadataCache,
);
}

Expand Down Expand Up @@ -624,7 +623,7 @@ public function testGetFileFromShareForGuest() {
->willReturn('name');
$node->expects($this->atLeastOnce())
->method('getMimeType')
->willReturn('text/plain');
->willReturn('image/png');
$node->expects($this->once())
->method('getSize')
->willReturn(65530);
Expand Down Expand Up @@ -660,22 +659,11 @@ public function testGetFileFromShareForGuest() {
->with($node)
->willReturn(true);

$metadata = $this->createMock(IFilesMetadata::class);
$metadata->expects($this->once())
->method('hasKey')
->with('photos-size')
->willReturn(true);

$metadata->expects($this->once())
->method('getArray')
->with('photos-size')
$this->filesMetadataCache->expects($this->once())
->method('getMetadataPhotosSizeForFileId')
->with(54)
->willReturn(['width' => 1234, 'height' => 4567]);

$this->metadataManager->expects($this->once())
->method('getMetaData')
->with(54, false)
->willReturn($metadata);

$participant = $this->createMock(Participant::class);
$participant->expects($this->once())
->method('isGuest')
Expand All @@ -691,7 +679,7 @@ public function testGetFileFromShareForGuest() {
'link' => 'absolute-link',
'etag' => '1872ade88f3013edeb33decd74a4f947',
'permissions' => 27,
'mimetype' => 'text/plain',
'mimetype' => 'image/png',
'preview-available' => 'yes',
'width' => 1234,
'height' => 4567,
Expand Down Expand Up @@ -747,8 +735,8 @@ public function testGetFileFromShareForOwner() {
])
->willReturn('absolute-link-owner');

$this->metadataManager->expects($this->never())
->method('getMetaData');
$this->filesMetadataCache->expects($this->never())
->method('getMetadataPhotosSizeForFileId');

$participant = $this->createMock(Participant::class);
$participant->expects($this->once())
Expand Down Expand Up @@ -839,8 +827,8 @@ public function testGetFileFromShareForRecipient() {
->with($file)
->willReturn(false);

$this->metadataManager->expects($this->never())
->method('getMetaData');
$this->filesMetadataCache->expects($this->never())
->method('getMetadataPhotosSizeForFileId');

$this->url->expects($this->once())
->method('linkToRouteAbsolute')
Expand Down
5 changes: 5 additions & 0 deletions tests/php/Controller/ChatControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
use OCA\Talk\Service\ParticipantService;
use OCA\Talk\Service\ReminderService;
use OCA\Talk\Service\SessionService;
use OCA\Talk\Share\Helper\FilesMetadataCache;
use OCA\Talk\Share\RoomShareProvider;
use OCP\App\IAppManager;
use OCP\AppFramework\Http;
Expand Down Expand Up @@ -86,6 +87,8 @@ class ChatControllerTest extends TestCase {
protected $messageParser;
/** @var RoomShareProvider|MockObject */
protected $roomShareProvider;
/** @var FilesMetadataCache|MockObject */
protected $filesMetadataCache;
/** @var IManager|MockObject */
protected $autoCompleteManager;
/** @var IUserStatusManager|MockObject */
Expand Down Expand Up @@ -131,6 +134,7 @@ public function setUp(): void {
$this->guestManager = $this->createMock(GuestManager::class);
$this->messageParser = $this->createMock(MessageParser::class);
$this->roomShareProvider = $this->createMock(RoomShareProvider::class);
$this->filesMetadataCache = $this->createMock(FilesMetadataCache::class);
$this->autoCompleteManager = $this->createMock(IManager::class);
$this->statusManager = $this->createMock(IUserStatusManager::class);
$this->matterbridgeManager = $this->createMock(MatterbridgeManager::class);
Expand Down Expand Up @@ -171,6 +175,7 @@ private function recreateChatController() {
$this->guestManager,
$this->messageParser,
$this->roomShareProvider,
$this->filesMetadataCache,
$this->autoCompleteManager,
$this->statusManager,
$this->matterbridgeManager,
Expand Down

0 comments on commit 66975ec

Please sign in to comment.