From 0beb1f9e7ace80afeeda817b4f0cae9a4366351b Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 28 Jan 2026 15:14:50 -0600 Subject: [PATCH] fix: width and height migration issue (#25643) * fix: width and height migration issue * chore: sync stream migration tests * lint and test --------- Co-authored-by: bwees --- mobile/lib/domain/models/store.model.dart | 4 +- .../domain/services/sync_stream.service.dart | 83 ++++++++++++++++ .../repositories/sync_api.repository.dart | 4 + .../sync_migration.repository.dart | 24 +++++ .../infrastructure/sync.provider.dart | 5 + mobile/lib/utils/migration.dart | 18 +--- mobile/test/api.mocks.dart | 2 + .../services/sync_stream_service_test.dart | 99 ++++++++++++++++++- .../test/infrastructure/repository.mock.dart | 3 + 9 files changed, 219 insertions(+), 23 deletions(-) create mode 100644 mobile/lib/infrastructure/repositories/sync_migration.repository.dart diff --git a/mobile/lib/domain/models/store.model.dart b/mobile/lib/domain/models/store.model.dart index eaf9593f0b..f6bed7cf61 100644 --- a/mobile/lib/domain/models/store.model.dart +++ b/mobile/lib/domain/models/store.model.dart @@ -89,7 +89,9 @@ enum StoreKey { cleanupKeepMediaType._(1009), cleanupKeepAlbumIds._(1010), cleanupCutoffDaysAgo._(1011), - cleanupDefaultsInitialized._(1012); + cleanupDefaultsInitialized._(1012), + + syncMigrationStatus._(1013); const StoreKey._(this.id); final int id; diff --git a/mobile/lib/domain/services/sync_stream.service.dart b/mobile/lib/domain/services/sync_stream.service.dart index d5029abac8..af1c94ca71 100644 --- a/mobile/lib/domain/services/sync_stream.service.dart +++ b/mobile/lib/domain/services/sync_stream.service.dart @@ -1,4 +1,7 @@ +// ignore_for_file: constant_identifier_names + import 'dart:async'; +import 'dart:convert'; import 'package:immich_mobile/domain/models/store.model.dart'; import 'package:immich_mobile/domain/models/sync_event.model.dart'; @@ -7,12 +10,21 @@ import 'package:immich_mobile/extensions/platform_extensions.dart'; import 'package:immich_mobile/infrastructure/repositories/local_asset.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/storage.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_api.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/sync_migration.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_stream.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/trashed_local_asset.repository.dart'; import 'package:immich_mobile/repositories/local_files_manager.repository.dart'; +import 'package:immich_mobile/services/api.service.dart'; +import 'package:immich_mobile/utils/semver.dart'; import 'package:logging/logging.dart'; import 'package:openapi/api.dart'; +enum SyncMigrationTask { + v20260128_ResetExifV1, // EXIF table has incorrect width and height information. + v20260128_CopyExifWidthHeightToAsset, // Asset table has incorrect width and height for video ratio calculations. + v20260128_ResetAssetV1, // Asset v2.5.0 has width and height information that were edited assets. +} + class SyncStreamService { final Logger _logger = Logger('SyncStreamService'); @@ -22,6 +34,8 @@ class SyncStreamService { final DriftTrashedLocalAssetRepository _trashedLocalAssetRepository; final LocalFilesManagerRepository _localFilesManager; final StorageRepository _storageRepository; + final SyncMigrationRepository _syncMigrationRepository; + final ApiService _api; final bool Function()? _cancelChecker; SyncStreamService({ @@ -31,6 +45,8 @@ class SyncStreamService { required DriftTrashedLocalAssetRepository trashedLocalAssetRepository, required LocalFilesManagerRepository localFilesManager, required StorageRepository storageRepository, + required SyncMigrationRepository syncMigrationRepository, + required ApiService api, bool Function()? cancelChecker, }) : _syncApiRepository = syncApiRepository, _syncStreamRepository = syncStreamRepository, @@ -38,12 +54,32 @@ class SyncStreamService { _trashedLocalAssetRepository = trashedLocalAssetRepository, _localFilesManager = localFilesManager, _storageRepository = storageRepository, + _syncMigrationRepository = syncMigrationRepository, + _api = api, _cancelChecker = cancelChecker; bool get isCancelled => _cancelChecker?.call() ?? false; Future sync() async { _logger.info("Remote sync request for user"); + final serverVersion = await _api.serverInfoApi.getServerVersion(); + if (serverVersion == null) { + _logger.severe("Cannot perform sync: unable to determine server version"); + return false; + } + + final semVer = SemVer(major: serverVersion.major, minor: serverVersion.minor, patch: serverVersion.patch_); + + final value = Store.get(StoreKey.syncMigrationStatus, "[]"); + final migrations = (jsonDecode(value) as List).cast(); + int previousLength = migrations.length; + await _runPreSyncTasks(migrations, semVer); + + if (migrations.length != previousLength) { + _logger.info("Updated pre-sync migration status: $migrations"); + await Store.put(StoreKey.syncMigrationStatus, jsonEncode(migrations)); + } + // Start the sync stream and handle events bool shouldReset = false; await _syncApiRepository.streamChanges(_handleEvents, onReset: () => shouldReset = true); @@ -51,9 +87,56 @@ class SyncStreamService { _logger.info("Resetting sync state as requested by server"); await _syncApiRepository.streamChanges(_handleEvents); } + + previousLength = migrations.length; + await _runPostSyncTasks(migrations); + + if (migrations.length != previousLength) { + _logger.info("Updated pre-sync migration status: $migrations"); + await Store.put(StoreKey.syncMigrationStatus, jsonEncode(migrations)); + } + return true; } + Future _runPreSyncTasks(List migrations, SemVer semVer) async { + if (!migrations.contains(SyncMigrationTask.v20260128_ResetExifV1.name)) { + _logger.info("Running pre-sync task: v20260128_ResetExifV1"); + await _syncApiRepository.deleteSyncAck([ + SyncEntityType.assetExifV1, + SyncEntityType.partnerAssetExifV1, + SyncEntityType.albumAssetExifCreateV1, + SyncEntityType.albumAssetExifUpdateV1, + ]); + migrations.add(SyncMigrationTask.v20260128_ResetExifV1.name); + } + + if (!migrations.contains(SyncMigrationTask.v20260128_ResetAssetV1.name) && + semVer >= const SemVer(major: 2, minor: 5, patch: 0)) { + _logger.info("Running pre-sync task: v20260128_ResetAssetV1"); + await _syncApiRepository.deleteSyncAck([ + SyncEntityType.assetV1, + SyncEntityType.partnerAssetV1, + SyncEntityType.albumAssetCreateV1, + SyncEntityType.albumAssetUpdateV1, + ]); + + migrations.add(SyncMigrationTask.v20260128_ResetAssetV1.name); + + if (!migrations.contains(SyncMigrationTask.v20260128_CopyExifWidthHeightToAsset.name)) { + migrations.add(SyncMigrationTask.v20260128_CopyExifWidthHeightToAsset.name); + } + } + } + + Future _runPostSyncTasks(List migrations) async { + if (!migrations.contains(SyncMigrationTask.v20260128_CopyExifWidthHeightToAsset.name)) { + _logger.info("Running post-sync task: v20260128_CopyExifWidthHeightToAsset"); + await _syncMigrationRepository.v20260128CopyExifWidthHeightToAsset(); + migrations.add(SyncMigrationTask.v20260128_CopyExifWidthHeightToAsset.name); + } + } + Future _handleEvents(List events, Function() abort, Function() reset) async { List items = []; for (final event in events) { diff --git a/mobile/lib/infrastructure/repositories/sync_api.repository.dart b/mobile/lib/infrastructure/repositories/sync_api.repository.dart index 7b59803891..d13083d706 100644 --- a/mobile/lib/infrastructure/repositories/sync_api.repository.dart +++ b/mobile/lib/infrastructure/repositories/sync_api.repository.dart @@ -19,6 +19,10 @@ class SyncApiRepository { return _api.syncApi.sendSyncAck(SyncAckSetDto(acks: data)); } + Future deleteSyncAck(List types) { + return _api.syncApi.deleteSyncAck(SyncAckDeleteDto(types: types)); + } + Future streamChanges( Future Function(List, Function() abort, Function() reset) onData, { Function()? onReset, diff --git a/mobile/lib/infrastructure/repositories/sync_migration.repository.dart b/mobile/lib/infrastructure/repositories/sync_migration.repository.dart new file mode 100644 index 0000000000..814c8780ad --- /dev/null +++ b/mobile/lib/infrastructure/repositories/sync_migration.repository.dart @@ -0,0 +1,24 @@ +import 'package:immich_mobile/infrastructure/repositories/db.repository.dart'; + +class SyncMigrationRepository extends DriftDatabaseRepository { + final Drift _db; + + const SyncMigrationRepository(super.db) : _db = db; + + Future v20260128CopyExifWidthHeightToAsset() async { + await _db.customStatement(''' + UPDATE remote_asset_entity + SET width = CASE + WHEN exif.orientation IN ('5', '6', '7', '8', '-90', '90') THEN exif.height + ELSE exif.width + END, + height = CASE + WHEN exif.orientation IN ('5', '6', '7', '8', '-90', '90') THEN exif.width + ELSE exif.height + END + FROM remote_exif_entity exif + WHERE exif.asset_id = remote_asset_entity.id + AND (exif.width IS NOT NULL OR exif.height IS NOT NULL); + '''); + } +} diff --git a/mobile/lib/providers/infrastructure/sync.provider.dart b/mobile/lib/providers/infrastructure/sync.provider.dart index 29dee6f726..5b9f29225e 100644 --- a/mobile/lib/providers/infrastructure/sync.provider.dart +++ b/mobile/lib/providers/infrastructure/sync.provider.dart @@ -3,6 +3,7 @@ import 'package:immich_mobile/domain/services/hash.service.dart'; import 'package:immich_mobile/domain/services/local_sync.service.dart'; import 'package:immich_mobile/domain/services/sync_stream.service.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_api.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/sync_migration.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_stream.repository.dart'; import 'package:immich_mobile/providers/api.provider.dart'; import 'package:immich_mobile/providers/infrastructure/album.provider.dart'; @@ -13,6 +14,8 @@ import 'package:immich_mobile/providers/infrastructure/platform.provider.dart'; import 'package:immich_mobile/providers/infrastructure/storage.provider.dart'; import 'package:immich_mobile/repositories/local_files_manager.repository.dart'; +final syncMigrationRepositoryProvider = Provider((ref) => SyncMigrationRepository(ref.watch(driftProvider))); + final syncStreamServiceProvider = Provider( (ref) => SyncStreamService( syncApiRepository: ref.watch(syncApiRepositoryProvider), @@ -21,6 +24,8 @@ final syncStreamServiceProvider = Provider( trashedLocalAssetRepository: ref.watch(trashedLocalAssetRepository), localFilesManager: ref.watch(localFilesManagerRepositoryProvider), storageRepository: ref.watch(storageRepositoryProvider), + syncMigrationRepository: ref.watch(syncMigrationRepositoryProvider), + api: ref.watch(apiServiceProvider), cancelChecker: ref.watch(cancellationProvider), ), ); diff --git a/mobile/lib/utils/migration.dart b/mobile/lib/utils/migration.dart index 56a95c889f..94ae69321f 100644 --- a/mobile/lib/utils/migration.dart +++ b/mobile/lib/utils/migration.dart @@ -28,6 +28,7 @@ import 'package:immich_mobile/utils/datetime_helpers.dart'; import 'package:immich_mobile/utils/debug_print.dart'; import 'package:immich_mobile/utils/diff.dart'; import 'package:isar/isar.dart'; + // ignore: import_rule_photo_manager import 'package:photo_manager/photo_manager.dart'; @@ -88,7 +89,6 @@ Future migrateDatabaseIfNeeded(Isar db, Drift drift) async { if (version < 20 && Store.isBetaTimelineEnabled) { await _syncLocalAlbumIsIosSharedAlbum(drift); - await _backfillAssetExifWidthHeight(drift); } if (targetVersion >= 12) { @@ -282,22 +282,6 @@ Future _syncLocalAlbumIsIosSharedAlbum(Drift db) async { } } -Future _backfillAssetExifWidthHeight(Drift db) async { - try { - await db.customStatement(''' - UPDATE remote_exif_entity AS remote_exif - SET width = asset.width, - height = asset.height - FROM remote_asset_entity AS asset - WHERE remote_exif.asset_id = asset.id; - '''); - - dPrint(() => "[MIGRATION] Successfully backfilled asset exif width and height"); - } catch (error) { - dPrint(() => "[MIGRATION] Error while backfilling asset exif width and height: $error"); - } -} - Future migrateDeviceAssetToSqlite(Isar db, Drift drift) async { try { final isarDeviceAssets = await db.deviceAssetEntitys.where().findAll(); diff --git a/mobile/test/api.mocks.dart b/mobile/test/api.mocks.dart index b0a4e9b8fd..c6a3a90582 100644 --- a/mobile/test/api.mocks.dart +++ b/mobile/test/api.mocks.dart @@ -4,3 +4,5 @@ import 'package:openapi/api.dart'; class MockAssetsApi extends Mock implements AssetsApi {} class MockSyncApi extends Mock implements SyncApi {} + +class MockServerApi extends Mock implements ServerApi {} diff --git a/mobile/test/domain/services/sync_stream_service_test.dart b/mobile/test/domain/services/sync_stream_service_test.dart index 109b54a907..0eabf3b612 100644 --- a/mobile/test/domain/services/sync_stream_service_test.dart +++ b/mobile/test/domain/services/sync_stream_service_test.dart @@ -19,12 +19,15 @@ import 'package:immich_mobile/infrastructure/repositories/sync_stream.repository import 'package:immich_mobile/infrastructure/repositories/trashed_local_asset.repository.dart'; import 'package:immich_mobile/repositories/local_files_manager.repository.dart'; import 'package:mocktail/mocktail.dart'; +import 'package:openapi/api.dart'; +import '../../api.mocks.dart'; import '../../fixtures/asset.stub.dart'; import '../../fixtures/sync_stream.stub.dart'; import '../../infrastructure/repository.mock.dart'; import '../../mocks/asset_entity.mock.dart'; import '../../repository.mocks.dart'; +import '../../service.mocks.dart'; class _AbortCallbackWrapper { const _AbortCallbackWrapper(); @@ -50,6 +53,9 @@ void main() { late DriftTrashedLocalAssetRepository mockTrashedLocalAssetRepo; late LocalFilesManagerRepository mockLocalFilesManagerRepo; late StorageRepository mockStorageRepo; + late MockApiService mockApi; + late MockServerApi mockServerApi; + late MockSyncMigrationRepository mockSyncMigrationRepo; late Future Function(List, Function(), Function()) handleEventsCallback; late _MockAbortCallbackWrapper mockAbortCallbackWrapper; late _MockAbortCallbackWrapper mockResetCallbackWrapper; @@ -82,6 +88,9 @@ void main() { mockStorageRepo = MockStorageRepository(); mockAbortCallbackWrapper = _MockAbortCallbackWrapper(); mockResetCallbackWrapper = _MockAbortCallbackWrapper(); + mockApi = MockApiService(); + mockServerApi = MockServerApi(); + mockSyncMigrationRepo = MockSyncMigrationRepository(); when(() => mockAbortCallbackWrapper()).thenReturn(false); @@ -94,6 +103,12 @@ void main() { }); when(() => mockSyncApiRepo.ack(any())).thenAnswer((_) async => {}); + when(() => mockSyncApiRepo.deleteSyncAck(any())).thenAnswer((_) async => {}); + + when(() => mockApi.serverInfoApi).thenReturn(mockServerApi); + when(() => mockServerApi.getServerVersion()).thenAnswer( + (_) async => ServerVersionResponseDto(major: 1, minor: 132, patch_: 0), + ); when(() => mockSyncStreamRepo.updateUsersV1(any())).thenAnswer(successHandler); when(() => mockSyncStreamRepo.deleteUsersV1(any())).thenAnswer(successHandler); @@ -127,6 +142,7 @@ void main() { when(() => mockSyncStreamRepo.deletePeopleV1(any())).thenAnswer(successHandler); when(() => mockSyncStreamRepo.updateAssetFacesV1(any())).thenAnswer(successHandler); when(() => mockSyncStreamRepo.deleteAssetFacesV1(any())).thenAnswer(successHandler); + when(() => mockSyncMigrationRepo.v20260128CopyExifWidthHeightToAsset()).thenAnswer(successHandler); sut = SyncStreamService( syncApiRepository: mockSyncApiRepo, @@ -135,6 +151,8 @@ void main() { trashedLocalAssetRepository: mockTrashedLocalAssetRepo, localFilesManager: mockLocalFilesManagerRepo, storageRepository: mockStorageRepo, + api: mockApi, + syncMigrationRepository: mockSyncMigrationRepo, ); when(() => mockLocalAssetRepo.getAssetsFromBackupAlbums(any())).thenAnswer((_) async => {}); @@ -216,6 +234,8 @@ void main() { localFilesManager: mockLocalFilesManagerRepo, storageRepository: mockStorageRepo, cancelChecker: cancellationChecker.call, + api: mockApi, + syncMigrationRepository: mockSyncMigrationRepo, ); await sut.sync(); @@ -255,6 +275,8 @@ void main() { localFilesManager: mockLocalFilesManagerRepo, storageRepository: mockStorageRepo, cancelChecker: cancellationChecker.call, + api: mockApi, + syncMigrationRepository: mockSyncMigrationRepo, ); await sut.sync(); @@ -474,11 +496,7 @@ void main() { }); final events = [ - SyncStreamStub.assetModified( - id: 'remote-1', - checksum: 'checksum-trash', - ack: 'asset-remote-1-11', - ), + SyncStreamStub.assetModified(id: 'remote-1', checksum: 'checksum-trash', ack: 'asset-remote-1-11'), ]; await simulateEvents(events); @@ -486,4 +504,75 @@ void main() { verify(() => mockTrashedLocalAssetRepo.applyRestoredAssets(restoredIds)).called(1); }); }); + + group('SyncStreamService - Sync Migration', () { + test('ensure that <2.5.0 migrations run', () async { + await Store.put(StoreKey.syncMigrationStatus, "[]"); + when( + () => mockServerApi.getServerVersion(), + ).thenAnswer((_) async => ServerVersionResponseDto(major: 2, minor: 4, patch_: 1)); + + await sut.sync(); + + verifyInOrder([ + () => mockSyncApiRepo.deleteSyncAck([ + SyncEntityType.assetExifV1, + SyncEntityType.partnerAssetExifV1, + SyncEntityType.albumAssetExifCreateV1, + SyncEntityType.albumAssetExifUpdateV1, + ]), + () => mockSyncMigrationRepo.v20260128CopyExifWidthHeightToAsset(), + ]); + + // should only run on server >2.5.0 + verifyNever( + () => mockSyncApiRepo.deleteSyncAck([ + SyncEntityType.assetV1, + SyncEntityType.partnerAssetV1, + SyncEntityType.albumAssetCreateV1, + SyncEntityType.albumAssetUpdateV1, + ]), + ); + }); + test('ensure that >=2.5.0 migrations run', () async { + await Store.put(StoreKey.syncMigrationStatus, "[]"); + when( + () => mockServerApi.getServerVersion(), + ).thenAnswer((_) async => ServerVersionResponseDto(major: 2, minor: 5, patch_: 0)); + await sut.sync(); + + verifyInOrder([ + () => mockSyncApiRepo.deleteSyncAck([ + SyncEntityType.assetExifV1, + SyncEntityType.partnerAssetExifV1, + SyncEntityType.albumAssetExifCreateV1, + SyncEntityType.albumAssetExifUpdateV1, + ]), + () => mockSyncApiRepo.deleteSyncAck([ + SyncEntityType.assetV1, + SyncEntityType.partnerAssetV1, + SyncEntityType.albumAssetCreateV1, + SyncEntityType.albumAssetUpdateV1, + ]), + ]); + + // v20260128_ResetAssetV1 writes that v20260128_CopyExifWidthHeightToAsset has been completed + verifyNever(() => mockSyncMigrationRepo.v20260128CopyExifWidthHeightToAsset()); + }); + + test('ensure that migrations do not re-run', () async { + await Store.put( + StoreKey.syncMigrationStatus, + '["${SyncMigrationTask.v20260128_CopyExifWidthHeightToAsset.name}"]', + ); + + when( + () => mockServerApi.getServerVersion(), + ).thenAnswer((_) async => ServerVersionResponseDto(major: 2, minor: 4, patch_: 1)); + + await sut.sync(); + + verifyNever(() => mockSyncMigrationRepo.v20260128CopyExifWidthHeightToAsset()); + }); + }); } diff --git a/mobile/test/infrastructure/repository.mock.dart b/mobile/test/infrastructure/repository.mock.dart index aac384c29e..2d4af5b308 100644 --- a/mobile/test/infrastructure/repository.mock.dart +++ b/mobile/test/infrastructure/repository.mock.dart @@ -8,6 +8,7 @@ import 'package:immich_mobile/infrastructure/repositories/remote_asset.repositor import 'package:immich_mobile/infrastructure/repositories/storage.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/store.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_api.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/sync_migration.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_stream.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/trashed_local_asset.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/user.repository.dart'; @@ -46,6 +47,8 @@ class MockDriftBackupRepository extends Mock implements DriftBackupRepository {} class MockUploadRepository extends Mock implements UploadRepository {} +class MockSyncMigrationRepository extends Mock implements SyncMigrationRepository {} + // API Repos class MockUserApiRepository extends Mock implements UserApiRepository {}