Skip to content

fix(mobile): handle missing file extension during uploads #25033

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

Closed
shenlong-tanwen wants to merge 1 commit into from
Closed

fix(mobile): handle missing file extension during uploads #25033

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
81 changes: 60 additions & 21 deletions mobile/lib/services/upload.service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,12 @@ class UploadService {
return null;
}

final file = await _storageRepository.getFileForAsset(asset.id);
if (file == null) {
final uploadFileResult = await prepareUploadFile(asset, isLivePhoto: entity.isLivePhoto);
if (uploadFileResult == null) {
return null;
}

final originalFileName = entity.isLivePhoto ? p.setExtension(asset.name, p.extension(file.path)) : asset.name;
final (:file, :originalFilename) = uploadFileResult;

String metadata = UploadTaskMetadata(
localAssetId: asset.id,
Expand All @@ -290,7 +290,7 @@ class UploadService {
file,
createdAt: asset.createdAt,
modifiedAt: asset.updatedAt,
originalFileName: originalFileName,
originalFileName: originalFilename,
deviceAssetId: asset.id,
metadata: metadata,
group: "group",
Expand All @@ -308,8 +308,6 @@ class UploadService {
return null;
}

File? file;

/// iOS LivePhoto has two files: a photo and a video.
/// They are uploaded separately, with video file being upload first, then returned with the assetId
/// The assetId is then used as a metadata for the photo file upload task.
Expand All @@ -320,18 +318,12 @@ class UploadService {
/// The cancel operation will only cancel the video group (normal group), the photo group will not
/// be touched, as the video file is already uploaded.

if (entity.isLivePhoto) {
file = await _storageRepository.getMotionFileForAsset(asset);
} else {
file = await _storageRepository.getFileForAsset(asset.id);
}

if (file == null) {
final uploadFileResult = await prepareUploadFile(asset, isLivePhoto: entity.isLivePhoto);
if (uploadFileResult == null) {
return null;
}

final fileName = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name;
final originalFileName = entity.isLivePhoto ? p.setExtension(fileName, p.extension(file.path)) : fileName;
final (:file, :originalFilename) = uploadFileResult;

String metadata = UploadTaskMetadata(
localAssetId: asset.id,
Expand All @@ -345,7 +337,7 @@ class UploadService {
file,
createdAt: asset.createdAt,
modifiedAt: asset.updatedAt,
originalFileName: originalFileName,
originalFileName: originalFilename,
deviceAssetId: asset.id,
metadata: metadata,
group: group,
Expand All @@ -362,21 +354,20 @@ class UploadService {
return null;
}

final file = await _storageRepository.getFileForAsset(asset.id);
if (file == null) {
final result = await prepareUploadFile(asset);
if (result == null) {
return null;
}

final fields = {'livePhotoVideoId': livePhotoVideoId};

final requiresWiFi = _shouldRequireWiFi(asset);
final originalFileName = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name;

return buildUploadTask(
file,
result.file,
createdAt: asset.createdAt,
modifiedAt: asset.updatedAt,
originalFileName: originalFileName,
originalFileName: result.originalFilename,
deviceAssetId: asset.id,
fields: fields,
group: kBackupLivePhotoGroup,
Expand All @@ -398,6 +389,54 @@ class UploadService {
return requiresWiFi;
}

@visibleForTesting
Future<({File file, String originalFilename})?> prepareUploadFile(
LocalAsset asset, {
bool isLivePhoto = false,
}) async {
final file = isLivePhoto
? await _storageRepository.getMotionFileForAsset(asset)
: await _storageRepository.getFileForAsset(asset.id);

if (file == null) {
return null;
}

final originalFilename = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name;

if (isLivePhoto) {
final livePhotoFilename = p.setExtension(originalFilename, p.extension(file.path));
return (file: file, originalFilename: livePhotoFilename);
}

final filenameExt = p.extension(originalFilename);
if (filenameExt.isNotEmpty) {
return (file: file, originalFilename: originalFilename);
}

final assetNameExt = p.extension(asset.name);
if (assetNameExt.isNotEmpty) {
final correctedFilename = p.setExtension(originalFilename, assetNameExt);
_logger.fine(
"Corrected filename $originalFilename to $correctedFilename using asset.name extension $assetNameExt",
);
return (file: file, originalFilename: correctedFilename);
}

final filePathExt = p.extension(file.path);
if (filePathExt.isEmpty) {
_logger.warning(
"Asset ${asset.id} has no file extension in any source, using original filename - $originalFilename",
);
return (file: file, originalFilename: originalFilename);
}

final correctedFilename = p.setExtension(originalFilename, filePathExt);
_logger.fine("Corrected filename $originalFilename to $correctedFilename using file path extension $filePathExt");

return (file: file, originalFilename: correctedFilename);
}

Future<UploadTask> buildUploadTask(
File file, {
required String group,
Expand Down
172 changes: 171 additions & 1 deletion mobile/test/services/upload.service_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'package:drift/drift.dart' hide isNull, isNotNull;
import 'package:drift/native.dart';
import 'package:flutter/services.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:immich_mobile/domain/models/asset/base_asset.model.dart';
import 'package:immich_mobile/domain/models/store.model.dart';
import 'package:immich_mobile/domain/services/store.service.dart';
import 'package:immich_mobile/entities/store.entity.dart';
Expand All @@ -16,8 +17,8 @@ import 'package:mocktail/mocktail.dart';
import '../domain/service.mock.dart';
import '../fixtures/asset.stub.dart';
import '../infrastructure/repository.mock.dart';
import '../repository.mocks.dart';
import '../mocks/asset_entity.mock.dart';
import '../repository.mocks.dart';

void main() {
late UploadService sut;
Expand Down Expand Up @@ -165,4 +166,173 @@ void main() {
verify(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).called(1);
});
});

group('prepareUploadFile', () {
test('should keep filename with existing extension unchanged', () async {
final asset = LocalAssetStub.image1;
final mockFile = File('/tmp/123.jpg');

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'photo.jpg');

final result = await sut.prepareUploadFile(asset);
expect(result, isNotNull);
expect(result!.file.path, equals('/tmp/123.jpg'));
expect(result.originalFilename, equals('photo.jpg'));
});

test('should use asset.name extension when original filename lacks one', () async {
final asset = LocalAssetStub.image1;
final mockFile = File('/tmp/cache/123.mov');

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => '2024-10-23_17-00-30');

final result = await sut.prepareUploadFile(asset);
expect(result, isNotNull);
expect(result!.originalFilename, equals('2024-10-23_17-00-30.jpg'));
});

test('should use file path extension as final fallback', () async {
final asset = LocalAssetStub.image1.copyWith(name: 'document');
final mockFile = File('/tmp/cache/123.mov');

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'document');

final result = await sut.prepareUploadFile(asset);
expect(result, isNotNull);
expect(result!.originalFilename, equals('document.mov'));
});

test('should handle file without extension anywhere', () async {
final asset = LocalAssetStub.image1.copyWith(name: 'document');
final mockFile = File('/tmp/temp');

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'document');

final result = await sut.prepareUploadFile(asset);
expect(result, isNotNull);
expect(result!.originalFilename, equals('document'));
});

test('should preserve existing extension even if asset.name has different one', () async {
final asset = LocalAssetStub.image1;
final mockFile = File('/tmp/123.mov');

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'photo.HEIC');

final result = await sut.prepareUploadFile(asset);
expect(result, isNotNull);
expect(result!.originalFilename, equals('photo.HEIC'));
});

test('should fall back to asset.name when getOriginalFilename returns null', () async {
final asset = LocalAssetStub.image1.copyWith(name: 'VID_1234.mp4');
final mockFile = File('/tmp/video.mov');

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => null);

final result = await sut.prepareUploadFile(asset);
expect(result, isNotNull);
expect(result!.originalFilename, equals('VID_1234.mp4')); // Uses asset.name directly
});

test('should return null when file is not found', () async {
final asset = LocalAssetStub.image1;

when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => null);

final result = await sut.prepareUploadFile(asset);
expect(result, isNull);
});
});

group('getUploadTask with missing extensions', () {
test('should add extension for regular photo without extension', () async {
final asset = LocalAssetStub.image1;
final mockEntity = MockAssetEntity();
final mockFile = File('/path/to/file.jpg');

when(() => mockEntity.isLivePhoto).thenReturn(false);
when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => '2024-10-23_17-00-30');

final task = await sut.getUploadTask(asset);

expect(task, isNotNull);
expect(task!.fields['filename'], equals('2024-10-23_17-00-30.jpg'));
});

test('should preserve existing extension for regular photo', () async {
final asset = LocalAssetStub.image1;
final mockEntity = MockAssetEntity();
final mockFile = File('/path/to/file.jpg');

when(() => mockEntity.isLivePhoto).thenReturn(false);
when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'MyPhoto.HEIC');

final task = await sut.getUploadTask(asset);

expect(task, isNotNull);
expect(task!.fields['filename'], equals('MyPhoto.HEIC'));
});

test('should add extension for video without extension', () async {
final asset = LocalAssetStub.image1.copyWith(id: 'video1', name: 'VID_20241023_170030', type: AssetType.video);
final mockEntity = MockAssetEntity();
final mockFile = File('/path/to/video.mov');

when(() => mockEntity.isLivePhoto).thenReturn(false);
when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'VID_20241023_170030');

final task = await sut.getUploadTask(asset);

expect(task, isNotNull);
expect(task!.fields['filename'], equals('VID_20241023_170030.mov'));
});
});

group('getLivePhotoUploadTask with missing extensions', () {
test('should add extension when live photo filename lacks one', () async {
final asset = LocalAssetStub.image1.copyWith(name: 'IMG_1234.heic');
final mockEntity = MockAssetEntity();
final mockFile = File('/path/to/photo.heic');

when(() => mockEntity.isLivePhoto).thenReturn(true);
when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'IMG_1234');

final task = await sut.getLivePhotoUploadTask(asset, 'video-id-123');

expect(task, isNotNull);
expect(task!.fields['filename'], equals('IMG_1234.heic'));
expect(task.fields['livePhotoVideoId'], equals('video-id-123'));
});

test('should preserve extension when live photo filename has one', () async {
final asset = LocalAssetStub.image1;
final mockEntity = MockAssetEntity();
final mockFile = File('/path/to/photo.heic');

when(() => mockEntity.isLivePhoto).thenReturn(true);
when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity);
when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile);
when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'MyLivePhoto.HEIC');

final task = await sut.getLivePhotoUploadTask(asset, 'video-id-456');

expect(task, isNotNull);
expect(task!.fields['filename'], equals('MyLivePhoto.HEIC'));
});
});
}
Loading