From 54bbb32f4a570ccb4d74f523ec760c2cdf65d7d7 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sat, 15 Aug 2026 08:00:14 +0200 Subject: [PATCH] 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: https://github.com/nextcloud/server/issues/63229 Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/private/Preview/Db/PreviewMapper.php | 23 +++++++- tests/lib/Preview/PreviewMapperTest.php | 72 ++++++++++++++++++++++-- 2 files changed, 89 insertions(+), 6 deletions(-) diff --git a/lib/private/Preview/Db/PreviewMapper.php b/lib/private/Preview/Db/PreviewMapper.php index 7940ab5e9b9ba..eb691bed61a66 100644 --- a/lib/private/Preview/Db/PreviewMapper.php +++ b/lib/private/Preview/Db/PreviewMapper.php @@ -31,6 +31,13 @@ class PreviewMapper extends QBMapper { private const string VERSION_TABLE_NAME = 'preview_versions'; public const MAX_CHUNK_SIZE = 1000; + // Columns selected by joinLocation() that do not belong to the previews table + private const array JOINED_COLUMN_ALIASES = [ + 'version' => 'v', + 'bucket_name' => 'l', + 'object_store_name' => 'l', + ]; + public function __construct( IDBConnection $db, private readonly IMimeTypeLoader $mimeTypeLoader, @@ -134,7 +141,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); } @@ -246,7 +253,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 { diff --git a/tests/lib/Preview/PreviewMapperTest.php b/tests/lib/Preview/PreviewMapperTest.php index f7922031cdc8a..625147092a5fb 100644 --- a/tests/lib/Preview/PreviewMapperTest.php +++ b/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,6 +22,7 @@ class PreviewMapperTest extends TestCase { private PreviewMapper $previewMapper; private IDBConnection $connection; private ISnowflakeGenerator $snowflake; + private IMimeTypeLoader $mimeTypeLoader; #[\Override] public function setUp(): void { @@ -28,6 +30,7 @@ public function setUp(): void { $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(); @@ -66,7 +69,7 @@ public function testGetAvailablePreviews(): void { $this->assertEquals('default', $previews[43][0]->getObjectStoreName()); } - private function createPreviewForFileId(int $fileId, ?int $bucket = null): string { + 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(); @@ -83,15 +86,16 @@ private function createPreviewForFileId(int $fileId, ?int $bucket = null): strin $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); @@ -101,6 +105,66 @@ private function createPreviewForFileId(int $fileId, ?int $bucket = null): strin 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);