Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
solracsf wants to merge 1 commit into
base: master
Choose a base branch
from
+89 −6
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions lib/private/Preview/Db/PreviewMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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 {
Expand Down
72 changes: 68 additions & 4 deletions tests/lib/Preview/PreviewMapperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -21,13 +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();
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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);
Expand Down
Loading