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

joinLocation() joins previews (p) with preview_locations (l) and
preview_versions (v). Both previews and preview_versions have a file_id
column, so any unqualified file_id condition is ambiguous and MySQL or
MariaDB reject the query with error 1052.

getPreviewForSpecification() built its conditions straight from the
caller's array keys, so this broke every preview save: savePreview() uses
that lookup to recover the existing row after a unique constraint
violation. getByFileId() had the same unqualified condition, while
getAvailablePreviewsForFile() next to it already used p.file_id.

Columns that come from the joined tables keep resolving to their own
alias, and keys that already carry one are passed through untouched.
The values are bound with an explicit type as well. An untyped false
binds as an empty string, which PostgreSQL rejects for a boolean column,
so qualifying the columns on their own only moved the error on that
backend.

Fixes: #63229
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>

Author: solracsf

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,
@@ -126,7 +133,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);
 	}
 
@@ -234,7 +241,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());
 	}
 }