refactor: don't join on filecache in defaultshareprovider

Signed-off-by: Robin Appelman <robin@icewind.nl>

lib/private/Share20/DefaultShareProvider.php

@@ -8,6 +8,8 @@
 namespace OC\Share20;
 
 use OC\Files\Cache\Cache;
+use OC\Files\Cache\CacheEntry;
+use OC\Files\Cache\FileAccess;
 use OC\Share20\Exception\BackendError;
 use OC\Share20\Exception\InvalidShare;
 use OC\Share20\Exception\ProviderException;
@@ -41,52 +43,21 @@ class DefaultShareProvider implements IShareProvider {
 	// Special share type for user modified group shares
 	public const SHARE_TYPE_USERGROUP = 2;
 
-	/** @var IDBConnection */
-	private $dbConn;
-
-	/** @var IUserManager */
-	private $userManager;
-
-	/** @var IGroupManager */
-	private $groupManager;
-
-	/** @var IRootFolder */
-	private $rootFolder;
-
-	/** @var IMailer */
-	private $mailer;
-
-	/** @var Defaults */
-	private $defaults;
-
-	/** @var IFactory */
-	private $l10nFactory;
-
-	/** @var IURLGenerator */
-	private $urlGenerator;
-
-	private ITimeFactory $timeFactory;
+	private IDBConnection $dbConn;
 
 	public function __construct(
-		IDBConnection $connection,
-		IUserManager $userManager,
-		IGroupManager $groupManager,
-		IRootFolder $rootFolder,
-		IMailer $mailer,
-		Defaults $defaults,
-		IFactory $l10nFactory,
-		IURLGenerator $urlGenerator,
-		ITimeFactory $timeFactory,
+		IDBConnection         $connection,
+		private IUserManager  $userManager,
+		private IGroupManager $groupManager,
+		private IRootFolder   $rootFolder,
+		private IMailer       $mailer,
+		private Defaults      $defaults,
+		private IFactory      $l10nFactory,
+		private IURLGenerator $urlGenerator,
+		private ITimeFactory  $timeFactory,
+		private FileAccess    $cacheAccess,
 	) {
 		$this->dbConn = $connection;
-		$this->userManager = $userManager;
-		$this->groupManager = $groupManager;
-		$this->rootFolder = $rootFolder;
-		$this->mailer = $mailer;
-		$this->defaults = $defaults;
-		$this->l10nFactory = $l10nFactory;
-		$this->urlGenerator = $urlGenerator;
-		$this->timeFactory = $timeFactory;
 	}
 
 	/**
@@ -631,10 +602,7 @@ class DefaultShareProvider implements IShareProvider {
 		}
 
 		$qb = $this->dbConn->getQueryBuilder();
-		$qb->select('s.*',
-			'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash',
-			'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime',
-			'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum')
+		$qb->select('s.*')
 			->from('share', 's')
 			->andWhere($qb->expr()->orX(
 				$qb->expr()->eq('item_type', $qb->createNamedParameter('file')),
@@ -661,27 +629,17 @@ class DefaultShareProvider implements IShareProvider {
 			);
 		}
 
-		// todo? maybe get these from the oc_mounts table
-		$childMountNodes = array_filter($node->getDirectoryListing(), function (Node $node): bool {
-			return $node->getInternalPath() === '';
-		});
-		$childMountRootIds = array_map(function (Node $node): int {
+		$childFileIds = array_map(function (Node $node): int {
 			return $node->getId();
-		}, $childMountNodes);
+		}, $node->getDirectoryListing());
 
-		$qb->innerJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'));
-		$qb->andWhere(
-			$qb->expr()->orX(
-				$qb->expr()->eq('f.parent', $qb->createNamedParameter($node->getId())),
-				$qb->expr()->in('f.fileid', $qb->createParameter('chunk'))
-			)
-		);
+		$qb->andWhere($qb->expr()->in('s.file_source', $qb->createParameter('chunk')));
 
 		$qb->orderBy('id');
 
 		$shares = [];
 
-		$chunks = array_chunk($childMountRootIds, 1000);
+		$chunks = array_chunk($childFileIds, 1000);
 
 		// Force the request to be run when there is 0 mount.
 		if (count($chunks) === 0) {
@@ -692,7 +650,7 @@ class DefaultShareProvider implements IShareProvider {
 			$qb->setParameter('chunk', $chunk, IQueryBuilder::PARAM_INT_ARRAY);
 			$cursor = $qb->executeQuery();
 			while ($data = $cursor->fetch()) {
-				$shares[$data['fileid']][] = $this->createShare($data);
+				$shares[$data['file_source']][] = $this->createShare($data);
 			}
 			$cursor->closeCursor();
 		}
@@ -834,25 +792,9 @@ class DefaultShareProvider implements IShareProvider {
 	 * Returns whether the given database result can be interpreted as
 	 * a share with accessible file (not trashed, not deleted)
 	 */
-	private function isAccessibleResult($data) {
-		// exclude shares leading to deleted file entries
-		if ($data['fileid'] === null || $data['path'] === null) {
-			return false;
-		}
-
-		// exclude shares leading to trashbin on home storages
-		$pathSections = explode('/', $data['path'], 2);
-		// FIXME: would not detect rare md5'd home storage case properly
-		if ($pathSections[0] !== 'files'
-			&& (str_starts_with($data['storage_string_id'], 'home::') || str_starts_with($data['storage_string_id'], 'object::user'))) {
-			return false;
-		} elseif ($pathSections[0] === '__groupfolders'
-			&& str_starts_with($pathSections[1], 'trash/')
-		) {
-			// exclude shares leading to trashbin on group folders storages
-			return false;
-		}
-		return true;
+	private function isAccessibleResult(CacheEntry $data) {
+		$path = $data->getPath();
+		return !(str_starts_with($path, 'files_trashbin/') || str_starts_with($path, '__groupfolders/trash/'));
 	}
 
 	/**
@@ -865,15 +807,8 @@ class DefaultShareProvider implements IShareProvider {
 		if ($shareType === IShare::TYPE_USER) {
 			//Get shares directly with this user
 			$qb = $this->dbConn->getQueryBuilder();
-			$qb->select('s.*',
-				'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash',
-				'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime',
-				'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum'
-			)
-				->selectAlias('st.id', 'storage_string_id')
-				->from('share', 's')
-				->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))
-				->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id'));
+			$qb->select('s.*')
+				->from('share', 's');
 
 			// Order by id
 			$qb->orderBy('s.id');
@@ -899,14 +834,7 @@ class DefaultShareProvider implements IShareProvider {
 			$cursor = $qb->execute();
 
 			while ($data = $cursor->fetch()) {
-				if ($data['fileid'] && $data['path'] === null) {
-					$data['path'] = (string) $data['path'];
-					$data['name'] = (string) $data['name'];
-					$data['checksum'] = (string) $data['checksum'];
-				}
-				if ($this->isAccessibleResult($data)) {
-					$shares[] = $this->createShare($data);
-				}
+				$shares[] = $this->createShare($data);
 			}
 			$cursor->closeCursor();
 		} elseif ($shareType === IShare::TYPE_GROUP) {
@@ -926,15 +854,8 @@ class DefaultShareProvider implements IShareProvider {
 				}
 
 				$qb = $this->dbConn->getQueryBuilder();
-				$qb->select('s.*',
-					'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash',
-					'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime',
-					'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum'
-				)
-					->selectAlias('st.id', 'storage_string_id')
+				$qb->select('s.*')
 					->from('share', 's')
-					->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid'))
-					->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id'))
 					->orderBy('s.id')
 					->setFirstResult(0);
 
@@ -967,10 +888,8 @@ class DefaultShareProvider implements IShareProvider {
 						continue;
 					}
 
-					if ($this->isAccessibleResult($data)) {
-						$share = $this->createShare($data);
-						$shares2[$share->getId()] = $share;
-					}
+					$share = $this->createShare($data);
+					$shares2[$share->getId()] = $share;
 				}
 				$cursor->closeCursor();
 			}
@@ -984,7 +903,31 @@ class DefaultShareProvider implements IShareProvider {
 		}
 
 
-		return $shares;
+		return $this->setNodes($shares);
+	}
+
+	/**
+	 * @param IShare[] $shares
+	 * @return IShare[]
+	 */
+	private function setNodes(array $shares): array {
+		$fileIds = array_map(function (IShare $share): int {
+			return $share->getNodeId();
+		}, $shares);
+		$files = $this->cacheAccess->getByFileIds($fileIds);
+
+		$sharesWithFiles = [];
+		foreach ($shares as $share) {
+			if (isset($files[$share->getNodeId()])) {
+				$cacheItem = $files[$share->getNodeId()];
+				if ($this->isAccessibleResult($cacheItem)) {
+					$share->setNodeCacheEntry($cacheItem);
+					$sharesWithFiles[] = $share;
+				}
+			}
+		}
+
+		return $sharesWithFiles;
 	}
 
 	/**

lib/private/Share20/ProviderFactory.php

@@ -15,14 +15,12 @@ use OCA\FederatedFileSharing\TokenHandler;
 use OCA\ShareByMail\Settings\SettingsManager;
 use OCA\ShareByMail\ShareByMailProvider;
 use OCA\Talk\Share\RoomShareProvider;
-use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Defaults;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Federation\ICloudFederationFactory;
 use OCP\Files\IRootFolder;
 use OCP\Http\Client\IClientService;
 use OCP\IServerContainer;
-use OCP\L10N\IFactory;
 use OCP\Mail\IMailer;
 use OCP\Security\IHasher;
 use OCP\Security\ISecureRandom;
@@ -77,17 +75,7 @@ class ProviderFactory implements IProviderFactory {
 	 */
 	protected function defaultShareProvider() {
 		if ($this->defaultProvider === null) {
-			$this->defaultProvider = new DefaultShareProvider(
-				$this->serverContainer->getDatabaseConnection(),
-				$this->serverContainer->getUserManager(),
-				$this->serverContainer->getGroupManager(),
-				$this->serverContainer->get(IRootFolder::class),
-				$this->serverContainer->get(IMailer::class),
-				$this->serverContainer->query(Defaults::class),
-				$this->serverContainer->get(IFactory::class),
-				$this->serverContainer->getURLGenerator(),
-				$this->serverContainer->query(ITimeFactory::class),
-			);
+			$this->defaultProvider = $this->serverContainer->get(DefaultShareProvider::class);
 		}
 
 		return $this->defaultProvider;

tests/lib/Share20/DefaultShareProviderTest.php

@@ -7,6 +7,8 @@
 
 namespace Test\Share20;
 
+use OC\Files\Cache\CacheEntry;
+use OC\Files\Cache\FileAccess;
 use OC\Share20\DefaultShareProvider;
 use OC\Share20\ShareAttributes;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -68,6 +70,11 @@ class DefaultShareProviderTest extends \Test\TestCase {
 	/** @var ITimeFactory|MockObject */
 	protected $timeFactory;
 
+	/** @var FileAccess|MockObject */
+	protected $cacheAccess;
+	/** @var array<int, CacheEntry> */
+	protected $cacheItems = [];
+
 	protected function setUp(): void {
 		$this->dbConn = \OC::$server->getDatabaseConnection();
 		$this->userManager = $this->createMock(IUserManager::class);
@@ -79,6 +86,26 @@ class DefaultShareProviderTest extends \Test\TestCase {
 		$this->defaults = $this->getMockBuilder(Defaults::class)->disableOriginalConstructor()->getMock();
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
+		$this->cacheAccess = $this->createMock(FileAccess::class);
+		$this->cacheItems = [];
+		$this->cacheAccess->method('getByFileIds')->willReturnCallback(function (array $fileIds) {
+			$result = [];
+			foreach ($fileIds as $fileId) {
+				if (isset($this->cacheItems[$fileId])) {
+					$result[$fileId] = $this->cacheItems[$fileId];
+				}
+			}
+			return $result;
+		});
+		$this->cacheAccess->method('getByFileIdsInStorage')->willReturnCallback(function (array $fileIds, int $storageId) {
+			$result = [];
+			foreach ($fileIds as $fileId) {
+				if (isset($this->cacheItems[$fileId]) && $this->cacheItems[$fileId]->getStorageId() === $storageId) {
+					$result[$fileId] = $this->cacheItems[$fileId];
+				}
+			}
+			return $result;
+		});
 
 		$this->userManager->expects($this->any())->method('userExists')->willReturn(true);
 		$this->timeFactory->expects($this->any())->method('now')->willReturn(new \DateTimeImmutable("2023-05-04 00:00 Europe/Berlin"));
@@ -95,13 +122,13 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->defaults,
 			$this->l10nFactory,
 			$this->urlGenerator,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->cacheAccess,
 		);
 	}
 
 	protected function tearDown(): void {
 		$this->dbConn->getQueryBuilder()->delete('share')->execute();
-		$this->dbConn->getQueryBuilder()->delete('filecache')->execute();
 		$this->dbConn->getQueryBuilder()->delete('storages')->execute();
 	}
 
@@ -456,7 +483,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 				$this->defaults,
 				$this->l10nFactory,
 				$this->urlGenerator,
-				$this->timeFactory
+				$this->timeFactory,
+				$this->cacheAccess,
 			])
 			->setMethods(['getShareById'])
 			->getMock();
@@ -551,7 +579,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 				$this->defaults,
 				$this->l10nFactory,
 				$this->urlGenerator,
-				$this->timeFactory
+				$this->timeFactory,
+				$this->cacheAccess,
 			])
 			->setMethods(['getShareById'])
 			->getMock();
@@ -906,16 +935,15 @@ class DefaultShareProviderTest extends \Test\TestCase {
 	}
 
 	private function createTestFileEntry($path, $storage = 1) {
-		$qb = $this->dbConn->getQueryBuilder();
-		$qb->insert('filecache')
-			->values([
-				'storage' => $qb->expr()->literal($storage),
-				'path' => $qb->expr()->literal($path),
-				'path_hash' => $qb->expr()->literal(md5($path)),
-				'name' => $qb->expr()->literal(basename($path)),
-			]);
-		$this->assertEquals(1, $qb->execute());
-		return $qb->getLastInsertId();
+		$id = count($this->cacheItems);
+		$this->cacheItems[$id] = new CacheEntry([
+			'fileid' => $id,
+			'storage' => $storage,
+			'path' => $path,
+			'path_hash' => md5($path),
+			'name' => basename($path),
+		]);
+		return $id;
 	}
 
 	public function storageAndFileNameProvider() {
@@ -924,8 +952,6 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			['home::shareOwner', 'files/test.txt', 'files/test2.txt'],
 			// regular file on external storage
 			['smb::whatever', 'files/test.txt', 'files/test2.txt'],
-			// regular file on external storage in trashbin-like folder,
-			['smb::whatever', 'files_trashbin/files/test.txt', 'files_trashbin/files/test2.txt'],
 		];
 	}
 
@@ -1279,6 +1305,7 @@ class DefaultShareProviderTest extends \Test\TestCase {
 
 		$user = $this->createMock(IUser::class);
 		$user->method('getUID')->willReturn('sharedWith');
+		$user->method('getDisplayName')->willReturn('sharedWith');
 		$owner = $this->createMock(IUser::class);
 		$owner->method('getUID')->willReturn('shareOwner');
 		$initiator = $this->createMock(IUser::class);
@@ -2519,7 +2546,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->defaults,
 			$this->l10nFactory,
 			$this->urlGenerator,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->cacheAccess,
 		);
 
 		$password = md5(time());
@@ -2617,7 +2645,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->defaults,
 			$this->l10nFactory,
 			$this->urlGenerator,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->cacheAccess,
 		);
 
 		$u1 = $userManager->createUser('testShare1', 'test');
@@ -2713,7 +2742,8 @@ class DefaultShareProviderTest extends \Test\TestCase {
 			$this->defaults,
 			$this->l10nFactory,
 			$this->urlGenerator,
-			$this->timeFactory
+			$this->timeFactory,
+			$this->cacheAccess,
 		);
 
 		$u1 = $userManager->createUser('testShare1', 'test');