From b5222a1b7acfd3fe1d15ed714ff0aeb727d4494f 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> Signed-off-by: Carl Schwan --- lib/private/Preview/Db/PreviewMapper.php | 23 ++++++- tests/lib/Preview/PreviewMapperTest.php | 85 ++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/lib/private/Preview/Db/PreviewMapper.php b/lib/private/Preview/Db/PreviewMapper.php index d6508c64afe52..5db2efd8160e5 100644 --- a/lib/private/Preview/Db/PreviewMapper.php +++ b/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 { diff --git a/tests/lib/Preview/PreviewMapperTest.php b/tests/lib/Preview/PreviewMapperTest.php index b4a392b78f9f5..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,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()); } }