From 984fb12adae6245e19eddb58d4501edf8e3f40f0 Mon Sep 17 00:00:00 2001 From: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com> Date: Fri, 23 Jan 2026 15:07:57 -0600 Subject: [PATCH] refactor: asset media service queries (#25477) --- server/src/queries/asset.repository.sql | 37 +++++++ server/src/repositories/asset.repository.ts | 43 ++++++++ .../src/services/asset-media.service.spec.ts | 102 +++--------------- server/src/services/asset-media.service.ts | 97 ++++++----------- .../repositories/asset.repository.mock.ts | 3 + 5 files changed, 133 insertions(+), 149 deletions(-) diff --git a/server/src/queries/asset.repository.sql b/server/src/queries/asset.repository.sql index abaae84036..f564be1850 100644 --- a/server/src/queries/asset.repository.sql +++ b/server/src/queries/asset.repository.sql @@ -585,3 +585,40 @@ where and "libraryId" = $2::uuid and "isExternal" = $3 ) + +-- AssetRepository.getForOriginal +select + "originalFileName", + "asset_file"."path" as "editedPath", + "originalPath" +from + "asset" + left join "asset_file" on "asset"."id" = "asset_file"."assetId" + and "asset_file"."isEdited" = $1 + and "asset_file"."type" = $2 +where + "asset"."id" = $3 + +-- AssetRepository.getForThumbnail +select + "asset_file"."path", + "asset"."originalPath", + "asset"."originalFileName" +from + "asset_file" + right join "asset" on "asset"."id" = "asset_file"."assetId" +where + "asset_file"."assetId" = $1 + and "asset_file"."type" = $2 +order by + "asset_file"."isEdited" desc + +-- AssetRepository.getForVideo +select + "asset"."encodedVideoPath", + "asset"."originalPath" +from + "asset" +where + "asset"."id" = $1 + and "asset"."type" = $2 diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 944417c0ad..63a397a59d 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -1009,4 +1009,47 @@ export class AssetRepository { return count; } + + @GenerateSql({ params: [DummyValue.UUID, true] }) + async getForOriginal(id: string, isEdited: boolean) { + return this.db + .selectFrom('asset') + .select('originalFileName') + .where('asset.id', '=', id) + .$if(isEdited, (qb) => + qb + .leftJoin('asset_file', (join) => + join + .onRef('asset.id', '=', 'asset_file.assetId') + .on('asset_file.isEdited', '=', true) + .on('asset_file.type', '=', AssetFileType.FullSize), + ) + .select('asset_file.path as editedPath'), + ) + .select('originalPath') + .executeTakeFirstOrThrow(); + } + + @GenerateSql({ params: [DummyValue.UUID, AssetFileType.Preview] }) + async getForThumbnail(id: string, type: AssetFileType) { + return this.db + .selectFrom('asset_file') + .select('asset_file.path') + .where('asset_file.assetId', '=', id) + .where('asset_file.type', '=', type) + .rightJoin('asset', (join) => join.onRef('asset.id', '=', 'asset_file.assetId')) + .select(['asset.originalPath', 'asset.originalFileName']) + .orderBy('asset_file.isEdited', 'desc') + .executeTakeFirstOrThrow(); + } + + @GenerateSql({ params: [DummyValue.UUID] }) + async getForVideo(id: string) { + return this.db + .selectFrom('asset') + .select(['asset.encodedVideoPath', 'asset.originalPath']) + .where('asset.id', '=', id) + .where('asset.type', '=', AssetType.Video) + .executeTakeFirst(); + } } diff --git a/server/src/services/asset-media.service.spec.ts b/server/src/services/asset-media.service.spec.ts index 0d099598e5..91beeeb777 100644 --- a/server/src/services/asset-media.service.spec.ts +++ b/server/src/services/asset-media.service.spec.ts @@ -500,17 +500,9 @@ describe(AssetMediaService.name, () => { expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith(authStub.admin.user.id, new Set(['asset-1'])); }); - it('should throw an error if the asset is not found', async () => { - mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - - await expect(sut.downloadOriginal(authStub.admin, 'asset-1', {})).rejects.toBeInstanceOf(NotFoundException); - - expect(mocks.asset.getById).toHaveBeenCalledWith('asset-1', { files: true, edits: true }); - }); - it('should download a file', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - mocks.asset.getById.mockResolvedValue(assetStub.image); + mocks.asset.getForOriginal.mockResolvedValue(assetStub.image); await expect(sut.downloadOriginal(authStub.admin, 'asset-1', {})).resolves.toEqual( new ImmichFileResponse({ @@ -536,7 +528,10 @@ describe(AssetMediaService.name, () => { ], }; mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - mocks.asset.getById.mockResolvedValue(editedAsset); + mocks.asset.getForOriginal.mockResolvedValue({ + ...editedAsset, + editedPath: '/uploads/user-id/fullsize/edited.jpg', + }); await expect(sut.downloadOriginal(authStub.admin, 'asset-1', { edited: true })).resolves.toEqual( new ImmichFileResponse({ @@ -562,7 +557,10 @@ describe(AssetMediaService.name, () => { ], }; mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - mocks.asset.getById.mockResolvedValue(editedAsset); + mocks.asset.getForOriginal.mockResolvedValue({ + ...editedAsset, + editedPath: '/uploads/user-id/fullsize/edited.jpg', + }); await expect(sut.downloadOriginal(authStub.admin, 'asset-1', { edited: true })).resolves.toEqual( new ImmichFileResponse({ @@ -588,7 +586,7 @@ describe(AssetMediaService.name, () => { ], }; mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - mocks.asset.getById.mockResolvedValue(editedAsset); + mocks.asset.getForOriginal.mockResolvedValue(editedAsset); await expect(sut.downloadOriginal(authStub.admin, 'asset-1', { edited: false })).resolves.toEqual( new ImmichFileResponse({ @@ -600,23 +598,9 @@ describe(AssetMediaService.name, () => { ); }); - it('should download original file when no edits exist', async () => { - mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - mocks.asset.getById.mockResolvedValue(assetStub.image); - - await expect(sut.downloadOriginal(authStub.admin, 'asset-1', { edited: true })).resolves.toEqual( - new ImmichFileResponse({ - path: '/original/path.jpg', - fileName: 'asset-id.jpg', - contentType: 'image/jpeg', - cacheControl: CacheControl.PrivateWithCache, - }), - ); - }); - it('should throw a not found when edits exist but no edited file available', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set(['asset-1'])); - mocks.asset.getById.mockResolvedValue(assetStub.withCropEdit); + mocks.asset.getForOriginal.mockResolvedValue({ ...assetStub.withCropEdit, editedPath: null }); await expect(sut.downloadOriginal(authStub.admin, 'asset-1', { edited: true })).rejects.toBeInstanceOf( NotFoundException, @@ -633,54 +617,9 @@ describe(AssetMediaService.name, () => { expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith(userStub.admin.id, new Set(['id'])); }); - it('should throw an error if the asset does not exist', async () => { - mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - - await expect( - sut.viewThumbnail(authStub.admin, assetStub.image.id, { size: AssetMediaSize.PREVIEW }), - ).rejects.toBeInstanceOf(NotFoundException); - }); - - it('should throw an error if the requested thumbnail file does not exist', async () => { - mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - mocks.asset.getById.mockResolvedValue({ ...assetStub.image, files: [] }); - - await expect( - sut.viewThumbnail(authStub.admin, assetStub.image.id, { size: AssetMediaSize.THUMBNAIL }), - ).rejects.toBeInstanceOf(NotFoundException); - }); - - it('should throw an error if the requested preview file does not exist', async () => { - mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - mocks.asset.getById.mockResolvedValue({ - ...assetStub.image, - files: [ - { - id: '42', - path: '/path/to/preview', - type: AssetFileType.Thumbnail, - isEdited: false, - }, - ], - }); - await expect( - sut.viewThumbnail(authStub.admin, assetStub.image.id, { size: AssetMediaSize.PREVIEW }), - ).rejects.toBeInstanceOf(NotFoundException); - }); - it('should fall back to preview if the requested thumbnail file does not exist', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - mocks.asset.getById.mockResolvedValue({ - ...assetStub.image, - files: [ - { - id: '42', - path: '/path/to/preview.jpg', - type: AssetFileType.Preview, - isEdited: false, - }, - ], - }); + mocks.asset.getForThumbnail.mockResolvedValue({ ...assetStub.image, path: '/path/to/preview.jpg' }); await expect( sut.viewThumbnail(authStub.admin, assetStub.image.id, { size: AssetMediaSize.THUMBNAIL }), @@ -696,7 +635,7 @@ describe(AssetMediaService.name, () => { it('should get preview file', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - mocks.asset.getById.mockResolvedValue({ ...assetStub.image }); + mocks.asset.getForThumbnail.mockResolvedValue({ ...assetStub.image, path: '/uploads/user-id/thumbs/path.jpg' }); await expect( sut.viewThumbnail(authStub.admin, assetStub.image.id, { size: AssetMediaSize.PREVIEW }), ).resolves.toEqual( @@ -711,7 +650,7 @@ describe(AssetMediaService.name, () => { it('should get thumbnail file', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - mocks.asset.getById.mockResolvedValue({ ...assetStub.image }); + mocks.asset.getForThumbnail.mockResolvedValue({ ...assetStub.image, path: '/uploads/user-id/webp/path.ext' }); await expect( sut.viewThumbnail(authStub.admin, assetStub.image.id, { size: AssetMediaSize.THUMBNAIL }), ).resolves.toEqual( @@ -736,22 +675,15 @@ describe(AssetMediaService.name, () => { expect(mocks.access.asset.checkPartnerAccess).toHaveBeenCalledWith(userStub.admin.id, new Set(['id'])); }); - it('should throw an error if the asset does not exist', async () => { + it('should throw an error if the video asset could not be found', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); await expect(sut.playbackVideo(authStub.admin, assetStub.image.id)).rejects.toBeInstanceOf(NotFoundException); }); - it('should throw an error if the asset is not a video', async () => { - mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.image.id])); - mocks.asset.getById.mockResolvedValue(assetStub.image); - - await expect(sut.playbackVideo(authStub.admin, assetStub.image.id)).rejects.toBeInstanceOf(BadRequestException); - }); - it('should return the encoded video path if available', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.hasEncodedVideo.id])); - mocks.asset.getById.mockResolvedValue(assetStub.hasEncodedVideo); + mocks.asset.getForVideo.mockResolvedValue(assetStub.hasEncodedVideo); await expect(sut.playbackVideo(authStub.admin, assetStub.hasEncodedVideo.id)).resolves.toEqual( new ImmichFileResponse({ @@ -764,7 +696,7 @@ describe(AssetMediaService.name, () => { it('should fall back to the original path', async () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([assetStub.video.id])); - mocks.asset.getById.mockResolvedValue(assetStub.video); + mocks.asset.getForVideo.mockResolvedValue(assetStub.video); await expect(sut.playbackVideo(authStub.admin, assetStub.video.id)).resolves.toEqual( new ImmichFileResponse({ diff --git a/server/src/services/asset-media.service.ts b/server/src/services/asset-media.service.ts index c2df6397b4..ca86a43a4b 100644 --- a/server/src/services/asset-media.service.ts +++ b/server/src/services/asset-media.service.ts @@ -25,7 +25,6 @@ import { AuthDto } from 'src/dtos/auth.dto'; import { AssetFileType, AssetStatus, - AssetType, AssetVisibility, CacheControl, JobName, @@ -36,7 +35,7 @@ import { AuthRequest } from 'src/middleware/auth.guard'; import { BaseService } from 'src/services/base.service'; import { UploadFile, UploadRequest } from 'src/types'; import { requireUploadAccess } from 'src/utils/access'; -import { asUploadRequest, getAssetFiles, onBeforeLink } from 'src/utils/asset.util'; +import { asUploadRequest, onBeforeLink } from 'src/utils/asset.util'; import { isAssetChecksumConstraint } from 'src/utils/database'; import { getFilenameExtension, getFileNameWithoutExtension, ImmichFileResponse } from 'src/utils/file'; import { mimeTypes } from 'src/utils/mime-types'; @@ -197,27 +196,21 @@ export class AssetMediaService extends BaseService { async downloadOriginal(auth: AuthDto, id: string, dto: AssetDownloadOriginalDto): Promise { await this.requireAccess({ auth, permission: Permission.AssetDownload, ids: [id] }); - const asset = await this.findOrFail(id); + const { originalPath, originalFileName, editedPath } = await this.assetRepository.getForOriginal( + id, + dto.edited ?? false, + ); - if (asset.edits!.length > 0 && (dto.edited ?? false)) { - const { editedFullsizeFile } = getAssetFiles(asset.files ?? []); - - if (!editedFullsizeFile) { - throw new NotFoundException('Edited asset media not found'); - } - - return new ImmichFileResponse({ - path: editedFullsizeFile.path, - fileName: getFileNameWithoutExtension(asset.originalFileName) + getFilenameExtension(editedFullsizeFile.path), - contentType: mimeTypes.lookup(editedFullsizeFile.path), - cacheControl: CacheControl.PrivateWithCache, - }); + if (dto.edited && !editedPath) { + throw new NotFoundException('Edited asset media not found'); } + const path = editedPath ?? originalPath!; + return new ImmichFileResponse({ - path: asset.originalPath, - fileName: asset.originalFileName, - contentType: mimeTypes.lookup(asset.originalPath), + path, + fileName: getFileNameWithoutExtension(originalFileName) + getFilenameExtension(path), + contentType: mimeTypes.lookup(path), cacheControl: CacheControl.PrivateWithCache, }); } @@ -229,45 +222,30 @@ export class AssetMediaService extends BaseService { ): Promise { await this.requireAccess({ auth, permission: Permission.AssetView, ids: [id] }); - const asset = await this.findOrFail(id); - const size = dto.size ?? AssetMediaSize.THUMBNAIL; - - const files = getAssetFiles(asset.files ?? []); - - const requestingEdited = (dto.edited ?? false) && asset.edits!.length > 0; - const { fullsizeFile, previewFile, thumbnailFile } = { - fullsizeFile: requestingEdited ? files.editedFullsizeFile : files.fullsizeFile, - previewFile: requestingEdited ? files.editedPreviewFile : files.previewFile, - thumbnailFile: requestingEdited ? files.editedThumbnailFile : files.thumbnailFile, - }; - - let filepath = previewFile?.path; - if (size === AssetMediaSize.THUMBNAIL && thumbnailFile) { - filepath = thumbnailFile.path; - } else if (size === AssetMediaSize.FULLSIZE) { - if (mimeTypes.isWebSupportedImage(asset.originalPath) && !dto.edited) { - // use original file for web supported images - return { targetSize: 'original' }; - } - if (!fullsizeFile) { - // downgrade to preview if fullsize is not available. - // e.g. disabled or not yet (re)generated - return { targetSize: AssetMediaSize.PREVIEW }; - } - filepath = fullsizeFile.path; + if (dto.size === AssetMediaSize.Original) { + throw new BadRequestException('May not request original file'); } - if (!filepath) { - throw new NotFoundException('Asset media not found'); + const size = (dto.size ?? AssetMediaSize.THUMBNAIL) as unknown as AssetFileType; + const { originalPath, originalFileName, path } = await this.assetRepository.getForThumbnail(id, size); + + if (size === AssetFileType.FullSize && mimeTypes.isWebSupportedImage(originalPath) && !dto.edited) { + // use original file for web supported images + return { targetSize: 'original' }; } - let fileName = getFileNameWithoutExtension(asset.originalFileName); - fileName += `_${size}`; - fileName += getFilenameExtension(filepath); + + if (dto.size === AssetMediaSize.FULLSIZE && !path) { + // downgrade to preview if fullsize is not available. + // e.g. disabled or not yet (re)generated + return { targetSize: AssetMediaSize.PREVIEW }; + } + + const fileName = `${getFileNameWithoutExtension(originalFileName)}_${size}${getFilenameExtension(path)}`; return new ImmichFileResponse({ fileName, - path: filepath, - contentType: mimeTypes.lookup(filepath), + path, + contentType: mimeTypes.lookup(path), cacheControl: CacheControl.PrivateWithCache, }); } @@ -275,10 +253,10 @@ export class AssetMediaService extends BaseService { async playbackVideo(auth: AuthDto, id: string): Promise { await this.requireAccess({ auth, permission: Permission.AssetView, ids: [id] }); - const asset = await this.findOrFail(id); + const asset = await this.assetRepository.getForVideo(id); - if (asset.type !== AssetType.Video) { - throw new BadRequestException('Asset is not a video'); + if (!asset) { + throw new NotFoundException('Asset not found or asset is not a video'); } const filepath = asset.encodedVideoPath || asset.originalPath; @@ -487,13 +465,4 @@ export class AssetMediaService extends BaseService { throw new BadRequestException('Quota has been exceeded!'); } } - - private async findOrFail(id: string) { - const asset = await this.assetRepository.getById(id, { files: true, edits: true }); - if (!asset) { - throw new NotFoundException('Asset not found'); - } - - return asset; - } } diff --git a/server/test/repositories/asset.repository.mock.ts b/server/test/repositories/asset.repository.mock.ts index da57485382..55dcf6456f 100644 --- a/server/test/repositories/asset.repository.mock.ts +++ b/server/test/repositories/asset.repository.mock.ts @@ -50,5 +50,8 @@ export const newAssetRepositoryMock = (): Mocked