Merge pull request #63346 from nextcloud/backport/63286/stable33

[stable33] fix(preview): qualify the ambiguous columns in PreviewMapper queries

Author: susnux

lib/private/Preview/Db/PreviewMapper.php

@@ -28,6 +28,13 @@ class PreviewMapper extends QBMapper {
 	private const LOCATION_TABLE_NAME = 'preview_locations';
 	private const VERSION_TABLE_NAME = 'preview_versions';
 
+	// Columns selected by joinLocation() that do not belong to the previews table
+	private const JOINED_COLUMN_ALIASES = [
+		'version' => 'v',
+		'bucket_name' => 'l',
+		'object_store_name' => 'l',
+	];
+
 	public function __construct(
 		IDBConnection $db,
 		private readonly IMimeTypeLoader $mimeTypeLoader,
@@ -130,7 +137,7 @@ public function getAvailablePreviews(array $fileIds): array {
 	public function getByFileId(int $fileId): \Generator {
 		$selectQb = $this->db->getQueryBuilder();
 		$this->joinLocation($selectQb)
-			->where($selectQb->expr()->eq('file_id', $selectQb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT)));
+			->where($selectQb->expr()->eq('p.file_id', $selectQb->createNamedParameter($fileId, IQueryBuilder::PARAM_INT)));
 		yield from $this->yieldEntities($selectQb);
 	}
 
@@ -238,7 +245,19 @@ public function getPreviewForSpecification(array $parameters): ?Preview {
 		$this->joinLocation($qb);
 
 		foreach ($parameters as $key => $value) {
-			$qb->andWhere($qb->expr()->eq($key, $qb->createNamedParameter($value)));
+			// The previews table is joined with preview_versions, which shares
+			// the file_id column name, so plain column names have to be aliased.
+			$column = str_contains($key, '.')
+				? $key
+				: (self::JOINED_COLUMN_ALIASES[$key] ?? 'p') . '.' . $key;
+			// An untyped false binds as an empty string, which PostgreSQL
+			// rejects for a boolean column.
+			$type = match (true) {
+				is_bool($value) => IQueryBuilder::PARAM_BOOL,
+				is_int($value) => IQueryBuilder::PARAM_INT,
+				default => IQueryBuilder::PARAM_STR,
+			};
+			$qb->andWhere($qb->expr()->eq($column, $qb->createNamedParameter($value, $type)));
 		}
 
 		try {

tests/lib/Preview/PreviewMapperTest.php

@@ -11,6 +11,7 @@
 
 use OC\Preview\Db\Preview;
 use OC\Preview\Db\PreviewMapper;
+use OCP\Files\IMimeTypeLoader;
 use OCP\IDBConnection;
 use OCP\Server;
 use OCP\Snowflake\ISnowflakeGenerator;
@@ -21,12 +22,15 @@ class PreviewMapperTest extends TestCase {
 	private PreviewMapper $previewMapper;
 	private IDBConnection $connection;
 	private ISnowflakeGenerator $snowflake;
+	private IMimeTypeLoader $mimeTypeLoader;
 
+	#[\Override]
 	public function setUp(): void {
 		parent::setUp();
 		$this->previewMapper = Server::get(PreviewMapper::class);
 		$this->connection = Server::get(IDBConnection::class);
 		$this->snowflake = Server::get(ISnowflakeGenerator::class);
+		$this->mimeTypeLoader = Server::get(IMimeTypeLoader::class);
 
 		$qb = $this->connection->getQueryBuilder();
 		$qb->delete('preview_locations')->executeStatement();
@@ -38,6 +42,7 @@ public function setUp(): void {
 		$qb->delete('previews')->executeStatement();
 	}
 
+	#[\Override]
 	public function tearDown(): void {
 		$this->previewMapper->deleteAll();
 		parent::tearDown();
@@ -64,7 +69,7 @@ public function testGetAvailablePreviews(): void {
 		$this->assertEquals('default', $previews[43][0]->getObjectStoreName());
 	}
 
-	private function createPreviewForFileId(int $fileId, ?int $bucket = null): void {
+	private function createPreviewForFileId(int $fileId, ?int $bucket = null, int $size = 100, ?string $version = null, bool $cropped = true): string {
 		$locationId = null;
 		if ($bucket) {
 			$qb = $this->connection->getQueryBuilder();
@@ -81,19 +86,91 @@ private function createPreviewForFileId(int $fileId, ?int $bucket = null): void
 		$preview->generateId();
 		$preview->setFileId($fileId);
 		$preview->setStorageId(1);
-		$preview->setCropped(true);
+		$preview->setCropped($cropped);
 		$preview->setMax(true);
-		$preview->setWidth(100);
+		$preview->setWidth($size);
 		$preview->setSourceMimeType('image/jpeg');
-		$preview->setHeight(100);
+		$preview->setHeight($size);
 		$preview->setSize(100);
 		$preview->setMtime(time());
 		$preview->setMimetype('image/jpeg');
 		$preview->setEtag('abcdefg');
+		$preview->setVersion($version);
 
 		if ($locationId !== null) {
 			$preview->setLocationId($locationId);
 		}
 		$this->previewMapper->insert($preview);
+
+		return $preview->id;
+	}
+
+	/**
+	 * The previews table is joined with preview_versions, which also has a
+	 * file_id column, so the condition has to be qualified with the alias.
+	 */
+	public function testGetByFileId(): void {
+		$fileId = 4242;
+		$this->createPreviewForFileId($fileId);
+		$this->createPreviewForFileId($fileId, size: 256);
+		$this->createPreviewForFileId(4243);
+
+		$previews = iterator_to_array($this->previewMapper->getByFileId($fileId));
+
+		$this->assertCount(2, $previews);
+		foreach ($previews as $preview) {
+			$this->assertSame($fileId, $preview->getFileId());
+		}
+	}
+
+	/**
+	 * Same ambiguity, reached through the specification lookup that
+	 * Generator::savePreview() uses to recover from a unique constraint
+	 * violation. It passes the cropped flag as a PHP bool, and false is the
+	 * common case, so both values have to be covered.
+	 */
+	#[\PHPUnit\Framework\Attributes\TestWith([false])]
+	#[\PHPUnit\Framework\Attributes\TestWith([true])]
+	public function testGetPreviewForSpecification(bool $cropped): void {
+		$fileId = 4244;
+		$previewId = $this->createPreviewForFileId($fileId, cropped: $cropped);
+
+		$preview = $this->previewMapper->getPreviewForSpecification([
+			'file_id' => $fileId,
+			'width' => 100,
+			'height' => 100,
+			'mimetype_id' => $this->mimeTypeLoader->getId('image/jpeg'),
+			'cropped' => $cropped,
+			'version_id' => '-1',
+		]);
+
+		$this->assertNotNull($preview);
+		$this->assertEquals($previewId, $preview->getId());
+	}
+
+	/**
+	 * version lives in the joined preview_versions table, so it has to keep
+	 * resolving to that alias rather than to the previews table.
+	 */
+	public function testGetPreviewForSpecificationOnJoinedColumn(): void {
+		$fileId = 4245;
+		$previewId = $this->createPreviewForFileId($fileId, version: '1000');
+
+		$preview = $this->previewMapper->getPreviewForSpecification([
+			'file_id' => $fileId,
+			'version' => '1000',
+		]);
+
+		$this->assertNotNull($preview);
+		$this->assertEquals($previewId, $preview->getId());
+	}
+
+	public function testLargeIdInsertRetrieve(): void {
+		$fileId = PHP_INT_MAX;
+		$originalPreviewId = $this->createPreviewForFileId($fileId);
+
+		$dbPreview = $this->previewMapper->getAvailablePreviews([$fileId])[$fileId][0];
+		$this->assertEquals($originalPreviewId, $dbPreview->id);
+		$this->assertEquals($fileId, $dbPreview->getFileId());
 	}
 }