diff --git a/mobile/lib/repositories/asset_media.repository.dart b/mobile/lib/repositories/asset_media.repository.dart index 4c9b6f6009..ce372f8b2c 100644 --- a/mobile/lib/repositories/asset_media.repository.dart +++ b/mobile/lib/repositories/asset_media.repository.dart @@ -22,7 +22,7 @@ import 'package:path/path.dart' as p; import 'package:photo_manager/photo_manager.dart'; import 'package:share_plus/share_plus.dart'; -typedef _ShareFile = ({File file, bool cleanup, String displayName}); +typedef _ShareFile = ({File file, FileSystemEntity? tempEntity, String displayName}); final assetMediaRepositoryProvider = Provider( (ref) => AssetMediaRepository(ref.watch(nativeSyncApiProvider), ref.watch(storageRepositoryProvider)), @@ -102,30 +102,60 @@ class AssetMediaRepository { } } - /// Deletes temporary files in parallel - Future _cleanupTempFiles(List tempFiles) async { + /// Deletes temporary entities in parallel, removing download task directories recursively + @visibleForTesting + static Future cleanupShareTempFiles(List tempEntities) async { await Future.wait( - tempFiles.map((file) async { + tempEntities.map((entity) async { try { - await file.delete(); + if (entity is Directory) { + await entity.delete(recursive: true); + } else { + await entity.delete(); + } } catch (e) { - _log.warning("Failed to delete temporary file: ${file.path}", e); + _log.warning("Failed to delete temporary file: ${entity.path}", e); } }), ); } - String _sanitizeFilename(String filename) { - return filename.replaceAll(RegExp(r'[\\/]'), '_'); + static final RegExp _pathSeparators = RegExp(r'[\\/]'); + + static String _sanitizeFilename(String filename) { + return filename.replaceAll(_pathSeparators, '_'); } - String _getPreviewFilename(BaseAsset asset) { + @visibleForTesting + static String getOriginalShareFilename(BaseAsset asset) { + final fallbackName = asset.remoteId ?? asset.localId ?? 'asset'; + final hasUsableName = asset.name.replaceAll(_pathSeparators, '').isNotEmpty; + return hasUsableName ? _sanitizeFilename(asset.name) : fallbackName; + } + + static String _getPreviewFilename(BaseAsset asset) { final sanitizedFilename = _sanitizeFilename(asset.name); final baseName = p.basenameWithoutExtension(sanitizedFilename); final fallbackName = asset.remoteId ?? asset.localId ?? 'asset'; return '${baseName.isEmpty ? fallbackName : baseName}-preview.jpg'; } + static String _shareDisplayName(BaseAsset asset, ShareAssetType fileType) => + switch (asset.isVideo ? ShareAssetType.original : fileType) { + ShareAssetType.original => getOriginalShareFilename(asset), + ShareAssetType.preview => _getPreviewFilename(asset), + }; + + @visibleForTesting + static Map countShareDisplayNames(List assets, ShareAssetType fileType) { + final counts = {}; + for (final asset in assets) { + final name = _shareDisplayName(asset, fileType); + counts[name] = (counts[name] ?? 0) + 1; + } + return counts; + } + bool _isCancelled(Completer? cancelCompleter) => cancelCompleter?.isCompleted ?? false; Future<_ShareFile?> _getLocalOriginalShareFile(BaseAsset asset, String localId) async { @@ -135,24 +165,44 @@ class AssetMediaRepository { return null; } - return (file: file, cleanup: CurrentPlatform.isIOS, displayName: _sanitizeFilename(asset.name)); + return (file: file, tempEntity: CurrentPlatform.isIOS ? file : null, displayName: getOriginalShareFilename(asset)); + } + + @visibleForTesting + static DownloadTask buildShareDownloadTask({ + required String taskId, + required String url, + required Map headers, + required String displayName, + required bool isDuplicate, + }) { + return DownloadTask( + taskId: taskId, + url: url, + headers: headers, + // receiving apps show the shared file's own name, so duplicates keep the task id prefix to stay unique + filename: isDuplicate ? '$taskId-$displayName' : displayName, + directory: taskId, + baseDirectory: BaseDirectory.temporary, + group: kShareDownloadGroup, + updates: Updates.statusAndProgress, + ); } Future<_ShareFile?> _downloadRemoteShareFile({ required String taskId, required String url, required String displayName, + required bool isDuplicate, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { - final task = DownloadTask( + final task = buildShareDownloadTask( taskId: taskId, url: url, headers: ApiService.getRequestHeaders(), - filename: '$taskId-$displayName', - baseDirectory: BaseDirectory.temporary, - group: kShareDownloadGroup, - updates: Updates.statusAndProgress, + displayName: displayName, + isDuplicate: isDuplicate, ); final downloader = FileDownloader(); final statusUpdate = await downloader.download( @@ -171,7 +221,8 @@ class AssetMediaRepository { } if (statusUpdate.status == TaskStatus.complete) { - return (file: File(await task.filePath()), cleanup: true, displayName: displayName); + final file = File(await task.filePath()); + return (file: file, tempEntity: file.parent, displayName: displayName); } _log.severe("Download for $displayName failed with status ${statusUpdate.status}", statusUpdate.exception); @@ -181,13 +232,15 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemoteOriginalShareFile( BaseAsset asset, String remoteId, { + required bool isDuplicate, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { return _downloadRemoteShareFile( taskId: 'share-original-$remoteId-${DateTime.now().microsecondsSinceEpoch}', url: getOriginalUrlForRemoteId(remoteId, edited: asset.isEdited), - displayName: _sanitizeFilename(asset.name), + displayName: getOriginalShareFilename(asset), + isDuplicate: isDuplicate, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -196,6 +249,7 @@ class AssetMediaRepository { Future<_ShareFile?> _getRemotePreviewShareFile( BaseAsset asset, String remoteId, { + required bool isDuplicate, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { @@ -203,6 +257,7 @@ class AssetMediaRepository { taskId: 'share-preview-$remoteId-${DateTime.now().microsecondsSinceEpoch}', url: getThumbnailUrlForRemoteId(remoteId, type: AssetMediaSize.preview, edited: asset.isEdited), displayName: _getPreviewFilename(asset), + isDuplicate: isDuplicate, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -210,6 +265,7 @@ class AssetMediaRepository { Future<_ShareFile?> _getOriginalShareFile( BaseAsset asset, { + required bool isDuplicate, Completer? cancelCompleter, required void Function(double progress) onProgress, }) { @@ -224,11 +280,18 @@ class AssetMediaRepository { return Future.value(null); } - return _getRemoteOriginalShareFile(asset, remoteId, cancelCompleter: cancelCompleter, onProgress: onProgress); + return _getRemoteOriginalShareFile( + asset, + remoteId, + isDuplicate: isDuplicate, + cancelCompleter: cancelCompleter, + onProgress: onProgress, + ); } Future<_ShareFile?> _getPreviewShareFile( BaseAsset asset, { + required bool isDuplicate, Completer? cancelCompleter, required void Function(double progress) onProgress, }) async { @@ -237,6 +300,7 @@ class AssetMediaRepository { final remotePreview = await _getRemotePreviewShareFile( asset, remoteId, + isDuplicate: isDuplicate, cancelCompleter: cancelCompleter, onProgress: onProgress, ); @@ -262,7 +326,7 @@ class AssetMediaRepository { void Function(double progress)? onAssetDownloadProgress, }) async { final downloadedXFiles = []; - final tempFiles = []; + final tempFiles = []; final totalAssets = assets.length; var processedAssets = 0; @@ -279,29 +343,34 @@ class AssetMediaRepository { updateProgress(); + final displayNameCounts = countShareDisplayNames(assets, fileType); + for (final asset in assets) { if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); return 0; } final effectiveFileType = asset.isVideo ? ShareAssetType.original : fileType; + final isDuplicate = displayNameCounts[_shareDisplayName(asset, fileType)]! > 1; final shareFile = switch (effectiveFileType) { ShareAssetType.original => await _getOriginalShareFile( asset, + isDuplicate: isDuplicate, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), ShareAssetType.preview => await _getPreviewShareFile( asset, + isDuplicate: isDuplicate, cancelCompleter: cancelCompleter, onProgress: updateProgress, ), }; if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); return 0; } @@ -312,8 +381,9 @@ class AssetMediaRepository { } downloadedXFiles.add(XFile(shareFile.file.path, name: shareFile.displayName)); - if (shareFile.cleanup) { - tempFiles.add(shareFile.file); + final tempEntity = shareFile.tempEntity; + if (tempEntity != null) { + tempFiles.add(tempEntity); } processedAssets++; updateProgress(); @@ -325,7 +395,7 @@ class AssetMediaRepository { } if (_isCancelled(cancelCompleter)) { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); return 0; } @@ -337,7 +407,7 @@ class AssetMediaRepository { downloadedXFiles, sharePositionOrigin: Rect.fromPoints(Offset.zero, Offset(size.width / 3, size.height)), ).then((result) async { - await _cleanupTempFiles(tempFiles); + await cleanupShareTempFiles(tempFiles); }), ); diff --git a/mobile/test/repositories/asset_media_repository_test.dart b/mobile/test/repositories/asset_media_repository_test.dart new file mode 100644 index 0000000000..5ef4ad9bd6 --- /dev/null +++ b/mobile/test/repositories/asset_media_repository_test.dart @@ -0,0 +1,141 @@ +import 'dart:io'; + +import 'package:background_downloader/background_downloader.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/constants/enums.dart'; +import 'package:immich_mobile/repositories/asset_media.repository.dart'; +import 'package:path/path.dart' as p; + +import '../test_utils.dart'; + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + setUpAll(() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger.setMockMethodCallHandler( + const MethodChannel('plugins.flutter.io/path_provider'), + (methodCall) async => '/tmp/immich-share-test', + ); + }); + + DownloadTask buildTask(String taskId, String displayName, {bool isDuplicate = false}) => + AssetMediaRepository.buildShareDownloadTask( + taskId: taskId, + url: 'https://example.com/api/assets/some-id/original', + headers: const {}, + displayName: displayName, + isDuplicate: isDuplicate, + ); + + group('buildShareDownloadTask', () { + test('saves a unique original under the asset name, without the task id prefix (#29468)', () async { + final task = buildTask('share-original-some-remote-id-123456', 'IMG-0001.jpg'); + + expect(task.filename, 'IMG-0001.jpg'); + expect(p.basename(await task.filePath()), 'IMG-0001.jpg'); + }); + + test('preview shares keep their -preview name', () async { + final task = buildTask('share-preview-some-remote-id-123456', 'IMG-0001-preview.jpg'); + + expect(p.basename(await task.filePath()), 'IMG-0001-preview.jpg'); + }); + + test('names with spaces and unusual characters pass through untouched', () { + const name = 'photo 1 (final) 名字.jpg'; + final task = buildTask('share-original-some-remote-id-123456', name); + + expect(task.filename, name); + }); + + test('two same-named assets download to two distinct prefixed names', () async { + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-0001.jpg'), + ]; + final counts = AssetMediaRepository.countShareDisplayNames(assets, ShareAssetType.original); + final isDuplicate = counts['IMG-0001.jpg']! > 1; + + final first = buildTask('share-original-remote-1-111', 'IMG-0001.jpg', isDuplicate: isDuplicate); + final second = buildTask('share-original-remote-2-222', 'IMG-0001.jpg', isDuplicate: isDuplicate); + + expect(p.basename(await first.filePath()), 'share-original-remote-1-111-IMG-0001.jpg'); + expect(p.basename(await second.filePath()), 'share-original-remote-2-222-IMG-0001.jpg'); + }); + + test('sharing the same asset twice downloads to distinct paths', () async { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'); + final counts = AssetMediaRepository.countShareDisplayNames([asset, asset], ShareAssetType.original); + final isDuplicate = counts['IMG-0001.jpg']! > 1; + + final first = buildTask('share-original-remote-1-111111', 'IMG-0001.jpg', isDuplicate: isDuplicate); + final second = buildTask('share-original-remote-1-222222', 'IMG-0001.jpg', isDuplicate: isDuplicate); + + expect(await first.filePath(), isNot(await second.filePath())); + }); + + test('a failed duplicate leaves the survivor with the prefixed name', () { + final assets = [ + TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'IMG-0001.jpg'), + TestUtils.createRemoteAsset(id: 'remote-2').copyWith(name: 'IMG-0001.jpg'), + ]; + final counts = AssetMediaRepository.countShareDisplayNames(assets, ShareAssetType.original); + + final survivor = buildTask( + 'share-original-remote-1-111', + 'IMG-0001.jpg', + isDuplicate: counts['IMG-0001.jpg']! > 1, + ); + + expect(survivor.filename, 'share-original-remote-1-111-IMG-0001.jpg'); + }); + }); + + group('getOriginalShareFilename', () { + test('falls back to the remote id when the name is empty', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: ''); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'remote-1'); + }); + + test('falls back when the name is only path separators', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: r'\/'); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'remote-1'); + }); + + test('falls back to the local id when there is no remote id', () { + final asset = TestUtils.createLocalAsset(id: 'local-1').copyWith(name: ''); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'local-1'); + }); + + test('sanitizes separators in a real name instead of falling back', () { + final asset = TestUtils.createRemoteAsset(id: 'remote-1').copyWith(name: 'holiday/IMG-0001.jpg'); + + expect(AssetMediaRepository.getOriginalShareFilename(asset), 'holiday_IMG-0001.jpg'); + }); + }); + + group('cleanupShareTempFiles', () { + test('removes only the owned task directories and temp files', () async { + final tempRoot = Directory.systemTemp.createTempSync('immich-share-cleanup'); + addTearDown(() => tempRoot.deleteSync(recursive: true)); + final taskDir = Directory(p.join(tempRoot.path, 'share-original-remote-1-111'))..createSync(); + final downloaded = File(p.join(taskDir.path, 'IMG-0001.jpg'))..createSync(); + final iosLocalTemp = File(p.join(tempRoot.path, 'local-original.jpg'))..createSync(); + final foreignDir = Directory(p.join(tempRoot.path, 'unrelated'))..createSync(); + final foreignFile = File(p.join(foreignDir.path, 'keep.jpg'))..createSync(); + + await AssetMediaRepository.cleanupShareTempFiles([taskDir, iosLocalTemp]); + + expect(taskDir.existsSync(), isFalse); + expect(downloaded.existsSync(), isFalse); + expect(iosLocalTemp.existsSync(), isFalse); + expect(foreignDir.existsSync(), isTrue); + expect(foreignFile.existsSync(), isTrue); + expect(tempRoot.existsSync(), isTrue); + }); + }); +}