From 1f8359ead400896065351e82f8959e02d9295e46 Mon Sep 17 00:00:00 2001 From: Jorge Montejo Date: Wed, 18 Feb 2026 22:47:45 +0100 Subject: [PATCH] fix: Download the edited version when downloading multiple photos (#26259) * fix: download the edited version when downloading multiple photos * test: update tests * chore: clean up --------- Co-authored-by: Jason Rasmussen --- mobile/openapi/README.md | 1 + mobile/openapi/lib/api.dart | 1 + mobile/openapi/lib/api/download_api.dart | 12 +- mobile/openapi/lib/api_client.dart | 2 + .../lib/model/download_archive_dto.dart | 120 ++++++++++++++++++ open-api/immich-openapi-specs.json | 22 +++- open-api/typescript-sdk/src/fetch-client.ts | 14 +- server/src/controllers/download.controller.ts | 5 +- server/src/dtos/download.dto.ts | 8 +- server/src/queries/asset.repository.sql | 17 ++- server/src/repositories/asset.repository.ts | 19 ++- server/src/services/download.service.spec.ts | 12 +- server/src/services/download.service.ts | 14 +- .../repositories/asset.repository.mock.ts | 1 + web/src/lib/utils/asset-utils.ts | 2 +- 15 files changed, 215 insertions(+), 35 deletions(-) create mode 100644 mobile/openapi/lib/model/download_archive_dto.dart diff --git a/mobile/openapi/README.md b/mobile/openapi/README.md index 4ebe5c7c65..afeeb694e1 100644 --- a/mobile/openapi/README.md +++ b/mobile/openapi/README.md @@ -416,6 +416,7 @@ Class | Method | HTTP request | Description - [DatabaseBackupDeleteDto](doc//DatabaseBackupDeleteDto.md) - [DatabaseBackupDto](doc//DatabaseBackupDto.md) - [DatabaseBackupListResponseDto](doc//DatabaseBackupListResponseDto.md) + - [DownloadArchiveDto](doc//DownloadArchiveDto.md) - [DownloadArchiveInfo](doc//DownloadArchiveInfo.md) - [DownloadInfoDto](doc//DownloadInfoDto.md) - [DownloadResponse](doc//DownloadResponse.md) diff --git a/mobile/openapi/lib/api.dart b/mobile/openapi/lib/api.dart index f10490e093..0d6a98c001 100644 --- a/mobile/openapi/lib/api.dart +++ b/mobile/openapi/lib/api.dart @@ -155,6 +155,7 @@ part 'model/database_backup_config.dart'; part 'model/database_backup_delete_dto.dart'; part 'model/database_backup_dto.dart'; part 'model/database_backup_list_response_dto.dart'; +part 'model/download_archive_dto.dart'; part 'model/download_archive_info.dart'; part 'model/download_info_dto.dart'; part 'model/download_response.dart'; diff --git a/mobile/openapi/lib/api/download_api.dart b/mobile/openapi/lib/api/download_api.dart index 5245622753..4d0c5c8165 100644 --- a/mobile/openapi/lib/api/download_api.dart +++ b/mobile/openapi/lib/api/download_api.dart @@ -24,17 +24,17 @@ class DownloadApi { /// /// Parameters: /// - /// * [AssetIdsDto] assetIdsDto (required): + /// * [DownloadArchiveDto] downloadArchiveDto (required): /// /// * [String] key: /// /// * [String] slug: - Future downloadArchiveWithHttpInfo(AssetIdsDto assetIdsDto, { String? key, String? slug, }) async { + Future downloadArchiveWithHttpInfo(DownloadArchiveDto downloadArchiveDto, { String? key, String? slug, }) async { // ignore: prefer_const_declarations final apiPath = r'/download/archive'; // ignore: prefer_final_locals - Object? postBody = assetIdsDto; + Object? postBody = downloadArchiveDto; final queryParams = []; final headerParams = {}; @@ -67,13 +67,13 @@ class DownloadApi { /// /// Parameters: /// - /// * [AssetIdsDto] assetIdsDto (required): + /// * [DownloadArchiveDto] downloadArchiveDto (required): /// /// * [String] key: /// /// * [String] slug: - Future downloadArchive(AssetIdsDto assetIdsDto, { String? key, String? slug, }) async { - final response = await downloadArchiveWithHttpInfo(assetIdsDto, key: key, slug: slug, ); + Future downloadArchive(DownloadArchiveDto downloadArchiveDto, { String? key, String? slug, }) async { + final response = await downloadArchiveWithHttpInfo(downloadArchiveDto, key: key, slug: slug, ); if (response.statusCode >= HttpStatus.badRequest) { throw ApiException(response.statusCode, await _decodeBodyBytes(response)); } diff --git a/mobile/openapi/lib/api_client.dart b/mobile/openapi/lib/api_client.dart index 470f3aec27..5aabf5cd4b 100644 --- a/mobile/openapi/lib/api_client.dart +++ b/mobile/openapi/lib/api_client.dart @@ -356,6 +356,8 @@ class ApiClient { return DatabaseBackupDto.fromJson(value); case 'DatabaseBackupListResponseDto': return DatabaseBackupListResponseDto.fromJson(value); + case 'DownloadArchiveDto': + return DownloadArchiveDto.fromJson(value); case 'DownloadArchiveInfo': return DownloadArchiveInfo.fromJson(value); case 'DownloadInfoDto': diff --git a/mobile/openapi/lib/model/download_archive_dto.dart b/mobile/openapi/lib/model/download_archive_dto.dart new file mode 100644 index 0000000000..20e8527f18 --- /dev/null +++ b/mobile/openapi/lib/model/download_archive_dto.dart @@ -0,0 +1,120 @@ +// +// AUTO-GENERATED FILE, DO NOT MODIFY! +// +// @dart=2.18 + +// ignore_for_file: unused_element, unused_import +// ignore_for_file: always_put_required_named_parameters_first +// ignore_for_file: constant_identifier_names +// ignore_for_file: lines_longer_than_80_chars + +part of openapi.api; + +class DownloadArchiveDto { + /// Returns a new [DownloadArchiveDto] instance. + DownloadArchiveDto({ + this.assetIds = const [], + this.edited, + }); + + /// Asset IDs + List assetIds; + + /// Download edited asset if available + /// + /// Please note: This property should have been non-nullable! Since the specification file + /// does not include a default value (using the "default:" property), however, the generated + /// source code must fall back to having a nullable type. + /// Consider adding a "default:" property in the specification file to hide this note. + /// + bool? edited; + + @override + bool operator ==(Object other) => identical(this, other) || other is DownloadArchiveDto && + _deepEquality.equals(other.assetIds, assetIds) && + other.edited == edited; + + @override + int get hashCode => + // ignore: unnecessary_parenthesis + (assetIds.hashCode) + + (edited == null ? 0 : edited!.hashCode); + + @override + String toString() => 'DownloadArchiveDto[assetIds=$assetIds, edited=$edited]'; + + Map toJson() { + final json = {}; + json[r'assetIds'] = this.assetIds; + if (this.edited != null) { + json[r'edited'] = this.edited; + } else { + // json[r'edited'] = null; + } + return json; + } + + /// Returns a new [DownloadArchiveDto] instance and imports its values from + /// [value] if it's a [Map], null otherwise. + // ignore: prefer_constructors_over_static_methods + static DownloadArchiveDto? fromJson(dynamic value) { + upgradeDto(value, "DownloadArchiveDto"); + if (value is Map) { + final json = value.cast(); + + return DownloadArchiveDto( + assetIds: json[r'assetIds'] is Iterable + ? (json[r'assetIds'] as Iterable).cast().toList(growable: false) + : const [], + edited: mapValueOfType(json, r'edited'), + ); + } + return null; + } + + static List listFromJson(dynamic json, {bool growable = false,}) { + final result = []; + if (json is List && json.isNotEmpty) { + for (final row in json) { + final value = DownloadArchiveDto.fromJson(row); + if (value != null) { + result.add(value); + } + } + } + return result.toList(growable: growable); + } + + static Map mapFromJson(dynamic json) { + final map = {}; + if (json is Map && json.isNotEmpty) { + json = json.cast(); // ignore: parameter_assignments + for (final entry in json.entries) { + final value = DownloadArchiveDto.fromJson(entry.value); + if (value != null) { + map[entry.key] = value; + } + } + } + return map; + } + + // maps a json object with a list of DownloadArchiveDto-objects as value to a dart map + static Map> mapListFromJson(dynamic json, {bool growable = false,}) { + final map = >{}; + if (json is Map && json.isNotEmpty) { + // ignore: parameter_assignments + json = json.cast(); + for (final entry in json.entries) { + map[entry.key] = DownloadArchiveDto.listFromJson(entry.value, growable: growable,); + } + } + return map; + } + + /// The list of required keys that must be present in a JSON. + static const requiredKeys = { + 'assetIds', + }; +} + diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 13d6ba7e56..420c7e1015 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -5052,7 +5052,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/AssetIdsDto" + "$ref": "#/components/schemas/DownloadArchiveDto" } } }, @@ -17662,6 +17662,26 @@ }, "type": "object" }, + "DownloadArchiveDto": { + "properties": { + "assetIds": { + "description": "Asset IDs", + "items": { + "format": "uuid", + "type": "string" + }, + "type": "array" + }, + "edited": { + "description": "Download edited asset if available", + "type": "boolean" + } + }, + "required": [ + "assetIds" + ], + "type": "object" + }, "DownloadArchiveInfo": { "properties": { "assetIds": { diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index 59a25d58b3..acd8109cd3 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -1132,9 +1132,11 @@ export type ValidateAccessTokenResponseDto = { /** Authentication status */ authStatus: boolean; }; -export type AssetIdsDto = { +export type DownloadArchiveDto = { /** Asset IDs */ assetIds: string[]; + /** Download edited asset if available */ + edited?: boolean; }; export type DownloadInfoDto = { /** Album ID to download */ @@ -2309,6 +2311,10 @@ export type SharedLinkEditDto = { /** Custom URL slug */ slug?: string | null; }; +export type AssetIdsDto = { + /** Asset IDs */ + assetIds: string[]; +}; export type AssetIdsResponseDto = { /** Asset ID */ assetId: string; @@ -4433,10 +4439,10 @@ export function validateAccessToken(opts?: Oazapfts.RequestOpts) { /** * Download asset archive */ -export function downloadArchive({ key, slug, assetIdsDto }: { +export function downloadArchive({ key, slug, downloadArchiveDto }: { key?: string; slug?: string; - assetIdsDto: AssetIdsDto; + downloadArchiveDto: DownloadArchiveDto; }, opts?: Oazapfts.RequestOpts) { return oazapfts.ok(oazapfts.fetchBlob<{ status: 200; @@ -4447,7 +4453,7 @@ export function downloadArchive({ key, slug, assetIdsDto }: { }))}`, oazapfts.json({ ...opts, method: "POST", - body: assetIdsDto + body: downloadArchiveDto }))); } /** diff --git a/server/src/controllers/download.controller.ts b/server/src/controllers/download.controller.ts index 942d44f4c3..e45eeb23f3 100644 --- a/server/src/controllers/download.controller.ts +++ b/server/src/controllers/download.controller.ts @@ -1,9 +1,8 @@ import { Body, Controller, HttpCode, HttpStatus, Post, StreamableFile } from '@nestjs/common'; import { ApiTags } from '@nestjs/swagger'; import { Endpoint, HistoryBuilder } from 'src/decorators'; -import { AssetIdsDto } from 'src/dtos/asset.dto'; import { AuthDto } from 'src/dtos/auth.dto'; -import { DownloadInfoDto, DownloadResponseDto } from 'src/dtos/download.dto'; +import { DownloadArchiveDto, DownloadInfoDto, DownloadResponseDto } from 'src/dtos/download.dto'; import { ApiTag, Permission } from 'src/enum'; import { Auth, Authenticated, FileResponse } from 'src/middleware/auth.guard'; import { DownloadService } from 'src/services/download.service'; @@ -36,7 +35,7 @@ export class DownloadController { 'Download a ZIP archive containing the specified assets. The assets must have been previously requested via the "getDownloadInfo" endpoint.', history: new HistoryBuilder().added('v1').beta('v1').stable('v2'), }) - downloadArchive(@Auth() auth: AuthDto, @Body() dto: AssetIdsDto): Promise { + downloadArchive(@Auth() auth: AuthDto, @Body() dto: DownloadArchiveDto): Promise { return this.service.downloadArchive(auth, dto).then(asStreamableFile); } } diff --git a/server/src/dtos/download.dto.ts b/server/src/dtos/download.dto.ts index 2f877e3c0b..ef52a72bd0 100644 --- a/server/src/dtos/download.dto.ts +++ b/server/src/dtos/download.dto.ts @@ -1,6 +1,7 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; import { IsInt, IsPositive } from 'class-validator'; -import { Optional, ValidateUUID } from 'src/validation'; +import { AssetIdsDto } from 'src/dtos/asset.dto'; +import { Optional, ValidateBoolean, ValidateUUID } from 'src/validation'; export class DownloadInfoDto { @ValidateUUID({ each: true, optional: true, description: 'Asset IDs to download' }) @@ -32,3 +33,8 @@ export class DownloadArchiveInfo { @ApiProperty({ description: 'Asset IDs in this archive' }) assetIds!: string[]; } + +export class DownloadArchiveDto extends AssetIdsDto { + @ValidateBoolean({ optional: true, description: 'Download edited asset if available' }) + edited?: boolean; +} diff --git a/server/src/queries/asset.repository.sql b/server/src/queries/asset.repository.sql index 0f3a458c35..e8cdd335e2 100644 --- a/server/src/queries/asset.repository.sql +++ b/server/src/queries/asset.repository.sql @@ -587,6 +587,7 @@ where -- AssetRepository.getForOriginal select + "asset"."id", "originalFileName", "asset_file"."path" as "editedPath", "originalPath" @@ -596,7 +597,21 @@ from and "asset_file"."isEdited" = $1 and "asset_file"."type" = $2 where - "asset"."id" = $3 + "asset"."id" in ($3) + +-- AssetRepository.getForOriginals +select + "asset"."id", + "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" in ($3) -- AssetRepository.getForThumbnail select diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 1a060c4715..d99d8cbab2 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -1008,12 +1008,12 @@ export class AssetRepository { return count; } - @GenerateSql({ params: [DummyValue.UUID, true] }) - async getForOriginal(id: string, isEdited: boolean) { + private buildGetForOriginal(ids: string[], isEdited: boolean) { return this.db .selectFrom('asset') + .select('asset.id') .select('originalFileName') - .where('asset.id', '=', id) + .where('asset.id', 'in', ids) .$if(isEdited, (qb) => qb .leftJoin('asset_file', (join) => @@ -1024,8 +1024,17 @@ export class AssetRepository { ) .select('asset_file.path as editedPath'), ) - .select('originalPath') - .executeTakeFirstOrThrow(); + .select('originalPath'); + } + + @GenerateSql({ params: [DummyValue.UUID, true] }) + getForOriginal(id: string, isEdited: boolean) { + return this.buildGetForOriginal([id], isEdited).executeTakeFirstOrThrow(); + } + + @GenerateSql({ params: [[DummyValue.UUID], true] }) + getForOriginals(ids: string[], isEdited: boolean) { + return this.buildGetForOriginal(ids, isEdited).execute(); } @GenerateSql({ params: [DummyValue.UUID, AssetFileType.Preview, true] }) diff --git a/server/src/services/download.service.spec.ts b/server/src/services/download.service.spec.ts index ae010623d8..1ae1b0b4d8 100644 --- a/server/src/services/download.service.spec.ts +++ b/server/src/services/download.service.spec.ts @@ -39,7 +39,7 @@ describe(DownloadService.name, () => { const asset = AssetFactory.create(); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset.id, 'unknown-asset'])); - mocks.asset.getByIds.mockResolvedValue([asset]); + mocks.asset.getForOriginals.mockResolvedValue([asset]); mocks.storage.createZipStream.mockReturnValue(archiveMock); await expect(sut.downloadArchive(authStub.admin, { assetIds: [asset.id, 'unknown-asset'] })).resolves.toEqual({ @@ -62,7 +62,7 @@ describe(DownloadService.name, () => { mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset1.id, asset2.id])); mocks.storage.realpath.mockRejectedValue(new Error('Could not read file')); - mocks.asset.getByIds.mockResolvedValue([asset1, asset2]); + mocks.asset.getForOriginals.mockResolvedValue([asset1, asset2]); mocks.storage.createZipStream.mockReturnValue(archiveMock); await expect(sut.downloadArchive(authStub.admin, { assetIds: [asset1.id, asset2.id] })).resolves.toEqual({ @@ -86,7 +86,7 @@ describe(DownloadService.name, () => { const asset2 = AssetFactory.create(); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset1.id, asset2.id])); - mocks.asset.getByIds.mockResolvedValue([asset1, asset2]); + mocks.asset.getForOriginals.mockResolvedValue([asset1, asset2]); mocks.storage.createZipStream.mockReturnValue(archiveMock); await expect(sut.downloadArchive(authStub.admin, { assetIds: [asset1.id, asset2.id] })).resolves.toEqual({ @@ -108,7 +108,7 @@ describe(DownloadService.name, () => { const asset2 = AssetFactory.create({ originalFileName: 'IMG_123.jpg' }); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset1.id, asset2.id])); - mocks.asset.getByIds.mockResolvedValue([asset1, asset2]); + mocks.asset.getForOriginals.mockResolvedValue([asset1, asset2]); mocks.storage.createZipStream.mockReturnValue(archiveMock); await expect(sut.downloadArchive(authStub.admin, { assetIds: [asset1.id, asset2.id] })).resolves.toEqual({ @@ -130,7 +130,7 @@ describe(DownloadService.name, () => { const asset2 = AssetFactory.create({ originalFileName: 'IMG_123.jpg' }); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset1.id, asset2.id])); - mocks.asset.getByIds.mockResolvedValue([asset2, asset1]); + mocks.asset.getForOriginals.mockResolvedValue([asset1, asset2]); mocks.storage.createZipStream.mockReturnValue(archiveMock); await expect(sut.downloadArchive(authStub.admin, { assetIds: [asset1.id, asset2.id] })).resolves.toEqual({ @@ -151,7 +151,7 @@ describe(DownloadService.name, () => { const asset = AssetFactory.create({ originalPath: '/path/to/symlink.jpg' }); mocks.access.asset.checkOwnerAccess.mockResolvedValue(new Set([asset.id])); - mocks.asset.getByIds.mockResolvedValue([asset]); + mocks.asset.getForOriginals.mockResolvedValue([asset]); mocks.storage.realpath.mockResolvedValue('/path/to/realpath.jpg'); mocks.storage.createZipStream.mockReturnValue(archiveMock); diff --git a/server/src/services/download.service.ts b/server/src/services/download.service.ts index a5f734e59c..8d939e9635 100644 --- a/server/src/services/download.service.ts +++ b/server/src/services/download.service.ts @@ -1,9 +1,8 @@ import { BadRequestException, Injectable } from '@nestjs/common'; import { parse } from 'node:path'; import { StorageCore } from 'src/cores/storage.core'; -import { AssetIdsDto } from 'src/dtos/asset.dto'; import { AuthDto } from 'src/dtos/auth.dto'; -import { DownloadArchiveInfo, DownloadInfoDto, DownloadResponseDto } from 'src/dtos/download.dto'; +import { DownloadArchiveDto, DownloadArchiveInfo, DownloadInfoDto, DownloadResponseDto } from 'src/dtos/download.dto'; import { Permission } from 'src/enum'; import { ImmichReadStream } from 'src/repositories/storage.repository'; import { BaseService } from 'src/services/base.service'; @@ -80,11 +79,11 @@ export class DownloadService extends BaseService { return { totalSize, archives }; } - async downloadArchive(auth: AuthDto, dto: AssetIdsDto): Promise { + async downloadArchive(auth: AuthDto, dto: DownloadArchiveDto): Promise { await this.requireAccess({ auth, permission: Permission.AssetDownload, ids: dto.assetIds }); const zip = this.storageRepository.createZipStream(); - const assets = await this.assetRepository.getByIds(dto.assetIds); + const assets = await this.assetRepository.getForOriginals(dto.assetIds, dto.edited ?? false); const assetMap = new Map(assets.map((asset) => [asset.id, asset])); const paths: Record = {}; @@ -94,7 +93,7 @@ export class DownloadService extends BaseService { continue; } - const { originalPath, originalFileName } = asset; + const { originalPath, editedPath, originalFileName } = asset; let filename = originalFileName; const count = paths[filename] || 0; @@ -104,9 +103,10 @@ export class DownloadService extends BaseService { filename = `${parsedFilename.name}+${count}${parsedFilename.ext}`; } - let realpath = originalPath; + let realpath = dto.edited && editedPath ? editedPath : originalPath; + try { - realpath = await this.storageRepository.realpath(originalPath); + realpath = await this.storageRepository.realpath(realpath); } catch { this.logger.warn('Unable to resolve realpath', { originalPath }); } diff --git a/server/test/repositories/asset.repository.mock.ts b/server/test/repositories/asset.repository.mock.ts index 55dcf6456f..ea7162c77a 100644 --- a/server/test/repositories/asset.repository.mock.ts +++ b/server/test/repositories/asset.repository.mock.ts @@ -51,6 +51,7 @@ export const newAssetRepositoryMock = (): Mocked downloadManager.update(downloadKey, event.loaded), });