fix(FileAccessTest): Adress review comments

Signed-off-by: Marcel Klehr <mklehr@gmx.net>

3 files changed, 58 insertions, 55 deletions

diff --git a/lib/private/Files/Cache/FileAccess.php b/lib/private/Files/Cache/FileAccess.php
index c24cc75ce00..3f997053c76 100644
--- a/lib/private/Files/Cache/FileAccess.php
+++ b/lib/private/Files/Cache/FileAccess.php
@@ -101,19 +101,19 @@ class FileAccess implements IFileAccess {
 	 * Allows filtering by mime types, encryption status, and limits the number of results.
 	 *
 	 * @param int $storageId The ID of the storage to search within.
-	 * @param int $rootId The file ID of the ancestor to base the search on.
-	 * @param int $lastFileId The last processed file ID. Only files with a higher ID will be included. Defaults to 0.
-	 * @param list<int> $mimeTypes An array of mime types to filter the results. If empty, no mime type filtering will be applied.
+	 * @param int $folderId The file ID of the ancestor to base the search on.
+	 * @param int $fileIdCursor The last processed file ID. Only files with a higher ID will be included. Defaults to 0.
+	 * @param int $maxResults The maximum number of results to retrieve. If set to 0, all matching files will be retrieved.
+	 * @param list<int> $mimeTypeIds An array of mime types to filter the results. If empty, no mime type filtering will be applied.
 	 * @param bool $endToEndEncrypted Whether to include EndToEndEncrypted files
 	 * @param bool $serverSideEncrypted Whether to include ServerSideEncrypted files
-	 * @param int $maxResults The maximum number of results to retrieve. If set to 0, all matching files will be retrieved.
 	 * @return \Generator A generator yielding matching files as cache entries.
 	 * @throws \OCP\DB\Exception
 	 */
-	public function getByAncestorInStorage(int $storageId, int $rootId, int $lastFileId = 0, array $mimeTypes = [], bool $endToEndEncrypted = true, bool $serverSideEncrypted = true, int $maxResults = 100): \Generator {
+	public function getByAncestorInStorage(int $storageId, int $folderId, int $fileIdCursor = 0, int $maxResults = 100, array $mimeTypeIds = [], bool $endToEndEncrypted = true, bool $serverSideEncrypted = true): \Generator {
 		$qb = $this->getQuery();
 		$qb->selectFileCache();
-		$qb->andWhere($qb->expr()->eq('filecache.fileid', $qb->createNamedParameter($rootId, IQueryBuilder::PARAM_INT)));
+		$qb->andWhere($qb->expr()->eq('filecache.fileid', $qb->createNamedParameter($folderId, IQueryBuilder::PARAM_INT)));
 		$result = $qb->executeQuery();
 		/** @var array{path:string}|false $root */
 		$root = $result->fetch();
@@ -131,7 +131,7 @@ class FileAccess implements IFileAccess {
 			->from('filecache', 'f')
 			->andWhere($qb->expr()->like('f.path', $qb->createNamedParameter($path . '%')))
 			->andWhere($qb->expr()->eq('f.storage', $qb->createNamedParameter($storageId)))
-			->andWhere($qb->expr()->gt('f.fileid', $qb->createNamedParameter($lastFileId)));
+			->andWhere($qb->expr()->gt('f.fileid', $qb->createNamedParameter($fileIdCursor, IQueryBuilder::PARAM_INT)));
 
 		if (!$endToEndEncrypted) {
 			// End to end encrypted files are descendants of a folder with encrypted=1
@@ -151,8 +151,8 @@ class FileAccess implements IFileAccess {
 			$qb->andWhere($qb->expr()->eq('f.encrypted', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
 		}
 
-		if (count($mimeTypes) > 0) {
-			$qb->andWhere($qb->expr()->in('f.mimetype', $qb->createNamedParameter($mimeTypes, IQueryBuilder::PARAM_INT_ARRAY)));
+		if (count($mimeTypeIds) > 0) {
+			$qb->andWhere($qb->expr()->in('f.mimetype', $qb->createNamedParameter($mimeTypeIds, IQueryBuilder::PARAM_INT_ARRAY)));
 		}
 
 		if ($maxResults !== 0) {
@@ -177,20 +177,20 @@ class FileAccess implements IFileAccess {
 	 * Optionally rewrites home directory root paths to avoid cache and trashbin.
 	 *
 	 * @param list<string> $mountProviders An array of mount provider class names to filter. If empty, all providers will be included.
-	 * @param string|false $excludeMountPoints A string containing an SQL LIKE pattern to exclude mount points. Set to false to not exclude any mount points.
+	 * @param bool $excludeTrashbinMounts Whether to exclude mounts that mount directories in a user's trashbin.
 	 * @param bool $rewriteHomeDirectories Whether to rewrite the root path IDs for home directories to only include user files.
 	 * @return \Generator A generator yielding mount configurations as an array containing 'storage_id', 'root_id', and 'override_root'.
 	 * @throws \OCP\DB\Exception
 	 */
-	public function getDistinctMounts(array $mountProviders = [], string|false $excludeMountPoints = false, bool $rewriteHomeDirectories = true): \Generator {
+	public function getDistinctMounts(array $mountProviders = [], bool $excludeTrashbinMounts = true, bool $rewriteHomeDirectories = true): \Generator {
 		$qb = $this->connection->getQueryBuilder();
 		$qb->selectDistinct(['root_id', 'storage_id', 'mount_provider_class'])
 			->from('mounts');
 		if (count($mountProviders) > 0) {
 			$qb->where($qb->expr()->in('mount_provider_class', $qb->createPositionalParameter($mountProviders, IQueryBuilder::PARAM_STR_ARRAY)));
 		}
-		if ($excludeMountPoints !== false) {
-			$qb->andWhere($qb->expr()->notLike('mount_point', $qb->createPositionalParameter($excludeMountPoints)));
+		if ($excludeTrashbinMounts === true) {
+			$qb->andWhere($qb->expr()->notLike('mount_point', $qb->createPositionalParameter('/%/files_trashbin/%')));
 		}
 		$qb->orderBy('root_id', 'ASC');
 		$result = $qb->executeQuery();
@@ -203,6 +203,8 @@ class FileAccess implements IFileAccess {
 			$storageId = (int)$row['storage_id'];
 			$rootId = (int)$row['root_id'];
 			$overrideRoot = $rootId;
+			// LocalHomeMountProvider is the default provider for user home directories
+			// ObjectHomeMountProvider is the home directory provider for when S3 primary storage is used
 			if ($rewriteHomeDirectories && in_array($row['mount_provider_class'], [
 				\OC\Files\Mount\LocalHomeMountProvider::class,
 				\OC\Files\Mount\ObjectHomeMountProvider::class,
@@ -214,7 +216,8 @@ class FileAccess implements IFileAccess {
 					/** @var array|false $root */
 					$root = $qb
 						->andWhere($qb->expr()->eq('filecache.storage', $qb->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)))
-						->andWhere($qb->expr()->eq('filecache.path', $qb->createNamedParameter('files')))
+						->andWhere($qb->expr()->eq('filecache.parent', $qb->createNamedParameter($rootId, IQueryBuilder::PARAM_INT)))
+						->andWhere($qb->expr()->eq('filecache.name', $qb->createNamedParameter('files')))
 						->executeQuery()->fetch();
 					if ($root !== false) {
 						$overrideRoot = (int)$root['fileid'];
@@ -224,10 +227,11 @@ class FileAccess implements IFileAccess {
 					continue;
 				}
 			}
+			// Reference to root_id is still necessary even if we have the overridden_root_id, because storage_id and root_id uniquely identify a mount
 			yield [
 				'storage_id' => $storageId,
 				'root_id' => $rootId,
-				'override_root' => $overrideRoot,
+				'overridden_root' => $overrideRoot,
 			];
 		}
 		$result->closeCursor();
diff --git a/lib/public/Files/Cache/IFileAccess.php b/lib/public/Files/Cache/IFileAccess.php
index be66c5a9a09..6f27950a0b0 100644
--- a/lib/public/Files/Cache/IFileAccess.php
+++ b/lib/public/Files/Cache/IFileAccess.php
@@ -85,18 +85,16 @@ interface IFileAccess {
 	 * Allows filtering by mime types, encryption status, and limits the number of results.
 	 *
 	 * @param int $storageId The ID of the storage to search within.
-	 * @param int $rootId The file ID of the ancestor to base the search on.
-	 * @param int $lastFileId The last processed file ID. Only files with a higher ID will be included. Defaults to 0.
-	 * @param list<int> $mimeTypes An array of mime types to filter the results. If empty, no mime type filtering will be applied.
+	 * @param int $folderId The file ID of the ancestor to base the search on.
+	 * @param int $fileIdCursor The last processed file ID. Only files with a higher ID will be included. Defaults to 0.
+	 * @param int $maxResults The maximum number of results to retrieve. If set to 0, all matching files will be retrieved.
+	 * @param list<int> $mimeTypeIds An array of mime types to filter the results. If empty, no mime type filtering will be applied.
 	 * @param bool $endToEndEncrypted Whether to include EndToEndEncrypted files
 	 * @param bool $serverSideEncrypted Whether to include ServerSideEncrypted files
-	 * @param int $maxResults The maximum number of results to retrieve. If set to 0, all matching files will be retrieved.
 	 * @return \Generator A generator yielding matching files as cache entries.
 	 * @throws \OCP\DB\Exception
-	 *
-	 * @since 32.0.0
 	 */
-	public function getByAncestorInStorage(int $storageId, int $rootId, int $lastFileId = 0, array $mimeTypes = [], bool $endToEndEncrypted = true, bool $serverSideEncrypted = true, int $maxResults = 100): \Generator;
+	public function getByAncestorInStorage(int $storageId, int $folderId, int $fileIdCursor = 0, int $maxResults = 100, array $mimeTypeIds = [], bool $endToEndEncrypted = true, bool $serverSideEncrypted = true): \Generator;
 
 	/**
 	 * Retrieves a list of all distinct mounts.
@@ -104,12 +102,12 @@ interface IFileAccess {
 	 * Optionally rewrites home directory root paths to avoid cache and trashbin.
 	 *
 	 * @param list<string> $mountProviders An array of mount provider class names to filter. If empty, all providers will be included.
-	 * @param string|false $excludeMountPoints A string containing an SQL LIKE pattern to exclude mount points. Set to false to not exclude any mount points.
+	 * @param bool $excludeTrashbinMounts Whether to include mounts that mount a directory in a user's trashbin.
 	 * @param bool $rewriteHomeDirectories Whether to rewrite the root path IDs for home directories to only include user files.
 	 * @return \Generator A generator yielding mount configurations as an array containing 'storage_id', 'root_id', and 'override_root'.
 	 * @throws \OCP\DB\Exception
 	 *
 	 * @since 32.0.0
 	 */
-	public function getDistinctMounts(array $mountProviders = [], string|false $excludeMountPoints = false, bool $rewriteHomeDirectories = true): \Generator;
+	public function getDistinctMounts(array $mountProviders = [], bool $excludeTrashbinMounts = true, bool $rewriteHomeDirectories = true): \Generator;
 }
diff --git a/tests/lib/Files/Cache/FileAccessTest.php b/tests/lib/Files/Cache/FileAccessTest.php
index 435a233a57e..8b796d86f9a 100644
--- a/tests/lib/Files/Cache/FileAccessTest.php
+++ b/tests/lib/Files/Cache/FileAccessTest.php
@@ -71,7 +71,7 @@ class FileAccessTest extends TestCase {
 				'storage_id' => $queryBuilder->createNamedParameter(2, IQueryBuilder::PARAM_INT),
 				'root_id' => $queryBuilder->createNamedParameter(20, IQueryBuilder::PARAM_INT),
 				'mount_provider_class' => $queryBuilder->createNamedParameter('TestProviderClass2'),
-				'mount_point' => $queryBuilder->createNamedParameter('/cache'),
+				'mount_point' => $queryBuilder->createNamedParameter('/foobar/files_trashbin/test'),
 				'user_id' => $queryBuilder->createNamedParameter('test'),
 			])
 			->executeStatement();
@@ -81,7 +81,7 @@ class FileAccessTest extends TestCase {
 				'storage_id' => $queryBuilder->createNamedParameter(3, IQueryBuilder::PARAM_INT),
 				'root_id' => $queryBuilder->createNamedParameter(30, IQueryBuilder::PARAM_INT),
 				'mount_provider_class' => $queryBuilder->createNamedParameter('TestProviderClass1'),
-				'mount_point' => $queryBuilder->createNamedParameter('/trashbin'),
+				'mount_point' => $queryBuilder->createNamedParameter('/documents'),
 				'user_id' => $queryBuilder->createNamedParameter('test'),
 			])
 			->executeStatement();
@@ -93,25 +93,19 @@ class FileAccessTest extends TestCase {
 	public function testGetDistinctMountsWithoutFilters(): void {
 		$result = iterator_to_array($this->fileAccess->getDistinctMounts());
 
-		$this->assertCount(3, $result);
+		$this->assertCount(2, $result);
 
 		$this->assertEquals([
 			'storage_id' => 1,
 			'root_id' => 10,
-			'override_root' => 10,
+			'overridden_root' => 10,
 		], $result[0]);
 
 		$this->assertEquals([
-			'storage_id' => 2,
-			'root_id' => 20,
-			'override_root' => 20,
-		], $result[1]);
-
-		$this->assertEquals([
 			'storage_id' => 3,
 			'root_id' => 30,
-			'override_root' => 30,
-		], $result[2]);
+			'overridden_root' => 30,
+		], $result[1]);
 	}
 
 	/**
@@ -125,13 +119,13 @@ class FileAccessTest extends TestCase {
 		$this->assertEquals([
 			'storage_id' => 1,
 			'root_id' => 10,
-			'override_root' => 10,
+			'overridden_root' => 10,
 		], $result[0]);
 
 		$this->assertEquals([
 			'storage_id' => 3,
 			'root_id' => 30,
-			'override_root' => 30,
+			'overridden_root' => 30,
 		], $result[1]);
 	}
 
@@ -139,21 +133,27 @@ class FileAccessTest extends TestCase {
 	 * Test that getDistinctMounts excludes certain mount points
 	 */
 	public function testGetDistinctMountsWithExclusionFilter(): void {
-		$result = iterator_to_array($this->fileAccess->getDistinctMounts([], '/cache'));
+		$result = iterator_to_array($this->fileAccess->getDistinctMounts([], false));
 
-		$this->assertCount(2, $result);
+		$this->assertCount(3, $result);
 
 		$this->assertEquals([
 			'storage_id' => 1,
 			'root_id' => 10,
-			'override_root' => 10,
+			'overridden_root' => 10,
 		], $result[0]);
 
 		$this->assertEquals([
+			'storage_id' => 2,
+			'root_id' => 20,
+			'overridden_root' => 20,
+		], $result[1]);
+
+		$this->assertEquals([
 			'storage_id' => 3,
 			'root_id' => 30,
-			'override_root' => 30,
-		], $result[1]);
+			'overridden_root' => 30,
+		], $result[2]);
 	}
 
 	/**
@@ -180,8 +180,9 @@ class FileAccessTest extends TestCase {
 			->values([
 				'fileid' => $queryBuilder->createNamedParameter(99, IQueryBuilder::PARAM_INT),
 				'storage' => $queryBuilder->createNamedParameter(4, IQueryBuilder::PARAM_INT),
-				'path' => $queryBuilder->createNamedParameter('files'),
-				'path_hash' => $queryBuilder->createNamedParameter(md5('files')),
+				'parent' => $queryBuilder->createNamedParameter(40),
+				'name' => $queryBuilder->createNamedParameter('files'),
+				'path_hash' => $queryBuilder->createNamedParameter(md5('/home/user/files')),
 			])
 			->executeStatement();
 
@@ -190,7 +191,7 @@ class FileAccessTest extends TestCase {
 		$this->assertEquals([
 			'storage_id' => 4,
 			'root_id' => 40,
-			'override_root' => 99,
+			'overridden_root' => 99,
 		], end($result));
 	}
 
@@ -298,17 +299,17 @@ class FileAccessTest extends TestCase {
 			1, // storageId
 			1, // rootId
 			0, // lastFileId
+			10, // maxResults
 			[], // mimeTypes
 			true, // include end-to-end encrypted files
 			true, // include server-side encrypted files
-			10 // maxResults
 		);
 
 		$result = iterator_to_array($generator);
 
 		$this->assertCount(4, $result);
 
-		$paths = array_map(fn(CacheEntry $entry) => $entry->getPath(), $result);
+		$paths = array_map(fn (CacheEntry $entry) => $entry->getPath(), $result);
 		$this->assertEquals([
 			'files/documents',
 			'files/photos',
@@ -325,10 +326,10 @@ class FileAccessTest extends TestCase {
 			1,
 			1,
 			0,
+			10,
 			[2], // Only include documents (mimetype=2)
 			true,
 			true,
-			10
 		);
 
 		$result = iterator_to_array($generator);
@@ -345,16 +346,16 @@ class FileAccessTest extends TestCase {
 			1,
 			1,
 			0,
+			10,
 			[],
 			false, // exclude end-to-end encrypted files
 			true,
-			10
 		);
 
 		$result = iterator_to_array($generator);
 
 		$this->assertCount(3, $result);
-		$paths = array_map(fn(CacheEntry $entry) => $entry->getPath(), $result);
+		$paths = array_map(fn (CacheEntry $entry) => $entry->getPath(), $result);
 		$this->assertEquals(['files/documents', 'files/photos', 'files/serversideencrypted'], $paths);
 	}
 
@@ -366,10 +367,10 @@ class FileAccessTest extends TestCase {
 			1,
 			1,
 			0,
+			10,
 			[],
 			true,
 			false, // exclude server-side encrypted files
-			10
 		);
 
 		$result = iterator_to_array($generator);
@@ -386,10 +387,10 @@ class FileAccessTest extends TestCase {
 			1,
 			1,
 			0,
+			1, // Limit to 1 result
 			[],
 			true,
 			true,
-			1 // Limit to 1 result
 		);
 
 		$result = iterator_to_array($generator);
@@ -406,10 +407,10 @@ class FileAccessTest extends TestCase {
 			1,
 			3, // Filter by rootId
 			0,
+			10,
 			[],
 			true,
 			true,
-			10
 		);
 
 		$result = iterator_to_array($generator);
@@ -426,10 +427,10 @@ class FileAccessTest extends TestCase {
 			2, // Filter by storage
 			6, // and by rootId
 			0,
+			10,
 			[],
 			true,
 			true,
-			10
 		);
 
 		$result = iterator_to_array($generator);