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 <jason@rasm.me>

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)

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';

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<Response> downloadArchiveWithHttpInfo(AssetIdsDto assetIdsDto, { String? key, String? slug, }) async {
+  Future<Response> 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 = <QueryParam>[];
     final headerParams = <String, String>{};
@@ -67,13 +67,13 @@ class DownloadApi {
   ///
   /// Parameters:
   ///
-  /// * [AssetIdsDto] assetIdsDto (required):
+  /// * [DownloadArchiveDto] downloadArchiveDto (required):
   ///
   /// * [String] key:
   ///
   /// * [String] slug:
-  Future<MultipartFile?> downloadArchive(AssetIdsDto assetIdsDto, { String? key, String? slug, }) async {
-    final response = await downloadArchiveWithHttpInfo(assetIdsDto,  key: key, slug: slug, );
+  Future<MultipartFile?> 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));
     }

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':

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<String> 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<String, dynamic> toJson() {
+    final json = <String, dynamic>{};
+      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<String, dynamic>();
+
+      return DownloadArchiveDto(
+        assetIds: json[r'assetIds'] is Iterable
+            ? (json[r'assetIds'] as Iterable).cast<String>().toList(growable: false)
+            : const [],
+        edited: mapValueOfType<bool>(json, r'edited'),
+      );
+    }
+    return null;
+  }
+
+  static List<DownloadArchiveDto> listFromJson(dynamic json, {bool growable = false,}) {
+    final result = <DownloadArchiveDto>[];
+    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<String, DownloadArchiveDto> mapFromJson(dynamic json) {
+    final map = <String, DownloadArchiveDto>{};
+    if (json is Map && json.isNotEmpty) {
+      json = json.cast<String, dynamic>(); // 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<String, List<DownloadArchiveDto>> mapListFromJson(dynamic json, {bool growable = false,}) {
+    final map = <String, List<DownloadArchiveDto>>{};
+    if (json is Map && json.isNotEmpty) {
+      // ignore: parameter_assignments
+      json = json.cast<String, dynamic>();
+      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 = <String>{
+    'assetIds',
+  };
+}
+

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": {

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
     })));
 }
 /**

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<StreamableFile> {
+  downloadArchive(@Auth() auth: AuthDto, @Body() dto: DownloadArchiveDto): Promise<StreamableFile> {
     return this.service.downloadArchive(auth, dto).then(asStreamableFile);
   }
 }

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;
+}

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

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] })

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);
 

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<ImmichReadStream> {
+  async downloadArchive(auth: AuthDto, dto: DownloadArchiveDto): Promise<ImmichReadStream> {
     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<string, number> = {};
 
@@ -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 });
       }

server/test/repositories/asset.repository.mock.ts

@@ -51,6 +51,7 @@ export const newAssetRepositoryMock = (): Mocked<RepositoryInterface<AssetReposi
     deleteMetadataByKey: vitest.fn(),
     deleteBulkMetadata: vitest.fn(),
     getForOriginal: vitest.fn(),
+    getForOriginals: vitest.fn(),
     getForThumbnail: vitest.fn(),
     getForVideo: vitest.fn(),
   };

web/src/lib/utils/asset-utils.ts

@@ -212,7 +212,7 @@ export const downloadArchive = async (fileName: string, options: Omit<DownloadIn
       const { data } = await downloadRequest({
         method: 'POST',
         url: getBaseUrl() + '/download/archive' + (queryParams ? `?${queryParams}` : ''),
-        data: { assetIds: archive.assetIds },
+        data: { assetIds: archive.assetIds, edited: true },
         signal: abort.signal,
         onDownloadProgress: (event) => downloadManager.update(downloadKey, event.loaded),
       });